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

[Brandon Brnich <b-brnich@ti.com>]

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@vger.kernel.org
>> Fixes: 88ff31fd0181 ("media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.")
>> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
>> ---
>>
>> 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@collabora.com>
> 
> regards,
> Nicolas

Best,
Brandon