Re: [PATCH v15 01/12] media: mediatek: jpeg: fix jpeg cores' amounts setting

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 7486 bytes --]

Hi,

Le lundi 13 juillet 2026 à 03:30 +0000, Kyrie Wu (吴晗) a écrit :
> On Sat, 2026-07-11 at 17:43 -0400, Nicolas Dufresne wrote:
> > Hi,
> > 
> > Le jeudi 02 juillet 2026 à 15:26 +0800, Kyrie Wu a écrit :
> > > Different ICs have different amounts of cores,
> > > use a variable to set the cores' amounts.
> > > 
> > > 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>
> > > Reviewed-by: AngeloGioacchino Del Regno
> > > <angelogioacchino.delregno@collabora.com>
> > > ---
> > >  drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c   | 8 ++++---
> > > -
> > >  drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h   | 2 ++
> > >  drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c | 1 +
> > >  drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c | 1 +
> > >  4 files changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > > b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > > index d147ec483081..1e014fd698ae 100644
> > > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > > @@ -1462,7 +1462,7 @@ static int mtk_jpegenc_get_hw(struct
> > > mtk_jpeg_ctx *ctx)
> > >  	int i;
> > >  
> > >  	spin_lock_irqsave(&jpeg->hw_lock, flags);
> > > -	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> > > +	for (i = 0; i < jpeg->max_hw_count; i++) {
> > >  		comp_jpeg = jpeg->enc_hw_dev[i];
> > >  		if (comp_jpeg->hw_state == MTK_JPEG_HW_IDLE) {
> > >  			hw_id = i;
> > > @@ -1509,7 +1509,7 @@ static int mtk_jpegdec_get_hw(struct
> > > mtk_jpeg_ctx *ctx)
> > >  	int i;
> > >  
> > >  	spin_lock_irqsave(&jpeg->hw_lock, flags);
> > > -	for (i = 0; i < MTK_JPEGDEC_HW_MAX; i++) {
> > > +	for (i = 0; i < jpeg->max_hw_count; i++) {
> > >  		comp_jpeg = jpeg->dec_hw_dev[i];
> > >  		if (comp_jpeg->hw_state == MTK_JPEG_HW_IDLE) {
> > >  			hw_id = i;
> > > @@ -1592,7 +1592,7 @@ static void mtk_jpegenc_worker(struct
> > > work_struct *work)
> > >  		jpeg_work);
> > >  	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > >  
> > > -	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++)
> > > +	for (i = 0; i < jpeg->max_hw_count; i++)
> > >  		comp_jpeg[i] = jpeg->enc_hw_dev[i];
> > >  	i = 0;
> > >  
> > > @@ -1687,7 +1687,7 @@ static void mtk_jpegdec_worker(struct
> > > work_struct *work)
> > >  	struct mtk_jpeg_fb fb;
> > >  	unsigned long flags;
> > >  
> > > -	for (i = 0; i < MTK_JPEGDEC_HW_MAX; i++)
> > > +	for (i = 0; i < jpeg->max_hw_count; i++)
> > >  		comp_jpeg[i] = jpeg->dec_hw_dev[i];
> > >  	i = 0;
> > >  
> > > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > > b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > > index 02ed0ed5b736..6be5cf30dea1 100644
> > > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.h
> > > @@ -212,6 +212,7 @@ struct mtk_jpegdec_comp_dev {
> > >   * @reg_decbase:	jpg decode register base addr
> > >   * @dec_hw_dev:	jpg decode hardware device
> > >   * @hw_index:		jpg hw index
> > > + * @max_hw_count:	jpeg hw-core count
> > >   */
> > >  struct mtk_jpeg_dev {
> > >  	struct mutex		lock;
> > > @@ -234,6 +235,7 @@ struct mtk_jpeg_dev {
> > >  	void __iomem *reg_decbase[MTK_JPEGDEC_HW_MAX];
> > >  	struct mtk_jpegdec_comp_dev
> > > *dec_hw_dev[MTK_JPEGDEC_HW_MAX];
> > >  	atomic_t hw_index;
> > > +	u32 max_hw_count;
> > >  };
> > >  
> > >  /**
> > > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> > > b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> > > index 32372781daf5..4534caeb104f 100644
> > > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> > > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> > > @@ -664,6 +664,7 @@ static int mtk_jpegdec_hw_probe(struct
> > > platform_device *pdev)
> > >  	master_dev->dec_hw_dev[i] = dev;
> > >  	master_dev->reg_decbase[i] = dev->reg_base;
> > >  	dev->master_dev = master_dev;
> > > +	master_dev->max_hw_count++;
> > 
> > Just one question, can the mtk_jpegenc_worker() starts before all
> > cores have
> > been probed ?
> > 
> > Nicolas
> 
> Dear Nicolas,
> 
> Thanks for the question.
> 
> mtk_jpegenc_worker() is not started from the probe path. For the
> multicore encoder, the worker is only initialized when userspace
> opens the video node:
>     mtk_jpeg_open()
>       INIT_WORK(&ctx->jpeg_work, jpeg->variant->jpeg_worker)
> 
> It is then queued only through the V4L2 mem2mem run path:
>     userspace QBUF/STREAMON
>       -> v4l2_m2m_try_schedule()
>       -> .device_run = mtk_jpeg_multicore_enc_device_run()
>       -> queue_work(jpeg->workqueue, &ctx->jpeg_work)
>       -> mtk_jpegenc_worker()
> 
> So the worker can run only after the video device has been registered 
> and after a userspace application opens the device and starts
> streaming buffers.
> 
> The JPEG hw core probe is part of the platform device probe flow during
> system boot. The userspace open/streaming path happens later, after the
> system has booted and the video node is available. Therefore we do not
> have a case where mtk_jpegenc_worker() starts before the JPEG hw probe
> has completed.

When I check the core implementation though, the video device is registered and
ready to open before childs driver, so even if small, there is gap where you
could race between probes and open.

I think I'd like to suggest a different approach, which to me is easier to
understand (and so should make it easier to maintain). In mtk_jpeg_probe(), we
already compute num_child. We could immediately set max_hw_count to its expected
value. That would replace this patch entirely. We should add some bug on if that
number is bigger then MTK_JPEGDEC_HW_MAX (bound checks, see shashiko report).

Then, in follow up patch, to fix the minor race I mention, we could avoid
setting hw_rdy counter to num_child immediately, and increment that at child
probe time (where you increase max_hw_count in this patch). The worker will then
wait properly for child driver to have probed (instead of spinning the
device_run() by calling v4l2_m2m_job_finish() when the max retry is reached.).

hope this make sense, at least to me, having a max value that cannot increase
sounds more logical.

Nicolas

> 
> Regards,
> Kyrie.
> > 
> > >  
> > >  	platform_set_drvdata(pdev, dev);
> > >  	pm_runtime_enable(&pdev->dev);
> > > 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 b6f5b2249f1f..2765dafab4ad 100644
> > > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> > > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> > > @@ -386,6 +386,7 @@ static int mtk_jpegenc_hw_probe(struct
> > > platform_device *pdev)
> > >  	master_dev->enc_hw_dev[i] = dev;
> > >  	master_dev->reg_encbase[i] = dev->reg_base;
> > >  	dev->master_dev = master_dev;
> > > +	master_dev->max_hw_count++;
> > >  
> > >  	platform_set_drvdata(pdev, dev);
> > >  	pm_runtime_enable(&pdev->dev);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]