[PATCH v5 06/12] media: mediatek: jpeg: refactor jpeg buffer payload setting

Kyrie Wu posted 12 patches 8 months, 2 weeks ago
There is a newer version of this series
[PATCH v5 06/12] media: mediatek: jpeg: refactor jpeg buffer payload setting
Posted by Kyrie Wu 8 months, 2 weeks ago
1. for multi-core jpegdec:
   core0: |<-------- decoding buffer0 and resolution changed to smaller
   core1:  |<-------- decoding buffer1
   core0:   |<- handling resolution changing
   core0:    |<- vb2_set_plane_payload
2. the payload size is changed on the step of set format. Because core1
is running and streaming has not been stopped, the format cannot be
set again, resulting in no change in the payload size.
3. at this time, the payload size is bigger than buffer length,
it will print a warnning call trace
4. set payload size must less than buffer length

Signed-off-by: Kyrie Wu <kyrie.wu@mediatek.com>
---
 .../platform/mediatek/jpeg/mtk_jpeg_core.c     | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
index 0074d1edb534..52d59bb5c9ad 100644
--- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
@@ -720,10 +720,22 @@ static int mtk_jpeg_buf_prepare(struct vb2_buffer *vb)
 		plane_fmt = q_data->pix_mp.plane_fmt[i];
 		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);
+			if (vb->planes[i].length > (plane_fmt.sizeimage +
+			    MTK_JPEG_MAX_EXIF_SIZE))
+				vb2_set_plane_payload(vb, i,
+						      plane_fmt.sizeimage +
+						      MTK_JPEG_MAX_EXIF_SIZE);
+			else
+				vb2_set_plane_payload(vb, i,
+						      vb->planes[i].length);
+
 		else
-			vb2_set_plane_payload(vb, i,  plane_fmt.sizeimage);
+			if (vb->planes[i].length > plane_fmt.sizeimage)
+				vb2_set_plane_payload(vb, i,
+						      plane_fmt.sizeimage);
+			else
+				vb2_set_plane_payload(vb, i,
+						      vb->planes[i].length);
 	}
 
 	return 0;
-- 
2.46.0
Re: [PATCH v5 06/12] media: mediatek: jpeg: refactor jpeg buffer payload setting
Posted by Nicolas Dufresne 8 months, 2 weeks ago
Hi Kyrie,

Le vendredi 30 mai 2025 à 15:45 +0800, Kyrie Wu a écrit :
> 1. for multi-core jpegdec:
>    core0: |<-------- decoding buffer0 and resolution changed to smaller
>    core1:  |<-------- decoding buffer1
>    core0:   |<- handling resolution changing
>    core0:    |<- vb2_set_plane_payload
> 2. the payload size is changed on the step of set format. Because core1
> is running and streaming has not been stopped, the format cannot be
> set again, resulting in no change in the payload size.
> 3. at this time, the payload size is bigger than buffer length,
> it will print a warnning call trace
> 4. set payload size must less than buffer length

You'll have to rework the text in this commit message, I must admit I don't
understand from this text what exactly the problem is, make it really hard to
review your solution.

> 
> Signed-off-by: Kyrie Wu <kyrie.wu@mediatek.com>
> ---
>  .../platform/mediatek/jpeg/mtk_jpeg_core.c     | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index 0074d1edb534..52d59bb5c9ad 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -720,10 +720,22 @@ static int mtk_jpeg_buf_prepare(struct vb2_buffer *vb)
>  		plane_fmt = q_data->pix_mp.plane_fmt[i];
>  		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);
> +			if (vb->planes[i].length > (plane_fmt.sizeimage +
> +			    MTK_JPEG_MAX_EXIF_SIZE))
> +				vb2_set_plane_payload(vb, i,
> +						      plane_fmt.sizeimage +
> +						      MTK_JPEG_MAX_EXIF_SIZE);
> +			else
> +				vb2_set_plane_payload(vb, i,
> +						      vb->planes[i].length);
> +
>  		else
> -			vb2_set_plane_payload(vb, i,  plane_fmt.sizeimage);
> +			if (vb->planes[i].length > plane_fmt.sizeimage)
> +				vb2_set_plane_payload(vb, i,
> +						      plane_fmt.sizeimage);
> +			else
> +				vb2_set_plane_payload(vb, i,
> +						      vb->planes[i].length);

Is this the same as ?

		unsigned long max_size = plane_fmt.sizeimage;

		if (ctx->enable_exif && 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));

It is unclear to me how this isn't just a workaround though, looking forward
your reworked commit message.

Nicolas

>  	}
>  
>  	return 0;
Re: [PATCH v5 06/12] media: mediatek: jpeg: refactor jpeg buffer payload setting
Posted by Kyrie Wu (吴晗) 8 months, 1 week ago
On Fri, 2025-05-30 at 13:33 -0400, Nicolas Dufresne wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hi Kyrie,
> 
> Le vendredi 30 mai 2025 à 15:45 +0800, Kyrie Wu a écrit :
> > 1. for multi-core jpegdec:
> >    core0: |<-------- decoding buffer0 and resolution changed to
> > smaller
> >    core1:  |<-------- decoding buffer1
> >    core0:   |<- handling resolution changing
> >    core0:    |<- vb2_set_plane_payload
> > 2. the payload size is changed on the step of set format. Because
> > core1
> > is running and streaming has not been stopped, the format cannot be
> > set again, resulting in no change in the payload size.
> > 3. at this time, the payload size is bigger than buffer length,
> > it will print a warnning call trace
> > 4. set payload size must less than buffer length
> 
> You'll have to rework the text in this commit message, I must admit I
> don't
> understand from this text what exactly the problem is, make it really
> hard to
> review your solution.

Dear Nicolas,

I'm terribly sorry for the inconvenience caused to you. I will change
the commit message and let it read easier.

Thanks.
> 
> > 
> > Signed-off-by: Kyrie Wu <kyrie.wu@mediatek.com>
> > ---
> >  .../platform/mediatek/jpeg/mtk_jpeg_core.c     | 18
> > +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > index 0074d1edb534..52d59bb5c9ad 100644
> > --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> > @@ -720,10 +720,22 @@ static int mtk_jpeg_buf_prepare(struct
> > vb2_buffer *vb)
> >               plane_fmt = q_data->pix_mp.plane_fmt[i];
> >               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)
> > ;
> > +                     if (vb->planes[i].length >
> > (plane_fmt.sizeimage +
> > +                         MTK_JPEG_MAX_EXIF_SIZE))
> > +                             vb2_set_plane_payload(vb, i,
> > +                                                   plane_fmt.sizei
> > mage +
> > +                                                   MTK_JPEG_MAX_EX
> > IF_SIZE);
> > +                     else
> > +                             vb2_set_plane_payload(vb, i,
> > +                                                   vb-
> > >planes[i].length);
> > +
> >               else
> > -                     vb2_set_plane_payload(vb,
> > i,  plane_fmt.sizeimage);
> > +                     if (vb->planes[i].length >
> > plane_fmt.sizeimage)
> > +                             vb2_set_plane_payload(vb, i,
> > +                                                   plane_fmt.sizei
> > mage);
> > +                     else
> > +                             vb2_set_plane_payload(vb, i,
> > +                                                   vb-
> > >planes[i].length);
> 
> Is this the same as ?
> 
>                 unsigned long max_size = plane_fmt.sizeimage;
> 
>                 if (ctx->enable_exif && 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));
> 
> It is unclear to me how this isn't just a workaround though, looking
> forward
> your reworked commit message.
> 
> Nicolas

Yes, Thanks for you suggestion. I will fix it in the next version.

Regards,
Kyrie.
> 
> >       }
> > 
> >       return 0;