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

From: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
To: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Cc: Ezequiel Garcia <ezequiel@collabora.com>,
	linux-media@vger.kernel.org,
	Hans Verkuil <hans.verkuil@cisco.com>,
	kernel@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 11:02:33 -0300	[thread overview]
Message-ID: <20190529110233.0ff81aff@coco.lan> (raw)
In-Reply-To: <4ef64e22f4035fccf09fbb7f0be04a9778b1230d.camel@collabora.com>

Em Wed, 29 May 2019 09:54:09 -0400
Nicolas Dufresne <nicolas.dufresne@collabora.com> 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 <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.
> 
> 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