Em Wed, 29 May 2019 09:54:09 -0400
Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> escreveu:
> >
> > > From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxx>
> > > ---
> > > 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
