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

From: Ricardo Ribalda Delgado
Date: Tue Mar 16 2021 - 14:30:14 EST


Hi Sergey

Thanks for the patch

On Mon, Feb 8, 2021 at 6:24 AM Sergey Senozhatsky
<sergey.senozhatsky@xxxxxxxxx> wrote:
>
> From: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> .../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