[PATCHv2 0/3] Add UVC 1.5 Region Of Interest control to uvcvideo

[PATCHv2 0/3] Add UVC 1.5 Region Of Interest control to uvcvideo

[Sergey Senozhatsky]

	Hello,

	RFC

This patch set adds UVC 1.5 Region of Interest support.

v1->v2:
- Address Laurent's comments

Sergey Senozhatsky (3):
  media: v4l UAPI docs: document ROI selection targets
  media: uvcvideo: add ROI auto controls
  media: uvcvideo: add UVC 1.5 ROI control

 .../media/v4l/ext-ctrls-camera.rst            |  25 +++
 .../media/v4l/selection-api-configuration.rst |  23 +++
 .../media/v4l/v4l2-selection-targets.rst      |  21 +++
 drivers/media/usb/uvc/uvc_ctrl.c              |  19 +++
 drivers/media/usb/uvc/uvc_v4l2.c              | 143 +++++++++++++++++-
 include/uapi/linux/usb/video.h                |   1 +
 include/uapi/linux/v4l2-common.h              |   8 +
 include/uapi/linux/v4l2-controls.h            |   9 ++
 8 files changed, 246 insertions(+), 3 deletions(-)

-- 
2.30.0

[PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets

[Sergey Senozhatsky]

From: Sergey Senozhatsky <senozhatsky@chromium.org>

Document new v4l2-selection target which will be used for the
Region of Interest v4l2 control.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../media/v4l/selection-api-configuration.rst | 23 +++++++++++++++++++
 .../media/v4l/v4l2-selection-targets.rst      | 21 +++++++++++++++++
 include/uapi/linux/v4l2-common.h              |  8 +++++++
 3 files changed, 52 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..9f69d71803f6 100644
--- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
+++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
@@ -135,3 +135,26 @@ 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 coordinates of the top left corner, width and height of
+areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target.
+It is recommended for the driver developers to put the top/left corner
+at position ``(0,0)``. The rectangle's coordinates are in global sensor
+coordinates. The units are in pixels and independent of the field of view.
+They are not impacted by any cropping or scaling that is currently being
+used.
+
+The top left corner, width and height of the Region of Interest area
+currently being employed by the device is given by the
+``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system
+as ``V4L2_SEL_TGT_ROI_BOUNDS``.
+
+In order to change active ROI top left, width and height coordinates
+use ``V4L2_SEL_TGT_ROI`` target.
+
+Each capture device has a default ROI rectangle, 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/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
index e877ebbdb32e..cb3809418fa6 100644
--- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
+++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
@@ -69,3 +69,24 @@ of the two interfaces they are used.
 	modified by hardware.
       - Yes
       - No
+    * - ``V4L2_SEL_TGT_ROI_CURRENT``
+      - 0x0200
+      - Current Region of Interest rectangle.
+      - Yes
+      - No
+    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
+      - 0x0201
+      - Suggested Region of Interest rectangle.
+      - Yes
+      - No
+    * - ``V4L2_SEL_TGT_ROI_BOUNDS``
+      - 0x0202
+      - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit
+	inside the ROI bounds rectangle.
+      - Yes
+      - No
+    * - ``V4L2_SEL_TGT_ROI``
+      - 0x0203
+      - Sets the new Region of Interest rectangle.
+      - Yes
+      - No
diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
index 7d21c1634b4d..d0c108fba638 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_CURRENT	0x0200
+/* Default Region of Interest area */
+#define V4L2_SEL_TGT_ROI_DEFAULT	0x0201
+/* Region of Interest bounds */
+#define V4L2_SEL_TGT_ROI_BOUNDS	0x0202
+/* Set Region of Interest area */
+#define V4L2_SEL_TGT_ROI		0x0203
 
 /* Selection flags */
 #define V4L2_SEL_FLAG_GE		(1 << 0)
-- 
2.30.0

Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets

[Ricardo Ribalda Delgado]

Hi Sergey

Thanks for the patch!

On Mon, Feb 8, 2021 at 6:21 AM Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
>
> From: Sergey Senozhatsky <senozhatsky@chromium.org>
>
> Document new v4l2-selection target which will be used for the
> Region of Interest v4l2 control.
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  .../media/v4l/selection-api-configuration.rst | 23 +++++++++++++++++++
>  .../media/v4l/v4l2-selection-targets.rst      | 21 +++++++++++++++++
>  include/uapi/linux/v4l2-common.h              |  8 +++++++
>  3 files changed, 52 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..9f69d71803f6 100644
> --- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
> +++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
> @@ -135,3 +135,26 @@ 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 coordinates of the top left corner, width and height of
> +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target.
> +It is recommended for the driver developers to put the top/left corner
> +at position ``(0,0)``. The rectangle's coordinates are in global sensor
> +coordinates. The units are in pixels and independent of the field of view.
> +They are not impacted by any cropping or scaling that is currently being
> +used.

Can we also mention binning here?

> +
> +The top left corner, width and height of the Region of Interest area
> +currently being employed by the device is given by the
> +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system
> +as ``V4L2_SEL_TGT_ROI_BOUNDS``.

Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ?

> +
> +In order to change active ROI top left, width and height coordinates
> +use ``V4L2_SEL_TGT_ROI`` target.
> +
> +Each capture device has a default ROI rectangle, 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/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> index e877ebbdb32e..cb3809418fa6 100644
> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> @@ -69,3 +69,24 @@ of the two interfaces they are used.
>         modified by hardware.
>        - Yes
>        - No
> +    * - ``V4L2_SEL_TGT_ROI_CURRENT``
> +      - 0x0200
> +      - Current Region of Interest rectangle.
> +      - Yes
> +      - No
> +    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
> +      - 0x0201
> +      - Suggested Region of Interest rectangle.
> +      - Yes
> +      - No
> +    * - ``V4L2_SEL_TGT_ROI_BOUNDS``
> +      - 0x0202
> +      - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit
> +       inside the ROI bounds rectangle.
> +      - Yes
> +      - No
> +    * - ``V4L2_SEL_TGT_ROI``
> +      - 0x0203
> +      - Sets the new Region of Interest rectangle.
> +      - Yes
> +      - No
As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI


> diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> index 7d21c1634b4d..d0c108fba638 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_CURRENT       0x0200
> +/* Default Region of Interest area */
> +#define V4L2_SEL_TGT_ROI_DEFAULT       0x0201
> +/* Region of Interest bounds */
> +#define V4L2_SEL_TGT_ROI_BOUNDS        0x0202
> +/* Set Region of Interest area */
> +#define V4L2_SEL_TGT_ROI               0x0203

Nit: Maybe it could be a good idea to split doc and code. This way the
backports/fixes are easier.

>
>  /* Selection flags */
>  #define V4L2_SEL_FLAG_GE               (1 << 0)
> --
> 2.30.0
>


-- 
Ricardo Ribalda

Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets

[Sergey Senozhatsky]

On (21/03/16 19:19), Ricardo Ribalda Delgado wrote:
> > +Configuration of Region of Interest (ROI)
> > +=========================================
> > +
> > +The range 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`` target.
> > +It is recommended for the driver developers to put the top/left corner
> > +at position ``(0,0)``. The rectangle's coordinates are in global sensor
> > +coordinates. The units are in pixels and independent of the field of view.
> > +They are not impacted by any cropping or scaling that is currently being
> > +used.
> 
> Can we also mention binning here?

What's binning? Is it in the UVC spec?

> > +The top left corner, width and height of the Region of Interest area
> > +currently being employed by the device is given by the
> > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system
> > +as ``V4L2_SEL_TGT_ROI_BOUNDS``.
> 
> Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ?

We don't. Will remove it.

> > +    * - ``V4L2_SEL_TGT_ROI_CURRENT``
> > +      - 0x0200
> > +      - Current Region of Interest rectangle.
> > +      - Yes
> > +      - No
> > +    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
> > +      - 0x0201
> > +      - Suggested Region of Interest rectangle.
> > +      - Yes
> > +      - No
> > +    * - ``V4L2_SEL_TGT_ROI_BOUNDS``
> > +      - 0x0202
> > +      - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit
> > +       inside the ROI bounds rectangle.
> > +      - Yes
> > +      - No
> > +    * - ``V4L2_SEL_TGT_ROI``
> > +      - 0x0203
> > +      - Sets the new Region of Interest rectangle.
> > +      - Yes
> > +      - No
> As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI

Agreed.

> > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> > index 7d21c1634b4d..d0c108fba638 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_CURRENT       0x0200
> > +/* Default Region of Interest area */
> > +#define V4L2_SEL_TGT_ROI_DEFAULT       0x0201
> > +/* Region of Interest bounds */
> > +#define V4L2_SEL_TGT_ROI_BOUNDS        0x0202
> > +/* Set Region of Interest area */
> > +#define V4L2_SEL_TGT_ROI               0x0203
> 
> Nit: Maybe it could be a good idea to split doc and code. This way the
> backports/fixes are easier.

I'm quite sure this is the first time I'm being asked to split code
and documentation :) I'm usually asked to do the opposite - merge code
and documentation.

Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets

[Ricardo Ribalda Delgado]

Hi

On Wed, Mar 17, 2021 at 2:31 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (21/03/16 19:19), Ricardo Ribalda Delgado wrote:
> > > +Configuration of Region of Interest (ROI)
> > > +=========================================
> > > +
> > > +The range 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`` target.
> > > +It is recommended for the driver developers to put the top/left corner
> > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor
> > > +coordinates. The units are in pixels and independent of the field of view.
> > > +They are not impacted by any cropping or scaling that is currently being
> > > +used.
> >
> > Can we also mention binning here?
>
> What's binning? Is it in the UVC spec?

Binning is when you reduce an image by adding up surrounding pixels.

So you have a 100x100 image that you convert to a 50x50 but showing
the same area of interest.


>
> > > +The top left corner, width and height of the Region of Interest area
> > > +currently being employed by the device is given by the
> > > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system
> > > +as ``V4L2_SEL_TGT_ROI_BOUNDS``.
> >
> > Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ?
>
> We don't. Will remove it.
>
> > > +    * - ``V4L2_SEL_TGT_ROI_CURRENT``
> > > +      - 0x0200
> > > +      - Current Region of Interest rectangle.
> > > +      - Yes
> > > +      - No
> > > +    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
> > > +      - 0x0201
> > > +      - Suggested Region of Interest rectangle.
> > > +      - Yes
> > > +      - No
> > > +    * - ``V4L2_SEL_TGT_ROI_BOUNDS``
> > > +      - 0x0202
> > > +      - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit
> > > +       inside the ROI bounds rectangle.
> > > +      - Yes
> > > +      - No
> > > +    * - ``V4L2_SEL_TGT_ROI``
> > > +      - 0x0203
> > > +      - Sets the new Region of Interest rectangle.
> > > +      - Yes
> > > +      - No
> > As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI
>
> Agreed.
>
> > > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> > > index 7d21c1634b4d..d0c108fba638 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_CURRENT       0x0200
> > > +/* Default Region of Interest area */
> > > +#define V4L2_SEL_TGT_ROI_DEFAULT       0x0201
> > > +/* Region of Interest bounds */
> > > +#define V4L2_SEL_TGT_ROI_BOUNDS        0x0202
> > > +/* Set Region of Interest area */
> > > +#define V4L2_SEL_TGT_ROI               0x0203
> >
> > Nit: Maybe it could be a good idea to split doc and code. This way the
> > backports/fixes are easier.
>
> I'm quite sure this is the first time I'm being asked to split code
> and documentation :) I'm usually asked to do the opposite - merge code
> and documentation.

I got answered in both directions.  I prefer to split it because the
doc can go to different audience than the code, and then it makes my
life easier when backporting.

But if you or Laurent prefer  otherwise I am of course happy with any option ;)



-- 
Ricardo Ribalda

Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets

[Sergey Senozhatsky]

On (21/03/17 09:04), Ricardo Ribalda Delgado wrote:
> On Wed, Mar 17, 2021 at 2:31 AM Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> >
> > On (21/03/16 19:19), Ricardo Ribalda Delgado wrote:
> > > > +Configuration of Region of Interest (ROI)
> > > > +=========================================
> > > > +
> > > > +The range 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`` target.
> > > > +It is recommended for the driver developers to put the top/left corner
> > > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor
> > > > +coordinates. The units are in pixels and independent of the field of view.
> > > > +They are not impacted by any cropping or scaling that is currently being
> > > > +used.
> > >
> > > Can we also mention binning here?
> >
> > What's binning? Is it in the UVC spec?
> 
> Binning is when you reduce an image by adding up surrounding pixels.
> 
> So you have a 100x100 image that you convert to a 50x50 but showing
> the same area of interest.

I see. Hmm, not sure if I can comment on this. It's not in the spec,
so it might be up to the firmware, maybe. What do you think?

> > > > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> > > > index 7d21c1634b4d..d0c108fba638 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_CURRENT       0x0200
> > > > +/* Default Region of Interest area */
> > > > +#define V4L2_SEL_TGT_ROI_DEFAULT       0x0201
> > > > +/* Region of Interest bounds */
> > > > +#define V4L2_SEL_TGT_ROI_BOUNDS        0x0202
> > > > +/* Set Region of Interest area */
> > > > +#define V4L2_SEL_TGT_ROI               0x0203
> > >
> > > Nit: Maybe it could be a good idea to split doc and code. This way the
> > > backports/fixes are easier.
> >
> > I'm quite sure this is the first time I'm being asked to split code
> > and documentation :) I'm usually asked to do the opposite - merge code
> > and documentation.
> 
> I got answered in both directions.  I prefer to split it because the
> doc can go to different audience than the code, and then it makes my
> life easier when backporting.
> 
> But if you or Laurent prefer  otherwise I am of course happy with any option ;)

Either way works for me. Laurent, any preferences?

	-ss

[PATCHv2 2/3] media: uvcvideo: add ROI auto controls

[Sergey Senozhatsky]

From: Sergey Senozhatsky <senozhatsky@chromium.org>

This patch adds support for Region of Interest bmAutoControls.

ROI control is a compound data type:
  Control Selector     CT_REGION_OF_INTEREST_CONTROL
  Mandatory Requests   SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
  wLength 10
  Offset   Field            Size
  0        wROI_Top         2
  2        wROI_Left        2
  4        wROI_Bottom      2
  6        wROI_Right       2
  8        bmAutoControls   2       (Bitmap)

uvc_control_mapping, however, can handle only s32 data type at the
moment: ->get() returns s32 value, ->set() accepts s32 value; while
v4l2_ctrl maximum/minimum/default_value can hold only s64 values.

Hence ROI control handling is split into two patches:
a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
   separately, by the means of selection API.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../media/v4l/ext-ctrls-camera.rst            | 25 +++++++++++++++++++
 drivers/media/usb/uvc/uvc_ctrl.c              | 19 ++++++++++++++
 include/uapi/linux/usb/video.h                |  1 +
 include/uapi/linux/v4l2-controls.h            |  9 +++++++
 4 files changed, 54 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index c05a2d2c675d..1593c999c8e2 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -653,6 +653,31 @@ enum v4l2_scene_mode -
                           |                    |
                           +--------------------+
 
+``V4L2_CID_REGION_OF_INTEREST_AUTO (bitmask)``
+    This determines which, if any, on board features should track to the
+    Region of Interest.
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
+      - Auto Exposure.
+    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
+      - Auto Iris.
+    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
+      - Auto White Balance.
+    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
+      - Auto Focus.
+    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
+      - Auto Face Detect.
+    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
+      - Auto Detect and Track.
+    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION``
+      - Image Stabilization.
+    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
+      - Higher Quality.
+
 
 .. [#f1]
    This control may be changed to a menu control in the future, if more
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index b3dde98499f4..5502fe540519 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -355,6 +355,15 @@ static const struct uvc_control_info uvc_ctrls[] = {
 		.flags		= UVC_CTRL_FLAG_GET_CUR
 				| UVC_CTRL_FLAG_AUTO_UPDATE,
 	},
+	{
+		.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
+	},
 };
 
 static const struct uvc_menu_info power_line_frequency_controls[] = {
@@ -753,6 +762,16 @@ 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_REGION_OF_INTEREST_AUTO,
+		.name		= "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,
+	},
 };
 
 /* ------------------------------------------------------------------------
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
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 039c0d7add1b..6a3dac481cb4 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -976,6 +976,15 @@ enum v4l2_auto_focus_range {
 
 #define V4L2_CID_PAN_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+32)
 #define V4L2_CID_TILT_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+33)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO	(V4L2_CID_CAMERA_CLASS_BASE+34)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE		(1 << 0)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS			(1 << 1)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE		(1 << 2)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS			(1 << 3)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT		(1 << 4)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK	(1 << 5)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION	(1 << 6)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY	(1 << 7)
 
 #define V4L2_CID_CAMERA_ORIENTATION		(V4L2_CID_CAMERA_CLASS_BASE+34)
 #define V4L2_CAMERA_ORIENTATION_FRONT		0
-- 
2.30.0

Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls

[Ricardo Ribalda Delgado]

Hi Sergey

Thanks for the patch

On Mon, Feb 8, 2021 at 6:24 AM Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
>
> From: Sergey Senozhatsky <senozhatsky@chromium.org>
>
> This patch adds support for Region of Interest bmAutoControls.
>
> ROI control is a compound data type:
>   Control Selector     CT_REGION_OF_INTEREST_CONTROL
>   Mandatory Requests   SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
>   wLength 10
>   Offset   Field            Size
>   0        wROI_Top         2
>   2        wROI_Left        2
>   4        wROI_Bottom      2
>   6        wROI_Right       2
>   8        bmAutoControls   2       (Bitmap)
>
> uvc_control_mapping, however, can handle only s32 data type at the
> moment: ->get() returns s32 value, ->set() accepts s32 value; while
> v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
>
> Hence ROI control handling is split into two patches:
> a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
>    separately, by the means of selection API.

Maybe a reference to the uvc doc would be a good thing to add here.
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  .../media/v4l/ext-ctrls-camera.rst            | 25 +++++++++++++++++++
>  drivers/media/usb/uvc/uvc_ctrl.c              | 19 ++++++++++++++
>  include/uapi/linux/usb/video.h                |  1 +
>  include/uapi/linux/v4l2-controls.h            |  9 +++++++
>  4 files changed, 54 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index c05a2d2c675d..1593c999c8e2 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -653,6 +653,31 @@ enum v4l2_scene_mode -
>                            |                    |
>                            +--------------------+
>
> +``V4L2_CID_REGION_OF_INTEREST_AUTO (bitmask)``
> +    This determines which, if any, on board features should track to the
> +    Region of Interest.
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
> +      - Auto Exposure.
> +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
> +      - Auto Iris.
> +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> +      - Auto White Balance.
> +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
> +      - Auto Focus.
> +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> +      - Auto Face Detect.
> +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> +      - Auto Detect and Track.
> +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION``
> +      - Image Stabilization.
> +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> +      - Higher Quality.
> +
>
Nit: Same as before splitting doc and code.

>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index b3dde98499f4..5502fe540519 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -355,6 +355,15 @@ static const struct uvc_control_info uvc_ctrls[] = {
>                 .flags          = UVC_CTRL_FLAG_GET_CUR
>                                 | UVC_CTRL_FLAG_AUTO_UPDATE,
>         },
> +       {
> +               .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
> +       },
>  };
>
>  static const struct uvc_menu_info power_line_frequency_controls[] = {
> @@ -753,6 +762,16 @@ 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_REGION_OF_INTEREST_AUTO,
> +               .name           = "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,
Are

> +               .data_type      = UVC_CTRL_DATA_TYPE_BITMASK,
> +       },
>  };
>
>  /* ------------------------------------------------------------------------
> 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
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 039c0d7add1b..6a3dac481cb4 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -976,6 +976,15 @@ enum v4l2_auto_focus_range {
>
>  #define V4L2_CID_PAN_SPEED                     (V4L2_CID_CAMERA_CLASS_BASE+32)
>  #define V4L2_CID_TILT_SPEED                    (V4L2_CID_CAMERA_CLASS_BASE+33)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO       (V4L2_CID_CAMERA_CLASS_BASE+34)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE              (1 << 0)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS                  (1 << 1)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE         (1 << 2)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS                 (1 << 3)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT           (1 << 4)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK      (1 << 5)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION   (1 << 6)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY        (1 << 7)
>
>  #define V4L2_CID_CAMERA_ORIENTATION            (V4L2_CID_CAMERA_CLASS_BASE+34)
>  #define V4L2_CAMERA_ORIENTATION_FRONT          0
> --
> 2.30.0
>

I think we have to add the CID to v4l2_ctrl_get_name()

-- 
Ricardo Ribalda

Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls

[Sergey Senozhatsky]

On (21/03/16 19:29), Ricardo Ribalda Delgado wrote:
> > ROI control is a compound data type:
> >   Control Selector     CT_REGION_OF_INTEREST_CONTROL
> >   Mandatory Requests   SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
> >   wLength 10
> >   Offset   Field            Size
> >   0        wROI_Top         2
> >   2        wROI_Left        2
> >   4        wROI_Bottom      2
> >   6        wROI_Right       2
> >   8        bmAutoControls   2       (Bitmap)
> >
> > uvc_control_mapping, however, can handle only s32 data type at the
> > moment: ->get() returns s32 value, ->set() accepts s32 value; while
> > v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
> >
> > Hence ROI control handling is split into two patches:
> > a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> > b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
> >    separately, by the means of selection API.
> 
> Maybe a reference to the uvc doc would be a good thing to add here.

OK. What should be referenced there?

> > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
> > +      - Auto Exposure.
> > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
> > +      - Auto Iris.
> > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> > +      - Auto White Balance.
> > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
> > +      - Auto Focus.
> > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> > +      - Auto Face Detect.
> > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> > +      - Auto Detect and Track.
> > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION``
> > +      - Image Stabilization.
> > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> > +      - Higher Quality.
> > +
> >
> Nit: Same as before splitting doc and code.

OK, I guess I can split.

> >  static const struct uvc_menu_info power_line_frequency_controls[] = {
> > @@ -753,6 +762,16 @@ 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_REGION_OF_INTEREST_AUTO,
> > +               .name           = "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,
> Are

Are?

> >  #define V4L2_CID_PAN_SPEED                     (V4L2_CID_CAMERA_CLASS_BASE+32)
> >  #define V4L2_CID_TILT_SPEED                    (V4L2_CID_CAMERA_CLASS_BASE+33)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO       (V4L2_CID_CAMERA_CLASS_BASE+34)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE              (1 << 0)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS                  (1 << 1)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE         (1 << 2)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS                 (1 << 3)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT           (1 << 4)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK      (1 << 5)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION   (1 << 6)
> > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY        (1 << 7)
> >
> >  #define V4L2_CID_CAMERA_ORIENTATION            (V4L2_CID_CAMERA_CLASS_BASE+34)
> >  #define V4L2_CAMERA_ORIENTATION_FRONT          0
> > --
> > 2.30.0
> >
> 
> I think we have to add the CID to v4l2_ctrl_get_name()

Sounds good. Only this one - V4L2_CID_REGION_OF_INTEREST_AUTO, right?

Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls

[Ricardo Ribalda Delgado]

On Wed, Mar 17, 2021 at 2:34 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (21/03/16 19:29), Ricardo Ribalda Delgado wrote:
> > > ROI control is a compound data type:
> > >   Control Selector     CT_REGION_OF_INTEREST_CONTROL
> > >   Mandatory Requests   SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
> > >   wLength 10
> > >   Offset   Field            Size
> > >   0        wROI_Top         2
> > >   2        wROI_Left        2
> > >   4        wROI_Bottom      2
> > >   6        wROI_Right       2
> > >   8        bmAutoControls   2       (Bitmap)
> > >
> > > uvc_control_mapping, however, can handle only s32 data type at the
> > > moment: ->get() returns s32 value, ->set() accepts s32 value; while
> > > v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
> > >
> > > Hence ROI control handling is split into two patches:
> > > a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> > > b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
> > >    separately, by the means of selection API.
> >
> > Maybe a reference to the uvc doc would be a good thing to add here.
>
> OK. What should be referenced there?

Nevermind, I thought there was a non-pdf version of the standard :(
https://www.usb.org/document-library/video-class-v15-document-set

>
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
> > > +      - Auto Exposure.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
> > > +      - Auto Iris.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> > > +      - Auto White Balance.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
> > > +      - Auto Focus.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> > > +      - Auto Face Detect.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> > > +      - Auto Detect and Track.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION``
> > > +      - Image Stabilization.
> > > +    * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> > > +      - Higher Quality.
> > > +
> > >
> > Nit: Same as before splitting doc and code.
>
> OK, I guess I can split.
>
> > >  static const struct uvc_menu_info power_line_frequency_controls[] = {
> > > @@ -753,6 +762,16 @@ 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_REGION_OF_INTEREST_AUTO,
> > > +               .name           = "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,
> > Are
>
> Are?

Aye!
You are not a good kernel reviewer if you do not talk pirate :P.

I wanted to make sure that V4L2_CTRL_TYPE_BITMASK was handled by the
uvc driver, and I think that it will work fine.
Sorry for the noise.

>
> > >  #define V4L2_CID_PAN_SPEED                     (V4L2_CID_CAMERA_CLASS_BASE+32)
> > >  #define V4L2_CID_TILT_SPEED                    (V4L2_CID_CAMERA_CLASS_BASE+33)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO       (V4L2_CID_CAMERA_CLASS_BASE+34)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE              (1 << 0)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS                  (1 << 1)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE         (1 << 2)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS                 (1 << 3)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT           (1 << 4)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK      (1 << 5)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIXATION   (1 << 6)
> > > +#define V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY        (1 << 7)
> > >
> > >  #define V4L2_CID_CAMERA_ORIENTATION            (V4L2_CID_CAMERA_CLASS_BASE+34)
> > >  #define V4L2_CAMERA_ORIENTATION_FRONT          0
> > > --
> > > 2.30.0
> > >
> >
> > I think we have to add the CID to v4l2_ctrl_get_name()
>
> Sounds good. Only this one - V4L2_CID_REGION_OF_INTEREST_AUTO, right?

I think so :)

Thanks!



-- 
Ricardo Ribalda

Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls

[Sergey Senozhatsky]

Fixed Tomasz's email

On (21/03/17 09:08), Ricardo Ribalda Delgado wrote:
[..]
> > > > +               .id             = V4L2_CID_REGION_OF_INTEREST_AUTO,
> > > > +               .name           = "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,
> > > Are
> >
> > Are?
> 
> Aye!
> You are not a good kernel reviewer if you do not talk pirate :P.

Arr, Matey!

	-ss

Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls

[Sergey Senozhatsky]

On (21/02/08 14:17), Sergey Senozhatsky wrote:
> This patch adds support for Region of Interest bmAutoControls.
> 
> ROI control is a compound data type:
>   Control Selector     CT_REGION_OF_INTEREST_CONTROL
>   Mandatory Requests   SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
>   wLength 10
>   Offset   Field            Size
>   0        wROI_Top         2
>   2        wROI_Left        2
>   4        wROI_Bottom      2
>   6        wROI_Right       2
>   8        bmAutoControls   2       (Bitmap)
> 
> uvc_control_mapping, however, can handle only s32 data type at the
> moment: ->get() returns s32 value, ->set() accepts s32 value; while
> v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
> 
> Hence ROI control handling is split into two patches:
> a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
>    separately, by the means of selection API.

This approach is "no go".

I just figured out (am still debugging tho) that this patch set works on
some devices and doesn't work on other. The root cause seems to be the
fact that some firmwares error out all ROI requests when sizeof() of the
ROI data is not 5 * __u16.

So those devices are not happy if we set/get ROI rectangle 4 * __u16
and auto-controls __u16 separately; they want to set/get rect and
rectanles in one shot.

This fixes ROI on those devices.

---

+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1190,6 +1190,7 @@ struct uvc_roi_rect {
        __u16                   left;
        __u16                   bottom;
        __u16                   right;
+       __u16                   auto_controls;
 };

---

Back to base 1.

Re: [PATCHv2 2/3] media: uvcvideo: add ROI auto controls

[Sergey Senozhatsky]

On (21/03/17 18:18), Sergey Senozhatsky wrote:
> On (21/02/08 14:17), Sergey Senozhatsky wrote:
> > This patch adds support for Region of Interest bmAutoControls.
> > 
> > ROI control is a compound data type:
> >   Control Selector     CT_REGION_OF_INTEREST_CONTROL
> >   Mandatory Requests   SET_CUR, GET_CUR, GET_MIN, GET_MAX, GET_DEF
> >   wLength 10
> >   Offset   Field            Size
> >   0        wROI_Top         2
> >   2        wROI_Left        2
> >   4        wROI_Bottom      2
> >   6        wROI_Right       2
> >   8        bmAutoControls   2       (Bitmap)
> > 
> > uvc_control_mapping, however, can handle only s32 data type at the
> > moment: ->get() returns s32 value, ->set() accepts s32 value; while
> > v4l2_ctrl maximum/minimum/default_value can hold only s64 values.
> > 
> > Hence ROI control handling is split into two patches:
> > a) bmAutoControls is handled via uvc_control_mapping as V4L2_CTRL_TYPE_MENU
> > b) ROI rectangle (SET_CUR, GET_CUR, GET_DEF) handling is implemented
> >    separately, by the means of selection API.
> 
> This approach is "no go".
> 
> I just figured out (am still debugging tho) that this patch set works on
> some devices and doesn't work on other. The root cause seems to be the
> fact that some firmwares error out all ROI requests when sizeof() of the
> ROI data is not 5 * __u16.
> 
> So those devices are not happy if we set/get ROI rectangle 4 * __u16
> and auto-controls __u16 separately; they want to set/get rect and
> rectanles in one shot.
> 
> This fixes ROI on those devices.
> 
> ---
> 
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1190,6 +1190,7 @@ struct uvc_roi_rect {
>         __u16                   left;
>         __u16                   bottom;
>         __u16                   right;
> +       __u16                   auto_controls;
>  };

Maybe I can drop the separate auto-control and just use v4l2_selection::flags
to set the roi auto-controls value.

[PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

From: Sergey Senozhatsky <senozhatsky@chromium.org>

This patch implements parts of UVC 1.5 Region of Interest (ROI)
control, using the uvcvideo selection API.

There are several things to mention here.

First, UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL; and 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.) This is not entirely clear if we need to implement
CT_DIGITAL_WINDOW_CONTROL. ROI is naturally limited by: ROI GET_MIN
and GET_MAX rectangles. Besides, the H/W that I'm playing with
implements ROI, but doesn't implement CT_DIGITAL_WINDOW_CONTROL,
so WINDOW_CONTROL is probably optional.

Second, the patch doesn't implement all of the ROI requests.
Namely, SEL_TGT_BOUNDS for ROI implements GET_MAX (that is maximal
ROI rectangle area). GET_MIN is not implemented (as of now) since
it's not very clear if user space would need such information.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 143 ++++++++++++++++++++++++++++++-
 1 file changed, 140 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 252136cc885c..71b4577196e5 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1139,14 +1139,60 @@ 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;
+};
+
+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_DEFAULT:
+		query = UVC_GET_DEF;
+		break;
+	case V4L2_SEL_TGT_ROI_CURRENT:
+		query = UVC_GET_CUR;
+		break;
+	case V4L2_SEL_TGT_ROI_BOUNDS:
+		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) {
+		sel->r.left	= roi->left;
+		sel->r.top	= roi->top;
+		sel->r.width	= roi->right;
+		sel->r.height	= roi->bottom;
+	}
+
+	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 +1219,96 @@ 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_CURRENT:
+	case V4L2_SEL_TGT_ROI_DEFAULT:
+	case V4L2_SEL_TGT_ROI_BOUNDS:
+		return uvc_ioctl_g_roi_target(file, fh, sel);
+	}
+
+	return -EINVAL;
+}
+
+static bool validate_roi_bounds(struct uvc_streaming *stream,
+				struct v4l2_selection *sel)
+{
+	bool ok = true;
+
+	if (sel->r.left > USHRT_MAX || sel->r.top > USHRT_MAX ||
+	    sel->r.width > USHRT_MAX || sel->r.height > USHRT_MAX)
+		return false;
+
+	/* perhaps also can test against ROI GET_MAX */
+
+	mutex_lock(&stream->mutex);
+	if ((u16)sel->r.width > stream->cur_frame->wWidth)
+		ok = false;
+	if ((u16)sel->r.height > stream->cur_frame->wHeight)
+		ok = false;
+	mutex_unlock(&stream->mutex);
+
+	return ok;
+}
+
+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	= sel->r.left;
+	roi->top	= sel->r.top;
+	roi->right	= sel->r.width;
+	roi->bottom	= sel->r.height;
+
+	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 +1669,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,
-- 
2.30.0

Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Ricardo Ribalda Delgado]

Hi Sergey

Thanks for the patch
On Mon, Feb 8, 2021 at 6:23 AM Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
>
> From: Sergey Senozhatsky <senozhatsky@chromium.org>
>
> This patch implements parts of UVC 1.5 Region of Interest (ROI)
> control, using the uvcvideo selection API.
>
> There are several things to mention here.
>
> First, UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL; and 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.) This is not entirely clear if we need to implement
> CT_DIGITAL_WINDOW_CONTROL. ROI is naturally limited by: ROI GET_MIN
> and GET_MAX rectangles. Besides, the H/W that I'm playing with
> implements ROI, but doesn't implement CT_DIGITAL_WINDOW_CONTROL,
> so WINDOW_CONTROL is probably optional.
>
> Second, the patch doesn't implement all of the ROI requests.
> Namely, SEL_TGT_BOUNDS for ROI implements GET_MAX (that is maximal
> ROI rectangle area). GET_MIN is not implemented (as of now) since
> it's not very clear if user space would need such information.
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 143 ++++++++++++++++++++++++++++++-
>  1 file changed, 140 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 252136cc885c..71b4577196e5 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1139,14 +1139,60 @@ 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;
> +};

Perhaps __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_DEFAULT:
> +               query = UVC_GET_DEF;
> +               break;
> +       case V4L2_SEL_TGT_ROI_CURRENT:
> +               query = UVC_GET_CUR;
> +               break;
> +       case V4L2_SEL_TGT_ROI_BOUNDS:
> +               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));

It is a pity that we have to alloc memory for this  :(.

@Laurent, do you know a better way?

> +       if (!ret) {
> +               sel->r.left     = roi->left;
> +               sel->r.top      = roi->top;
> +               sel->r.width    = roi->right;
> +               sel->r.height   = roi->bottom;
> +       }
> +
> +       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 +1219,96 @@ 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_CURRENT:
> +       case V4L2_SEL_TGT_ROI_DEFAULT:
> +       case V4L2_SEL_TGT_ROI_BOUNDS:
> +               return uvc_ioctl_g_roi_target(file, fh, sel);
> +       }
> +
> +       return -EINVAL;
> +}

Are you sure that there is no lock needed between the control and the
selection API?

> +
> +static bool validate_roi_bounds(struct uvc_streaming *stream,
> +                               struct v4l2_selection *sel)
> +{
> +       bool ok = true;
> +
> +       if (sel->r.left > USHRT_MAX || sel->r.top > USHRT_MAX ||
> +           sel->r.width > USHRT_MAX || sel->r.height > USHRT_MAX)
> +               return false;
> +
> +       /* perhaps also can test against ROI GET_MAX */
> +
> +       mutex_lock(&stream->mutex);
Maybe you should not release this mutex until query_ctrl is done?

> +       if ((u16)sel->r.width > stream->cur_frame->wWidth)
> +               ok = false;
> +       if ((u16)sel->r.height > stream->cur_frame->wHeight)
> +               ok = false;
> +       mutex_unlock(&stream->mutex);
> +
> +       return ok;
> +}
> +
> +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       = sel->r.left;
> +       roi->top        = sel->r.top;
> +       roi->right      = sel->r.width;
> +       roi->bottom     = sel->r.height;
> +
> +       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));

I think you need to read back from the device the actual value

https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection
On success the struct v4l2_rect r field contains the adjusted
rectangle. When the parameters are unsuitable the application may
modify the cropping (composing) or image parameters and repeat the
cycle until satisfactory parameters have been negotiated. If
constraints flags have to be violated at then ERANGE is returned. The
error indicates that there exist no rectangle that satisfies the
constraints.


> +
> +       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 +1669,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,
> --
> 2.30.0
>


-- 
Ricardo Ribalda

Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

On (21/03/16 19:46), Ricardo Ribalda Delgado wrote:
> > -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;
> > +};
> 
> Perhaps __packed; ?

Perhaps.

> > +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_CURRENT:
> > +       case V4L2_SEL_TGT_ROI_DEFAULT:
> > +       case V4L2_SEL_TGT_ROI_BOUNDS:
> > +               return uvc_ioctl_g_roi_target(file, fh, sel);
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> 
> Are you sure that there is no lock needed between the control and the
> selection API?

Between V4L2_CID_REGION_OF_INTEREST_AUTO and this path?
Hmm. They write to different offsets and don't seem to be overlapping.

> > +static bool validate_roi_bounds(struct uvc_streaming *stream,
> > +                               struct v4l2_selection *sel)
> > +{
> > +       bool ok = true;
> > +
> > +       if (sel->r.left > USHRT_MAX || sel->r.top > USHRT_MAX ||
> > +           sel->r.width > USHRT_MAX || sel->r.height > USHRT_MAX)
> > +               return false;
> > +
> > +       /* perhaps also can test against ROI GET_MAX */
> > +
> > +       mutex_lock(&stream->mutex);
[...]
> > +       if ((u16)sel->r.width > stream->cur_frame->wWidth)
> > +               ok = false;
> > +       if ((u16)sel->r.height > stream->cur_frame->wHeight)
> > +               ok = false;

> Maybe you should not release this mutex until query_ctrl is done?

Maybe... These two tests are somewhat made up. Not sure if we need them.
On one hand it's reasonable to keep ROI within the frames. On the other
hand - such relation is not spelled out in the spec. If the stream change
its frame dimensions ROI is not getting invalidated or re-calculated by
the firmware. UVC spec says that ROI should be bounded only by the
CT_DIGITAL_WINDOW_CONTROL (GET_MIN / GET_MAX), ut we don't implement
WINDOW_CONTROL.

So maybe I can remove stream->cuf_frame boundaries check instead.

> > +       mutex_unlock(&stream->mutex);
> > +
> > +       return ok;
> > +}
> > +
> > +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       = sel->r.left;
> > +       roi->top        = sel->r.top;
> > +       roi->right      = sel->r.width;
> > +       roi->bottom     = sel->r.height;
> > +
> > +       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));
> 
> I think you need to read back from the device the actual value

GET_CUR?

> https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection
> On success the struct v4l2_rect r field contains the adjusted
> rectangle.

What is the adjusted rectangle here? Does this mean that firmware can
successfully apply SET_CUR and return 0, but in reality it was not happy
with the rectangle dimensions so it modified it behind the scenes?

I can add GET_CUR I guess, but do we want to double the traffic, given
that ROI SET_CUR can be executed relatively often?

Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Ricardo Ribalda Delgado]

Hi

On Wed, Mar 17, 2021 at 2:59 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (21/03/16 19:46), Ricardo Ribalda Delgado wrote:
> > > -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;
> > > +};
> >
> > Perhaps __packed; ?
>
> Perhaps.
>
> > > +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_CURRENT:
> > > +       case V4L2_SEL_TGT_ROI_DEFAULT:
> > > +       case V4L2_SEL_TGT_ROI_BOUNDS:
> > > +               return uvc_ioctl_g_roi_target(file, fh, sel);
> > > +       }
> > > +
> > > +       return -EINVAL;
> > > +}
> >
> > Are you sure that there is no lock needed between the control and the
> > selection API?
>
> Between V4L2_CID_REGION_OF_INTEREST_AUTO and this path?
> Hmm. They write to different offsets and don't seem to be overlapping.
>
> > > +static bool validate_roi_bounds(struct uvc_streaming *stream,
> > > +                               struct v4l2_selection *sel)
> > > +{
> > > +       bool ok = true;
> > > +
> > > +       if (sel->r.left > USHRT_MAX || sel->r.top > USHRT_MAX ||
> > > +           sel->r.width > USHRT_MAX || sel->r.height > USHRT_MAX)
> > > +               return false;
> > > +
> > > +       /* perhaps also can test against ROI GET_MAX */
> > > +
> > > +       mutex_lock(&stream->mutex);
> [...]
> > > +       if ((u16)sel->r.width > stream->cur_frame->wWidth)
> > > +               ok = false;
> > > +       if ((u16)sel->r.height > stream->cur_frame->wHeight)
> > > +               ok = false;
>
> > Maybe you should not release this mutex until query_ctrl is done?
>
> Maybe... These two tests are somewhat made up. Not sure if we need them.
> On one hand it's reasonable to keep ROI within the frames. On the other
> hand - such relation is not spelled out in the spec. If the stream change
> its frame dimensions ROI is not getting invalidated or re-calculated by
> the firmware. UVC spec says that ROI should be bounded only by the
> CT_DIGITAL_WINDOW_CONTROL (GET_MIN / GET_MAX), ut we don't implement
> WINDOW_CONTROL.

I would remove this check completely then, and rely on set_cur,
get_cur. Leave only the < 0x10000 check

>
> So maybe I can remove stream->cuf_frame boundaries check instead.
>
> > > +       mutex_unlock(&stream->mutex);
> > > +
> > > +       return ok;
> > > +}
> > > +
> > > +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       = sel->r.left;
> > > +       roi->top        = sel->r.top;
> > > +       roi->right      = sel->r.width;
> > > +       roi->bottom     = sel->r.height;
> > > +
> > > +       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));
> >
> > I think you need to read back from the device the actual value
>
> GET_CUR?
yep

>
> > https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection
> > On success the struct v4l2_rect r field contains the adjusted
> > rectangle.
>
> What is the adjusted rectangle here? Does this mean that firmware can
> successfully apply SET_CUR and return 0, but in reality it was not happy
> with the rectangle dimensions so it modified it behind the scenes?

I can imagine that some hw might have spooky requirements for the roi
rectangle (multiple of 4, not crossing the bayer filter, odd/even
line...) so they might be able to go the closest valid config.


>
> I can add GET_CUR I guess, but do we want to double the traffic, given
> that ROI SET_CUR can be executed relatively often?



--
Ricardo Ribalda

Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

On (21/03/17 08:58), Ricardo Ribalda Delgado wrote:
[..]
> >
> > GET_CUR?
> yep
> 
> >
> > > https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection
> > > On success the struct v4l2_rect r field contains the adjusted
> > > rectangle.
> >
> > What is the adjusted rectangle here? Does this mean that firmware can
> > successfully apply SET_CUR and return 0, but in reality it was not happy
> > with the rectangle dimensions so it modified it behind the scenes?
> 
> I can imagine that some hw might have spooky requirements for the roi
> rectangle (multiple of 4, not crossing the bayer filter, odd/even
> line...) so they might be able to go the closest valid config.

Hmm. Honestly, I'm very unsure about it. ROI::SET_CUR can be a very
hot path, depending on what user-space considers to be of interest
and how frequently that object of interest changes its position/shape/etc.
Doing GET_CUR after every SET_CUR doubles the number of firmware calls
we issue, that's for sure; is it worth it - that's something that I'm
not sure of.

May I please ask for more opinions on this?

	-ss

Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Ricardo Ribalda]

Hi Sergey

On Thu, Mar 18, 2021 at 5:47 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (21/03/17 08:58), Ricardo Ribalda Delgado wrote:
> [..]
> > >
> > > GET_CUR?
> > yep
> >
> > >
> > > > https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection
> > > > On success the struct v4l2_rect r field contains the adjusted
> > > > rectangle.
> > >
> > > What is the adjusted rectangle here? Does this mean that firmware can
> > > successfully apply SET_CUR and return 0, but in reality it was not happy
> > > with the rectangle dimensions so it modified it behind the scenes?
> >
> > I can imagine that some hw might have spooky requirements for the roi
> > rectangle (multiple of 4, not crossing the bayer filter, odd/even
> > line...) so they might be able to go the closest valid config.
>
> Hmm. Honestly, I'm very unsure about it. ROI::SET_CUR can be a very
> hot path, depending on what user-space considers to be of interest
> and how frequently that object of interest changes its position/shape/etc.
> Doing GET_CUR after every SET_CUR doubles the number of firmware calls
> we issue, that's for sure; is it worth it - that's something that I'm
> not sure of.
>
> May I please ask for more opinions on this?

Could you try setting the roi in a loop in your device and verify that
it accepts all the values with no modification. If so we can implement
the set/get as a quirk for other devices.

>
>         -ss



-- 
Ricardo Ribalda

Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Ricardo Ribalda]

On Thu, Mar 18, 2021 at 10:19 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Sergey
>
> On Thu, Mar 18, 2021 at 5:47 AM Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> >
> > On (21/03/17 08:58), Ricardo Ribalda Delgado wrote:
> > [..]
> > > >
> > > > GET_CUR?
> > > yep
> > >
> > > >
> > > > > https://www.kernel.org/doc/html/v4.13/media/uapi/v4l/vidioc-g-selection.html?highlight=vidioc_s_selection
> > > > > On success the struct v4l2_rect r field contains the adjusted
> > > > > rectangle.
> > > >
> > > > What is the adjusted rectangle here? Does this mean that firmware can
> > > > successfully apply SET_CUR and return 0, but in reality it was not happy
> > > > with the rectangle dimensions so it modified it behind the scenes?
> > >
> > > I can imagine that some hw might have spooky requirements for the roi
> > > rectangle (multiple of 4, not crossing the bayer filter, odd/even
> > > line...) so they might be able to go the closest valid config.
> >
> > Hmm. Honestly, I'm very unsure about it. ROI::SET_CUR can be a very
> > hot path, depending on what user-space considers to be of interest
> > and how frequently that object of interest changes its position/shape/etc.
> > Doing GET_CUR after every SET_CUR doubles the number of firmware calls
> > we issue, that's for sure; is it worth it - that's something that I'm
> > not sure of.
> >
> > May I please ask for more opinions on this?
>
> Could you try setting the roi in a loop in your device and verify that
> it accepts all the values with no modification. If so we can implement
> the set/get as a quirk for other devices.

as a loop I mean testing all the values not the same value again-and-again
;)


>
> >
> >         -ss
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Sergey Senozhatsky]

On (21/03/18 22:19), Ricardo Ribalda wrote:
> >
> > May I please ask for more opinions on this?
> 
> Could you try setting the roi in a loop in your device and verify that
> it accepts all the values with no modification. If so we can implement
> the set/get as a quirk for other devices.

Tested on two (very) different devices.

Firmware D:

   CLAP all passed, we are cool

Firmware H:

   CLAP all passed, we are cool


Code sample

---
       in_selection.target     = V4L2_SEL_TGT_ROI;
       in_selection.flags      = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;

       for (int i = 0; i < 1001; i++) {
               in_selection.r.left     = 0 + i;
               in_selection.r.top      = 0 + i;
               in_selection.r.width    = 42 + i;
               in_selection.r.height   = 42 + i;

               ret = doioctl(fd, VIDIOC_S_SELECTION, &in_selection);
               if (ret) {
                       fprintf(stderr, "BOOM %d\n", ret);
                       exit(1);
               }

               ret = doioctl(fd, VIDIOC_G_SELECTION, &in_selection);
               if (ret) {
                       fprintf(stderr, "BANG %d\n", ret);
                       exit(2);
               }

               if (in_selection.r.left != i ||
                   in_selection.r.top != i ||
                   in_selection.r.width != i + 42 ||
                   in_selection.r.height != i + 42) {

                       fprintf(stderr, "SNAP %d %d %d %d != %d %d %d %d\n",
                               i, i, i + 42, i + 42,
                               in_selection.r.left,
                               in_selection.r.top,
                               in_selection.r.width,
                               in_selection.r.height);
                       exit(3);
               }
       }

       fprintf(stderr, "CLAP all passed, we are cool\n");
---

Re: [PATCHv2 3/3] media: uvcvideo: add UVC 1.5 ROI control

[Ricardo Ribalda]

Hi Sergey

On Fri, Mar 19, 2021 at 6:35 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (21/03/18 22:19), Ricardo Ribalda wrote:
> > >
> > > May I please ask for more opinions on this?
> >
> > Could you try setting the roi in a loop in your device and verify that
> > it accepts all the values with no modification. If so we can implement
> > the set/get as a quirk for other devices.
>
> Tested on two (very) different devices.

Ahoy, Matey ;)

That is great news. Thanks for testing this.


>
> Firmware D:
>
>    CLAP all passed, we are cool
>
> Firmware H:
>
>    CLAP all passed, we are cool
>
>
> Code sample
>
> ---
>        in_selection.target     = V4L2_SEL_TGT_ROI;
>        in_selection.flags      = V4L2_SEL_FLAG_ROI_AUTO_EXPOSURE;
>
>        for (int i = 0; i < 1001; i++) {
>                in_selection.r.left     = 0 + i;
>                in_selection.r.top      = 0 + i;
>                in_selection.r.width    = 42 + i;
>                in_selection.r.height   = 42 + i;
>
>                ret = doioctl(fd, VIDIOC_S_SELECTION, &in_selection);
>                if (ret) {
>                        fprintf(stderr, "BOOM %d\n", ret);
>                        exit(1);
>                }
>
>                ret = doioctl(fd, VIDIOC_G_SELECTION, &in_selection);
>                if (ret) {
>                        fprintf(stderr, "BANG %d\n", ret);
>                        exit(2);
>                }
>
>                if (in_selection.r.left != i ||
>                    in_selection.r.top != i ||
>                    in_selection.r.width != i + 42 ||
>                    in_selection.r.height != i + 42) {
>
>                        fprintf(stderr, "SNAP %d %d %d %d != %d %d %d %d\n",
>                                i, i, i + 42, i + 42,
>                                in_selection.r.left,
>                                in_selection.r.top,
>                                in_selection.r.width,
>                                in_selection.r.height);
>                        exit(3);
>                }
>        }
>
>        fprintf(stderr, "CLAP all passed, we are cool\n");
> ---



-- 
Ricardo Ribalda

Re: [PATCHv2 0/3] Add UVC 1.5 Region Of Interest control to uvcvideo

[Sergey Senozhatsky]

On (21/02/08 14:17), Sergey Senozhatsky wrote:
> 	Hello,
> 
> 	RFC

Hi Laurent,

Gentle ping.

	-ss