* Re: [PATCH] media: v4l2: Fix the _MPLANE format check in v4l_enum_fmt()
2019-06-06 6:51 ` Boris Brezillon
@ 2019-06-06 7:09 ` Hans Verkuil
2019-06-06 8:01 ` Tomasz Figa
2019-06-06 7:09 ` Boris Brezillon
2019-06-06 13:58 ` Nicolas Dufresne
2 siblings, 1 reply; 8+ messages in thread
From: Hans Verkuil @ 2019-06-06 7:09 UTC (permalink / raw)
To: Boris Brezillon, Tomasz Figa
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Linux Media Mailing List, kernel, Hirokazu Honda,
Nicolas Dufresne, Sylwester Nawrocki, Maxime Jourdan
On 6/6/19 8:51 AM, Boris Brezillon 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).
>
No, we decided at some point not to do that. The M2M cap makes it explicit
that this is a memory to memory device, and it avoids applications from
trying to use it as a webcam since most apps just check for the VIDEO_CAPTURE
capability.
Regards,
Hans
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] media: v4l2: Fix the _MPLANE format check in v4l_enum_fmt()
2019-06-06 7:09 ` Hans Verkuil
@ 2019-06-06 8:01 ` Tomasz Figa
0 siblings, 0 replies; 8+ messages in thread
From: Tomasz Figa @ 2019-06-06 8:01 UTC (permalink / raw)
To: Hans Verkuil
Cc: Boris Brezillon, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus, Linux Media Mailing List, kernel,
Hirokazu Honda, Nicolas Dufresne, Sylwester Nawrocki,
Maxime Jourdan
On Thu, Jun 6, 2019 at 4:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 6/6/19 8:51 AM, Boris Brezillon 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).
> >
>
> No, we decided at some point not to do that. The M2M cap makes it explicit
> that this is a memory to memory device, and it avoids applications from
> trying to use it as a webcam since most apps just check for the VIDEO_CAPTURE
> capability.
Fair enough. Feel free to add
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: v4l2: Fix the _MPLANE format check in v4l_enum_fmt()
2019-06-06 6:51 ` Boris Brezillon
2019-06-06 7:09 ` Hans Verkuil
@ 2019-06-06 7:09 ` Boris Brezillon
2019-06-06 7:10 ` Hans Verkuil
2019-06-06 13:58 ` Nicolas Dufresne
2 siblings, 1 reply; 8+ messages in thread
From: Boris Brezillon @ 2019-06-06 7:09 UTC (permalink / raw)
To: Tomasz Figa
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Linux Media Mailing List, kernel, Hirokazu Honda,
Nicolas Dufresne, Sylwester Nawrocki, Maxime Jourdan
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);
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] media: v4l2: Fix the _MPLANE format check in v4l_enum_fmt()
2019-06-06 7:09 ` Boris Brezillon
@ 2019-06-06 7:10 ` Hans Verkuil
0 siblings, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2019-06-06 7:10 UTC (permalink / raw)
To: Boris Brezillon, Tomasz Figa
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Linux Media Mailing List, kernel, Hirokazu Honda,
Nicolas Dufresne, Sylwester Nawrocki, Maxime Jourdan
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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: v4l2: Fix the _MPLANE format check in v4l_enum_fmt()
2019-06-06 6:51 ` Boris Brezillon
2019-06-06 7:09 ` Hans Verkuil
2019-06-06 7:09 ` Boris Brezillon
@ 2019-06-06 13:58 ` Nicolas Dufresne
2 siblings, 0 replies; 8+ messages in thread
From: Nicolas Dufresne @ 2019-06-06 13:58 UTC (permalink / raw)
To: Boris Brezillon, Tomasz Figa
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, Linux Media Mailing List, kernel, Hirokazu Honda,
Sylwester Nawrocki, Maxime Jourdan
[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]
Le jeudi 06 juin 2019 à 08:51 +0200, Boris Brezillon a écrit :
> 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 not be backward compatible, some userspace would still
detect these as being capture device and propose these as cameras.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread