[PATCHv3 0/6] media: uvcvideo: implement UVC 1.5 ROI

[PATCHv3 0/6] media: uvcvideo: implement UVC 1.5 ROI

[Sergey Senozhatsky]

Hello,

	This patch set implements UVC 1.5 ROI using v4l2_selection API.

V3:
- reimplemented ROI. We dont' use split controls anymore.
- Ricardo's feedback

Sergey Senozhatsky (6):
  media: v4l UAPI: add ROI selection targets
  media: v4l UAPI: document ROI selection targets
  media: v4l UAPI: add ROI auto-controls flags
  media: v4l UAPI: document ROI auto-controls flags
  media: uvcvideo: add UVC 1.5 ROI control
  MAINTAINERS: update Senozhatsky email address

 .../media/v4l/selection-api-configuration.rst |  22 +++
 .../media/v4l/selection-api-examples.rst      |  28 ++++
 .../media/v4l/v4l2-selection-flags.rst        |  40 +++++
 .../media/v4l/v4l2-selection-targets.rst      |  24 +++
 MAINTAINERS                                   |   8 +-
 drivers/media/usb/uvc/uvc_v4l2.c              | 147 +++++++++++++++++-
 include/uapi/linux/usb/video.h                |   1 +
 include/uapi/linux/v4l2-common.h              |  18 +++
 8 files changed, 281 insertions(+), 7 deletions(-)

-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCHv3 1/6] media: v4l UAPI: add ROI selection targets

[Sergey Senozhatsky]

UVC 1.5 requires Region Of Interest control to implement
GET_CUR, GET_DEF, GET_MIN and GET_MAX requests. This patch
adds new V4L2 selection API targets that will implement
those ROI requests.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 include/uapi/linux/v4l2-common.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
index 7d21c1634b4d..3651ebb8cb23 100644
--- a/include/uapi/linux/v4l2-common.h
+++ b/include/uapi/linux/v4l2-common.h
@@ -78,6 +78,14 @@
 #define V4L2_SEL_TGT_COMPOSE_BOUNDS	0x0102
 /* Current composing area plus all padding pixels */
 #define V4L2_SEL_TGT_COMPOSE_PADDED	0x0103
+/* Current Region of Interest area */
+#define V4L2_SEL_TGT_ROI		0x0200
+/* Default Region of Interest area */
+#define V4L2_SEL_TGT_ROI_DEFAULT	0x0201
+/* Region of Interest minimum values */
+#define V4L2_SEL_TGT_ROI_BOUNDS_MIN	0x0202
+/* Region of Interest maximum values */
+#define V4L2_SEL_TGT_ROI_BOUNDS_MAX	0x0203
 
 /* Selection flags */
 #define V4L2_SEL_FLAG_GE		(1 << 0)
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCHv3 2/6] media: v4l UAPI: document ROI selection targets

[Sergey Senozhatsky]

Document V4L2 selection targets that will be used to ROI
implementation.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../media/v4l/selection-api-configuration.rst | 22 +++++++++++++++
 .../media/v4l/selection-api-examples.rst      | 28 +++++++++++++++++++
 .../media/v4l/v4l2-selection-targets.rst      | 24 ++++++++++++++++
 3 files changed, 74 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
index fee49bf1a1c0..b5fdd765e2db 100644
--- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
+++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
@@ -135,3 +135,25 @@ and the height of rectangles obtained using ``V4L2_SEL_TGT_CROP`` and
 ``V4L2_SEL_TGT_COMPOSE`` targets. If these are not equal then the
 scaling is applied. The application can compute the scaling ratios using
 these values.
+
+Configuration of Region of Interest (ROI)
+=========================================
+
+The range of auto-controls values and of coordinates of the top left
+corner, width and height of areas that can be ROI is given by the
+``V4L2_SEL_TGT_ROI_BOUNDS_MIN`` and ``V4L2_SEL_TGT_ROI_BOUNDS_MAX``
+targets. It is recommended for the driver developers to put the top/left
+corner at position ``(0,0)``.
+
+The top left corner, width and height of the Region of Interest area
+and auto-controls currently being employed by the device are given by
+the ``V4L2_SEL_TGT_ROI`` target. It uses the same coordinate system
+as ``V4L2_SEL_TGT_ROI_BOUNDS_MIN`` and ``V4L2_SEL_TGT_ROI_BOUNDS_MAX``.
+
+In order to change active ROI top left, width and height coordinates
+and ROI auto-controls use ``V4L2_SEL_TGT_ROI`` target.
+
+Each capture device has a default ROI rectangle and auto-controls
+value given by the ``V4L2_SEL_TGT_ROI_DEFAULT`` target. Drivers shall
+set the ROI rectangle to the default when the driver is first loaded,
+but not later.
diff --git a/Documentation/userspace-api/media/v4l/selection-api-examples.rst b/Documentation/userspace-api/media/v4l/selection-api-examples.rst
index 5f8e8a1f59d7..ad2664888700 100644
--- a/Documentation/userspace-api/media/v4l/selection-api-examples.rst
+++ b/Documentation/userspace-api/media/v4l/selection-api-examples.rst
@@ -82,3 +82,31 @@ Example: Querying for scaling factors
 	/* computing scaling factors */
 	hscale = (double)compose.r.width / crop.r.width;
 	vscale = (double)compose.r.height / crop.r.height;
+
+Setting Region Of Interest area to half of the default value
+
+Example: Simple ROI
+===========================
+
+.. code-block:: c
+
+	struct v4l2_selection roi = {
+	    .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+	    .target = V4L2_SEL_TGT_ROI_DEFAULT,
+	};
+	struct v4l2_rect r;
+
+	ret = ioctl(fd, VIDIOC_G_SELECTION, &roi);
+	if (ret)
+	    exit(-1);
+	/* setting smaller ROI rectangle */
+	r.width = roi.r.width / 2;
+	r.height = roi.r.height / 2;
+	r.left = roi.r.width / 4;
+	r.top = roi.r.height / 4;
+	roi.r = r;
+	roi.target = V4L2_SEL_TGT_ROI;
+	roi.flags = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;
+	ret = ioctl(fd, VIDIOC_S_SELECTION, &roi);
+	if (ret)
+	    exit(-1);
diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
index b46bae984f35..d1dc9c50eb05 100644
--- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
+++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
@@ -75,6 +75,30 @@ of the two interfaces they are used.
 	modified by hardware.
       - Yes
       - No
+    * - ``V4L2_SEL_TGT_ROI``
+      - 0x0200
+      - Current Region of Interest rectangle and auto-controls value.
+      - Yes
+      - No
+    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
+      - 0x0201
+      - Suggested Region of Interest rectangle and auto-controls value.
+      - Yes
+      - No
+    * - ``V4L2_SEL_TGT_ROI_BOUNDS_MIN``
+      - 0x0202
+      - Minimum bounds of the Region of Interest rectangle and minimum
+	auto-controls value. All valid ROI rectangles and auto-controls
+	should be within minimum-maximum range.
+      - Yes
+      - No
+    * - ``V4L2_SEL_TGT_ROI_BOUNDS_MAX``
+      - 0x0203
+      - Maximum bounds of the Region of Interest rectangle and maximum
+	auto-controls value. All valid ROI rectangles and auto-controls
+	should be within minimum-maximum range.
+      - Yes
+      - No
 
 .. raw:: latex
 
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCHv3 3/6] media: v4l UAPI: add ROI auto-controls flags

[Sergey Senozhatsky]

UVC 1.5 defines the following Region Of Interest auto controls:

D0: Auto Exposure
D1: Auto Iris
D2: Auto White Balance
D3: Auto Focus
D4: Auto Face Detect
D5: Auto Detect and Track
D6: Image Stabilization
D7: Higher Quality
D8 – D15: Reserved, set to zero

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 include/uapi/linux/v4l2-common.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
index 3651ebb8cb23..34f1c262d6aa 100644
--- a/include/uapi/linux/v4l2-common.h
+++ b/include/uapi/linux/v4l2-common.h
@@ -92,6 +92,16 @@
 #define V4L2_SEL_FLAG_LE		(1 << 1)
 #define V4L2_SEL_FLAG_KEEP_CONFIG	(1 << 2)
 
+/* ROI auto-controls flags */
+#define V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE		(1 << 0)
+#define V4L2_SEL_FLAG_ROI_AUTO_IRIS			(1 << 1)
+#define V4L2_SEL_FLAG_ROI_AUTO_WHITE_BALANCE		(1 << 2)
+#define V4L2_SEL_FLAG_ROI_AUTO_FOCUS			(1 << 3)
+#define V4L2_SEL_FLAG_ROI_AUTO_FACE_DETECT		(1 << 4)
+#define V4L2_SEL_FLAG_ROI_AUTO_DETECT_AND_TRACK	(1 << 5)
+#define V4L2_SEL_FLAG_ROI_AUTO_IMAGE_STABILIXATION	(1 << 6)
+#define V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY		(1 << 7)
+
 struct v4l2_edid {
 	__u32 pad;
 	__u32 start_block;
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCHv3 4/6] media: v4l UAPI: document ROI auto-controls flags

[Sergey Senozhatsky]

Document ROI auto controls.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../media/v4l/v4l2-selection-flags.rst        | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-flags.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-flags.rst
index 1cb1531c1e52..536d29a6c4a5 100644
--- a/Documentation/userspace-api/media/v4l/v4l2-selection-flags.rst
+++ b/Documentation/userspace-api/media/v4l/v4l2-selection-flags.rst
@@ -48,6 +48,46 @@ Selection flags
 	inside the subdevice to all further processing steps.
       - No
       - Yes
+    * - ``V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE``
+      - (1 << 0)
+      - Auto Exposure.
+      - Yes
+      - No
+    * - ``V4L2_SEL_FLAG_ROI_AUTO_IRIS``
+      - (1 << 1)
+      - Auto Iris.
+      - Yes
+      - No
+    * - ``V4L2_SEL_FLAG_ROI_AUTO_WHITE_BALANCE``
+      - (1 << 2)
+      - Auto White Balance.
+      - Yes
+      - No
+    * - ``V4L2_SEL_FLAG_ROI_AUTO_FOCUS``
+      - (1 << 3)
+      - Auto Focus.
+      - Yes
+      - No
+    * - ``V4L2_SEL_FLAG_ROI_AUTO_FACE_DETECT``
+      - (1 << 4)
+      - Auto Face Detect.
+      - Yes
+      - No
+    * - ``V4L2_SEL_FLAG_ROI_AUTO_DETECT_AND_TRACK``
+      - (1 << 5)
+      - Auto Detect and Track.
+      - Yes
+      - No
+    * - ``V4L2_SEL_FLAG_ROI_AUTO_IMAGE_STABILIXATION``
+      - (1 << 6)
+      - Image Stabilization.
+      - Yes
+      - No
+    * - ``V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY``
+      - (1 << 7)
+      - Higher Quality.
+      - Yes
+      - No
 
 .. raw:: latex
 
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

This patch implements UVC 1.5 Region of Interest (ROI) control.

Note that, UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL controls
and mentions that ROI rectangle coordinates "must be within
the current Digital Window as specified by the CT_WINDOW control."
(4.2.2.1.20 Digital Region of Interest (ROI) Control).

It's is not entirely clear if we need to implement WINDOW_CONTROL.
ROI is naturally limited by GET_MIN and GET_MAX rectangles.

Another thing to note is that ROI support is implemented as
V4L2 selection target: selection rectangle represents ROI
rectangle and selection flags represent ROI auto-controls.
User-space is required to set valid values for both rectangle
and auto-controls every time SET_CUR is issued.

Usage example:

       struct v4l2_selection roi = {0, };

       roi.target     = V4L2_SEL_TGT_ROI;
       roi.r.left     = 0;
       roi.r.top      = 0;
       roi.r.width    = 42;
       roi.r.height   = 42;
       roi.flags      = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;

       ioctl(fd, VIDIOC_S_SELECTION, &roi);

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 147 ++++++++++++++++++++++++++++++-
 include/uapi/linux/usb/video.h   |   1 +
 2 files changed, 145 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 252136cc885c..d0fe6c33fab6 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1139,14 +1139,66 @@ static int uvc_ioctl_querymenu(struct file *file, void *fh,
 	return uvc_query_v4l2_menu(chain, qm);
 }
 
-static int uvc_ioctl_g_selection(struct file *file, void *fh,
-				 struct v4l2_selection *sel)
+/* UVC 1.5 ROI rectangle is half the size of v4l2_rect */
+struct uvc_roi_rect {
+	__u16			top;
+	__u16			left;
+	__u16			bottom;
+	__u16			right;
+	__u16			auto_controls;
+} __packed;
+
+static int uvc_ioctl_g_roi_target(struct file *file, void *fh,
+				  struct v4l2_selection *sel)
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
+	struct uvc_roi_rect *roi;
+	u8 query;
+	int ret;
 
-	if (sel->type != stream->type)
+	switch (sel->target) {
+	case V4L2_SEL_TGT_ROI:
+		query = UVC_GET_CUR;
+		break;
+	case V4L2_SEL_TGT_ROI_DEFAULT:
+		query = UVC_GET_DEF;
+		break;
+	case V4L2_SEL_TGT_ROI_BOUNDS_MIN:
+		query = UVC_GET_MAX;
+		break;
+	case V4L2_SEL_TGT_ROI_BOUNDS_MAX:
+		query = UVC_GET_MAX;
+		break;
+	default:
 		return -EINVAL;
+	}
+
+	roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
+	if (!roi)
+		return -ENOMEM;
+
+	ret = uvc_query_ctrl(stream->dev, query, 1, stream->dev->intfnum,
+			     UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
+			     sizeof(struct uvc_roi_rect));
+	if (!ret) {
+		/* ROI left, top, right, bottom are global coordinates. */
+		sel->r.left	= roi->left;
+		sel->r.top	= roi->top;
+		sel->r.width	= roi->right - roi->left + 1;
+		sel->r.height	= roi->bottom - roi->top + 1;
+		sel->flags	= roi->auto_controls;
+	}
+
+	kfree(roi);
+	return ret;
+}
+
+static int uvc_ioctl_g_sel_target(struct file *file, void *fh,
+				  struct v4l2_selection *sel)
+{
+	struct uvc_fh *handle = fh;
+	struct uvc_streaming *stream = handle->stream;
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_DEFAULT:
@@ -1173,6 +1225,94 @@ static int uvc_ioctl_g_selection(struct file *file, void *fh,
 	return 0;
 }
 
+static int uvc_ioctl_g_selection(struct file *file, void *fh,
+				 struct v4l2_selection *sel)
+{
+	struct uvc_fh *handle = fh;
+	struct uvc_streaming *stream = handle->stream;
+
+	if (sel->type != stream->type)
+		return -EINVAL;
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+		return uvc_ioctl_g_sel_target(file, fh, sel);
+	case V4L2_SEL_TGT_ROI:
+	case V4L2_SEL_TGT_ROI_DEFAULT:
+	case V4L2_SEL_TGT_ROI_BOUNDS_MIN:
+	case V4L2_SEL_TGT_ROI_BOUNDS_MAX:
+		return uvc_ioctl_g_roi_target(file, fh, sel);
+	}
+
+	return -EINVAL;
+}
+
+static bool validate_roi_bounds(struct uvc_streaming *stream,
+				struct v4l2_selection *sel)
+{
+	if (sel->r.left > USHRT_MAX ||
+	    sel->r.top > USHRT_MAX ||
+	    (sel->r.width + sel->r.left) > USHRT_MAX ||
+	    (sel->r.height + sel->r.top) > USHRT_MAX ||
+	    !sel->r.width || !sel->r.height)
+		return false;
+
+	if (sel->flags > V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY)
+		return false;
+
+	return true;
+}
+
+static int uvc_ioctl_s_roi(struct file *file, void *fh,
+			   struct v4l2_selection *sel)
+{
+	struct uvc_fh *handle = fh;
+	struct uvc_streaming *stream = handle->stream;
+	struct uvc_roi_rect *roi;
+	int ret;
+
+	if (!validate_roi_bounds(stream, sel))
+		return -E2BIG;
+
+	roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
+	if (!roi)
+		return -ENOMEM;
+
+	/* ROI left, top, right, bottom are global coordinates. */
+	roi->left		= sel->r.left;
+	roi->top		= sel->r.top;
+	roi->right		= sel->r.width + sel->r.left - 1;
+	roi->bottom		= sel->r.height + sel->r.top - 1;
+	roi->auto_controls	= sel->flags;
+
+	ret = uvc_query_ctrl(stream->dev, UVC_SET_CUR, 1, stream->dev->intfnum,
+			     UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
+			     sizeof(struct uvc_roi_rect));
+
+	kfree(roi);
+	return ret;
+}
+
+static int uvc_ioctl_s_selection(struct file *file, void *fh,
+				 struct v4l2_selection *sel)
+{
+	struct uvc_fh *handle = fh;
+	struct uvc_streaming *stream = handle->stream;
+
+	if (sel->type != stream->type)
+		return -EINVAL;
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_ROI:
+		return uvc_ioctl_s_roi(file, fh, sel);
+	}
+
+	return -EINVAL;
+}
+
 static int uvc_ioctl_g_parm(struct file *file, void *fh,
 			    struct v4l2_streamparm *parm)
 {
@@ -1533,6 +1673,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
 	.vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
 	.vidioc_querymenu = uvc_ioctl_querymenu,
 	.vidioc_g_selection = uvc_ioctl_g_selection,
+	.vidioc_s_selection = uvc_ioctl_s_selection,
 	.vidioc_g_parm = uvc_ioctl_g_parm,
 	.vidioc_s_parm = uvc_ioctl_s_parm,
 	.vidioc_enum_framesizes = uvc_ioctl_enum_framesizes,
diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
index d854cb19c42c..c87624962896 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
-- 
2.31.0.rc2.261.g7f71774620-goog

[PATCHv3 6/6] MAINTAINERS: update Senozhatsky email address

[Sergey Senozhatsky]

I don't check my @gmail.com addresses often enough these days.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 MAINTAINERS | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index b2baeb5e4a68..01b000cd5774 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14433,7 +14433,7 @@ F:	kernel/sched/psi.c
 
 PRINTK
 M:	Petr Mladek <pmladek@suse.com>
-M:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
+M:	Sergey Senozhatsky <senozhatsky@chromium.org>
 R:	Steven Rostedt <rostedt@goodmis.org>
 R:	John Ogness <john.ogness@linutronix.de>
 S:	Maintained
@@ -19293,7 +19293,7 @@ F:	drivers/net/vrf.c
 VSPRINTF
 M:	Petr Mladek <pmladek@suse.com>
 M:	Steven Rostedt <rostedt@goodmis.org>
-M:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
+M:	Sergey Senozhatsky <senozhatsky@chromium.org>
 R:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
 R:	Rasmus Villemoes <linux@rasmusvillemoes.dk>
 S:	Maintained
@@ -19944,7 +19944,7 @@ F:	drivers/staging/media/zoran/
 ZRAM COMPRESSED RAM BLOCK DEVICE DRVIER
 M:	Minchan Kim <minchan@kernel.org>
 M:	Nitin Gupta <ngupta@vflare.org>
-R:	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
+R:	Sergey Senozhatsky <senozhatsky@chromium.org>
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	Documentation/admin-guide/blockdev/zram.rst
@@ -19958,7 +19958,7 @@ F:	drivers/tty/serial/zs.*
 ZSMALLOC COMPRESSED SLAB MEMORY ALLOCATOR
 M:	Minchan Kim <minchan@kernel.org>
 M:	Nitin Gupta <ngupta@vflare.org>
-R:	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
+R:	Sergey Senozhatsky <senozhatsky@chromium.org>
 L:	linux-mm@kvack.org
 S:	Maintained
 F:	Documentation/vm/zsmalloc.rst
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCHv3 6/6] MAINTAINERS: update Senozhatsky email address

[Sergey Senozhatsky]

On (21/03/19 14:53), Sergey Senozhatsky wrote:
> 
> I don't check my @gmail.com addresses often enough these days.
> 

Please ignore this one. It's a different story and does not belong
to this series.

	-ss

Re: [PATCHv3 3/6] media: v4l UAPI: add ROI auto-controls flags

[Ricardo Ribalda]

Hi Sergey

On Fri, Mar 19, 2021 at 6:53 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> UVC 1.5 defines the following Region Of Interest auto controls:
>
> D0: Auto Exposure
> D1: Auto Iris
> D2: Auto White Balance
> D3: Auto Focus
> D4: Auto Face Detect
> D5: Auto Detect and Track
> D6: Image Stabilization
> D7: Higher Quality
> D8 – D15: Reserved, set to zero
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  include/uapi/linux/v4l2-common.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> index 3651ebb8cb23..34f1c262d6aa 100644
> --- a/include/uapi/linux/v4l2-common.h
> +++ b/include/uapi/linux/v4l2-common.h
> @@ -92,6 +92,16 @@
>  #define V4L2_SEL_FLAG_LE               (1 << 1)
>  #define V4L2_SEL_FLAG_KEEP_CONFIG      (1 << 2)
>

Are you sure that you do not want to start with 1<<3, there might be
some hardware that support LE/SE
> +/* ROI auto-controls flags */
> +#define V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE                (1 << 0)
> +#define V4L2_SEL_FLAG_ROI_AUTO_IRIS                    (1 << 1)
> +#define V4L2_SEL_FLAG_ROI_AUTO_WHITE_BALANCE           (1 << 2)
> +#define V4L2_SEL_FLAG_ROI_AUTO_FOCUS                   (1 << 3)
> +#define V4L2_SEL_FLAG_ROI_AUTO_FACE_DETECT             (1 << 4)
> +#define V4L2_SEL_FLAG_ROI_AUTO_DETECT_AND_TRACK        (1 << 5)
> +#define V4L2_SEL_FLAG_ROI_AUTO_IMAGE_STABILIXATION     (1 << 6)
> +#define V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY          (1 << 7)
> +
>  struct v4l2_edid {
>         __u32 pad;
>         __u32 start_block;
> --
> 2.31.0.rc2.261.g7f71774620-goog
>


-- 
Ricardo Ribalda

Re: [PATCHv3 2/6] media: v4l UAPI: document ROI selection targets

[Ricardo Ribalda]

Hi Sergey

On Fri, Mar 19, 2021 at 6:53 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> Document V4L2 selection targets that will be used to ROI
> implementation.
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  .../media/v4l/selection-api-configuration.rst | 22 +++++++++++++++
>  .../media/v4l/selection-api-examples.rst      | 28 +++++++++++++++++++
>  .../media/v4l/v4l2-selection-targets.rst      | 24 ++++++++++++++++
>  3 files changed, 74 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
> index fee49bf1a1c0..b5fdd765e2db 100644
> --- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
> +++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
> @@ -135,3 +135,25 @@ and the height of rectangles obtained using ``V4L2_SEL_TGT_CROP`` and
>  ``V4L2_SEL_TGT_COMPOSE`` targets. If these are not equal then the
>  scaling is applied. The application can compute the scaling ratios using
>  these values.
> +
> +Configuration of Region of Interest (ROI)
> +=========================================
> +
> +The range of auto-controls values and of coordinates of the top left
> +corner, width and height of areas that can be ROI is given by the
> +``V4L2_SEL_TGT_ROI_BOUNDS_MIN`` and ``V4L2_SEL_TGT_ROI_BOUNDS_MAX``
> +targets. It is recommended for the driver developers to put the top/left
> +corner at position ``(0,0)``.
> +
> +The top left corner, width and height of the Region of Interest area
> +and auto-controls currently being employed by the device are given by
> +the ``V4L2_SEL_TGT_ROI`` target. It uses the same coordinate system
> +as ``V4L2_SEL_TGT_ROI_BOUNDS_MIN`` and ``V4L2_SEL_TGT_ROI_BOUNDS_MAX``.
> +
> +In order to change active ROI top left, width and height coordinates
> +and ROI auto-controls use ``V4L2_SEL_TGT_ROI`` target.
> +
> +Each capture device has a default ROI rectangle and auto-controls
> +value given by the ``V4L2_SEL_TGT_ROI_DEFAULT`` target. Drivers shall

nit:  Drivers may, instead of shall?



> +set the ROI rectangle to the default when the driver is first loaded,
> +but not later.
> diff --git a/Documentation/userspace-api/media/v4l/selection-api-examples.rst b/Documentation/userspace-api/media/v4l/selection-api-examples.rst
> index 5f8e8a1f59d7..ad2664888700 100644
> --- a/Documentation/userspace-api/media/v4l/selection-api-examples.rst
> +++ b/Documentation/userspace-api/media/v4l/selection-api-examples.rst
> @@ -82,3 +82,31 @@ Example: Querying for scaling factors
>         /* computing scaling factors */
>         hscale = (double)compose.r.width / crop.r.width;
>         vscale = (double)compose.r.height / crop.r.height;
> +
> +Setting Region Of Interest area to half of the default value
> +
> +Example: Simple ROI
> +===========================
> +
> +.. code-block:: c
> +
> +       struct v4l2_selection roi = {
> +           .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
> +           .target = V4L2_SEL_TGT_ROI_DEFAULT,
> +       };
> +       struct v4l2_rect r;
> +
> +       ret = ioctl(fd, VIDIOC_G_SELECTION, &roi);
> +       if (ret)
> +           exit(-1);
> +       /* setting smaller ROI rectangle */
> +       r.width = roi.r.width / 2;
> +       r.height = roi.r.height / 2;
> +       r.left = roi.r.width / 4;
> +       r.top = roi.r.height / 4;
> +       roi.r = r;
> +       roi.target = V4L2_SEL_TGT_ROI;
> +       roi.flags = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;
> +       ret = ioctl(fd, VIDIOC_S_SELECTION, &roi);
> +       if (ret)
> +           exit(-1);
> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> index b46bae984f35..d1dc9c50eb05 100644
> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> @@ -75,6 +75,30 @@ of the two interfaces they are used.
>         modified by hardware.
>        - Yes
>        - No
> +    * - ``V4L2_SEL_TGT_ROI``
> +      - 0x0200
> +      - Current Region of Interest rectangle and auto-controls value.
> +      - Yes
> +      - No
> +    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
> +      - 0x0201
> +      - Suggested Region of Interest rectangle and auto-controls value.
> +      - Yes
> +      - No
> +    * - ``V4L2_SEL_TGT_ROI_BOUNDS_MIN``
> +      - 0x0202
> +      - Minimum bounds of the Region of Interest rectangle and minimum
> +       auto-controls value. All valid ROI rectangles and auto-controls
> +       should be within minimum-maximum range.
> +      - Yes
> +      - No
> +    * - ``V4L2_SEL_TGT_ROI_BOUNDS_MAX``
> +      - 0x0203
> +      - Maximum bounds of the Region of Interest rectangle and maximum
> +       auto-controls value. All valid ROI rectangles and auto-controls
> +       should be within minimum-maximum range.
> +      - Yes
> +      - No
>
>  .. raw:: latex
>
> --
> 2.31.0.rc2.261.g7f71774620-goog
>


--
Ricardo Ribalda

Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Ricardo Ribalda]

Hi Sergey

On Fri, Mar 19, 2021 at 6:54 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> This patch implements UVC 1.5 Region of Interest (ROI) control.
>
> Note that, UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL controls
> and mentions that ROI rectangle coordinates "must be within
> the current Digital Window as specified by the CT_WINDOW control."
> (4.2.2.1.20 Digital Region of Interest (ROI) Control).
>
> It's is not entirely clear if we need to implement WINDOW_CONTROL.
> ROI is naturally limited by GET_MIN and GET_MAX rectangles.
>
> Another thing to note is that ROI support is implemented as
> V4L2 selection target: selection rectangle represents ROI
> rectangle and selection flags represent ROI auto-controls.
> User-space is required to set valid values for both rectangle
> and auto-controls every time SET_CUR is issued.
>
> Usage example:
>
>        struct v4l2_selection roi = {0, };
>
>        roi.target     = V4L2_SEL_TGT_ROI;
>        roi.r.left     = 0;
>        roi.r.top      = 0;
>        roi.r.width    = 42;
>        roi.r.height   = 42;
>        roi.flags      = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;
>
>        ioctl(fd, VIDIOC_S_SELECTION, &roi);
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 147 ++++++++++++++++++++++++++++++-
>  include/uapi/linux/usb/video.h   |   1 +
>  2 files changed, 145 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 252136cc885c..d0fe6c33fab6 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1139,14 +1139,66 @@ static int uvc_ioctl_querymenu(struct file *file, void *fh,
>         return uvc_query_v4l2_menu(chain, qm);
>  }
>
> -static int uvc_ioctl_g_selection(struct file *file, void *fh,
> -                                struct v4l2_selection *sel)
> +/* UVC 1.5 ROI rectangle is half the size of v4l2_rect */
> +struct uvc_roi_rect {
> +       __u16                   top;
> +       __u16                   left;
> +       __u16                   bottom;
> +       __u16                   right;
> +       __u16                   auto_controls;
> +} __packed;
> +
> +static int uvc_ioctl_g_roi_target(struct file *file, void *fh,
> +                                 struct v4l2_selection *sel)
>  {
>         struct uvc_fh *handle = fh;
>         struct uvc_streaming *stream = handle->stream;
> +       struct uvc_roi_rect *roi;
> +       u8 query;
> +       int ret;
>
> -       if (sel->type != stream->type)
> +       switch (sel->target) {
> +       case V4L2_SEL_TGT_ROI:
> +               query = UVC_GET_CUR;
> +               break;
> +       case V4L2_SEL_TGT_ROI_DEFAULT:
> +               query = UVC_GET_DEF;
> +               break;
> +       case V4L2_SEL_TGT_ROI_BOUNDS_MIN:
> +               query = UVC_GET_MAX;
> +               break;
> +       case V4L2_SEL_TGT_ROI_BOUNDS_MAX:
> +               query = UVC_GET_MAX;
> +               break;
> +       default:
>                 return -EINVAL;
> +       }
> +
> +       roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
> +       if (!roi)
> +               return -ENOMEM;
> +
> +       ret = uvc_query_ctrl(stream->dev, query, 1, stream->dev->intfnum,
> +                            UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
> +                            sizeof(struct uvc_roi_rect));
> +       if (!ret) {
> +               /* ROI left, top, right, bottom are global coordinates. */
> +               sel->r.left     = roi->left;
> +               sel->r.top      = roi->top;
> +               sel->r.width    = roi->right - roi->left + 1;
> +               sel->r.height   = roi->bottom - roi->top + 1;
> +               sel->flags      = roi->auto_controls;
> +       }
> +
> +       kfree(roi);
> +       return ret;
> +}
> +
> +static int uvc_ioctl_g_sel_target(struct file *file, void *fh,
> +                                 struct v4l2_selection *sel)
> +{
> +       struct uvc_fh *handle = fh;
> +       struct uvc_streaming *stream = handle->stream;
>
>         switch (sel->target) {
>         case V4L2_SEL_TGT_CROP_DEFAULT:
> @@ -1173,6 +1225,94 @@ static int uvc_ioctl_g_selection(struct file *file, void *fh,
>         return 0;
>  }
>
> +static int uvc_ioctl_g_selection(struct file *file, void *fh,
> +                                struct v4l2_selection *sel)
> +{
> +       struct uvc_fh *handle = fh;
> +       struct uvc_streaming *stream = handle->stream;
> +
> +       if (sel->type != stream->type)
> +               return -EINVAL;
> +
> +       switch (sel->target) {
> +       case V4L2_SEL_TGT_CROP_DEFAULT:
> +       case V4L2_SEL_TGT_CROP_BOUNDS:
> +       case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +       case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +               return uvc_ioctl_g_sel_target(file, fh, sel);
> +       case V4L2_SEL_TGT_ROI:
> +       case V4L2_SEL_TGT_ROI_DEFAULT:
> +       case V4L2_SEL_TGT_ROI_BOUNDS_MIN:
> +       case V4L2_SEL_TGT_ROI_BOUNDS_MAX:
> +               return uvc_ioctl_g_roi_target(file, fh, sel);
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static bool validate_roi_bounds(struct uvc_streaming *stream,
> +                               struct v4l2_selection *sel)
> +{
> +       if (sel->r.left > USHRT_MAX ||
> +           sel->r.top > USHRT_MAX ||
> +           (sel->r.width + sel->r.left) > USHRT_MAX ||
> +           (sel->r.height + sel->r.top) > USHRT_MAX ||
> +           !sel->r.width || !sel->r.height)
> +               return false;
> +
> +       if (sel->flags > V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY)
> +               return false;

Is it not allowed V4L2_SEL_FLAG_ROI_AUTO_IRIS |
V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY   ?

> +
> +       return true;
> +}
> +
> +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> +                          struct v4l2_selection *sel)
> +{
> +       struct uvc_fh *handle = fh;
> +       struct uvc_streaming *stream = handle->stream;
> +       struct uvc_roi_rect *roi;
> +       int ret;
> +
> +       if (!validate_roi_bounds(stream, sel))
> +               return -E2BIG;

Not sure if this is the correct approach or if we should convert the
value to the closest valid...


> +
> +       roi = kzalloc(sizeof(struct uvc_roi_rect), GFP_KERNEL);
> +       if (!roi)
> +               return -ENOMEM;
> +
> +       /* ROI left, top, right, bottom are global coordinates. */
> +       roi->left               = sel->r.left;
> +       roi->top                = sel->r.top;
> +       roi->right              = sel->r.width + sel->r.left - 1;
> +       roi->bottom             = sel->r.height + sel->r.top - 1;
> +       roi->auto_controls      = sel->flags;
> +
> +       ret = uvc_query_ctrl(stream->dev, UVC_SET_CUR, 1, stream->dev->intfnum,
> +                            UVC_CT_REGION_OF_INTEREST_CONTROL, roi,
> +                            sizeof(struct uvc_roi_rect));
> +
> +       kfree(roi);
> +       return ret;
> +}
> +
> +static int uvc_ioctl_s_selection(struct file *file, void *fh,
> +                                struct v4l2_selection *sel)
> +{
> +       struct uvc_fh *handle = fh;
> +       struct uvc_streaming *stream = handle->stream;
> +
> +       if (sel->type != stream->type)
> +               return -EINVAL;
> +
> +       switch (sel->target) {
> +       case V4L2_SEL_TGT_ROI:
> +               return uvc_ioctl_s_roi(file, fh, sel);
> +       }
> +
> +       return -EINVAL;
> +}
> +
>  static int uvc_ioctl_g_parm(struct file *file, void *fh,
>                             struct v4l2_streamparm *parm)
>  {
> @@ -1533,6 +1673,7 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>         .vidioc_try_ext_ctrls = uvc_ioctl_try_ext_ctrls,
>         .vidioc_querymenu = uvc_ioctl_querymenu,
>         .vidioc_g_selection = uvc_ioctl_g_selection,
> +       .vidioc_s_selection = uvc_ioctl_s_selection,
>         .vidioc_g_parm = uvc_ioctl_g_parm,
>         .vidioc_s_parm = uvc_ioctl_s_parm,
>         .vidioc_enum_framesizes = uvc_ioctl_enum_framesizes,
> diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
> index d854cb19c42c..c87624962896 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
> --
> 2.31.0.rc2.261.g7f71774620-goog
>


-- 
Ricardo Ribalda

Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

On (21/03/23 17:16), Ricardo Ribalda wrote:
[..]
> > +static bool validate_roi_bounds(struct uvc_streaming *stream,
> > +                               struct v4l2_selection *sel)
> > +{
> > +       if (sel->r.left > USHRT_MAX ||
> > +           sel->r.top > USHRT_MAX ||
> > +           (sel->r.width + sel->r.left) > USHRT_MAX ||
> > +           (sel->r.height + sel->r.top) > USHRT_MAX ||
> > +           !sel->r.width || !sel->r.height)
> > +               return false;
> > +
> > +       if (sel->flags > V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY)
> > +               return false;
> 
> Is it not allowed V4L2_SEL_FLAG_ROI_AUTO_IRIS |
> V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY   ?

Good question.

I don't know. Depends on what HIGHER_QUALITY can stand for (UVC doesn't
specify). But overall it seems like features there are mutually
exclusive. E.g. AUTO_FACE_DETECT and AUTO_DETECT_AND_TRACK.


I think it'll be better to replace this with

	if (sel->flags > USHRT_MAX)
		return false;

so that we don't let overflow happen and accidentally enable/disable
some of the features.

> > +
> > +       return true;
> > +}
> > +
> > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > +                          struct v4l2_selection *sel)
> > +{
> > +       struct uvc_fh *handle = fh;
> > +       struct uvc_streaming *stream = handle->stream;
> > +       struct uvc_roi_rect *roi;
> > +       int ret;
> > +
> > +       if (!validate_roi_bounds(stream, sel))
> > +               return -E2BIG;
> 
> Not sure if this is the correct approach or if we should convert the
> value to the closest valid...

Well, at this point we know that ROI rectangle dimensions are out of
sane value range. I'd rather tell user-space about integer overflow.

Looking for the closest ROI rectangle that suffice can be rather
tricky. It may sounds like we can just use BOUNDARIES_MAX, but this
is what Firmware D returns for GET_MAX

ioctl(V4L2_SEL_TGT_ROI_BOUNDS_MAX)

	0, 0, 65535, 65535

	-ss

Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Tomasz Figa]

On Wed, Mar 24, 2021 at 11:01 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (21/03/23 17:16), Ricardo Ribalda wrote:
> [..]
> > > +static bool validate_roi_bounds(struct uvc_streaming *stream,
> > > +                               struct v4l2_selection *sel)
> > > +{
> > > +       if (sel->r.left > USHRT_MAX ||
> > > +           sel->r.top > USHRT_MAX ||
> > > +           (sel->r.width + sel->r.left) > USHRT_MAX ||
> > > +           (sel->r.height + sel->r.top) > USHRT_MAX ||
> > > +           !sel->r.width || !sel->r.height)
> > > +               return false;
> > > +
> > > +       if (sel->flags > V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY)
> > > +               return false;
> >
> > Is it not allowed V4L2_SEL_FLAG_ROI_AUTO_IRIS |
> > V4L2_SEL_FLAG_ROI_AUTO_HIGHER_QUALITY   ?
>
> Good question.
>
> I don't know. Depends on what HIGHER_QUALITY can stand for (UVC doesn't
> specify). But overall it seems like features there are mutually
> exclusive. E.g. AUTO_FACE_DETECT and AUTO_DETECT_AND_TRACK.
>
>
> I think it'll be better to replace this with
>
>         if (sel->flags > USHRT_MAX)
>                 return false;
>
> so that we don't let overflow happen and accidentally enable/disable
> some of the features.
>
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > > +                          struct v4l2_selection *sel)
> > > +{
> > > +       struct uvc_fh *handle = fh;
> > > +       struct uvc_streaming *stream = handle->stream;
> > > +       struct uvc_roi_rect *roi;
> > > +       int ret;
> > > +
> > > +       if (!validate_roi_bounds(stream, sel))
> > > +               return -E2BIG;
> >
> > Not sure if this is the correct approach or if we should convert the
> > value to the closest valid...
>
> Well, at this point we know that ROI rectangle dimensions are out of
> sane value range. I'd rather tell user-space about integer overflow.

Adjusting the rectangle to something supported by the hardware is
mentioned explicitly in the V4L2 API documentation and is what drivers
have to implement. Returning an error on invalid value is not a
correct behavior here (and similarly for many other operations, e.g.
S_FMT).

https://www.kernel.org/doc/html/v4.8/media/uapi/v4l/vidioc-g-selection.html

>
> Looking for the closest ROI rectangle that suffice can be rather
> tricky. It may sounds like we can just use BOUNDARIES_MAX, but this
> is what Firmware D returns for GET_MAX
>
> ioctl(V4L2_SEL_TGT_ROI_BOUNDS_MAX)
>
>         0, 0, 65535, 65535

Perhaps the frame size would be the correct bounds?

Best regards,
Tomasz

Re: [PATCHv3 3/6] media: v4l UAPI: add ROI auto-controls flags

[Sergey Senozhatsky]

On (21/03/23 17:04), Ricardo Ribalda wrote:
> On Fri, Mar 19, 2021 at 6:53 AM Sergey Senozhatsky
> <senozhatsky@chromium.org> wrote:
> >
> > UVC 1.5 defines the following Region Of Interest auto controls:
> >
> > D0: Auto Exposure
> > D1: Auto Iris
> > D2: Auto White Balance
> > D3: Auto Focus
> > D4: Auto Face Detect
> > D5: Auto Detect and Track
> > D6: Image Stabilization
> > D7: Higher Quality
> > D8 – D15: Reserved, set to zero
> >
> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > ---
> >  include/uapi/linux/v4l2-common.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> > index 3651ebb8cb23..34f1c262d6aa 100644
> > --- a/include/uapi/linux/v4l2-common.h
> > +++ b/include/uapi/linux/v4l2-common.h
> > @@ -92,6 +92,16 @@
> >  #define V4L2_SEL_FLAG_LE               (1 << 1)
> >  #define V4L2_SEL_FLAG_KEEP_CONFIG      (1 << 2)
> >
> 
> Are you sure that you do not want to start with 1<<3, there might be
> some hardware that support LE/SE

How the hardware's going to support this? There is simply no way to
pass these flags to the firmware, the values already overlap with
auto-controls. So I guess these flags are for the driver (C code).
uvcvideo driver is not doing any "lesser or equal rectangle" magic
for ROI. No such thing is defined by UVC spec.

I can move these flags to entirely different value range and do
remapping to uvc auto-controls values in uvcvideo.

Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

On (21/03/24 11:14), Tomasz Figa wrote:
> > > > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > > > +                          struct v4l2_selection *sel)
> > > > +{
> > > > +       struct uvc_fh *handle = fh;
> > > > +       struct uvc_streaming *stream = handle->stream;
> > > > +       struct uvc_roi_rect *roi;
> > > > +       int ret;
> > > > +
> > > > +       if (!validate_roi_bounds(stream, sel))
> > > > +               return -E2BIG;
> > >
> > > Not sure if this is the correct approach or if we should convert the
> > > value to the closest valid...
> >
> > Well, at this point we know that ROI rectangle dimensions are out of
> > sane value range. I'd rather tell user-space about integer overflow.
> 
> Adjusting the rectangle to something supported by the hardware is
> mentioned explicitly in the V4L2 API documentation and is what drivers
> have to implement. Returning an error on invalid value is not a
> correct behavior here (and similarly for many other operations, e.g.
> S_FMT).

Well, in this particular case we are talking about user-space that wants
to set ROI rectangle that is knowingly violates device's GET_MAX and
overflows UVC ROI rectangle u16 value range. That's a clear bug in user-space.
Do we want to pretend that user-space does the correct thing and fixup
stuff behind the scenes?

> > Looking for the closest ROI rectangle that suffice can be rather
> > tricky. It may sounds like we can just use BOUNDARIES_MAX, but this
> > is what Firmware D returns for GET_MAX
> >
> > ioctl(V4L2_SEL_TGT_ROI_BOUNDS_MAX)
> >
> >         0, 0, 65535, 65535
> 
> Perhaps the frame size would be the correct bounds?

I can check that.

Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Tomasz Figa]

On Wed, Mar 24, 2021 at 11:31 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (21/03/24 11:14), Tomasz Figa wrote:
> > > > > +static int uvc_ioctl_s_roi(struct file *file, void *fh,
> > > > > +                          struct v4l2_selection *sel)
> > > > > +{
> > > > > +       struct uvc_fh *handle = fh;
> > > > > +       struct uvc_streaming *stream = handle->stream;
> > > > > +       struct uvc_roi_rect *roi;
> > > > > +       int ret;
> > > > > +
> > > > > +       if (!validate_roi_bounds(stream, sel))
> > > > > +               return -E2BIG;
> > > >
> > > > Not sure if this is the correct approach or if we should convert the
> > > > value to the closest valid...
> > >
> > > Well, at this point we know that ROI rectangle dimensions are out of
> > > sane value range. I'd rather tell user-space about integer overflow.
> >
> > Adjusting the rectangle to something supported by the hardware is
> > mentioned explicitly in the V4L2 API documentation and is what drivers
> > have to implement. Returning an error on invalid value is not a
> > correct behavior here (and similarly for many other operations, e.g.
> > S_FMT).
>
> Well, in this particular case we are talking about user-space that wants
> to set ROI rectangle that is knowingly violates device's GET_MAX and
> overflows UVC ROI rectangle u16 value range. That's a clear bug in user-space.
> Do we want to pretend that user-space does the correct thing and fixup
> stuff behind the scenes?
>

That's how the API is defined. There is a valid use case for this -
you don't need to run QUERY_CTRL if all you need is setting the
biggest possible rectangle, just set it to (0, 0), (INT_MAX, INT_MAX).

> > > Looking for the closest ROI rectangle that suffice can be rather
> > > tricky. It may sounds like we can just use BOUNDARIES_MAX, but this
> > > is what Firmware D returns for GET_MAX
> > >
> > > ioctl(V4L2_SEL_TGT_ROI_BOUNDS_MAX)
> > >
> > >         0, 0, 65535, 65535
> >
> > Perhaps the frame size would be the correct bounds?
>
> I can check that.

Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

On (21/03/24 11:34), Tomasz Figa wrote:
> On Wed, Mar 24, 2021 at 11:31 AM Sergey Senozhatsky
> <senozhatsky@chromium.org> wrote:
[..]
> > > Adjusting the rectangle to something supported by the hardware is
> > > mentioned explicitly in the V4L2 API documentation and is what drivers
> > > have to implement. Returning an error on invalid value is not a
> > > correct behavior here (and similarly for many other operations, e.g.
> > > S_FMT).
> >
> > Well, in this particular case we are talking about user-space that wants
> > to set ROI rectangle that is knowingly violates device's GET_MAX and
> > overflows UVC ROI rectangle u16 value range. That's a clear bug in user-space.
> > Do we want to pretend that user-space does the correct thing and fixup
> > stuff behind the scenes?
> >
> 
> That's how the API is defined. There is a valid use case for this -
> you don't need to run QUERY_CTRL if all you need is setting the
> biggest possible rectangle, just set it to (0, 0), (INT_MAX, INT_MAX).

I guess in our case we need to talk about rectangle,auto-controls tuple
that we send to firmware

	rect {
		(0, 0), (INT_MAX, INT_MAX)
	}
	auto-controls {
		INT_MAX
	}

For ROI user-space also must provide valid auto-controls value, which
normally requires GET_MIN/GET_MAX discovery.

v4l2 selection API mentions only rectangle adjustments and errnos like
-ERANGE also mention "It is not possible to adjust struct v4l2_rect r
rectangle to satisfy all constraints given in the flags argument".

So in case when auto-controls is out of supported range (out of
GET_MIN, GET_MAX range) there is no way for us to tell user-space that
auto-controls is wrong. We probably need silently pick up the first
supported value, but not sure how well this will work out in the end.

Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Tomasz Figa]

On Wed, Mar 24, 2021 at 11:52 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (21/03/24 11:34), Tomasz Figa wrote:
> > On Wed, Mar 24, 2021 at 11:31 AM Sergey Senozhatsky
> > <senozhatsky@chromium.org> wrote:
> [..]
> > > > Adjusting the rectangle to something supported by the hardware is
> > > > mentioned explicitly in the V4L2 API documentation and is what drivers
> > > > have to implement. Returning an error on invalid value is not a
> > > > correct behavior here (and similarly for many other operations, e.g.
> > > > S_FMT).
> > >
> > > Well, in this particular case we are talking about user-space that wants
> > > to set ROI rectangle that is knowingly violates device's GET_MAX and
> > > overflows UVC ROI rectangle u16 value range. That's a clear bug in user-space.
> > > Do we want to pretend that user-space does the correct thing and fixup
> > > stuff behind the scenes?
> > >
> >
> > That's how the API is defined. There is a valid use case for this -
> > you don't need to run QUERY_CTRL if all you need is setting the
> > biggest possible rectangle, just set it to (0, 0), (INT_MAX, INT_MAX).
>
> I guess in our case we need to talk about rectangle,auto-controls tuple
> that we send to firmware
>
>         rect {
>                 (0, 0), (INT_MAX, INT_MAX)
>         }
>         auto-controls {
>                 INT_MAX
>         }
>
> For ROI user-space also must provide valid auto-controls value, which
> normally requires GET_MIN/GET_MAX discovery.
>
> v4l2 selection API mentions only rectangle adjustments and errnos like
> -ERANGE also mention "It is not possible to adjust struct v4l2_rect r
> rectangle to satisfy all constraints given in the flags argument".
>
> So in case when auto-controls is out of supported range (out of
> GET_MIN, GET_MAX range) there is no way for us to tell user-space that
> auto-controls is wrong. We probably need silently pick up the first
> supported value, but not sure how well this will work out in the end.

Shouldn't the autocontrol selection be done via a separate bitmask
control rather than some custom flags in the selection API?

Best regards,
Tomasz

Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

On (21/03/24 12:00), Tomasz Figa wrote:
[..]
> > I guess in our case we need to talk about rectangle,auto-controls tuple
> > that we send to firmware
> >
> >         rect {
> >                 (0, 0), (INT_MAX, INT_MAX)
> >         }
> >         auto-controls {
> >                 INT_MAX
> >         }
> >
> > For ROI user-space also must provide valid auto-controls value, which
> > normally requires GET_MIN/GET_MAX discovery.
> >
> > v4l2 selection API mentions only rectangle adjustments and errnos like
> > -ERANGE also mention "It is not possible to adjust struct v4l2_rect r
> > rectangle to satisfy all constraints given in the flags argument".
> >
> > So in case when auto-controls is out of supported range (out of
> > GET_MIN, GET_MAX range) there is no way for us to tell user-space that
> > auto-controls is wrong. We probably need silently pick up the first
> > supported value, but not sure how well this will work out in the end.
> 
> Shouldn't the autocontrol selection be done via a separate bitmask
> control rather than some custom flags in the selection API?

That selection must be done before we send ROI to the firmware.
Firmware H that I have supports split controls - we can send
ROI::rectangle and ROI::autocontrols separately. But other
firmwares don't tolerate such a thing and by the time we issue

	uvc_query_ctrl(stream->dev,
	               UVC_SET_CUR
	               UVC_CT_REGION_OF_INTEREST_CONTROL
		       roi,
+                      sizeof(struct uvc_roi_rect))

roi rectangle should be of size 5 * u16 and contain values that firmware
will accept, including autocontrols.

Re: [PATCHv3 5/6] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

On (21/03/24 12:05), Sergey Senozhatsky wrote:
> > > For ROI user-space also must provide valid auto-controls value, which
> > > normally requires GET_MIN/GET_MAX discovery.
> > >
> > > v4l2 selection API mentions only rectangle adjustments and errnos like
> > > -ERANGE also mention "It is not possible to adjust struct v4l2_rect r
> > > rectangle to satisfy all constraints given in the flags argument".
> > >
> > > So in case when auto-controls is out of supported range (out of
> > > GET_MIN, GET_MAX range) there is no way for us to tell user-space that
> > > auto-controls is wrong. We probably need silently pick up the first
> > > supported value, but not sure how well this will work out in the end.
> > 
> > Shouldn't the autocontrol selection be done via a separate bitmask
> > control rather than some custom flags in the selection API?
> 
> That selection must be done before we send ROI to the firmware.
> Firmware H that I have supports split controls - we can send
> ROI::rectangle and ROI::autocontrols separately. But other
> firmwares don't tolerate such a thing and by the time we issue
> 
> 	uvc_query_ctrl(stream->dev,
> 	               UVC_SET_CUR
> 	               UVC_CT_REGION_OF_INTEREST_CONTROL
> 		       roi,
> +                      sizeof(struct uvc_roi_rect))
> 
> roi rectangle should be of size 5 * u16 and contain values that firmware

      ^^^ roi structure

> will accept, including autocontrols.

Re: [PATCHv3 3/6] media: v4l UAPI: add ROI auto-controls flags

[Ricardo Ribalda]

HI Sergey

On Wed, Mar 24, 2021 at 3:22 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (21/03/23 17:04), Ricardo Ribalda wrote:
> > On Fri, Mar 19, 2021 at 6:53 AM Sergey Senozhatsky
> > <senozhatsky@chromium.org> wrote:
> > >
> > > UVC 1.5 defines the following Region Of Interest auto controls:
> > >
> > > D0: Auto Exposure
> > > D1: Auto Iris
> > > D2: Auto White Balance
> > > D3: Auto Focus
> > > D4: Auto Face Detect
> > > D5: Auto Detect and Track
> > > D6: Image Stabilization
> > > D7: Higher Quality
> > > D8 – D15: Reserved, set to zero
> > >
> > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > ---
> > >  include/uapi/linux/v4l2-common.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> > > index 3651ebb8cb23..34f1c262d6aa 100644
> > > --- a/include/uapi/linux/v4l2-common.h
> > > +++ b/include/uapi/linux/v4l2-common.h
> > > @@ -92,6 +92,16 @@
> > >  #define V4L2_SEL_FLAG_LE               (1 << 1)
> > >  #define V4L2_SEL_FLAG_KEEP_CONFIG      (1 << 2)
> > >
> >
> > Are you sure that you do not want to start with 1<<3, there might be
> > some hardware that support LE/SE
>
> How the hardware's going to support this? There is simply no way to
> pass these flags to the firmware, the values already overlap with
> auto-controls. So I guess these flags are for the driver (C code).
> uvcvideo driver is not doing any "lesser or equal rectangle" magic
> for ROI. No such thing is defined by UVC spec.

The driver can implement se/le.

>
> I can move these flags to entirely different value range and do
> remapping to uvc auto-controls values in uvcvideo.
I think that is more correct in this case. Yes it is annoying, but if
more devices support this....



-- 
Ricardo Ribalda

Re: [PATCHv3 3/6] media: v4l UAPI: add ROI auto-controls flags

[Sergey Senozhatsky]

On (21/03/24 08:28), Ricardo Ribalda wrote:
[..]
> > >
> > > Are you sure that you do not want to start with 1<<3, there might be
> > > some hardware that support LE/SE
> >
> > How the hardware's going to support this? There is simply no way to
> > pass these flags to the firmware, the values already overlap with
> > auto-controls. So I guess these flags are for the driver (C code).
> > uvcvideo driver is not doing any "lesser or equal rectangle" magic
> > for ROI. No such thing is defined by UVC spec.
> 
> The driver can implement se/le.

Right. I wonder if we can actually fit ROI into selection API.
v4l2 selection is focusing on rectangle, that's the only thing
that matters, but in ROI rectangle and autocontrols are equally
important.