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

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

[Brandon Brnich]

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)
+/* Worst case: Allocate bitstream buffer size based on resolution and max depth */
+#define DEFAULT_BS_SIZE(width, height) ((width) * (height) * 2)
 
 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);
 	}
-- 
2.43.0

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

[Nicolas Dufresne]

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

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.

> +/* 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.

>  
>  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

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

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@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