Re: [PATCH v5 03/15] media: v4l2-common: Support custom imagesize/bytesperline in fill_pixfmt()

From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Hans Verkuil <hverkuil@xs4all.nl>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Boris Brezillon <boris.brezillon@collabora.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Sakari Ailus <sakari.ailus@iki.fi>,
	linux-media@vger.kernel.org, Tomasz Figa <tfiga@chromium.org>,
	kernel@collabora.com,
	Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
	Ezequiel Garcia <ezequiel@collabora.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	linux-rockchip@lists.infradead.org,
	Heiko Stuebner <heiko@sntech.de>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>
Subject: Re: [PATCH v5 03/15] media: v4l2-common: Support custom imagesize/bytesperline in fill_pixfmt()
Date: Fri, 10 May 2019 12:01:04 -0400	[thread overview]
Message-ID: <703e94a8eaf995ffedf94d3d6d88a83cf8a5366e.camel@ndufresne.ca> (raw)
In-Reply-To: <4e7f49b2-48ad-b504-5826-d993ba85e624@xs4all.nl>

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

Le vendredi 10 mai 2019 à 16:07 +0200, Hans Verkuil a écrit :
> On 5/10/19 3:51 PM, Nicolas Dufresne wrote:
> > Le vendredi 10 mai 2019 à 14:30 +0200, Hans Verkuil a écrit :
> > > On 5/10/19 2:24 PM, Laurent Pinchart wrote:
> > > > Hi Hans,
> > > > 
> > > > On Fri, May 10, 2019 at 02:17:32PM +0200, Hans Verkuil wrote:
> > > > > On 5/10/19 1:28 PM, Laurent Pinchart wrote:
> > > > > > On Fri, May 10, 2019 at 10:57:26AM +0200, Hans Verkuil wrote:
> > > > > > > On 5/3/19 1:47 PM, Boris Brezillon wrote:
> > > > > > > > Users can define custom sizeimage and bytesperline 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.
> > > > > > > > 
> > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > > > > ---
> > > > > > > > Hello Hans,
> > > > > > > > 
> > > > > > > > The sizeimage/bytesperline check on !MPLANE formats is still not 100%
> > > > > > > > sure, as custom bytesperline might induce bigger sizeimage than what
> > > > > > > > we calculate.
> > > > > > > > 
> > > > > > > > I tried implementing something smarter taking the per-component plane
> > > > > > > > bpp + hdiv param as we discussed the other day but decided to step
> > > > > > > > back after realizing the per-component plane macro block might also
> > > > > > > > differ at least in theory (not sure that's true in practice) and that
> > > > > > > > has an impact on bytesperline too.
> > > > > > > > 
> > > > > > > > Let me know how you want to handle that case.
> > > > > > > > 
> > > > > > > > Regards,
> > > > > > > > 
> > > > > > > > Boris
> > > > > > > > 
> > > > > > > > Changes in v5:
> > > > > > > > * New patch
> > > > > > > > ---
> > > > > > > >  drivers/media/v4l2-core/v4l2-common.c | 54 +++++++++++++++++++--------
> > > > > > > >  1 file changed, 39 insertions(+), 15 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > > > > > > > index 3c6f5c115fc5..37bfc984a8b5 100644
> > > > > > > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > > > > > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > > > > > > @@ -563,9 +563,10 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
> > > > > > > >  	pixfmt->num_planes = info->mem_planes;
> > > > > > > >  
> > > > > > > >  	if (info->mem_planes == 1) {
> > > > > > > > +		u32 bytesperline, sizeimage = 0;
> > > > > > > > +
> > > > > > > >  		plane = &pixfmt->plane_fmt[0];
> > > > > > > > -		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> > > > > > > > -		plane->sizeimage = 0;
> > > > > > > > +		bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> > > > > > > >  
> > > > > > > >  		for (i = 0; i < info->comp_planes; i++) {
> > > > > > > >  			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> > > > > > > > @@ -576,10 +577,17 @@ 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);
> > > > > > > >  		}
> > > > > > > > +
> > > > > > > > +		/*
> > > > > > > > +		 * The user might have specified custom sizeimage/bytesperline,
> > > > > > > > +		 * only override them if they're not big enough.
> > > > > > > > +		 */
> > > > > > > > +		plane->sizeimage = max(sizeimage, plane->sizeimage);
> > > > > > > > +		plane->bytesperline = max(bytesperline, plane->bytesperline);
> > > > > > > 
> > > > > > > Let's just set bytesperline, ignoring the value the user supplied. There are very
> > > > > > > few drivers that allow the user to set bytesperline anyway, so it's not a big deal
> > > > > > > to drop support for that for now. We can add it back later.
> > > > > > > 
> > > > > > > Just add a comment that a user-defined bytesperline value isn't currently supported.
> > > > > > 
> > > > > > Kieran recently ran into an issue related to this, when sharing buffers
> > > > > > between a CSI-2 receiver and an ISP. The ISP has alignment constraints
> > > > > > in both the horizontal and vertical directions on the line stride and
> > > > > > total image size. Out software framework currently allocates buffers
> > > > > > from the CSI-2 receiver which doesn't have those constraints, and not
> > > > > > being able to specify sizeimage is thus a problem.
> > > > > 
> > > > > Not being able to specify sizeimage where? From userspace? Sorry, I don't
> > > > > quite understand the specific issue here.
> > > > 
> > > > Yes, from userspace.
> > > 
> > > Ah, OK. But why not use CREATEBUFS? You can provide your own size when allocating
> > > the buffers.
> > > 
> > > Also note this patch: https://patchwork.linuxtv.org/patch/55656/
> > > 
> > > Although this is specific for compressed formats.
> > 
> > While this work for compressed formats, it does not do anything for raw
> > image horizontal padding. The importation bit of V4L2 is pretty
> > difficult, so if we add helpers, we should load the way and simplify
> > things for userspace rather then enforcing the existing difficulty.
> > 
> > I think from now own we should design with the mindset that a DMABuf
> > that cannot be imported back into another driver due to software
> > limitations is a useless waste of FD.
> 
> I agree, but I feel that this is part of the new fmt and streaming
> ioctls project that Boris started. Doing further hacking of the existing API
> is just complicating matters even more.
> 
> Creating new format ioctls that are much more flexible in describing image
> formats (and closer to drm where possible) seems to be the right approach.
> 
> Hmmm... "waste of FD": Face Detection? File Descriptor? Probably not.
> 
> The urbandictionary doesn't help either: https://www.urbandictionary.com/define.php?term=FD
> 
> No idea what FD means :-), although I get the sentiment.

I was not swearing. An FD on Unix systems like Linux is also well know
as a File Descriptor. DMABuf, memfd, an open file, etc.

regards,
Nicolas

> 
> Regards,
> 
> 	Hans
> 

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