diff mbox series

[v15,18/19] media: uvcvideo: implement UVC v1.5 ROI

Message ID 20241114-uvc-roi-v15-18-64cfeb56b6f8@chromium.org (mailing list archive)
State New
Headers show
Series media: uvcvideo: Implement UVC v1.5 ROI | expand

Commit Message

Ricardo Ribalda Nov. 14, 2024, 7:10 p.m. UTC 
From: Yunke Cao <yunkec@google.com>

Implement support for ROI as described in UVC 1.5:
4.2.2.1.20 Digital Region of Interest (ROI) Control

ROI control is implemented using V4L2 control API as
two UVC-specific controls:
V4L2_CID_UVC_REGION_OF_INTEREST_RECT and
V4L2_CID_UVC_REGION_OF_INTEREST_AUTO.

Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Yunke Cao <yunkec@google.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c   | 81 ++++++++++++++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  7 ++++
 include/uapi/linux/usb/video.h     |  1 +
 include/uapi/linux/uvcvideo.h      | 13 ++++++
 include/uapi/linux/v4l2-controls.h |  9 +++++
 5 files changed, 111 insertions(+)

Comments

Gergo Koteles Nov. 14, 2024, 7:53 p.m. UTC | #1 
Hi Ricardo,

On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote:
> 
> +	},
> +	{
> +		.id		= V4L2_CID_UVC_REGION_OF_INTEREST_AUTO,
> +		.entity		= UVC_GUID_UVC_CAMERA,
> +		.selector	= UVC_CT_REGION_OF_INTEREST_CONTROL,
> +		.size		= 16,
> +		.offset		= 64,
> +		.v4l2_type	= V4L2_CTRL_TYPE_BITMASK,
> +		.data_type	= UVC_CTRL_DATA_TYPE_BITMASK,
> +		.name		= "Region Of Interest Auto Controls",
> +	},
>  };
>  

Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this?

Thanks,
Gergo
Ricardo Ribalda Nov. 14, 2024, 8:03 p.m. UTC | #2 
Hi Gergo

Sorry, I forgot to reply to your comment in v14.

On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote:
>
> Hi Ricardo,
>
> On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote:
> >
> > +     },
> > +     {
> > +             .id             = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO,
> > +             .entity         = UVC_GUID_UVC_CAMERA,
> > +             .selector       = UVC_CT_REGION_OF_INTEREST_CONTROL,
> > +             .size           = 16,
> > +             .offset         = 64,
> > +             .v4l2_type      = V4L2_CTRL_TYPE_BITMASK,
> > +             .data_type      = UVC_CTRL_DATA_TYPE_BITMASK,
> > +             .name           = "Region Of Interest Auto Controls",
> > +     },
> >  };
> >
>
> Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this?

If I create 8 Booleans, they will always be shown in the device. And
the user will not have a way to know which values are available and
which are not.

We will also fail the v4l2-compliance test, because there will be up
to 7 boolean controls that will not be able to be set to 1, eventhough
they are writable.

Thanks for the prompt review.
>
> Thanks,
> Gergo
>



--
Ricardo Ribalda
Gergo Koteles Nov. 14, 2024, 8:16 p.m. UTC | #3 
Hi Ricardo,

On Thu, 2024-11-14 at 21:03 +0100, Ricardo Ribalda wrote:
> Hi Gergo
> 
> Sorry, I forgot to reply to your comment in v14.
> 
> On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote:
> > 
> > Hi Ricardo,
> > 
> > On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote:
> > > 
> > > +     },
> > > +     {
> > > +             .id             = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO,
> > > +             .entity         = UVC_GUID_UVC_CAMERA,
> > > +             .selector       = UVC_CT_REGION_OF_INTEREST_CONTROL,
> > > +             .size           = 16,
> > > +             .offset         = 64,
> > > +             .v4l2_type      = V4L2_CTRL_TYPE_BITMASK,
> > > +             .data_type      = UVC_CTRL_DATA_TYPE_BITMASK,
> > > +             .name           = "Region Of Interest Auto Controls",
> > > +     },
> > >  };
> > > 
> > 
> > Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this?
> 
> If I create 8 Booleans, they will always be shown in the device. And
> the user will not have a way to know which values are available and
> which are not.
> 
> We will also fail the v4l2-compliance test, because there will be up
> to 7 boolean controls that will not be able to be set to 1, eventhough
> they are writable.
> 

And can't it be that only those returned by GET_MAX be added?

```
The bmAutoControls bitmask determines which, if any, on board features
should track to the region of interest. To detect if a device supports
a particular Auto Control, use GET_MAX which returns a mask indicating
all supported Auto Controls.
```

Sorry for the misunderstanding, I just don't quite understand.

Thanks,

Gergo
Ricardo Ribalda Nov. 14, 2024, 8:28 p.m. UTC | #4 
Hi

On Thu, 14 Nov 2024 at 21:16, Gergo Koteles <soyer@irl.hu> wrote:
>
> Hi Ricardo,
>
> On Thu, 2024-11-14 at 21:03 +0100, Ricardo Ribalda wrote:
> > Hi Gergo
> >
> > Sorry, I forgot to reply to your comment in v14.
> >
> > On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote:
> > > >
> > > > +     },
> > > > +     {
> > > > +             .id             = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO,
> > > > +             .entity         = UVC_GUID_UVC_CAMERA,
> > > > +             .selector       = UVC_CT_REGION_OF_INTEREST_CONTROL,
> > > > +             .size           = 16,
> > > > +             .offset         = 64,
> > > > +             .v4l2_type      = V4L2_CTRL_TYPE_BITMASK,
> > > > +             .data_type      = UVC_CTRL_DATA_TYPE_BITMASK,
> > > > +             .name           = "Region Of Interest Auto Controls",
> > > > +     },
> > > >  };
> > > >
> > >
> > > Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this?
> >
> > If I create 8 Booleans, they will always be shown in the device. And
> > the user will not have a way to know which values are available and
> > which are not.
> >
> > We will also fail the v4l2-compliance test, because there will be up
> > to 7 boolean controls that will not be able to be set to 1, eventhough
> > they are writable.
> >
>
> And can't it be that only those returned by GET_MAX be added?
>
> ```
> The bmAutoControls bitmask determines which, if any, on board features
> should track to the region of interest. To detect if a device supports
> a particular Auto Control, use GET_MAX which returns a mask indicating
> all supported Auto Controls.
> ```
>
> Sorry for the misunderstanding, I just don't quite understand.

I guess we could, but we would have to make a big change in the way
the controls are probed today. uvc does not use the control framework.

What will be the benefit of using 8 controls?
- Applications still have to know what those controls do, they should
not rely on the control name.
- Changing from lets say AUTO_EXPOSURE to AUTO_FOCUS, will require to
send at least 2 controls via v4l2_s_ext_control... I think it is more
practical and less prone to errrors to send just one control

If the number of autos were unknown, then having multiple controls
could be a good idea, but they are part of the uvc standard and
unlikely to be changed.


Thanks!
>
> Thanks,
>
> Gergo
Gergo Koteles Nov. 15, 2024, 12:04 a.m. UTC | #5 
Hi Ricardo,

On Thu, 2024-11-14 at 21:28 +0100, Ricardo Ribalda wrote:
> Hi
> 
> On Thu, 14 Nov 2024 at 21:16, Gergo Koteles <soyer@irl.hu> wrote:
> > 
> > Hi Ricardo,
> > 
> > On Thu, 2024-11-14 at 21:03 +0100, Ricardo Ribalda wrote:
> > > Hi Gergo
> > > 
> > > Sorry, I forgot to reply to your comment in v14.
> > > 
> > > On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote:
> > > > 
> > > > Hi Ricardo,
> > > > 
> > > > On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote:
> > > > > 
> > > > > +     },
> > > > > +     {
> > > > > +             .id             = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO,
> > > > > +             .entity         = UVC_GUID_UVC_CAMERA,
> > > > > +             .selector       = UVC_CT_REGION_OF_INTEREST_CONTROL,
> > > > > +             .size           = 16,
> > > > > +             .offset         = 64,
> > > > > +             .v4l2_type      = V4L2_CTRL_TYPE_BITMASK,
> > > > > +             .data_type      = UVC_CTRL_DATA_TYPE_BITMASK,
> > > > > +             .name           = "Region Of Interest Auto Controls",
> > > > > +     },
> > > > >  };
> > > > > 
> > > > 
> > > > Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this?
> > > 
> > > If I create 8 Booleans, they will always be shown in the device. And
> > > the user will not have a way to know which values are available and
> > > which are not.
> > > 
> > > We will also fail the v4l2-compliance test, because there will be up
> > > to 7 boolean controls that will not be able to be set to 1, eventhough
> > > they are writable.
> > > 
> > 
> > And can't it be that only those returned by GET_MAX be added?
> > 
> > ```
> > The bmAutoControls bitmask determines which, if any, on board features
> > should track to the region of interest. To detect if a device supports
> > a particular Auto Control, use GET_MAX which returns a mask indicating
> > all supported Auto Controls.
> > ```
> > 
> > Sorry for the misunderstanding, I just don't quite understand.
> 
> I guess we could, but we would have to make a big change in the way
> the controls are probed today. uvc does not use the control framework.
> 
> What will be the benefit of using 8 controls?
> - Applications still have to know what those controls do, they should
> not rely on the control name.

Applications like v4l2-ctl are not aware of every controls, work by
control type, and let the user decide what to do, based on the name.

To avoid having to know each bitmask type control, they need to be able
to query which bit means what and what to display to the user.

Could VIDIOC_QUERYMENU be supplemented with this?


> - Changing from lets say AUTO_EXPOSURE to AUTO_FOCUS, will require to
> send at least 2 controls via v4l2_s_ext_control... I think it is more
> practical and less prone to errrors to send just one control
> 

Yes, that could be a good reason.

Thanks,
Gergo
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index f262e05ad3a8..5b619ef40dd3 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -358,6 +358,24 @@  static const struct uvc_control_info uvc_ctrls[] = {
 		.flags		= UVC_CTRL_FLAG_GET_CUR
 				| UVC_CTRL_FLAG_AUTO_UPDATE,
 	},
+	/*
+	 * UVC_CTRL_FLAG_AUTO_UPDATE is needed because the RoI may get updated
+	 * by sensors.
+	 * "This RoI should be the same as specified in most recent SET_CUR
+	 * except in the case where the ‘Auto Detect and Track’ and/or
+	 * ‘Image Stabilization’ bit have been set."
+	 * 4.2.2.1.20 Digital Region of Interest (ROI) Control
+	 */
+	{
+		.entity		= UVC_GUID_UVC_CAMERA,
+		.selector	= UVC_CT_REGION_OF_INTEREST_CONTROL,
+		.index		= 21,
+		.size		= 10,
+		.flags		= UVC_CTRL_FLAG_SET_CUR | UVC_CTRL_FLAG_GET_CUR
+				| UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX
+				| UVC_CTRL_FLAG_GET_DEF
+				| UVC_CTRL_FLAG_AUTO_UPDATE,
+	},
 };
 
 static const u32 uvc_control_classes[] = {
@@ -603,6 +621,44 @@  static const struct uvc_control_mapping *uvc_ctrl_filter_plf_mapping(
 	return out_mapping;
 }
 
+static int uvc_get_rect(struct uvc_control_mapping *mapping, u8 query,
+			const void *uvc_in, size_t v4l2_size, void *v4l2_out)
+{
+	const struct uvc_rect *uvc_rect = uvc_in;
+	struct v4l2_rect *v4l2_rect = v4l2_out;
+
+	if (WARN_ON(v4l2_size != sizeof(struct v4l2_rect)))
+		return -EINVAL;
+
+	if (uvc_rect->left > uvc_rect->right ||
+	    uvc_rect->top > uvc_rect->bottom)
+		return -EIO;
+
+	v4l2_rect->top = uvc_rect->top;
+	v4l2_rect->left = uvc_rect->left;
+	v4l2_rect->height = uvc_rect->bottom - uvc_rect->top + 1;
+	v4l2_rect->width = uvc_rect->right - uvc_rect->left + 1;
+
+	return 0;
+}
+
+static int uvc_set_rect(struct uvc_control_mapping *mapping, size_t v4l2_size,
+			const void *v4l2_in, void *uvc_out)
+{
+	struct uvc_rect *uvc_rect = uvc_out;
+	const struct v4l2_rect *v4l2_rect = v4l2_in;
+
+	if (WARN_ON(v4l2_size != sizeof(struct v4l2_rect)))
+		return -EINVAL;
+
+	uvc_rect->top = max(0xffff, v4l2_rect->top);
+	uvc_rect->left = max(0xffff, v4l2_rect->left);
+	uvc_rect->bottom = max(0xffff, v4l2_rect->height + v4l2_rect->top - 1);
+	uvc_rect->right = max(0xffff, v4l2_rect->width + v4l2_rect->left - 1);
+
+	return 0;
+}
+
 static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 	{
 		.id		= V4L2_CID_BRIGHTNESS,
@@ -897,6 +953,28 @@  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
 		.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
 		.filter_mapping	= uvc_ctrl_filter_plf_mapping,
 	},
+	{
+		.id		= V4L2_CID_UVC_REGION_OF_INTEREST_RECT,
+		.entity		= UVC_GUID_UVC_CAMERA,
+		.selector	= UVC_CT_REGION_OF_INTEREST_CONTROL,
+		.size		= sizeof(struct uvc_rect) * 8,
+		.offset		= 0,
+		.v4l2_type	= V4L2_CTRL_TYPE_RECT,
+		.data_type	= UVC_CTRL_DATA_TYPE_RECT,
+		.get		= uvc_get_rect,
+		.set		= uvc_set_rect,
+		.name		= "Region Of Interest Rectangle",
+	},
+	{
+		.id		= V4L2_CID_UVC_REGION_OF_INTEREST_AUTO,
+		.entity		= UVC_GUID_UVC_CAMERA,
+		.selector	= UVC_CT_REGION_OF_INTEREST_CONTROL,
+		.size		= 16,
+		.offset		= 64,
+		.v4l2_type	= V4L2_CTRL_TYPE_BITMASK,
+		.data_type	= UVC_CTRL_DATA_TYPE_BITMASK,
+		.name		= "Region Of Interest Auto Controls",
+	},
 };
 
 /* ------------------------------------------------------------------------
@@ -1465,6 +1543,9 @@  static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain,
 
 static size_t uvc_mapping_v4l2_size(struct uvc_control_mapping *mapping)
 {
+	if (mapping->v4l2_type == V4L2_CTRL_TYPE_RECT)
+		return sizeof(struct v4l2_rect);
+
 	if (uvc_ctrl_mapping_is_compound(mapping))
 		return DIV_ROUND_UP(mapping->size, 8);
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 8aca1a2fe587..d910a5e5b514 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -294,6 +294,13 @@  struct uvc_streaming_header {
 	u8 bTriggerUsage;
 };
 
+struct uvc_rect {
+	u16 top;
+	u16 left;
+	u16 bottom;
+	u16 right;
+} __packed;
+
 enum uvc_buffer_state {
 	UVC_BUF_STATE_IDLE	= 0,
 	UVC_BUF_STATE_QUEUED	= 1,
diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
index 2ff0e8a3a683..2afb4420e6c4 100644
--- a/include/uapi/linux/usb/video.h
+++ b/include/uapi/linux/usb/video.h
@@ -104,6 +104,7 @@ 
 #define UVC_CT_ROLL_ABSOLUTE_CONTROL			0x0f
 #define UVC_CT_ROLL_RELATIVE_CONTROL			0x10
 #define UVC_CT_PRIVACY_CONTROL				0x11
+#define UVC_CT_REGION_OF_INTEREST_CONTROL		0x14
 
 /* A.9.5. Processing Unit Control Selectors */
 #define UVC_PU_CONTROL_UNDEFINED			0x00
diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index f86185456dc5..cbe15bca9569 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -16,6 +16,7 @@ 
 #define UVC_CTRL_DATA_TYPE_BOOLEAN	3
 #define UVC_CTRL_DATA_TYPE_ENUM		4
 #define UVC_CTRL_DATA_TYPE_BITMASK	5
+#define UVC_CTRL_DATA_TYPE_RECT		6
 
 /* Control flags */
 #define UVC_CTRL_FLAG_SET_CUR		(1 << 0)
@@ -38,6 +39,18 @@ 
 
 #define UVC_MENU_NAME_LEN 32
 
+/* V4L2 driver-specific controls */
+#define V4L2_CID_UVC_REGION_OF_INTEREST_RECT	(V4L2_CID_USER_UVC_BASE + 1)
+#define V4L2_CID_UVC_REGION_OF_INTEREST_AUTO	(V4L2_CID_USER_UVC_BASE + 2)
+#define V4L2_UVC_REGION_OF_INTEREST_AUTO_EXPOSURE		(1 << 0)
+#define V4L2_UVC_REGION_OF_INTEREST_AUTO_IRIS			(1 << 1)
+#define V4L2_UVC_REGION_OF_INTEREST_AUTO_WHITE_BALANCE		(1 << 2)
+#define V4L2_UVC_REGION_OF_INTEREST_AUTO_FOCUS			(1 << 3)
+#define V4L2_UVC_REGION_OF_INTEREST_AUTO_FACE_DETECT		(1 << 4)
+#define V4L2_UVC_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK	(1 << 5)
+#define V4L2_UVC_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION	(1 << 6)
+#define V4L2_UVC_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY		(1 << 7)
+
 struct uvc_menu_info {
 	__u32 value;
 	__u8 name[UVC_MENU_NAME_LEN];
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 974fd254e573..6c91d6fa4708 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -215,6 +215,13 @@  enum v4l2_colorfx {
  */
 #define V4L2_CID_USER_THP7312_BASE		(V4L2_CID_USER_BASE + 0x11c0)
 
+/*
+ * The base for the uvc driver controls.
+ * See linux/uvcvideo.h for the list of controls.
+ * We reserve 64 controls for this driver.
+ */
+#define V4L2_CID_USER_UVC_BASE			(V4L2_CID_USER_BASE + 0x11e0)
+
 /* MPEG-class control IDs */
 /* The MPEG controls are applicable to all codec controls
  * and the 'MPEG' part of the define is historical */
@@ -1089,6 +1096,8 @@  enum v4l2_auto_focus_range {
 
 #define V4L2_CID_HDR_SENSOR_MODE		(V4L2_CID_CAMERA_CLASS_BASE+36)
 
+/* CAMERA-class private control IDs */
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)