Re: [PATCH v11 02/12] media: mediatek: jpeg: fix jpeg buffer payload setting

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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

Hi.

Le mardi 02 décembre 2025 à 17:47 +0800, Kyrie Wu a écrit :
> For multi-core jpegdec, if one of hws gets the event of resolution
> changing, the payload size, representing the size of Y/C data,

-> "gets the resolution change event, ..."

> needed to change. But others hws are decoding at the same time and

needed -> needs,               hws -> cores ?

It this specific to decoders or any type of cores ?

> it can not be changed immediately, which results that the payload

                                                    in the playload

> size is not equal to the real buffer length of the hw's, which occurred

size to not match the real buffer lenght for the ... hw ?

> resolution changing and a warnning call trace will print.

Can't parse. You can probably split that large sentence, and it needs to be
rework.

> So the setting of payload size must less than the real buffer length
> to remove the warnning logs.
> 
> Fixes: 0fa49df4222f ("media: mtk-jpegdec: support jpegdec multi-hardware")
> 

Don't add blank line in tags please.

> Signed-off-by: Kyrie Wu <kyrie.wu@mediatek.com>
> ---
>  .../platform/mediatek/jpeg/mtk_jpeg_core.c    | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 6a7e01130f1c..0cf3dc5407e4 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -709,6 +709,7 @@ static int mtk_jpeg_buf_prepare(struct vb2_buffer *vb)
>  	struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
>  	struct mtk_jpeg_q_data *q_data = NULL;
>  	struct v4l2_plane_pix_format plane_fmt = {};
> +	unsigned long max_size;

size_t ?

>  	int i;
>  
>  	q_data = mtk_jpeg_get_q_data(ctx, vb->vb2_queue->type);
> @@ -717,12 +718,20 @@ static int mtk_jpeg_buf_prepare(struct vb2_buffer *vb)
>  
>  	for (i = 0; i < q_data->fmt->colplanes; i++) {
>  		plane_fmt = q_data->pix_mp.plane_fmt[i];
> +		max_size = plane_fmt.sizeimage;
> +
>  		if (ctx->enable_exif &&
> -		    q_data->fmt->fourcc == V4L2_PIX_FMT_JPEG)
> -			vb2_set_plane_payload(vb, i, plane_fmt.sizeimage +
> -					      MTK_JPEG_MAX_EXIF_SIZE);
> -		else
> -			vb2_set_plane_payload(vb, i,  plane_fmt.sizeimage);
> +			q_data->fmt->fourcc == V4L2_PIX_FMT_JPEG) {
> +			max_size += MTK_JPEG_MAX_EXIF_SIZE;
> +
> +			vb2_set_plane_payload(vb, i,
> +					      MIN(vb->planes[i].length,
> +						  max_size));

This is still not quite right. sizeimage, straight from s_fmt should already
account for the EXIF headers. If enable_exif is unknown at the moment, then you
should just always include that space. This way, the buffer length will only be
bigger if userspace asked for more at allocation time, and wil never be smaller
then this.

The point of sizeimage it to allow userspace allocate externally the right size,
this driver failed at that task it seems.

Stepping back a little, I don't even understand why you set the payload size
like this here. Why isn't the true payload size written once, when the buffer is
encoded ? Are you using bytesused to tell the HW how much space can be written
too ? Just give it the full space, wich is the allocated size (length).

Nicolas


> +		} else {
> +			vb2_set_plane_payload(vb, i,
> +					      MIN(plane_fmt.sizeimage,
> +						  vb->planes[i].length));
> +		}
>  	}
>  
>  	return 0;

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