Re: [PATCH v17 05/12] media: mediatek: jpeg: Fix multi-core clk suspend and resume setting

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 9/22/26 11:15, Kyrie Wu wrote:
> The suspend/resume callback function is defined in the dev_pm_ops
> structure, which is defined in platform_driver. For multiple-core
> architecture, each hardware driver will register a platform_driver
> structure, so it is necessary to add a suspend/resume callback
> function for each hardware to support this operation.
> 
> Fixes: 934e8bccac95 ("mtk-jpegenc: support jpegenc multi-hardware")
> Fixes: 0fa49df4222f ("media: mtk-jpegdec: support jpegdec multi-hardware")
> Signed-off-by: Kyrie Wu <kyrie.wu@mediatek.com>
> ---
>   .../platform/mediatek/jpeg/mtk_jpeg_core.c    | 28 +++-------
>   .../platform/mediatek/jpeg/mtk_jpeg_dec_hw.c  | 55 +++++++++++++++++--
>   .../platform/mediatek/jpeg/mtk_jpeg_enc_hw.c  | 53 ++++++++++++++++--
>   3 files changed, 107 insertions(+), 29 deletions(-)
> 

..snip..

> @@ -673,11 +669,58 @@ static int mtk_jpegdec_hw_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static int mtk_jpegdec_pm_suspend(struct device *dev)
> +{
> +	struct mtk_jpegdec_comp_dev *jpeg = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable_unprepare(jpeg->jdec_clk.clk_num,
> +				   jpeg->jdec_clk.clks);
> +
> +	return 0;
> +}
> +
> +static int mtk_jpegdec_pm_resume(struct device *dev)
> +{
> +	struct mtk_jpegdec_comp_dev *jpeg = dev_get_drvdata(dev);
> +
> +	return clk_bulk_prepare_enable(jpeg->jdec_clk.clk_num,
> +				       jpeg->jdec_clk.clks);
> +}
> +
> +static int mtk_jpegdec_suspend(struct device *dev)
> +{
> +	struct mtk_jpegdec_comp_dev *jpeg = dev_get_drvdata(dev);
> +
> +	v4l2_m2m_suspend(jpeg->master_dev->m2m_dev);
> +
> +	return pm_runtime_force_suspend(dev);

Uhm, are you validating that the JPEG decoder IP is actually idle, or that
the job actually stopped, before forcing clk-off suspend?

I don't see that done anywhere... am I missing anything?

Also, can't we unify single and multicore suspend strategies?
That'd be nice for maintainability.

Cheers,
Angelo

> +}
> +
> +static int mtk_jpegdec_resume(struct device *dev)
> +{
> +	struct mtk_jpegdec_comp_dev *jpeg = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	v4l2_m2m_resume(jpeg->master_dev->m2m_dev);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_jpegdec_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(mtk_jpegdec_suspend, mtk_jpegdec_resume)
> +	RUNTIME_PM_OPS(mtk_jpegdec_pm_suspend, mtk_jpegdec_pm_resume, NULL)
> +};
> +
>   static struct platform_driver mtk_jpegdec_hw_driver = {
>   	.probe = mtk_jpegdec_hw_probe,
>   	.driver = {
>   		.name = "mtk-jpegdec-hw",
>   		.of_match_table = mtk_jpegdec_hw_ids,
> +		.pm             = &mtk_jpegdec_pm_ops,
>   	},
>   };
>   
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> index 44a064dc7..4a8559c35 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> @@ -263,13 +263,12 @@ static void mtk_jpegenc_timeout_work(struct work_struct *work)
>   	v4l2_m2m_buf_copy_metadata(src_buf, dst_buf);
>   
>   	mtk_jpeg_enc_reset(cjpeg->reg_base);
> -	clk_disable_unprepare(cjpeg->venc_clk.clks->clk);
> -	pm_runtime_put(cjpeg->dev);
>   	cjpeg->hw_state = MTK_JPEG_HW_IDLE;
>   	atomic_inc(&master_jpeg->hw_rdy);
>   	wake_up(&master_jpeg->hw_wq);
>   	v4l2_m2m_buf_done(src_buf, buf_state);
>   	mtk_jpegenc_put_buf(cjpeg);
> +	pm_runtime_put(cjpeg->dev);
>   }
>   
>   static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> @@ -303,12 +302,11 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
>   	buf_state = VB2_BUF_STATE_DONE;
>   	v4l2_m2m_buf_done(src_buf, buf_state);
>   	mtk_jpegenc_put_buf(jpeg);
> -	pm_runtime_put(ctx->jpeg->dev);
> -	clk_disable_unprepare(jpeg->venc_clk.clks->clk);
>   
>   	jpeg->hw_state = MTK_JPEG_HW_IDLE;
>   	wake_up(&master_jpeg->hw_wq);
>   	atomic_inc(&master_jpeg->hw_rdy);
> +	pm_runtime_put(jpeg->dev);
>   
>   	return IRQ_HANDLED;
>   }
> @@ -394,11 +392,58 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static int mtk_jpegenc_pm_suspend(struct device *dev)
> +{
> +	struct mtk_jpegenc_comp_dev *jpeg = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable_unprepare(jpeg->venc_clk.clk_num,
> +				   jpeg->venc_clk.clks);
> +
> +	return 0;
> +}
> +
> +static int mtk_jpegenc_pm_resume(struct device *dev)
> +{
> +	struct mtk_jpegenc_comp_dev *jpeg = dev_get_drvdata(dev);
> +
> +	return clk_bulk_prepare_enable(jpeg->venc_clk.clk_num,
> +				       jpeg->venc_clk.clks);
> +}
> +
> +static int mtk_jpegenc_suspend(struct device *dev)
> +{
> +	struct mtk_jpegenc_comp_dev *jpeg = dev_get_drvdata(dev);
> +
> +	v4l2_m2m_suspend(jpeg->master_dev->m2m_dev);
> +
> +	return pm_runtime_force_suspend(dev);
> +}
> +
> +static int mtk_jpegenc_resume(struct device *dev)
> +{
> +	struct mtk_jpegenc_comp_dev *jpeg = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	v4l2_m2m_resume(jpeg->master_dev->m2m_dev);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_jpegenc_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(mtk_jpegenc_suspend, mtk_jpegenc_resume)
> +	RUNTIME_PM_OPS(mtk_jpegenc_pm_suspend, mtk_jpegenc_pm_resume, NULL)
> +};
> +
>   static struct platform_driver mtk_jpegenc_hw_driver = {
>   	.probe = mtk_jpegenc_hw_probe,
>   	.driver = {
>   		.name = "mtk-jpegenc-hw",
>   		.of_match_table = mtk_jpegenc_drv_ids,
> +		.pm = &mtk_jpegenc_pm_ops,
>   	},
>   };
>