Re: [PATCH] media: v4l2: Fix the _MPLANE format check in v4l_enum_fmt()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 6 Jun 2019 08:51:59 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 6 Jun 2019 12:53:57 +0900
> Tomasz Figa <tfiga@chromium.org> wrote:
> 
> > On Thu, Jun 6, 2019 at 1:46 AM Boris Brezillon
> > <boris.brezillon@collabora.com> wrote:  
> > >
> > > CAP_M2M_MPLANE means the device supports _MPLANE formats for both
> > > capture and output. Adjust the check to avoid EINVAL errors on
> > > such devices.
> > >
> > > Fixes: 366c719d6479 ("media: v4l2: Get rid of ->vidioc_enum_fmt_vid_{cap,out}_mplane")
> > > Reported-by: Maxime Jourdan <mjourdan@baylibre.com>
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-ioctl.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > index b4c73e8f23c5..ace9b9761bed 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > @@ -1385,6 +1385,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > >         struct video_device *vdev = video_devdata(file);
> > >         struct v4l2_fmtdesc *p = arg;
> > >         int ret = check_fmt(file, p->type);
> > > +       u32 cap_mask;
> > >
> > >         if (ret)
> > >                 return ret;
> > > @@ -1393,7 +1394,9 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> > >         switch (p->type) {
> > >         case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > >         case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > > -               if (!!(vdev->device_caps & V4L2_CAP_VIDEO_CAPTURE_MPLANE) !=
> > > +               cap_mask = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> > > +                          V4L2_CAP_VIDEO_M2M_MPLANE;
> > > +               if (!!(vdev->device_caps & cap_mask) !=    
> > 
> > Shouldn't devices that report V4L2_CAP_VIDEO_M2M_MPLANE have also
> > V4L2_CAP_VIDEO_CAPTURE_MPLANE and V4L2_CAP_VIDEO_OUTPUT_MPLANE
> > reported anyway?  
> 
> That's the other option, force drivers that set
> V4L2_CAP_VIDEO_M2M_MPLANE to also set
> V4L2_CAP_VIDEO_{CAPTURE,OUTPUT}_MPLANE (or we can let the core do it).

That would basically look like this. Hans, let me know if you want me
to send a new version with the below diff.

--->8---
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 29946a2b2752..9cb782c62f69 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -933,6 +933,14 @@ int __video_register_device(struct video_device *vdev,
        }
 #endif
 
+       /*
+        * Adjust ->device_caps: CAP_M2M_MPLANE implies CAP_CAPTURE_MPLANES and
+        * CAP_OUTPUT_MLANES.
+        */
+       if (vdev->device_caps & V4L2_CAP_VIDEO_M2M_MPLANE)
+               vdev->device_caps |= V4L2_CAP_VIDEO_CAPTURE_MPLANE |
+                                    V4L2_CAP_VIDEO_OUTPUT_MPLANE;
+
        /* Pick a device node number */
        mutex_lock(&videodev_lock);
        nr = devnode_find(vdev, nr == -1 ? 0 : nr, minor_cnt);