[PATCH 00/10] uvcvideo: Pass v4l2-compliance test

[PATCH 00/10] uvcvideo: Pass v4l2-compliance test

[Ricardo Ribalda]

Current version of the driver fails to pass v4l2-compliance v1.20.0,
lets patch it it so some million cameras are compliant.

Ricardo Ribalda (10):
  media: uvcvideo: Return -EINVAL for REQUEST API
  media: uvcvideo: Set capability in s_param
  media: uvcvideo: Return -EIO for control errors
  media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS
  media: uvcvideo: Define Control and GUIDs for class ctrls
  media: uvcvideo: Implement UVC_CTRL_CLASS_UNIT
  media: uvcvideo: set error_idx to count on EACCESS
  media: uvcvideo: Always return a value on V4L2_CTRL_WHICH_DEF_VAL
  media: uvcvideo: Do not create initial events for class ctrls
  media: uvcvideo: Populate only active control classes

 drivers/media/usb/uvc/uvc_ctrl.c   | 59 +++++++++++++++++++++++++++++-
 drivers/media/usb/uvc/uvc_driver.c | 51 +++++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_entity.c |  1 +
 drivers/media/usb/uvc/uvc_v4l2.c   | 20 +++++-----
 drivers/media/usb/uvc/uvc_video.c  |  2 +-
 drivers/media/usb/uvc/uvcvideo.h   | 17 +++++++++
 6 files changed, 134 insertions(+), 16 deletions(-)

-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 01/10] media: uvcvideo: Return -EINVAL for REQUEST API

[Ricardo Ribalda]

The driver does not support it.

Fixes v4l2-compliance:

Buffer ioctls (Input 0):
                fail: v4l2-test-buffers.cpp(1925): ret != EINVAL && ret != EBADR && ret != ENOTTY
        test Requests: FAIL

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

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 252136cc885c..5e3ec4a376e4 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1046,6 +1046,9 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 	unsigned int i;
 	int ret;
 
+	if (ctrls->which == V4L2_CTRL_WHICH_REQUEST_VAL)
+		return -EINVAL;
+
 	if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
 		for (i = 0; i < ctrls->count; ++ctrl, ++i) {
 			struct v4l2_queryctrl qc = { .id = ctrl->id };
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH] media: videobuf2: Explicitly state max size of planes

[Ricardo Ribalda]

The plane size needs to be PAGE_ALIGNED, so it is not possible to have
sizes bigger than MAX_INT - PAGE_SIZE.

We already check for overflows when that happen:
 if (size < vb->planes[plane].length)
	goto free;

But it is good to explicitly state our max allowed value, in order to
align with the driver expectations.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 include/media/videobuf2-core.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 799ba61b5b6f..12955cb460d2 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -154,9 +154,11 @@ struct vb2_mem_ops {
  * @dbuf:	dma_buf - shared buffer object.
  * @dbuf_mapped:	flag to show whether dbuf is mapped or not
  * @bytesused:	number of bytes occupied by data in the plane (payload).
- * @length:	size of this plane (NOT the payload) in bytes.
+ * @length:	size of this plane (NOT the payload) in bytes. The maximum
+ *		valid size is MAX_UINT - PAGE_SIZE.
  * @min_length:	minimum required size of this plane (NOT the payload) in bytes.
- *		@length is always greater or equal to @min_length.
+ *		@length is always greater or equal to @min_length, and like
+ *		@length, it is limited to MAX_UINT - PAGE_SIZE.
  * @m:		Union with memtype-specific data.
  * @m.offset:	when memory in the associated struct vb2_buffer is
  *		%VB2_MEMORY_MMAP, equals the offset from the start of
-- 
2.30.1.766.gb4fecdf3b7-goog

[PATCH 02/10] 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

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 5e3ec4a376e4..625c216c46b5 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -472,10 +472,13 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
 	uvc_simplify_fraction(&timeperframe.numerator,
 		&timeperframe.denominator, 8, 333);
 
-	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		parm->parm.capture.timeperframe = timeperframe;
-	else
+		parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
+	} else {
 		parm->parm.output.timeperframe = timeperframe;
+		parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
+	}
 
 	return 0;
 }
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH 02/10] media: uvcvideo: Set capability in s_param

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Mar 11, 2021 at 01:20:32PM +0100, Ricardo Ribalda wrote:
> Fixes v4l2-compliance:
> 
> Format ioctls (Input 0):
>                 warn: v4l2-test-formats.cpp(1339): S_PARM is supported but doesn't report V4L2_CAP_TIMEPERFRAME
>                 fail: v4l2-test-formats.cpp(1241): node->has_frmintervals && !cap->capability
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

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

> ---
>  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 5e3ec4a376e4..625c216c46b5 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;
>  }

-- 
Regards,

Laurent Pinchart

[PATCH 03/10] media: uvcvideo: Return -EIO for control errors

[Ricardo Ribalda]

Fixes v4l2-compliance:

Control ioctls (Input 0):
                fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22)
        test VIDIOC_G/S_CTRL: FAIL
                fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22)
        test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 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 f2f565281e63..5442e9be1c55 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	case 6: /* Invalid control */
 	case 7: /* Invalid Request */
 	case 8: /* Invalid value within range */
-		return -EINVAL;
+		return -EIO;
 	default: /* reserved or unknown */
 		break;
 	}
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH 03/10] media: uvcvideo: Return -EIO for control errors

[Ricardo Ribalda]

As discussed in the IRC with Hans

We need to specify in the commit message that this is most likely due
to hw error.

On Thu, Mar 11, 2021 at 1:20 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Fixes v4l2-compliance:
>
> Control ioctls (Input 0):
>                 fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22)
>         test VIDIOC_G/S_CTRL: FAIL
>                 fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22)
>         test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 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 f2f565281e63..5442e9be1c55 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>         case 6: /* Invalid control */
>         case 7: /* Invalid Request */
>         case 8: /* Invalid value within range */
> -               return -EINVAL;
> +               return -EIO;
>         default: /* reserved or unknown */
>                 break;
>         }
> --
> 2.31.0.rc2.261.g7f71774620-goog
>


-- 
Ricardo Ribalda

Re: [PATCH 03/10] media: uvcvideo: Return -EIO for control errors

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Mar 11, 2021 at 03:08:22PM +0100, Ricardo Ribalda wrote:
> As discussed in the IRC with Hans
> 
> We need to specify in the commit message that this is most likely due
> to hw error.
> 
> On Thu, Mar 11, 2021 at 1:20 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
> >
> > 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

As this isn't supposed to happen, how do you reproduce this ? 

> > 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 f2f565281e63..5442e9be1c55 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> >         case 6: /* Invalid control */
> >         case 7: /* Invalid Request */

For cases 5-7 I think -EIO is fine, as the driver should really not call
this function with an invalid unit, control or request. If it does, it's
a bug in the driver (we can check the units and controls the device
claims to support, and the requests are defined by the UVC
specification), if it doesn't and the device still returns this error,
it's a bug on the device side.

> >         case 8: /* Invalid value within range */

For this case, however, isn't it valid for a device to return an error
if the control value isn't valid ? There's one particular code path I'm
concerned about, uvc_ioctl_default(UVCIOC_CTRL_QUERY) ->
uvc_xu_ctrl_query() -> uvc_query_ctrl(), where it could be useful for
userspace to know that the value it sets isn't valid.

> > -               return -EINVAL;
> > +               return -EIO;
> >         default: /* reserved or unknown */
> >                 break;
> >         }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 03/10] media: uvcvideo: Return -EIO for control errors

[Ricardo Ribalda Delgado]

Hi Laurent

On Thu, Mar 11, 2021 at 4:59 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Thu, Mar 11, 2021 at 03:08:22PM +0100, Ricardo Ribalda wrote:
> > As discussed in the IRC with Hans
> >
> > We need to specify in the commit message that this is most likely due
> > to hw error.
> >
> > On Thu, Mar 11, 2021 at 1:20 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
> > >
> > > 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
>
> As this isn't supposed to happen, how do you reproduce this ?

Just run v4l2-compliance in my notebook camera.

>
> > > 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 f2f565281e63..5442e9be1c55 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > >         case 6: /* Invalid control */
> > >         case 7: /* Invalid Request */
>
> For cases 5-7 I think -EIO is fine, as the driver should really not call
> this function with an invalid unit, control or request. If it does, it's
> a bug in the driver (we can check the units and controls the device
> claims to support, and the requests are defined by the UVC
> specification), if it doesn't and the device still returns this error,
> it's a bug on the device side.
>
> > >         case 8: /* Invalid value within range */
>
> For this case, however, isn't it valid for a device to return an error
> if the control value isn't valid ? There's one particular code path I'm
> concerned about, uvc_ioctl_default(UVCIOC_CTRL_QUERY) ->
> uvc_xu_ctrl_query() -> uvc_query_ctrl(), where it could be useful for
> userspace to know that the value it sets isn't valid.
>

Will fix in v2

Thanks!

> > > -               return -EINVAL;
> > > +               return -EIO;
> > >         default: /* reserved or unknown */
> > >                 break;
> > >         }
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH 03/10] media: uvcvideo: Return -EIO for control errors

[Hans Verkuil]

On 11/03/2021 13:20, Ricardo Ribalda wrote:
> 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

-EIO is specifically meant for FW/HW issues. So make clear in this commit
log that if an error occurs in the code at that place, then that's because
of the device doing something unexpected.

Actually, that should be in a comment before the 'return -EIO;'.

Regards,

	Hans

> 
> 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 f2f565281e63..5442e9be1c55 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	case 6: /* Invalid control */
>  	case 7: /* Invalid Request */
>  	case 8: /* Invalid value within range */
> -		return -EINVAL;
> +		return -EIO;
>  	default: /* reserved or unknown */
>  		break;
>  	}
> 

[PATCH 04/10] media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS

[Ricardo Ribalda]

Fill all the fields for V4L2_CTRL_TYPE_CTRL_CLASS control types.

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

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index b3dde98499f4..08877897dc5a 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1097,6 +1097,13 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
 		v4l2_ctrl->step = 0;
 		return 0;
 
+	case V4L2_CTRL_TYPE_CTRL_CLASS:
+		v4l2_ctrl->minimum = 0;
+		v4l2_ctrl->maximum = 0;
+		v4l2_ctrl->step = 0;
+		v4l2_ctrl->default_value = 0;
+		return 0;
+
 	default:
 		break;
 	}
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 05/10] media: uvcvideo: Define Control and GUIDs for class ctrls

[Ricardo Ribalda]

Add new bindings for class controls. This controls will be implemented
by a virtual entity.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 34 ++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h |  8 ++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 08877897dc5a..fd4d5ad098b9 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -355,6 +355,20 @@ static const struct uvc_control_info uvc_ctrls[] = {
 		.flags		= UVC_CTRL_FLAG_GET_CUR
 				| UVC_CTRL_FLAG_AUTO_UPDATE,
 	},
+	{
+		.entity		= UVC_GUID_CTRL_CLASS,
+		.selector	= UVC_CC_CAMERA_CLASS,
+		.index		= 0,
+		.size		= 1,
+		.flags		= 0,
+	},
+	{
+		.entity		= UVC_GUID_CTRL_CLASS,
+		.selector	= UVC_CC_USER_CLASS,
+		.index		= 1,
+		.size		= 1,
+		.flags		= 0,
+	},
 };
 
 static const struct uvc_menu_info power_line_frequency_controls[] = {
@@ -753,6 +767,26 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
 		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
 	},
+	{
+		.id		= V4L2_CID_CAMERA_CLASS,
+		.name		= "Camera Controls",
+		.entity		= UVC_GUID_CTRL_CLASS,
+		.selector	= UVC_CC_CAMERA_CLASS,
+		.size		= 1,
+		.offset		= 0,
+		.v4l2_type	= V4L2_CTRL_TYPE_CTRL_CLASS,
+		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
+	},
+	{
+		.id		= V4L2_CID_USER_CLASS,
+		.name		= "User Controls",
+		.entity		= UVC_GUID_CTRL_CLASS,
+		.selector	= UVC_CC_USER_CLASS,
+		.size		= 1,
+		.offset		= 0,
+		.v4l2_type	= V4L2_CTRL_TYPE_CTRL_CLASS,
+		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
+	},
 };
 
 /* ------------------------------------------------------------------------
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 97df5ecd66c9..5792232ed312 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -62,6 +62,9 @@
 #define UVC_GUID_EXT_GPIO_CONTROLLER \
 	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
 	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03}
+#define UVC_GUID_CTRL_CLASS \
+	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
+	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x04}
 
 #define UVC_GUID_FORMAT_MJPEG \
 	{ 'M',  'J',  'P',  'G', 0x00, 0x00, 0x10, 0x00, \
@@ -175,6 +178,11 @@
 	{ 'H',  'E',  'V',  'C', 0x00, 0x00, 0x10, 0x00, \
 	 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71}
 
+/* ------------------------------------------------------------------------
+ * Control Class Constants
+ */
+#define UVC_CC_CAMERA_CLASS	0
+#define UVC_CC_USER_CLASS	1
 
 /* ------------------------------------------------------------------------
  * Driver specific constants.
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 06/10] media: uvcvideo: Implement UVC_CTRL_CLASS_UNIT

[Ricardo Ribalda]

Create a virtual entity that holds all the class control.

Fixes v4l2-compliance:
Control ioctls (Input 0):
                fail: v4l2-test-controls.cpp(216): missing control class for class 00980000
                fail: v4l2-test-controls.cpp(253): missing control class for class 009a0000
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   |  3 ++
 drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_entity.c |  1 +
 drivers/media/usb/uvc/uvcvideo.h   | 10 ++++++
 4 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index fd4d5ad098b9..273eccc136b8 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2354,6 +2354,9 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 		} else if (UVC_ENTITY_TYPE(entity) == UVC_EXT_GPIO_UNIT) {
 			bmControls = entity->gpio.bmControls;
 			bControlSize = entity->gpio.bControlSize;
+		} else if (UVC_ENTITY_TYPE(entity) == UVC_CTRL_CLASS_UNIT) {
+			bmControls = entity->ctrl_class.bmControls;
+			bControlSize = entity->ctrl_class.bControlSize;
 		}
 
 		/* Remove bogus/blacklisted controls */
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 30ef2a3110f7..996e8bd06ac5 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1025,6 +1025,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
 }
 
 static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
+static const u8 uvc_ctrl_class_guid[16] = UVC_GUID_CTRL_CLASS;
 static const u8 uvc_gpio_guid[16] = UVC_GUID_EXT_GPIO_CONTROLLER;
 static const u8 uvc_media_transport_input_guid[16] =
 	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
@@ -1057,6 +1058,9 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
 	 * is initialized by the caller.
 	 */
 	switch (type) {
+	case UVC_CTRL_CLASS_UNIT:
+		memcpy(entity->guid, uvc_ctrl_class_guid, 16);
+		break;
 	case UVC_EXT_GPIO_UNIT:
 		memcpy(entity->guid, uvc_gpio_guid, 16);
 		break;
@@ -1474,6 +1478,39 @@ static int uvc_parse_control(struct uvc_device *dev)
 	return 0;
 }
 
+/* -----------------------------------------------------------------------------
+ * Control Class
+ */
+
+static int uvc_ctrl_class_get_info(struct uvc_device *dev,
+				   struct uvc_entity *entity,
+				   u8 cs, u8 *caps)
+{
+	*caps = 0;
+	return 0;
+}
+
+static int uvc_ctrl_class_parse(struct uvc_device *dev)
+{
+	struct uvc_entity *unit;
+
+	unit = uvc_alloc_entity(UVC_CTRL_CLASS_UNIT,
+				UVC_CTRL_CLASS_UNIT_ID, 0, 1);
+	if (!unit)
+		return -ENOMEM;
+
+	unit->ctrl_class.bControlSize = 1;
+	unit->ctrl_class.bmControls = (u8 *)unit + sizeof(*unit);
+	unit->ctrl_class.bmControls[0] = (1 << (UVC_CC_LAST_CLASS + 1)) - 1;
+	unit->get_info = uvc_ctrl_class_get_info;
+	strncpy(unit->name, "Control Class", sizeof(unit->name) - 1);
+
+	list_add_tail(&unit->list, &dev->entities);
+	dev->ctrl_class_unit = unit;
+
+	return 0;
+}
+
 /* -----------------------------------------------------------------------------
  * Privacy GPIO
  */
@@ -2054,12 +2091,11 @@ static int uvc_scan_device(struct uvc_device *dev)
 		return -1;
 	}
 
-	/* Add GPIO entity to the first chain. */
-	if (dev->gpio_unit) {
-		chain = list_first_entry(&dev->chains,
-					 struct uvc_video_chain, list);
+	/* Add virtual entities to the first chain. */
+	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
+	list_add_tail(&dev->ctrl_class_unit->chain, &chain->entities);
+	if (dev->gpio_unit)
 		list_add_tail(&dev->gpio_unit->chain, &chain->entities);
-	}
 
 	return 0;
 }
@@ -2399,6 +2435,12 @@ static int uvc_probe(struct usb_interface *intf,
 		goto error;
 	}
 
+	/* Parse the control class. */
+	if (uvc_ctrl_class_parse(dev) < 0) {
+		uvc_dbg(dev, PROBE, "Unable to parse UVC CTRL CLASS\n");
+		goto error;
+	}
+
 	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
 		 dev->uvc_version >> 8, dev->uvc_version & 0xff,
 		 udev->product ? udev->product : "<unnamed>",
diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
index 7c4d2f93d351..5285030a738c 100644
--- a/drivers/media/usb/uvc/uvc_entity.c
+++ b/drivers/media/usb/uvc/uvc_entity.c
@@ -106,6 +106,7 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,
 		case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
 		case UVC_EXTERNAL_VENDOR_SPECIFIC:
 		case UVC_EXT_GPIO_UNIT:
+		case UVC_CTRL_CLASS_UNIT:
 		default:
 			function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
 			break;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 5792232ed312..1d59ac10c2eb 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -41,6 +41,9 @@
 #define UVC_EXT_GPIO_UNIT		0x7ffe
 #define UVC_EXT_GPIO_UNIT_ID		0x100
 
+#define UVC_CTRL_CLASS_UNIT		0x7ffd
+#define UVC_CTRL_CLASS_UNIT_ID		0x101
+
 /* ------------------------------------------------------------------------
  * GUIDs
  */
@@ -183,6 +186,7 @@
  */
 #define UVC_CC_CAMERA_CLASS	0
 #define UVC_CC_USER_CLASS	1
+#define UVC_CC_LAST_CLASS	UVC_CC_USER_CLASS
 
 /* ------------------------------------------------------------------------
  * Driver specific constants.
@@ -375,6 +379,11 @@ struct uvc_entity {
 			struct gpio_desc *gpio_privacy;
 			int irq;
 		} gpio;
+
+		struct {
+			u8  bControlSize;
+			u8  *bmControls;
+		} ctrl_class;
 	};
 
 	u8 bNrInPins;
@@ -715,6 +724,7 @@ struct uvc_device {
 	} async_ctrl;
 
 	struct uvc_entity *gpio_unit;
+	struct uvc_entity *ctrl_class_unit;
 };
 
 enum uvc_handle_state {
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH 06/10] media: uvcvideo: Implement UVC_CTRL_CLASS_UNIT

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Mar 11, 2021 at 01:20:36PM +0100, Ricardo Ribalda wrote:
> Create a virtual entity that holds all the class control.

Isn't this making control classes too complicated ? Can't they be
handled explicitly in uvc_query_v4l2_ctrl(), as that's the only place
where it should matter ? When registering V4L2 controls you'd set a
bitmask to track which classes are used (similarly to 10/10, but with
the bitmask stored in the chain, not an entity), and
uvc_query_v4l2_ctrl() would then handle the classes explicitly.

> Fixes v4l2-compliance:
> Control ioctls (Input 0):
>                 fail: v4l2-test-controls.cpp(216): missing control class for class 00980000
>                 fail: v4l2-test-controls.cpp(253): missing control class for class 009a0000
>         test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   |  3 ++
>  drivers/media/usb/uvc/uvc_driver.c | 52 +++++++++++++++++++++++++++---
>  drivers/media/usb/uvc/uvc_entity.c |  1 +
>  drivers/media/usb/uvc/uvcvideo.h   | 10 ++++++
>  4 files changed, 61 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index fd4d5ad098b9..273eccc136b8 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2354,6 +2354,9 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  		} else if (UVC_ENTITY_TYPE(entity) == UVC_EXT_GPIO_UNIT) {
>  			bmControls = entity->gpio.bmControls;
>  			bControlSize = entity->gpio.bControlSize;
> +		} else if (UVC_ENTITY_TYPE(entity) == UVC_CTRL_CLASS_UNIT) {
> +			bmControls = entity->ctrl_class.bmControls;
> +			bControlSize = entity->ctrl_class.bControlSize;
>  		}
>  
>  		/* Remove bogus/blacklisted controls */
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 30ef2a3110f7..996e8bd06ac5 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1025,6 +1025,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
>  }
>  
>  static const u8 uvc_camera_guid[16] = UVC_GUID_UVC_CAMERA;
> +static const u8 uvc_ctrl_class_guid[16] = UVC_GUID_CTRL_CLASS;
>  static const u8 uvc_gpio_guid[16] = UVC_GUID_EXT_GPIO_CONTROLLER;
>  static const u8 uvc_media_transport_input_guid[16] =
>  	UVC_GUID_UVC_MEDIA_TRANSPORT_INPUT;
> @@ -1057,6 +1058,9 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id,
>  	 * is initialized by the caller.
>  	 */
>  	switch (type) {
> +	case UVC_CTRL_CLASS_UNIT:
> +		memcpy(entity->guid, uvc_ctrl_class_guid, 16);
> +		break;
>  	case UVC_EXT_GPIO_UNIT:
>  		memcpy(entity->guid, uvc_gpio_guid, 16);
>  		break;
> @@ -1474,6 +1478,39 @@ static int uvc_parse_control(struct uvc_device *dev)
>  	return 0;
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Control Class
> + */
> +
> +static int uvc_ctrl_class_get_info(struct uvc_device *dev,
> +				   struct uvc_entity *entity,
> +				   u8 cs, u8 *caps)
> +{
> +	*caps = 0;
> +	return 0;
> +}
> +
> +static int uvc_ctrl_class_parse(struct uvc_device *dev)
> +{
> +	struct uvc_entity *unit;
> +
> +	unit = uvc_alloc_entity(UVC_CTRL_CLASS_UNIT,
> +				UVC_CTRL_CLASS_UNIT_ID, 0, 1);
> +	if (!unit)
> +		return -ENOMEM;
> +
> +	unit->ctrl_class.bControlSize = 1;
> +	unit->ctrl_class.bmControls = (u8 *)unit + sizeof(*unit);
> +	unit->ctrl_class.bmControls[0] = (1 << (UVC_CC_LAST_CLASS + 1)) - 1;
> +	unit->get_info = uvc_ctrl_class_get_info;
> +	strncpy(unit->name, "Control Class", sizeof(unit->name) - 1);
> +
> +	list_add_tail(&unit->list, &dev->entities);
> +	dev->ctrl_class_unit = unit;
> +
> +	return 0;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Privacy GPIO
>   */
> @@ -2054,12 +2091,11 @@ static int uvc_scan_device(struct uvc_device *dev)
>  		return -1;
>  	}
>  
> -	/* Add GPIO entity to the first chain. */
> -	if (dev->gpio_unit) {
> -		chain = list_first_entry(&dev->chains,
> -					 struct uvc_video_chain, list);
> +	/* Add virtual entities to the first chain. */
> +	chain = list_first_entry(&dev->chains, struct uvc_video_chain, list);
> +	list_add_tail(&dev->ctrl_class_unit->chain, &chain->entities);
> +	if (dev->gpio_unit)
>  		list_add_tail(&dev->gpio_unit->chain, &chain->entities);
> -	}
>  
>  	return 0;
>  }
> @@ -2399,6 +2435,12 @@ static int uvc_probe(struct usb_interface *intf,
>  		goto error;
>  	}
>  
> +	/* Parse the control class. */
> +	if (uvc_ctrl_class_parse(dev) < 0) {
> +		uvc_dbg(dev, PROBE, "Unable to parse UVC CTRL CLASS\n");
> +		goto error;
> +	}
> +
>  	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
>  		 dev->uvc_version >> 8, dev->uvc_version & 0xff,
>  		 udev->product ? udev->product : "<unnamed>",
> diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
> index 7c4d2f93d351..5285030a738c 100644
> --- a/drivers/media/usb/uvc/uvc_entity.c
> +++ b/drivers/media/usb/uvc/uvc_entity.c
> @@ -106,6 +106,7 @@ static int uvc_mc_init_entity(struct uvc_video_chain *chain,
>  		case UVC_OTT_MEDIA_TRANSPORT_OUTPUT:
>  		case UVC_EXTERNAL_VENDOR_SPECIFIC:
>  		case UVC_EXT_GPIO_UNIT:
> +		case UVC_CTRL_CLASS_UNIT:
>  		default:
>  			function = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>  			break;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 5792232ed312..1d59ac10c2eb 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -41,6 +41,9 @@
>  #define UVC_EXT_GPIO_UNIT		0x7ffe
>  #define UVC_EXT_GPIO_UNIT_ID		0x100
>  
> +#define UVC_CTRL_CLASS_UNIT		0x7ffd
> +#define UVC_CTRL_CLASS_UNIT_ID		0x101
> +
>  /* ------------------------------------------------------------------------
>   * GUIDs
>   */
> @@ -183,6 +186,7 @@
>   */
>  #define UVC_CC_CAMERA_CLASS	0
>  #define UVC_CC_USER_CLASS	1
> +#define UVC_CC_LAST_CLASS	UVC_CC_USER_CLASS
>  
>  /* ------------------------------------------------------------------------
>   * Driver specific constants.
> @@ -375,6 +379,11 @@ struct uvc_entity {
>  			struct gpio_desc *gpio_privacy;
>  			int irq;
>  		} gpio;
> +
> +		struct {
> +			u8  bControlSize;
> +			u8  *bmControls;
> +		} ctrl_class;
>  	};
>  
>  	u8 bNrInPins;
> @@ -715,6 +724,7 @@ struct uvc_device {
>  	} async_ctrl;
>  
>  	struct uvc_entity *gpio_unit;
> +	struct uvc_entity *ctrl_class_unit;
>  };
>  
>  enum uvc_handle_state {

-- 
Regards,

Laurent Pinchart

[PATCH 07/10] media: uvcvideo: set error_idx to count on EACCESS

[Ricardo Ribalda]

According to the doc:
The, in hindsight quite poor, solution for that is to set error_idx to
count if the validation failed.

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

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

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 625c216c46b5..9b6454bb2f28 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1076,7 +1076,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 		ret = uvc_ctrl_get(chain, ctrl);
 		if (ret < 0) {
 			uvc_ctrl_rollback(handle);
-			ctrls->error_idx = i;
+			ctrls->error_idx = (ret == -EACCES) ?
+						ctrls->count : i;
 			return ret;
 		}
 	}
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH 07/10] media: uvcvideo: set error_idx to count on EACCESS

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Mar 11, 2021 at 01:20:37PM +0100, Ricardo Ribalda wrote:
> According to the doc:

The previous paragraph states:

This check is done to avoid leaving the hardware in an inconsistent
state due to easy-to-avoid problems. But it leads to another problem:
the application needs to know whether an error came from the validation
step (meaning that the hardware was not touched) or from an error during
the actual reading from/writing to hardware.

> The, in hindsight quite poor, solution for that is to set error_idx to
> count if the validation failed.
> 
> Fixes v4l2-compliance:
> Control ioctls (Input 0):
>                 fail: v4l2-test-controls.cpp(645): invalid error index write only control
>         test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 625c216c46b5..9b6454bb2f28 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1076,7 +1076,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  		ret = uvc_ctrl_get(chain, ctrl);
>  		if (ret < 0) {
>  			uvc_ctrl_rollback(handle);
> -			ctrls->error_idx = i;
> +			ctrls->error_idx = (ret == -EACCES) ?
> +						ctrls->count : i;

No need for parentheses.

I'm not sure this is correct though. -EACCES is returned by
__uvc_ctrl_get() when the control is found and is a write-only control.
The uvc_ctrl_get() calls for the previous controls will have potentially
touched the device to read the current control value if it wasn't cached
already, to this contradicts the rationale from the specification.

I understand the need for this when setting controls, but when reading
them, it's more puzzling, as the interactions with the hardware to read
the controls are not supposed to affect the hardware state in a way that
applications should care about. It may be an issue in the V4L2
specification.

>  			return ret;
>  		}
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 07/10] media: uvcvideo: set error_idx to count on EACCESS

[Ricardo Ribalda Delgado]

Hi Laurent

On Thu, Mar 11, 2021 at 5:20 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.

Thank you for the review :)

>
> On Thu, Mar 11, 2021 at 01:20:37PM +0100, Ricardo Ribalda wrote:
> > According to the doc:
>
> The previous paragraph states:
>
> This check is done to avoid leaving the hardware in an inconsistent
> state due to easy-to-avoid problems. But it leads to another problem:
> the application needs to know whether an error came from the validation
> step (meaning that the hardware was not touched) or from an error during
> the actual reading from/writing to hardware.

I think we wrote his standard when we were young and naive ;)

>
> > The, in hindsight quite poor, solution for that is to set error_idx to
> > count if the validation failed.
> >
> > Fixes v4l2-compliance:
> > Control ioctls (Input 0):
> >                 fail: v4l2-test-controls.cpp(645): invalid error index write only control
> >         test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 625c216c46b5..9b6454bb2f28 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -1076,7 +1076,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
> >               ret = uvc_ctrl_get(chain, ctrl);
> >               if (ret < 0) {
> >                       uvc_ctrl_rollback(handle);
> > -                     ctrls->error_idx = i;
> > +                     ctrls->error_idx = (ret == -EACCES) ?
> > +                                             ctrls->count : i;
>
> No need for parentheses.

I really like my parenthesis before the ? :. Can I leave it? :)

If it bothers you a lot I remove it, but I like to delimit where the
ternary operators end.
>
> I'm not sure this is correct though. -EACCES is returned by
> __uvc_ctrl_get() when the control is found and is a write-only control.
> The uvc_ctrl_get() calls for the previous controls will have potentially
> touched the device to read the current control value if it wasn't cached
> already, to this contradicts the rationale from the specification.
>
> I understand the need for this when setting controls, but when reading
> them, it's more puzzling, as the interactions with the hardware to read
> the controls are not supposed to affect the hardware state in a way that
> applications should care about. It may be an issue in the V4L2
> specification.

I have no strong opinions in both directions. If v4l2-compliance is
the de-facto standard I believe we should keep this patch or change
the compliance test.

Hans, what do think?

>
> >                       return ret;
> >               }
> >       }
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

[PATCH 08/10] media: uvcvideo: Always return a value on V4L2_CTRL_WHICH_DEF_VAL

[Ricardo Ribalda]

Fixes v4l2-compliance:
Control ioctls (Input 0):
                fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)
        test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL

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

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 9b6454bb2f28..b500356fd06c 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1057,12 +1057,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 			struct v4l2_queryctrl qc = { .id = ctrl->id };
 
 			ret = uvc_query_v4l2_ctrl(chain, &qc);
-			if (ret < 0) {
-				ctrls->error_idx = i;
-				return ret;
-			}
-
-			ctrl->value = qc.default_value;
+			ctrl->value = (ret < 0) ? 0 : qc.default_value;
 		}
 
 		return 0;
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH 08/10] media: uvcvideo: Always return a value on V4L2_CTRL_WHICH_DEF_VAL

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Mar 11, 2021 at 01:20:38PM +0100, Ricardo Ribalda wrote:
> Fixes v4l2-compliance:
> Control ioctls (Input 0):
>                 fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)
>         test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 9b6454bb2f28..b500356fd06c 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1057,12 +1057,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  			struct v4l2_queryctrl qc = { .id = ctrl->id };
>  
>  			ret = uvc_query_v4l2_ctrl(chain, &qc);
> -			if (ret < 0) {
> -				ctrls->error_idx = i;
> -				return ret;
> -			}
> -
> -			ctrl->value = qc.default_value;
> +			ctrl->value = (ret < 0) ? 0 : qc.default_value;

That's not great, if an error occurs, it should be reported to the user,
not ignored silently. Sounds like this needs to be addressed in
v4l2-compliance, as the V4L2 specification doesn't forbid errors being
returned from V4L2_CTRL_WHICH_DEF_VAL.

>  		}
>  
>  		return 0;

-- 
Regards,

Laurent Pinchart

[PATCH 09/10] media: uvcvideo: Do not create initial events for class ctrls

[Ricardo Ribalda]

V4L2_CTRL_TYPE_CTRL_CLASS do not generate events.

Fixes v4l2-compliance:
Control ioctls (Input 0):
                fail: v4l2-test-controls.cpp(844): found event for control class 'User Controls'
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL

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

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 273eccc136b8..433342efc63f 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1456,6 +1456,7 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
 	}
 }
 
+static const u8 uvc_ctrl_class_guid[16] = UVC_GUID_CTRL_CLASS;
 static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 {
 	struct uvc_fh *handle = container_of(sev->fh, struct uvc_fh, vfh);
@@ -1474,7 +1475,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 	}
 
 	list_add_tail(&sev->node, &mapping->ev_subs);
-	if (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL) {
+	if ((sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL) &&
+	    memcmp(ctrl->info.entity, uvc_ctrl_class_guid, 16)) {
 		struct v4l2_event ev;
 		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
 		s32 val = 0;
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCH 10/10] media: uvcvideo: Populate only active control classes

[Ricardo Ribalda]

Do not create Control Classes for empty classes.

Fixes v4l2-compliance:

Control ioctls (Input 0):
	                fail: v4l2-test-controls.cpp(255): no controls in class 009d0000
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   | 11 +++++++++++
 drivers/media/usb/uvc/uvc_driver.c |  1 -
 drivers/media/usb/uvc/uvcvideo.h   |  1 -
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 433342efc63f..5efbb3b5aa5b 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2128,6 +2128,17 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev,
 	if (map->set == NULL)
 		map->set = uvc_set_le_value;
 
+	switch (V4L2_CTRL_ID2WHICH(map->id)) {
+	case V4L2_CTRL_ID2WHICH(V4L2_CID_CAMERA_CLASS):
+		dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
+						BIT(UVC_CC_CAMERA_CLASS);
+		break;
+	case V4L2_CTRL_ID2WHICH(V4L2_CID_USER_CLASS):
+		dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
+						BIT(UVC_CC_USER_CLASS);
+		break;
+	}
+
 	list_add_tail(&map->list, &ctrl->info.mappings);
 	uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
 		map->name, ctrl->info.entity, ctrl->info.selector);
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 996e8bd06ac5..4f368ab3a1f1 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1501,7 +1501,6 @@ static int uvc_ctrl_class_parse(struct uvc_device *dev)
 
 	unit->ctrl_class.bControlSize = 1;
 	unit->ctrl_class.bmControls = (u8 *)unit + sizeof(*unit);
-	unit->ctrl_class.bmControls[0] = (1 << (UVC_CC_LAST_CLASS + 1)) - 1;
 	unit->get_info = uvc_ctrl_class_get_info;
 	strncpy(unit->name, "Control Class", sizeof(unit->name) - 1);
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 1d59ac10c2eb..cc573d63e459 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -186,7 +186,6 @@
  */
 #define UVC_CC_CAMERA_CLASS	0
 #define UVC_CC_USER_CLASS	1
-#define UVC_CC_LAST_CLASS	UVC_CC_USER_CLASS
 
 /* ------------------------------------------------------------------------
  * Driver specific constants.
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH 10/10] media: uvcvideo: Populate only active control classes

[Hans Verkuil]

On 11/03/2021 13:20, Ricardo Ribalda wrote:
> Do not create Control Classes for empty classes.

Shouldn't this be squashed with patch 06/10?

Regards,

	Hans

> 
> Fixes v4l2-compliance:
> 
> Control ioctls (Input 0):
> 	                fail: v4l2-test-controls.cpp(255): no controls in class 009d0000
> 	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   | 11 +++++++++++
>  drivers/media/usb/uvc/uvc_driver.c |  1 -
>  drivers/media/usb/uvc/uvcvideo.h   |  1 -
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 433342efc63f..5efbb3b5aa5b 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2128,6 +2128,17 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev,
>  	if (map->set == NULL)
>  		map->set = uvc_set_le_value;
>  
> +	switch (V4L2_CTRL_ID2WHICH(map->id)) {
> +	case V4L2_CTRL_ID2WHICH(V4L2_CID_CAMERA_CLASS):
> +		dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
> +						BIT(UVC_CC_CAMERA_CLASS);
> +		break;
> +	case V4L2_CTRL_ID2WHICH(V4L2_CID_USER_CLASS):
> +		dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
> +						BIT(UVC_CC_USER_CLASS);
> +		break;
> +	}
> +
>  	list_add_tail(&map->list, &ctrl->info.mappings);
>  	uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
>  		map->name, ctrl->info.entity, ctrl->info.selector);
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 996e8bd06ac5..4f368ab3a1f1 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1501,7 +1501,6 @@ static int uvc_ctrl_class_parse(struct uvc_device *dev)
>  
>  	unit->ctrl_class.bControlSize = 1;
>  	unit->ctrl_class.bmControls = (u8 *)unit + sizeof(*unit);
> -	unit->ctrl_class.bmControls[0] = (1 << (UVC_CC_LAST_CLASS + 1)) - 1;
>  	unit->get_info = uvc_ctrl_class_get_info;
>  	strncpy(unit->name, "Control Class", sizeof(unit->name) - 1);
>  
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 1d59ac10c2eb..cc573d63e459 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -186,7 +186,6 @@
>   */
>  #define UVC_CC_CAMERA_CLASS	0
>  #define UVC_CC_USER_CLASS	1
> -#define UVC_CC_LAST_CLASS	UVC_CC_USER_CLASS
>  
>  /* ------------------------------------------------------------------------
>   * Driver specific constants.
> 

Re: [PATCH 10/10] media: uvcvideo: Populate only active control classes

[Ricardo Ribalda Delgado]

Hi Hans

Thanks for your review!

On Thu, Mar 11, 2021 at 3:32 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/03/2021 13:20, Ricardo Ribalda wrote:
> > Do not create Control Classes for empty classes.
>
> Shouldn't this be squashed with patch 06/10?

Most of the cameras I have used have the two classes, So  I am not
sure if squash with 6/10, or remove it. I separated it to feel what
Laurent has to say :)

>
> Regards,
>
>         Hans
>
> >
> > Fixes v4l2-compliance:
> >
> > Control ioctls (Input 0):
> >                       fail: v4l2-test-controls.cpp(255): no controls in class 009d0000
> >       test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 11 +++++++++++
> >  drivers/media/usb/uvc/uvc_driver.c |  1 -
> >  drivers/media/usb/uvc/uvcvideo.h   |  1 -
> >  3 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 433342efc63f..5efbb3b5aa5b 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -2128,6 +2128,17 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev,
> >       if (map->set == NULL)
> >               map->set = uvc_set_le_value;
> >
> > +     switch (V4L2_CTRL_ID2WHICH(map->id)) {
> > +     case V4L2_CTRL_ID2WHICH(V4L2_CID_CAMERA_CLASS):
> > +             dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
> > +                                             BIT(UVC_CC_CAMERA_CLASS);
> > +             break;
> > +     case V4L2_CTRL_ID2WHICH(V4L2_CID_USER_CLASS):
> > +             dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
> > +                                             BIT(UVC_CC_USER_CLASS);
> > +             break;
> > +     }
> > +
> >       list_add_tail(&map->list, &ctrl->info.mappings);
> >       uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
> >               map->name, ctrl->info.entity, ctrl->info.selector);
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 996e8bd06ac5..4f368ab3a1f1 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1501,7 +1501,6 @@ static int uvc_ctrl_class_parse(struct uvc_device *dev)
> >
> >       unit->ctrl_class.bControlSize = 1;
> >       unit->ctrl_class.bmControls = (u8 *)unit + sizeof(*unit);
> > -     unit->ctrl_class.bmControls[0] = (1 << (UVC_CC_LAST_CLASS + 1)) - 1;
> >       unit->get_info = uvc_ctrl_class_get_info;
> >       strncpy(unit->name, "Control Class", sizeof(unit->name) - 1);
> >
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 1d59ac10c2eb..cc573d63e459 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -186,7 +186,6 @@
> >   */
> >  #define UVC_CC_CAMERA_CLASS  0
> >  #define UVC_CC_USER_CLASS    1
> > -#define UVC_CC_LAST_CLASS    UVC_CC_USER_CLASS
> >
> >  /* ------------------------------------------------------------------------
> >   * Driver specific constants.
> >
>


-- 
Ricardo Ribalda

Re: [PATCH 10/10] media: uvcvideo: Populate only active control classes

[Laurent Pinchart]

Hi Ricardo,

On Thu, Mar 11, 2021 at 04:21:38PM +0100, Ricardo Ribalda Delgado wrote:
> On Thu, Mar 11, 2021 at 3:32 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > On 11/03/2021 13:20, Ricardo Ribalda wrote:
> > > Do not create Control Classes for empty classes.
> >
> > Shouldn't this be squashed with patch 06/10?
> 
> Most of the cameras I have used have the two classes, So  I am not
> sure if squash with 6/10, or remove it. I separated it to feel what
> Laurent has to say :)

I think it makes sense to only expose the classes that are being used,
so the change is good. As it fixes a bug introduced in 06/10, I'd squash
it.

> > > Fixes v4l2-compliance:
> > >
> > > Control ioctls (Input 0):
> > >                       fail: v4l2-test-controls.cpp(255): no controls in class 009d0000
> > >       test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: FAIL
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c   | 11 +++++++++++
> > >  drivers/media/usb/uvc/uvc_driver.c |  1 -
> > >  drivers/media/usb/uvc/uvcvideo.h   |  1 -
> > >  3 files changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 433342efc63f..5efbb3b5aa5b 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -2128,6 +2128,17 @@ static int __uvc_ctrl_add_mapping(struct uvc_device *dev,
> > >       if (map->set == NULL)
> > >               map->set = uvc_set_le_value;
> > >
> > > +     switch (V4L2_CTRL_ID2WHICH(map->id)) {
> > > +     case V4L2_CTRL_ID2WHICH(V4L2_CID_CAMERA_CLASS):
> > > +             dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
> > > +                                             BIT(UVC_CC_CAMERA_CLASS);
> > > +             break;
> > > +     case V4L2_CTRL_ID2WHICH(V4L2_CID_USER_CLASS):
> > > +             dev->ctrl_class_unit->ctrl_class.bmControls[0] |=
> > > +                                             BIT(UVC_CC_USER_CLASS);
> > > +             break;
> > > +     }
> > > +
> > >       list_add_tail(&map->list, &ctrl->info.mappings);
> > >       uvc_dbg(dev, CONTROL, "Adding mapping '%s' to control %pUl/%u\n",
> > >               map->name, ctrl->info.entity, ctrl->info.selector);
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 996e8bd06ac5..4f368ab3a1f1 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1501,7 +1501,6 @@ static int uvc_ctrl_class_parse(struct uvc_device *dev)
> > >
> > >       unit->ctrl_class.bControlSize = 1;
> > >       unit->ctrl_class.bmControls = (u8 *)unit + sizeof(*unit);
> > > -     unit->ctrl_class.bmControls[0] = (1 << (UVC_CC_LAST_CLASS + 1)) - 1;
> > >       unit->get_info = uvc_ctrl_class_get_info;
> > >       strncpy(unit->name, "Control Class", sizeof(unit->name) - 1);
> > >
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index 1d59ac10c2eb..cc573d63e459 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -186,7 +186,6 @@
> > >   */
> > >  #define UVC_CC_CAMERA_CLASS  0
> > >  #define UVC_CC_USER_CLASS    1
> > > -#define UVC_CC_LAST_CLASS    UVC_CC_USER_CLASS
> > >
> > >  /* ------------------------------------------------------------------------
> > >   * Driver specific constants.

-- 
Regards,

Laurent Pinchart