Re: [PATCH v6 03/16] media: v4l2-common: Support custom imagesize in fill_pixfmt()

From: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
To: Ezequiel Garcia <ezequiel@collabora.com>
Cc: linux-media@vger.kernel.org,
	Hans Verkuil <hans.verkuil@cisco.com>,
	kernel@collabora.com,
	Nicolas Dufresne <nicolas.dufresne@collabora.com>,
	Tomasz Figa <tfiga@chromium.org>,
	linux-rockchip@lists.infradead.org,
	Heiko Stuebner <heiko@sntech.de>, Jonas Karlman <jonas@kwiboo.se>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Boris Brezillon <boris.brezillon@collabora.com>
Subject: Re: [PATCH v6 03/16] media: v4l2-common: Support custom imagesize in fill_pixfmt()
Date: Wed, 29 May 2019 08:28:09 -0300	[thread overview]
Message-ID: <20190529082809.0b9f3553@coco.lan> (raw)
In-Reply-To: <20190528170232.2091-4-ezequiel@collabora.com>

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <ezequiel@collabora.com> escreveu:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Users can define custom sizeimage as long as they're big enough to
> store the amount of pixels required for a specific width/height under a
> specific format. Avoid overriding those fields in this case.
> 
> We could possibly do the same for bytesperline, but it gets tricky when
> dealing with !MPLANE definitions, so this case is omitted for now and
> ->bytesperline is always overwritten with the value calculated in
> fill_pixfmt().
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes from v5:
> * Overwrite bytesperline with the value calculated in fill_pixfmt()
> 
> Changes from v4:
> * New patch
> 
>  drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
>  1 file changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index b2d1e55d9561..fd286f6e17d7 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
>  	pixfmt->num_planes = info->mem_planes;
>  
>  	if (info->mem_planes == 1) {
> +		u32 sizeimage = 0;
> +
>  		plane = &pixfmt->plane_fmt[0];
> -		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> -		plane->sizeimage = 0;
>  
>  		for (i = 0; i < info->comp_planes; i++) {
>  			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> @@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
>  			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
>  			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
>  
> -			plane->sizeimage += info->bpp[i] *
> -				DIV_ROUND_UP(aligned_width, hdiv) *
> -				DIV_ROUND_UP(aligned_height, vdiv);
> +			sizeimage += info->bpp[i] *
> +				     DIV_ROUND_UP(aligned_width, hdiv) *
> +				     DIV_ROUND_UP(aligned_height, vdiv);
>  		}
> +
> +		/* Custom bytesperline value is not supported yet. */
> +		plane->bytesperline = ALIGN(width,
> +					    v4l2_format_block_width(info, 0)) *
> +				      info->bpp[0];
> +
> +		/*
> +		 * The user might have specified a custom sizeimage, only
> +		 * override it if it's not big enough.
> +		 */
> +		plane->sizeimage = max(sizeimage, plane->sizeimage);

No upper limit? That doesn't sound a good idea to me, specially since some
(broken) app might not be memset the format to zero before filling the ioctl
structure.

Perhaps we could do something like:

		sizeimage = min (sizeimage, 2 * plane->sizeimage)

or something similar that would be reasonable.

>  	} else {
>  		for (i = 0; i < info->comp_planes; i++) {
>  			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> @@ -613,10 +624,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
>  			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
>  
>  			plane = &pixfmt->plane_fmt[i];
> -			plane->bytesperline =
> -				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
> -			plane->sizeimage =
> -				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);

> +
> +			/* Custom bytesperline value is not supported yet. */

Supporting custom bytesperline seems too risky of breaking apps. 
So, I would drop this comment.

> +			plane->bytesperline = info->bpp[i] *
> +					      DIV_ROUND_UP(aligned_width, hdiv);

> +
> +			/*
> +			 * The user might have specified a custom sizeimage,
> +			 * only override it if it's not big enough.
> +			 */
> +			plane->sizeimage = max_t(u32,
> +						 plane->bytesperline *
> +						 DIV_ROUND_UP(aligned_height, vdiv),
> +						 plane->sizeimage);
>  		}
>  	}
>  	return 0;
> @@ -627,6 +647,7 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  		     u32 width, u32 height)
>  {
>  	const struct v4l2_format_info *info;
> +	u32 sizeimage = 0;
>  	int i;
>  
>  	info = v4l2_format_info(pixelformat);
> @@ -640,8 +661,6 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  	pixfmt->width = width;
>  	pixfmt->height = height;
>  	pixfmt->pixelformat = pixelformat;
> -	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> -	pixfmt->sizeimage = 0;
>  
>  	for (i = 0; i < info->comp_planes; i++) {
>  		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> @@ -651,11 +670,20 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  
>  		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
>  		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> -
> -		pixfmt->sizeimage += info->bpp[i] *
> -			DIV_ROUND_UP(aligned_width, hdiv) *
> -			DIV_ROUND_UP(aligned_height, vdiv);
> +		sizeimage += info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) *
> +			     DIV_ROUND_UP(aligned_height, vdiv);
>  	}
> +
> +	/* Custom bytesperline value is not supported yet. */
> +	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) *
> +			       info->bpp[0];
> +
> +	/*
> +	 * The user might have specified its own sizeimage value, only override
> +	 * it if it's not big enough.
> +	 */
> +	pixfmt->sizeimage = max(sizeimage, pixfmt->sizeimage);
> +

Same comment applies here: We need to sanitize it from too big sizeimages.

>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);

Thanks,
Mauro