Re: [PATCH V6, 3/5] media: mtk-jpegenc: add jpegenc timeout func interface

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: "kyrie.wu" <kyrie.wu@mediatek.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Tomasz Figa <tfiga@chromium.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Tzung-Bi Shih <tzungbi@chromium.org>
Cc: Project_Global_Chrome_Upstream_Group@mediatek.com,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, xia.jiang@mediatek.com,
	maoguang.meng@mediatek.com, srv_heupstream@mediatek.com,
	irui.wang@mediatek.com
Subject: Re: [PATCH V6, 3/5] media: mtk-jpegenc: add jpegenc timeout func interface
Date: Mon, 7 Feb 2022 15:50:36 +0100	[thread overview]
Message-ID: <d91f056c-227f-7a01-9e9c-9b4d53ec04f4@collabora.com> (raw)
In-Reply-To: <1638501230-13417-4-git-send-email-kyrie.wu@mediatek.com>

Il 03/12/21 04:13, kyrie.wu ha scritto:
> Generalizes jpegenc timeout func interfaces to handle HW timeout.
> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> ---
> V6: no change
> ---
>   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  8 ++++++++
>   drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23 +++++++++++++++++++++++
>   2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> index 7d013de..baab305 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -75,6 +75,12 @@ struct mtk_jpeg_variant {
>   	u32 cap_q_default_fourcc;
>   };
>   
> +struct mtk_jpeg_hw_param {
> +	struct vb2_v4l2_buffer *src_buffer;
> +	struct vb2_v4l2_buffer *dst_buffer;
> +	struct mtk_jpeg_ctx *curr_ctx;
> +};
> +
>   enum mtk_jpegenc_hw_id {
>   	MTK_JPEGENC_HW0,
>   	MTK_JPEGENC_HW1,
> @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev {
>   	struct mtk_jpeg_dev *master_dev;
>   	struct mtk_jpegenc_pm pm;
>   	int jpegenc_irq;
> +	struct delayed_work job_timeout_work;
> +	struct mtk_jpeg_hw_param hw_param;
>   };
>   
>   /**
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> index 4ccda1d..e62b875 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx,  void __iomem *base)
>   	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
>   }
>   
> +static void mtk_jpegenc_timeout_work(struct work_struct *work)
> +{
> +	struct delayed_work *Pwork =
> +		container_of(work, struct delayed_work, work);
> +	struct mtk_jpegenc_comp_dev *cjpeg =
> +		container_of(Pwork, struct mtk_jpegenc_comp_dev,
> +		job_timeout_work);
> +	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> +	struct vb2_v4l2_buffer *src_buf;
> +
> +	src_buf = cjpeg->hw_param.src_buffer;
> +
> +	mtk_jpeg_enc_reset(cjpeg->reg_base);
> +	clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info->jpegenc_clk);

You disable and unprepare the clock, but you never turn it back on?

This will lead to unbalanced disable on module removal but, more importantly,
will lead to unclocked access after a timeout, for which the platform may or
may not crash, but the jpgenc hardware will be unrecoverable until performing
a full system reboot.

Please fix this.

> +	pm_runtime_put(cjpeg->pm.dev);
> +	v4l2_m2m_buf_done(src_buf, buf_state);
> +}
> +
>   static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
>   {
>   	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> @@ -194,6 +212,8 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
>   	struct mtk_jpegenc_comp_dev *jpeg = priv;
>   	struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev;
>   
> +	cancel_delayed_work(&jpeg->job_timeout_work);
> +
>   	irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) &
>   		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
>   	if (irq_status)
> @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>   
>   	dev->plat_dev = pdev;
>   
> +	INIT_DELAYED_WORK(&dev->job_timeout_work,
> +		mtk_jpegenc_timeout_work);
> +
>   	ret = mtk_jpegenc_init_pm(dev);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Failed to get jpeg enc clock source");

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek