[PATCH v9 00/22] uvcvideo: Fix v4l2-compliance errors

[PATCH v9 00/22] uvcvideo: Fix v4l2-compliance errors

[Ricardo Ribalda]

*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: 0
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: 0

YES, NO MORE WARNINGS :)

Note that we depend on:

https://patchwork.linuxtv.org/project/linux-media/patch/20210317143453.483470-1-ribalda@chromium.org/

With Hans patch we can also pass v4l2-compliance -s.

Changelog from v8 (Thanks to Hans)
- 3 patches from Hans
- Add Reviewed-by

Hans Verkuil (4):
  uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
  uvcvideo: improve error handling in uvc_query_ctrl()
  uvcvideo: don't spam the log in uvc_ctrl_restore_values()
  uvc: use vb2 ioctl and fop helpers

Ricardo Ribalda (18):
  media: v4l2-ioctl: Fix check_ext_ctrls
  media: pvrusb2: Do not check for V4L2_CTRL_WHICH_DEF_VAL
  media: uvcvideo: Do not check for V4L2_CTRL_WHICH_DEF_VAL
  media: v4l2-ioctl: S_CTRL output the right value
  media: uvcvideo: Remove s_ctrl and g_ctrl
  media: uvcvideo: Set capability in s_param
  media: uvcvideo: Return -EIO for control errors
  media: uvcvideo: refactor __uvc_ctrl_add_mapping
  media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS
  media: uvcvideo: Use dev->name for querycap()
  media: uvcvideo: Set unique vdev name based in type
  media: uvcvideo: Increase the size of UVC_METADATA_BUF_SIZE
  media: uvcvideo: Use control names from framework
  media: uvcvideo: Check controls flags before accessing them
  media: uvcvideo: Set error_idx during ctrl_commit errors
  media: uvcvideo: Return -EACCES to inactive controls
  media: docs: Document the behaviour of uvcdriver
  media: uvcvideo: Downgrade control error messages

 .../userspace-api/media/v4l/vidioc-g-ctrl.rst |   5 +
 .../media/v4l/vidioc-g-ext-ctrls.rst          |   5 +
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c      |   4 -
 drivers/media/usb/uvc/uvc_ctrl.c              | 343 +++++++++++----
 drivers/media/usb/uvc/uvc_driver.c            |  22 +-
 drivers/media/usb/uvc/uvc_metadata.c          |  10 +-
 drivers/media/usb/uvc/uvc_queue.c             | 143 -------
 drivers/media/usb/uvc/uvc_v4l2.c              | 389 +++---------------
 drivers/media/usb/uvc/uvc_video.c             |  51 ++-
 drivers/media/usb/uvc/uvcvideo.h              |  54 +--
 drivers/media/v4l2-core/v4l2-ioctl.c          |  67 +--
 11 files changed, 444 insertions(+), 649 deletions(-)

-- 
2.31.0.291.g576ba9dcdaf-goog

[PATCH v9 01/22] media: v4l2-ioctl: Fix check_ext_ctrls

[Ricardo Ribalda]

Drivers that do not use the ctrl-framework use this function instead.

Fix the following issues:

- Do not check for multiple classes when getting the DEF_VAL.
- Return -EINVAL for request_api calls
- Default value cannot be changed, return EINVAL as soon as possible.
- Return the right error_idx
[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):
        warn: v4l2-test-controls.cpp(834): error_idx should be equal to count
        warn: v4l2-test-controls.cpp(855): error_idx should be equal to count
		fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)
	test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
Buffer ioctls (Input 0):
		fail: v4l2-test-buffers.cpp(1994): ret != EINVAL && ret != EBADR && ret != ENOTTY
	test Requests: FAIL

Cc: stable@vger.kernel.org
Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support")
Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 60 ++++++++++++++++++----------
 1 file changed, 39 insertions(+), 21 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 31d1342e61e8..7b5ebdd329e8 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only)
 	pr_cont("driver-specific ioctl\n");
 }
 
-static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
+static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl)
 {
 	__u32 i;
 
@@ -917,23 +917,41 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
 	for (i = 0; i < c->count; i++)
 		c->controls[i].reserved2[0] = 0;
 
-	/* V4L2_CID_PRIVATE_BASE cannot be used as control class
-	   when using extended controls.
-	   Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL
-	   is it allowed for backwards compatibility.
-	 */
-	if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE)
-		return 0;
-	if (!c->which)
-		return 1;
+	switch (c->which) {
+	case V4L2_CID_PRIVATE_BASE:
+		/*
+		 * V4L2_CID_PRIVATE_BASE cannot be used as control class
+		 * when using extended controls.
+		 * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL
+		 * is it allowed for backwards compatibility.
+		 */
+		if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CTRL)
+			return false;
+		break;
+	case V4L2_CTRL_WHICH_DEF_VAL:
+		/* Default value cannot be changed */
+		if (ioctl == VIDIOC_S_EXT_CTRLS ||
+		    ioctl == VIDIOC_TRY_EXT_CTRLS) {
+			c->error_idx = c->count;
+			return false;
+		}
+		return true;
+	case V4L2_CTRL_WHICH_CUR_VAL:
+		return true;
+	case V4L2_CTRL_WHICH_REQUEST_VAL:
+		c->error_idx = c->count;
+		return false;
+	}
+
 	/* 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) {
-			c->error_idx = i;
-			return 0;
+			c->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i :
+								      c->count;
+			return false;
 		}
 	}
-	return 1;
+	return true;
 }
 
 static int check_fmt(struct file *file, enum v4l2_buf_type type)
@@ -2229,7 +2247,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops,
 	ctrls.controls = &ctrl;
 	ctrl.id = p->id;
 	ctrl.value = p->value;
-	if (check_ext_ctrls(&ctrls, 1)) {
+	if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) {
 		int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls);
 
 		if (ret == 0)
@@ -2263,7 +2281,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
 	ctrls.controls = &ctrl;
 	ctrl.id = p->id;
 	ctrl.value = p->value;
-	if (check_ext_ctrls(&ctrls, 1))
+	if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL))
 		return ops->vidioc_s_ext_ctrls(file, fh, &ctrls);
 	return -EINVAL;
 }
@@ -2285,8 +2303,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 					vfd, vfd->v4l2_dev->mdev, p);
 	if (ops->vidioc_g_ext_ctrls == NULL)
 		return -ENOTTY;
-	return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) :
-					-EINVAL;
+	return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ?
+				ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL;
 }
 
 static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
@@ -2306,8 +2324,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 					vfd, vfd->v4l2_dev->mdev, p);
 	if (ops->vidioc_s_ext_ctrls == NULL)
 		return -ENOTTY;
-	return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) :
-					-EINVAL;
+	return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ?
+				ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL;
 }
 
 static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
@@ -2327,8 +2345,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 					  vfd, vfd->v4l2_dev->mdev, p);
 	if (ops->vidioc_try_ext_ctrls == NULL)
 		return -ENOTTY;
-	return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) :
-					-EINVAL;
+	return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ?
+			ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL;
 }
 
 /*
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 01/22] media: v4l2-ioctl: Fix check_ext_ctrls

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:19AM +0100, Ricardo Ribalda wrote:
> Drivers that do not use the ctrl-framework use this function instead.
> 
> Fix the following issues:
> 
> - Do not check for multiple classes when getting the DEF_VAL.
> - Return -EINVAL for request_api calls
> - Default value cannot be changed, return EINVAL as soon as possible.
> - Return the right error_idx
> [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):
>         warn: v4l2-test-controls.cpp(834): error_idx should be equal to count
>         warn: v4l2-test-controls.cpp(855): error_idx should be equal to count
> 		fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)
> 	test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
> Buffer ioctls (Input 0):
> 		fail: v4l2-test-buffers.cpp(1994): ret != EINVAL && ret != EBADR && ret != ENOTTY
> 	test Requests: FAIL
> 
> Cc: stable@vger.kernel.org
> Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support")
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 60 ++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 31d1342e61e8..7b5ebdd329e8 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -908,7 +908,7 @@ static void v4l_print_default(const void *arg, bool write_only)
>  	pr_cont("driver-specific ioctl\n");
>  }
>  
> -static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
> +static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl)
>  {
>  	__u32 i;
>  
> @@ -917,23 +917,41 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
>  	for (i = 0; i < c->count; i++)
>  		c->controls[i].reserved2[0] = 0;
>  
> -	/* V4L2_CID_PRIVATE_BASE cannot be used as control class
> -	   when using extended controls.
> -	   Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL
> -	   is it allowed for backwards compatibility.
> -	 */
> -	if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE)
> -		return 0;
> -	if (!c->which)
> -		return 1;
> +	switch (c->which) {
> +	case V4L2_CID_PRIVATE_BASE:
> +		/*
> +		 * V4L2_CID_PRIVATE_BASE cannot be used as control class
> +		 * when using extended controls.
> +		 * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL
> +		 * is it allowed for backwards compatibility.
> +		 */
> +		if (ioctl == VIDIOC_G_CTRL || ioctl == VIDIOC_S_CTRL)
> +			return false;
> +		break;
> +	case V4L2_CTRL_WHICH_DEF_VAL:
> +		/* Default value cannot be changed */
> +		if (ioctl == VIDIOC_S_EXT_CTRLS ||
> +		    ioctl == VIDIOC_TRY_EXT_CTRLS) {
> +			c->error_idx = c->count;
> +			return false;
> +		}
> +		return true;
> +	case V4L2_CTRL_WHICH_CUR_VAL:
> +		return true;
> +	case V4L2_CTRL_WHICH_REQUEST_VAL:
> +		c->error_idx = c->count;
> +		return false;
> +	}
> +
>  	/* 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) {
> -			c->error_idx = i;
> -			return 0;
> +			c->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i :
> +								      c->count;
> +			return false;
>  		}
>  	}
> -	return 1;
> +	return true;
>  }
>  
>  static int check_fmt(struct file *file, enum v4l2_buf_type type)
> @@ -2229,7 +2247,7 @@ static int v4l_g_ctrl(const struct v4l2_ioctl_ops *ops,
>  	ctrls.controls = &ctrl;
>  	ctrl.id = p->id;
>  	ctrl.value = p->value;
> -	if (check_ext_ctrls(&ctrls, 1)) {
> +	if (check_ext_ctrls(&ctrls, VIDIOC_G_CTRL)) {
>  		int ret = ops->vidioc_g_ext_ctrls(file, fh, &ctrls);
>  
>  		if (ret == 0)
> @@ -2263,7 +2281,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
>  	ctrls.controls = &ctrl;
>  	ctrl.id = p->id;
>  	ctrl.value = p->value;
> -	if (check_ext_ctrls(&ctrls, 1))
> +	if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL))
>  		return ops->vidioc_s_ext_ctrls(file, fh, &ctrls);
>  	return -EINVAL;
>  }
> @@ -2285,8 +2303,8 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  					vfd, vfd->v4l2_dev->mdev, p);
>  	if (ops->vidioc_g_ext_ctrls == NULL)
>  		return -ENOTTY;
> -	return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) :
> -					-EINVAL;
> +	return check_ext_ctrls(p, VIDIOC_G_EXT_CTRLS) ?
> +				ops->vidioc_g_ext_ctrls(file, fh, p) : -EINVAL;
>  }
>  
>  static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> @@ -2306,8 +2324,8 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  					vfd, vfd->v4l2_dev->mdev, p);
>  	if (ops->vidioc_s_ext_ctrls == NULL)
>  		return -ENOTTY;
> -	return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) :
> -					-EINVAL;
> +	return check_ext_ctrls(p, VIDIOC_S_EXT_CTRLS) ?
> +				ops->vidioc_s_ext_ctrls(file, fh, p) : -EINVAL;
>  }
>  
>  static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
> @@ -2327,8 +2345,8 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
>  					  vfd, vfd->v4l2_dev->mdev, p);
>  	if (ops->vidioc_try_ext_ctrls == NULL)
>  		return -ENOTTY;
> -	return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) :
> -					-EINVAL;
> +	return check_ext_ctrls(p, VIDIOC_TRY_EXT_CTRLS) ?
> +			ops->vidioc_try_ext_ctrls(file, fh, p) : -EINVAL;
>  }
>  
>  /*

-- 
Regards,

Laurent Pinchart

[PATCH v9 02/22] media: pvrusb2: Do not check for V4L2_CTRL_WHICH_DEF_VAL

[Ricardo Ribalda]

The framework already checks for us if V4L2_CTRL_WHICH_DEF_VAL is
written.

Cc: Mike Isely <isely@pobox.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index 9657c1883311..c04ab7258d64 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -640,10 +640,6 @@ static int pvr2_s_ext_ctrls(struct file *file, void *priv,
 	unsigned int idx;
 	int ret;
 
-	/* Default value cannot be changed */
-	if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL)
-		return -EINVAL;
-
 	ret = 0;
 	for (idx = 0; idx < ctls->count; idx++) {
 		ctrl = ctls->controls + idx;
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 02/22] media: pvrusb2: Do not check for V4L2_CTRL_WHICH_DEF_VAL

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:20AM +0100, Ricardo Ribalda wrote:
> The framework already checks for us if V4L2_CTRL_WHICH_DEF_VAL is
> written.
> 
> Cc: Mike Isely <isely@pobox.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> index 9657c1883311..c04ab7258d64 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> @@ -640,10 +640,6 @@ static int pvr2_s_ext_ctrls(struct file *file, void *priv,
>  	unsigned int idx;
>  	int ret;
>  
> -	/* Default value cannot be changed */
> -	if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL)
> -		return -EINVAL;
> -
>  	ret = 0;
>  	for (idx = 0; idx < ctls->count; idx++) {
>  		ctrl = ctls->controls + idx;

-- 
Regards,

Laurent Pinchart

[PATCH v9 03/22] media: uvcvideo: Do not check for V4L2_CTRL_WHICH_DEF_VAL

[Ricardo Ribalda]

The framework already checks for us if V4L2_CTRL_WHICH_DEF_VAL is
written.

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 252136cc885c..47b0e3224205 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1089,10 +1089,6 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 	unsigned int i;
 	int ret;
 
-	/* Default value cannot be changed */
-	if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL)
-		return -EINVAL;
-
 	ret = uvc_ctrl_begin(chain);
 	if (ret < 0)
 		return ret;
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 03/22] media: uvcvideo: Do not check for V4L2_CTRL_WHICH_DEF_VAL

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:21AM +0100, Ricardo Ribalda wrote:
> The framework already checks for us if V4L2_CTRL_WHICH_DEF_VAL is
> written.
> 
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 252136cc885c..47b0e3224205 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1089,10 +1089,6 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  	unsigned int i;
>  	int ret;
>  
> -	/* Default value cannot be changed */
> -	if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL)
> -		return -EINVAL;
> -
>  	ret = uvc_ctrl_begin(chain);
>  	if (ret < 0)
>  		return ret;

-- 
Regards,

Laurent Pinchart

[PATCH v9 04/22] media: v4l2-ioctl: S_CTRL output the right value

[Ricardo Ribalda]

If the driver does not implement s_ctrl, but it does implement
s_ext_ctrls, we convert the call.

When that happens we have also to convert back the response from
s_ext_ctrls.

Fixes v4l2_compliance:
Control ioctls (Input 0):
		fail: v4l2-test-controls.cpp(411): returned control value out of range
		fail: v4l2-test-controls.cpp(507): invalid control 00980900
	test VIDIOC_G/S_CTRL: FAIL

Fixes: 35ea11ff8471 ("V4L/DVB (8430): videodev: move some functions from v4l2-dev.h to v4l2-common.h or v4l2-ioctl.h")
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 7b5ebdd329e8..b8f73a48872b 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2266,6 +2266,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
 		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
 	struct v4l2_ext_controls ctrls;
 	struct v4l2_ext_control ctrl;
+	int ret;
 
 	if (vfh && vfh->ctrl_handler)
 		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, p);
@@ -2281,9 +2282,11 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
 	ctrls.controls = &ctrl;
 	ctrl.id = p->id;
 	ctrl.value = p->value;
-	if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL))
-		return ops->vidioc_s_ext_ctrls(file, fh, &ctrls);
-	return -EINVAL;
+	if (!check_ext_ctrls(&ctrls, VIDIOC_S_CTRL))
+		return -EINVAL;
+	ret = ops->vidioc_s_ext_ctrls(file, fh, &ctrls);
+	p->value = ctrl.value;
+	return ret;
 }
 
 static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 04/22] media: v4l2-ioctl: S_CTRL output the right value

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:22AM +0100, Ricardo Ribalda wrote:
> If the driver does not implement s_ctrl, but it does implement
> s_ext_ctrls, we convert the call.
> 
> When that happens we have also to convert back the response from
> s_ext_ctrls.
> 
> Fixes v4l2_compliance:
> Control ioctls (Input 0):
> 		fail: v4l2-test-controls.cpp(411): returned control value out of range
> 		fail: v4l2-test-controls.cpp(507): invalid control 00980900
> 	test VIDIOC_G/S_CTRL: FAIL
> 
> Fixes: 35ea11ff8471 ("V4L/DVB (8430): videodev: move some functions from v4l2-dev.h to v4l2-common.h or v4l2-ioctl.h")
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 7b5ebdd329e8..b8f73a48872b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2266,6 +2266,7 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
>  		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
>  	struct v4l2_ext_controls ctrls;
>  	struct v4l2_ext_control ctrl;
> +	int ret;
>  
>  	if (vfh && vfh->ctrl_handler)
>  		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, p);
> @@ -2281,9 +2282,11 @@ static int v4l_s_ctrl(const struct v4l2_ioctl_ops *ops,
>  	ctrls.controls = &ctrl;
>  	ctrl.id = p->id;
>  	ctrl.value = p->value;
> -	if (check_ext_ctrls(&ctrls, VIDIOC_S_CTRL))
> -		return ops->vidioc_s_ext_ctrls(file, fh, &ctrls);
> -	return -EINVAL;
> +	if (!check_ext_ctrls(&ctrls, VIDIOC_S_CTRL))
> +		return -EINVAL;
> +	ret = ops->vidioc_s_ext_ctrls(file, fh, &ctrls);
> +	p->value = ctrl.value;
> +	return ret;
>  }
>  
>  static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,

-- 
Regards,

Laurent Pinchart

[PATCH v9 05/22] media: uvcvideo: Remove s_ctrl and g_ctrl

[Ricardo Ribalda]

If we do not implement these callbacks the framework will call the
ext_ctrl callbaks instead, which are a superset of this functions.

Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 56 --------------------------------
 1 file changed, 56 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 47b0e3224205..ac98869d5a05 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -983,60 +983,6 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
 	return 0;
 }
 
-static int uvc_ioctl_g_ctrl(struct file *file, void *fh,
-			    struct v4l2_control *ctrl)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_video_chain *chain = handle->chain;
-	struct v4l2_ext_control xctrl;
-	int ret;
-
-	memset(&xctrl, 0, sizeof(xctrl));
-	xctrl.id = ctrl->id;
-
-	ret = uvc_ctrl_begin(chain);
-	if (ret < 0)
-		return ret;
-
-	ret = uvc_ctrl_get(chain, &xctrl);
-	uvc_ctrl_rollback(handle);
-	if (ret < 0)
-		return ret;
-
-	ctrl->value = xctrl.value;
-	return 0;
-}
-
-static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
-			    struct v4l2_control *ctrl)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_video_chain *chain = handle->chain;
-	struct v4l2_ext_control xctrl;
-	int ret;
-
-	memset(&xctrl, 0, sizeof(xctrl));
-	xctrl.id = ctrl->id;
-	xctrl.value = ctrl->value;
-
-	ret = uvc_ctrl_begin(chain);
-	if (ret < 0)
-		return ret;
-
-	ret = uvc_ctrl_set(handle, &xctrl);
-	if (ret < 0) {
-		uvc_ctrl_rollback(handle);
-		return ret;
-	}
-
-	ret = uvc_ctrl_commit(handle, &xctrl, 1);
-	if (ret < 0)
-		return ret;
-
-	ctrl->value = xctrl.value;
-	return 0;
-}
-
 static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 				 struct v4l2_ext_controls *ctrls)
 {
@@ -1522,8 +1468,6 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
 	.vidioc_s_input = uvc_ioctl_s_input,
 	.vidioc_queryctrl = uvc_ioctl_queryctrl,
 	.vidioc_query_ext_ctrl = uvc_ioctl_query_ext_ctrl,
-	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
-	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
 	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
 	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
 	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 05/22] media: uvcvideo: Remove s_ctrl and g_ctrl

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:23AM +0100, Ricardo Ribalda wrote:
> If we do not implement these callbacks the framework will call the
> ext_ctrl callbaks instead, which are a superset of this functions.
> 
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 56 --------------------------------
>  1 file changed, 56 deletions(-)

Nice diffstat.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 47b0e3224205..ac98869d5a05 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -983,60 +983,6 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
>  	return 0;
>  }
>  
> -static int uvc_ioctl_g_ctrl(struct file *file, void *fh,
> -			    struct v4l2_control *ctrl)
> -{
> -	struct uvc_fh *handle = fh;
> -	struct uvc_video_chain *chain = handle->chain;
> -	struct v4l2_ext_control xctrl;
> -	int ret;
> -
> -	memset(&xctrl, 0, sizeof(xctrl));
> -	xctrl.id = ctrl->id;
> -
> -	ret = uvc_ctrl_begin(chain);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = uvc_ctrl_get(chain, &xctrl);
> -	uvc_ctrl_rollback(handle);
> -	if (ret < 0)
> -		return ret;
> -
> -	ctrl->value = xctrl.value;
> -	return 0;
> -}
> -
> -static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
> -			    struct v4l2_control *ctrl)
> -{
> -	struct uvc_fh *handle = fh;
> -	struct uvc_video_chain *chain = handle->chain;
> -	struct v4l2_ext_control xctrl;
> -	int ret;
> -
> -	memset(&xctrl, 0, sizeof(xctrl));
> -	xctrl.id = ctrl->id;
> -	xctrl.value = ctrl->value;
> -
> -	ret = uvc_ctrl_begin(chain);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = uvc_ctrl_set(handle, &xctrl);
> -	if (ret < 0) {
> -		uvc_ctrl_rollback(handle);
> -		return ret;
> -	}
> -
> -	ret = uvc_ctrl_commit(handle, &xctrl, 1);
> -	if (ret < 0)
> -		return ret;
> -
> -	ctrl->value = xctrl.value;
> -	return 0;
> -}
> -
>  static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  				 struct v4l2_ext_controls *ctrls)
>  {
> @@ -1522,8 +1468,6 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>  	.vidioc_s_input = uvc_ioctl_s_input,
>  	.vidioc_queryctrl = uvc_ioctl_queryctrl,
>  	.vidioc_query_ext_ctrl = uvc_ioctl_query_ext_ctrl,
> -	.vidioc_g_ctrl = uvc_ioctl_g_ctrl,
> -	.vidioc_s_ctrl = uvc_ioctl_s_ctrl,
>  	.vidioc_g_ext_ctrls = uvc_ioctl_g_ext_ctrls,
>  	.vidioc_s_ext_ctrls = uvc_ioctl_s_ext_ctrls,
>  	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,

-- 
Regards,

Laurent Pinchart

[PATCH v9 06/22] media: uvcvideo: Set capability in s_param

[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>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
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 ac98869d5a05..1eeeb00280e4 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.291.g576ba9dcdaf-goog

[PATCH v9 07/22] media: uvcvideo: Return -EIO for control errors

[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

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
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.291.g576ba9dcdaf-goog

[PATCH v9 08/22] media: uvcvideo: refactor __uvc_ctrl_add_mapping

[Ricardo Ribalda]

Pass the chain instead of the device. We want to keep the reference to
the chain that controls belong to.

We need to delay the initialization of the controls after the chains
have been initialized.

This is a cleanup needed for the next patches.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   | 41 ++++++++++++++++++++----------
 drivers/media/usb/uvc/uvc_driver.c |  8 +++---
 2 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index b3dde98499f4..b75da65115ef 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2057,7 +2057,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl,
 /*
  * Add a control mapping to a given control.
  */
-static int __uvc_ctrl_add_mapping(struct uvc_device *dev,
+static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
 {
 	struct uvc_control_mapping *map;
@@ -2086,7 +2086,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev,
 		map->set = uvc_set_le_value;
 
 	list_add_tail(&map->list, &ctrl->info.mappings);
-	uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
+	uvc_dbg(chain->dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
 		map->name, ctrl->info.entity, ctrl->info.selector);
 
 	return 0;
@@ -2168,7 +2168,7 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 		goto done;
 	}
 
-	ret = __uvc_ctrl_add_mapping(dev, ctrl, mapping);
+	ret = __uvc_ctrl_add_mapping(chain, ctrl, mapping);
 	if (ret < 0)
 		atomic_dec(&dev->nmappings);
 
@@ -2244,7 +2244,8 @@ static void uvc_ctrl_prune_entity(struct uvc_device *dev,
  * Add control information and hardcoded stock control mappings to the given
  * device.
  */
-static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl)
+static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
+			       struct uvc_control *ctrl)
 {
 	const struct uvc_control_info *info = uvc_ctrls;
 	const struct uvc_control_info *iend = info + ARRAY_SIZE(uvc_ctrls);
@@ -2263,14 +2264,14 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl)
 	for (; info < iend; ++info) {
 		if (uvc_entity_match_guid(ctrl->entity, info->entity) &&
 		    ctrl->index == info->index) {
-			uvc_ctrl_add_info(dev, ctrl, info);
+			uvc_ctrl_add_info(chain->dev, ctrl, info);
 			/*
 			 * Retrieve control flags from the device. Ignore errors
 			 * and work with default flag values from the uvc_ctrl
 			 * array when the device doesn't properly implement
 			 * GET_INFO on standard controls.
 			 */
-			uvc_ctrl_get_flags(dev, ctrl, &ctrl->info);
+			uvc_ctrl_get_flags(chain->dev, ctrl, &ctrl->info);
 			break;
 		 }
 	}
@@ -2281,22 +2282,20 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl)
 	for (; mapping < mend; ++mapping) {
 		if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&
 		    ctrl->info.selector == mapping->selector)
-			__uvc_ctrl_add_mapping(dev, ctrl, mapping);
+			__uvc_ctrl_add_mapping(chain, ctrl, mapping);
 	}
 }
 
 /*
  * Initialize device controls.
  */
-int uvc_ctrl_init_device(struct uvc_device *dev)
+static int uvc_ctrl_init_chain(struct uvc_video_chain *chain)
 {
 	struct uvc_entity *entity;
 	unsigned int i;
 
-	INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
-
 	/* Walk the entities list and instantiate controls */
-	list_for_each_entry(entity, &dev->entities, list) {
+	list_for_each_entry(entity, &chain->entities, chain) {
 		struct uvc_control *ctrl;
 		unsigned int bControlSize = 0, ncontrols;
 		u8 *bmControls = NULL;
@@ -2316,7 +2315,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 		}
 
 		/* Remove bogus/blacklisted controls */
-		uvc_ctrl_prune_entity(dev, entity);
+		uvc_ctrl_prune_entity(chain->dev, entity);
 
 		/* Count supported controls and allocate the controls array */
 		ncontrols = memweight(bmControls, bControlSize);
@@ -2338,7 +2337,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 			ctrl->entity = entity;
 			ctrl->index = i;
 
-			uvc_ctrl_init_ctrl(dev, ctrl);
+			uvc_ctrl_init_ctrl(chain, ctrl);
 			ctrl++;
 		}
 	}
@@ -2346,6 +2345,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 	return 0;
 }
 
+int uvc_ctrl_init_device(struct uvc_device *dev)
+{
+	struct uvc_video_chain *chain;
+	int ret;
+
+	INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
+
+	list_for_each_entry(chain, &dev->chains, list) {
+		ret = uvc_ctrl_init_chain(chain);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /*
  * Cleanup device controls.
  */
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 30ef2a3110f7..35873cf2773d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2423,14 +2423,14 @@ static int uvc_probe(struct usb_interface *intf,
 	if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
 		goto error;
 
-	/* Initialize controls. */
-	if (uvc_ctrl_init_device(dev) < 0)
-		goto error;
-
 	/* Scan the device for video chains. */
 	if (uvc_scan_device(dev) < 0)
 		goto error;
 
+	/* Initialize controls. */
+	if (uvc_ctrl_init_device(dev) < 0)
+		goto error;
+
 	/* Register video device nodes. */
 	if (uvc_register_chains(dev) < 0)
 		goto error;
-- 
2.31.0.291.g576ba9dcdaf-goog

[PATCH v9 09/22] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS

[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

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 94 ++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h |  5 ++
 2 files changed, 99 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index b75da65115ef..ba14733db757 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -357,6 +357,15 @@ static const struct uvc_control_info uvc_ctrls[] = {
 	},
 };
 
+static const struct uvc_control_class uvc_control_class[] = {
+	{
+		.id		= V4L2_CID_CAMERA_CLASS,
+	},
+	{
+		.id		= V4L2_CID_USER_CLASS,
+	},
+};
+
 static const struct uvc_menu_info power_line_frequency_controls[] = {
 	{ 0, "Disabled" },
 	{ 1, "50 Hz" },
@@ -1024,6 +1033,49 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
 	return 0;
 }
 
+static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
+				  u32 found_id)
+{
+	bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL;
+	unsigned int i;
+
+	req_id &= V4L2_CTRL_ID_MASK;
+
+	for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) {
+		if (!(chain->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_video_chain *chain, u32 req_id,
+				u32 found_id, struct v4l2_queryctrl *v4l2_ctrl)
+{
+	int idx;
+
+	idx = __uvc_query_v4l2_class(chain, 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, v4l2_ctrl_get_name(v4l2_ctrl->id),
+		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,
@@ -1127,12 +1179,31 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 	if (ret < 0)
 		return -ERESTARTSYS;
 
+	/* Check if the ctrl is a know class */
+	if (!(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)) {
+		ret = uvc_query_v4l2_class(chain, v4l2_ctrl->id, 0, v4l2_ctrl);
+		if (!ret)
+			goto done;
+	}
+
 	ctrl = uvc_find_control(chain, v4l2_ctrl->id, &mapping);
 	if (ctrl == NULL) {
 		ret = -EINVAL;
 		goto done;
 	}
 
+	/*
+	 * 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.
+	 */
+	if (v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL) {
+		ret = uvc_query_v4l2_class(chain, 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);
@@ -1426,6 +1497,11 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 	if (ret < 0)
 		return -ERESTARTSYS;
 
+	if (__uvc_query_v4l2_class(handle->chain, sev->id, 0) >= 0) {
+		ret = 0;
+		goto done;
+	}
+
 	ctrl = uvc_find_control(handle->chain, sev->id, &mapping);
 	if (ctrl == NULL) {
 		ret = -EINVAL;
@@ -1459,7 +1535,10 @@ static void uvc_ctrl_del_event(struct v4l2_subscribed_event *sev)
 	struct uvc_fh *handle = container_of(sev->fh, struct uvc_fh, vfh);
 
 	mutex_lock(&handle->chain->ctrl_mutex);
+	if (__uvc_query_v4l2_class(handle->chain, sev->id, 0) >= 0)
+		goto done;
 	list_del(&sev->node);
+done:
 	mutex_unlock(&handle->chain->ctrl_mutex);
 }
 
@@ -1577,6 +1656,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, xctrl->id, 0) >= 0)
+		return -EACCES;
+
 	ctrl = uvc_find_control(chain, xctrl->id, &mapping);
 	if (ctrl == NULL)
 		return -EINVAL;
@@ -1596,6 +1678,9 @@ int uvc_ctrl_set(struct uvc_fh *handle,
 	s32 max;
 	int ret;
 
+	if (__uvc_query_v4l2_class(chain, xctrl->id, 0) >= 0)
+		return -EACCES;
+
 	ctrl = uvc_find_control(chain, xctrl->id, &mapping);
 	if (ctrl == NULL)
 		return -EINVAL;
@@ -2062,6 +2147,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 {
 	struct uvc_control_mapping *map;
 	unsigned int size;
+	unsigned 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 +2171,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 	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)) {
+			chain->ctrl_class_bitmap |= BIT(i);
+			break;
+		}
+	}
+
 	list_add_tail(&map->list, &ctrl->info.mappings);
 	uvc_dbg(chain->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..b81d3f65e52e 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -262,6 +262,10 @@ struct uvc_control_mapping {
 		    u8 *data);
 };
 
+struct uvc_control_class {
+	u32 id;
+};
+
 struct uvc_control {
 	struct uvc_entity *entity;
 	struct uvc_control_info info;
@@ -475,6 +479,7 @@ struct uvc_video_chain {
 
 	struct v4l2_prio_state prio;		/* V4L2 priority state */
 	u32 caps;				/* V4L2 chain-wide caps */
+	u8 ctrl_class_bitmap;			/* Bitmap of valid classes */
 };
 
 struct uvc_stats_frame {
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 09/22] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:27AM +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
> 
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 94 ++++++++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h |  5 ++
>  2 files changed, 99 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index b75da65115ef..ba14733db757 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -357,6 +357,15 @@ static const struct uvc_control_info uvc_ctrls[] = {
>  	},
>  };
>  
> +static const struct uvc_control_class uvc_control_class[] = {

Nitpicking, I'd name the array (not the struct) uvc_control_classes. And
as the structure only contains a single field, could we make this an
array of u32 ?

> +	{
> +		.id		= V4L2_CID_CAMERA_CLASS,
> +	},
> +	{
> +		.id		= V4L2_CID_USER_CLASS,
> +	},
> +};
> +
>  static const struct uvc_menu_info power_line_frequency_controls[] = {
>  	{ 0, "Disabled" },
>  	{ 1, "50 Hz" },
> @@ -1024,6 +1033,49 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
>  	return 0;
>  }
>  
> +static int __uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
> +				  u32 found_id)
> +{
> +	bool find_next = req_id & V4L2_CTRL_FLAG_NEXT_CTRL;
> +	unsigned int i;
> +
> +	req_id &= V4L2_CTRL_ID_MASK;
> +
> +	for (i = 0; i < ARRAY_SIZE(uvc_control_class); i++) {
> +		if (!(chain->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_video_chain *chain, u32 req_id,
> +				u32 found_id, struct v4l2_queryctrl *v4l2_ctrl)
> +{
> +	int idx;
> +
> +	idx = __uvc_query_v4l2_class(chain, 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, v4l2_ctrl_get_name(v4l2_ctrl->id),
> +		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;

Could you align the | below the = ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	return 0;
> +}
> +
>  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl,
>  	struct uvc_control_mapping *mapping,
> @@ -1127,12 +1179,31 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  	if (ret < 0)
>  		return -ERESTARTSYS;
>  
> +	/* Check if the ctrl is a know class */
> +	if (!(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)) {
> +		ret = uvc_query_v4l2_class(chain, v4l2_ctrl->id, 0, v4l2_ctrl);
> +		if (!ret)
> +			goto done;
> +	}
> +
>  	ctrl = uvc_find_control(chain, v4l2_ctrl->id, &mapping);
>  	if (ctrl == NULL) {
>  		ret = -EINVAL;
>  		goto done;
>  	}
>  
> +	/*
> +	 * 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.
> +	 */
> +	if (v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL) {
> +		ret = uvc_query_v4l2_class(chain, 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);
> @@ -1426,6 +1497,11 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
>  	if (ret < 0)
>  		return -ERESTARTSYS;
>  
> +	if (__uvc_query_v4l2_class(handle->chain, sev->id, 0) >= 0) {
> +		ret = 0;
> +		goto done;
> +	}
> +
>  	ctrl = uvc_find_control(handle->chain, sev->id, &mapping);
>  	if (ctrl == NULL) {
>  		ret = -EINVAL;
> @@ -1459,7 +1535,10 @@ static void uvc_ctrl_del_event(struct v4l2_subscribed_event *sev)
>  	struct uvc_fh *handle = container_of(sev->fh, struct uvc_fh, vfh);
>  
>  	mutex_lock(&handle->chain->ctrl_mutex);
> +	if (__uvc_query_v4l2_class(handle->chain, sev->id, 0) >= 0)
> +		goto done;
>  	list_del(&sev->node);
> +done:
>  	mutex_unlock(&handle->chain->ctrl_mutex);
>  }
>  
> @@ -1577,6 +1656,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, xctrl->id, 0) >= 0)
> +		return -EACCES;
> +
>  	ctrl = uvc_find_control(chain, xctrl->id, &mapping);
>  	if (ctrl == NULL)
>  		return -EINVAL;
> @@ -1596,6 +1678,9 @@ int uvc_ctrl_set(struct uvc_fh *handle,
>  	s32 max;
>  	int ret;
>  
> +	if (__uvc_query_v4l2_class(chain, xctrl->id, 0) >= 0)
> +		return -EACCES;
> +
>  	ctrl = uvc_find_control(chain, xctrl->id, &mapping);
>  	if (ctrl == NULL)
>  		return -EINVAL;
> @@ -2062,6 +2147,7 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  {
>  	struct uvc_control_mapping *map;
>  	unsigned int size;
> +	unsigned 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 +2171,14 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  	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)) {
> +			chain->ctrl_class_bitmap |= BIT(i);
> +			break;
> +		}
> +	}
> +
>  	list_add_tail(&map->list, &ctrl->info.mappings);
>  	uvc_dbg(chain->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..b81d3f65e52e 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -262,6 +262,10 @@ struct uvc_control_mapping {
>  		    u8 *data);
>  };
>  
> +struct uvc_control_class {
> +	u32 id;
> +};
> +
>  struct uvc_control {
>  	struct uvc_entity *entity;
>  	struct uvc_control_info info;
> @@ -475,6 +479,7 @@ struct uvc_video_chain {
>  
>  	struct v4l2_prio_state prio;		/* V4L2 priority state */
>  	u32 caps;				/* V4L2 chain-wide caps */
> +	u8 ctrl_class_bitmap;			/* Bitmap of valid classes */
>  };
>  
>  struct uvc_stats_frame {

-- 
Regards,

Laurent Pinchart

[PATCH v9 10/22] media: uvcvideo: Use dev->name for querycap()

[Ricardo Ribalda]

Use the device name for the card name instead of vdev->name.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_metadata.c | 2 +-
 drivers/media/usb/uvc/uvc_v4l2.c     | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
index b6279ad7ac84..82de7781f5b6 100644
--- a/drivers/media/usb/uvc/uvc_metadata.c
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -30,7 +30,7 @@ static int uvc_meta_v4l2_querycap(struct file *file, void *fh,
 	struct uvc_video_chain *chain = stream->chain;
 
 	strscpy(cap->driver, "uvcvideo", sizeof(cap->driver));
-	strscpy(cap->card, vfh->vdev->name, sizeof(cap->card));
+	strscpy(cap->card, stream->dev->name, sizeof(cap->card));
 	usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
 	cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
 			  | chain->caps;
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 1eeeb00280e4..9cdd30eff495 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -617,13 +617,12 @@ static int uvc_v4l2_release(struct file *file)
 static int uvc_ioctl_querycap(struct file *file, void *fh,
 			      struct v4l2_capability *cap)
 {
-	struct video_device *vdev = video_devdata(file);
 	struct uvc_fh *handle = file->private_data;
 	struct uvc_video_chain *chain = handle->chain;
 	struct uvc_streaming *stream = handle->stream;
 
 	strscpy(cap->driver, "uvcvideo", sizeof(cap->driver));
-	strscpy(cap->card, vdev->name, sizeof(cap->card));
+	strscpy(cap->card, handle->stream->dev->name, sizeof(cap->card));
 	usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
 	cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
 			  | chain->caps;
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 10/22] media: uvcvideo: Use dev->name for querycap()

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:28AM +0100, Ricardo Ribalda wrote:
> Use the device name for the card name instead of vdev->name.

The commit message should explain at least briefly why this is desired.

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_metadata.c | 2 +-
>  drivers/media/usb/uvc/uvc_v4l2.c     | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> index b6279ad7ac84..82de7781f5b6 100644
> --- a/drivers/media/usb/uvc/uvc_metadata.c
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -30,7 +30,7 @@ static int uvc_meta_v4l2_querycap(struct file *file, void *fh,
>  	struct uvc_video_chain *chain = stream->chain;
>  
>  	strscpy(cap->driver, "uvcvideo", sizeof(cap->driver));
> -	strscpy(cap->card, vfh->vdev->name, sizeof(cap->card));
> +	strscpy(cap->card, stream->dev->name, sizeof(cap->card));
>  	usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
>  	cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
>  			  | chain->caps;
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 1eeeb00280e4..9cdd30eff495 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -617,13 +617,12 @@ static int uvc_v4l2_release(struct file *file)
>  static int uvc_ioctl_querycap(struct file *file, void *fh,
>  			      struct v4l2_capability *cap)
>  {
> -	struct video_device *vdev = video_devdata(file);
>  	struct uvc_fh *handle = file->private_data;
>  	struct uvc_video_chain *chain = handle->chain;
>  	struct uvc_streaming *stream = handle->stream;
>  
>  	strscpy(cap->driver, "uvcvideo", sizeof(cap->driver));
> -	strscpy(cap->card, vdev->name, sizeof(cap->card));
> +	strscpy(cap->card, handle->stream->dev->name, sizeof(cap->card));
>  	usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap->bus_info));
>  	cap->capabilities = V4L2_CAP_DEVICE_CAPS | V4L2_CAP_STREAMING
>  			  | chain->caps;

-- 
Regards,

Laurent Pinchart

[PATCH v9 11/22] media: uvcvideo: Set unique vdev name based in type

[Ricardo Ribalda]

All the entities must have a unique name. We can have a descriptive and
unique name by appending the function and the entity->id.

This is even resilent to multi chain devices.

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>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/uvc/uvc_driver.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 35873cf2773d..76ab6acecbc9 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2163,6 +2163,7 @@ int uvc_register_video_device(struct uvc_device *dev,
 			      const struct v4l2_ioctl_ops *ioctl_ops)
 {
 	int ret;
+	const char *name;
 
 	/* Initialize the video buffers queue. */
 	ret = uvc_queue_init(queue, type, !uvc_no_drop_param);
@@ -2190,16 +2191,20 @@ int uvc_register_video_device(struct uvc_device *dev,
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 	default:
 		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+		name = "Video Capture";
 		break;
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
 		vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
+		name = "Video Output";
 		break;
 	case V4L2_BUF_TYPE_META_CAPTURE:
 		vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
+		name = "Metadata";
 		break;
 	}
 
-	strscpy(vdev->name, dev->name, sizeof(vdev->name));
+	snprintf(vdev->name, sizeof(vdev->name), "%s %u", name,
+		 stream->header.bTerminalLink);
 
 	/*
 	 * Set the driver data before calling video_register_device, otherwise
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 11/22] media: uvcvideo: Set unique vdev name based in type

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:29AM +0100, Ricardo Ribalda wrote:
> All the entities must have a unique name. We can have a descriptive and
> unique name by appending the function and the entity->id.
> 
> This is even resilent to multi chain devices.
> 
> 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>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 35873cf2773d..76ab6acecbc9 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2163,6 +2163,7 @@ int uvc_register_video_device(struct uvc_device *dev,
>  			      const struct v4l2_ioctl_ops *ioctl_ops)
>  {
>  	int ret;
> +	const char *name;

Please swap the two variables.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	/* Initialize the video buffers queue. */
>  	ret = uvc_queue_init(queue, type, !uvc_no_drop_param);
> @@ -2190,16 +2191,20 @@ int uvc_register_video_device(struct uvc_device *dev,
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>  	default:
>  		vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +		name = "Video Capture";
>  		break;
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>  		vdev->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
> +		name = "Video Output";
>  		break;
>  	case V4L2_BUF_TYPE_META_CAPTURE:
>  		vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
> +		name = "Metadata";
>  		break;
>  	}
>  
> -	strscpy(vdev->name, dev->name, sizeof(vdev->name));
> +	snprintf(vdev->name, sizeof(vdev->name), "%s %u", name,
> +		 stream->header.bTerminalLink);
>  
>  	/*
>  	 * Set the driver data before calling video_register_device, otherwise

-- 
Regards,

Laurent Pinchart

[PATCH v9 12/22] media: uvcvideo: Increase the size of UVC_METADATA_BUF_SIZE

[Ricardo Ribalda]

Hans has discovered that in his test device, for the H264 format
bytesused goes up to about 570, for YUYV it will actually go up
to a bit over 5000 bytes, and for MJPG up to about 2706 bytes.

We should also, according to V4L2_META_FMT_UVC docs, drop headers when
the buffer is full.

Credit-to: Hans Verkuil <hverkuil@xs4all.nl>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 8 +++++---
 drivers/media/usb/uvc/uvcvideo.h  | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 25fd8aa23529..ea2903dc3252 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1244,11 +1244,13 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
 	if (!meta_buf || length == 2)
 		return;
 
+	/*
+	 * According to V4L2_META_FMT_UVC docs, we should drop headers when
+	 * the buffer is full.
+	 */
 	if (meta_buf->length - meta_buf->bytesused <
-	    length + sizeof(meta->ns) + sizeof(meta->sof)) {
-		meta_buf->error = 1;
+	    length + sizeof(meta->ns) + sizeof(meta->sof))
 		return;
-	}
 
 	has_pts = mem[1] & UVC_STREAM_PTS;
 	has_scr = mem[1] & UVC_STREAM_SCR;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b81d3f65e52e..a26bbec8d37b 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -527,7 +527,7 @@ struct uvc_stats_stream {
 	unsigned int max_sof;		/* Maximum STC.SOF value */
 };
 
-#define UVC_METADATA_BUF_SIZE 1024
+#define UVC_METADATA_BUF_SIZE 10240
 
 /**
  * struct uvc_copy_op: Context structure to schedule asynchronous memcpy
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 12/22] media: uvcvideo: Increase the size of UVC_METADATA_BUF_SIZE

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:30AM +0100, Ricardo Ribalda wrote:
> Hans has discovered that in his test device, for the H264 format
> bytesused goes up to about 570, for YUYV it will actually go up
> to a bit over 5000 bytes, and for MJPG up to about 2706 bytes.
> 
> We should also, according to V4L2_META_FMT_UVC docs, drop headers when
> the buffer is full.
> 
> Credit-to: Hans Verkuil <hverkuil@xs4all.nl>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 8 +++++---
>  drivers/media/usb/uvc/uvcvideo.h  | 2 +-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 25fd8aa23529..ea2903dc3252 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1244,11 +1244,13 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
>  	if (!meta_buf || length == 2)
>  		return;
>  
> +	/*
> +	 * According to V4L2_META_FMT_UVC docs, we should drop headers when
> +	 * the buffer is full.
> +	 */
>  	if (meta_buf->length - meta_buf->bytesused <
> -	    length + sizeof(meta->ns) + sizeof(meta->sof)) {
> -		meta_buf->error = 1;

Shouldn't you still set meta_buf->error to 1, even if you drop headers ?
Otherwise the application can't tell if headers have been dropped.

With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	    length + sizeof(meta->ns) + sizeof(meta->sof))
>  		return;
> -	}
>  
>  	has_pts = mem[1] & UVC_STREAM_PTS;
>  	has_scr = mem[1] & UVC_STREAM_SCR;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b81d3f65e52e..a26bbec8d37b 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -527,7 +527,7 @@ struct uvc_stats_stream {
>  	unsigned int max_sof;		/* Maximum STC.SOF value */
>  };
>  
> -#define UVC_METADATA_BUF_SIZE 1024
> +#define UVC_METADATA_BUF_SIZE 10240
>  
>  /**
>   * struct uvc_copy_op: Context structure to schedule asynchronous memcpy

-- 
Regards,

Laurent Pinchart

[PATCH v9 13/22] media: uvcvideo: Use control names from framework

[Ricardo Ribalda]

The framework already contains a map of IDs to names, lets use it when
possible.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 57 ++++++++++++--------------------
 drivers/media/usb/uvc/uvc_v4l2.c |  8 ++++-
 drivers/media/usb/uvc/uvcvideo.h |  2 +-
 3 files changed, 30 insertions(+), 37 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index ba14733db757..929e70dff11a 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -436,7 +436,6 @@ static void uvc_ctrl_set_rel_speed(struct uvc_control_mapping *mapping,
 static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	{
 		.id		= V4L2_CID_BRIGHTNESS,
-		.name		= "Brightness",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_BRIGHTNESS_CONTROL,
 		.size		= 16,
@@ -446,7 +445,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_CONTRAST,
-		.name		= "Contrast",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_CONTRAST_CONTROL,
 		.size		= 16,
@@ -456,7 +454,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_HUE,
-		.name		= "Hue",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_HUE_CONTROL,
 		.size		= 16,
@@ -468,7 +465,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_SATURATION,
-		.name		= "Saturation",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_SATURATION_CONTROL,
 		.size		= 16,
@@ -478,7 +474,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_SHARPNESS,
-		.name		= "Sharpness",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_SHARPNESS_CONTROL,
 		.size		= 16,
@@ -488,7 +483,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_GAMMA,
-		.name		= "Gamma",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_GAMMA_CONTROL,
 		.size		= 16,
@@ -498,7 +492,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_BACKLIGHT_COMPENSATION,
-		.name		= "Backlight Compensation",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_BACKLIGHT_COMPENSATION_CONTROL,
 		.size		= 16,
@@ -508,7 +501,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_GAIN,
-		.name		= "Gain",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_GAIN_CONTROL,
 		.size		= 16,
@@ -518,7 +510,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_POWER_LINE_FREQUENCY,
-		.name		= "Power Line Frequency",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
 		.size		= 2,
@@ -530,7 +521,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_HUE_AUTO,
-		.name		= "Hue, Auto",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_HUE_AUTO_CONTROL,
 		.size		= 1,
@@ -541,7 +531,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_EXPOSURE_AUTO,
-		.name		= "Exposure, Auto",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_AE_MODE_CONTROL,
 		.size		= 4,
@@ -554,7 +543,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_EXPOSURE_AUTO_PRIORITY,
-		.name		= "Exposure, Auto Priority",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_AE_PRIORITY_CONTROL,
 		.size		= 1,
@@ -564,7 +552,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_EXPOSURE_ABSOLUTE,
-		.name		= "Exposure (Absolute)",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_EXPOSURE_TIME_ABSOLUTE_CONTROL,
 		.size		= 32,
@@ -576,7 +563,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_AUTO_WHITE_BALANCE,
-		.name		= "White Balance Temperature, Auto",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_WHITE_BALANCE_TEMPERATURE_AUTO_CONTROL,
 		.size		= 1,
@@ -587,7 +573,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_WHITE_BALANCE_TEMPERATURE,
-		.name		= "White Balance Temperature",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_WHITE_BALANCE_TEMPERATURE_CONTROL,
 		.size		= 16,
@@ -599,7 +584,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_AUTO_WHITE_BALANCE,
-		.name		= "White Balance Component, Auto",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_WHITE_BALANCE_COMPONENT_AUTO_CONTROL,
 		.size		= 1,
@@ -611,7 +595,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_BLUE_BALANCE,
-		.name		= "White Balance Blue Component",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_WHITE_BALANCE_COMPONENT_CONTROL,
 		.size		= 16,
@@ -623,7 +606,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_RED_BALANCE,
-		.name		= "White Balance Red Component",
 		.entity		= UVC_GUID_UVC_PROCESSING,
 		.selector	= UVC_PU_WHITE_BALANCE_COMPONENT_CONTROL,
 		.size		= 16,
@@ -635,7 +617,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_FOCUS_ABSOLUTE,
-		.name		= "Focus (absolute)",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_FOCUS_ABSOLUTE_CONTROL,
 		.size		= 16,
@@ -647,7 +628,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_FOCUS_AUTO,
-		.name		= "Focus, Auto",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_FOCUS_AUTO_CONTROL,
 		.size		= 1,
@@ -658,7 +638,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_IRIS_ABSOLUTE,
-		.name		= "Iris, Absolute",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_IRIS_ABSOLUTE_CONTROL,
 		.size		= 16,
@@ -668,7 +647,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_IRIS_RELATIVE,
-		.name		= "Iris, Relative",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_IRIS_RELATIVE_CONTROL,
 		.size		= 8,
@@ -678,7 +656,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_ZOOM_ABSOLUTE,
-		.name		= "Zoom, Absolute",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_ZOOM_ABSOLUTE_CONTROL,
 		.size		= 16,
@@ -688,7 +665,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_ZOOM_CONTINUOUS,
-		.name		= "Zoom, Continuous",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_ZOOM_RELATIVE_CONTROL,
 		.size		= 0,
@@ -700,7 +676,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_PAN_ABSOLUTE,
-		.name		= "Pan (Absolute)",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_PANTILT_ABSOLUTE_CONTROL,
 		.size		= 32,
@@ -710,7 +685,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_TILT_ABSOLUTE,
-		.name		= "Tilt (Absolute)",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_PANTILT_ABSOLUTE_CONTROL,
 		.size		= 32,
@@ -720,7 +694,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_PAN_SPEED,
-		.name		= "Pan (Speed)",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_PANTILT_RELATIVE_CONTROL,
 		.size		= 16,
@@ -732,7 +705,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_TILT_SPEED,
-		.name		= "Tilt (Speed)",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_PANTILT_RELATIVE_CONTROL,
 		.size		= 16,
@@ -744,7 +716,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_PRIVACY,
-		.name		= "Privacy",
 		.entity		= UVC_GUID_UVC_CAMERA,
 		.selector	= UVC_CT_PRIVACY_CONTROL,
 		.size		= 1,
@@ -754,7 +725,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	},
 	{
 		.id		= V4L2_CID_PRIVACY,
-		.name		= "Privacy",
 		.entity		= UVC_GUID_EXT_GPIO_CONTROLLER,
 		.selector	= UVC_CT_PRIVACY_CONTROL,
 		.size		= 1,
@@ -1076,6 +1046,20 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
 	return 0;
 }
 
+static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
+{
+	const char *name;
+
+	if (map->name)
+		return map->name;
+
+	name = v4l2_ctrl_get_name(map->id);
+	if (name)
+		return name;
+
+	return "Unknown Control";
+}
+
 static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 	struct uvc_control *ctrl,
 	struct uvc_control_mapping *mapping,
@@ -1089,7 +1073,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 	memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
 	v4l2_ctrl->id = mapping->id;
 	v4l2_ctrl->type = mapping->v4l2_type;
-	strscpy(v4l2_ctrl->name, mapping->name, sizeof(v4l2_ctrl->name));
+	strscpy(v4l2_ctrl->name, uvc_map_get_name(mapping),
+		sizeof(v4l2_ctrl->name));
 	v4l2_ctrl->flags = 0;
 
 	if (!(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
@@ -2181,7 +2166,8 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 
 	list_add_tail(&map->list, &ctrl->info.mappings);
 	uvc_dbg(chain->dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
-		map->name, ctrl->info.entity, ctrl->info.selector);
+		uvc_map_get_name(map), ctrl->info.entity,
+		ctrl->info.selector);
 
 	return 0;
 }
@@ -2199,7 +2185,7 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 	if (mapping->id & ~V4L2_CTRL_ID_MASK) {
 		uvc_dbg(dev, CONTROL,
 			"Can't add mapping '%s', control id 0x%08x is invalid\n",
-			mapping->name, mapping->id);
+			uvc_map_get_name(mapping), mapping->id);
 		return -EINVAL;
 	}
 
@@ -2246,7 +2232,7 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 		if (mapping->id == map->id) {
 			uvc_dbg(dev, CONTROL,
 				"Can't add mapping '%s', control id 0x%08x already exists\n",
-				mapping->name, mapping->id);
+				uvc_map_get_name(mapping), mapping->id);
 			ret = -EEXIST;
 			goto done;
 		}
@@ -2257,7 +2243,7 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 		atomic_dec(&dev->nmappings);
 		uvc_dbg(dev, CONTROL,
 			"Can't add mapping '%s', maximum mappings count (%u) exceeded\n",
-			mapping->name, UVC_MAX_CONTROL_MAPPINGS);
+			uvc_map_get_name(mapping), UVC_MAX_CONTROL_MAPPINGS);
 		ret = -ENOMEM;
 		goto done;
 	}
@@ -2466,6 +2452,7 @@ static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev,
 	list_for_each_entry_safe(mapping, nm, &ctrl->info.mappings, list) {
 		list_del(&mapping->list);
 		kfree(mapping->menu_info);
+		kfree(mapping->name);
 		kfree(mapping);
 	}
 }
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 9cdd30eff495..28ccaa8b9e42 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -40,7 +40,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
 		return -ENOMEM;
 
 	map->id = xmap->id;
-	memcpy(map->name, xmap->name, sizeof(map->name));
+	/* Non standard control id. */
+	if (v4l2_ctrl_get_name(map->id) == NULL) {
+		map->name = kmemdup(xmap->name, sizeof(xmap->name),
+				    GFP_KERNEL);
+		if (!map->name)
+			return -ENOMEM;
+	}
 	memcpy(map->entity, xmap->entity, sizeof(map->entity));
 	map->selector = xmap->selector;
 	map->size = xmap->size;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index a26bbec8d37b..dc20021f7ee0 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -240,7 +240,7 @@ struct uvc_control_mapping {
 	struct list_head ev_subs;
 
 	u32 id;
-	u8 name[32];
+	char *name;
 	u8 entity[16];
 	u8 selector;
 
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 13/22] media: uvcvideo: Use control names from framework

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:31AM +0100, Ricardo Ribalda wrote:
> The framework already contains a map of IDs to names, lets use it when
> possible.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 57 ++++++++++++--------------------
>  drivers/media/usb/uvc/uvc_v4l2.c |  8 ++++-
>  drivers/media/usb/uvc/uvcvideo.h |  2 +-
>  3 files changed, 30 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index ba14733db757..929e70dff11a 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -436,7 +436,6 @@ static void uvc_ctrl_set_rel_speed(struct uvc_control_mapping *mapping,
>  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	{
>  		.id		= V4L2_CID_BRIGHTNESS,
> -		.name		= "Brightness",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_BRIGHTNESS_CONTROL,
>  		.size		= 16,
> @@ -446,7 +445,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_CONTRAST,
> -		.name		= "Contrast",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_CONTRAST_CONTROL,
>  		.size		= 16,
> @@ -456,7 +454,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_HUE,
> -		.name		= "Hue",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_HUE_CONTROL,
>  		.size		= 16,
> @@ -468,7 +465,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_SATURATION,
> -		.name		= "Saturation",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_SATURATION_CONTROL,
>  		.size		= 16,
> @@ -478,7 +474,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_SHARPNESS,
> -		.name		= "Sharpness",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_SHARPNESS_CONTROL,
>  		.size		= 16,
> @@ -488,7 +483,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_GAMMA,
> -		.name		= "Gamma",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_GAMMA_CONTROL,
>  		.size		= 16,
> @@ -498,7 +492,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_BACKLIGHT_COMPENSATION,
> -		.name		= "Backlight Compensation",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_BACKLIGHT_COMPENSATION_CONTROL,
>  		.size		= 16,
> @@ -508,7 +501,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_GAIN,
> -		.name		= "Gain",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_GAIN_CONTROL,
>  		.size		= 16,
> @@ -518,7 +510,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> -		.name		= "Power Line Frequency",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
>  		.size		= 2,
> @@ -530,7 +521,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_HUE_AUTO,
> -		.name		= "Hue, Auto",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_HUE_AUTO_CONTROL,
>  		.size		= 1,
> @@ -541,7 +531,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_EXPOSURE_AUTO,
> -		.name		= "Exposure, Auto",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_AE_MODE_CONTROL,
>  		.size		= 4,
> @@ -554,7 +543,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_EXPOSURE_AUTO_PRIORITY,
> -		.name		= "Exposure, Auto Priority",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_AE_PRIORITY_CONTROL,
>  		.size		= 1,
> @@ -564,7 +552,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_EXPOSURE_ABSOLUTE,
> -		.name		= "Exposure (Absolute)",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_EXPOSURE_TIME_ABSOLUTE_CONTROL,
>  		.size		= 32,
> @@ -576,7 +563,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_AUTO_WHITE_BALANCE,
> -		.name		= "White Balance Temperature, Auto",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_WHITE_BALANCE_TEMPERATURE_AUTO_CONTROL,
>  		.size		= 1,
> @@ -587,7 +573,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_WHITE_BALANCE_TEMPERATURE,
> -		.name		= "White Balance Temperature",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_WHITE_BALANCE_TEMPERATURE_CONTROL,
>  		.size		= 16,
> @@ -599,7 +584,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_AUTO_WHITE_BALANCE,
> -		.name		= "White Balance Component, Auto",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_WHITE_BALANCE_COMPONENT_AUTO_CONTROL,
>  		.size		= 1,
> @@ -611,7 +595,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_BLUE_BALANCE,
> -		.name		= "White Balance Blue Component",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_WHITE_BALANCE_COMPONENT_CONTROL,
>  		.size		= 16,
> @@ -623,7 +606,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_RED_BALANCE,
> -		.name		= "White Balance Red Component",
>  		.entity		= UVC_GUID_UVC_PROCESSING,
>  		.selector	= UVC_PU_WHITE_BALANCE_COMPONENT_CONTROL,
>  		.size		= 16,
> @@ -635,7 +617,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_FOCUS_ABSOLUTE,
> -		.name		= "Focus (absolute)",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_FOCUS_ABSOLUTE_CONTROL,
>  		.size		= 16,
> @@ -647,7 +628,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_FOCUS_AUTO,
> -		.name		= "Focus, Auto",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_FOCUS_AUTO_CONTROL,
>  		.size		= 1,
> @@ -658,7 +638,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_IRIS_ABSOLUTE,
> -		.name		= "Iris, Absolute",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_IRIS_ABSOLUTE_CONTROL,
>  		.size		= 16,
> @@ -668,7 +647,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_IRIS_RELATIVE,
> -		.name		= "Iris, Relative",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_IRIS_RELATIVE_CONTROL,
>  		.size		= 8,
> @@ -678,7 +656,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_ZOOM_ABSOLUTE,
> -		.name		= "Zoom, Absolute",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_ZOOM_ABSOLUTE_CONTROL,
>  		.size		= 16,
> @@ -688,7 +665,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_ZOOM_CONTINUOUS,
> -		.name		= "Zoom, Continuous",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_ZOOM_RELATIVE_CONTROL,
>  		.size		= 0,
> @@ -700,7 +676,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_PAN_ABSOLUTE,
> -		.name		= "Pan (Absolute)",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_PANTILT_ABSOLUTE_CONTROL,
>  		.size		= 32,
> @@ -710,7 +685,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_TILT_ABSOLUTE,
> -		.name		= "Tilt (Absolute)",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_PANTILT_ABSOLUTE_CONTROL,
>  		.size		= 32,
> @@ -720,7 +694,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_PAN_SPEED,
> -		.name		= "Pan (Speed)",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_PANTILT_RELATIVE_CONTROL,
>  		.size		= 16,
> @@ -732,7 +705,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_TILT_SPEED,
> -		.name		= "Tilt (Speed)",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_PANTILT_RELATIVE_CONTROL,
>  		.size		= 16,
> @@ -744,7 +716,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_PRIVACY,
> -		.name		= "Privacy",
>  		.entity		= UVC_GUID_UVC_CAMERA,
>  		.selector	= UVC_CT_PRIVACY_CONTROL,
>  		.size		= 1,
> @@ -754,7 +725,6 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	},
>  	{
>  		.id		= V4L2_CID_PRIVACY,
> -		.name		= "Privacy",
>  		.entity		= UVC_GUID_EXT_GPIO_CONTROLLER,
>  		.selector	= UVC_CT_PRIVACY_CONTROL,
>  		.size		= 1,
> @@ -1076,6 +1046,20 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>  	return 0;
>  }
>  
> +static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
> +{
> +	const char *name;
> +
> +	if (map->name)
> +		return map->name;
> +
> +	name = v4l2_ctrl_get_name(map->id);
> +	if (name)
> +		return name;
> +
> +	return "Unknown Control";
> +}
> +
>  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl,
>  	struct uvc_control_mapping *mapping,
> @@ -1089,7 +1073,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  	memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
>  	v4l2_ctrl->id = mapping->id;
>  	v4l2_ctrl->type = mapping->v4l2_type;
> -	strscpy(v4l2_ctrl->name, mapping->name, sizeof(v4l2_ctrl->name));
> +	strscpy(v4l2_ctrl->name, uvc_map_get_name(mapping),
> +		sizeof(v4l2_ctrl->name));
>  	v4l2_ctrl->flags = 0;
>  
>  	if (!(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
> @@ -2181,7 +2166,8 @@ static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  
>  	list_add_tail(&map->list, &ctrl->info.mappings);
>  	uvc_dbg(chain->dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
> -		map->name, ctrl->info.entity, ctrl->info.selector);
> +		uvc_map_get_name(map), ctrl->info.entity,
> +		ctrl->info.selector);
>  
>  	return 0;
>  }
> @@ -2199,7 +2185,7 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  	if (mapping->id & ~V4L2_CTRL_ID_MASK) {
>  		uvc_dbg(dev, CONTROL,
>  			"Can't add mapping '%s', control id 0x%08x is invalid\n",
> -			mapping->name, mapping->id);
> +			uvc_map_get_name(mapping), mapping->id);
>  		return -EINVAL;
>  	}
>  
> @@ -2246,7 +2232,7 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  		if (mapping->id == map->id) {
>  			uvc_dbg(dev, CONTROL,
>  				"Can't add mapping '%s', control id 0x%08x already exists\n",
> -				mapping->name, mapping->id);
> +				uvc_map_get_name(mapping), mapping->id);
>  			ret = -EEXIST;
>  			goto done;
>  		}
> @@ -2257,7 +2243,7 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  		atomic_dec(&dev->nmappings);
>  		uvc_dbg(dev, CONTROL,
>  			"Can't add mapping '%s', maximum mappings count (%u) exceeded\n",
> -			mapping->name, UVC_MAX_CONTROL_MAPPINGS);
> +			uvc_map_get_name(mapping), UVC_MAX_CONTROL_MAPPINGS);
>  		ret = -ENOMEM;
>  		goto done;
>  	}
> @@ -2466,6 +2452,7 @@ static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev,
>  	list_for_each_entry_safe(mapping, nm, &ctrl->info.mappings, list) {
>  		list_del(&mapping->list);
>  		kfree(mapping->menu_info);
> +		kfree(mapping->name);
>  		kfree(mapping);
>  	}
>  }
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 9cdd30eff495..28ccaa8b9e42 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -40,7 +40,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
>  		return -ENOMEM;
>  
>  	map->id = xmap->id;
> -	memcpy(map->name, xmap->name, sizeof(map->name));
> +	/* Non standard control id. */
> +	if (v4l2_ctrl_get_name(map->id) == NULL) {
> +		map->name = kmemdup(xmap->name, sizeof(xmap->name),
> +				    GFP_KERNEL);
> +		if (!map->name)
> +			return -ENOMEM;
> +	}
>  	memcpy(map->entity, xmap->entity, sizeof(map->entity));
>  	map->selector = xmap->selector;
>  	map->size = xmap->size;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index a26bbec8d37b..dc20021f7ee0 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -240,7 +240,7 @@ struct uvc_control_mapping {
>  	struct list_head ev_subs;
>  
>  	u32 id;
> -	u8 name[32];
> +	char *name;
>  	u8 entity[16];
>  	u8 selector;
>  

-- 
Regards,

Laurent Pinchart

[PATCH v9 14/22] media: uvcvideo: Check controls flags before accessing them

[Ricardo Ribalda]

We can figure out if reading/writing a set of controls can fail without
accessing them by checking their flags.

This way we can honor the API closer:

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.

Fixes v4l2-compliance:
Control ioctls (Input 0):
		warn: v4l2-test-controls.cpp(765): g_ext_ctrls(0) invalid error_idx 0
                fail: v4l2-test-controls.cpp(645): invalid error index write only control
        test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 22 ++++++++++++++++++
 drivers/media/usb/uvc/uvc_v4l2.c | 39 ++++++++++++++++++++++++++++----
 drivers/media/usb/uvc/uvcvideo.h |  2 ++
 3 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 929e70dff11a..24fd5afc4e4f 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1046,6 +1046,28 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
 	return 0;
 }
 
+int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
+			   bool read)
+{
+	struct uvc_control_mapping *mapping;
+	struct uvc_control *ctrl;
+
+	if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
+		return -EACCES;
+
+	ctrl = uvc_find_control(chain, v4l2_id, &mapping);
+	if (!ctrl)
+		return -EINVAL;
+
+	if (!(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) && read)
+		return -EACCES;
+
+	if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
+		return -EACCES;
+
+	return 0;
+}
+
 static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
 {
 	const char *name;
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 28ccaa8b9e42..a3ee1dc003fc 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -991,6 +991,26 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
 	return 0;
 }
 
+static int uvc_ctrl_check_access(struct uvc_video_chain *chain,
+				 struct v4l2_ext_controls *ctrls,
+				 unsigned long ioctl)
+{
+	struct v4l2_ext_control *ctrl = ctrls->controls;
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
+		ret = uvc_ctrl_is_accessible(chain, ctrl->id,
+					    ioctl == VIDIOC_G_EXT_CTRLS);
+		if (ret)
+			break;
+	}
+
+	ctrls->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i : ctrls->count;
+
+	return ret;
+}
+
 static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 				 struct v4l2_ext_controls *ctrls)
 {
@@ -1000,6 +1020,10 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 	unsigned int i;
 	int ret;
 
+	ret = uvc_ctrl_check_access(chain, ctrls, VIDIOC_G_EXT_CTRLS);
+	if (ret < 0)
+		return ret;
+
 	if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
 		for (i = 0; i < ctrls->count; ++ctrl, ++i) {
 			struct v4l2_queryctrl qc = { .id = ctrl->id };
@@ -1036,13 +1060,17 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 
 static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 				     struct v4l2_ext_controls *ctrls,
-				     bool commit)
+				     unsigned long ioctl)
 {
 	struct v4l2_ext_control *ctrl = ctrls->controls;
 	struct uvc_video_chain *chain = handle->chain;
 	unsigned int i;
 	int ret;
 
+	ret = uvc_ctrl_check_access(chain, ctrls, ioctl);
+	if (ret < 0)
+		return ret;
+
 	ret = uvc_ctrl_begin(chain);
 	if (ret < 0)
 		return ret;
@@ -1051,14 +1079,15 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 		ret = uvc_ctrl_set(handle, ctrl);
 		if (ret < 0) {
 			uvc_ctrl_rollback(handle);
-			ctrls->error_idx = commit ? ctrls->count : i;
+			ctrls->error_idx = ioctl == VIDIOC_S_EXT_CTRLS ?
+						    ctrls->count : i;
 			return ret;
 		}
 	}
 
 	ctrls->error_idx = 0;
 
-	if (commit)
+	if (ioctl == VIDIOC_S_EXT_CTRLS)
 		return uvc_ctrl_commit(handle, ctrls->controls, ctrls->count);
 	else
 		return uvc_ctrl_rollback(handle);
@@ -1069,7 +1098,7 @@ static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 
-	return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, true);
+	return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_S_EXT_CTRLS);
 }
 
 static int uvc_ioctl_try_ext_ctrls(struct file *file, void *fh,
@@ -1077,7 +1106,7 @@ static int uvc_ioctl_try_ext_ctrls(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 
-	return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, false);
+	return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS);
 }
 
 static int uvc_ioctl_querymenu(struct file *file, void *fh,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index dc20021f7ee0..9471c342a310 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -902,6 +902,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
 
 int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
 int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
+int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
+			   bool read);
 
 int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
 		      struct uvc_xu_control_query *xqry);
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 14/22] media: uvcvideo: Check controls flags before accessing them

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:32AM +0100, Ricardo Ribalda wrote:
> We can figure out if reading/writing a set of controls can fail without
> accessing them by checking their flags.
> 
> This way we can honor the API closer:
> 
> 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.
> 
> Fixes v4l2-compliance:
> Control ioctls (Input 0):
> 		warn: v4l2-test-controls.cpp(765): g_ext_ctrls(0) invalid error_idx 0
>                 fail: v4l2-test-controls.cpp(645): invalid error index write only control
>         test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
> 
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 22 ++++++++++++++++++
>  drivers/media/usb/uvc/uvc_v4l2.c | 39 ++++++++++++++++++++++++++++----
>  drivers/media/usb/uvc/uvcvideo.h |  2 ++
>  3 files changed, 58 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 929e70dff11a..24fd5afc4e4f 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1046,6 +1046,28 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>  	return 0;
>  }
>  
> +int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> +			   bool read)
> +{
> +	struct uvc_control_mapping *mapping;
> +	struct uvc_control *ctrl;
> +
> +	if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
> +		return -EACCES;
> +
> +	ctrl = uvc_find_control(chain, v4l2_id, &mapping);
> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	if (!(ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) && read)
> +		return -EACCES;
> +
> +	if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
> +		return -EACCES;
> +
> +	return 0;
> +}
> +
>  static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
>  {
>  	const char *name;
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 28ccaa8b9e42..a3ee1dc003fc 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -991,6 +991,26 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
>  	return 0;
>  }
>  
> +static int uvc_ctrl_check_access(struct uvc_video_chain *chain,
> +				 struct v4l2_ext_controls *ctrls,
> +				 unsigned long ioctl)
> +{
> +	struct v4l2_ext_control *ctrl = ctrls->controls;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> +		ret = uvc_ctrl_is_accessible(chain, ctrl->id,
> +					    ioctl == VIDIOC_G_EXT_CTRLS);
> +		if (ret)
> +			break;
> +	}
> +
> +	ctrls->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i : ctrls->count;
> +
> +	return ret;
> +}
> +
>  static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  				 struct v4l2_ext_controls *ctrls)
>  {
> @@ -1000,6 +1020,10 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  	unsigned int i;
>  	int ret;
>  
> +	ret = uvc_ctrl_check_access(chain, ctrls, VIDIOC_G_EXT_CTRLS);
> +	if (ret < 0)
> +		return ret;
> +
>  	if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
>  		for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>  			struct v4l2_queryctrl qc = { .id = ctrl->id };
> @@ -1036,13 +1060,17 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  
>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  				     struct v4l2_ext_controls *ctrls,
> -				     bool commit)
> +				     unsigned long ioctl)
>  {
>  	struct v4l2_ext_control *ctrl = ctrls->controls;
>  	struct uvc_video_chain *chain = handle->chain;
>  	unsigned int i;
>  	int ret;
>  
> +	ret = uvc_ctrl_check_access(chain, ctrls, ioctl);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = uvc_ctrl_begin(chain);
>  	if (ret < 0)
>  		return ret;
> @@ -1051,14 +1079,15 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  		ret = uvc_ctrl_set(handle, ctrl);
>  		if (ret < 0) {
>  			uvc_ctrl_rollback(handle);
> -			ctrls->error_idx = commit ? ctrls->count : i;
> +			ctrls->error_idx = ioctl == VIDIOC_S_EXT_CTRLS ?
> +						    ctrls->count : i;
>  			return ret;
>  		}
>  	}
>  
>  	ctrls->error_idx = 0;
>  
> -	if (commit)
> +	if (ioctl == VIDIOC_S_EXT_CTRLS)
>  		return uvc_ctrl_commit(handle, ctrls->controls, ctrls->count);
>  	else
>  		return uvc_ctrl_rollback(handle);
> @@ -1069,7 +1098,7 @@ static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  
> -	return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, true);
> +	return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_S_EXT_CTRLS);
>  }
>  
>  static int uvc_ioctl_try_ext_ctrls(struct file *file, void *fh,
> @@ -1077,7 +1106,7 @@ static int uvc_ioctl_try_ext_ctrls(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  
> -	return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, false);
> +	return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS);
>  }
>  
>  static int uvc_ioctl_querymenu(struct file *file, void *fh,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index dc20021f7ee0..9471c342a310 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -902,6 +902,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
>  
>  int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
>  int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
> +int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> +			   bool read);
>  
>  int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>  		      struct uvc_xu_control_query *xqry);

-- 
Regards,

Laurent Pinchart

[PATCH v9 15/22] media: uvcvideo: Set error_idx during ctrl_commit errors

[Ricardo Ribalda]

If we have an error setting a control, return the affected control in
the error_idx field.

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 42 ++++++++++++++++++++++++++------
 drivers/media/usb/uvc/uvc_v4l2.c |  2 +-
 drivers/media/usb/uvc/uvcvideo.h | 10 +++-----
 3 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 24fd5afc4e4f..bcebf9d1a46f 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1586,7 +1586,7 @@ int uvc_ctrl_begin(struct uvc_video_chain *chain)
 }
 
 static int uvc_ctrl_commit_entity(struct uvc_device *dev,
-	struct uvc_entity *entity, int rollback)
+	struct uvc_entity *entity, int rollback, struct uvc_control **err_ctrl)
 {
 	struct uvc_control *ctrl;
 	unsigned int i;
@@ -1628,31 +1628,59 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 
 		ctrl->dirty = 0;
 
-		if (ret < 0)
+		if (ret < 0) {
+			if (err_ctrl)
+				*err_ctrl = ctrl;
 			return ret;
+		}
 	}
 
 	return 0;
 }
 
+static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity,
+				 struct v4l2_ext_controls *ctrls,
+				 struct uvc_control *uvc_control)
+{
+	struct uvc_control_mapping *mapping;
+	struct uvc_control *ctrl_found;
+	unsigned int i;
+
+	if (!entity)
+		return ctrls->count;
+
+	for (i = 0; i < ctrls->count; i++) {
+		__uvc_find_control(entity, ctrls->controls[i].id, &mapping,
+				   &ctrl_found, 0);
+		if (uvc_control == ctrl_found)
+			return i;
+	}
+
+	return ctrls->count;
+}
+
 int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
-		      const struct v4l2_ext_control *xctrls,
-		      unsigned int xctrls_count)
+		      struct v4l2_ext_controls *ctrls)
 {
 	struct uvc_video_chain *chain = handle->chain;
+	struct uvc_control *err_ctrl;
 	struct uvc_entity *entity;
 	int ret = 0;
 
 	/* Find the control. */
 	list_for_each_entry(entity, &chain->entities, chain) {
-		ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback);
+		ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback,
+					     &err_ctrl);
 		if (ret < 0)
 			goto done;
 	}
 
 	if (!rollback)
-		uvc_ctrl_send_events(handle, xctrls, xctrls_count);
+		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
 done:
+	if (ret < 0 && ctrls)
+		ctrls->error_idx = uvc_ctrl_find_ctrlidx(entity, ctrls,
+							 err_ctrl);
 	mutex_unlock(&chain->ctrl_mutex);
 	return ret;
 }
@@ -2110,7 +2138,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
 			ctrl->dirty = 1;
 		}
 
-		ret = uvc_ctrl_commit_entity(dev, entity, 0);
+		ret = uvc_ctrl_commit_entity(dev, entity, 0, NULL);
 		if (ret < 0)
 			return ret;
 	}
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index a3ee1dc003fc..8d8b12a4db34 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1088,7 +1088,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 	ctrls->error_idx = 0;
 
 	if (ioctl == VIDIOC_S_EXT_CTRLS)
-		return uvc_ctrl_commit(handle, ctrls->controls, ctrls->count);
+		return uvc_ctrl_commit(handle, ctrls);
 	else
 		return uvc_ctrl_rollback(handle);
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 9471c342a310..0313b30f0cea 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -887,17 +887,15 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
 
 int uvc_ctrl_begin(struct uvc_video_chain *chain);
 int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
-		      const struct v4l2_ext_control *xctrls,
-		      unsigned int xctrls_count);
+		      struct v4l2_ext_controls *ctrls);
 static inline int uvc_ctrl_commit(struct uvc_fh *handle,
-				  const struct v4l2_ext_control *xctrls,
-				  unsigned int xctrls_count)
+				  struct v4l2_ext_controls *ctrls)
 {
-	return __uvc_ctrl_commit(handle, 0, xctrls, xctrls_count);
+	return __uvc_ctrl_commit(handle, 0, ctrls);
 }
 static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
 {
-	return __uvc_ctrl_commit(handle, 1, NULL, 0);
+	return __uvc_ctrl_commit(handle, 1, NULL);
 }
 
 int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 15/22] media: uvcvideo: Set error_idx during ctrl_commit errors

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:33AM +0100, Ricardo Ribalda wrote:
> If we have an error setting a control, return the affected control in
> the error_idx field.
> 
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 42 ++++++++++++++++++++++++++------
>  drivers/media/usb/uvc/uvc_v4l2.c |  2 +-
>  drivers/media/usb/uvc/uvcvideo.h | 10 +++-----
>  3 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 24fd5afc4e4f..bcebf9d1a46f 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1586,7 +1586,7 @@ int uvc_ctrl_begin(struct uvc_video_chain *chain)
>  }
>  
>  static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> -	struct uvc_entity *entity, int rollback)
> +	struct uvc_entity *entity, int rollback, struct uvc_control **err_ctrl)
>  {
>  	struct uvc_control *ctrl;
>  	unsigned int i;
> @@ -1628,31 +1628,59 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  
>  		ctrl->dirty = 0;
>  
> -		if (ret < 0)
> +		if (ret < 0) {
> +			if (err_ctrl)
> +				*err_ctrl = ctrl;
>  			return ret;
> +		}
>  	}
>  
>  	return 0;
>  }
>  
> +static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity,

s/uvc_ctrl_find_ctrlidx/uvc_ctrl_find_ctrl_idx/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +				 struct v4l2_ext_controls *ctrls,
> +				 struct uvc_control *uvc_control)
> +{
> +	struct uvc_control_mapping *mapping;
> +	struct uvc_control *ctrl_found;
> +	unsigned int i;
> +
> +	if (!entity)
> +		return ctrls->count;
> +
> +	for (i = 0; i < ctrls->count; i++) {
> +		__uvc_find_control(entity, ctrls->controls[i].id, &mapping,
> +				   &ctrl_found, 0);
> +		if (uvc_control == ctrl_found)
> +			return i;
> +	}
> +
> +	return ctrls->count;
> +}
> +
>  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> -		      const struct v4l2_ext_control *xctrls,
> -		      unsigned int xctrls_count)
> +		      struct v4l2_ext_controls *ctrls)
>  {
>  	struct uvc_video_chain *chain = handle->chain;
> +	struct uvc_control *err_ctrl;
>  	struct uvc_entity *entity;
>  	int ret = 0;
>  
>  	/* Find the control. */
>  	list_for_each_entry(entity, &chain->entities, chain) {
> -		ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback);
> +		ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback,
> +					     &err_ctrl);
>  		if (ret < 0)
>  			goto done;
>  	}
>  
>  	if (!rollback)
> -		uvc_ctrl_send_events(handle, xctrls, xctrls_count);
> +		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
>  done:
> +	if (ret < 0 && ctrls)
> +		ctrls->error_idx = uvc_ctrl_find_ctrlidx(entity, ctrls,
> +							 err_ctrl);
>  	mutex_unlock(&chain->ctrl_mutex);
>  	return ret;
>  }
> @@ -2110,7 +2138,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
>  			ctrl->dirty = 1;
>  		}
>  
> -		ret = uvc_ctrl_commit_entity(dev, entity, 0);
> +		ret = uvc_ctrl_commit_entity(dev, entity, 0, NULL);
>  		if (ret < 0)
>  			return ret;
>  	}
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index a3ee1dc003fc..8d8b12a4db34 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1088,7 +1088,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  	ctrls->error_idx = 0;
>  
>  	if (ioctl == VIDIOC_S_EXT_CTRLS)
> -		return uvc_ctrl_commit(handle, ctrls->controls, ctrls->count);
> +		return uvc_ctrl_commit(handle, ctrls);
>  	else
>  		return uvc_ctrl_rollback(handle);
>  }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 9471c342a310..0313b30f0cea 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -887,17 +887,15 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>  
>  int uvc_ctrl_begin(struct uvc_video_chain *chain);
>  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> -		      const struct v4l2_ext_control *xctrls,
> -		      unsigned int xctrls_count);
> +		      struct v4l2_ext_controls *ctrls);
>  static inline int uvc_ctrl_commit(struct uvc_fh *handle,
> -				  const struct v4l2_ext_control *xctrls,
> -				  unsigned int xctrls_count)
> +				  struct v4l2_ext_controls *ctrls)
>  {
> -	return __uvc_ctrl_commit(handle, 0, xctrls, xctrls_count);
> +	return __uvc_ctrl_commit(handle, 0, ctrls);
>  }
>  static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
>  {
> -	return __uvc_ctrl_commit(handle, 1, NULL, 0);
> +	return __uvc_ctrl_commit(handle, 1, NULL);
>  }
>  
>  int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);

-- 
Regards,

Laurent Pinchart

[PATCH v9 16/22] media: uvcvideo: Return -EACCES to inactive controls

[Ricardo Ribalda]

If a control is inactive return -EACCES to let the userspace know that
the value will not be applied automatically when the control is active
again.

Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 71 +++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index bcebf9d1a46f..d9d4add1e813 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1082,13 +1082,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
 	return "Unknown Control";
 }
 
+static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain,
+				 struct uvc_control *ctrl,
+				 struct uvc_control_mapping *mapping)
+{
+	struct uvc_control_mapping *master_map = NULL;
+	struct uvc_control *master_ctrl = NULL;
+	s32 val;
+	int ret;
+
+	if (!mapping->master_id)
+		return false;
+
+	__uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
+			   &master_ctrl, 0);
+
+	if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
+		return false;
+
+	ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
+	if (ret < 0 || val == mapping->master_manual)
+		return false;
+
+	return true;
+}
+
 static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 	struct uvc_control *ctrl,
 	struct uvc_control_mapping *mapping,
 	struct v4l2_queryctrl *v4l2_ctrl)
 {
-	struct uvc_control_mapping *master_map = NULL;
-	struct uvc_control *master_ctrl = NULL;
 	const struct uvc_menu_info *menu;
 	unsigned int i;
 
@@ -1104,18 +1127,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 	if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
 		v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-	if (mapping->master_id)
-		__uvc_find_control(ctrl->entity, mapping->master_id,
-				   &master_map, &master_ctrl, 0);
-	if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
-		s32 val;
-		int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
-		if (ret < 0)
-			return ret;
-
-		if (val != mapping->master_manual)
-				v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
-	}
+	if (uvc_ctrl_is_inactive(chain, ctrl, mapping))
+		v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
 
 	if (!ctrl->cached) {
 		int ret = uvc_ctrl_populate_cache(chain, ctrl);
@@ -1638,25 +1651,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 	return 0;
 }
 
-static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity,
+static int uvc_ctrl_commit_error(struct uvc_video_chain *chain,
+				 struct uvc_entity *entity,
 				 struct v4l2_ext_controls *ctrls,
-				 struct uvc_control *uvc_control)
+				 struct uvc_control *err_control,
+				 int ret)
 {
 	struct uvc_control_mapping *mapping;
 	struct uvc_control *ctrl_found;
 	unsigned int i;
 
-	if (!entity)
-		return ctrls->count;
+	if (!entity) {
+		ctrls->error_idx = ctrls->count;
+		return ret;
+	}
 
 	for (i = 0; i < ctrls->count; i++) {
 		__uvc_find_control(entity, ctrls->controls[i].id, &mapping,
 				   &ctrl_found, 0);
-		if (uvc_control == ctrl_found)
-			return i;
+		if (err_control == ctrl_found)
+			break;
 	}
+	ctrls->error_idx = i;
+
+	/* We could not find the control that failed. */
+	if (i == ctrls->count)
+		return ret;
 
-	return ctrls->count;
+	if (uvc_ctrl_is_inactive(chain, err_control, mapping))
+		return -EACCES;
+
+	return ret;
 }
 
 int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
@@ -1679,8 +1704,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
 		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
 done:
 	if (ret < 0 && ctrls)
-		ctrls->error_idx = uvc_ctrl_find_ctrlidx(entity, ctrls,
-							 err_ctrl);
+		ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl,
+					    ret);
 	mutex_unlock(&chain->ctrl_mutex);
 	return ret;
 }
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 16/22] media: uvcvideo: Return -EACCES to inactive controls

[Hans Verkuil]

On 26/03/2021 10:58, Ricardo Ribalda wrote:
> If a control is inactive return -EACCES to let the userspace know that
> the value will not be applied automatically when the control is active
> again.
> 
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 71 +++++++++++++++++++++-----------
>  1 file changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index bcebf9d1a46f..d9d4add1e813 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1082,13 +1082,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
>  	return "Unknown Control";
>  }
>  
> +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain,
> +				 struct uvc_control *ctrl,
> +				 struct uvc_control_mapping *mapping)
> +{
> +	struct uvc_control_mapping *master_map = NULL;
> +	struct uvc_control *master_ctrl = NULL;
> +	s32 val;
> +	int ret;
> +
> +	if (!mapping->master_id)
> +		return false;
> +
> +	__uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
> +			   &master_ctrl, 0);
> +
> +	if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
> +		return false;
> +
> +	ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> +	if (ret < 0 || val == mapping->master_manual)
> +		return false;
> +
> +	return true;
> +}
> +
>  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl,
>  	struct uvc_control_mapping *mapping,
>  	struct v4l2_queryctrl *v4l2_ctrl)
>  {
> -	struct uvc_control_mapping *master_map = NULL;
> -	struct uvc_control *master_ctrl = NULL;
>  	const struct uvc_menu_info *menu;
>  	unsigned int i;
>  
> @@ -1104,18 +1127,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  	if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
>  		v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
> -	if (mapping->master_id)
> -		__uvc_find_control(ctrl->entity, mapping->master_id,
> -				   &master_map, &master_ctrl, 0);
> -	if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> -		s32 val;
> -		int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> -		if (ret < 0)
> -			return ret;
> -
> -		if (val != mapping->master_manual)
> -				v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> -	}
> +	if (uvc_ctrl_is_inactive(chain, ctrl, mapping))
> +		v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>  
>  	if (!ctrl->cached) {
>  		int ret = uvc_ctrl_populate_cache(chain, ctrl);
> @@ -1638,25 +1651,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  	return 0;
>  }
>  
> -static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity,
> +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain,
> +				 struct uvc_entity *entity,
>  				 struct v4l2_ext_controls *ctrls,
> -				 struct uvc_control *uvc_control)
> +				 struct uvc_control *err_control,
> +				 int ret)
>  {
>  	struct uvc_control_mapping *mapping;
>  	struct uvc_control *ctrl_found;
>  	unsigned int i;
>  
> -	if (!entity)
> -		return ctrls->count;
> +	if (!entity) {
> +		ctrls->error_idx = ctrls->count;
> +		return ret;
> +	}
>  
>  	for (i = 0; i < ctrls->count; i++) {
>  		__uvc_find_control(entity, ctrls->controls[i].id, &mapping,
>  				   &ctrl_found, 0);
> -		if (uvc_control == ctrl_found)
> -			return i;
> +		if (err_control == ctrl_found)
> +			break;
>  	}
> +	ctrls->error_idx = i;
> +
> +	/* We could not find the control that failed. */
> +	if (i == ctrls->count)
> +		return ret;
>  
> -	return ctrls->count;
> +	if (uvc_ctrl_is_inactive(chain, err_control, mapping))
> +		return -EACCES;
> +
> +	return ret;
>  }
>  
>  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> @@ -1679,8 +1704,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
>  		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
>  done:
>  	if (ret < 0 && ctrls)
> -		ctrls->error_idx = uvc_ctrl_find_ctrlidx(entity, ctrls,
> -							 err_ctrl);
> +		ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl,
> +					    ret);
>  	mutex_unlock(&chain->ctrl_mutex);
>  	return ret;
>  }
> 

Re: [PATCH v9 16/22] media: uvcvideo: Return -EACCES to inactive controls

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:34AM +0100, Ricardo Ribalda wrote:
> If a control is inactive return -EACCES to let the userspace know that
> the value will not be applied automatically when the control is active
> again.

Isn't the device supposed to stall the control set in that case, with
the bRequestErrorCode set to "Wrong state", which uvc_query_ctrl()
translates to -EACCES already ? Could you elaborate on why this patch is
needed ?

> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 71 +++++++++++++++++++++-----------
>  1 file changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index bcebf9d1a46f..d9d4add1e813 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1082,13 +1082,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
>  	return "Unknown Control";
>  }
>  
> +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain,
> +				 struct uvc_control *ctrl,
> +				 struct uvc_control_mapping *mapping)
> +{
> +	struct uvc_control_mapping *master_map = NULL;
> +	struct uvc_control *master_ctrl = NULL;
> +	s32 val;
> +	int ret;
> +
> +	if (!mapping->master_id)
> +		return false;
> +
> +	__uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
> +			   &master_ctrl, 0);
> +
> +	if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
> +		return false;
> +
> +	ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> +	if (ret < 0 || val == mapping->master_manual)
> +		return false;
> +
> +	return true;
> +}
> +
>  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl,
>  	struct uvc_control_mapping *mapping,
>  	struct v4l2_queryctrl *v4l2_ctrl)
>  {
> -	struct uvc_control_mapping *master_map = NULL;
> -	struct uvc_control *master_ctrl = NULL;
>  	const struct uvc_menu_info *menu;
>  	unsigned int i;
>  
> @@ -1104,18 +1127,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>  	if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
>  		v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
> -	if (mapping->master_id)
> -		__uvc_find_control(ctrl->entity, mapping->master_id,
> -				   &master_map, &master_ctrl, 0);
> -	if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> -		s32 val;
> -		int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> -		if (ret < 0)
> -			return ret;

There's a small change in behaviour here, the driver used to return an
error, now it will ignore it. Is it intentional ?

> -
> -		if (val != mapping->master_manual)
> -				v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> -	}
> +	if (uvc_ctrl_is_inactive(chain, ctrl, mapping))
> +		v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>  
>  	if (!ctrl->cached) {
>  		int ret = uvc_ctrl_populate_cache(chain, ctrl);
> @@ -1638,25 +1651,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  	return 0;
>  }
>  
> -static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity,
> +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain,
> +				 struct uvc_entity *entity,
>  				 struct v4l2_ext_controls *ctrls,
> -				 struct uvc_control *uvc_control)
> +				 struct uvc_control *err_control,
> +				 int ret)
>  {
>  	struct uvc_control_mapping *mapping;
>  	struct uvc_control *ctrl_found;
>  	unsigned int i;
>  
> -	if (!entity)
> -		return ctrls->count;
> +	if (!entity) {
> +		ctrls->error_idx = ctrls->count;
> +		return ret;
> +	}
>  
>  	for (i = 0; i < ctrls->count; i++) {
>  		__uvc_find_control(entity, ctrls->controls[i].id, &mapping,
>  				   &ctrl_found, 0);
> -		if (uvc_control == ctrl_found)
> -			return i;
> +		if (err_control == ctrl_found)
> +			break;
>  	}
> +	ctrls->error_idx = i;

I think this line should be moved after the next check.

> +
> +	/* We could not find the control that failed. */
> +	if (i == ctrls->count)
> +		return ret;
>  
> -	return ctrls->count;
> +	if (uvc_ctrl_is_inactive(chain, err_control, mapping))
> +		return -EACCES;
> +
> +	return ret;
>  }
>  
>  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> @@ -1679,8 +1704,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
>  		uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
>  done:
>  	if (ret < 0 && ctrls)
> -		ctrls->error_idx = uvc_ctrl_find_ctrlidx(entity, ctrls,
> -							 err_ctrl);
> +		ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl,
> +					    ret);
>  	mutex_unlock(&chain->ctrl_mutex);
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v9 16/22] media: uvcvideo: Return -EACCES to inactive controls

[Ricardo Ribalda]

Hi Laurent

Thanks for your review!

On Thu, 10 Jun 2021 at 19:28, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Mar 26, 2021 at 10:58:34AM +0100, Ricardo Ribalda wrote:
> > If a control is inactive return -EACCES to let the userspace know that
> > the value will not be applied automatically when the control is active
> > again.
>
> Isn't the device supposed to stall the control set in that case, with
> the bRequestErrorCode set to "Wrong state", which uvc_query_ctrl()
> translates to -EACCES already ? Could you elaborate on why this patch is
> needed ?

The problem is that the hardware was not returning the equivalent of
EACCES, so we had to capture it manually.
I will try to revert the patch and capture an error.

>
> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 71 +++++++++++++++++++++-----------
> >  1 file changed, 48 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index bcebf9d1a46f..d9d4add1e813 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1082,13 +1082,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
> >       return "Unknown Control";
> >  }
> >
> > +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain,
> > +                              struct uvc_control *ctrl,
> > +                              struct uvc_control_mapping *mapping)
> > +{
> > +     struct uvc_control_mapping *master_map = NULL;
> > +     struct uvc_control *master_ctrl = NULL;
> > +     s32 val;
> > +     int ret;
> > +
> > +     if (!mapping->master_id)
> > +             return false;
> > +
> > +     __uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
> > +                        &master_ctrl, 0);
> > +
> > +     if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
> > +             return false;
> > +
> > +     ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> > +     if (ret < 0 || val == mapping->master_manual)
> > +             return false;
> > +
> > +     return true;
> > +}
> > +
> >  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> >       struct uvc_control *ctrl,
> >       struct uvc_control_mapping *mapping,
> >       struct v4l2_queryctrl *v4l2_ctrl)
> >  {
> > -     struct uvc_control_mapping *master_map = NULL;
> > -     struct uvc_control *master_ctrl = NULL;
> >       const struct uvc_menu_info *menu;
> >       unsigned int i;
> >
> > @@ -1104,18 +1127,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> >       if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
> >               v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > -     if (mapping->master_id)
> > -             __uvc_find_control(ctrl->entity, mapping->master_id,
> > -                                &master_map, &master_ctrl, 0);
> > -     if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> > -             s32 val;
> > -             int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> > -             if (ret < 0)
> > -                     return ret;
>
> There's a small change in behaviour here, the driver used to return an
> error, now it will ignore it. Is it intentional ?

AFAIK The error did not follow the v4l2 spec. You shall always be able
to query a ctrl.

I will add it to the commit message to make it clear.

>
> > -
> > -             if (val != mapping->master_manual)
> > -                             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > -     }
> > +     if (uvc_ctrl_is_inactive(chain, ctrl, mapping))
> > +             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> >
> >       if (!ctrl->cached) {
> >               int ret = uvc_ctrl_populate_cache(chain, ctrl);
> > @@ -1638,25 +1651,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> >       return 0;
> >  }
> >
> > -static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity,
> > +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain,
> > +                              struct uvc_entity *entity,
> >                                struct v4l2_ext_controls *ctrls,
> > -                              struct uvc_control *uvc_control)
> > +                              struct uvc_control *err_control,
> > +                              int ret)
> >  {
> >       struct uvc_control_mapping *mapping;
> >       struct uvc_control *ctrl_found;
> >       unsigned int i;
> >
> > -     if (!entity)
> > -             return ctrls->count;
> > +     if (!entity) {
> > +             ctrls->error_idx = ctrls->count;
> > +             return ret;
> > +     }
> >
> >       for (i = 0; i < ctrls->count; i++) {
> >               __uvc_find_control(entity, ctrls->controls[i].id, &mapping,
> >                                  &ctrl_found, 0);
> > -             if (uvc_control == ctrl_found)
> > -                     return i;
> > +             if (err_control == ctrl_found)
> > +                     break;
> >       }
> > +     ctrls->error_idx = i;
>
> I think this line should be moved after the next check.

Not really, if we cannot find a control, we cannot blame it on control 0 ;)

>
> > +
> > +     /* We could not find the control that failed. */
> > +     if (i == ctrls->count)
> > +             return ret;
> >
> > -     return ctrls->count;
> > +     if (uvc_ctrl_is_inactive(chain, err_control, mapping))
> > +             return -EACCES;
> > +
> > +     return ret;
> >  }
> >
> >  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > @@ -1679,8 +1704,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> >               uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
> >  done:
> >       if (ret < 0 && ctrls)
> > -             ctrls->error_idx = uvc_ctrl_find_ctrlidx(entity, ctrls,
> > -                                                      err_ctrl);
> > +             ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl,
> > +                                         ret);
> >       mutex_unlock(&chain->ctrl_mutex);
> >       return ret;
> >  }
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v9 16/22] media: uvcvideo: Return -EACCES to inactive controls

[Ricardo Ribalda Delgado]

Hi Laurent

On Thu, Jun 10, 2021 at 8:43 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Laurent
>
> Thanks for your review!
>
> On Thu, 10 Jun 2021 at 19:28, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Ricardo,
> >
> > Thank you for the patch.
> >
> > On Fri, Mar 26, 2021 at 10:58:34AM +0100, Ricardo Ribalda wrote:
> > > If a control is inactive return -EACCES to let the userspace know that
> > > the value will not be applied automatically when the control is active
> > > again.
> >
> > Isn't the device supposed to stall the control set in that case, with
> > the bRequestErrorCode set to "Wrong state", which uvc_query_ctrl()
> > translates to -EACCES already ? Could you elaborate on why this patch is
> > needed ?
>
> The problem is that the hardware was not returning the equivalent of
> EACCES, so we had to capture it manually.
> I will try to revert the patch and capture an error.

In my hardware after reverting the patch I am still passing v4l2-compliance.

The benefits of this patch are:

1) We return -EACCESS even if the hardware mistakenly returns a different error
2) query_ctrl returns a value for hardware errors.

Hans can you comment on this? Shall we remove the patch ?

Thanks!

>
> >
> > > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c | 71 +++++++++++++++++++++-----------
> > >  1 file changed, 48 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index bcebf9d1a46f..d9d4add1e813 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1082,13 +1082,36 @@ static const char *uvc_map_get_name(const struct uvc_control_mapping *map)
> > >       return "Unknown Control";
> > >  }
> > >
> > > +static bool uvc_ctrl_is_inactive(struct uvc_video_chain *chain,
> > > +                              struct uvc_control *ctrl,
> > > +                              struct uvc_control_mapping *mapping)
> > > +{
> > > +     struct uvc_control_mapping *master_map = NULL;
> > > +     struct uvc_control *master_ctrl = NULL;
> > > +     s32 val;
> > > +     int ret;
> > > +
> > > +     if (!mapping->master_id)
> > > +             return false;
> > > +
> > > +     __uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
> > > +                        &master_ctrl, 0);
> > > +
> > > +     if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
> > > +             return false;
> > > +
> > > +     ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> > > +     if (ret < 0 || val == mapping->master_manual)
> > > +             return false;
> > > +
> > > +     return true;
> > > +}
> > > +
> > >  static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > >       struct uvc_control *ctrl,
> > >       struct uvc_control_mapping *mapping,
> > >       struct v4l2_queryctrl *v4l2_ctrl)
> > >  {
> > > -     struct uvc_control_mapping *master_map = NULL;
> > > -     struct uvc_control *master_ctrl = NULL;
> > >       const struct uvc_menu_info *menu;
> > >       unsigned int i;
> > >
> > > @@ -1104,18 +1127,8 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > >       if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
> > >               v4l2_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >
> > > -     if (mapping->master_id)
> > > -             __uvc_find_control(ctrl->entity, mapping->master_id,
> > > -                                &master_map, &master_ctrl, 0);
> > > -     if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> > > -             s32 val;
> > > -             int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> > > -             if (ret < 0)
> > > -                     return ret;
> >
> > There's a small change in behaviour here, the driver used to return an
> > error, now it will ignore it. Is it intentional ?
>
> AFAIK The error did not follow the v4l2 spec. You shall always be able
> to query a ctrl.
>
> I will add it to the commit message to make it clear.
>
> >
> > > -
> > > -             if (val != mapping->master_manual)
> > > -                             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > > -     }
> > > +     if (uvc_ctrl_is_inactive(chain, ctrl, mapping))
> > > +             v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > >
> > >       if (!ctrl->cached) {
> > >               int ret = uvc_ctrl_populate_cache(chain, ctrl);
> > > @@ -1638,25 +1651,37 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> > >       return 0;
> > >  }
> > >
> > > -static int uvc_ctrl_find_ctrlidx(struct uvc_entity *entity,
> > > +static int uvc_ctrl_commit_error(struct uvc_video_chain *chain,
> > > +                              struct uvc_entity *entity,
> > >                                struct v4l2_ext_controls *ctrls,
> > > -                              struct uvc_control *uvc_control)
> > > +                              struct uvc_control *err_control,
> > > +                              int ret)
> > >  {
> > >       struct uvc_control_mapping *mapping;
> > >       struct uvc_control *ctrl_found;
> > >       unsigned int i;
> > >
> > > -     if (!entity)
> > > -             return ctrls->count;
> > > +     if (!entity) {
> > > +             ctrls->error_idx = ctrls->count;
> > > +             return ret;
> > > +     }
> > >
> > >       for (i = 0; i < ctrls->count; i++) {
> > >               __uvc_find_control(entity, ctrls->controls[i].id, &mapping,
> > >                                  &ctrl_found, 0);
> > > -             if (uvc_control == ctrl_found)
> > > -                     return i;
> > > +             if (err_control == ctrl_found)
> > > +                     break;
> > >       }
> > > +     ctrls->error_idx = i;
> >
> > I think this line should be moved after the next check.
>
> Not really, if we cannot find a control, we cannot blame it on control 0 ;)
>
> >
> > > +
> > > +     /* We could not find the control that failed. */
> > > +     if (i == ctrls->count)
> > > +             return ret;
> > >
> > > -     return ctrls->count;
> > > +     if (uvc_ctrl_is_inactive(chain, err_control, mapping))
> > > +             return -EACCES;
> > > +
> > > +     return ret;
> > >  }
> > >
> > >  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > > @@ -1679,8 +1704,8 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > >               uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count);
> > >  done:
> > >       if (ret < 0 && ctrls)
> > > -             ctrls->error_idx = uvc_ctrl_find_ctrlidx(entity, ctrls,
> > > -                                                      err_ctrl);
> > > +             ret = uvc_ctrl_commit_error(chain, entity, ctrls, err_ctrl,
> > > +                                         ret);
> > >       mutex_unlock(&chain->ctrl_mutex);
> > >       return ret;
> > >  }
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

[PATCH v9 17/22] media: docs: Document the behaviour of uvcdriver

[Ricardo Ribalda]

The uvc driver relies on the camera firmware to keep the control states
and therefore is not capable of changing an inactive control.

Allow returning -EACESS in those cases.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst      | 5 +++++
 Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
index 4f1bed53fad5..8c0a203385c2 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
@@ -95,3 +95,8 @@ EBUSY
 
 EACCES
     Attempt to set a read-only control or to get a write-only control.
+
+    Or if there is an attempt to set an inactive control and the driver is
+    not capable of keeping the new value until the control is active again.
+    This is the case for drivers that do not use the standard control
+    framework and rely purely on the hardware to keep the controls' state.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
index b9c62affbb5a..bb7de7a25241 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
@@ -438,3 +438,8 @@ EACCES
 
     Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
     device does not support requests.
+
+    Or if there is an attempt to set an inactive control and the driver is
+    not capable of keeping the new value until the control is active again.
+    This is the case for drivers that do not use the standard control
+    framework and rely purely on the hardware to keep the controls' state.
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 17/22] media: docs: Document the behaviour of uvcdriver

[Hans Verkuil]

On 26/03/2021 10:58, Ricardo Ribalda wrote:
> The uvc driver relies on the camera firmware to keep the control states
> and therefore is not capable of changing an inactive control.
> 
> Allow returning -EACESS in those cases.

-EACCES

> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst      | 5 +++++
>  Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
> index 4f1bed53fad5..8c0a203385c2 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
> @@ -95,3 +95,8 @@ EBUSY
>  
>  EACCES
>      Attempt to set a read-only control or to get a write-only control.
> +
> +    Or if there is an attempt to set an inactive control and the driver is
> +    not capable of keeping the new value until the control is active again.

keeping: 'caching' or 'storing' are better words, I think.

> +    This is the case for drivers that do not use the standard control
> +    framework and rely purely on the hardware to keep the controls' state.

I would drop that last sentence. It is not relevant information to the users of
the API.

> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index b9c62affbb5a..bb7de7a25241 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -438,3 +438,8 @@ EACCES
>  
>      Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
>      device does not support requests.
> +
> +    Or if there is an attempt to set an inactive control and the driver is
> +    not capable of keeping the new value until the control is active again.
> +    This is the case for drivers that do not use the standard control
> +    framework and rely purely on the hardware to keep the controls' state.

Same comments as above.

> 

Regards,

	Hans

Re: [PATCH v9 17/22] media: docs: Document the behaviour of uvcdriver

[Ricardo Ribalda]

Hi Hans

Thanks for your review!

On Sat, Mar 27, 2021 at 12:19 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 26/03/2021 10:58, Ricardo Ribalda wrote:
> > The uvc driver relies on the camera firmware to keep the control states
> > and therefore is not capable of changing an inactive control.
> >
> > Allow returning -EACESS in those cases.
>
> -EACCES

This british people that like to have a lot of double consonants :)

I have updated the series at:

https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/log/?h=uvc-compliance-v10

Will not post until there is more feedback to avoid spamming the list.

Thanks :)

>
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst      | 5 +++++
> >  Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 5 +++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
> > index 4f1bed53fad5..8c0a203385c2 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
> > @@ -95,3 +95,8 @@ EBUSY
> >
> >  EACCES
> >      Attempt to set a read-only control or to get a write-only control.
> > +
> > +    Or if there is an attempt to set an inactive control and the driver is
> > +    not capable of keeping the new value until the control is active again.
>
> keeping: 'caching' or 'storing' are better words, I think.
>
> > +    This is the case for drivers that do not use the standard control
> > +    framework and rely purely on the hardware to keep the controls' state.
>
> I would drop that last sentence. It is not relevant information to the users of
> the API.
>
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > index b9c62affbb5a..bb7de7a25241 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > @@ -438,3 +438,8 @@ EACCES
> >
> >      Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
> >      device does not support requests.
> > +
> > +    Or if there is an attempt to set an inactive control and the driver is
> > +    not capable of keeping the new value until the control is active again.
> > +    This is the case for drivers that do not use the standard control
> > +    framework and rely purely on the hardware to keep the controls' state.
>
> Same comments as above.
>
> >
>
> Regards,
>
>         Hans



-- 
Ricardo Ribalda

Re: [PATCH v9 17/22] media: docs: Document the behaviour of uvcdriver

[Hans Verkuil]

On 27/03/2021 13:01, Ricardo Ribalda wrote:
> Hi Hans
> 
> Thanks for your review!
> 
> On Sat, Mar 27, 2021 at 12:19 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 26/03/2021 10:58, Ricardo Ribalda wrote:
>>> The uvc driver relies on the camera firmware to keep the control states
>>> and therefore is not capable of changing an inactive control.
>>>
>>> Allow returning -EACESS in those cases.
>>
>> -EACCES
> 
> This british people that like to have a lot of double consonants :)
> 
> I have updated the series at:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/log/?h=uvc-compliance-v10

For v10:

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thanks!

	Hans

> 
> Will not post until there is more feedback to avoid spamming the list.
> 
> Thanks :)
> 
>>
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst      | 5 +++++
>>>  Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 5 +++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
>>> index 4f1bed53fad5..8c0a203385c2 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
>>> @@ -95,3 +95,8 @@ EBUSY
>>>
>>>  EACCES
>>>      Attempt to set a read-only control or to get a write-only control.
>>> +
>>> +    Or if there is an attempt to set an inactive control and the driver is
>>> +    not capable of keeping the new value until the control is active again.
>>
>> keeping: 'caching' or 'storing' are better words, I think.
>>
>>> +    This is the case for drivers that do not use the standard control
>>> +    framework and rely purely on the hardware to keep the controls' state.
>>
>> I would drop that last sentence. It is not relevant information to the users of
>> the API.
>>
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> index b9c62affbb5a..bb7de7a25241 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> @@ -438,3 +438,8 @@ EACCES
>>>
>>>      Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
>>>      device does not support requests.
>>> +
>>> +    Or if there is an attempt to set an inactive control and the driver is
>>> +    not capable of keeping the new value until the control is active again.
>>> +    This is the case for drivers that do not use the standard control
>>> +    framework and rely purely on the hardware to keep the controls' state.
>>
>> Same comments as above.
>>
>>>
>>
>> Regards,
>>
>>         Hans
> 
> 
> 

Re: [PATCH v9 17/22] media: docs: Document the behaviour of uvcdriver

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Sat, Mar 27, 2021 at 01:01:05PM +0100, Ricardo Ribalda wrote:
> On Sat, Mar 27, 2021 at 12:19 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > On 26/03/2021 10:58, Ricardo Ribalda wrote:
> > > The uvc driver relies on the camera firmware to keep the control states
> > > and therefore is not capable of changing an inactive control.
> > >
> > > Allow returning -EACESS in those cases.
> >
> > -EACCES
> 
> This british people that like to have a lot of double consonants :)
> 
> I have updated the series at:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/log/?h=uvc-compliance-v10
> 
> Will not post until there is more feedback to avoid spamming the list.

s/uvcdriver/uvcvideo driver/ in the subject line.

For the version in that branch,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst      | 5 +++++
> > >  Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 5 +++++
> > >  2 files changed, 10 insertions(+)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
> > > index 4f1bed53fad5..8c0a203385c2 100644
> > > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
> > > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst
> > > @@ -95,3 +95,8 @@ EBUSY
> > >
> > >  EACCES
> > >      Attempt to set a read-only control or to get a write-only control.
> > > +
> > > +    Or if there is an attempt to set an inactive control and the driver is
> > > +    not capable of keeping the new value until the control is active again.
> >
> > keeping: 'caching' or 'storing' are better words, I think.
> >
> > > +    This is the case for drivers that do not use the standard control
> > > +    framework and rely purely on the hardware to keep the controls' state.
> >
> > I would drop that last sentence. It is not relevant information to the users of
> > the API.
> >
> > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > > index b9c62affbb5a..bb7de7a25241 100644
> > > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > > @@ -438,3 +438,8 @@ EACCES
> > >
> > >      Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
> > >      device does not support requests.
> > > +
> > > +    Or if there is an attempt to set an inactive control and the driver is
> > > +    not capable of keeping the new value until the control is active again.
> > > +    This is the case for drivers that do not use the standard control
> > > +    framework and rely purely on the hardware to keep the controls' state.
> >
> > Same comments as above.

-- 
Regards,

Laurent Pinchart

[PATCH v9 18/22] media: uvcvideo: Downgrade control error messages

[Ricardo Ribalda]

Convert the error into a debug message, so they are still valid for
debugging but do not fill dmesg.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index ea2903dc3252..b63c073ec30e 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -76,7 +76,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	if (likely(ret == size))
 		return 0;
 
-	dev_err(&dev->udev->dev,
+	dev_dbg(&dev->udev->dev,
 		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
 		uvc_query_name(query), cs, unit, ret, size);
 
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 18/22] media: uvcvideo: Downgrade control error messages

[Hans Verkuil]

On 26/03/2021 10:58, Ricardo Ribalda wrote:
> Convert the error into a debug message, so they are still valid for
> debugging but do not fill dmesg.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

> ---
>  drivers/media/usb/uvc/uvc_video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index ea2903dc3252..b63c073ec30e 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -76,7 +76,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	if (likely(ret == size))
>  		return 0;
>  
> -	dev_err(&dev->udev->dev,
> +	dev_dbg(&dev->udev->dev,
>  		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
>  		uvc_query_name(query), cs, unit, ret, size);
>  
> 

Re: [PATCH v9 18/22] media: uvcvideo: Downgrade control error messages

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:36AM +0100, Ricardo Ribalda wrote:
> Convert the error into a debug message, so they are still valid for
> debugging but do not fill dmesg.

This isn't supposed to happen, doesn't it deserve an error message ?

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index ea2903dc3252..b63c073ec30e 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -76,7 +76,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	if (likely(ret == size))
>  		return 0;
>  
> -	dev_err(&dev->udev->dev,
> +	dev_dbg(&dev->udev->dev,
>  		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
>  		uvc_query_name(query), cs, unit, ret, size);
>  

-- 
Regards,

Laurent Pinchart

[PATCH v9 19/22] uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE

[Ricardo Ribalda]

From: Hans Verkuil <hverkuil@xs4all.nl>

Check for inactive controls in uvc_ctrl_is_accessible().
Use the new value for the master_id controls if present,
otherwise use the existing value to determine if it is OK
to set the control. Doing this here avoids attempting to
set an inactive control, which will return an error from the
USB device.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 28 +++++++++++++++++++++++++++-
 drivers/media/usb/uvc/uvc_v4l2.c |  4 ++--
 drivers/media/usb/uvc/uvcvideo.h |  3 ++-
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index d9d4add1e813..6e7b904bc33d 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1047,10 +1047,18 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
 }
 
 int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
-			   bool read)
+			   const struct v4l2_ext_controls *ctrls,
+			   unsigned long ioctl)
 {
+	struct uvc_control_mapping *master_map = NULL;
+	struct uvc_control *master_ctrl = NULL;
 	struct uvc_control_mapping *mapping;
 	struct uvc_control *ctrl;
+	bool read = ioctl == VIDIOC_G_EXT_CTRLS;
+	bool try = ioctl == VIDIOC_TRY_EXT_CTRLS;
+	s32 val;
+	int ret;
+	int i;
 
 	if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
 		return -EACCES;
@@ -1065,6 +1073,24 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
 	if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
 		return -EACCES;
 
+	if (read || try || !mapping->master_id)
+		return 0;
+
+	for (i = ctrls->count - 1; i >= 0; i--)
+		if (ctrls->controls[i].id == mapping->master_id)
+			return ctrls->controls[i].value ==
+					mapping->master_manual ? 0 : -EACCES;
+
+	__uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
+			   &master_ctrl, 0);
+
+	if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
+		return 0;
+
+	ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
+	if (ret >= 0 && val != mapping->master_manual)
+		return -EACCES;
+
 	return 0;
 }
 
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 8d8b12a4db34..0f4d893eff46 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1000,8 +1000,8 @@ static int uvc_ctrl_check_access(struct uvc_video_chain *chain,
 	int ret = 0;
 
 	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
-		ret = uvc_ctrl_is_accessible(chain, ctrl->id,
-					    ioctl == VIDIOC_G_EXT_CTRLS);
+		ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls,
+					    ioctl);
 		if (ret)
 			break;
 	}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 0313b30f0cea..20bc681315fb 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -901,7 +901,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
 int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
 int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
 int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
-			   bool read);
+			   const struct v4l2_ext_controls *ctrls,
+			   unsigned long ioctl);
 
 int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
 		      struct uvc_xu_control_query *xqry);
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 19/22] uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE

[Ricardo Ribalda Delgado]

Hello Hans

On Fri, Mar 26, 2021 at 11:01 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> From: Hans Verkuil <hverkuil@xs4all.nl>
>
> Check for inactive controls in uvc_ctrl_is_accessible().
> Use the new value for the master_id controls if present,
> otherwise use the existing value to determine if it is OK
> to set the control. Doing this here avoids attempting to
> set an inactive control, which will return an error from the
> USB device.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 28 +++++++++++++++++++++++++++-
>  drivers/media/usb/uvc/uvc_v4l2.c |  4 ++--
>  drivers/media/usb/uvc/uvcvideo.h |  3 ++-
>  3 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index d9d4add1e813..6e7b904bc33d 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1047,10 +1047,18 @@ static int uvc_query_v4l2_class(struct uvc_video_chain *chain, u32 req_id,
>  }
>
>  int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> -                          bool read)
> +                          const struct v4l2_ext_controls *ctrls,
> +                          unsigned long ioctl)
>  {
> +       struct uvc_control_mapping *master_map = NULL;
> +       struct uvc_control *master_ctrl = NULL;
>         struct uvc_control_mapping *mapping;
>         struct uvc_control *ctrl;
> +       bool read = ioctl == VIDIOC_G_EXT_CTRLS;
> +       bool try = ioctl == VIDIOC_TRY_EXT_CTRLS;
> +       s32 val;
> +       int ret;
> +       int i;
>
>         if (__uvc_query_v4l2_class(chain, v4l2_id, 0) >= 0)
>                 return -EACCES;
> @@ -1065,6 +1073,24 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
>         if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR) && !read)
>                 return -EACCES;
>
> +       if (read || try || !mapping->master_id)
> +               return 0;
> +
> +       for (i = ctrls->count - 1; i >= 0; i--)
> +               if (ctrls->controls[i].id == mapping->master_id)
> +                       return ctrls->controls[i].value ==
> +                                       mapping->master_manual ? 0 : -EACCES;
> +
> +       __uvc_find_control(ctrl->entity, mapping->master_id, &master_map,
> +                          &master_ctrl, 0);
> +
> +       if (!master_ctrl || !(master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR))
> +               return 0;
> +
> +       ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> +       if (ret >= 0 && val != mapping->master_manual)
> +               return -EACCES;

Maybe we could use  uvc_ctrl_is_inactive().

Please checkout:
https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/commit/?h=uvc-compliance-v10&id=f49f7c84ece18df72094a18011f2fbeb20dc1b15

and if you like it better that will be what I resend for v10 ;)

Thanks!


> +
>         return 0;
>  }
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 8d8b12a4db34..0f4d893eff46 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1000,8 +1000,8 @@ static int uvc_ctrl_check_access(struct uvc_video_chain *chain,
>         int ret = 0;
>
>         for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> -               ret = uvc_ctrl_is_accessible(chain, ctrl->id,
> -                                           ioctl == VIDIOC_G_EXT_CTRLS);
> +               ret = uvc_ctrl_is_accessible(chain, ctrl->id, ctrls,
> +                                           ioctl);
>                 if (ret)
>                         break;
>         }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 0313b30f0cea..20bc681315fb 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -901,7 +901,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
>  int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
>  int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
>  int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
> -                          bool read);
> +                          const struct v4l2_ext_controls *ctrls,
> +                          unsigned long ioctl);
>
>  int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>                       struct uvc_xu_control_query *xqry);
> --
> 2.31.0.291.g576ba9dcdaf-goog
>


--
Ricardo Ribalda

[PATCH v9 20/22] uvcvideo: improve error handling in uvc_query_ctrl()

[Ricardo Ribalda]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

- If __uvc_query_ctrl() failed with a non-EPIPE error, then
  report that with dev_err. If an error code is obtained, then
  report that with dev_dbg.

- For error 2 (Wrong state) return -EACCES instead of -EILSEQ.
  EACCES is a much more appropriate error code. EILSEQ will return
  "Invalid or incomplete multibyte or wide character." in strerror(),
  which is a *very* confusing message.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---

I have changed a bit the patch from the original version.

drivers/media/usb/uvc/uvc_video.c | 38 +++++++++++++++++--------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index b63c073ec30e..1c3a94d91724 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -76,35 +76,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	if (likely(ret == size))
 		return 0;
 
-	dev_dbg(&dev->udev->dev,
-		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
-		uvc_query_name(query), cs, unit, ret, size);
-
-	if (ret != -EPIPE)
-		return ret;
+	if (ret < 0 && ret != -EPIPE)
+		goto err;
 
+	// reuse data[0] for request the error code.
 	tmp = *(u8 *)data;
-
 	ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum,
 			       UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1,
 			       UVC_CTRL_CONTROL_TIMEOUT);
-
 	error = *(u8 *)data;
 	*(u8 *)data = tmp;
 
-	if (ret != 1)
-		return ret < 0 ? ret : -EPIPE;
+	if (ret != 1) {
+		ret = ret < 0 ? ret : -EPIPE;
+		goto err;
+	}
 
-	uvc_dbg(dev, CONTROL, "Control error %u\n", error);
+	dev_dbg(&dev->udev->dev,
+		"Failed to query (%s) UVC control %u on unit %u: got error %u.\n",
+		uvc_query_name(query), cs, unit, error);
 
 	switch (error) {
-	case 0:
-		/* Cannot happen - we received a STALL */
-		return -EPIPE;
 	case 1: /* Not ready */
 		return -EBUSY;
 	case 2: /* Wrong state */
-		return -EILSEQ;
+		return -EACCES;
 	case 3: /* Power */
 		return -EREMOTE;
 	case 4: /* Out of range */
@@ -120,10 +116,18 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	case 8: /* Invalid value within range */
 		return -EINVAL;
 	default: /* reserved or unknown */
-		break;
+		dev_err(&dev->udev->dev,
+			"Failed to query (%s) UVC control %u on unit %u: got error %u.\n",
+			uvc_query_name(query), cs, unit, error);
+		return -EPIPE;
 	}
 
-	return -EPIPE;
+err:
+	dev_err(&dev->udev->dev,
+		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
+		uvc_query_name(query), cs, unit, ret, size);
+
+	return ret;
 }
 
 static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 20/22] uvcvideo: improve error handling in uvc_query_ctrl()

[Laurent Pinchart]

Hi Ricardo and Hans,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:38AM +0100, Ricardo Ribalda wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> - If __uvc_query_ctrl() failed with a non-EPIPE error, then
>   report that with dev_err. If an error code is obtained, then
>   report that with dev_dbg.
> 
> - For error 2 (Wrong state) return -EACCES instead of -EILSEQ.
>   EACCES is a much more appropriate error code. EILSEQ will return
>   "Invalid or incomplete multibyte or wide character." in strerror(),
>   which is a *very* confusing message.

I would have split the commit in two.

> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> 
> I have changed a bit the patch from the original version.
> 
> drivers/media/usb/uvc/uvc_video.c | 38 +++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index b63c073ec30e..1c3a94d91724 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -76,35 +76,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	if (likely(ret == size))
>  		return 0;
>  
> -	dev_dbg(&dev->udev->dev,
> -		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> -		uvc_query_name(query), cs, unit, ret, size);
> -
> -	if (ret != -EPIPE)
> -		return ret;
> +	if (ret < 0 && ret != -EPIPE)
> +		goto err;
>  
> +	// reuse data[0] for request the error code.

C-style comments please.

s/for request/to request/

>  	tmp = *(u8 *)data;
> -
>  	ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum,
>  			       UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1,
>  			       UVC_CTRL_CONTROL_TIMEOUT);
> -

No need to drop the blank lines.

>  	error = *(u8 *)data;
>  	*(u8 *)data = tmp;
>  
> -	if (ret != 1)
> -		return ret < 0 ? ret : -EPIPE;
> +	if (ret != 1) {
> +		ret = ret < 0 ? ret : -EPIPE;
> +		goto err;
> +	}
>  
> -	uvc_dbg(dev, CONTROL, "Control error %u\n", error);
> +	dev_dbg(&dev->udev->dev,

Why not uvc_dbg ?

> +		"Failed to query (%s) UVC control %u on unit %u: got error %u.\n",
> +		uvc_query_name(query), cs, unit, error);
>  
>  	switch (error) {
> -	case 0:
> -		/* Cannot happen - we received a STALL */
> -		return -EPIPE;
>  	case 1: /* Not ready */
>  		return -EBUSY;
>  	case 2: /* Wrong state */
> -		return -EILSEQ;
> +		return -EACCES;
>  	case 3: /* Power */
>  		return -EREMOTE;
>  	case 4: /* Out of range */
> @@ -120,10 +116,18 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	case 8: /* Invalid value within range */
>  		return -EINVAL;
>  	default: /* reserved or unknown */
> -		break;
> +		dev_err(&dev->udev->dev,
> +			"Failed to query (%s) UVC control %u on unit %u: got error %u.\n",
> +			uvc_query_name(query), cs, unit, error);

when debugging is enabled, this will print the error message a second
time, it's not very nice.

> +		return -EPIPE;
>  	}
>  
> -	return -EPIPE;
> +err:
> +	dev_err(&dev->udev->dev,
> +		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> +		uvc_query_name(query), cs, unit, ret, size);
> +
> +	return ret;
>  }
>  
>  static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,

-- 
Regards,

Laurent Pinchart

[PATCH v9 21/22] uvcvideo: don't spam the log in uvc_ctrl_restore_values()

[Ricardo Ribalda]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Don't report the restored controls with dev_info, use dev_dbg instead.
This prevents a lot of noise in the kernel log.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 6e7b904bc33d..df6c33932557 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2182,10 +2182,10 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
 			if (!ctrl->initialized || !ctrl->modified ||
 			    (ctrl->info.flags & UVC_CTRL_FLAG_RESTORE) == 0)
 				continue;
-			dev_info(&dev->udev->dev,
-				 "restoring control %pUl/%u/%u\n",
-				 ctrl->info.entity, ctrl->info.index,
-				 ctrl->info.selector);
+			dev_dbg(&dev->udev->dev,
+				"restoring control %pUl/%u/%u\n",
+				ctrl->info.entity, ctrl->info.index,
+				ctrl->info.selector);
 			ctrl->dirty = 1;
 		}
 
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 21/22] uvcvideo: don't spam the log in uvc_ctrl_restore_values()

[Laurent Pinchart]

Hi Ricardo and Hans,

Thank you for the patch.

On Fri, Mar 26, 2021 at 10:58:39AM +0100, Ricardo Ribalda wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Don't report the restored controls with dev_info, use dev_dbg instead.
> This prevents a lot of noise in the kernel log.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 6e7b904bc33d..df6c33932557 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2182,10 +2182,10 @@ int uvc_ctrl_restore_values(struct uvc_device *dev)
>  			if (!ctrl->initialized || !ctrl->modified ||
>  			    (ctrl->info.flags & UVC_CTRL_FLAG_RESTORE) == 0)
>  				continue;
> -			dev_info(&dev->udev->dev,
> -				 "restoring control %pUl/%u/%u\n",
> -				 ctrl->info.entity, ctrl->info.index,
> -				 ctrl->info.selector);
> +			dev_dbg(&dev->udev->dev,
> +				"restoring control %pUl/%u/%u\n",
> +				ctrl->info.entity, ctrl->info.index,
> +				ctrl->info.selector);
>  			ctrl->dirty = 1;
>  		}
>  

-- 
Regards,

Laurent Pinchart

[PATCH v9 22/22] uvc: use vb2 ioctl and fop helpers

[Ricardo Ribalda]

From: Hans Verkuil <hverkuil@xs4all.nl>

When uvc was written the vb2 ioctl and file operation helpers didn't exist.

This patch switches uvc over to those helpers, which removes a lot of boilerplate
code and simplifies VIDIOC_G/S_PRIORITY handling and allows us to drop the
'privileges' scheme, since that's now handled inside the vb2 helpers.

This makes it possible for uvc to pass the v4l2-compliance streaming tests.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/uvc/uvc_driver.c   |   7 +-
 drivers/media/usb/uvc/uvc_metadata.c |   8 +-
 drivers/media/usb/uvc/uvc_queue.c    | 143 -------------
 drivers/media/usb/uvc/uvc_v4l2.c     | 288 ++-------------------------
 drivers/media/usb/uvc/uvcvideo.h     |  32 ---
 5 files changed, 25 insertions(+), 453 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 76ab6acecbc9..1a38ea31e218 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1911,7 +1911,6 @@ static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
 	INIT_LIST_HEAD(&chain->entities);
 	mutex_init(&chain->ctrl_mutex);
 	chain->dev = dev;
-	v4l2_prio_init(&chain->prio);
 
 	return chain;
 }
@@ -2181,7 +2180,7 @@ int uvc_register_video_device(struct uvc_device *dev,
 	vdev->fops = fops;
 	vdev->ioctl_ops = ioctl_ops;
 	vdev->release = uvc_release;
-	vdev->prio = &stream->chain->prio;
+	vdev->queue = &queue->queue;
 	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
 		vdev->vfl_dir = VFL_DIR_TX;
 	else
@@ -2546,8 +2545,8 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
 		if (stream->intf == intf) {
 			ret = uvc_video_resume(stream, reset);
 			if (ret < 0)
-				uvc_queue_streamoff(&stream->queue,
-						    stream->queue.queue.type);
+				vb2_streamoff(&stream->queue.queue,
+					      stream->queue.queue.type);
 			return ret;
 		}
 	}
diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
index 82de7781f5b6..d3aab22f91ce 100644
--- a/drivers/media/usb/uvc/uvc_metadata.c
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -96,7 +96,7 @@ static int uvc_meta_v4l2_set_format(struct file *file, void *fh,
 	 */
 	mutex_lock(&stream->mutex);
 
-	if (uvc_queue_allocated(&stream->queue))
+	if (vb2_is_busy(&stream->meta.queue.queue))
 		ret = -EBUSY;
 	else
 		stream->meta.format = fmt->dataformat;
@@ -164,12 +164,6 @@ int uvc_meta_register(struct uvc_streaming *stream)
 
 	stream->meta.format = V4L2_META_FMT_UVC;
 
-	/*
-	 * The video interface queue uses manual locking and thus does not set
-	 * the queue pointer. Set it manually here.
-	 */
-	vdev->queue = &queue->queue;
-
 	return uvc_register_video_device(dev, stream, vdev, queue,
 					 V4L2_BUF_TYPE_META_CAPTURE,
 					 &uvc_meta_fops, &uvc_meta_ioctl_ops);
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 21a907d32bb7..437e48b32480 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -247,153 +247,10 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
 	return 0;
 }
 
-void uvc_queue_release(struct uvc_video_queue *queue)
-{
-	mutex_lock(&queue->mutex);
-	vb2_queue_release(&queue->queue);
-	mutex_unlock(&queue->mutex);
-}
-
-/* -----------------------------------------------------------------------------
- * V4L2 queue operations
- */
-
-int uvc_request_buffers(struct uvc_video_queue *queue,
-			struct v4l2_requestbuffers *rb)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_reqbufs(&queue->queue, rb);
-	mutex_unlock(&queue->mutex);
-
-	return ret ? ret : rb->count;
-}
-
-int uvc_query_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_querybuf(&queue->queue, buf);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_create_buffers(struct uvc_video_queue *queue,
-		       struct v4l2_create_buffers *cb)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_create_bufs(&queue->queue, cb);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_queue_buffer(struct uvc_video_queue *queue,
-		     struct media_device *mdev, struct v4l2_buffer *buf)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_qbuf(&queue->queue, mdev, buf);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_export_buffer(struct uvc_video_queue *queue,
-		      struct v4l2_exportbuffer *exp)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_expbuf(&queue->queue, exp);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf,
-		       int nonblocking)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_dqbuf(&queue->queue, buf, nonblocking);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_streamon(&queue->queue, type);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_queue_streamoff(struct uvc_video_queue *queue, enum v4l2_buf_type type)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_streamoff(&queue->queue, type);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_queue_mmap(struct uvc_video_queue *queue, struct vm_area_struct *vma)
-{
-	return vb2_mmap(&queue->queue, vma);
-}
-
-#ifndef CONFIG_MMU
-unsigned long uvc_queue_get_unmapped_area(struct uvc_video_queue *queue,
-		unsigned long pgoff)
-{
-	return vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0);
-}
-#endif
-
-__poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
-			    poll_table *wait)
-{
-	__poll_t ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_poll(&queue->queue, file, wait);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
 /* -----------------------------------------------------------------------------
  *
  */
 
-/*
- * Check if buffers have been allocated.
- */
-int uvc_queue_allocated(struct uvc_video_queue *queue)
-{
-	int allocated;
-
-	mutex_lock(&queue->mutex);
-	allocated = vb2_is_busy(&queue->queue);
-	mutex_unlock(&queue->mutex);
-
-	return allocated;
-}
-
 /*
  * Cancel the video buffers queue.
  *
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 0f4d893eff46..e40db7ae18b1 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -352,17 +352,13 @@ static int uvc_v4l2_set_format(struct uvc_streaming *stream,
 		return ret;
 
 	mutex_lock(&stream->mutex);
-
-	if (uvc_queue_allocated(&stream->queue)) {
+	if (vb2_is_busy(&stream->queue.queue)) {
 		ret = -EBUSY;
-		goto done;
+	} else {
+		stream->ctrl = probe;
+		stream->cur_format = format;
+		stream->cur_frame = frame;
 	}
-
-	stream->ctrl = probe;
-	stream->cur_format = format;
-	stream->cur_frame = frame;
-
-done:
 	mutex_unlock(&stream->mutex);
 	return ret;
 }
@@ -489,62 +485,6 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
 	return 0;
 }
 
-/* ------------------------------------------------------------------------
- * Privilege management
- */
-
-/*
- * Privilege management is the multiple-open implementation basis. The current
- * implementation is completely transparent for the end-user and doesn't
- * require explicit use of the VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY ioctls.
- * Those ioctls enable finer control on the device (by making possible for a
- * user to request exclusive access to a device), but are not mature yet.
- * Switching to the V4L2 priority mechanism might be considered in the future
- * if this situation changes.
- *
- * Each open instance of a UVC device can either be in a privileged or
- * unprivileged state. Only a single instance can be in a privileged state at
- * a given time. Trying to perform an operation that requires privileges will
- * automatically acquire the required privileges if possible, or return -EBUSY
- * otherwise. Privileges are dismissed when closing the instance or when
- * freeing the video buffers using VIDIOC_REQBUFS.
- *
- * Operations that require privileges are:
- *
- * - VIDIOC_S_INPUT
- * - VIDIOC_S_PARM
- * - VIDIOC_S_FMT
- * - VIDIOC_REQBUFS
- */
-static int uvc_acquire_privileges(struct uvc_fh *handle)
-{
-	/* Always succeed if the handle is already privileged. */
-	if (handle->state == UVC_HANDLE_ACTIVE)
-		return 0;
-
-	/* Check if the device already has a privileged handle. */
-	if (atomic_inc_return(&handle->stream->active) != 1) {
-		atomic_dec(&handle->stream->active);
-		return -EBUSY;
-	}
-
-	handle->state = UVC_HANDLE_ACTIVE;
-	return 0;
-}
-
-static void uvc_dismiss_privileges(struct uvc_fh *handle)
-{
-	if (handle->state == UVC_HANDLE_ACTIVE)
-		atomic_dec(&handle->stream->active);
-
-	handle->state = UVC_HANDLE_PASSIVE;
-}
-
-static int uvc_has_privileges(struct uvc_fh *handle)
-{
-	return handle->state == UVC_HANDLE_ACTIVE;
-}
-
 /* ------------------------------------------------------------------------
  * V4L2 file operations
  */
@@ -587,7 +527,6 @@ static int uvc_v4l2_open(struct file *file)
 	v4l2_fh_add(&handle->vfh);
 	handle->chain = stream->chain;
 	handle->stream = stream;
-	handle->state = UVC_HANDLE_PASSIVE;
 	file->private_data = handle;
 
 	return 0;
@@ -600,15 +539,8 @@ static int uvc_v4l2_release(struct file *file)
 
 	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
 
-	/* Only free resources if this is a privileged handle. */
-	if (uvc_has_privileges(handle))
-		uvc_queue_release(&stream->queue);
-
 	/* Release the file handle. */
-	uvc_dismiss_privileges(handle);
-	v4l2_fh_del(&handle->vfh);
-	v4l2_fh_exit(&handle->vfh);
-	kfree(handle);
+	vb2_fop_release(file);
 	file->private_data = NULL;
 
 	mutex_lock(&stream->dev->lock);
@@ -701,11 +633,6 @@ static int uvc_ioctl_s_fmt_vid_cap(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
 
 	return uvc_v4l2_set_format(stream, fmt);
 }
@@ -715,11 +642,6 @@ static int uvc_ioctl_s_fmt_vid_out(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
 
 	return uvc_v4l2_set_format(stream, fmt);
 }
@@ -744,124 +666,6 @@ static int uvc_ioctl_try_fmt_vid_out(struct file *file, void *fh,
 	return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
 }
 
-static int uvc_ioctl_reqbufs(struct file *file, void *fh,
-			     struct v4l2_requestbuffers *rb)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
-
-	mutex_lock(&stream->mutex);
-	ret = uvc_request_buffers(&stream->queue, rb);
-	mutex_unlock(&stream->mutex);
-	if (ret < 0)
-		return ret;
-
-	if (ret == 0)
-		uvc_dismiss_privileges(handle);
-
-	return 0;
-}
-
-static int uvc_ioctl_querybuf(struct file *file, void *fh,
-			      struct v4l2_buffer *buf)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	return uvc_query_buffer(&stream->queue, buf);
-}
-
-static int uvc_ioctl_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	return uvc_queue_buffer(&stream->queue,
-				stream->vdev.v4l2_dev->mdev, buf);
-}
-
-static int uvc_ioctl_expbuf(struct file *file, void *fh,
-			    struct v4l2_exportbuffer *exp)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	return uvc_export_buffer(&stream->queue, exp);
-}
-
-static int uvc_ioctl_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	return uvc_dequeue_buffer(&stream->queue, buf,
-				  file->f_flags & O_NONBLOCK);
-}
-
-static int uvc_ioctl_create_bufs(struct file *file, void *fh,
-				  struct v4l2_create_buffers *cb)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
-
-	return uvc_create_buffers(&stream->queue, cb);
-}
-
-static int uvc_ioctl_streamon(struct file *file, void *fh,
-			      enum v4l2_buf_type type)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	mutex_lock(&stream->mutex);
-	ret = uvc_queue_streamon(&stream->queue, type);
-	mutex_unlock(&stream->mutex);
-
-	return ret;
-}
-
-static int uvc_ioctl_streamoff(struct file *file, void *fh,
-			       enum v4l2_buf_type type)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	mutex_lock(&stream->mutex);
-	uvc_queue_streamoff(&stream->queue, type);
-	mutex_unlock(&stream->mutex);
-
-	return 0;
-}
-
 static int uvc_ioctl_enum_input(struct file *file, void *fh,
 				struct v4l2_input *input)
 {
@@ -929,13 +733,12 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
 static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
 {
 	struct uvc_fh *handle = fh;
+	struct uvc_streaming *stream = handle->stream;
 	struct uvc_video_chain *chain = handle->chain;
-	int ret;
 	u32 i;
 
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
+	if (vb2_is_busy(&stream->queue.queue))
+		return -EBUSY;
 
 	if (chain->selector == NULL ||
 	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
@@ -1166,11 +969,6 @@ static int uvc_ioctl_s_parm(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
 
 	return uvc_v4l2_set_streamparm(stream, parm);
 }
@@ -1438,50 +1236,6 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 }
 #endif
 
-static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
-		    size_t count, loff_t *ppos)
-{
-	struct uvc_fh *handle = file->private_data;
-	struct uvc_streaming *stream = handle->stream;
-
-	uvc_dbg(stream->dev, CALLS, "%s: not implemented\n", __func__);
-	return -EINVAL;
-}
-
-static int uvc_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
-{
-	struct uvc_fh *handle = file->private_data;
-	struct uvc_streaming *stream = handle->stream;
-
-	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
-
-	return uvc_queue_mmap(&stream->queue, vma);
-}
-
-static __poll_t uvc_v4l2_poll(struct file *file, poll_table *wait)
-{
-	struct uvc_fh *handle = file->private_data;
-	struct uvc_streaming *stream = handle->stream;
-
-	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
-
-	return uvc_queue_poll(&stream->queue, file, wait);
-}
-
-#ifndef CONFIG_MMU
-static unsigned long uvc_v4l2_get_unmapped_area(struct file *file,
-		unsigned long addr, unsigned long len, unsigned long pgoff,
-		unsigned long flags)
-{
-	struct uvc_fh *handle = file->private_data;
-	struct uvc_streaming *stream = handle->stream;
-
-	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
-
-	return uvc_queue_get_unmapped_area(&stream->queue, pgoff);
-}
-#endif
-
 const struct v4l2_ioctl_ops uvc_ioctl_ops = {
 	.vidioc_querycap = uvc_ioctl_querycap,
 	.vidioc_enum_fmt_vid_cap = uvc_ioctl_enum_fmt_vid_cap,
@@ -1492,14 +1246,15 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
 	.vidioc_s_fmt_vid_out = uvc_ioctl_s_fmt_vid_out,
 	.vidioc_try_fmt_vid_cap = uvc_ioctl_try_fmt_vid_cap,
 	.vidioc_try_fmt_vid_out = uvc_ioctl_try_fmt_vid_out,
-	.vidioc_reqbufs = uvc_ioctl_reqbufs,
-	.vidioc_querybuf = uvc_ioctl_querybuf,
-	.vidioc_qbuf = uvc_ioctl_qbuf,
-	.vidioc_expbuf = uvc_ioctl_expbuf,
-	.vidioc_dqbuf = uvc_ioctl_dqbuf,
-	.vidioc_create_bufs = uvc_ioctl_create_bufs,
-	.vidioc_streamon = uvc_ioctl_streamon,
-	.vidioc_streamoff = uvc_ioctl_streamoff,
+	.vidioc_reqbufs = vb2_ioctl_reqbufs,
+	.vidioc_querybuf = vb2_ioctl_querybuf,
+	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
+	.vidioc_qbuf = vb2_ioctl_qbuf,
+	.vidioc_expbuf = vb2_ioctl_expbuf,
+	.vidioc_dqbuf = vb2_ioctl_dqbuf,
+	.vidioc_create_bufs = vb2_ioctl_create_bufs,
+	.vidioc_streamon = vb2_ioctl_streamon,
+	.vidioc_streamoff = vb2_ioctl_streamoff,
 	.vidioc_enum_input = uvc_ioctl_enum_input,
 	.vidioc_g_input = uvc_ioctl_g_input,
 	.vidioc_s_input = uvc_ioctl_s_input,
@@ -1527,11 +1282,10 @@ const struct v4l2_file_operations uvc_fops = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
 #endif
-	.read		= uvc_v4l2_read,
-	.mmap		= uvc_v4l2_mmap,
-	.poll		= uvc_v4l2_poll,
+	.mmap		= vb2_fop_mmap,
+	.poll		= vb2_fop_poll,
 #ifndef CONFIG_MMU
-	.get_unmapped_area = uvc_v4l2_get_unmapped_area,
+	.get_unmapped_area = vb2_fop_get_unmapped_area,
 #endif
 };
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 20bc681315fb..8849d7953767 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -477,7 +477,6 @@ struct uvc_video_chain {
 
 	struct mutex ctrl_mutex;		/* Protects ctrl.info */
 
-	struct v4l2_prio_state prio;		/* V4L2 priority state */
 	u32 caps;				/* V4L2 chain-wide caps */
 	u8 ctrl_class_bitmap;			/* Bitmap of valid classes */
 };
@@ -714,16 +713,10 @@ struct uvc_device {
 	struct uvc_entity *gpio_unit;
 };
 
-enum uvc_handle_state {
-	UVC_HANDLE_PASSIVE	= 0,
-	UVC_HANDLE_ACTIVE	= 1,
-};
-
 struct uvc_fh {
 	struct v4l2_fh vfh;
 	struct uvc_video_chain *chain;
 	struct uvc_streaming *stream;
-	enum uvc_handle_state state;
 };
 
 struct uvc_driver {
@@ -788,36 +781,11 @@ struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
 /* Video buffers queue management. */
 int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type,
 		   int drop_corrupted);
-void uvc_queue_release(struct uvc_video_queue *queue);
-int uvc_request_buffers(struct uvc_video_queue *queue,
-			struct v4l2_requestbuffers *rb);
-int uvc_query_buffer(struct uvc_video_queue *queue,
-		     struct v4l2_buffer *v4l2_buf);
-int uvc_create_buffers(struct uvc_video_queue *queue,
-		       struct v4l2_create_buffers *v4l2_cb);
-int uvc_queue_buffer(struct uvc_video_queue *queue,
-		     struct media_device *mdev,
-		     struct v4l2_buffer *v4l2_buf);
-int uvc_export_buffer(struct uvc_video_queue *queue,
-		      struct v4l2_exportbuffer *exp);
-int uvc_dequeue_buffer(struct uvc_video_queue *queue,
-		       struct v4l2_buffer *v4l2_buf, int nonblocking);
-int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type);
-int uvc_queue_streamoff(struct uvc_video_queue *queue, enum v4l2_buf_type type);
 void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect);
 struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 					 struct uvc_buffer *buf);
 struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
 void uvc_queue_buffer_release(struct uvc_buffer *buf);
-int uvc_queue_mmap(struct uvc_video_queue *queue,
-		   struct vm_area_struct *vma);
-__poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
-			poll_table *wait);
-#ifndef CONFIG_MMU
-unsigned long uvc_queue_get_unmapped_area(struct uvc_video_queue *queue,
-					  unsigned long pgoff);
-#endif
-int uvc_queue_allocated(struct uvc_video_queue *queue);
 static inline int uvc_queue_streaming(struct uvc_video_queue *queue)
 {
 	return vb2_is_streaming(&queue->queue);
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH v9 22/22] uvc: use vb2 ioctl and fop helpers

[Tomasz Figa]

Hi Ricardo,

On Fri, Mar 26, 2021 at 7:00 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> From: Hans Verkuil <hverkuil@xs4all.nl>
>
> When uvc was written the vb2 ioctl and file operation helpers didn't exist.
>
> This patch switches uvc over to those helpers, which removes a lot of boilerplate
> code and simplifies VIDIOC_G/S_PRIORITY handling and allows us to drop the
> 'privileges' scheme, since that's now handled inside the vb2 helpers.
>
> This makes it possible for uvc to pass the v4l2-compliance streaming tests.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Thanks for the patch. Did you perhaps miss adding your sign-off?

Also, see my comments inline.

[snip]
> @@ -1166,11 +969,6 @@ static int uvc_ioctl_s_parm(struct file *file, void *fh,
>  {
>         struct uvc_fh *handle = fh;
>         struct uvc_streaming *stream = handle->stream;
> -       int ret;
> -
> -       ret = uvc_acquire_privileges(handle);
> -       if (ret < 0)
> -               return ret;

Why is it okay not to acquire the privileges here?

>
>         return uvc_v4l2_set_streamparm(stream, parm);
>  }

Best regards,
Tomasz

Re: [PATCH v9 00/22] uvcvideo: Fix v4l2-compliance errors

[Tomasz Figa]

Hi everyone,

On Fri, Mar 26, 2021 at 6:58 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> *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: 0
> 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: 0
>
> YES, NO MORE WARNINGS :)
>
> Note that we depend on:
>
> https://patchwork.linuxtv.org/project/linux-media/patch/20210317143453.483470-1-ribalda@chromium.org/
>
> With Hans patch we can also pass v4l2-compliance -s.
>
> Changelog from v8 (Thanks to Hans)
> - 3 patches from Hans
> - Add Reviewed-by
>
> Hans Verkuil (4):
>   uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE
>   uvcvideo: improve error handling in uvc_query_ctrl()
>   uvcvideo: don't spam the log in uvc_ctrl_restore_values()
>   uvc: use vb2 ioctl and fop helpers
>
> Ricardo Ribalda (18):
>   media: v4l2-ioctl: Fix check_ext_ctrls
>   media: pvrusb2: Do not check for V4L2_CTRL_WHICH_DEF_VAL
>   media: uvcvideo: Do not check for V4L2_CTRL_WHICH_DEF_VAL
>   media: v4l2-ioctl: S_CTRL output the right value
>   media: uvcvideo: Remove s_ctrl and g_ctrl
>   media: uvcvideo: Set capability in s_param
>   media: uvcvideo: Return -EIO for control errors
>   media: uvcvideo: refactor __uvc_ctrl_add_mapping
>   media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS
>   media: uvcvideo: Use dev->name for querycap()
>   media: uvcvideo: Set unique vdev name based in type
>   media: uvcvideo: Increase the size of UVC_METADATA_BUF_SIZE
>   media: uvcvideo: Use control names from framework
>   media: uvcvideo: Check controls flags before accessing them
>   media: uvcvideo: Set error_idx during ctrl_commit errors
>   media: uvcvideo: Return -EACCES to inactive controls
>   media: docs: Document the behaviour of uvcdriver
>   media: uvcvideo: Downgrade control error messages
>
>  .../userspace-api/media/v4l/vidioc-g-ctrl.rst |   5 +
>  .../media/v4l/vidioc-g-ext-ctrls.rst          |   5 +
>  drivers/media/usb/pvrusb2/pvrusb2-v4l2.c      |   4 -
>  drivers/media/usb/uvc/uvc_ctrl.c              | 343 +++++++++++----
>  drivers/media/usb/uvc/uvc_driver.c            |  22 +-
>  drivers/media/usb/uvc/uvc_metadata.c          |  10 +-
>  drivers/media/usb/uvc/uvc_queue.c             | 143 -------
>  drivers/media/usb/uvc/uvc_v4l2.c              | 389 +++---------------
>  drivers/media/usb/uvc/uvc_video.c             |  51 ++-
>  drivers/media/usb/uvc/uvcvideo.h              |  54 +--
>  drivers/media/v4l2-core/v4l2-ioctl.c          |  67 +--
>  11 files changed, 444 insertions(+), 649 deletions(-)
>
> --
> 2.31.0.291.g576ba9dcdaf-goog
>

Any comments on this? Could we have this merged?

Thanks,
Tomasz