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@xxxxxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > 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:
signature.asc
Description: This is a digitally signed message part
