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

[Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>]

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