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

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

On Tue, 2025-12-16 at 16:15 -0500, Nicolas Dufresne wrote:
> 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

Dear Nicolas,

The value, max_size = plane_fmt.sizeimage, recorded the current size,
but if the resolution changed to a smaller one, the sizeimage will
bigger
than the vb->planes[i].length, in the former sw flow, the bigger value
will set to payload size and caused a warnning call trace printed.

Here are the answers of your questions:
1. The enable_exif is a
determined value, which is set in s_ctrl before 
jpeg decoder/encoder starting.

2. If enable_exif is set as true, the buffer planes length is equal to
sizeimage + MTK_JPEG_MAX_EXIF_SIZE.

3. The reason, Why isn't the true payload size written once,
is due to this setting is used to avoid the condition descripted in
commit message. And it will set the true value in irq handler.

Thanks.

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