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

["Kyrie Wu (吴晗)" <Kyrie.Wu@mediatek.com>]

On Tue, 2026-09-22 at 13:39 +0200, AngeloGioacchino Del Regno wrote:
> 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
> 
Dear Angelo,

The suspend/resume callbacks added here are for the multi-hw component
devices.
Each JPEG core is a separate platform device and owns its clocks, while
the single-hw callbacks remain on the master device.
For that reason, I think keeping these callbacks in the component
drivers is clearer.

For system suspend, v4l2_m2m_suspend() stops new jobs and waits for the
running job to
finish before pm_runtime_force_suspend() can disable the component
clocks. For runtime PM,
each active job holds a runtime-PM reference until the IRQ or timeout
path marks the hardware idle and drops the reference.
So the clocks are not disabled while a job is running.

I’ll clarify this ordering in the next revision.

Reagrds,
Kyrie

> > +}
> > +
> > +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,
> >   	},
> >   };
> >