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

Commit Message

Ezequiel Garcia May 28, 2019, 5:02 p.m. UTC

From: Boris Brezillon <[e-mail]>

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 <[e-mail]>
---
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(-)

Comments

Mauro Carvalho Chehab May 29, 2019, 11:28 a.m. UTC | #1

Em Tue, 28 May 2019 14:02:19 -0300
Ezequiel Garcia <[e-mail]> escreveu:

> From: Boris Brezillon <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 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

Hans Verkuil May 29, 2019, 11:43 a.m. UTC | #2

On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 28 May 2019 14:02:19 -0300
> Ezequiel Garcia <[e-mail]> escreveu:
> 
>> From: Boris Brezillon <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>> 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.

I've no idea what's sane.

Buffers can be really large. The largest video resolution defined by CTA-861-G
is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
instead of min/max.

Regards,

	Hans

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

Mauro Carvalho Chehab May 29, 2019, 11:58 a.m. UTC | #3

Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <[e-mail]> escreveu:

> On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:
> > Em Tue, 28 May 2019 14:02:19 -0300
> > Ezequiel Garcia <[e-mail]> escreveu:
> >   
> >> From: Boris Brezillon <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> ---
> >> 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.  
> 
> I've no idea what's sane.
> 
> Buffers can be really large. The largest video resolution defined by CTA-861-G
> is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> instead of min/max.

Well, the max is driver-specific. 

For example, for a camera with a max resolution of 640x480 with 2 bytes
per pixel as the max format can only be

	max_size = 640*480*2 (plus some alignment value if pertinent)

It sounds to me that the best would be to have a callback function
or value filled by the drivers that would support custom sizeimage.

The core could actually calculate during init (by asking the driver
to a very big resolution and getting the returned value), but
it sounds better to let the drivers to explicitly calculate it.

> 
> Regards,
> 
> 	Hans
> 
> >   
> >>  	} 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
> >   
> 



Thanks,
Mauro

Boris Brezillon May 29, 2019, 12:16 p.m. UTC | #4

On Wed, 29 May 2019 08:58:54 -0300
Mauro Carvalho Chehab <[e-mail]> wrote:

> Em Wed, 29 May 2019 13:43:20 +0200
> Hans Verkuil <[e-mail]> escreveu:
> 
> > On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:  
> > > Em Tue, 28 May 2019 14:02:19 -0300
> > > Ezequiel Garcia <[e-mail]> escreveu:
> > >     
> > >> From: Boris Brezillon <[e-mail]>
> > >>
> > >> 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 <[e-mail]>
> > >> ---
> > >> 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.    
> > 
> > I've no idea what's sane.
> > 
> > Buffers can be really large. The largest video resolution defined by CTA-861-G
> > is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> > use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> > instead of min/max.  
> 
> Well, the max is driver-specific. 
> 
> For example, for a camera with a max resolution of 640x480 with 2 bytes
> per pixel as the max format can only be
> 
> 	max_size = 640*480*2 (plus some alignment value if pertinent)
> 
> It sounds to me that the best would be to have a callback function
> or value filled by the drivers that would support custom sizeimage.
> 
> The core could actually calculate during init (by asking the driver
> to a very big resolution and getting the returned value), but
> it sounds better to let the drivers to explicitly calculate it.

If we want max_sizeimage to be driver specific I can add it as an extra
arg to the fill_pixfmt() funcs.

If that works for you, we'll send a new version of this patch alone
(unless you want us to send the whole series again).

Mauro Carvalho Chehab May 29, 2019, 12:26 p.m. UTC | #5

Em Wed, 29 May 2019 14:16:33 +0200
Boris Brezillon <[e-mail]> escreveu:

> On Wed, 29 May 2019 08:58:54 -0300
> Mauro Carvalho Chehab <[e-mail]> wrote:
> 
> > Em Wed, 29 May 2019 13:43:20 +0200
> > Hans Verkuil <[e-mail]> escreveu:
> >   
> > > On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:    
> > > > Em Tue, 28 May 2019 14:02:19 -0300
> > > > Ezequiel Garcia <[e-mail]> escreveu:
> > > >       
> > > >> From: Boris Brezillon <[e-mail]>
> > > >>
> > > >> 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 <[e-mail]>
> > > >> ---
> > > >> 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.      
> > > 
> > > I've no idea what's sane.
> > > 
> > > Buffers can be really large. The largest video resolution defined by CTA-861-G
> > > is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> > > use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> > > instead of min/max.    
> > 
> > Well, the max is driver-specific. 
> > 
> > For example, for a camera with a max resolution of 640x480 with 2 bytes
> > per pixel as the max format can only be
> > 
> > 	max_size = 640*480*2 (plus some alignment value if pertinent)
> > 
> > It sounds to me that the best would be to have a callback function
> > or value filled by the drivers that would support custom sizeimage.
> > 
> > The core could actually calculate during init (by asking the driver
> > to a very big resolution and getting the returned value), but
> > it sounds better to let the drivers to explicitly calculate it.  
> 
> If we want max_sizeimage to be driver specific I can add it as an extra
> arg to the fill_pixfmt() funcs.

Works for me.

> If that works for you, we'll send a new version of this patch alone
> (unless you want us to send the whole series again).

If the other patches on this series don't depend on this, I can
apply the PR just skipping this one, applying your patch afterwards.

Thanks,
Mauro

Mauro Carvalho Chehab May 29, 2019, 12:29 p.m. UTC | #6

Em Wed, 29 May 2019 09:26:32 -0300
Mauro Carvalho Chehab <[e-mail]> escreveu:

> Em Wed, 29 May 2019 14:16:33 +0200
> Boris Brezillon <[e-mail]> escreveu:
> 
> > On Wed, 29 May 2019 08:58:54 -0300
> > Mauro Carvalho Chehab <[e-mail]> wrote:
> >   
> > > Em Wed, 29 May 2019 13:43:20 +0200
> > > Hans Verkuil <[e-mail]> escreveu:
> > >     
> > > > On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:      
> > > > > Em Tue, 28 May 2019 14:02:19 -0300
> > > > > Ezequiel Garcia <[e-mail]> escreveu:
> > > > >         
> > > > >> From: Boris Brezillon <[e-mail]>
> > > > >>
> > > > >> 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 <[e-mail]>
> > > > >> ---
> > > > >> 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.        
> > > > 
> > > > I've no idea what's sane.
> > > > 
> > > > Buffers can be really large. The largest video resolution defined by CTA-861-G
> > > > is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> > > > use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> > > > instead of min/max.      
> > > 
> > > Well, the max is driver-specific. 
> > > 
> > > For example, for a camera with a max resolution of 640x480 with 2 bytes
> > > per pixel as the max format can only be
> > > 
> > > 	max_size = 640*480*2 (plus some alignment value if pertinent)
> > > 
> > > It sounds to me that the best would be to have a callback function
> > > or value filled by the drivers that would support custom sizeimage.
> > > 
> > > The core could actually calculate during init (by asking the driver
> > > to a very big resolution and getting the returned value), but
> > > it sounds better to let the drivers to explicitly calculate it.    
> > 
> > If we want max_sizeimage to be driver specific I can add it as an extra
> > arg to the fill_pixfmt() funcs.  
> 
> Works for me.
> 
> > If that works for you, we'll send a new version of this patch alone
> > (unless you want us to send the whole series again).  
> 
> If the other patches on this series don't depend on this, I can
> apply the PR just skipping this one, applying your patch afterwards.

Well, I can also apply the PR as-is and apply an extra patch at the
end.

> 
> Thanks,
> Mauro



Thanks,
Mauro

Hans Verkuil May 29, 2019, 12:31 p.m. UTC | #7

On 5/29/19 2:16 PM, Boris Brezillon wrote:
> On Wed, 29 May 2019 08:58:54 -0300
> Mauro Carvalho Chehab <[e-mail]> wrote:
> 
>> Em Wed, 29 May 2019 13:43:20 +0200
>> Hans Verkuil <[e-mail]> escreveu:
>>
>>> On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:  
>>>> Em Tue, 28 May 2019 14:02:19 -0300
>>>> Ezequiel Garcia <[e-mail]> escreveu:
>>>>     
>>>>> From: Boris Brezillon <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> ---
>>>>> 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.    
>>>
>>> I've no idea what's sane.
>>>
>>> Buffers can be really large. The largest video resolution defined by CTA-861-G
>>> is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
>>> use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
>>> instead of min/max.  
>>
>> Well, the max is driver-specific. 
>>
>> For example, for a camera with a max resolution of 640x480 with 2 bytes
>> per pixel as the max format can only be
>>
>> 	max_size = 640*480*2 (plus some alignment value if pertinent)
>>
>> It sounds to me that the best would be to have a callback function
>> or value filled by the drivers that would support custom sizeimage.
>>
>> The core could actually calculate during init (by asking the driver
>> to a very big resolution and getting the returned value), but
>> it sounds better to let the drivers to explicitly calculate it.
> 
> If we want max_sizeimage to be driver specific I can add it as an extra
> arg to the fill_pixfmt() funcs.

Looking more closely, only compressed formats can accept a user-specified
sizeimage value, and this function is only called for uncompressed formats.

So doesn't that mean that this sizeimage override code can be dropped?

Regards,

	Hans

> 
> If that works for you, we'll send a new version of this patch alone
> (unless you want us to send the whole series again).
>

Boris Brezillon May 29, 2019, 12:31 p.m. UTC | #8

On Wed, 29 May 2019 09:26:32 -0300
Mauro Carvalho Chehab <[e-mail]> wrote:

> Em Wed, 29 May 2019 14:16:33 +0200
> Boris Brezillon <[e-mail]> escreveu:
> 
> > On Wed, 29 May 2019 08:58:54 -0300
> > Mauro Carvalho Chehab <[e-mail]> wrote:
> >   
> > > Em Wed, 29 May 2019 13:43:20 +0200
> > > Hans Verkuil <[e-mail]> escreveu:
> > >     
> > > > On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:      
> > > > > Em Tue, 28 May 2019 14:02:19 -0300
> > > > > Ezequiel Garcia <[e-mail]> escreveu:
> > > > >         
> > > > >> From: Boris Brezillon <[e-mail]>
> > > > >>
> > > > >> 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 <[e-mail]>
> > > > >> ---
> > > > >> 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.        
> > > > 
> > > > I've no idea what's sane.
> > > > 
> > > > Buffers can be really large. The largest video resolution defined by CTA-861-G
> > > > is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> > > > use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> > > > instead of min/max.      
> > > 
> > > Well, the max is driver-specific. 
> > > 
> > > For example, for a camera with a max resolution of 640x480 with 2 bytes
> > > per pixel as the max format can only be
> > > 
> > > 	max_size = 640*480*2 (plus some alignment value if pertinent)
> > > 
> > > It sounds to me that the best would be to have a callback function
> > > or value filled by the drivers that would support custom sizeimage.
> > > 
> > > The core could actually calculate during init (by asking the driver
> > > to a very big resolution and getting the returned value), but
> > > it sounds better to let the drivers to explicitly calculate it.    
> > 
> > If we want max_sizeimage to be driver specific I can add it as an extra
> > arg to the fill_pixfmt() funcs.  
> 
> Works for me.
> 
> > If that works for you, we'll send a new version of this patch alone
> > (unless you want us to send the whole series again).  
> 
> If the other patches on this series don't depend on this, I can
> apply the PR just skipping this one, applying your patch afterwards.

Unfortunately they do. I was just asking if we could send a new version
of this patch and let you or Hans apply the other ones from this series
on top. If that's too complicated, we'll submit a v7 containing all
patches. BTW, any concerns on other patches in this series before we
send a new version?

Boris Brezillon May 29, 2019, 12:39 p.m. UTC | #9

On Wed, 29 May 2019 14:31:03 +0200
Hans Verkuil <[e-mail]> wrote:

> On 5/29/19 2:16 PM, Boris Brezillon wrote:
> > On Wed, 29 May 2019 08:58:54 -0300
> > Mauro Carvalho Chehab <[e-mail]> wrote:
> >   
> >> Em Wed, 29 May 2019 13:43:20 +0200
> >> Hans Verkuil <[e-mail]> escreveu:
> >>  
> >>> On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:    
> >>>> Em Tue, 28 May 2019 14:02:19 -0300
> >>>> Ezequiel Garcia <[e-mail]> escreveu:
> >>>>       
> >>>>> From: Boris Brezillon <[e-mail]>
> >>>>>
> >>>>> 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 <[e-mail]>
> >>>>> ---
> >>>>> 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.      
> >>>
> >>> I've no idea what's sane.
> >>>
> >>> Buffers can be really large. The largest video resolution defined by CTA-861-G
> >>> is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> >>> use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> >>> instead of min/max.    
> >>
> >> Well, the max is driver-specific. 
> >>
> >> For example, for a camera with a max resolution of 640x480 with 2 bytes
> >> per pixel as the max format can only be
> >>
> >> 	max_size = 640*480*2 (plus some alignment value if pertinent)
> >>
> >> It sounds to me that the best would be to have a callback function
> >> or value filled by the drivers that would support custom sizeimage.
> >>
> >> The core could actually calculate during init (by asking the driver
> >> to a very big resolution and getting the returned value), but
> >> it sounds better to let the drivers to explicitly calculate it.  
> > 
> > If we want max_sizeimage to be driver specific I can add it as an extra
> > arg to the fill_pixfmt() funcs.  
> 
> Looking more closely, only compressed formats can accept a user-specified
> sizeimage value, and this function is only called for uncompressed formats.
> 
> So doesn't that mean that this sizeimage override code can be dropped?

Hehe, IIRC, you were the one asking for this  :P (or maybe I
misunderstood what you suggested). I don't think we need to preserve
user-defined ->sizeimage for uncompressed fmt in the VPU driver, so I'm
perfectly fine implementing a version that overrides ->sizeimage
unconditionally.

Nicolas Dufresne May 29, 2019, 1:54 p.m. UTC | #10

Hi Mauro,

Le mercredi 29 mai 2019 à 08:28 -0300, Mauro Carvalho Chehab a écrit :
> Em Tue, 28 May 2019 14:02:19 -0300
> Ezequiel Garcia <[e-mail]> escreveu:
> 
> > From: Boris Brezillon <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> > 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.

We will really need this in the long run in many drivers in order to
allow import/export of DMABuf. Without such adaptive feature, we have a
software limitation that forces bouncing memory. I have already
discussed about adding this feature notably in vivid and uvcvideo on
IRC and in conference, which both have no restriction the memory
alignment, so should allow importing any kind of video layout.

We already have a partial userspace implementation for this in
GStreamer and upstream driver submission should come when the IP is
considered stable enough.

Why I think it won't break userspace is that the correct way to use
these read-only members of V4L2 struct is to set these to 0, which is
also documented.

Adding upper bound seems like a good idea though.

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

Mauro Carvalho Chehab May 29, 2019, 2:02 p.m. UTC | #11

Em Wed, 29 May 2019 09:54:09 -0400
Nicolas Dufresne <[e-mail]> escreveu:

> Hi Mauro,
> 
> Le mercredi 29 mai 2019 à 08:28 -0300, Mauro Carvalho Chehab a écrit :
> > Em Tue, 28 May 2019 14:02:19 -0300
> > Ezequiel Garcia <[e-mail]> escreveu:
> > 
> > > From: Boris Brezillon <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > ---
> > > 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.
> 
> We will really need this in the long run in many drivers in order to
> allow import/export of DMABuf. Without such adaptive feature, we have a
> software limitation that forces bouncing memory. I have already
> discussed about adding this feature notably in vivid and uvcvideo on
> IRC and in conference, which both have no restriction the memory
> alignment, so should allow importing any kind of video layout.
> 
> We already have a partial userspace implementation for this in
> GStreamer and upstream driver submission should come when the IP is
> considered stable enough.

I understand the need. I'm not against an implementation for such
feature, provided that it won't break anything.

I guess one of the things we miss at V4L2 API is an indication from
userspace about what it supports. I mean, just like we have the
caps flags where the Kernel reports what it supports, we could have
a similar "userspace caps"  field.

> Why I think it won't break userspace is that the correct way to use
> these read-only members of V4L2 struct is to set these to 0, which is
> also documented.

Yeah, the apps I'm aware of usually call memset() before filling
V4L2 structs. On those, adding this behavior would be ok. Yet,
I'm not sure if 100% of (open source) apps do that.

> Adding upper bound seems like a good idea though.

Agreed.

> 
>
Thanks,
Mauro

Ezequiel Garcia May 29, 2019, 2:04 p.m. UTC | #12

On Wed, 2019-05-29 at 14:31 +0200, Hans Verkuil wrote:
> On 5/29/19 2:16 PM, Boris Brezillon wrote:
> > On Wed, 29 May 2019 08:58:54 -0300
> > Mauro Carvalho Chehab <[e-mail]> wrote:
> > 
> > > Em Wed, 29 May 2019 13:43:20 +0200
> > > Hans Verkuil <[e-mail]> escreveu:
> > > 
> > > > On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:  
> > > > > Em Tue, 28 May 2019 14:02:19 -0300
> > > > > Ezequiel Garcia <[e-mail]> escreveu:
> > > > >     
> > > > > > From: Boris Brezillon <[e-mail]>
> > > > > > 
> > > > > > 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 <[e-mail]>
> > > > > > ---
> > > > > > 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.    
> > > > 
> > > > I've no idea what's sane.
> > > > 
> > > > Buffers can be really large. The largest video resolution defined by CTA-861-G
> > > > is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> > > > use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> > > > instead of min/max.  
> > > 
> > > Well, the max is driver-specific. 
> > > 
> > > For example, for a camera with a max resolution of 640x480 with 2 bytes
> > > per pixel as the max format can only be
> > > 
> > > 	max_size = 640*480*2 (plus some alignment value if pertinent)
> > > 
> > > It sounds to me that the best would be to have a callback function
> > > or value filled by the drivers that would support custom sizeimage.
> > > 
> > > The core could actually calculate during init (by asking the driver
> > > to a very big resolution and getting the returned value), but
> > > it sounds better to let the drivers to explicitly calculate it.
> > 
> > If we want max_sizeimage to be driver specific I can add it as an extra
> > arg to the fill_pixfmt() funcs.
> 
> Looking more closely, only compressed formats can accept a user-specified
> sizeimage value, and this function is only called for uncompressed formats.
> 
> So doesn't that mean that this sizeimage override code can be dropped?
> 

I think this is a good idea, which means just picking all patches except this one
(and the RK3328 one).

So it would be patches: 1,2 and 4 to 15.

Boris, Hans: are you OK with this?

Thanks,
Ezequiel

Boris Brezillon May 29, 2019, 2:06 p.m. UTC | #13

On Wed, 29 May 2019 11:04:35 -0300
Ezequiel Garcia <[e-mail]> wrote:

> On Wed, 2019-05-29 at 14:31 +0200, Hans Verkuil wrote:
> > On 5/29/19 2:16 PM, Boris Brezillon wrote:  
> > > On Wed, 29 May 2019 08:58:54 -0300
> > > Mauro Carvalho Chehab <[e-mail]> wrote:
> > >   
> > > > Em Wed, 29 May 2019 13:43:20 +0200
> > > > Hans Verkuil <[e-mail]> escreveu:
> > > >   
> > > > > On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:    
> > > > > > Em Tue, 28 May 2019 14:02:19 -0300
> > > > > > Ezequiel Garcia <[e-mail]> escreveu:
> > > > > >       
> > > > > > > From: Boris Brezillon <[e-mail]>
> > > > > > > 
> > > > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > > 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.      
> > > > > 
> > > > > I've no idea what's sane.
> > > > > 
> > > > > Buffers can be really large. The largest video resolution defined by CTA-861-G
> > > > > is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> > > > > use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> > > > > instead of min/max.    
> > > > 
> > > > Well, the max is driver-specific. 
> > > > 
> > > > For example, for a camera with a max resolution of 640x480 with 2 bytes
> > > > per pixel as the max format can only be
> > > > 
> > > > 	max_size = 640*480*2 (plus some alignment value if pertinent)
> > > > 
> > > > It sounds to me that the best would be to have a callback function
> > > > or value filled by the drivers that would support custom sizeimage.
> > > > 
> > > > The core could actually calculate during init (by asking the driver
> > > > to a very big resolution and getting the returned value), but
> > > > it sounds better to let the drivers to explicitly calculate it.  
> > > 
> > > If we want max_sizeimage to be driver specific I can add it as an extra
> > > arg to the fill_pixfmt() funcs.  
> > 
> > Looking more closely, only compressed formats can accept a user-specified
> > sizeimage value, and this function is only called for uncompressed formats.
> > 
> > So doesn't that mean that this sizeimage override code can be dropped?
> >   
> 
> I think this is a good idea, which means just picking all patches except this one
> (and the RK3328 one).
> 
> So it would be patches: 1,2 and 4 to 15.
> 
> Boris, Hans: are you OK with this?

I doesn't work => the prototype of the fill_pixfmt() changed.

Boris Brezillon May 29, 2019, 2:17 p.m. UTC | #14

On Wed, 29 May 2019 16:06:10 +0200
Boris Brezillon <[e-mail]> wrote:

> On Wed, 29 May 2019 11:04:35 -0300
> Ezequiel Garcia <[e-mail]> wrote:
> 
> > On Wed, 2019-05-29 at 14:31 +0200, Hans Verkuil wrote:  
> > > On 5/29/19 2:16 PM, Boris Brezillon wrote:    
> > > > On Wed, 29 May 2019 08:58:54 -0300
> > > > Mauro Carvalho Chehab <[e-mail]> wrote:
> > > >     
> > > > > Em Wed, 29 May 2019 13:43:20 +0200
> > > > > Hans Verkuil <[e-mail]> escreveu:
> > > > >     
> > > > > > On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:      
> > > > > > > Em Tue, 28 May 2019 14:02:19 -0300
> > > > > > > Ezequiel Garcia <[e-mail]> escreveu:
> > > > > > >         
> > > > > > > > From: Boris Brezillon <[e-mail]>
> > > > > > > > 
> > > > > > > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > > 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.        
> > > > > > 
> > > > > > I've no idea what's sane.
> > > > > > 
> > > > > > Buffers can be really large. The largest video resolution defined by CTA-861-G
> > > > > > is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> > > > > > use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> > > > > > instead of min/max.      
> > > > > 
> > > > > Well, the max is driver-specific. 
> > > > > 
> > > > > For example, for a camera with a max resolution of 640x480 with 2 bytes
> > > > > per pixel as the max format can only be
> > > > > 
> > > > > 	max_size = 640*480*2 (plus some alignment value if pertinent)
> > > > > 
> > > > > It sounds to me that the best would be to have a callback function
> > > > > or value filled by the drivers that would support custom sizeimage.
> > > > > 
> > > > > The core could actually calculate during init (by asking the driver
> > > > > to a very big resolution and getting the returned value), but
> > > > > it sounds better to let the drivers to explicitly calculate it.    
> > > > 
> > > > If we want max_sizeimage to be driver specific I can add it as an extra
> > > > arg to the fill_pixfmt() funcs.    
> > > 
> > > Looking more closely, only compressed formats can accept a user-specified
> > > sizeimage value, and this function is only called for uncompressed formats.
> > > 
> > > So doesn't that mean that this sizeimage override code can be dropped?
> > >     
> > 
> > I think this is a good idea, which means just picking all patches except this one
> > (and the RK3328 one).
> > 
> > So it would be patches: 1,2 and 4 to 15.
> > 
> > Boris, Hans: are you OK with this?  
> 
> I doesn't work => the prototype of the fill_pixfmt() changed.

Forget what I said, dropping patch 3 should work just fine.

Ezequiel Garcia May 29, 2019, 2:47 p.m. UTC | #15

On Wed, 2019-05-29 at 16:17 +0200, Boris Brezillon wrote:
> On Wed, 29 May 2019 16:06:10 +0200
> Boris Brezillon <[e-mail]> wrote:
> 
> > On Wed, 29 May 2019 11:04:35 -0300
> > Ezequiel Garcia <[e-mail]> wrote:
> > 
> > > On Wed, 2019-05-29 at 14:31 +0200, Hans Verkuil wrote:  
> > > > On 5/29/19 2:16 PM, Boris Brezillon wrote:    
> > > > > On Wed, 29 May 2019 08:58:54 -0300
> > > > > Mauro Carvalho Chehab <[e-mail]> wrote:
> > > > >     
> > > > > > Em Wed, 29 May 2019 13:43:20 +0200
> > > > > > Hans Verkuil <[e-mail]> escreveu:
> > > > > >     
> > > > > > > On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:      
> > > > > > > > Em Tue, 28 May 2019 14:02:19 -0300
> > > > > > > > Ezequiel Garcia <[e-mail]> escreveu:
> > > > > > > >         
> > > > > > > > > From: Boris Brezillon <[e-mail]>
> > > > > > > > > 
> > > > > > > > > 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 <[e-mail]>
> > > > > > > > > ---
> > > > > > > > > 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.        
> > > > > > > 
> > > > > > > I've no idea what's sane.
> > > > > > > 
> > > > > > > Buffers can be really large. The largest video resolution defined by CTA-861-G
> > > > > > > is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> > > > > > > use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> > > > > > > instead of min/max.      
> > > > > > 
> > > > > > Well, the max is driver-specific. 
> > > > > > 
> > > > > > For example, for a camera with a max resolution of 640x480 with 2 bytes
> > > > > > per pixel as the max format can only be
> > > > > > 
> > > > > > 	max_size = 640*480*2 (plus some alignment value if pertinent)
> > > > > > 
> > > > > > It sounds to me that the best would be to have a callback function
> > > > > > or value filled by the drivers that would support custom sizeimage.
> > > > > > 
> > > > > > The core could actually calculate during init (by asking the driver
> > > > > > to a very big resolution and getting the returned value), but
> > > > > > it sounds better to let the drivers to explicitly calculate it.    
> > > > > 
> > > > > If we want max_sizeimage to be driver specific I can add it as an extra
> > > > > arg to the fill_pixfmt() funcs.    
> > > > 
> > > > Looking more closely, only compressed formats can accept a user-specified
> > > > sizeimage value, and this function is only called for uncompressed formats.
> > > > 
> > > > So doesn't that mean that this sizeimage override code can be dropped?
> > > >     
> > > 
> > > I think this is a good idea, which means just picking all patches except this one
> > > (and the RK3328 one).
> > > 
> > > So it would be patches: 1,2 and 4 to 15.
> > > 
> > > Boris, Hans: are you OK with this?  
> > 
> > I doesn't work => the prototype of the fill_pixfmt() changed.
> 
> Forget what I said, dropping patch 3 should work just fine.

Yeah, I have reviewed the code and it seems OK. The prototype is not changed,
and the driver is not relying on the sizeimage override feature.

Also, tested on my RK3399 Rockpi board, and MPEG-2 decoding works fine.

v4l2-compliance also passes with no failures.

Seems OK to just drop patch 3.

Thanks,
Ezequiel

Patch

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);
 	} 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. */
+			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);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);