[v7,7/7] media: uvcvideo: document UVC v1.5 ROI

  Added documentation of V4L2_CID_UVC_REGION_OF_INTEREST_RECT and
V4L2_CID_UVC_REGION_OF_INTEREST_AUTO.

Signed-off-by: Yunke Cao <yunkec@google.com>
---
 .../userspace-api/media/drivers/uvcvideo.rst  | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
  

On Tue, 28 Jun 2022 at 09:58, Yunke Cao <yunkec@google.com> wrote:
>
> Added documentation of V4L2_CID_UVC_REGION_OF_INTEREST_RECT and
> V4L2_CID_UVC_REGION_OF_INTEREST_AUTO.
>
> Signed-off-by: Yunke Cao <yunkec@google.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  .../userspace-api/media/drivers/uvcvideo.rst  | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/drivers/uvcvideo.rst b/Documentation/userspace-api/media/drivers/uvcvideo.rst
> index a290f9fadae9..ee4c182aa274 100644
> --- a/Documentation/userspace-api/media/drivers/uvcvideo.rst
> +++ b/Documentation/userspace-api/media/drivers/uvcvideo.rst
> @@ -181,6 +181,7 @@ Argument: struct uvc_xu_control_mapping
>         UVC_CTRL_DATA_TYPE_BOOLEAN      Boolean
>         UVC_CTRL_DATA_TYPE_ENUM         Enumeration
>         UVC_CTRL_DATA_TYPE_BITMASK      Bitmask
> +       UVC_CTRL_DATA_TYPE_RECT         Rectangular area
>
>
>  UVCIOC_CTRL_QUERY - Query a UVC XU control
> @@ -255,3 +256,63 @@ Argument: struct uvc_xu_control_query
>         __u8    query           Request code to send to the device
>         __u16   size            Control data size (in bytes)
>         __u8    *data           Control value
> +
> +Private V4L2 controls
> +---------------------
> +
> +A few UVC specific V4L2 control IDs are listed below.
> +
> +``V4L2_CID_UVC_REGION_OF_INTEREST_RECT (struct)``
> +       This control determines the region of interest (ROI). ROI is an
> +       rectangular area represented by a struct :c:type:`v4l2_rect`. The
> +       rectangle is in global sensor coordinates and pixel units. It is
> +       independent of the field of view, not impacted by any cropping or
> +       scaling.
> +
> +       Use ``V4L2_CTRL_WHICH_MIN_VAL`` and ``V4L2_CTRL_WHICH_MAX_VAL`` to query
> +       the range of rectangle sizes. For example, a device can have a minimum
> +       ROI rectangle of 1x1@0x0 and a maximum of 640x480@0x0.
> +
> +       Setting a ROI allows the camera to optimize the capture for the region.
> +       The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control determines
> +       the detailed behavior.
> +
> +
> +``V4L2_CID_UVC_REGION_OF_INTEREST_AUTO (bitmask)``
> +       This determines which, if any, on board features should track to the
> +       Region of Interest specified by the current value of
> +       ``V4L2_CID_UVD__REGION_OF_INTEREST_RECT``.
> +
> +       Max value is a mask indicating all supported Auto
> +       Controls.
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_EXPOSURE``
> +      - Setting this to true enables automatic exposure time for the specified
> +       region.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_IRIS``
> +      - Setting this to true enables automatic iris aperture for the specified
> +       region.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> +      - Setting this to true enables automatic white balance adjustment for the
> +       specified region.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_FOCUS``
> +      - Setting this to true enables automatic focus adjustment for the
> +       specified region.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> +      - Setting this to true enables automatic face detection for the
> +       specified region.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> +      - Setting this to true enables automatic face detection and tracking. The
> +       current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> +       the driver.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION``
> +      - Setting this to true enables automatic image stabilization. The
> +       current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> +       the driver.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> +      - Setting this to true enables automatically capture the specified region
> +       with higher quality if possible.
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
  

Hi Yunke,

Thank you for the patch.

On Tue, Jun 28, 2022 at 04:57:05PM +0900, Yunke Cao wrote:
> Added documentation of V4L2_CID_UVC_REGION_OF_INTEREST_RECT and
> V4L2_CID_UVC_REGION_OF_INTEREST_AUTO.
> 
> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
>  .../userspace-api/media/drivers/uvcvideo.rst  | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/drivers/uvcvideo.rst b/Documentation/userspace-api/media/drivers/uvcvideo.rst
> index a290f9fadae9..ee4c182aa274 100644
> --- a/Documentation/userspace-api/media/drivers/uvcvideo.rst
> +++ b/Documentation/userspace-api/media/drivers/uvcvideo.rst
> @@ -181,6 +181,7 @@ Argument: struct uvc_xu_control_mapping
>  	UVC_CTRL_DATA_TYPE_BOOLEAN	Boolean
>  	UVC_CTRL_DATA_TYPE_ENUM		Enumeration
>  	UVC_CTRL_DATA_TYPE_BITMASK	Bitmask
> +	UVC_CTRL_DATA_TYPE_RECT		Rectangular area
>  
>  
>  UVCIOC_CTRL_QUERY - Query a UVC XU control
> @@ -255,3 +256,63 @@ Argument: struct uvc_xu_control_query
>  	__u8	query		Request code to send to the device
>  	__u16	size		Control data size (in bytes)
>  	__u8	*data		Control value
> +

You can add a second blank line here, as above the "Extension Unit (XU)
support" title.

> +Private V4L2 controls

"Driver-specific V4L2 controls" would be better, those controls are not
private.

> +---------------------
> +
> +A few UVC specific V4L2 control IDs are listed below.

s/UVC specific/UVC-specific/

But I'd write

The uvcvideo driver implements the following UVC-specific controls:

> +
> +``V4L2_CID_UVC_REGION_OF_INTEREST_RECT (struct)``
> +	This control determines the region of interest (ROI). ROI is an

s/is an/is a/

> +	rectangular area represented by a struct :c:type:`v4l2_rect`. The
> +	rectangle is in global sensor coordinates and pixel units. It is
> +	independent of the field of view, not impacted by any cropping or
> +	scaling.
> +
> +	Use ``V4L2_CTRL_WHICH_MIN_VAL`` and ``V4L2_CTRL_WHICH_MAX_VAL`` to query
> +	the range of rectangle sizes. For example, a device can have a minimum
> +	ROI rectangle of 1x1@0x0 and a maximum of 640x480@0x0.

Minimum and maximum values for rectangles are ill-defined. Are the top
and left coordinates always 0 ? If so that should be documented.

> +
> +	Setting a ROI allows the camera to optimize the capture for the region.
> +	The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control determines
> +	the detailed behavior.
> +
> +
> +``V4L2_CID_UVC_REGION_OF_INTEREST_AUTO (bitmask)``
> +	This determines which, if any, on board features should track to the
> +	Region of Interest specified by the current value of
> +	``V4L2_CID_UVD__REGION_OF_INTEREST_RECT``.
> +
> +	Max value is a mask indicating all supported Auto
> +	Controls.

No need to wrap this line.

> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_EXPOSURE``

s/V4L2/V4L2_UVC/

Same below.

> +      - Setting this to true enables automatic exposure time for the specified

s/exposure time/exposure/ (as it can also include gain)

> +	region.

It's not that it enables automatic exposure for the specified region, is
that it causes the automatic exposure to track the region of interest
instead of the whole image. Same below.

> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_IRIS``
> +      - Setting this to true enables automatic iris aperture for the specified
> +	region.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> +      - Setting this to true enables automatic white balance adjustment for the
> +	specified region.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_FOCUS``
> +      - Setting this to true enables automatic focus adjustment for the
> +	specified region.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> +      - Setting this to true enables automatic face detection for the
> +	specified region.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> +      - Setting this to true enables automatic face detection and tracking. The
> +	current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> +	the driver.
> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION``
> +      - Setting this to true enables automatic image stabilization. The
> +	current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> +	the driver.

How so ?

> +    * - ``V4L2_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> +      - Setting this to true enables automatically capture the specified region
> +	with higher quality if possible.

Could you please clarify this one ? I'm not sure to understand what it
means.
  

Hi Laurent,

Thanks for the review!

On Thu, Aug 25, 2022 at 5:17 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Yunke,
>
> Thank you for the patch.
>
> On Tue, Jun 28, 2022 at 04:57:05PM +0900, Yunke Cao wrote:
> > Added documentation of V4L2_CID_UVC_REGION_OF_INTEREST_RECT and
> > V4L2_CID_UVC_REGION_OF_INTEREST_AUTO.
> >
> > Signed-off-by: Yunke Cao <yunkec@google.com>
> > ---
> >  .../userspace-api/media/drivers/uvcvideo.rst  | 61 +++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/drivers/uvcvideo.rst b/Documentation/userspace-api/media/drivers/uvcvideo.rst
> > index a290f9fadae9..ee4c182aa274 100644
> > --- a/Documentation/userspace-api/media/drivers/uvcvideo.rst
> > +++ b/Documentation/userspace-api/media/drivers/uvcvideo.rst
> > @@ -181,6 +181,7 @@ Argument: struct uvc_xu_control_mapping
> >       UVC_CTRL_DATA_TYPE_BOOLEAN      Boolean
> >       UVC_CTRL_DATA_TYPE_ENUM         Enumeration
> >       UVC_CTRL_DATA_TYPE_BITMASK      Bitmask
> > +     UVC_CTRL_DATA_TYPE_RECT         Rectangular area
> >
> >
> >  UVCIOC_CTRL_QUERY - Query a UVC XU control
> > @@ -255,3 +256,63 @@ Argument: struct uvc_xu_control_query
> >       __u8    query           Request code to send to the device
> >       __u16   size            Control data size (in bytes)
> >       __u8    *data           Control value
> > +
>
> You can add a second blank line here, as above the "Extension Unit (XU)
> support" title.
>
> > +Private V4L2 controls
>
> "Driver-specific V4L2 controls" would be better, those controls are not
> private.
>
> > +---------------------
> > +
> > +A few UVC specific V4L2 control IDs are listed below.
>
> s/UVC specific/UVC-specific/
>
> But I'd write
>
> The uvcvideo driver implements the following UVC-specific controls:
>
> > +
> > +``V4L2_CID_UVC_REGION_OF_INTEREST_RECT (struct)``
> > +     This control determines the region of interest (ROI). ROI is an
>
> s/is an/is a/
>
> > +     rectangular area represented by a struct :c:type:`v4l2_rect`. The
> > +     rectangle is in global sensor coordinates and pixel units. It is
> > +     independent of the field of view, not impacted by any cropping or
> > +     scaling.
> > +
> > +     Use ``V4L2_CTRL_WHICH_MIN_VAL`` and ``V4L2_CTRL_WHICH_MAX_VAL`` to query
> > +     the range of rectangle sizes. For example, a device can have a minimum
> > +     ROI rectangle of 1x1@0x0 and a maximum of 640x480@0x0.
>
> Minimum and maximum values for rectangles are ill-defined. Are the top
> and left coordinates always 0 ? If so that should be documented.
>
> > +
> > +     Setting a ROI allows the camera to optimize the capture for the region.
> > +     The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control determines
> > +     the detailed behavior.
> > +
> > +
> > +``V4L2_CID_UVC_REGION_OF_INTEREST_AUTO (bitmask)``
> > +     This determines which, if any, on board features should track to the
> > +     Region of Interest specified by the current value of
> > +     ``V4L2_CID_UVD__REGION_OF_INTEREST_RECT``.
> > +
> > +     Max value is a mask indicating all supported Auto
> > +     Controls.
>
> No need to wrap this line.
>
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +
> > +    * - ``V4L2_REGION_OF_INTEREST_AUTO_EXPOSURE``
>
> s/V4L2/V4L2_UVC/
>
> Same below.
>
> > +      - Setting this to true enables automatic exposure time for the specified
>
> s/exposure time/exposure/ (as it can also include gain)
>
> > +     region.
>
> It's not that it enables automatic exposure for the specified region, is
> that it causes the automatic exposure to track the region of interest
> instead of the whole image. Same below.
>

I see. Thanks for clarifying. I will update this and the comments above in the
next version.

> > +    * - ``V4L2_REGION_OF_INTEREST_AUTO_IRIS``
> > +      - Setting this to true enables automatic iris aperture for the specified
> > +     region.
> > +    * - ``V4L2_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> > +      - Setting this to true enables automatic white balance adjustment for the
> > +     specified region.
> > +    * - ``V4L2_REGION_OF_INTEREST_AUTO_FOCUS``
> > +      - Setting this to true enables automatic focus adjustment for the
> > +     specified region.
> > +    * - ``V4L2_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> > +      - Setting this to true enables automatic face detection for the
> > +     specified region.
> > +    * - ``V4L2_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> > +      - Setting this to true enables automatic face detection and tracking. The
> > +     current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> > +     the driver.
> > +    * - ``V4L2_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION``
> > +      - Setting this to true enables automatic image stabilization. The
> > +     current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> > +     the driver.
>
> How so ?


If I read the UVC specification correctly, the ROI rectangles are
volatile when these two
flags are set.
"This RoI should be the same as specified in most recent SET_CUR except in the
case where the ‘Auto Detect and Track’ and/or ‘Image Stabilization’
bit have been set."
(4.2.2.1.20 Digital Region of Interest (ROI) Control)

Maybe it's better to say "updated by the hardware" instead of driver?

>
>
> > +    * - ``V4L2_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> > +      - Setting this to true enables automatically capture the specified region
> > +     with higher quality if possible.
>
> Could you please clarify this one ? I'm not sure to understand what it
> means.

Sorry, it's not very clear to me either... I just added it because there's a
"Higher Quality" in the specification.

Best,
Yunke

>
> --
> Regards,
>
> Laurent Pinchart