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

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

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