Re: [PATCH] media: chips-media: wave5: Account for sizeimage 0 case

[Brandon Brnich]

Hi Nicolas,

Thanks for the quick review.

On 10/19/26 13:46, Nicolas Dufresne wrote:
> Hi,
>
> Le lundi 19 octobre 2026 à 11:00 -0500, Brandon Brnich a écrit :
> > V4L2 uAPI states that sizeimage can be set to 0 in struct
> > v4l2_pix_format_mplane if client wants the driver to configure a proper
> > sizeimage. If client leaves this field unset and configures an encode with
> > high bitrate, low QP, or any other parameters resulting in high quality
> > encode, then the driver-calculated bitstream buffer size (MIN_BS_SIZE) will
> > be too small resulting in undetermined behavior.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 88ff31fd0181 ("media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.")
> > Signed-off-by: Brandon Brnich <b-brnich@xxxxxx>
> > ---
> >
> > Current DEFAULT_BS_SIZE is just a placeholder value - I can lower if
> > required so memory consumption doesn't go up drastically if sizeimage is
> > set 0 by user. I saw this was the common equation in rockchip and hantro
> > driver.
> >
> > Link to uAPI requirement: https://docs.kernel.org/userspace-api/media/v4l/pixfmt-v4l2-mplane.html
> >
> >   .../media/platform/chips-media/wave5/wave5-helper.c  | 12 +++++++++---
> >   1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > index c3d34be833ff0..384a0a9aac3ae 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > @@ -7,7 +7,10 @@
> >   
> >   #include "wave5-helper.h"
> >   
> > -#define DEFAULT_BS_SIZE(width, height) ((width) * (height) / 8 * 3)
> > +/* Smallest bitstream buffer accepted when the client provides a sizeimage */
> > +#define MIN_BS_SIZE(width, height) ((width) * (height) * 3 / 8)
>
> Does that change put us at risk of integer overflow ? I just don't remember the
> max width/height, but please do that math to be sure.

From wave5-vpuconfig.h:

Decoder supports 8192x4320
Encoder supports 8192x8192

So worst case is 8192x8192x3 = 201326592 before divide by 8.

No risk for integer overflow here.

> > +/* Worst case: Allocate bitstream buffer size based on resolution and max depth */
> > +#define DEFAULT_BS_SIZE(width, height) ((width) * (height) * 2)
>
> Should be adjust based on the sub-sampling and depth? We could use 12 and 15 bit
> for YUV420, and then 16 (what you do now) and 20 for YUV422. Though, as far I
> remember, 10bit has not been included as the TI hardware it was tested on didn't
> have it.

Looking at the driver, it looks like 10-bit is supported by the Wave515 
variant. But as you mentioned no TI product has Wave5 variant with 
10-bit so I can't validate :(.

I can update the function to account for worst case of 20bpp if you 
think that is more appropriate. Memory consumption will already increase 
if client leaves sizeimage 0 - might be worth allocating for actual 
worst case here.

> >   
> >   const char *state_to_str(enum vpu_instance_state state)
> >   {
> > @@ -261,8 +264,11 @@ void wave5_update_pix_fmt(struct v4l2_pix_format_mplane *pix_mp,
> >   		pix_mp->height = height;
> >   		pix_mp->num_planes = 1;
> >   		pix_mp->plane_fmt[0].bytesperline = 0;
> > -		pix_mp->plane_fmt[0].sizeimage = max(DEFAULT_BS_SIZE(width, height),
> > -						     pix_mp->plane_fmt[0].sizeimage);
> > +		if (!pix_mp->plane_fmt[0].sizeimage)
> > +			pix_mp->plane_fmt[0].sizeimage = DEFAULT_BS_SIZE(width, height);
> > +		else
> > +			pix_mp->plane_fmt[0].sizeimage = max(MIN_BS_SIZE(width, height),
> > +							     pix_mp->plane_fmt[0].sizeimage);
> >   	} else {
> >   		v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat, width, height);
> >   	}
>
> This is overall an improvement for userspace that is not picking a size, so in
> any case:
>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
>
> regards,
> Nicolas

Best,
Brandon