Re: [PATCH v15 06/12] media: mediatek: jpeg: fix decoding buffer number setting timing issue

[Nicolas Dufresne]

Le jeudi 02 juillet 2026 à 15:26 +0800, Kyrie Wu a écrit :
> The src buffer doesn't need set information and dst buf parameters
> only need to set when the power set succussed and protect the

Can you rework this, I'm not sure I understand what you are trying to say.

> setting by spinlock ensuring that any later operations acting
> on this buffer reflect accurate state and frame data.
> 
> Fixes: dedc21500334 ("media: mtk-jpegdec: add jpeg decode worker interface")
> Signed-off-by: Kyrie Wu <kyrie.wu@xxxxxxxxxxxx>
> ---
>  drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c   | 9 +++------
>  drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c | 1 +
>  drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c | 1 +
>  3 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 89048aba8dca..4dc574e03bd5 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -1734,7 +1734,6 @@ static void mtk_jpegdec_worker(struct work_struct *work)
>  
>  	v4l2_m2m_buf_copy_metadata(src_buf, dst_buf);
>  	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
> -	jpeg_dst_buf = mtk_jpeg_vb2_to_srcbuf(&dst_buf->vb2_buf);
>  
>  	if (mtk_jpeg_check_resolution_change(ctx,
>  					     &jpeg_src_buf->dec_param)) {
> @@ -1743,11 +1742,6 @@ static void mtk_jpegdec_worker(struct work_struct *work)
>  		goto getbuf_fail;
>  	}
>  
> -	jpeg_src_buf->curr_ctx = ctx;
> -	jpeg_src_buf->frame_num = ctx->total_frame_num;
> -	jpeg_dst_buf->curr_ctx = ctx;
> -	jpeg_dst_buf->frame_num = ctx->total_frame_num;
> -
>  	mtk_jpegdec_set_hw_param(ctx, hw_id, src_buf, dst_buf);
>  	ret = pm_runtime_resume_and_get(comp_jpeg[hw_id]->dev);
>  	if (ret < 0) {
> @@ -1772,6 +1766,9 @@ static void mtk_jpegdec_worker(struct work_struct *work)
>  			      msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));
>  
>  	spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags);

I didn't dig very deep, but in extreme case, the timeout worker (hidden above)
could be called concurrently to the remaining of this code, which gives me the
impression everything would be left in a unstable state since that spinlock is
not being held by the timeout worker. Perhaps something to improve further ?

This is a step in the right direction for sure, so for this patch:

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>

> +	jpeg_dst_buf = mtk_jpeg_vb2_to_srcbuf(&dst_buf->vb2_buf);
> +	jpeg_dst_buf->curr_ctx = ctx;
> +	jpeg_dst_buf->frame_num = ctx->total_frame_num;
>  	ctx->total_frame_num++;
>  	mtk_jpeg_dec_reset(comp_jpeg[hw_id]->reg_base);
>  	mtk_jpeg_dec_set_config(comp_jpeg[hw_id]->reg_base,
> 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 9a8dbca6af00..e4d2c5d4ec73 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> @@ -513,6 +513,7 @@ static void mtk_jpegdec_put_buf(struct mtk_jpegdec_comp_dev *jpeg)
>  				v4l2_m2m_buf_done(&tmp_dst_done_buf->b,
>  						  VB2_BUF_STATE_DONE);
>  				ctx->last_done_frame_num++;
> +				break;
>  			}
>  		}
>  	}
> 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 5d1c217fea0f..2adea3aca50b 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> @@ -242,6 +242,7 @@ static void mtk_jpegenc_put_buf(struct mtk_jpegenc_comp_dev *jpeg)
>  				v4l2_m2m_buf_done(&tmp_dst_done_buf->b,
>  						  VB2_BUF_STATE_DONE);
>  				ctx->last_done_frame_num++;
> +				break;
>  			}
>  		}
>  	}

Attachment: signature.asc
Description: This is a digitally signed message part