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

[Hans Verkuil <hverkuil@xs4all.nl>]

On 6/6/19 9:09 AM, Boris Brezillon wrote:
> 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.

No, I don't want that :-)

See my earlier reply for the reason.

Regards,

	Hans

> 
> --->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);
>