* [PATCH v2 0/6] uvcvideo: Fix v4l2-compliance errors @ 2021-03-11 22:19 Ricardo Ribalda 2021-03-11 22:19 ` [PATCH v2 1/6] media: v4l2-ioctl: Fix check_ext_ctrls Ricardo Ribalda ` (5 more replies) 0 siblings, 6 replies; 24+ messages in thread From: Ricardo Ribalda @ 2021-03-11 22:19 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Cc: Ricardo Ribalda In my computer I am getting this output for v4l2-compliance -m /dev/media0 -a -f Total for uvcvideo device /dev/media0: 8, Succeeded: 6, Failed: 2, Warnings: 0 Total for uvcvideo device /dev/video0: 54, Succeeded: 50, Failed: 4, Warnings: 2 Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0 Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 102, Failed: 6, Warnings: 2 After fixing all of them we go down to: Total for uvcvideo device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0 Total for uvcvideo device /dev/video0: 54, Succeeded: 54, Failed: 0, Warnings: 9 Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0 Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 108, Failed: 0, Warnings: 9 We are still not compliant with v4l2-compliance -s: Streaming ioctls: test read/write: OK (Not Supported) test blocking wait: OK fail: v4l2-test-buffers.cpp(1265): node->streamon(q.g_type()) != EINVAL test MMAP (no poll): FAIL fail: v4l2-test-buffers.cpp(1265): node->streamon(q.g_type()) != EINVAL test MMAP (select): FAIL fail: v4l2-test-buffers.cpp(1265): node->streamon(q.g_type()) != EINVAL test MMAP (epoll): FAIL But fixing that will probably require a lot of changes in the driver that are already implemented in the vb2 helpers. It is better to continue Hans work on that: https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=uvc-4.19&id=a6a0a05f643521d29a4c1e551b0be73ce66b7108 Changelog v2 (Thanks to Hans and Laurent) - Reimplement the CTRL_CLASS as a patch on queryctl - Do not return -EIO for case 8 - Handle request bug and which_def multiclass in core Ricardo Ribalda (6): media: v4l2-ioctl: Fix check_ext_ctrls media: uvcvideo: Set capability in s_param media: uvcvideo: Return -EIO for control errors media: uvcvideo: set error_idx to count on EACCESS media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS media: uvcvideo: Set a different name for the metadata entity drivers/media/usb/uvc/uvc_ctrl.c | 90 ++++++++++++++++++++++++++++ drivers/media/usb/uvc/uvc_driver.c | 5 +- drivers/media/usb/uvc/uvc_v4l2.c | 10 +++- drivers/media/usb/uvc/uvc_video.c | 5 ++ drivers/media/usb/uvc/uvcvideo.h | 7 +++ drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- 6 files changed, 116 insertions(+), 5 deletions(-) -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/6] media: v4l2-ioctl: Fix check_ext_ctrls 2021-03-11 22:19 [PATCH v2 0/6] uvcvideo: Fix v4l2-compliance errors Ricardo Ribalda @ 2021-03-11 22:19 ` Ricardo Ribalda 2021-03-11 23:43 ` Laurent Pinchart 2021-03-11 22:19 ` [PATCH v2 2/6] media: uvcvideo: Set capability in s_param Ricardo Ribalda ` (4 subsequent siblings) 5 siblings, 1 reply; 24+ messages in thread From: Ricardo Ribalda @ 2021-03-11 22:19 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Cc: Ricardo Ribalda, stable, Hans Verkuil Drivers that do not use the ctrl-framework use this function instead. - Return error when handling of REQUEST_VAL. - Do not check for multiple classes when getting the DEF_VAL. Fixes v4l2-compliance: Control ioctls (Input 0): fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL Cc: stable@vger.kernel.org Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/v4l2-core/v4l2-ioctl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 31d1342e61e8..6f6b310e2802 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -924,8 +924,10 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) */ if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) return 0; - if (!c->which) + if (!c->which || c->which == V4L2_CTRL_WHICH_DEF_VAL) return 1; + if (c->which == V4L2_CTRL_WHICH_REQUEST_VAL) + return 0; /* Check that all controls are from the same control class. */ for (i = 0; i < c->count; i++) { if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/6] media: v4l2-ioctl: Fix check_ext_ctrls 2021-03-11 22:19 ` [PATCH v2 1/6] media: v4l2-ioctl: Fix check_ext_ctrls Ricardo Ribalda @ 2021-03-11 23:43 ` Laurent Pinchart 0 siblings, 0 replies; 24+ messages in thread From: Laurent Pinchart @ 2021-03-11 23:43 UTC (permalink / raw) To: Ricardo Ribalda Cc: Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil, stable, Hans Verkuil Hi Ricardo, Thank you for the patch. On Thu, Mar 11, 2021 at 11:19:41PM +0100, Ricardo Ribalda wrote: > Drivers that do not use the ctrl-framework use this function instead. > > - Return error when handling of REQUEST_VAL. > - Do not check for multiple classes when getting the DEF_VAL. > > Fixes v4l2-compliance: > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Cc: stable@vger.kernel.org > Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 31d1342e61e8..6f6b310e2802 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -924,8 +924,10 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv) > */ > if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE) > return 0; > - if (!c->which) > + if (!c->which || c->which == V4L2_CTRL_WHICH_DEF_VAL) How about if (c->which == V4L2_CTRL_WHICH_CUR_VAL || c->which == V4L2_CTRL_WHICH_DEF_VAL) > return 1; > + if (c->which == V4L2_CTRL_WHICH_REQUEST_VAL) > + return 0; Or possibly switch (c->which) { case V4L2_CTRL_WHICH_CUR_VAL: case V4L2_CTRL_WHICH_DEF_VAL: return 1; case V4L2_CTRL_WHICH_REQUEST_VAL: return 0; } Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > /* Check that all controls are from the same control class. */ > for (i = 0; i < c->count; i++) { > if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) { -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 2/6] media: uvcvideo: Set capability in s_param 2021-03-11 22:19 [PATCH v2 0/6] uvcvideo: Fix v4l2-compliance errors Ricardo Ribalda 2021-03-11 22:19 ` [PATCH v2 1/6] media: v4l2-ioctl: Fix check_ext_ctrls Ricardo Ribalda @ 2021-03-11 22:19 ` Ricardo Ribalda 2021-03-12 7:07 ` Hans Verkuil 2021-03-11 22:19 ` [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors Ricardo Ribalda ` (3 subsequent siblings) 5 siblings, 1 reply; 24+ messages in thread From: Ricardo Ribalda @ 2021-03-11 22:19 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Cc: Ricardo Ribalda Fixes v4l2-compliance: Format ioctls (Input 0): warn: v4l2-test-formats.cpp(1339): S_PARM is supported but doesn't report V4L2_CAP_TIMEPERFRAME fail: v4l2-test-formats.cpp(1241): node->has_frmintervals && !cap->capability Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_v4l2.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 252136cc885c..157310c0ca87 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -472,10 +472,13 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream, uvc_simplify_fraction(&timeperframe.numerator, &timeperframe.denominator, 8, 333); - if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) + if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) { parm->parm.capture.timeperframe = timeperframe; - else + parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME; + } else { parm->parm.output.timeperframe = timeperframe; + parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME; + } return 0; } -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/6] media: uvcvideo: Set capability in s_param 2021-03-11 22:19 ` [PATCH v2 2/6] media: uvcvideo: Set capability in s_param Ricardo Ribalda @ 2021-03-12 7:07 ` Hans Verkuil 0 siblings, 0 replies; 24+ messages in thread From: Hans Verkuil @ 2021-03-12 7:07 UTC (permalink / raw) To: Ricardo Ribalda, Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky On 11/03/2021 23:19, Ricardo Ribalda wrote: > Fixes v4l2-compliance: > > Format ioctls (Input 0): > warn: v4l2-test-formats.cpp(1339): S_PARM is supported but doesn't report V4L2_CAP_TIMEPERFRAME > fail: v4l2-test-formats.cpp(1241): node->has_frmintervals && !cap->capability > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Thanks! Hans > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_v4l2.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 252136cc885c..157310c0ca87 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -472,10 +472,13 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream, > uvc_simplify_fraction(&timeperframe.numerator, > &timeperframe.denominator, 8, 333); > > - if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) > + if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) { > parm->parm.capture.timeperframe = timeperframe; > - else > + parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME; > + } else { > parm->parm.output.timeperframe = timeperframe; > + parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME; > + } > > return 0; > } > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors 2021-03-11 22:19 [PATCH v2 0/6] uvcvideo: Fix v4l2-compliance errors Ricardo Ribalda 2021-03-11 22:19 ` [PATCH v2 1/6] media: v4l2-ioctl: Fix check_ext_ctrls Ricardo Ribalda 2021-03-11 22:19 ` [PATCH v2 2/6] media: uvcvideo: Set capability in s_param Ricardo Ribalda @ 2021-03-11 22:19 ` Ricardo Ribalda 2021-03-11 22:50 ` Laurent Pinchart 2021-03-12 7:08 ` Hans Verkuil 2021-03-11 22:19 ` [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS Ricardo Ribalda ` (2 subsequent siblings) 5 siblings, 2 replies; 24+ messages in thread From: Ricardo Ribalda @ 2021-03-11 22:19 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Cc: Ricardo Ribalda The device is doing something unspected with the control. Either because the protocol is not properly implemented or there has been a HW error. Fixes v4l2-compliance: Control ioctls (Input 0): fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) test VIDIOC_G/S_CTRL: FAIL fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_video.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index f2f565281e63..25fd8aa23529 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -112,6 +112,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, case 5: /* Invalid unit */ case 6: /* Invalid control */ case 7: /* Invalid Request */ + /* + * The firmware has not properly implemented + * the control or there has been a HW error. + */ + return -EIO; case 8: /* Invalid value within range */ return -EINVAL; default: /* reserved or unknown */ -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors 2021-03-11 22:19 ` [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors Ricardo Ribalda @ 2021-03-11 22:50 ` Laurent Pinchart 2021-03-11 22:59 ` Ricardo Ribalda Delgado 2021-03-12 7:08 ` Hans Verkuil 1 sibling, 1 reply; 24+ messages in thread From: Laurent Pinchart @ 2021-03-11 22:50 UTC (permalink / raw) To: Ricardo Ribalda Cc: Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Hi Ricardo, Thank you for the patch. On Thu, Mar 11, 2021 at 11:19:43PM +0100, Ricardo Ribalda wrote: > The device is doing something unspected with the control. Either because > the protocol is not properly implemented or there has been a HW error. > > Fixes v4l2-compliance: > > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > test VIDIOC_G/S_CTRL: FAIL > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> The change looks good to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Which of the error codes below do you get with your camera, and for which control ? > --- > drivers/media/usb/uvc/uvc_video.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index f2f565281e63..25fd8aa23529 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -112,6 +112,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > case 5: /* Invalid unit */ > case 6: /* Invalid control */ > case 7: /* Invalid Request */ > + /* > + * The firmware has not properly implemented > + * the control or there has been a HW error. > + */ > + return -EIO; > case 8: /* Invalid value within range */ > return -EINVAL; > default: /* reserved or unknown */ -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors 2021-03-11 22:50 ` Laurent Pinchart @ 2021-03-11 22:59 ` Ricardo Ribalda Delgado 2021-03-11 23:30 ` Laurent Pinchart 0 siblings, 1 reply; 24+ messages in thread From: Ricardo Ribalda Delgado @ 2021-03-11 22:59 UTC (permalink / raw) To: Laurent Pinchart Cc: Ricardo Ribalda, Mauro Carvalho Chehab, Tomasz Figa, linux-media, LKML, Sergey Senozhatsky, Hans Verkuil Hi Laurent On Thu, Mar 11, 2021 at 11:53 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. Thank you :) > > On Thu, Mar 11, 2021 at 11:19:43PM +0100, Ricardo Ribalda wrote: > > The device is doing something unspected with the control. Either because > > the protocol is not properly implemented or there has been a HW error. > > > > Fixes v4l2-compliance: > > > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > > test VIDIOC_G/S_CTRL: FAIL > > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > The change looks good to me. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Which of the error codes below do you get with your camera, and for > which control ? Bus 001 Device 003: ID 05c8:03a2 Cheng Uei Precision Industry Co., Ltd (Foxlink) XiaoMi USB 2.0 Webcam info: checking extended control 'White Balance Temperature' (0x0098091a) Control error 7 info: checking extended control 'Exposure (Absolute)' (0x009a0902) Control error 6 > > > --- > > drivers/media/usb/uvc/uvc_video.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index f2f565281e63..25fd8aa23529 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -112,6 +112,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > case 5: /* Invalid unit */ > > case 6: /* Invalid control */ > > case 7: /* Invalid Request */ > > + /* > > + * The firmware has not properly implemented > > + * the control or there has been a HW error. > > + */ > > + return -EIO; > > case 8: /* Invalid value within range */ > > return -EINVAL; > > default: /* reserved or unknown */ > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors 2021-03-11 22:59 ` Ricardo Ribalda Delgado @ 2021-03-11 23:30 ` Laurent Pinchart 2021-03-12 6:51 ` Ricardo Ribalda Delgado 0 siblings, 1 reply; 24+ messages in thread From: Laurent Pinchart @ 2021-03-11 23:30 UTC (permalink / raw) To: Ricardo Ribalda Delgado Cc: Ricardo Ribalda, Mauro Carvalho Chehab, Tomasz Figa, linux-media, LKML, Sergey Senozhatsky, Hans Verkuil Hi Ricardo, On Thu, Mar 11, 2021 at 11:59:27PM +0100, Ricardo Ribalda Delgado wrote: > On Thu, Mar 11, 2021 at 11:53 PM Laurent Pinchart wrote: > > On Thu, Mar 11, 2021 at 11:19:43PM +0100, Ricardo Ribalda wrote: > > > The device is doing something unspected with the control. Either because > > > the protocol is not properly implemented or there has been a HW error. > > > > > > Fixes v4l2-compliance: > > > > > > Control ioctls (Input 0): > > > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > > > test VIDIOC_G/S_CTRL: FAIL > > > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > The change looks good to me. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Which of the error codes below do you get with your camera, and for > > which control ? > > Bus 001 Device 003: ID 05c8:03a2 Cheng Uei Precision Industry Co., Ltd > (Foxlink) XiaoMi USB 2.0 Webcam > > info: checking extended control 'White Balance Temperature' (0x0098091a) > Control error 7 > info: checking extended control 'Exposure (Absolute)' (0x009a0902) > Control error 6 :-S And what's the request (GET_CUR, GET_INFO, ...) ? > > > --- > > > drivers/media/usb/uvc/uvc_video.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > index f2f565281e63..25fd8aa23529 100644 > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > @@ -112,6 +112,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > case 5: /* Invalid unit */ > > > case 6: /* Invalid control */ > > > case 7: /* Invalid Request */ > > > + /* > > > + * The firmware has not properly implemented > > > + * the control or there has been a HW error. > > > + */ > > > + return -EIO; > > > case 8: /* Invalid value within range */ > > > return -EINVAL; > > > default: /* reserved or unknown */ -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors 2021-03-11 23:30 ` Laurent Pinchart @ 2021-03-12 6:51 ` Ricardo Ribalda Delgado 0 siblings, 0 replies; 24+ messages in thread From: Ricardo Ribalda Delgado @ 2021-03-12 6:51 UTC (permalink / raw) To: Laurent Pinchart Cc: Ricardo Ribalda, Mauro Carvalho Chehab, Tomasz Figa, linux-media, LKML, Sergey Senozhatsky, Hans Verkuil Hi Laurent On Fri, Mar 12, 2021 at 12:30 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > On Thu, Mar 11, 2021 at 11:59:27PM +0100, Ricardo Ribalda Delgado wrote: > > On Thu, Mar 11, 2021 at 11:53 PM Laurent Pinchart wrote: > > > On Thu, Mar 11, 2021 at 11:19:43PM +0100, Ricardo Ribalda wrote: > > > > The device is doing something unspected with the control. Either because > > > > the protocol is not properly implemented or there has been a HW error. > > > > > > > > Fixes v4l2-compliance: > > > > > > > > Control ioctls (Input 0): > > > > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > > > > test VIDIOC_G/S_CTRL: FAIL > > > > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > > > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > The change looks good to me. > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Which of the error codes below do you get with your camera, and for > > > which control ? > > > > Bus 001 Device 003: ID 05c8:03a2 Cheng Uei Precision Industry Co., Ltd > > (Foxlink) XiaoMi USB 2.0 Webcam > > > > info: checking extended control 'White Balance Temperature' (0x0098091a) > > Control error 7 > > info: checking extended control 'Exposure (Absolute)' (0x009a0902) > > Control error 6 > > :-S And what's the request (GET_CUR, GET_INFO, ...) ? Failed to query (SET_CUR) UVC control 10 on unit 2: -32 (exp. 2). Failed to query (SET_CUR) UVC control 4 on unit 1: -32 (exp. 4) > > > > > --- > > > > drivers/media/usb/uvc/uvc_video.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > > index f2f565281e63..25fd8aa23529 100644 > > > > --- a/drivers/media/usb/uvc/uvc_video.c > > > > +++ b/drivers/media/usb/uvc/uvc_video.c > > > > @@ -112,6 +112,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > > > case 5: /* Invalid unit */ > > > > case 6: /* Invalid control */ > > > > case 7: /* Invalid Request */ > > > > + /* > > > > + * The firmware has not properly implemented > > > > + * the control or there has been a HW error. > > > > + */ > > > > + return -EIO; > > > > case 8: /* Invalid value within range */ > > > > return -EINVAL; > > > > default: /* reserved or unknown */ > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors 2021-03-11 22:19 ` [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors Ricardo Ribalda 2021-03-11 22:50 ` Laurent Pinchart @ 2021-03-12 7:08 ` Hans Verkuil 1 sibling, 0 replies; 24+ messages in thread From: Hans Verkuil @ 2021-03-12 7:08 UTC (permalink / raw) To: Ricardo Ribalda, Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky On 11/03/2021 23:19, Ricardo Ribalda wrote: > The device is doing something unspected with the control. Either because > the protocol is not properly implemented or there has been a HW error. > > Fixes v4l2-compliance: > > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > test VIDIOC_G/S_CTRL: FAIL > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Thanks! Hans > --- > drivers/media/usb/uvc/uvc_video.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index f2f565281e63..25fd8aa23529 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -112,6 +112,11 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > case 5: /* Invalid unit */ > case 6: /* Invalid control */ > case 7: /* Invalid Request */ > + /* > + * The firmware has not properly implemented > + * the control or there has been a HW error. > + */ > + return -EIO; > case 8: /* Invalid value within range */ > return -EINVAL; > default: /* reserved or unknown */ > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS 2021-03-11 22:19 [PATCH v2 0/6] uvcvideo: Fix v4l2-compliance errors Ricardo Ribalda ` (2 preceding siblings ...) 2021-03-11 22:19 ` [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors Ricardo Ribalda @ 2021-03-11 22:19 ` Ricardo Ribalda 2021-03-11 23:40 ` Laurent Pinchart 2021-03-12 7:14 ` Hans Verkuil 2021-03-11 22:19 ` [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS Ricardo Ribalda 2021-03-11 22:19 ` [PATCH v2 6/6] media: uvcvideo: Set a different name for the metadata entity Ricardo Ribalda 5 siblings, 2 replies; 24+ messages in thread From: Ricardo Ribalda @ 2021-03-11 22:19 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Cc: Ricardo Ribalda According to the doc: The, in hindsight quite poor, solution for that is to set error_idx to count if the validation failed. Fixes v4l2-compliance: Control ioctls (Input 0): fail: v4l2-test-controls.cpp(645): invalid error index write only control test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 157310c0ca87..36eb48622d48 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -1073,7 +1073,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, ret = uvc_ctrl_get(chain, ctrl); if (ret < 0) { uvc_ctrl_rollback(handle); - ctrls->error_idx = i; + ctrls->error_idx = (ret == -EACCES) ? + ctrls->count : i; return ret; } } -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS 2021-03-11 22:19 ` [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS Ricardo Ribalda @ 2021-03-11 23:40 ` Laurent Pinchart 2021-03-12 7:14 ` Hans Verkuil 1 sibling, 0 replies; 24+ messages in thread From: Laurent Pinchart @ 2021-03-11 23:40 UTC (permalink / raw) To: Ricardo Ribalda Cc: Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Hi Ricardo, Thank you for the patch. On Thu, Mar 11, 2021 at 11:19:44PM +0100, Ricardo Ribalda wrote: > According to the doc: > The, in hindsight quite poor, solution for that is to set error_idx to > count if the validation failed. > > Fixes v4l2-compliance: > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(645): invalid error index write only control > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> I'll hold off on this one until we conclude the discussion with Hans on v1. > --- > drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 157310c0ca87..36eb48622d48 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -1073,7 +1073,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, > ret = uvc_ctrl_get(chain, ctrl); > if (ret < 0) { > uvc_ctrl_rollback(handle); > - ctrls->error_idx = i; > + ctrls->error_idx = (ret == -EACCES) ? > + ctrls->count : i; > return ret; > } > } -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS 2021-03-11 22:19 ` [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS Ricardo Ribalda 2021-03-11 23:40 ` Laurent Pinchart @ 2021-03-12 7:14 ` Hans Verkuil 2021-03-12 10:18 ` Laurent Pinchart 1 sibling, 1 reply; 24+ messages in thread From: Hans Verkuil @ 2021-03-12 7:14 UTC (permalink / raw) To: Ricardo Ribalda, Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky On 11/03/2021 23:19, Ricardo Ribalda wrote: > According to the doc: > The, in hindsight quite poor, solution for that is to set error_idx to > count if the validation failed. I think this needs a bit more explanation. How about this: "If an error is found when validating the list of controls passed with VIDIOC_G_EXT_CTRLS, then error_idx shall be set to ctrls->count to indicate to userspace that no actual hardware was touched. It would have been much nicer of course if error_idx could point to the control index that failed the validation, but sadly that's not how the API was designed." > > Fixes v4l2-compliance: > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(645): invalid error index write only control > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> After improving the commit log you can add my: Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Thanks! Hans > --- > drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 157310c0ca87..36eb48622d48 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -1073,7 +1073,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, > ret = uvc_ctrl_get(chain, ctrl); > if (ret < 0) { > uvc_ctrl_rollback(handle); > - ctrls->error_idx = i; > + ctrls->error_idx = (ret == -EACCES) ? > + ctrls->count : i; > return ret; > } > } > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS 2021-03-12 7:14 ` Hans Verkuil @ 2021-03-12 10:18 ` Laurent Pinchart 0 siblings, 0 replies; 24+ messages in thread From: Laurent Pinchart @ 2021-03-12 10:18 UTC (permalink / raw) To: Hans Verkuil Cc: Ricardo Ribalda, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky Hi Hans, On Fri, Mar 12, 2021 at 08:14:13AM +0100, Hans Verkuil wrote: > On 11/03/2021 23:19, Ricardo Ribalda wrote: > > According to the doc: > > The, in hindsight quite poor, solution for that is to set error_idx to > > count if the validation failed. > > I think this needs a bit more explanation. How about this: > > "If an error is found when validating the list of controls passed with > VIDIOC_G_EXT_CTRLS, then error_idx shall be set to ctrls->count to indicate > to userspace that no actual hardware was touched. > > It would have been much nicer of course if error_idx could point to the > control index that failed the validation, but sadly that's not how the > API was designed." Here's what I commented on v1: The [V4L2 spec] states: "This check is done to avoid leaving the hardware in an inconsistent state due to easy-to-avoid problems. But it leads to another problem: the application needs to know whether an error came from the validation step (meaning that the hardware was not touched) or from an error during the actual reading from/writing to hardware." I'm not sure this is correct though. -EACCES is returned by __uvc_ctrl_get() when the control is found and is a write-only control. The uvc_ctrl_get() calls for the previous controls will have potentially touched the device to read the current control value if it wasn't cached already, to this contradicts the rationale from the specification. I understand the need for this when setting controls, but when reading them, it's more puzzling, as the interactions with the hardware to read the controls are not supposed to affect the hardware state in a way that applications should care about. It may be an issue in the V4L2 specification. > > > > Fixes v4l2-compliance: > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(645): invalid error index write only control > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > After improving the commit log you can add my: > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > > --- > > drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 157310c0ca87..36eb48622d48 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -1073,7 +1073,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, > > ret = uvc_ctrl_get(chain, ctrl); > > if (ret < 0) { > > uvc_ctrl_rollback(handle); > > - ctrls->error_idx = i; > > + ctrls->error_idx = (ret == -EACCES) ? > > + ctrls->count : i; > > return ret; > > } > > } -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS 2021-03-11 22:19 [PATCH v2 0/6] uvcvideo: Fix v4l2-compliance errors Ricardo Ribalda ` (3 preceding siblings ...) 2021-03-11 22:19 ` [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS Ricardo Ribalda @ 2021-03-11 22:19 ` Ricardo Ribalda 2021-03-12 1:21 ` Laurent Pinchart 2021-03-11 22:19 ` [PATCH v2 6/6] media: uvcvideo: Set a different name for the metadata entity Ricardo Ribalda 5 siblings, 1 reply; 24+ messages in thread From: Ricardo Ribalda @ 2021-03-11 22:19 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Cc: Ricardo Ribalda Create all the class controls for the device defined controls. Fixes v4l2-compliance: Control ioctls (Input 0): fail: v4l2-test-controls.cpp(216): missing control class for class 00980000 fail: v4l2-test-controls.cpp(216): missing control tclass for class 009a0000 test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 90 ++++++++++++++++++++++++++++++++ drivers/media/usb/uvc/uvcvideo.h | 7 +++ 2 files changed, 97 insertions(+) diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index b3dde98499f4..4e0ed2595ae9 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -357,6 +357,17 @@ static const struct uvc_control_info uvc_ctrls[] = { }, }; +static const struct uvc_control_class uvc_control_class[] = { + { + .id = V4L2_CID_CAMERA_CLASS, + .name = "Camera Controls", + }, + { + .id = V4L2_CID_USER_CLASS, + .name = "User Controls", + }, +}; + static const struct uvc_menu_info power_line_frequency_controls[] = { { 0, "Disabled" }, { 1, "50 Hz" }, @@ -1024,6 +1035,49 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, return 0; } +static int __uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, + u32 found_id) +{ + bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; + int i; + + req_id &= V4L2_CTRL_ID_MASK; + + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { + if (!(dev->ctrl_class_bitmap & BIT(i))) + continue; + if (!find_next) { + if (uvc_control_class[i].id == req_id) + return i; + continue; + } + if ((uvc_control_class[i].id > req_id) && + (uvc_control_class[i].id < found_id)) + return i; + } + + return -ENODEV; +} + +static int uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, + u32 found_id, struct v4l2_queryctrl *v4l2_ctrl) +{ + int idx; + + idx = __uvc_query_v4l2_class(dev, req_id, found_id); + if (idx < 0) + return -ENODEV; + + memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl)); + v4l2_ctrl->id = uvc_control_class[idx].id; + strscpy(v4l2_ctrl->name, uvc_control_class[idx].name, + sizeof(v4l2_ctrl->name)); + v4l2_ctrl->type = V4L2_CTRL_TYPE_CTRL_CLASS; + v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY | + V4L2_CTRL_FLAG_READ_ONLY; + return 0; +} + static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, struct uvc_control *ctrl, struct uvc_control_mapping *mapping, @@ -1123,6 +1177,14 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, struct uvc_control_mapping *mapping; int ret; + /* Check if the ctrl is a know class */ + if (!(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)) { + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, + v4l2_ctrl->id, v4l2_ctrl); + if (!ret) + return 0; + } + ret = mutex_lock_interruptible(&chain->ctrl_mutex); if (ret < 0) return -ERESTARTSYS; @@ -1133,6 +1195,13 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, goto done; } + if (v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL) { + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, + mapping->id, v4l2_ctrl); + if (!ret) + goto done; + } + ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl); done: mutex_unlock(&chain->ctrl_mutex); @@ -1422,6 +1491,9 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) struct uvc_control *ctrl; int ret; + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) + return 0; + ret = mutex_lock_interruptible(&handle->chain->ctrl_mutex); if (ret < 0) return -ERESTARTSYS; @@ -1458,6 +1530,9 @@ static void uvc_ctrl_del_event(struct v4l2_subscribed_event *sev) { struct uvc_fh *handle = container_of(sev->fh, struct uvc_fh, vfh); + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) + return; + mutex_lock(&handle->chain->ctrl_mutex); list_del(&sev->node); mutex_unlock(&handle->chain->ctrl_mutex); @@ -1577,6 +1652,9 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, struct uvc_control *ctrl; struct uvc_control_mapping *mapping; + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) + return -EACCES; + ctrl = uvc_find_control(chain, xctrl->id, &mapping); if (ctrl == NULL) return -EINVAL; @@ -1596,6 +1674,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, s32 max; int ret; + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) + return -EACCES; + ctrl = uvc_find_control(chain, xctrl->id, &mapping); if (ctrl == NULL) return -EINVAL; @@ -2062,6 +2143,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, { struct uvc_control_mapping *map; unsigned int size; + int i; /* Most mappings come from static kernel data and need to be duplicated. * Mappings that come from userspace will be unnecessarily duplicated, @@ -2085,6 +2167,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, if (map->set == NULL) map->set = uvc_set_le_value; + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { + if (V4L2_CTRL_ID2WHICH(uvc_control_class[i].id) == + V4L2_CTRL_ID2WHICH(map->id)) { + dev->ctrl_class_bitmap |= BIT(i); + break; + } + } + list_add_tail(&map->list, &ctrl->info.mappings); uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", map->name, ctrl->info.entity, ctrl->info.selector); diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 97df5ecd66c9..63b5d697a438 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -262,6 +262,11 @@ struct uvc_control_mapping { u8 *data); }; +struct uvc_control_class { + u32 id; + char name[32]; +}; + struct uvc_control { struct uvc_entity *entity; struct uvc_control_info info; @@ -707,6 +712,8 @@ struct uvc_device { } async_ctrl; struct uvc_entity *gpio_unit; + + u8 ctrl_class_bitmap; }; enum uvc_handle_state { -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS 2021-03-11 22:19 ` [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS Ricardo Ribalda @ 2021-03-12 1:21 ` Laurent Pinchart 2021-03-12 9:57 ` Ricardo Ribalda Delgado 0 siblings, 1 reply; 24+ messages in thread From: Laurent Pinchart @ 2021-03-12 1:21 UTC (permalink / raw) To: Ricardo Ribalda Cc: Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Hi Ricardo, Thank you for the patch. On Thu, Mar 11, 2021 at 11:19:45PM +0100, Ricardo Ribalda wrote: > Create all the class controls for the device defined controls. > > Fixes v4l2-compliance: > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(216): missing control class for class 00980000 > fail: v4l2-test-controls.cpp(216): missing control tclass for class 009a0000 > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 90 ++++++++++++++++++++++++++++++++ > drivers/media/usb/uvc/uvcvideo.h | 7 +++ > 2 files changed, 97 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index b3dde98499f4..4e0ed2595ae9 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -357,6 +357,17 @@ static const struct uvc_control_info uvc_ctrls[] = { > }, > }; > > +static const struct uvc_control_class uvc_control_class[] = { > + { > + .id = V4L2_CID_CAMERA_CLASS, > + .name = "Camera Controls", > + }, > + { > + .id = V4L2_CID_USER_CLASS, > + .name = "User Controls", > + }, > +}; > + > static const struct uvc_menu_info power_line_frequency_controls[] = { > { 0, "Disabled" }, > { 1, "50 Hz" }, > @@ -1024,6 +1035,49 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, > return 0; > } > > +static int __uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, > + u32 found_id) > +{ > + bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; > + int i; unsigned int as i will never be negative. > + > + req_id &= V4L2_CTRL_ID_MASK; > + > + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { > + if (!(dev->ctrl_class_bitmap & BIT(i))) > + continue; > + if (!find_next) { > + if (uvc_control_class[i].id == req_id) > + return i; > + continue; > + } > + if ((uvc_control_class[i].id > req_id) && > + (uvc_control_class[i].id < found_id)) No need for the inner parentheses. > + return i; > + } > + > + return -ENODEV; > +} > + > +static int uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, > + u32 found_id, struct v4l2_queryctrl *v4l2_ctrl) > +{ > + int idx; > + > + idx = __uvc_query_v4l2_class(dev, req_id, found_id); > + if (idx < 0) > + return -ENODEV; > + > + memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl)); > + v4l2_ctrl->id = uvc_control_class[idx].id; > + strscpy(v4l2_ctrl->name, uvc_control_class[idx].name, > + sizeof(v4l2_ctrl->name)); > + v4l2_ctrl->type = V4L2_CTRL_TYPE_CTRL_CLASS; > + v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY | > + V4L2_CTRL_FLAG_READ_ONLY; v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY | V4L2_CTRL_FLAG_READ_ONLY; > + return 0; > +} If you agree with the comments below, you could inline __uvc_query_v4l2_class() in uvc_query_v4l2_class() as it won't be called separately. > + > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > struct uvc_control *ctrl, > struct uvc_control_mapping *mapping, > @@ -1123,6 +1177,14 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > struct uvc_control_mapping *mapping; > int ret; > > + /* Check if the ctrl is a know class */ > + if (!(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)) { > + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, > + v4l2_ctrl->id, v4l2_ctrl); You could pass 0 for found_id here. > + if (!ret) > + return 0; > + } > + Should this be done with the chain->ctrl_mutex locked, as __uvc_query_v4l2_class() accesses dev->ctrl_class_bitmap that could be modified concurrently ? > ret = mutex_lock_interruptible(&chain->ctrl_mutex); > if (ret < 0) > return -ERESTARTSYS; > @@ -1133,6 +1195,13 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > goto done; > } > A comment here along the lines of /* * If we're enumerating control with V4L2_CTRL_FLAG_NEXT_CTRL, check if * a class should be inserted between the previous control and the one * we have just found. */ could be useful, as it's not trivial. > + if (v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL) { > + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, > + mapping->id, v4l2_ctrl); > + if (!ret) > + goto done; > + } > + > ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl); > done: > mutex_unlock(&chain->ctrl_mutex); > @@ -1422,6 +1491,9 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) > struct uvc_control *ctrl; > int ret; > > + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) > + return 0; Do we really need to succeed ? What's the point in subscribing for control change events on a class ? Can't we just check if sev->id is a class, and return -EINVAL in that case ? > + > ret = mutex_lock_interruptible(&handle->chain->ctrl_mutex); > if (ret < 0) > return -ERESTARTSYS; > @@ -1458,6 +1530,9 @@ static void uvc_ctrl_del_event(struct v4l2_subscribed_event *sev) > { > struct uvc_fh *handle = container_of(sev->fh, struct uvc_fh, vfh); > > + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) > + return; And this could then be dropped, as this function won't be called if the subscription failed. > + > mutex_lock(&handle->chain->ctrl_mutex); > list_del(&sev->node); > mutex_unlock(&handle->chain->ctrl_mutex); > @@ -1577,6 +1652,9 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > struct uvc_control *ctrl; > struct uvc_control_mapping *mapping; > > + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) > + return -EACCES; > + > ctrl = uvc_find_control(chain, xctrl->id, &mapping); > if (ctrl == NULL) > return -EINVAL; > @@ -1596,6 +1674,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, > s32 max; > int ret; > > + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) > + return -EACCES; > + Similarly as in patch 1/6, should these two checks be moved to v4l_s_ctrl() and v4l_s_ext_ctrls(), as it's never valid to get/set a class ? > ctrl = uvc_find_control(chain, xctrl->id, &mapping); > if (ctrl == NULL) > return -EINVAL; > @@ -2062,6 +2143,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > { > struct uvc_control_mapping *map; > unsigned int size; > + int i; This can be unsigned as i never takes negative values. > > /* Most mappings come from static kernel data and need to be duplicated. > * Mappings that come from userspace will be unnecessarily duplicated, > @@ -2085,6 +2167,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > if (map->set == NULL) > map->set = uvc_set_le_value; > > + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { > + if (V4L2_CTRL_ID2WHICH(uvc_control_class[i].id) == > + V4L2_CTRL_ID2WHICH(map->id)) { You can write this if (uvc_control_class[i].id == V4L2_CTRL_ID2WHICH(map->id)) { as the uvc_control_class array contains control classes only. > + dev->ctrl_class_bitmap |= BIT(i); > + break; > + } > + } > + > list_add_tail(&map->list, &ctrl->info.mappings); > uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", > map->name, ctrl->info.entity, ctrl->info.selector); > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 97df5ecd66c9..63b5d697a438 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -262,6 +262,11 @@ struct uvc_control_mapping { > u8 *data); > }; > > +struct uvc_control_class { > + u32 id; > + char name[32]; > +}; > + > struct uvc_control { > struct uvc_entity *entity; > struct uvc_control_info info; > @@ -707,6 +712,8 @@ struct uvc_device { > } async_ctrl; > > struct uvc_entity *gpio_unit; > + > + u8 ctrl_class_bitmap; Should this be stored in the chain, as different chains can have different controls ? > }; > > enum uvc_handle_state { -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS 2021-03-12 1:21 ` Laurent Pinchart @ 2021-03-12 9:57 ` Ricardo Ribalda Delgado 2021-03-12 10:13 ` Laurent Pinchart 0 siblings, 1 reply; 24+ messages in thread From: Ricardo Ribalda Delgado @ 2021-03-12 9:57 UTC (permalink / raw) To: Laurent Pinchart Cc: Ricardo Ribalda, Mauro Carvalho Chehab, Tomasz Figa, linux-media, LKML, Sergey Senozhatsky, Hans Verkuil HI Laurent Thanks for the prompt reply :) On Fri, Mar 12, 2021 at 2:25 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Thu, Mar 11, 2021 at 11:19:45PM +0100, Ricardo Ribalda wrote: > > Create all the class controls for the device defined controls. > > > > Fixes v4l2-compliance: > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(216): missing control class for class 00980000 > > fail: v4l2-test-controls.cpp(216): missing control tclass for class 009a0000 > > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 90 ++++++++++++++++++++++++++++++++ > > drivers/media/usb/uvc/uvcvideo.h | 7 +++ > > 2 files changed, 97 insertions(+) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index b3dde98499f4..4e0ed2595ae9 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -357,6 +357,17 @@ static const struct uvc_control_info uvc_ctrls[] = { > > }, > > }; > > > > +static const struct uvc_control_class uvc_control_class[] = { > > + { > > + .id = V4L2_CID_CAMERA_CLASS, > > + .name = "Camera Controls", > > + }, > > + { > > + .id = V4L2_CID_USER_CLASS, > > + .name = "User Controls", > > + }, > > +}; > > + > > static const struct uvc_menu_info power_line_frequency_controls[] = { > > { 0, "Disabled" }, > > { 1, "50 Hz" }, > > @@ -1024,6 +1035,49 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > return 0; > > } > > > > +static int __uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, > > + u32 found_id) > > +{ > > + bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; > > + int i; > > unsigned int as i will never be negative. Sometimes you are a bit negative with my patches... :) (sorry, it is Friday) > > > + > > + req_id &= V4L2_CTRL_ID_MASK; > > + > > + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { > > + if (!(dev->ctrl_class_bitmap & BIT(i))) > > + continue; > > + if (!find_next) { > > + if (uvc_control_class[i].id == req_id) > > + return i; > > + continue; > > + } > > + if ((uvc_control_class[i].id > req_id) && > > + (uvc_control_class[i].id < found_id)) > > No need for the inner parentheses. > > > + return i; > > + } > > + > > + return -ENODEV; > > +} > > + > > +static int uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, > > + u32 found_id, struct v4l2_queryctrl *v4l2_ctrl) > > +{ > > + int idx; > > + > > + idx = __uvc_query_v4l2_class(dev, req_id, found_id); > > + if (idx < 0) > > + return -ENODEV; > > + > > + memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl)); > > + v4l2_ctrl->id = uvc_control_class[idx].id; > > + strscpy(v4l2_ctrl->name, uvc_control_class[idx].name, > > + sizeof(v4l2_ctrl->name)); > > + v4l2_ctrl->type = V4L2_CTRL_TYPE_CTRL_CLASS; > > + v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY | > > + V4L2_CTRL_FLAG_READ_ONLY; > > v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY > | V4L2_CTRL_FLAG_READ_ONLY; > > > + return 0; > > +} > > If you agree with the comments below, you could inline > __uvc_query_v4l2_class() in uvc_query_v4l2_class() as it won't be called > separately. > > > + > > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > struct uvc_control *ctrl, > > struct uvc_control_mapping *mapping, > > @@ -1123,6 +1177,14 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > struct uvc_control_mapping *mapping; > > int ret; > > > > + /* Check if the ctrl is a know class */ > > + if (!(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)) { > > + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, > > + v4l2_ctrl->id, v4l2_ctrl); > > You could pass 0 for found_id here. > > > + if (!ret) > > + return 0; > > + } > > + > > Should this be done with the chain->ctrl_mutex locked, as > __uvc_query_v4l2_class() accesses dev->ctrl_class_bitmap that could be > modified concurrently ? > > > ret = mutex_lock_interruptible(&chain->ctrl_mutex); > > if (ret < 0) > > return -ERESTARTSYS; > > @@ -1133,6 +1195,13 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > goto done; > > } > > > > A comment here along the lines of > > /* > * If we're enumerating control with V4L2_CTRL_FLAG_NEXT_CTRL, check if > * a class should be inserted between the previous control and the one > * we have just found. > */ > > could be useful, as it's not trivial. yes, it looks better thanks! > > > + if (v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL) { > > + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, > > + mapping->id, v4l2_ctrl); > > + if (!ret) > > + goto done; > > + } > > + > > ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl); > > done: > > mutex_unlock(&chain->ctrl_mutex); > > @@ -1422,6 +1491,9 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) > > struct uvc_control *ctrl; > > int ret; > > > > + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) > > + return 0; > > Do we really need to succeed ? What's the point in subscribing for > control change events on a class ? Can't we just check if sev->id is a > class, and return -EINVAL in that case ? Unfortunately it is expected that you can subscribe to all the events, even the ctrl_classes test VIDIOC_G/S/TRY_EXT_CTRLS: OK fail: v4l2-test-controls.cpp(835): subscribe event for control 'User Controls' failed test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL > > > + > > ret = mutex_lock_interruptible(&handle->chain->ctrl_mutex); > > if (ret < 0) > > return -ERESTARTSYS; > > @@ -1458,6 +1530,9 @@ static void uvc_ctrl_del_event(struct v4l2_subscribed_event *sev) > > { > > struct uvc_fh *handle = container_of(sev->fh, struct uvc_fh, vfh); > > > > + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) > > + return; > > And this could then be dropped, as this function won't be called if the > subscription failed. > > > + > > mutex_lock(&handle->chain->ctrl_mutex); > > list_del(&sev->node); > > mutex_unlock(&handle->chain->ctrl_mutex); > > @@ -1577,6 +1652,9 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > > struct uvc_control *ctrl; > > struct uvc_control_mapping *mapping; > > > > + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) > > + return -EACCES; > > + > > ctrl = uvc_find_control(chain, xctrl->id, &mapping); > > if (ctrl == NULL) > > return -EINVAL; > > @@ -1596,6 +1674,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > s32 max; > > int ret; > > > > + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) > > + return -EACCES; > > + > > Similarly as in patch 1/6, should these two checks be moved to > v4l_s_ctrl() and v4l_s_ext_ctrls(), as it's never valid to get/set a > class ? I do not think that it is possible, you need to return -EACCESS if the control exists and -EINVAL if it does not exist. v4l_s_ext_ctrls does not know if the ctrl exists. > > > ctrl = uvc_find_control(chain, xctrl->id, &mapping); > > if (ctrl == NULL) > > return -EINVAL; > > @@ -2062,6 +2143,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > > { > > struct uvc_control_mapping *map; > > unsigned int size; > > + int i; > > This can be unsigned as i never takes negative values. I cannot repeat the same joke... even if it is a bad joke > > > > > /* Most mappings come from static kernel data and need to be duplicated. > > * Mappings that come from userspace will be unnecessarily duplicated, > > @@ -2085,6 +2167,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > > if (map->set == NULL) > > map->set = uvc_set_le_value; > > > > + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { > > + if (V4L2_CTRL_ID2WHICH(uvc_control_class[i].id) == > > + V4L2_CTRL_ID2WHICH(map->id)) { > > You can write this > > if (uvc_control_class[i].id == V4L2_CTRL_ID2WHICH(map->id)) { > > as the uvc_control_class array contains control classes only. Are you sure? #define V4L2_CID_CAMERA_CLASS (V4L2_CTRL_CLASS_CAMERA | 1) we are sasing the cid, not the class. > > > + dev->ctrl_class_bitmap |= BIT(i); > > + break; > > + } > > + } > > + > > list_add_tail(&map->list, &ctrl->info.mappings); > > uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", > > map->name, ctrl->info.entity, ctrl->info.selector); > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 97df5ecd66c9..63b5d697a438 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -262,6 +262,11 @@ struct uvc_control_mapping { > > u8 *data); > > }; > > > > +struct uvc_control_class { > > + u32 id; > > + char name[32]; > > +}; > > + > > struct uvc_control { > > struct uvc_entity *entity; > > struct uvc_control_info info; > > @@ -707,6 +712,8 @@ struct uvc_device { > > } async_ctrl; > > > > struct uvc_entity *gpio_unit; > > + > > + u8 ctrl_class_bitmap; > > Should this be stored in the chain, as different chains can have > different controls ? > > > }; > > > > enum uvc_handle_state { > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS 2021-03-12 9:57 ` Ricardo Ribalda Delgado @ 2021-03-12 10:13 ` Laurent Pinchart 2021-03-12 10:22 ` Hans Verkuil 0 siblings, 1 reply; 24+ messages in thread From: Laurent Pinchart @ 2021-03-12 10:13 UTC (permalink / raw) To: Ricardo Ribalda Delgado Cc: Ricardo Ribalda, Mauro Carvalho Chehab, Tomasz Figa, linux-media, LKML, Sergey Senozhatsky, Hans Verkuil Hi Ricardo, On Fri, Mar 12, 2021 at 10:57:33AM +0100, Ricardo Ribalda Delgado wrote: > On Fri, Mar 12, 2021 at 2:25 AM Laurent Pinchart wrote: > > On Thu, Mar 11, 2021 at 11:19:45PM +0100, Ricardo Ribalda wrote: > > > Create all the class controls for the device defined controls. > > > > > > Fixes v4l2-compliance: > > > Control ioctls (Input 0): > > > fail: v4l2-test-controls.cpp(216): missing control class for class 00980000 > > > fail: v4l2-test-controls.cpp(216): missing control tclass for class 009a0000 > > > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > drivers/media/usb/uvc/uvc_ctrl.c | 90 ++++++++++++++++++++++++++++++++ > > > drivers/media/usb/uvc/uvcvideo.h | 7 +++ > > > 2 files changed, 97 insertions(+) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > index b3dde98499f4..4e0ed2595ae9 100644 > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > @@ -357,6 +357,17 @@ static const struct uvc_control_info uvc_ctrls[] = { > > > }, > > > }; > > > > > > +static const struct uvc_control_class uvc_control_class[] = { > > > + { > > > + .id = V4L2_CID_CAMERA_CLASS, > > > + .name = "Camera Controls", > > > + }, > > > + { > > > + .id = V4L2_CID_USER_CLASS, > > > + .name = "User Controls", > > > + }, > > > +}; > > > + > > > static const struct uvc_menu_info power_line_frequency_controls[] = { > > > { 0, "Disabled" }, > > > { 1, "50 Hz" }, > > > @@ -1024,6 +1035,49 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > return 0; > > > } > > > > > > +static int __uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, > > > + u32 found_id) > > > +{ > > > + bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; > > > + int i; > > > > unsigned int as i will never be negative. > > Sometimes you are a bit negative with my patches... :) > > (sorry, it is Friday) > > > > + > > > + req_id &= V4L2_CTRL_ID_MASK; > > > + > > > + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { > > > + if (!(dev->ctrl_class_bitmap & BIT(i))) > > > + continue; > > > + if (!find_next) { > > > + if (uvc_control_class[i].id == req_id) > > > + return i; > > > + continue; > > > + } > > > + if ((uvc_control_class[i].id > req_id) && > > > + (uvc_control_class[i].id < found_id)) > > > > No need for the inner parentheses. > > > > > + return i; > > > + } > > > + > > > + return -ENODEV; > > > +} > > > + > > > +static int uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, > > > + u32 found_id, struct v4l2_queryctrl *v4l2_ctrl) > > > +{ > > > + int idx; > > > + > > > + idx = __uvc_query_v4l2_class(dev, req_id, found_id); > > > + if (idx < 0) > > > + return -ENODEV; > > > + > > > + memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl)); > > > + v4l2_ctrl->id = uvc_control_class[idx].id; > > > + strscpy(v4l2_ctrl->name, uvc_control_class[idx].name, > > > + sizeof(v4l2_ctrl->name)); > > > + v4l2_ctrl->type = V4L2_CTRL_TYPE_CTRL_CLASS; > > > + v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY | > > > + V4L2_CTRL_FLAG_READ_ONLY; > > > > v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY > > | V4L2_CTRL_FLAG_READ_ONLY; > > > > > + return 0; > > > +} > > > > If you agree with the comments below, you could inline > > __uvc_query_v4l2_class() in uvc_query_v4l2_class() as it won't be called > > separately. > > > > > + > > > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > struct uvc_control *ctrl, > > > struct uvc_control_mapping *mapping, > > > @@ -1123,6 +1177,14 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > struct uvc_control_mapping *mapping; > > > int ret; > > > > > > + /* Check if the ctrl is a know class */ > > > + if (!(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)) { > > > + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, > > > + v4l2_ctrl->id, v4l2_ctrl); > > > > You could pass 0 for found_id here. > > > > > + if (!ret) > > > + return 0; > > > + } > > > + > > > > Should this be done with the chain->ctrl_mutex locked, as > > __uvc_query_v4l2_class() accesses dev->ctrl_class_bitmap that could be > > modified concurrently ? > > > > > ret = mutex_lock_interruptible(&chain->ctrl_mutex); > > > if (ret < 0) > > > return -ERESTARTSYS; > > > @@ -1133,6 +1195,13 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > goto done; > > > } > > > > > > > A comment here along the lines of > > > > /* > > * If we're enumerating control with V4L2_CTRL_FLAG_NEXT_CTRL, check if > > * a class should be inserted between the previous control and the one > > * we have just found. > > */ > > > > could be useful, as it's not trivial. > > yes, it looks better thanks! > > > > + if (v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL) { > > > + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, > > > + mapping->id, v4l2_ctrl); > > > + if (!ret) > > > + goto done; > > > + } > > > + > > > ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl); > > > done: > > > mutex_unlock(&chain->ctrl_mutex); > > > @@ -1422,6 +1491,9 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) > > > struct uvc_control *ctrl; > > > int ret; > > > > > > + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) > > > + return 0; > > > > Do we really need to succeed ? What's the point in subscribing for > > control change events on a class ? Can't we just check if sev->id is a > > class, and return -EINVAL in that case ? > > Unfortunately it is expected that you can subscribe to all the events, > even the ctrl_classes > test VIDIOC_G/S/TRY_EXT_CTRLS: OK > fail: v4l2-test-controls.cpp(835): subscribe event for > control 'User Controls' failed > test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL Looks like something that should be dropped from v4l2-compliance, there's no use case for subscribing to a class. > > > + > > > ret = mutex_lock_interruptible(&handle->chain->ctrl_mutex); > > > if (ret < 0) > > > return -ERESTARTSYS; > > > @@ -1458,6 +1530,9 @@ static void uvc_ctrl_del_event(struct v4l2_subscribed_event *sev) > > > { > > > struct uvc_fh *handle = container_of(sev->fh, struct uvc_fh, vfh); > > > > > > + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) > > > + return; > > > > And this could then be dropped, as this function won't be called if the > > subscription failed. > > > > > + > > > mutex_lock(&handle->chain->ctrl_mutex); > > > list_del(&sev->node); > > > mutex_unlock(&handle->chain->ctrl_mutex); > > > @@ -1577,6 +1652,9 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > > > struct uvc_control *ctrl; > > > struct uvc_control_mapping *mapping; > > > > > > + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) > > > + return -EACCES; > > > + > > > ctrl = uvc_find_control(chain, xctrl->id, &mapping); > > > if (ctrl == NULL) > > > return -EINVAL; > > > @@ -1596,6 +1674,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > s32 max; > > > int ret; > > > > > > + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) > > > + return -EACCES; > > > + > > > > Similarly as in patch 1/6, should these two checks be moved to > > v4l_s_ctrl() and v4l_s_ext_ctrls(), as it's never valid to get/set a > > class ? > > I do not think that it is possible, you need to return -EACCESS if the > control exists and -EINVAL if it does not exist. > v4l_s_ext_ctrls does not know if the ctrl exists. *sigh* I'm sad that we need this kind of complexity in drivers because the API requires us to implement a behaviour that nobody actually cares about :-( The way classes are implemented is really a big hack. > > > ctrl = uvc_find_control(chain, xctrl->id, &mapping); > > > if (ctrl == NULL) > > > return -EINVAL; > > > @@ -2062,6 +2143,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > > > { > > > struct uvc_control_mapping *map; > > > unsigned int size; > > > + int i; > > > > This can be unsigned as i never takes negative values. > I cannot repeat the same joke... even if it is a bad joke > > > > > > > > /* Most mappings come from static kernel data and need to be duplicated. > > > * Mappings that come from userspace will be unnecessarily duplicated, > > > @@ -2085,6 +2167,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > > > if (map->set == NULL) > > > map->set = uvc_set_le_value; > > > > > > + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { > > > + if (V4L2_CTRL_ID2WHICH(uvc_control_class[i].id) == > > > + V4L2_CTRL_ID2WHICH(map->id)) { > > > > You can write this > > > > if (uvc_control_class[i].id == V4L2_CTRL_ID2WHICH(map->id)) { > > > > as the uvc_control_class array contains control classes only. > > Are you sure? > #define V4L2_CID_CAMERA_CLASS (V4L2_CTRL_CLASS_CAMERA | 1) > > we are sasing the cid, not the class. Indeed, my bad. > > > + dev->ctrl_class_bitmap |= BIT(i); > > > + break; > > > + } > > > + } > > > + > > > list_add_tail(&map->list, &ctrl->info.mappings); > > > uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", > > > map->name, ctrl->info.entity, ctrl->info.selector); > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > index 97df5ecd66c9..63b5d697a438 100644 > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > @@ -262,6 +262,11 @@ struct uvc_control_mapping { > > > u8 *data); > > > }; > > > > > > +struct uvc_control_class { > > > + u32 id; > > > + char name[32]; > > > +}; > > > + > > > struct uvc_control { > > > struct uvc_entity *entity; > > > struct uvc_control_info info; > > > @@ -707,6 +712,8 @@ struct uvc_device { > > > } async_ctrl; > > > > > > struct uvc_entity *gpio_unit; > > > + > > > + u8 ctrl_class_bitmap; > > > > Should this be stored in the chain, as different chains can have > > different controls ? > > > > > }; > > > > > > enum uvc_handle_state { -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS 2021-03-12 10:13 ` Laurent Pinchart @ 2021-03-12 10:22 ` Hans Verkuil 2021-03-12 10:56 ` Laurent Pinchart 0 siblings, 1 reply; 24+ messages in thread From: Hans Verkuil @ 2021-03-12 10:22 UTC (permalink / raw) To: Laurent Pinchart, Ricardo Ribalda Delgado Cc: Ricardo Ribalda, Mauro Carvalho Chehab, Tomasz Figa, linux-media, LKML, Sergey Senozhatsky On 12/03/2021 11:13, Laurent Pinchart wrote: > Hi Ricardo, > > On Fri, Mar 12, 2021 at 10:57:33AM +0100, Ricardo Ribalda Delgado wrote: >> On Fri, Mar 12, 2021 at 2:25 AM Laurent Pinchart wrote: >>> On Thu, Mar 11, 2021 at 11:19:45PM +0100, Ricardo Ribalda wrote: >>>> Create all the class controls for the device defined controls. >>>> >>>> Fixes v4l2-compliance: >>>> Control ioctls (Input 0): >>>> fail: v4l2-test-controls.cpp(216): missing control class for class 00980000 >>>> fail: v4l2-test-controls.cpp(216): missing control tclass for class 009a0000 >>>> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL >>>> >>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>>> --- >>>> drivers/media/usb/uvc/uvc_ctrl.c | 90 ++++++++++++++++++++++++++++++++ >>>> drivers/media/usb/uvc/uvcvideo.h | 7 +++ >>>> 2 files changed, 97 insertions(+) >>>> >>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >>>> index b3dde98499f4..4e0ed2595ae9 100644 >>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c >>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >>>> @@ -357,6 +357,17 @@ static const struct uvc_control_info uvc_ctrls[] = { >>>> }, >>>> }; >>>> >>>> +static const struct uvc_control_class uvc_control_class[] = { >>>> + { >>>> + .id = V4L2_CID_CAMERA_CLASS, >>>> + .name = "Camera Controls", >>>> + }, >>>> + { >>>> + .id = V4L2_CID_USER_CLASS, >>>> + .name = "User Controls", >>>> + }, >>>> +}; >>>> + >>>> static const struct uvc_menu_info power_line_frequency_controls[] = { >>>> { 0, "Disabled" }, >>>> { 1, "50 Hz" }, >>>> @@ -1024,6 +1035,49 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, >>>> return 0; >>>> } >>>> >>>> +static int __uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, >>>> + u32 found_id) >>>> +{ >>>> + bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; >>>> + int i; >>> >>> unsigned int as i will never be negative. >> >> Sometimes you are a bit negative with my patches... :) >> >> (sorry, it is Friday) >> >>>> + >>>> + req_id &= V4L2_CTRL_ID_MASK; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { >>>> + if (!(dev->ctrl_class_bitmap & BIT(i))) >>>> + continue; >>>> + if (!find_next) { >>>> + if (uvc_control_class[i].id == req_id) >>>> + return i; >>>> + continue; >>>> + } >>>> + if ((uvc_control_class[i].id > req_id) && >>>> + (uvc_control_class[i].id < found_id)) >>> >>> No need for the inner parentheses. >>> >>>> + return i; >>>> + } >>>> + >>>> + return -ENODEV; >>>> +} >>>> + >>>> +static int uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, >>>> + u32 found_id, struct v4l2_queryctrl *v4l2_ctrl) >>>> +{ >>>> + int idx; >>>> + >>>> + idx = __uvc_query_v4l2_class(dev, req_id, found_id); >>>> + if (idx < 0) >>>> + return -ENODEV; >>>> + >>>> + memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl)); >>>> + v4l2_ctrl->id = uvc_control_class[idx].id; >>>> + strscpy(v4l2_ctrl->name, uvc_control_class[idx].name, >>>> + sizeof(v4l2_ctrl->name)); >>>> + v4l2_ctrl->type = V4L2_CTRL_TYPE_CTRL_CLASS; >>>> + v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY | >>>> + V4L2_CTRL_FLAG_READ_ONLY; >>> >>> v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY >>> | V4L2_CTRL_FLAG_READ_ONLY; >>> >>>> + return 0; >>>> +} >>> >>> If you agree with the comments below, you could inline >>> __uvc_query_v4l2_class() in uvc_query_v4l2_class() as it won't be called >>> separately. >>> >>>> + >>>> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >>>> struct uvc_control *ctrl, >>>> struct uvc_control_mapping *mapping, >>>> @@ -1123,6 +1177,14 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >>>> struct uvc_control_mapping *mapping; >>>> int ret; >>>> >>>> + /* Check if the ctrl is a know class */ >>>> + if (!(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)) { >>>> + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, >>>> + v4l2_ctrl->id, v4l2_ctrl); >>> >>> You could pass 0 for found_id here. >>> >>>> + if (!ret) >>>> + return 0; >>>> + } >>>> + >>> >>> Should this be done with the chain->ctrl_mutex locked, as >>> __uvc_query_v4l2_class() accesses dev->ctrl_class_bitmap that could be >>> modified concurrently ? >>> >>>> ret = mutex_lock_interruptible(&chain->ctrl_mutex); >>>> if (ret < 0) >>>> return -ERESTARTSYS; >>>> @@ -1133,6 +1195,13 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >>>> goto done; >>>> } >>>> >>> >>> A comment here along the lines of >>> >>> /* >>> * If we're enumerating control with V4L2_CTRL_FLAG_NEXT_CTRL, check if >>> * a class should be inserted between the previous control and the one >>> * we have just found. >>> */ >>> >>> could be useful, as it's not trivial. >> >> yes, it looks better thanks! >> >>>> + if (v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL) { >>>> + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, >>>> + mapping->id, v4l2_ctrl); >>>> + if (!ret) >>>> + goto done; >>>> + } >>>> + >>>> ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl); >>>> done: >>>> mutex_unlock(&chain->ctrl_mutex); >>>> @@ -1422,6 +1491,9 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) >>>> struct uvc_control *ctrl; >>>> int ret; >>>> >>>> + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) >>>> + return 0; >>> >>> Do we really need to succeed ? What's the point in subscribing for >>> control change events on a class ? Can't we just check if sev->id is a >>> class, and return -EINVAL in that case ? >> >> Unfortunately it is expected that you can subscribe to all the events, >> even the ctrl_classes >> test VIDIOC_G/S/TRY_EXT_CTRLS: OK >> fail: v4l2-test-controls.cpp(835): subscribe event for >> control 'User Controls' failed >> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL > > Looks like something that should be dropped from v4l2-compliance, > there's no use case for subscribing to a class. It's allowed in the API. You never get an event, since it doesn't change, but you can subscribe to it. I chose to allow it to avoid exceptions. Basically if a control never changes, you just never get an event. Whether it is a control class or a read-only control or a control with just a fixed value, it doesn't matter for the event control API. Regards, Hans > >>>> + >>>> ret = mutex_lock_interruptible(&handle->chain->ctrl_mutex); >>>> if (ret < 0) >>>> return -ERESTARTSYS; >>>> @@ -1458,6 +1530,9 @@ static void uvc_ctrl_del_event(struct v4l2_subscribed_event *sev) >>>> { >>>> struct uvc_fh *handle = container_of(sev->fh, struct uvc_fh, vfh); >>>> >>>> + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) >>>> + return; >>> >>> And this could then be dropped, as this function won't be called if the >>> subscription failed. >>> >>>> + >>>> mutex_lock(&handle->chain->ctrl_mutex); >>>> list_del(&sev->node); >>>> mutex_unlock(&handle->chain->ctrl_mutex); >>>> @@ -1577,6 +1652,9 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, >>>> struct uvc_control *ctrl; >>>> struct uvc_control_mapping *mapping; >>>> >>>> + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) >>>> + return -EACCES; >>>> + >>>> ctrl = uvc_find_control(chain, xctrl->id, &mapping); >>>> if (ctrl == NULL) >>>> return -EINVAL; >>>> @@ -1596,6 +1674,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, >>>> s32 max; >>>> int ret; >>>> >>>> + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) >>>> + return -EACCES; >>>> + >>> >>> Similarly as in patch 1/6, should these two checks be moved to >>> v4l_s_ctrl() and v4l_s_ext_ctrls(), as it's never valid to get/set a >>> class ? >> >> I do not think that it is possible, you need to return -EACCESS if the >> control exists and -EINVAL if it does not exist. >> v4l_s_ext_ctrls does not know if the ctrl exists. > > *sigh* I'm sad that we need this kind of complexity in drivers because > the API requires us to implement a behaviour that nobody actually cares > about :-( The way classes are implemented is really a big hack. > >>>> ctrl = uvc_find_control(chain, xctrl->id, &mapping); >>>> if (ctrl == NULL) >>>> return -EINVAL; >>>> @@ -2062,6 +2143,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, >>>> { >>>> struct uvc_control_mapping *map; >>>> unsigned int size; >>>> + int i; >>> >>> This can be unsigned as i never takes negative values. >> I cannot repeat the same joke... even if it is a bad joke >>> >>>> >>>> /* Most mappings come from static kernel data and need to be duplicated. >>>> * Mappings that come from userspace will be unnecessarily duplicated, >>>> @@ -2085,6 +2167,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, >>>> if (map->set == NULL) >>>> map->set = uvc_set_le_value; >>>> >>>> + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { >>>> + if (V4L2_CTRL_ID2WHICH(uvc_control_class[i].id) == >>>> + V4L2_CTRL_ID2WHICH(map->id)) { >>> >>> You can write this >>> >>> if (uvc_control_class[i].id == V4L2_CTRL_ID2WHICH(map->id)) { >>> >>> as the uvc_control_class array contains control classes only. >> >> Are you sure? >> #define V4L2_CID_CAMERA_CLASS (V4L2_CTRL_CLASS_CAMERA | 1) >> >> we are sasing the cid, not the class. > > Indeed, my bad. > >>>> + dev->ctrl_class_bitmap |= BIT(i); >>>> + break; >>>> + } >>>> + } >>>> + >>>> list_add_tail(&map->list, &ctrl->info.mappings); >>>> uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", >>>> map->name, ctrl->info.entity, ctrl->info.selector); >>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h >>>> index 97df5ecd66c9..63b5d697a438 100644 >>>> --- a/drivers/media/usb/uvc/uvcvideo.h >>>> +++ b/drivers/media/usb/uvc/uvcvideo.h >>>> @@ -262,6 +262,11 @@ struct uvc_control_mapping { >>>> u8 *data); >>>> }; >>>> >>>> +struct uvc_control_class { >>>> + u32 id; >>>> + char name[32]; >>>> +}; >>>> + >>>> struct uvc_control { >>>> struct uvc_entity *entity; >>>> struct uvc_control_info info; >>>> @@ -707,6 +712,8 @@ struct uvc_device { >>>> } async_ctrl; >>>> >>>> struct uvc_entity *gpio_unit; >>>> + >>>> + u8 ctrl_class_bitmap; >>> >>> Should this be stored in the chain, as different chains can have >>> different controls ? >>> >>>> }; >>>> >>>> enum uvc_handle_state { > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS 2021-03-12 10:22 ` Hans Verkuil @ 2021-03-12 10:56 ` Laurent Pinchart 0 siblings, 0 replies; 24+ messages in thread From: Laurent Pinchart @ 2021-03-12 10:56 UTC (permalink / raw) To: Hans Verkuil Cc: Ricardo Ribalda Delgado, Ricardo Ribalda, Mauro Carvalho Chehab, Tomasz Figa, linux-media, LKML, Sergey Senozhatsky Hi Hans, On Fri, Mar 12, 2021 at 11:22:07AM +0100, Hans Verkuil wrote: > On 12/03/2021 11:13, Laurent Pinchart wrote: > > On Fri, Mar 12, 2021 at 10:57:33AM +0100, Ricardo Ribalda Delgado wrote: > >> On Fri, Mar 12, 2021 at 2:25 AM Laurent Pinchart wrote: > >>> On Thu, Mar 11, 2021 at 11:19:45PM +0100, Ricardo Ribalda wrote: > >>>> Create all the class controls for the device defined controls. > >>>> > >>>> Fixes v4l2-compliance: > >>>> Control ioctls (Input 0): > >>>> fail: v4l2-test-controls.cpp(216): missing control class for class 00980000 > >>>> fail: v4l2-test-controls.cpp(216): missing control tclass for class 009a0000 > >>>> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL > >>>> > >>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > >>>> --- > >>>> drivers/media/usb/uvc/uvc_ctrl.c | 90 ++++++++++++++++++++++++++++++++ > >>>> drivers/media/usb/uvc/uvcvideo.h | 7 +++ > >>>> 2 files changed, 97 insertions(+) > >>>> > >>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > >>>> index b3dde98499f4..4e0ed2595ae9 100644 > >>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c > >>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c > >>>> @@ -357,6 +357,17 @@ static const struct uvc_control_info uvc_ctrls[] = { > >>>> }, > >>>> }; > >>>> > >>>> +static const struct uvc_control_class uvc_control_class[] = { > >>>> + { > >>>> + .id = V4L2_CID_CAMERA_CLASS, > >>>> + .name = "Camera Controls", > >>>> + }, > >>>> + { > >>>> + .id = V4L2_CID_USER_CLASS, > >>>> + .name = "User Controls", > >>>> + }, > >>>> +}; > >>>> + > >>>> static const struct uvc_menu_info power_line_frequency_controls[] = { > >>>> { 0, "Disabled" }, > >>>> { 1, "50 Hz" }, > >>>> @@ -1024,6 +1035,49 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain, > >>>> return 0; > >>>> } > >>>> > >>>> +static int __uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, > >>>> + u32 found_id) > >>>> +{ > >>>> + bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL; > >>>> + int i; > >>> > >>> unsigned int as i will never be negative. > >> > >> Sometimes you are a bit negative with my patches... :) > >> > >> (sorry, it is Friday) > >> > >>>> + > >>>> + req_id &= V4L2_CTRL_ID_MASK; > >>>> + > >>>> + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { > >>>> + if (!(dev->ctrl_class_bitmap & BIT(i))) > >>>> + continue; > >>>> + if (!find_next) { > >>>> + if (uvc_control_class[i].id == req_id) > >>>> + return i; > >>>> + continue; > >>>> + } > >>>> + if ((uvc_control_class[i].id > req_id) && > >>>> + (uvc_control_class[i].id < found_id)) > >>> > >>> No need for the inner parentheses. > >>> > >>>> + return i; > >>>> + } > >>>> + > >>>> + return -ENODEV; > >>>> +} > >>>> + > >>>> +static int uvc_query_v4l2_class(struct uvc_device *dev, u32 req_id, > >>>> + u32 found_id, struct v4l2_queryctrl *v4l2_ctrl) > >>>> +{ > >>>> + int idx; > >>>> + > >>>> + idx = __uvc_query_v4l2_class(dev, req_id, found_id); > >>>> + if (idx < 0) > >>>> + return -ENODEV; > >>>> + > >>>> + memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl)); > >>>> + v4l2_ctrl->id = uvc_control_class[idx].id; > >>>> + strscpy(v4l2_ctrl->name, uvc_control_class[idx].name, > >>>> + sizeof(v4l2_ctrl->name)); > >>>> + v4l2_ctrl->type = V4L2_CTRL_TYPE_CTRL_CLASS; > >>>> + v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY | > >>>> + V4L2_CTRL_FLAG_READ_ONLY; > >>> > >>> v4l2_ctrl->flags = V4L2_CTRL_FLAG_WRITE_ONLY > >>> | V4L2_CTRL_FLAG_READ_ONLY; > >>> > >>>> + return 0; > >>>> +} > >>> > >>> If you agree with the comments below, you could inline > >>> __uvc_query_v4l2_class() in uvc_query_v4l2_class() as it won't be called > >>> separately. > >>> > >>>> + > >>>> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > >>>> struct uvc_control *ctrl, > >>>> struct uvc_control_mapping *mapping, > >>>> @@ -1123,6 +1177,14 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > >>>> struct uvc_control_mapping *mapping; > >>>> int ret; > >>>> > >>>> + /* Check if the ctrl is a know class */ > >>>> + if (!(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)) { > >>>> + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, > >>>> + v4l2_ctrl->id, v4l2_ctrl); > >>> > >>> You could pass 0 for found_id here. > >>> > >>>> + if (!ret) > >>>> + return 0; > >>>> + } > >>>> + > >>> > >>> Should this be done with the chain->ctrl_mutex locked, as > >>> __uvc_query_v4l2_class() accesses dev->ctrl_class_bitmap that could be > >>> modified concurrently ? > >>> > >>>> ret = mutex_lock_interruptible(&chain->ctrl_mutex); > >>>> if (ret < 0) > >>>> return -ERESTARTSYS; > >>>> @@ -1133,6 +1195,13 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > >>>> goto done; > >>>> } > >>>> > >>> > >>> A comment here along the lines of > >>> > >>> /* > >>> * If we're enumerating control with V4L2_CTRL_FLAG_NEXT_CTRL, check if > >>> * a class should be inserted between the previous control and the one > >>> * we have just found. > >>> */ > >>> > >>> could be useful, as it's not trivial. > >> > >> yes, it looks better thanks! > >> > >>>> + if (v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL) { > >>>> + ret = uvc_query_v4l2_class(chain->dev, v4l2_ctrl->id, > >>>> + mapping->id, v4l2_ctrl); > >>>> + if (!ret) > >>>> + goto done; > >>>> + } > >>>> + > >>>> ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl); > >>>> done: > >>>> mutex_unlock(&chain->ctrl_mutex); > >>>> @@ -1422,6 +1491,9 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems) > >>>> struct uvc_control *ctrl; > >>>> int ret; > >>>> > >>>> + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) > >>>> + return 0; > >>> > >>> Do we really need to succeed ? What's the point in subscribing for > >>> control change events on a class ? Can't we just check if sev->id is a > >>> class, and return -EINVAL in that case ? > >> > >> Unfortunately it is expected that you can subscribe to all the events, > >> even the ctrl_classes > >> test VIDIOC_G/S/TRY_EXT_CTRLS: OK > >> fail: v4l2-test-controls.cpp(835): subscribe event for > >> control 'User Controls' failed > >> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL > > > > Looks like something that should be dropped from v4l2-compliance, > > there's no use case for subscribing to a class. > > It's allowed in the API. You never get an event, since it doesn't change, but > you can subscribe to it. I chose to allow it to avoid exceptions. Basically if > a control never changes, you just never get an event. Whether it is a control > class or a read-only control or a control with just a fixed value, it doesn't > matter for the event control API. Why do we inflict such pain upon ourselves, designing APIs that force us to support features that we know from the start are useless and will never be used ? :-( > >>>> + > >>>> ret = mutex_lock_interruptible(&handle->chain->ctrl_mutex); > >>>> if (ret < 0) > >>>> return -ERESTARTSYS; > >>>> @@ -1458,6 +1530,9 @@ static void uvc_ctrl_del_event(struct v4l2_subscribed_event *sev) > >>>> { > >>>> struct uvc_fh *handle = container_of(sev->fh, struct uvc_fh, vfh); > >>>> > >>>> + if (__uvc_query_v4l2_class(handle->chain->dev, sev->id, 0) >= 0) > >>>> + return; > >>> > >>> And this could then be dropped, as this function won't be called if the > >>> subscription failed. > >>> > >>>> + > >>>> mutex_lock(&handle->chain->ctrl_mutex); > >>>> list_del(&sev->node); > >>>> mutex_unlock(&handle->chain->ctrl_mutex); > >>>> @@ -1577,6 +1652,9 @@ int uvc_ctrl_get(struct uvc_video_chain *chain, > >>>> struct uvc_control *ctrl; > >>>> struct uvc_control_mapping *mapping; > >>>> > >>>> + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) > >>>> + return -EACCES; > >>>> + > >>>> ctrl = uvc_find_control(chain, xctrl->id, &mapping); > >>>> if (ctrl == NULL) > >>>> return -EINVAL; > >>>> @@ -1596,6 +1674,9 @@ int uvc_ctrl_set(struct uvc_fh *handle, > >>>> s32 max; > >>>> int ret; > >>>> > >>>> + if (__uvc_query_v4l2_class(chain->dev, xctrl->id, 0) >= 0) > >>>> + return -EACCES; > >>>> + > >>> > >>> Similarly as in patch 1/6, should these two checks be moved to > >>> v4l_s_ctrl() and v4l_s_ext_ctrls(), as it's never valid to get/set a > >>> class ? > >> > >> I do not think that it is possible, you need to return -EACCESS if the > >> control exists and -EINVAL if it does not exist. > >> v4l_s_ext_ctrls does not know if the ctrl exists. > > > > *sigh* I'm sad that we need this kind of complexity in drivers because > > the API requires us to implement a behaviour that nobody actually cares > > about :-( The way classes are implemented is really a big hack. > > > >>>> ctrl = uvc_find_control(chain, xctrl->id, &mapping); > >>>> if (ctrl == NULL) > >>>> return -EINVAL; > >>>> @@ -2062,6 +2143,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > >>>> { > >>>> struct uvc_control_mapping *map; > >>>> unsigned int size; > >>>> + int i; > >>> > >>> This can be unsigned as i never takes negative values. > >> I cannot repeat the same joke... even if it is a bad joke > >>> > >>>> > >>>> /* Most mappings come from static kernel data and need to be duplicated. > >>>> * Mappings that come from userspace will be unnecessarily duplicated, > >>>> @@ -2085,6 +2167,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev, > >>>> if (map->set == NULL) > >>>> map->set = uvc_set_le_value; > >>>> > >>>> + for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) { > >>>> + if (V4L2_CTRL_ID2WHICH(uvc_control_class[i].id) == > >>>> + V4L2_CTRL_ID2WHICH(map->id)) { > >>> > >>> You can write this > >>> > >>> if (uvc_control_class[i].id == V4L2_CTRL_ID2WHICH(map->id)) { > >>> > >>> as the uvc_control_class array contains control classes only. > >> > >> Are you sure? > >> #define V4L2_CID_CAMERA_CLASS (V4L2_CTRL_CLASS_CAMERA | 1) > >> > >> we are sasing the cid, not the class. > > > > Indeed, my bad. > > > >>>> + dev->ctrl_class_bitmap |= BIT(i); > >>>> + break; > >>>> + } > >>>> + } > >>>> + > >>>> list_add_tail(&map->list, &ctrl->info.mappings); > >>>> uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n", > >>>> map->name, ctrl->info.entity, ctrl->info.selector); > >>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > >>>> index 97df5ecd66c9..63b5d697a438 100644 > >>>> --- a/drivers/media/usb/uvc/uvcvideo.h > >>>> +++ b/drivers/media/usb/uvc/uvcvideo.h > >>>> @@ -262,6 +262,11 @@ struct uvc_control_mapping { > >>>> u8 *data); > >>>> }; > >>>> > >>>> +struct uvc_control_class { > >>>> + u32 id; > >>>> + char name[32]; > >>>> +}; > >>>> + > >>>> struct uvc_control { > >>>> struct uvc_entity *entity; > >>>> struct uvc_control_info info; > >>>> @@ -707,6 +712,8 @@ struct uvc_device { > >>>> } async_ctrl; > >>>> > >>>> struct uvc_entity *gpio_unit; > >>>> + > >>>> + u8 ctrl_class_bitmap; > >>> > >>> Should this be stored in the chain, as different chains can have > >>> different controls ? > >>> > >>>> }; > >>>> > >>>> enum uvc_handle_state { -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 6/6] media: uvcvideo: Set a different name for the metadata entity 2021-03-11 22:19 [PATCH v2 0/6] uvcvideo: Fix v4l2-compliance errors Ricardo Ribalda ` (4 preceding siblings ...) 2021-03-11 22:19 ` [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS Ricardo Ribalda @ 2021-03-11 22:19 ` Ricardo Ribalda 2021-03-11 23:38 ` Laurent Pinchart 5 siblings, 1 reply; 24+ messages in thread From: Ricardo Ribalda @ 2021-03-11 22:19 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Cc: Ricardo Ribalda All the entities must have a unique name. Fixes v4l2-compliance: Media Controller ioctls: fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end() test MEDIA_IOC_G_TOPOLOGY: FAIL fail: v4l2-test-media.cpp(394): num_data_links != num_links test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 30ef2a3110f7..47efa9a9be99 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -2199,7 +2199,10 @@ int uvc_register_video_device(struct uvc_device *dev, break; } - strscpy(vdev->name, dev->name, sizeof(vdev->name)); + if (type == V4L2_BUF_TYPE_META_CAPTURE) + strscpy(vdev->name, "Metadata Videodev", sizeof(vdev->name)); + else + strscpy(vdev->name, dev->name, sizeof(vdev->name)); /* * Set the driver data before calling video_register_device, otherwise -- 2.31.0.rc2.261.g7f71774620-goog ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 6/6] media: uvcvideo: Set a different name for the metadata entity 2021-03-11 22:19 ` [PATCH v2 6/6] media: uvcvideo: Set a different name for the metadata entity Ricardo Ribalda @ 2021-03-11 23:38 ` Laurent Pinchart 2021-03-12 7:17 ` Hans Verkuil 0 siblings, 1 reply; 24+ messages in thread From: Laurent Pinchart @ 2021-03-11 23:38 UTC (permalink / raw) To: Ricardo Ribalda Cc: Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky, Hans Verkuil Hi Ricardo, Thank you for the patch. On Thu, Mar 11, 2021 at 11:19:46PM +0100, Ricardo Ribalda wrote: > All the entities must have a unique name. > > Fixes v4l2-compliance: > Media Controller ioctls: > fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end() > test MEDIA_IOC_G_TOPOLOGY: FAIL > fail: v4l2-test-media.cpp(394): num_data_links != num_links > test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_driver.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 30ef2a3110f7..47efa9a9be99 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -2199,7 +2199,10 @@ int uvc_register_video_device(struct uvc_device *dev, > break; > } > > - strscpy(vdev->name, dev->name, sizeof(vdev->name)); > + if (type == V4L2_BUF_TYPE_META_CAPTURE) > + strscpy(vdev->name, "Metadata Videodev", sizeof(vdev->name)); > + else > + strscpy(vdev->name, dev->name, sizeof(vdev->name)); A UVC device could contain multiple output terminals (either in the same chain or in different chains), which would still result in multiple entities having the same name. Could this be fixed at the same time ? You can use the unit ID of the output terminal to create unique names (and it would be nice if the video and metadata nodes has similar names, with "video" and "metadata" being the only difference between them). > > /* > * Set the driver data before calling video_register_device, otherwise -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 6/6] media: uvcvideo: Set a different name for the metadata entity 2021-03-11 23:38 ` Laurent Pinchart @ 2021-03-12 7:17 ` Hans Verkuil 0 siblings, 0 replies; 24+ messages in thread From: Hans Verkuil @ 2021-03-12 7:17 UTC (permalink / raw) To: Laurent Pinchart, Ricardo Ribalda Cc: Mauro Carvalho Chehab, Tomasz Figa, linux-media, linux-kernel, senozhatsky On 12/03/2021 00:38, Laurent Pinchart wrote: > Hi Ricardo, > > Thank you for the patch. > > On Thu, Mar 11, 2021 at 11:19:46PM +0100, Ricardo Ribalda wrote: >> All the entities must have a unique name. >> >> Fixes v4l2-compliance: >> Media Controller ioctls: >> fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end() >> test MEDIA_IOC_G_TOPOLOGY: FAIL >> fail: v4l2-test-media.cpp(394): num_data_links != num_links >> test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL >> >> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >> --- >> drivers/media/usb/uvc/uvc_driver.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c >> index 30ef2a3110f7..47efa9a9be99 100644 >> --- a/drivers/media/usb/uvc/uvc_driver.c >> +++ b/drivers/media/usb/uvc/uvc_driver.c >> @@ -2199,7 +2199,10 @@ int uvc_register_video_device(struct uvc_device *dev, >> break; >> } >> >> - strscpy(vdev->name, dev->name, sizeof(vdev->name)); >> + if (type == V4L2_BUF_TYPE_META_CAPTURE) >> + strscpy(vdev->name, "Metadata Videodev", sizeof(vdev->name)); >> + else >> + strscpy(vdev->name, dev->name, sizeof(vdev->name)); > > A UVC device could contain multiple output terminals (either in the same > chain or in different chains), which would still result in multiple > entities having the same name. Could this be fixed at the same time ? > You can use the unit ID of the output terminal to create unique names > (and it would be nice if the video and metadata nodes has similar names, > with "video" and "metadata" being the only difference between them). I agree with Laurent. How about using something like this for the videodevs: snprintf(vdev->name, sizeof(vdev->name), "Meta %s", dev->name); and: snprintf(vdev->name, sizeof(vdev->name), "Video %s", dev->name); Regards, Hans > >> >> /* >> * Set the driver data before calling video_register_device, otherwise > ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-03-12 10:58 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-11 22:19 [PATCH v2 0/6] uvcvideo: Fix v4l2-compliance errors Ricardo Ribalda 2021-03-11 22:19 ` [PATCH v2 1/6] media: v4l2-ioctl: Fix check_ext_ctrls Ricardo Ribalda 2021-03-11 23:43 ` Laurent Pinchart 2021-03-11 22:19 ` [PATCH v2 2/6] media: uvcvideo: Set capability in s_param Ricardo Ribalda 2021-03-12 7:07 ` Hans Verkuil 2021-03-11 22:19 ` [PATCH v2 3/6] media: uvcvideo: Return -EIO for control errors Ricardo Ribalda 2021-03-11 22:50 ` Laurent Pinchart 2021-03-11 22:59 ` Ricardo Ribalda Delgado 2021-03-11 23:30 ` Laurent Pinchart 2021-03-12 6:51 ` Ricardo Ribalda Delgado 2021-03-12 7:08 ` Hans Verkuil 2021-03-11 22:19 ` [PATCH v2 4/6] media: uvcvideo: set error_idx to count on EACCESS Ricardo Ribalda 2021-03-11 23:40 ` Laurent Pinchart 2021-03-12 7:14 ` Hans Verkuil 2021-03-12 10:18 ` Laurent Pinchart 2021-03-11 22:19 ` [PATCH v2 5/6] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS Ricardo Ribalda 2021-03-12 1:21 ` Laurent Pinchart 2021-03-12 9:57 ` Ricardo Ribalda Delgado 2021-03-12 10:13 ` Laurent Pinchart 2021-03-12 10:22 ` Hans Verkuil 2021-03-12 10:56 ` Laurent Pinchart 2021-03-11 22:19 ` [PATCH v2 6/6] media: uvcvideo: Set a different name for the metadata entity Ricardo Ribalda 2021-03-11 23:38 ` Laurent Pinchart 2021-03-12 7:17 ` Hans Verkuil
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.