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@xxxxxxxxxxxx>
> ---
> 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
