* [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control
2022-05-18 6:24 [PATCH v3 0/6] media: Implement UVC v1.5 ROI Yunke Cao
@ 2022-05-18 6:24 ` Yunke Cao
2022-05-19 7:49 ` Hans Verkuil
2022-05-19 8:14 ` Hans Verkuil
2022-05-18 6:24 ` [PATCH v3 2/6] media: v4l2_ctrl: Add region of interest auto control Yunke Cao
` (5 subsequent siblings)
6 siblings, 2 replies; 16+ messages in thread
From: Yunke Cao @ 2022-05-18 6:24 UTC (permalink / raw)
To: Hans Verkuil, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media, Yunke Cao
Including:
1. Add a control ID.
2. Add p_rect to struct v4l2_ext_control with basic support in
v4l2-ctrls.
Signed-off-by: Yunke Cao <yunkec@google.com>
---
Changelog since v2:
- Better documentation.
.../media/v4l/ext-ctrls-camera.rst | 10 ++++++++++
.../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++
.../media/videodev2.h.rst.exceptions | 1 +
drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 ++++
include/media/v4l2-ctrls.h | 2 ++
include/uapi/linux/v4l2-controls.h | 2 ++
include/uapi/linux/videodev2.h | 2 ++
8 files changed, 45 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index 4c5061aa9cd4..c988a72b97b2 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -661,3 +661,13 @@ enum v4l2_scene_mode -
.. [#f1]
This control may be changed to a menu control in the future, if more
options are required.
+
+``V4L2_CID_REGION_OF_INTEREST_RECT (struct)``
+ This control determines the region of interest. Region of interest is an
+ rectangular area represented by a struct v4l2_rect. The rectangle is in
+ pixel units and global coordinates. Use ``V4L2_CTRL_WHICH_MIN_VAL`` and
+ ``V4L2_CTRL_WHICH_MAX_VAL`` to query the range of coordinates.
+
+ Setting a region of interest allows the camera to optimize the capture for
+ the region. The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control
+ determines the detailed behavior.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
index 29971a45a2d4..f4e205ead0a2 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
@@ -189,6 +189,10 @@ still cause this situation.
- ``p_area``
- A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
of type ``V4L2_CTRL_TYPE_AREA``.
+ * - struct :c:type:`v4l2_rect` *
+ - ``p_area``
+ - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is
+ of type ``V4L2_CTRL_TYPE_RECT``.
* - struct :c:type:`v4l2_ctrl_h264_sps` *
- ``p_h264_sps``
- A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index 9cbb7a0c354a..7b423475281d 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type`
replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type`
replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type`
replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type`
+replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type`
replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type`
replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type`
replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type`
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index 8968cec8454e..dcde405c2713 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
return ptr1.p_u16[idx] == ptr2.p_u16[idx];
case V4L2_CTRL_TYPE_U32:
return ptr1.p_u32[idx] == ptr2.p_u32[idx];
+ case V4L2_CTRL_TYPE_RECT:
+ return ptr1.p_rect->top == ptr2.p_rect->top &&
+ ptr1.p_rect->left == ptr2.p_rect->left &&
+ ptr1.p_rect->height == ptr2.p_rect->height &&
+ ptr1.p_rect->width == ptr2.p_rect->width;
default:
if (ctrl->is_int)
return ptr1.p_s32[idx] == ptr2.p_s32[idx];
@@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl)
case V4L2_CTRL_TYPE_VP9_FRAME:
pr_cont("VP9_FRAME");
break;
+ case V4L2_CTRL_TYPE_RECT:
+ pr_cont("l: %d, t: %d, w: %u, h: %u",
+ ptr.p_rect->left, ptr.p_rect->top,
+ ptr.p_rect->width, ptr.p_rect->height);
+ break;
default:
pr_cont("unknown type %d", ctrl->type);
break;
@@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
struct v4l2_area *area;
+ struct v4l2_rect *rect;
void *p = ptr.p + idx * ctrl->elem_size;
unsigned int i;
@@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
return -EINVAL;
break;
+ case V4L2_CTRL_TYPE_RECT:
+ rect = p;
+ if (!rect->width || !rect->height)
+ return -EINVAL;
+ break;
+
default:
return -EINVAL;
}
@@ -1456,6 +1473,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
case V4L2_CTRL_TYPE_AREA:
elem_size = sizeof(struct v4l2_area);
break;
+ case V4L2_CTRL_TYPE_RECT:
+ elem_size = sizeof(struct v4l2_rect);
+ break;
default:
if (type < V4L2_CTRL_COMPOUND_TYPES)
elem_size = sizeof(s32);
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 54ca4e6b820b..95f39a2d2ad2 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
+ case V4L2_CID_REGION_OF_INTEREST_RECT: return "Region Of Interest Rectangle";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1524,6 +1525,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
*type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
break;
+ case V4L2_CID_REGION_OF_INTEREST_RECT:
+ *type = V4L2_CTRL_TYPE_RECT;
+ break;
default:
*type = V4L2_CTRL_TYPE_INTEGER;
break;
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index b3ce438f1329..919e104de50b 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -58,6 +58,7 @@ struct video_device;
* @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure.
* @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure.
* @p_area: Pointer to an area.
+ * @p_rect: Pointer to a rectangle.
* @p: Pointer to a compound value.
* @p_const: Pointer to a constant compound value.
*/
@@ -87,6 +88,7 @@ union v4l2_ctrl_ptr {
struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll;
struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
struct v4l2_area *p_area;
+ struct v4l2_rect *p_rect;
void *p;
const void *p_const;
};
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index bb40129446d4..499fcddb6254 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
#define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35)
+#define V4L2_CID_REGION_OF_INTEREST_RECT (V4L2_CID_CAMERA_CLASS_BASE+36)
+
/* FM Modulator class control IDs */
#define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 3768a0a80830..b712412cf763 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1751,6 +1751,7 @@ struct v4l2_ext_control {
__u16 __user *p_u16;
__u32 __user *p_u32;
struct v4l2_area __user *p_area;
+ struct v4l2_rect __user *p_rect;
struct v4l2_ctrl_h264_sps __user *p_h264_sps;
struct v4l2_ctrl_h264_pps *p_h264_pps;
struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix;
@@ -1810,6 +1811,7 @@ enum v4l2_ctrl_type {
V4L2_CTRL_TYPE_U16 = 0x0101,
V4L2_CTRL_TYPE_U32 = 0x0102,
V4L2_CTRL_TYPE_AREA = 0x0106,
+ V4L2_CTRL_TYPE_RECT = 0x0107,
V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110,
V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111,
--
2.36.0.550.gb090851708-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control
2022-05-18 6:24 ` [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control Yunke Cao
@ 2022-05-19 7:49 ` Hans Verkuil
2022-05-19 8:14 ` Hans Verkuil
1 sibling, 0 replies; 16+ messages in thread
From: Hans Verkuil @ 2022-05-19 7:49 UTC (permalink / raw)
To: Yunke Cao, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media
On 5/18/22 08:24, Yunke Cao wrote:
> Including:
> 1. Add a control ID.
> 2. Add p_rect to struct v4l2_ext_control with basic support in
> v4l2-ctrls.
>
> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> Changelog since v2:
> - Better documentation.
>
> .../media/v4l/ext-ctrls-camera.rst | 10 ++++++++++
> .../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++
> .../media/videodev2.h.rst.exceptions | 1 +
> drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 ++++
> include/media/v4l2-ctrls.h | 2 ++
> include/uapi/linux/v4l2-controls.h | 2 ++
> include/uapi/linux/videodev2.h | 2 ++
> 8 files changed, 45 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 4c5061aa9cd4..c988a72b97b2 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -661,3 +661,13 @@ enum v4l2_scene_mode -
> .. [#f1]
> This control may be changed to a menu control in the future, if more
> options are required.
> +
> +``V4L2_CID_REGION_OF_INTEREST_RECT (struct)``
> + This control determines the region of interest. Region of interest is an
> + rectangular area represented by a struct v4l2_rect. The rectangle is in
> + pixel units and global coordinates. Use ``V4L2_CTRL_WHICH_MIN_VAL`` and
Hmm, what are 'global coordinates'?
Is a ROI always contained inside the captured image? What if that image was
cropped from a larger image? Is the ROI before or after scaling?
This needs to be more precise.
> + ``V4L2_CTRL_WHICH_MAX_VAL`` to query the range of coordinates.
> +
> + Setting a region of interest allows the camera to optimize the capture for
> + the region. The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control
> + determines the detailed behavior.
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index 29971a45a2d4..f4e205ead0a2 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -189,6 +189,10 @@ still cause this situation.
> - ``p_area``
> - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> of type ``V4L2_CTRL_TYPE_AREA``.
> + * - struct :c:type:`v4l2_rect` *
> + - ``p_area``
> + - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is
> + of type ``V4L2_CTRL_TYPE_RECT``.
> * - struct :c:type:`v4l2_ctrl_h264_sps` *
> - ``p_h264_sps``
> - A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index 9cbb7a0c354a..7b423475281d 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type`
> +replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type`
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index 8968cec8454e..dcde405c2713 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> return ptr1.p_u16[idx] == ptr2.p_u16[idx];
> case V4L2_CTRL_TYPE_U32:
> return ptr1.p_u32[idx] == ptr2.p_u32[idx];
> + case V4L2_CTRL_TYPE_RECT:
> + return ptr1.p_rect->top == ptr2.p_rect->top &&
> + ptr1.p_rect->left == ptr2.p_rect->left &&
> + ptr1.p_rect->height == ptr2.p_rect->height &&
> + ptr1.p_rect->width == ptr2.p_rect->width;
> default:
> if (ctrl->is_int)
> return ptr1.p_s32[idx] == ptr2.p_s32[idx];
> @@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> case V4L2_CTRL_TYPE_VP9_FRAME:
> pr_cont("VP9_FRAME");
> break;
> + case V4L2_CTRL_TYPE_RECT:
> + pr_cont("l: %d, t: %d, w: %u, h: %u",
I'd write this as:
pr_cont("%ux%u@%dx%d",
ptr.p_rect->width, ptr.p_rect->height,
ptr.p_rect->left, ptr.p_rect->top);
> + ptr.p_rect->left, ptr.p_rect->top,
> + ptr.p_rect->width, ptr.p_rect->height);
> + break;
> default:
> pr_cont("unknown type %d", ctrl->type);
> break;
> @@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
> struct v4l2_area *area;
> + struct v4l2_rect *rect;
> void *p = ptr.p + idx * ctrl->elem_size;
> unsigned int i;
>
> @@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> return -EINVAL;
> break;
>
> + case V4L2_CTRL_TYPE_RECT:
> + rect = p;
> + if (!rect->width || !rect->height)
> + return -EINVAL;
> + break;
> +
> default:
> return -EINVAL;
> }
> @@ -1456,6 +1473,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> case V4L2_CTRL_TYPE_AREA:
> elem_size = sizeof(struct v4l2_area);
> break;
> + case V4L2_CTRL_TYPE_RECT:
> + elem_size = sizeof(struct v4l2_rect);
> + break;
> default:
> if (type < V4L2_CTRL_COMPOUND_TYPES)
> elem_size = sizeof(s32);
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6b820b..95f39a2d2ad2 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
> case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
> case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
> + case V4L2_CID_REGION_OF_INTEREST_RECT: return "Region Of Interest Rectangle";
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1524,6 +1525,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> break;
> + case V4L2_CID_REGION_OF_INTEREST_RECT:
> + *type = V4L2_CTRL_TYPE_RECT;
> + break;
> default:
> *type = V4L2_CTRL_TYPE_INTEGER;
> break;
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index b3ce438f1329..919e104de50b 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -58,6 +58,7 @@ struct video_device;
> * @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure.
> * @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure.
> * @p_area: Pointer to an area.
> + * @p_rect: Pointer to a rectangle.
> * @p: Pointer to a compound value.
> * @p_const: Pointer to a constant compound value.
> */
> @@ -87,6 +88,7 @@ union v4l2_ctrl_ptr {
> struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll;
> struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> struct v4l2_area *p_area;
> + struct v4l2_rect *p_rect;
> void *p;
> const void *p_const;
> };
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index bb40129446d4..499fcddb6254 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
>
> #define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35)
>
> +#define V4L2_CID_REGION_OF_INTEREST_RECT (V4L2_CID_CAMERA_CLASS_BASE+36)
> +
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 3768a0a80830..b712412cf763 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1751,6 +1751,7 @@ struct v4l2_ext_control {
> __u16 __user *p_u16;
> __u32 __user *p_u32;
> struct v4l2_area __user *p_area;
> + struct v4l2_rect __user *p_rect;
> struct v4l2_ctrl_h264_sps __user *p_h264_sps;
> struct v4l2_ctrl_h264_pps *p_h264_pps;
> struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix;
> @@ -1810,6 +1811,7 @@ enum v4l2_ctrl_type {
> V4L2_CTRL_TYPE_U16 = 0x0101,
> V4L2_CTRL_TYPE_U32 = 0x0102,
> V4L2_CTRL_TYPE_AREA = 0x0106,
> + V4L2_CTRL_TYPE_RECT = 0x0107,
>
> V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110,
> V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111,
You are mixing different things in this patch, and in general the order of patches in
this series isn't correct.
Instead, first add the new type, then add WHICH_MIN/MAX_VAL, then add the new controls,
and finally add the documentation. Since the two controls are related to one another,
add them both in the same patch (one for adding the controls, one documenting the controls).
It's easier to review this series if it is ordered like that.
Regards,
Hans
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control
2022-05-18 6:24 ` [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control Yunke Cao
2022-05-19 7:49 ` Hans Verkuil
@ 2022-05-19 8:14 ` Hans Verkuil
2022-05-19 10:39 ` Laurent Pinchart
1 sibling, 1 reply; 16+ messages in thread
From: Hans Verkuil @ 2022-05-19 8:14 UTC (permalink / raw)
To: Yunke Cao, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media
On 5/18/22 08:24, Yunke Cao wrote:
> Including:
> 1. Add a control ID.
> 2. Add p_rect to struct v4l2_ext_control with basic support in
> v4l2-ctrls.
>
> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> Changelog since v2:
> - Better documentation.
>
> .../media/v4l/ext-ctrls-camera.rst | 10 ++++++++++
> .../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++
> .../media/videodev2.h.rst.exceptions | 1 +
> drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 ++++
> include/media/v4l2-ctrls.h | 2 ++
> include/uapi/linux/v4l2-controls.h | 2 ++
> include/uapi/linux/videodev2.h | 2 ++
> 8 files changed, 45 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 4c5061aa9cd4..c988a72b97b2 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -661,3 +661,13 @@ enum v4l2_scene_mode -
> .. [#f1]
> This control may be changed to a menu control in the future, if more
> options are required.
> +
> +``V4L2_CID_REGION_OF_INTEREST_RECT (struct)``
> + This control determines the region of interest. Region of interest is an
> + rectangular area represented by a struct v4l2_rect. The rectangle is in
> + pixel units and global coordinates. Use ``V4L2_CTRL_WHICH_MIN_VAL`` and
> + ``V4L2_CTRL_WHICH_MAX_VAL`` to query the range of coordinates.
Hmm, what does MIN and MAX mean in terms of a rectangle? It makes sense for
the width and height, but how is that interpreted for top and left?
Are these the minimum and maximum values each field of the struct can have?
So if the image is, say, 640x480, then the minimum value for a rectangle might
be 1x1@0x0, and the maximum 640x480@639x479. So in that case these are not real
rectangles, but they give the range for each field of the struct.
An alternative would be to see this as the min and max rectangle size and keep
the top/left values at 0.
To be honest, I'm not sure which one I would prefer.
Regards,
Hans
> +
> + Setting a region of interest allows the camera to optimize the capture for
> + the region. The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control
> + determines the detailed behavior.
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index 29971a45a2d4..f4e205ead0a2 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -189,6 +189,10 @@ still cause this situation.
> - ``p_area``
> - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> of type ``V4L2_CTRL_TYPE_AREA``.
> + * - struct :c:type:`v4l2_rect` *
> + - ``p_area``
> + - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is
> + of type ``V4L2_CTRL_TYPE_RECT``.
> * - struct :c:type:`v4l2_ctrl_h264_sps` *
> - ``p_h264_sps``
> - A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index 9cbb7a0c354a..7b423475281d 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type`
> +replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type`
> replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type`
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index 8968cec8454e..dcde405c2713 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> return ptr1.p_u16[idx] == ptr2.p_u16[idx];
> case V4L2_CTRL_TYPE_U32:
> return ptr1.p_u32[idx] == ptr2.p_u32[idx];
> + case V4L2_CTRL_TYPE_RECT:
> + return ptr1.p_rect->top == ptr2.p_rect->top &&
> + ptr1.p_rect->left == ptr2.p_rect->left &&
> + ptr1.p_rect->height == ptr2.p_rect->height &&
> + ptr1.p_rect->width == ptr2.p_rect->width;
> default:
> if (ctrl->is_int)
> return ptr1.p_s32[idx] == ptr2.p_s32[idx];
> @@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> case V4L2_CTRL_TYPE_VP9_FRAME:
> pr_cont("VP9_FRAME");
> break;
> + case V4L2_CTRL_TYPE_RECT:
> + pr_cont("l: %d, t: %d, w: %u, h: %u",
> + ptr.p_rect->left, ptr.p_rect->top,
> + ptr.p_rect->width, ptr.p_rect->height);
> + break;
> default:
> pr_cont("unknown type %d", ctrl->type);
> break;
> @@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
> struct v4l2_area *area;
> + struct v4l2_rect *rect;
> void *p = ptr.p + idx * ctrl->elem_size;
> unsigned int i;
>
> @@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> return -EINVAL;
> break;
>
> + case V4L2_CTRL_TYPE_RECT:
> + rect = p;
> + if (!rect->width || !rect->height)
> + return -EINVAL;
> + break;
> +
> default:
> return -EINVAL;
> }
> @@ -1456,6 +1473,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> case V4L2_CTRL_TYPE_AREA:
> elem_size = sizeof(struct v4l2_area);
> break;
> + case V4L2_CTRL_TYPE_RECT:
> + elem_size = sizeof(struct v4l2_rect);
> + break;
> default:
> if (type < V4L2_CTRL_COMPOUND_TYPES)
> elem_size = sizeof(s32);
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 54ca4e6b820b..95f39a2d2ad2 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
> case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
> case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
> + case V4L2_CID_REGION_OF_INTEREST_RECT: return "Region Of Interest Rectangle";
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1524,6 +1525,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> break;
> + case V4L2_CID_REGION_OF_INTEREST_RECT:
> + *type = V4L2_CTRL_TYPE_RECT;
> + break;
> default:
> *type = V4L2_CTRL_TYPE_INTEGER;
> break;
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index b3ce438f1329..919e104de50b 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -58,6 +58,7 @@ struct video_device;
> * @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure.
> * @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure.
> * @p_area: Pointer to an area.
> + * @p_rect: Pointer to a rectangle.
> * @p: Pointer to a compound value.
> * @p_const: Pointer to a constant compound value.
> */
> @@ -87,6 +88,7 @@ union v4l2_ctrl_ptr {
> struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll;
> struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> struct v4l2_area *p_area;
> + struct v4l2_rect *p_rect;
> void *p;
> const void *p_const;
> };
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index bb40129446d4..499fcddb6254 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
>
> #define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35)
>
> +#define V4L2_CID_REGION_OF_INTEREST_RECT (V4L2_CID_CAMERA_CLASS_BASE+36)
> +
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 3768a0a80830..b712412cf763 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1751,6 +1751,7 @@ struct v4l2_ext_control {
> __u16 __user *p_u16;
> __u32 __user *p_u32;
> struct v4l2_area __user *p_area;
> + struct v4l2_rect __user *p_rect;
> struct v4l2_ctrl_h264_sps __user *p_h264_sps;
> struct v4l2_ctrl_h264_pps *p_h264_pps;
> struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix;
> @@ -1810,6 +1811,7 @@ enum v4l2_ctrl_type {
> V4L2_CTRL_TYPE_U16 = 0x0101,
> V4L2_CTRL_TYPE_U32 = 0x0102,
> V4L2_CTRL_TYPE_AREA = 0x0106,
> + V4L2_CTRL_TYPE_RECT = 0x0107,
>
> V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110,
> V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111,
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control
2022-05-19 8:14 ` Hans Verkuil
@ 2022-05-19 10:39 ` Laurent Pinchart
2022-05-19 10:55 ` Hans Verkuil
0 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2022-05-19 10:39 UTC (permalink / raw)
To: Hans Verkuil
Cc: Yunke Cao, Nicolas Dufresne, Tomasz Figa, Sergey Senozhatsky,
Ricardo Ribalda, linux-media
On Thu, May 19, 2022 at 10:14:05AM +0200, Hans Verkuil wrote:
> On 5/18/22 08:24, Yunke Cao wrote:
> > Including:
> > 1. Add a control ID.
> > 2. Add p_rect to struct v4l2_ext_control with basic support in
> > v4l2-ctrls.
> >
> > Signed-off-by: Yunke Cao <yunkec@google.com>
> > ---
> > Changelog since v2:
> > - Better documentation.
> >
> > .../media/v4l/ext-ctrls-camera.rst | 10 ++++++++++
> > .../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++
> > .../media/videodev2.h.rst.exceptions | 1 +
> > drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++
> > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 ++++
> > include/media/v4l2-ctrls.h | 2 ++
> > include/uapi/linux/v4l2-controls.h | 2 ++
> > include/uapi/linux/videodev2.h | 2 ++
> > 8 files changed, 45 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > index 4c5061aa9cd4..c988a72b97b2 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > @@ -661,3 +661,13 @@ enum v4l2_scene_mode -
> > .. [#f1]
> > This control may be changed to a menu control in the future, if more
> > options are required.
> > +
> > +``V4L2_CID_REGION_OF_INTEREST_RECT (struct)``
> > + This control determines the region of interest. Region of interest is an
> > + rectangular area represented by a struct v4l2_rect. The rectangle is in
> > + pixel units and global coordinates. Use ``V4L2_CTRL_WHICH_MIN_VAL`` and
> > + ``V4L2_CTRL_WHICH_MAX_VAL`` to query the range of coordinates.
>
> Hmm, what does MIN and MAX mean in terms of a rectangle? It makes sense for
> the width and height, but how is that interpreted for top and left?
>
> Are these the minimum and maximum values each field of the struct can have?
> So if the image is, say, 640x480, then the minimum value for a rectangle might
> be 1x1@0x0, and the maximum 640x480@639x479. So in that case these are not real
> rectangles, but they give the range for each field of the struct.
>
> An alternative would be to see this as the min and max rectangle size and keep
> the top/left values at 0.
>
> To be honest, I'm not sure which one I would prefer.
I'm also really worried fo the interactions between this control and
selection rectangles. The uvcvideo driver won't need to care, but this
is a generic control, so we need to define it clearly.
To be honest, given how specific to UVC this is, I'd create
device-specific controls. I don't foresee any other driver being able to
make use of V4L2_CID_REGION_OF_INTEREST_RECT and
V4L2_CID_REGION_OF_INTEREST_AUTO.
> > +
> > + Setting a region of interest allows the camera to optimize the capture for
> > + the region. The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control
> > + determines the detailed behavior.
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > index 29971a45a2d4..f4e205ead0a2 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > @@ -189,6 +189,10 @@ still cause this situation.
> > - ``p_area``
> > - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> > of type ``V4L2_CTRL_TYPE_AREA``.
> > + * - struct :c:type:`v4l2_rect` *
> > + - ``p_area``
> > + - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is
> > + of type ``V4L2_CTRL_TYPE_RECT``.
> > * - struct :c:type:`v4l2_ctrl_h264_sps` *
> > - ``p_h264_sps``
> > - A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is
> > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > index 9cbb7a0c354a..7b423475281d 100644
> > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > @@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type`
> > replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type`
> > replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type`
> > replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type`
> > +replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type`
> > replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type`
> > replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type`
> > replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type`
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > index 8968cec8454e..dcde405c2713 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > @@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> > return ptr1.p_u16[idx] == ptr2.p_u16[idx];
> > case V4L2_CTRL_TYPE_U32:
> > return ptr1.p_u32[idx] == ptr2.p_u32[idx];
> > + case V4L2_CTRL_TYPE_RECT:
> > + return ptr1.p_rect->top == ptr2.p_rect->top &&
> > + ptr1.p_rect->left == ptr2.p_rect->left &&
> > + ptr1.p_rect->height == ptr2.p_rect->height &&
> > + ptr1.p_rect->width == ptr2.p_rect->width;
> > default:
> > if (ctrl->is_int)
> > return ptr1.p_s32[idx] == ptr2.p_s32[idx];
> > @@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> > case V4L2_CTRL_TYPE_VP9_FRAME:
> > pr_cont("VP9_FRAME");
> > break;
> > + case V4L2_CTRL_TYPE_RECT:
> > + pr_cont("l: %d, t: %d, w: %u, h: %u",
> > + ptr.p_rect->left, ptr.p_rect->top,
> > + ptr.p_rect->width, ptr.p_rect->height);
> > + break;
> > default:
> > pr_cont("unknown type %d", ctrl->type);
> > break;
> > @@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> > struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
> > struct v4l2_area *area;
> > + struct v4l2_rect *rect;
> > void *p = ptr.p + idx * ctrl->elem_size;
> > unsigned int i;
> >
> > @@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> > return -EINVAL;
> > break;
> >
> > + case V4L2_CTRL_TYPE_RECT:
> > + rect = p;
> > + if (!rect->width || !rect->height)
> > + return -EINVAL;
> > + break;
> > +
> > default:
> > return -EINVAL;
> > }
> > @@ -1456,6 +1473,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> > case V4L2_CTRL_TYPE_AREA:
> > elem_size = sizeof(struct v4l2_area);
> > break;
> > + case V4L2_CTRL_TYPE_RECT:
> > + elem_size = sizeof(struct v4l2_rect);
> > + break;
> > default:
> > if (type < V4L2_CTRL_COMPOUND_TYPES)
> > elem_size = sizeof(s32);
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index 54ca4e6b820b..95f39a2d2ad2 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
> > case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
> > case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
> > + case V4L2_CID_REGION_OF_INTEREST_RECT: return "Region Of Interest Rectangle";
> >
> > /* FM Radio Modulator controls */
> > /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > @@ -1524,6 +1525,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> > *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> > break;
> > + case V4L2_CID_REGION_OF_INTEREST_RECT:
> > + *type = V4L2_CTRL_TYPE_RECT;
> > + break;
> > default:
> > *type = V4L2_CTRL_TYPE_INTEGER;
> > break;
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index b3ce438f1329..919e104de50b 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -58,6 +58,7 @@ struct video_device;
> > * @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure.
> > * @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure.
> > * @p_area: Pointer to an area.
> > + * @p_rect: Pointer to a rectangle.
> > * @p: Pointer to a compound value.
> > * @p_const: Pointer to a constant compound value.
> > */
> > @@ -87,6 +88,7 @@ union v4l2_ctrl_ptr {
> > struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll;
> > struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> > struct v4l2_area *p_area;
> > + struct v4l2_rect *p_rect;
> > void *p;
> > const void *p_const;
> > };
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index bb40129446d4..499fcddb6254 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
> >
> > #define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35)
> >
> > +#define V4L2_CID_REGION_OF_INTEREST_RECT (V4L2_CID_CAMERA_CLASS_BASE+36)
> > +
> > /* FM Modulator class control IDs */
> >
> > #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 3768a0a80830..b712412cf763 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1751,6 +1751,7 @@ struct v4l2_ext_control {
> > __u16 __user *p_u16;
> > __u32 __user *p_u32;
> > struct v4l2_area __user *p_area;
> > + struct v4l2_rect __user *p_rect;
> > struct v4l2_ctrl_h264_sps __user *p_h264_sps;
> > struct v4l2_ctrl_h264_pps *p_h264_pps;
> > struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix;
> > @@ -1810,6 +1811,7 @@ enum v4l2_ctrl_type {
> > V4L2_CTRL_TYPE_U16 = 0x0101,
> > V4L2_CTRL_TYPE_U32 = 0x0102,
> > V4L2_CTRL_TYPE_AREA = 0x0106,
> > + V4L2_CTRL_TYPE_RECT = 0x0107,
> >
> > V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110,
> > V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control
2022-05-19 10:39 ` Laurent Pinchart
@ 2022-05-19 10:55 ` Hans Verkuil
2022-05-23 9:32 ` Yunke Cao
0 siblings, 1 reply; 16+ messages in thread
From: Hans Verkuil @ 2022-05-19 10:55 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yunke Cao, Nicolas Dufresne, Tomasz Figa, Sergey Senozhatsky,
Ricardo Ribalda, linux-media
On 5/19/22 12:39, Laurent Pinchart wrote:
> On Thu, May 19, 2022 at 10:14:05AM +0200, Hans Verkuil wrote:
>> On 5/18/22 08:24, Yunke Cao wrote:
>>> Including:
>>> 1. Add a control ID.
>>> 2. Add p_rect to struct v4l2_ext_control with basic support in
>>> v4l2-ctrls.
>>>
>>> Signed-off-by: Yunke Cao <yunkec@google.com>
>>> ---
>>> Changelog since v2:
>>> - Better documentation.
>>>
>>> .../media/v4l/ext-ctrls-camera.rst | 10 ++++++++++
>>> .../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++
>>> .../media/videodev2.h.rst.exceptions | 1 +
>>> drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++
>>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 ++++
>>> include/media/v4l2-ctrls.h | 2 ++
>>> include/uapi/linux/v4l2-controls.h | 2 ++
>>> include/uapi/linux/videodev2.h | 2 ++
>>> 8 files changed, 45 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>>> index 4c5061aa9cd4..c988a72b97b2 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>>> @@ -661,3 +661,13 @@ enum v4l2_scene_mode -
>>> .. [#f1]
>>> This control may be changed to a menu control in the future, if more
>>> options are required.
>>> +
>>> +``V4L2_CID_REGION_OF_INTEREST_RECT (struct)``
>>> + This control determines the region of interest. Region of interest is an
>>> + rectangular area represented by a struct v4l2_rect. The rectangle is in
>>> + pixel units and global coordinates. Use ``V4L2_CTRL_WHICH_MIN_VAL`` and
>>> + ``V4L2_CTRL_WHICH_MAX_VAL`` to query the range of coordinates.
>>
>> Hmm, what does MIN and MAX mean in terms of a rectangle? It makes sense for
>> the width and height, but how is that interpreted for top and left?
>>
>> Are these the minimum and maximum values each field of the struct can have?
>> So if the image is, say, 640x480, then the minimum value for a rectangle might
>> be 1x1@0x0, and the maximum 640x480@639x479. So in that case these are not real
>> rectangles, but they give the range for each field of the struct.
>>
>> An alternative would be to see this as the min and max rectangle size and keep
>> the top/left values at 0.
>>
>> To be honest, I'm not sure which one I would prefer.
>
> I'm also really worried fo the interactions between this control and
> selection rectangles. The uvcvideo driver won't need to care, but this
> is a generic control, so we need to define it clearly.
>
> To be honest, given how specific to UVC this is, I'd create
> device-specific controls. I don't foresee any other driver being able to
> make use of V4L2_CID_REGION_OF_INTEREST_RECT and
> V4L2_CID_REGION_OF_INTEREST_AUTO.
Yeah, I'm leaning in the same direction. It might be wise to reduce the
scope of these controls to just the UVC driver.
Regards,
Hans
>
>>> +
>>> + Setting a region of interest allows the camera to optimize the capture for
>>> + the region. The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control
>>> + determines the detailed behavior.
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> index 29971a45a2d4..f4e205ead0a2 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> @@ -189,6 +189,10 @@ still cause this situation.
>>> - ``p_area``
>>> - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
>>> of type ``V4L2_CTRL_TYPE_AREA``.
>>> + * - struct :c:type:`v4l2_rect` *
>>> + - ``p_area``
>>> + - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is
>>> + of type ``V4L2_CTRL_TYPE_RECT``.
>>> * - struct :c:type:`v4l2_ctrl_h264_sps` *
>>> - ``p_h264_sps``
>>> - A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is
>>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> index 9cbb7a0c354a..7b423475281d 100644
>>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> @@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type`
>>> replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type`
>>> replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type`
>>> replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type`
>>> +replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type`
>>> replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type`
>>> replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type`
>>> replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type`
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>> index 8968cec8454e..dcde405c2713 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>> @@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
>>> return ptr1.p_u16[idx] == ptr2.p_u16[idx];
>>> case V4L2_CTRL_TYPE_U32:
>>> return ptr1.p_u32[idx] == ptr2.p_u32[idx];
>>> + case V4L2_CTRL_TYPE_RECT:
>>> + return ptr1.p_rect->top == ptr2.p_rect->top &&
>>> + ptr1.p_rect->left == ptr2.p_rect->left &&
>>> + ptr1.p_rect->height == ptr2.p_rect->height &&
>>> + ptr1.p_rect->width == ptr2.p_rect->width;
>>> default:
>>> if (ctrl->is_int)
>>> return ptr1.p_s32[idx] == ptr2.p_s32[idx];
>>> @@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl)
>>> case V4L2_CTRL_TYPE_VP9_FRAME:
>>> pr_cont("VP9_FRAME");
>>> break;
>>> + case V4L2_CTRL_TYPE_RECT:
>>> + pr_cont("l: %d, t: %d, w: %u, h: %u",
>>> + ptr.p_rect->left, ptr.p_rect->top,
>>> + ptr.p_rect->width, ptr.p_rect->height);
>>> + break;
>>> default:
>>> pr_cont("unknown type %d", ctrl->type);
>>> break;
>>> @@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>> struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
>>> struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
>>> struct v4l2_area *area;
>>> + struct v4l2_rect *rect;
>>> void *p = ptr.p + idx * ctrl->elem_size;
>>> unsigned int i;
>>>
>>> @@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>> return -EINVAL;
>>> break;
>>>
>>> + case V4L2_CTRL_TYPE_RECT:
>>> + rect = p;
>>> + if (!rect->width || !rect->height)
>>> + return -EINVAL;
>>> + break;
>>> +
>>> default:
>>> return -EINVAL;
>>> }
>>> @@ -1456,6 +1473,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>> case V4L2_CTRL_TYPE_AREA:
>>> elem_size = sizeof(struct v4l2_area);
>>> break;
>>> + case V4L2_CTRL_TYPE_RECT:
>>> + elem_size = sizeof(struct v4l2_rect);
>>> + break;
>>> default:
>>> if (type < V4L2_CTRL_COMPOUND_TYPES)
>>> elem_size = sizeof(s32);
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> index 54ca4e6b820b..95f39a2d2ad2 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>> case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
>>> case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
>>> case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
>>> + case V4L2_CID_REGION_OF_INTEREST_RECT: return "Region Of Interest Rectangle";
>>>
>>> /* FM Radio Modulator controls */
>>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
>>> @@ -1524,6 +1525,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>> case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
>>> *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
>>> break;
>>> + case V4L2_CID_REGION_OF_INTEREST_RECT:
>>> + *type = V4L2_CTRL_TYPE_RECT;
>>> + break;
>>> default:
>>> *type = V4L2_CTRL_TYPE_INTEGER;
>>> break;
>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>> index b3ce438f1329..919e104de50b 100644
>>> --- a/include/media/v4l2-ctrls.h
>>> +++ b/include/media/v4l2-ctrls.h
>>> @@ -58,6 +58,7 @@ struct video_device;
>>> * @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure.
>>> * @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure.
>>> * @p_area: Pointer to an area.
>>> + * @p_rect: Pointer to a rectangle.
>>> * @p: Pointer to a compound value.
>>> * @p_const: Pointer to a constant compound value.
>>> */
>>> @@ -87,6 +88,7 @@ union v4l2_ctrl_ptr {
>>> struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll;
>>> struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
>>> struct v4l2_area *p_area;
>>> + struct v4l2_rect *p_rect;
>>> void *p;
>>> const void *p_const;
>>> };
>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>> index bb40129446d4..499fcddb6254 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
>>>
>>> #define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35)
>>>
>>> +#define V4L2_CID_REGION_OF_INTEREST_RECT (V4L2_CID_CAMERA_CLASS_BASE+36)
>>> +
>>> /* FM Modulator class control IDs */
>>>
>>> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 3768a0a80830..b712412cf763 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1751,6 +1751,7 @@ struct v4l2_ext_control {
>>> __u16 __user *p_u16;
>>> __u32 __user *p_u32;
>>> struct v4l2_area __user *p_area;
>>> + struct v4l2_rect __user *p_rect;
>>> struct v4l2_ctrl_h264_sps __user *p_h264_sps;
>>> struct v4l2_ctrl_h264_pps *p_h264_pps;
>>> struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix;
>>> @@ -1810,6 +1811,7 @@ enum v4l2_ctrl_type {
>>> V4L2_CTRL_TYPE_U16 = 0x0101,
>>> V4L2_CTRL_TYPE_U32 = 0x0102,
>>> V4L2_CTRL_TYPE_AREA = 0x0106,
>>> + V4L2_CTRL_TYPE_RECT = 0x0107,
>>>
>>> V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110,
>>> V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111,
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control
2022-05-19 10:55 ` Hans Verkuil
@ 2022-05-23 9:32 ` Yunke Cao
0 siblings, 0 replies; 16+ messages in thread
From: Yunke Cao @ 2022-05-23 9:32 UTC (permalink / raw)
To: Hans Verkuil
Cc: Laurent Pinchart, Nicolas Dufresne, Tomasz Figa,
Sergey Senozhatsky, Ricardo Ribalda, linux-media
Thank you for the feedback!
Making this uvc-specific sounds good to me. I'm working on v4 :).
On Thu, May 19, 2022 at 7:55 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 5/19/22 12:39, Laurent Pinchart wrote:
> > On Thu, May 19, 2022 at 10:14:05AM +0200, Hans Verkuil wrote:
> >> On 5/18/22 08:24, Yunke Cao wrote:
> >>> Including:
> >>> 1. Add a control ID.
> >>> 2. Add p_rect to struct v4l2_ext_control with basic support in
> >>> v4l2-ctrls.
> >>>
> >>> Signed-off-by: Yunke Cao <yunkec@google.com>
> >>> ---
> >>> Changelog since v2:
> >>> - Better documentation.
> >>>
> >>> .../media/v4l/ext-ctrls-camera.rst | 10 ++++++++++
> >>> .../media/v4l/vidioc-g-ext-ctrls.rst | 4 ++++
> >>> .../media/videodev2.h.rst.exceptions | 1 +
> >>> drivers/media/v4l2-core/v4l2-ctrls-core.c | 20 +++++++++++++++++++
> >>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 ++++
> >>> include/media/v4l2-ctrls.h | 2 ++
> >>> include/uapi/linux/v4l2-controls.h | 2 ++
> >>> include/uapi/linux/videodev2.h | 2 ++
> >>> 8 files changed, 45 insertions(+)
> >>>
> >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >>> index 4c5061aa9cd4..c988a72b97b2 100644
> >>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >>> @@ -661,3 +661,13 @@ enum v4l2_scene_mode -
> >>> .. [#f1]
> >>> This control may be changed to a menu control in the future, if more
> >>> options are required.
> >>> +
> >>> +``V4L2_CID_REGION_OF_INTEREST_RECT (struct)``
> >>> + This control determines the region of interest. Region of interest is an
> >>> + rectangular area represented by a struct v4l2_rect. The rectangle is in
> >>> + pixel units and global coordinates. Use ``V4L2_CTRL_WHICH_MIN_VAL`` and
> >>> + ``V4L2_CTRL_WHICH_MAX_VAL`` to query the range of coordinates.
> >>
> >> Hmm, what does MIN and MAX mean in terms of a rectangle? It makes sense for
> >> the width and height, but how is that interpreted for top and left?
> >>
> >> Are these the minimum and maximum values each field of the struct can have?
> >> So if the image is, say, 640x480, then the minimum value for a rectangle might
> >> be 1x1@0x0, and the maximum 640x480@639x479. So in that case these are not real
> >> rectangles, but they give the range for each field of the struct.
> >>
> >> An alternative would be to see this as the min and max rectangle size and keep
> >> the top/left values at 0.
> >>
> >> To be honest, I'm not sure which one I would prefer.
It looks like the UVC GET_MAX returns a valid rectangle (640x480@0x0).
UVC 1.5 defines CT_DIGITAL_WINDOW_CONTROL controls (we haven't
implemented it yet).
"The ROI must be within the current Digital Window as specified by the
CT_WINDOW control.
GET_MAX shall return the current Window as specified by
CT_DIGITAL_WINDOW_CONTROL."
The devices I tested with followed this: returning 1x1@0x0 and 640x480@0x0.
The current implementation simply takes the rectangle returned by
UVC_GET_MAX/MIN
and converts it to a v4l2_rect. As we are making them uvc-specific, I
guess keeping the current
behavior is fine?
Best,
Yunke
> >
> > I'm also really worried fo the interactions between this control and
> > selection rectangles. The uvcvideo driver won't need to care, but this
> > is a generic control, so we need to define it clearly.
> >
> > To be honest, given how specific to UVC this is, I'd create
> > device-specific controls. I don't foresee any other driver being able to
> > make use of V4L2_CID_REGION_OF_INTEREST_RECT and
> > V4L2_CID_REGION_OF_INTEREST_AUTO.
>
> Yeah, I'm leaning in the same direction. It might be wise to reduce the
> scope of these controls to just the UVC driver.
>
> Regards,
>
> Hans
>
> >
> >>> +
> >>> + Setting a region of interest allows the camera to optimize the capture for
> >>> + the region. The value of ``V4L2_CID_REGION_OF_INTEREST_AUTO`` control
> >>> + determines the detailed behavior.
> >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> index 29971a45a2d4..f4e205ead0a2 100644
> >>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> @@ -189,6 +189,10 @@ still cause this situation.
> >>> - ``p_area``
> >>> - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> >>> of type ``V4L2_CTRL_TYPE_AREA``.
> >>> + * - struct :c:type:`v4l2_rect` *
> >>> + - ``p_area``
> >>> + - A pointer to a struct :c:type:`v4l2_rect`. Valid if this control is
> >>> + of type ``V4L2_CTRL_TYPE_RECT``.
> >>> * - struct :c:type:`v4l2_ctrl_h264_sps` *
> >>> - ``p_h264_sps``
> >>> - A pointer to a struct :c:type:`v4l2_ctrl_h264_sps`. Valid if this control is
> >>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >>> index 9cbb7a0c354a..7b423475281d 100644
> >>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >>> @@ -147,6 +147,7 @@ replace symbol V4L2_CTRL_TYPE_HEVC_SPS :c:type:`v4l2_ctrl_type`
> >>> replace symbol V4L2_CTRL_TYPE_HEVC_PPS :c:type:`v4l2_ctrl_type`
> >>> replace symbol V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS :c:type:`v4l2_ctrl_type`
> >>> replace symbol V4L2_CTRL_TYPE_AREA :c:type:`v4l2_ctrl_type`
> >>> +replace symbol V4L2_CTRL_TYPE_RECT :c:type:`v4l2_ctrl_type`
> >>> replace symbol V4L2_CTRL_TYPE_FWHT_PARAMS :c:type:`v4l2_ctrl_type`
> >>> replace symbol V4L2_CTRL_TYPE_VP8_FRAME :c:type:`v4l2_ctrl_type`
> >>> replace symbol V4L2_CTRL_TYPE_VP9_COMPRESSED_HDR :c:type:`v4l2_ctrl_type`
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>> index 8968cec8454e..dcde405c2713 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>> @@ -84,6 +84,11 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
> >>> return ptr1.p_u16[idx] == ptr2.p_u16[idx];
> >>> case V4L2_CTRL_TYPE_U32:
> >>> return ptr1.p_u32[idx] == ptr2.p_u32[idx];
> >>> + case V4L2_CTRL_TYPE_RECT:
> >>> + return ptr1.p_rect->top == ptr2.p_rect->top &&
> >>> + ptr1.p_rect->left == ptr2.p_rect->left &&
> >>> + ptr1.p_rect->height == ptr2.p_rect->height &&
> >>> + ptr1.p_rect->width == ptr2.p_rect->width;
> >>> default:
> >>> if (ctrl->is_int)
> >>> return ptr1.p_s32[idx] == ptr2.p_s32[idx];
> >>> @@ -307,6 +312,11 @@ static void std_log(const struct v4l2_ctrl *ctrl)
> >>> case V4L2_CTRL_TYPE_VP9_FRAME:
> >>> pr_cont("VP9_FRAME");
> >>> break;
> >>> + case V4L2_CTRL_TYPE_RECT:
> >>> + pr_cont("l: %d, t: %d, w: %u, h: %u",
> >>> + ptr.p_rect->left, ptr.p_rect->top,
> >>> + ptr.p_rect->width, ptr.p_rect->height);
> >>> + break;
> >>> default:
> >>> pr_cont("unknown type %d", ctrl->type);
> >>> break;
> >>> @@ -525,6 +535,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >>> struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> >>> struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
> >>> struct v4l2_area *area;
> >>> + struct v4l2_rect *rect;
> >>> void *p = ptr.p + idx * ctrl->elem_size;
> >>> unsigned int i;
> >>>
> >>> @@ -888,6 +899,12 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >>> return -EINVAL;
> >>> break;
> >>>
> >>> + case V4L2_CTRL_TYPE_RECT:
> >>> + rect = p;
> >>> + if (!rect->width || !rect->height)
> >>> + return -EINVAL;
> >>> + break;
> >>> +
> >>> default:
> >>> return -EINVAL;
> >>> }
> >>> @@ -1456,6 +1473,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >>> case V4L2_CTRL_TYPE_AREA:
> >>> elem_size = sizeof(struct v4l2_area);
> >>> break;
> >>> + case V4L2_CTRL_TYPE_RECT:
> >>> + elem_size = sizeof(struct v4l2_rect);
> >>> + break;
> >>> default:
> >>> if (type < V4L2_CTRL_COMPOUND_TYPES)
> >>> elem_size = sizeof(s32);
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>> index 54ca4e6b820b..95f39a2d2ad2 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>> @@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>> case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size";
> >>> case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
> >>> case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
> >>> + case V4L2_CID_REGION_OF_INTEREST_RECT: return "Region Of Interest Rectangle";
> >>>
> >>> /* FM Radio Modulator controls */
> >>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> >>> @@ -1524,6 +1525,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>> case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> >>> *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> >>> break;
> >>> + case V4L2_CID_REGION_OF_INTEREST_RECT:
> >>> + *type = V4L2_CTRL_TYPE_RECT;
> >>> + break;
> >>> default:
> >>> *type = V4L2_CTRL_TYPE_INTEGER;
> >>> break;
> >>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>> index b3ce438f1329..919e104de50b 100644
> >>> --- a/include/media/v4l2-ctrls.h
> >>> +++ b/include/media/v4l2-ctrls.h
> >>> @@ -58,6 +58,7 @@ struct video_device;
> >>> * @p_hdr10_cll: Pointer to an HDR10 Content Light Level structure.
> >>> * @p_hdr10_mastering: Pointer to an HDR10 Mastering Display structure.
> >>> * @p_area: Pointer to an area.
> >>> + * @p_rect: Pointer to a rectangle.
> >>> * @p: Pointer to a compound value.
> >>> * @p_const: Pointer to a constant compound value.
> >>> */
> >>> @@ -87,6 +88,7 @@ union v4l2_ctrl_ptr {
> >>> struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll;
> >>> struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering;
> >>> struct v4l2_area *p_area;
> >>> + struct v4l2_rect *p_rect;
> >>> void *p;
> >>> const void *p_const;
> >>> };
> >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>> index bb40129446d4..499fcddb6254 100644
> >>> --- a/include/uapi/linux/v4l2-controls.h
> >>> +++ b/include/uapi/linux/v4l2-controls.h
> >>> @@ -1008,6 +1008,8 @@ enum v4l2_auto_focus_range {
> >>>
> >>> #define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35)
> >>>
> >>> +#define V4L2_CID_REGION_OF_INTEREST_RECT (V4L2_CID_CAMERA_CLASS_BASE+36)
> >>> +
> >>> /* FM Modulator class control IDs */
> >>>
> >>> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
> >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>> index 3768a0a80830..b712412cf763 100644
> >>> --- a/include/uapi/linux/videodev2.h
> >>> +++ b/include/uapi/linux/videodev2.h
> >>> @@ -1751,6 +1751,7 @@ struct v4l2_ext_control {
> >>> __u16 __user *p_u16;
> >>> __u32 __user *p_u32;
> >>> struct v4l2_area __user *p_area;
> >>> + struct v4l2_rect __user *p_rect;
> >>> struct v4l2_ctrl_h264_sps __user *p_h264_sps;
> >>> struct v4l2_ctrl_h264_pps *p_h264_pps;
> >>> struct v4l2_ctrl_h264_scaling_matrix __user *p_h264_scaling_matrix;
> >>> @@ -1810,6 +1811,7 @@ enum v4l2_ctrl_type {
> >>> V4L2_CTRL_TYPE_U16 = 0x0101,
> >>> V4L2_CTRL_TYPE_U32 = 0x0102,
> >>> V4L2_CTRL_TYPE_AREA = 0x0106,
> >>> + V4L2_CTRL_TYPE_RECT = 0x0107,
> >>>
> >>> V4L2_CTRL_TYPE_HDR10_CLL_INFO = 0x0110,
> >>> V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY = 0x0111,
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/6] media: v4l2_ctrl: Add region of interest auto control
2022-05-18 6:24 [PATCH v3 0/6] media: Implement UVC v1.5 ROI Yunke Cao
2022-05-18 6:24 ` [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control Yunke Cao
@ 2022-05-18 6:24 ` Yunke Cao
2022-05-19 7:52 ` Hans Verkuil
2022-05-18 6:24 ` [PATCH v3 3/6] media: v4l2_ctrl: Add V4L2_CTRL_WHICH_MIN/MAX_VAL Yunke Cao
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Yunke Cao @ 2022-05-18 6:24 UTC (permalink / raw)
To: Hans Verkuil, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media, Yunke Cao
Follows the UVC v1.5 class specification.
Signed-off-by: Yunke Cao <yunkec@google.com>
---
Changelog since v2:
- Better documentation.
- Rename V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE to
V4L2_REGION_OF_INTEREST_AUTO_EXPOSURE, etc. The bit masks shouldn't
have "CID" in it.
.../media/v4l/ext-ctrls-camera.rst | 39 +++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 3 ++
include/uapi/linux/v4l2-controls.h | 9 +++++
3 files changed, 51 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index c988a72b97b2..c26c28cfcf6a 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -671,3 +671,42 @@ enum v4l2_scene_mode -
Setting a region of interest 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_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_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 detection and tracking. The
+ current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
+ the firmware.
+ * - ``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 firmware.
+ * - ``V4L2_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
+ - Setting this to true enables automatically capture the specified region
+ with higher quality if possible.
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 95f39a2d2ad2..220afc4d5244 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1043,6 +1043,8 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
case V4L2_CID_REGION_OF_INTEREST_RECT: return "Region Of Interest Rectangle";
+ case V4L2_CID_REGION_OF_INTEREST_AUTO: return "Region Of Interest Auto Controls";
+
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1415,6 +1417,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
case V4L2_CID_JPEG_ACTIVE_MARKER:
case V4L2_CID_3A_LOCK:
case V4L2_CID_AUTO_FOCUS_STATUS:
+ case V4L2_CID_REGION_OF_INTEREST_AUTO:
case V4L2_CID_DV_TX_HOTPLUG:
case V4L2_CID_DV_TX_RXSENSE:
case V4L2_CID_DV_TX_EDID_PRESENT:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 499fcddb6254..13db0638533c 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1009,6 +1009,15 @@ enum v4l2_auto_focus_range {
#define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35)
#define V4L2_CID_REGION_OF_INTEREST_RECT (V4L2_CID_CAMERA_CLASS_BASE+36)
+#define V4L2_CID_REGION_OF_INTEREST_AUTO (V4L2_CID_CAMERA_CLASS_BASE+37)
+#define V4L2_REGION_OF_INTEREST_AUTO_EXPOSURE (1 << 0)
+#define V4L2_REGION_OF_INTEREST_AUTO_IRIS (1 << 1)
+#define V4L2_REGION_OF_INTEREST_AUTO_WHITE_BALANCE (1 << 2)
+#define V4L2_REGION_OF_INTEREST_AUTO_FOCUS (1 << 3)
+#define V4L2_REGION_OF_INTEREST_AUTO_FACE_DETECT (1 << 4)
+#define V4L2_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK (1 << 5)
+#define V4L2_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION (1 << 6)
+#define V4L2_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY (1 << 7)
/* FM Modulator class control IDs */
--
2.36.0.550.gb090851708-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/6] media: v4l2_ctrl: Add region of interest auto control
2022-05-18 6:24 ` [PATCH v3 2/6] media: v4l2_ctrl: Add region of interest auto control Yunke Cao
@ 2022-05-19 7:52 ` Hans Verkuil
0 siblings, 0 replies; 16+ messages in thread
From: Hans Verkuil @ 2022-05-19 7:52 UTC (permalink / raw)
To: Yunke Cao, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media
On 5/18/22 08:24, Yunke Cao wrote:
> Follows the UVC v1.5 class specification.
>
> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> Changelog since v2:
> - Better documentation.
> - Rename V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE to
> V4L2_REGION_OF_INTEREST_AUTO_EXPOSURE, etc. The bit masks shouldn't
> have "CID" in it.
>
> .../media/v4l/ext-ctrls-camera.rst | 39 +++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 3 ++
> include/uapi/linux/v4l2-controls.h | 9 +++++
> 3 files changed, 51 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index c988a72b97b2..c26c28cfcf6a 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -671,3 +671,42 @@ enum v4l2_scene_mode -
> Setting a region of interest 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_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_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 detection and tracking. The
"AUTO_DETECT": detect what? Faces?
> + current value of ``V4L2_CID_REGION_OF_INTEREST_RECT`` may be updated by
> + the firmware.
I'd say 'driver' instead of 'firmware'.
> + * - ``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 firmware.
Ditto.
> + * - ``V4L2_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> + - Setting this to true enables automatically capture the specified region
> + with higher quality if possible.
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 95f39a2d2ad2..220afc4d5244 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1043,6 +1043,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
> case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
> case V4L2_CID_REGION_OF_INTEREST_RECT: return "Region Of Interest Rectangle";
> + case V4L2_CID_REGION_OF_INTEREST_AUTO: return "Region Of Interest Auto Controls";
> +
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1415,6 +1417,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_JPEG_ACTIVE_MARKER:
> case V4L2_CID_3A_LOCK:
> case V4L2_CID_AUTO_FOCUS_STATUS:
> + case V4L2_CID_REGION_OF_INTEREST_AUTO:
> case V4L2_CID_DV_TX_HOTPLUG:
> case V4L2_CID_DV_TX_RXSENSE:
> case V4L2_CID_DV_TX_EDID_PRESENT:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 499fcddb6254..13db0638533c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1009,6 +1009,15 @@ enum v4l2_auto_focus_range {
> #define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35)
>
> #define V4L2_CID_REGION_OF_INTEREST_RECT (V4L2_CID_CAMERA_CLASS_BASE+36)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO (V4L2_CID_CAMERA_CLASS_BASE+37)
> +#define V4L2_REGION_OF_INTEREST_AUTO_EXPOSURE (1 << 0)
> +#define V4L2_REGION_OF_INTEREST_AUTO_IRIS (1 << 1)
> +#define V4L2_REGION_OF_INTEREST_AUTO_WHITE_BALANCE (1 << 2)
> +#define V4L2_REGION_OF_INTEREST_AUTO_FOCUS (1 << 3)
> +#define V4L2_REGION_OF_INTEREST_AUTO_FACE_DETECT (1 << 4)
> +#define V4L2_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK (1 << 5)
> +#define V4L2_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION (1 << 6)
> +#define V4L2_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY (1 << 7)
>
> /* FM Modulator class control IDs */
>
Regards,
Hans
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 3/6] media: v4l2_ctrl: Add V4L2_CTRL_WHICH_MIN/MAX_VAL
2022-05-18 6:24 [PATCH v3 0/6] media: Implement UVC v1.5 ROI Yunke Cao
2022-05-18 6:24 ` [PATCH v3 1/6] media: v4l2_ctrl: Add region of interest rectangle control Yunke Cao
2022-05-18 6:24 ` [PATCH v3 2/6] media: v4l2_ctrl: Add region of interest auto control Yunke Cao
@ 2022-05-18 6:24 ` Yunke Cao
2022-05-18 6:24 ` [PATCH v3 4/6] media: uvcvideo: implement UVC v1.5 ROI Yunke Cao
` (3 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Yunke Cao @ 2022-05-18 6:24 UTC (permalink / raw)
To: Hans Verkuil, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media, Yunke Cao
Added V4L2_CTRL_WHICH_MIN/MAX_VAL and basic support in v4l2-core.
Mostly reusing a previous attempt:
https://lore.kernel.org/all/20191220134843.25977-1-m.tretter@pengutronix.de/T/#m5c83970af8b774a4b1ea5f2dca4c0a534da4ccbe
Signed-off-by: Yunke Cao <yunkec@google.com>
---
Changelog since v2:
- Fix minor typo.
.../media/v4l/vidioc-g-ext-ctrls.rst | 8 +-
.../media/videodev2.h.rst.exceptions | 2 +
drivers/media/i2c/imx214.c | 4 +-
drivers/media/v4l2-core/v4l2-ctrls-api.c | 51 +++++--
drivers/media/v4l2-core/v4l2-ctrls-core.c | 135 ++++++++++++++++--
drivers/media/v4l2-core/v4l2-ioctl.c | 4 +-
include/media/v4l2-ctrls.h | 30 +++-
include/uapi/linux/videodev2.h | 2 +
8 files changed, 209 insertions(+), 27 deletions(-)
diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
index f4e205ead0a2..a89577726efa 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
@@ -284,14 +284,18 @@ still cause this situation.
- Which value of the control to get/set/try.
* - :cspan:`2` ``V4L2_CTRL_WHICH_CUR_VAL`` will return the current value of
the control, ``V4L2_CTRL_WHICH_DEF_VAL`` will return the default
+ value of the control, ``V4L2_CTRL_WHICH_MIN_VAL`` will return the minimum
+ value of the control, ``V4L2_CTRL_WHICH_MAX_VAL`` will return the maximum
value of the control and ``V4L2_CTRL_WHICH_REQUEST_VAL`` indicates that
these controls have to be retrieved from a request or tried/set for
a request. In the latter case the ``request_fd`` field contains the
file descriptor of the request that should be used. If the device
does not support requests, then ``EACCES`` will be returned.
- When using ``V4L2_CTRL_WHICH_DEF_VAL`` be aware that you can only
- get the default value of the control, you cannot set or try it.
+ When using ``V4L2_CTRL_WHICH_DEF_VAL``, ``V4L2_CTRL_WHICH_MIN_VAL``
+ or ``V4L2_CTRL_WHICH_MAX_VAL`` be aware that you can only
+ get the default/minimum/maximum value of the control, you cannot set
+ or try it.
For backwards compatibility you can also use a control class here
(see :ref:`ctrl-class`). In that case all controls have to
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index 7b423475281d..e2dde31d76df 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -553,6 +553,8 @@ ignore define V4L2_CTRL_DRIVER_PRIV
ignore define V4L2_CTRL_MAX_DIMS
ignore define V4L2_CTRL_WHICH_CUR_VAL
ignore define V4L2_CTRL_WHICH_DEF_VAL
+ignore define V4L2_CTRL_WHICH_MIN_VAL
+ignore define V4L2_CTRL_WHICH_MAX_VAL
ignore define V4L2_CTRL_WHICH_REQUEST_VAL
ignore define V4L2_OUT_CAP_CUSTOM_TIMINGS
ignore define V4L2_CID_MAX_CTRLS
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 83c1737abeec..1541a81dcd46 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -1037,7 +1037,9 @@ static int imx214_probe(struct i2c_client *client)
imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
NULL,
V4L2_CID_UNIT_CELL_SIZE,
- v4l2_ctrl_ptr_create((void *)&unit_size));
+ v4l2_ctrl_ptr_create((void *)&unit_size),
+ v4l2_ctrl_ptr_create(NULL),
+ v4l2_ctrl_ptr_create(NULL));
ret = imx214->ctrls.error;
if (ret) {
dev_err(&client->dev, "%s control init failed (%d)\n",
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index db9baa0bd05f..8a9c816b0dab 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
@@ -97,6 +97,28 @@ static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
return ptr_to_user(c, ctrl, ctrl->p_new);
}
+/* Helper function: copy the minimum control value back to the caller */
+static int min_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
+{
+ int idx;
+
+ for (idx = 0; idx < ctrl->elems; idx++)
+ ctrl->type_ops->minimum(ctrl, idx, ctrl->p_new);
+
+ return ptr_to_user(c, ctrl, ctrl->p_new);
+}
+
+/* Helper function: copy the maximum control value back to the caller */
+static int max_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
+{
+ int idx;
+
+ for (idx = 0; idx < ctrl->elems; idx++)
+ ctrl->type_ops->maximum(ctrl, idx, ctrl->p_new);
+
+ return ptr_to_user(c, ctrl, ctrl->p_new);
+}
+
/* Helper function: copy the caller-provider value to the given control value */
static int user_to_ptr(struct v4l2_ext_control *c,
struct v4l2_ctrl *ctrl,
@@ -220,8 +242,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
cs->error_idx = i;
if (cs->which &&
- cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
- cs->which != V4L2_CTRL_WHICH_REQUEST_VAL &&
+ (cs->which < V4L2_CTRL_WHICH_DEF_VAL ||
+ cs->which > V4L2_CTRL_WHICH_MAX_VAL) &&
V4L2_CTRL_ID2WHICH(id) != cs->which) {
dprintk(vdev,
"invalid which 0x%x or control id 0x%x\n",
@@ -335,8 +357,8 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
*/
static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
{
- if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL ||
- which == V4L2_CTRL_WHICH_REQUEST_VAL)
+ if (which == 0 || (which >= V4L2_CTRL_WHICH_DEF_VAL &&
+ which <= V4L2_CTRL_WHICH_MAX_VAL))
return 0;
return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
}
@@ -356,10 +378,12 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
struct v4l2_ctrl_helper *helpers = helper;
int ret;
int i, j;
- bool is_default, is_request;
+ bool is_default, is_request, is_min, is_max;
is_default = (cs->which == V4L2_CTRL_WHICH_DEF_VAL);
is_request = (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL);
+ is_min = (cs->which == V4L2_CTRL_WHICH_MIN_VAL);
+ is_max = (cs->which == V4L2_CTRL_WHICH_MAX_VAL);
cs->error_idx = cs->count;
cs->which = V4L2_CTRL_ID2WHICH(cs->which);
@@ -399,13 +423,14 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
/*
* g_volatile_ctrl will update the new control values.
- * This makes no sense for V4L2_CTRL_WHICH_DEF_VAL and
+ * This makes no sense for V4L2_CTRL_WHICH_DEF_VAL,
+ * V4L2_CTRL_WHICH_MIN_VAL, V4L2_CTRL_WHICH_MAX_VAL and
* V4L2_CTRL_WHICH_REQUEST_VAL. In the case of requests
* it is v4l2_ctrl_request_complete() that copies the
* volatile controls at the time of request completion
* to the request, so you don't want to do that again.
*/
- if (!is_default && !is_request &&
+ if (!is_default && !is_request && !is_min && !is_max &&
((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
(master->has_volatiles && !is_cur_manual(master)))) {
for (j = 0; j < master->ncontrols; j++)
@@ -432,6 +457,10 @@ int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
ret = def_to_user(cs->controls + idx, ref->ctrl);
else if (is_request && ref->valid_p_req)
ret = req_to_user(cs->controls + idx, ref);
+ else if (is_min)
+ ret = min_to_user(cs->controls + idx, ref->ctrl);
+ else if (is_max)
+ ret = max_to_user(cs->controls + idx, ref->ctrl);
else if (is_volatile)
ret = new_to_user(cs->controls + idx, ref->ctrl);
else
@@ -523,9 +552,11 @@ int try_set_ext_ctrls_common(struct v4l2_fh *fh,
cs->error_idx = cs->count;
- /* Default value cannot be changed */
- if (cs->which == V4L2_CTRL_WHICH_DEF_VAL) {
- dprintk(vdev, "%s: cannot change default value\n",
+ /* Default/minimum/maximum values cannot be changed */
+ if (cs->which == V4L2_CTRL_WHICH_DEF_VAL ||
+ cs->which == V4L2_CTRL_WHICH_MIN_VAL ||
+ cs->which == V4L2_CTRL_WHICH_MAX_VAL) {
+ dprintk(vdev, "%s: cannot change default/min/max value\n",
video_device_node_name(vdev));
return -EINVAL;
}
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index dcde405c2713..d056bd6a1561 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -186,6 +186,28 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
}
}
+static void std_min_compound(const struct v4l2_ctrl *ctrl, u32 idx,
+ union v4l2_ctrl_ptr ptr)
+{
+ void *p = ptr.p + idx * ctrl->elem_size;
+
+ if (ctrl->p_min.p_const)
+ memcpy(p, ctrl->p_min.p_const, ctrl->elem_size);
+ else
+ memset(p, 0, ctrl->elem_size);
+}
+
+static void std_max_compound(const struct v4l2_ctrl *ctrl, u32 idx,
+ union v4l2_ctrl_ptr ptr)
+{
+ void *p = ptr.p + idx * ctrl->elem_size;
+
+ if (ctrl->p_max.p_const)
+ memcpy(p, ctrl->p_max.p_const, ctrl->elem_size);
+ else
+ memset(p, 0, ctrl->elem_size);
+}
+
static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
union v4l2_ctrl_ptr ptr)
{
@@ -224,6 +246,82 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
}
}
+static void std_minimum(const struct v4l2_ctrl *ctrl, u32 idx,
+ union v4l2_ctrl_ptr ptr)
+{
+ switch (ctrl->type) {
+ case V4L2_CTRL_TYPE_STRING:
+ idx *= ctrl->elem_size;
+ memset(ptr.p_char + idx, ' ', ctrl->minimum);
+ ptr.p_char[idx + ctrl->minimum] = '\0';
+ break;
+ case V4L2_CTRL_TYPE_INTEGER64:
+ ptr.p_s64[idx] = ctrl->minimum;
+ break;
+ case V4L2_CTRL_TYPE_INTEGER:
+ case V4L2_CTRL_TYPE_INTEGER_MENU:
+ case V4L2_CTRL_TYPE_MENU:
+ case V4L2_CTRL_TYPE_BITMASK:
+ case V4L2_CTRL_TYPE_BOOLEAN:
+ ptr.p_s32[idx] = ctrl->minimum;
+ break;
+ case V4L2_CTRL_TYPE_BUTTON:
+ case V4L2_CTRL_TYPE_CTRL_CLASS:
+ ptr.p_s32[idx] = 0;
+ break;
+ case V4L2_CTRL_TYPE_U8:
+ ptr.p_u8[idx] = ctrl->minimum;
+ break;
+ case V4L2_CTRL_TYPE_U16:
+ ptr.p_u16[idx] = ctrl->minimum;
+ break;
+ case V4L2_CTRL_TYPE_U32:
+ ptr.p_u32[idx] = ctrl->minimum;
+ break;
+ default:
+ std_min_compound(ctrl, idx, ptr);
+ break;
+ }
+}
+
+static void std_maximum(const struct v4l2_ctrl *ctrl, u32 idx,
+ union v4l2_ctrl_ptr ptr)
+{
+ switch (ctrl->type) {
+ case V4L2_CTRL_TYPE_STRING:
+ idx *= ctrl->elem_size;
+ memset(ptr.p_char + idx, ' ', ctrl->maximum);
+ ptr.p_char[idx + ctrl->maximum] = '\0';
+ break;
+ case V4L2_CTRL_TYPE_INTEGER64:
+ ptr.p_s64[idx] = ctrl->maximum;
+ break;
+ case V4L2_CTRL_TYPE_INTEGER:
+ case V4L2_CTRL_TYPE_INTEGER_MENU:
+ case V4L2_CTRL_TYPE_MENU:
+ case V4L2_CTRL_TYPE_BITMASK:
+ case V4L2_CTRL_TYPE_BOOLEAN:
+ ptr.p_s32[idx] = ctrl->maximum;
+ break;
+ case V4L2_CTRL_TYPE_BUTTON:
+ case V4L2_CTRL_TYPE_CTRL_CLASS:
+ ptr.p_s32[idx] = 0;
+ break;
+ case V4L2_CTRL_TYPE_U8:
+ ptr.p_u8[idx] = ctrl->maximum;
+ break;
+ case V4L2_CTRL_TYPE_U16:
+ ptr.p_u16[idx] = ctrl->maximum;
+ break;
+ case V4L2_CTRL_TYPE_U32:
+ ptr.p_u32[idx] = ctrl->maximum;
+ break;
+ default:
+ std_max_compound(ctrl, idx, ptr);
+ break;
+ }
+}
+
static void std_log(const struct v4l2_ctrl *ctrl)
{
union v4l2_ctrl_ptr ptr = ctrl->p_cur;
@@ -986,6 +1084,8 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
static const struct v4l2_ctrl_type_ops std_type_ops = {
.equal = std_equal,
.init = std_init,
+ .minimum = std_minimum,
+ .maximum = std_maximum,
.log = std_log,
.validate = std_validate,
};
@@ -1369,7 +1469,10 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
s64 min, s64 max, u64 step, s64 def,
const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
u32 flags, const char * const *qmenu,
- const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
+ const s64 *qmenu_int,
+ const union v4l2_ctrl_ptr p_def,
+ const union v4l2_ctrl_ptr p_min,
+ const union v4l2_ctrl_ptr p_max,
void *priv)
{
struct v4l2_ctrl *ctrl;
@@ -1516,7 +1619,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
sz_extra += 2 * tot_ctrl_size;
if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p_const)
- sz_extra += elem_size;
+ sz_extra += elem_size * 3;
ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
if (ctrl == NULL) {
@@ -1566,6 +1669,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
memcpy(ctrl->p_def.p, p_def.p_const, elem_size);
}
+ if (type >= V4L2_CTRL_COMPOUND_TYPES &&
+ p_min.p_const && p_max.p_const) {
+ ctrl->p_min.p = ctrl->p_cur.p + 2 * tot_ctrl_size;
+ memcpy(ctrl->p_min.p, p_min.p_const, elem_size);
+ ctrl->p_max.p = ctrl->p_cur.p + 3 * tot_ctrl_size;
+ memcpy(ctrl->p_max.p, p_max.p_const, elem_size);
+ }
for (idx = 0; idx < elems; idx++) {
ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
@@ -1618,7 +1728,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
type, min, max,
is_menu ? cfg->menu_skip_mask : step, def,
cfg->dims, cfg->elem_size,
- flags, qmenu, qmenu_int, cfg->p_def, priv);
+ flags, qmenu, qmenu_int, cfg->p_def, cfg->p_min,
+ cfg->p_max, priv);
if (ctrl)
ctrl->is_private = cfg->is_private;
return ctrl;
@@ -1643,7 +1754,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
min, max, step, def, NULL, 0,
- flags, NULL, NULL, ptr_null, NULL);
+ flags, NULL, NULL, ptr_null, ptr_null,
+ ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std);
@@ -1676,7 +1788,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
- flags, qmenu, qmenu_int, ptr_null, NULL);
+ flags, qmenu, qmenu_int, ptr_null, ptr_null,
+ ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
@@ -1708,7 +1821,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
- flags, qmenu, NULL, ptr_null, NULL);
+ flags, qmenu, NULL, ptr_null, ptr_null,
+ ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
@@ -1716,7 +1830,9 @@ EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
/* Helper function for standard compound controls */
struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
const struct v4l2_ctrl_ops *ops, u32 id,
- const union v4l2_ctrl_ptr p_def)
+ const union v4l2_ctrl_ptr p_def,
+ const union v4l2_ctrl_ptr p_min,
+ const union v4l2_ctrl_ptr p_max)
{
const char *name;
enum v4l2_ctrl_type type;
@@ -1730,7 +1846,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
min, max, step, def, NULL, 0,
- flags, NULL, NULL, p_def, NULL);
+ flags, NULL, NULL, p_def, p_min, p_max, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
@@ -1754,7 +1870,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, 0, def, NULL, 0,
- flags, NULL, qmenu_int, ptr_null, NULL);
+ flags, NULL, qmenu_int, ptr_null, ptr_null,
+ ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 96e307fe3aab..6ed6ef87c7ff 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -892,7 +892,9 @@ static bool check_ext_ctrls(struct v4l2_ext_controls *c, unsigned long ioctl)
return false;
break;
case V4L2_CTRL_WHICH_DEF_VAL:
- /* Default value cannot be changed */
+ case V4L2_CTRL_WHICH_MIN_VAL:
+ case V4L2_CTRL_WHICH_MAX_VAL:
+ /* Default, minimum or maximum value cannot be changed */
if (ioctl == VIDIOC_S_EXT_CTRLS ||
ioctl == VIDIOC_TRY_EXT_CTRLS) {
c->error_idx = c->count;
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 919e104de50b..25378304b2fb 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -131,6 +131,8 @@ struct v4l2_ctrl_ops {
*
* @equal: return true if both values are equal.
* @init: initialize the value.
+ * @minimum: set the value to the minimum value of the control.
+ * @maximum: set the value to the maximum value of the control.
* @log: log the value.
* @validate: validate the value. Return 0 on success and a negative value
* otherwise.
@@ -141,6 +143,10 @@ struct v4l2_ctrl_type_ops {
union v4l2_ctrl_ptr ptr2);
void (*init)(const struct v4l2_ctrl *ctrl, u32 idx,
union v4l2_ctrl_ptr ptr);
+ void (*minimum)(const struct v4l2_ctrl *ctrl, u32 idx,
+ union v4l2_ctrl_ptr ptr);
+ void (*maximum)(const struct v4l2_ctrl *ctrl, u32 idx,
+ union v4l2_ctrl_ptr ptr);
void (*log)(const struct v4l2_ctrl *ctrl);
int (*validate)(const struct v4l2_ctrl *ctrl, u32 idx,
union v4l2_ctrl_ptr ptr);
@@ -237,6 +243,12 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
* @p_def: The control's default value represented via a union which
* provides a standard way of accessing control types
* through a pointer (for compound controls only).
+ * @p_min: The control's minimum value represented via a union which
+ * provides a standard way of accessing control types
+ * through a pointer (for compound controls only).
+ * @p_max: The control's maximum value represented via a union which
+ * provides a standard way of accessing control types
+ * through a pointer (for compound controls only).
* @p_cur: The control's current value represented via a union which
* provides a standard way of accessing control types
* through a pointer.
@@ -292,6 +304,8 @@ struct v4l2_ctrl {
} cur;
union v4l2_ctrl_ptr p_def;
+ union v4l2_ctrl_ptr p_min;
+ union v4l2_ctrl_ptr p_max;
union v4l2_ctrl_ptr p_new;
union v4l2_ctrl_ptr p_cur;
};
@@ -398,6 +412,8 @@ struct v4l2_ctrl_handler {
* @step: The control's step value for non-menu controls.
* @def: The control's default value.
* @p_def: The control's default value for compound controls.
+ * @p_min: The control's minimum value for compound controls.
+ * @p_max: The control's maximum value for compound controls.
* @dims: The size of each dimension.
* @elem_size: The size in bytes of the control.
* @flags: The control's flags.
@@ -427,6 +443,8 @@ struct v4l2_ctrl_config {
u64 step;
s64 def;
union v4l2_ctrl_ptr p_def;
+ union v4l2_ctrl_ptr p_min;
+ union v4l2_ctrl_ptr p_max;
u32 dims[V4L2_CTRL_MAX_DIMS];
u32 elem_size;
u32 flags;
@@ -696,17 +714,21 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
* @ops: The control ops.
* @id: The control ID.
* @p_def: The control's default value.
+ * @p_min: The control's default value.
+ * @p_max: The control's default value.
*
* Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks
- * to the @p_def field. Use v4l2_ctrl_ptr_create() to create @p_def from a
- * pointer. Use v4l2_ctrl_ptr_create(NULL) if the default value of the
- * compound control should be all zeroes.
+ * to the @p_def/min/max field. Use v4l2_ctrl_ptr_create() to create
+ * @p_def/min/max from a pointer. Use v4l2_ctrl_ptr_create(NULL) if the default
+ * value of the compound control should be all zeroes.
*
*/
struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
const struct v4l2_ctrl_ops *ops,
u32 id,
- const union v4l2_ctrl_ptr p_def);
+ const union v4l2_ctrl_ptr p_def,
+ const union v4l2_ctrl_ptr p_min,
+ const union v4l2_ctrl_ptr p_max);
/**
* v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control.
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index b712412cf763..d22ebb0102d4 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1793,6 +1793,8 @@ struct v4l2_ext_controls {
#define V4L2_CTRL_WHICH_CUR_VAL 0
#define V4L2_CTRL_WHICH_DEF_VAL 0x0f000000
#define V4L2_CTRL_WHICH_REQUEST_VAL 0x0f010000
+#define V4L2_CTRL_WHICH_MIN_VAL 0x0f020000
+#define V4L2_CTRL_WHICH_MAX_VAL 0x0f030000
enum v4l2_ctrl_type {
V4L2_CTRL_TYPE_INTEGER = 1,
--
2.36.0.550.gb090851708-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v3 4/6] media: uvcvideo: implement UVC v1.5 ROI
2022-05-18 6:24 [PATCH v3 0/6] media: Implement UVC v1.5 ROI Yunke Cao
` (2 preceding siblings ...)
2022-05-18 6:24 ` [PATCH v3 3/6] media: v4l2_ctrl: Add V4L2_CTRL_WHICH_MIN/MAX_VAL Yunke Cao
@ 2022-05-18 6:24 ` Yunke Cao
2022-05-18 6:24 ` [PATCH v3 5/6] media: uvcvideo: Initialize roi to default value Yunke Cao
` (2 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Yunke Cao @ 2022-05-18 6:24 UTC (permalink / raw)
To: Hans Verkuil, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media, Yunke Cao
Supports GET_CUR, GET_DEF, GET_MIN and GET_MAX requests for UVC v1.5
using V4L2 control API.
References a rejected attempt that uses v4l2 selection API:
https://lore.kernel.org/lkml/20210501082001.100533-2-senozhatsky@chromium.org
Signed-off-by: Yunke Cao <yunkec@google.com>
---
Changelog since v2:
- Add UVC_CTRL_FLAG_AUTO_UPDATE flag.
.../userspace-api/media/drivers/uvcvideo.rst | 1 +
drivers/media/usb/uvc/uvc_ctrl.c | 174 ++++++++++++++++--
drivers/media/usb/uvc/uvc_v4l2.c | 12 +-
drivers/media/usb/uvc/uvcvideo.h | 10 +-
include/uapi/linux/usb/video.h | 1 +
include/uapi/linux/uvcvideo.h | 1 +
6 files changed, 174 insertions(+), 25 deletions(-)
diff --git a/Documentation/userspace-api/media/drivers/uvcvideo.rst b/Documentation/userspace-api/media/drivers/uvcvideo.rst
index e5fd8fad333c..43b8431b0d9f 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
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index b4f6edf968bc..0abe174231ab 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -355,6 +355,16 @@ 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
+ | UVC_CTRL_FLAG_AUTO_UPDATE,
+ },
};
static const u32 uvc_control_classes[] = {
@@ -728,6 +738,24 @@ 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_RECT,
+ .entity = UVC_GUID_UVC_CAMERA,
+ .selector = UVC_CT_REGION_OF_INTEREST_CONTROL,
+ .size = 64,
+ .offset = 0,
+ .v4l2_type = V4L2_CTRL_TYPE_RECT,
+ .data_type = UVC_CTRL_DATA_TYPE_RECT,
+ },
+ {
+ .id = V4L2_CID_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,
+ },
};
/* ------------------------------------------------------------------------
@@ -749,6 +777,33 @@ static inline void uvc_clear_bit(u8 *data, int bit)
data[bit >> 3] &= ~(1 << (bit & 7));
}
+static void uvc_to_v4l2_rect(struct v4l2_rect *v4l2_rect,
+ const struct uvc_rect *uvc_rect)
+{
+ v4l2_rect->top = uvc_rect->top;
+ v4l2_rect->left = uvc_rect->left;
+ v4l2_rect->height = uvc_rect->bottom - uvc_rect->top + 1;
+ v4l2_rect->width = uvc_rect->right - uvc_rect->left + 1;
+}
+
+static int v4l2_to_uvc_rect(struct uvc_rect *uvc_rect,
+ const struct v4l2_rect *v4l2_rect)
+{
+ // Safely converts s32 and u32 to u16.
+ if (v4l2_rect->top > U16_MAX || v4l2_rect->top < 0 ||
+ v4l2_rect->left > U16_MAX || v4l2_rect->left < 0 ||
+ v4l2_rect->height > U16_MAX || v4l2_rect->width > U16_MAX ||
+ v4l2_rect->height + v4l2_rect->top - 1 > U16_MAX ||
+ v4l2_rect->width + v4l2_rect->left - 1 > U16_MAX)
+ return -ERANGE;
+
+ uvc_rect->top = v4l2_rect->top;
+ uvc_rect->left = v4l2_rect->left;
+ uvc_rect->bottom = v4l2_rect->height + v4l2_rect->top - 1;
+ uvc_rect->right = v4l2_rect->width + v4l2_rect->left - 1;
+ return 0;
+}
+
/* Extract the bit string specified by mapping->offset and mapping->size
* from the little-endian data stored at 'data' and return the result as
* a signed 32bit integer. Sign extension will be performed if the mapping
@@ -963,11 +1018,23 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
return value;
}
+static void __uvc_ctrl_get_v4l2_rect(struct uvc_control *ctrl,
+ struct uvc_control_mapping *mapping,
+ u32 id,
+ struct v4l2_rect *rect)
+{
+ struct uvc_rect *uvc_rect =
+ (struct uvc_rect *)(uvc_ctrl_data(ctrl, id)
+ + mapping->offset / 8);
+ uvc_to_v4l2_rect(rect, uvc_rect);
+}
+
static int __uvc_ctrl_get(struct uvc_video_chain *chain,
struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
- s32 *value)
+ struct v4l2_ext_control *xctrl)
{
int ret;
+ struct v4l2_rect v4l2_rect;
if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
return -EACCES;
@@ -993,8 +1060,17 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
ctrl->loaded = 1;
}
- *value = __uvc_ctrl_get_value(mapping,
+ switch (mapping->v4l2_type) {
+ case V4L2_CTRL_TYPE_RECT:
+ __uvc_ctrl_get_v4l2_rect(ctrl, mapping, UVC_CTRL_DATA_CURRENT,
+ &v4l2_rect);
+ return copy_to_user(xctrl->p_rect, &v4l2_rect,
+ sizeof(v4l2_rect));
+
+ default:
+ xctrl->value = __uvc_ctrl_get_value(mapping,
uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
+ }
return 0;
}
@@ -1104,13 +1180,14 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
__uvc_find_control(ctrl->entity, mapping->master_id,
&master_map, &master_ctrl, 0);
if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
- s32 val;
- int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
+ struct v4l2_ext_control xctrl;
+ int ret =
+ __uvc_ctrl_get(chain, master_ctrl, master_map, &xctrl);
if (ret < 0)
return ret;
- if (val != mapping->master_manual)
- v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
+ if (xctrl.value != mapping->master_manual)
+ v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
}
if (!ctrl->cached) {
@@ -1344,16 +1421,16 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
struct uvc_control_mapping *mapping = NULL;
struct uvc_control *ctrl = NULL;
u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
- s32 val = 0;
+ struct v4l2_ext_control xctrl;
__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
if (ctrl == NULL)
return;
- if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)
+ if (__uvc_ctrl_get(chain, ctrl, mapping, &xctrl) == 0)
changes |= V4L2_EVENT_CTRL_CH_VALUE;
- uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
+ uvc_ctrl_send_event(chain, handle, ctrl, mapping, xctrl.value, changes);
}
void uvc_ctrl_status_event(struct uvc_video_chain *chain,
@@ -1515,13 +1592,13 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
if (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL) {
struct v4l2_event ev;
u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
- s32 val = 0;
+ struct v4l2_ext_control xctrl;
- if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
+ if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &xctrl) == 0)
changes |= V4L2_EVENT_CTRL_CH_VALUE;
- uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, val,
- changes);
+ uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping,
+ xctrl.value, changes);
/* Mark the queue as active, allowing this initial
event to be accepted. */
sev->elems = elems;
@@ -1682,10 +1759,14 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
}
int uvc_ctrl_get(struct uvc_video_chain *chain,
- struct v4l2_ext_control *xctrl)
+ struct v4l2_ext_control *xctrl, u32 v4l2_which)
{
struct uvc_control *ctrl;
struct uvc_control_mapping *mapping;
+ int ret;
+ u32 flag;
+ u32 id;
+ u8 query;
if (__uvc_query_v4l2_class(chain, xctrl->id, 0) >= 0)
return -EACCES;
@@ -1694,7 +1775,46 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
if (ctrl == NULL)
return -EINVAL;
- return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
+ switch (v4l2_which) {
+ case V4L2_CTRL_WHICH_DEF_VAL:
+ flag = UVC_CTRL_FLAG_GET_DEF;
+ id = UVC_CTRL_DATA_DEF;
+ query = UVC_GET_DEF;
+ break;
+ case V4L2_CTRL_WHICH_MIN_VAL:
+ flag = UVC_CTRL_FLAG_GET_MIN;
+ id = UVC_CTRL_DATA_MIN;
+ query = UVC_GET_MIN;
+ break;
+ case V4L2_CTRL_WHICH_MAX_VAL:
+ flag = UVC_CTRL_FLAG_GET_MAX;
+ id = UVC_CTRL_DATA_MAX;
+ query = UVC_GET_MAX;
+ break;
+ case V4L2_CTRL_WHICH_CUR_VAL:
+ default:
+ return __uvc_ctrl_get(chain, ctrl, mapping, xctrl);
+ }
+
+ if (!ctrl->cached) {
+ ret = uvc_ctrl_populate_cache(chain, ctrl);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (!(ctrl->info.flags & flag))
+ return -EACCES;
+
+ if (mapping->v4l2_type == V4L2_CTRL_TYPE_RECT) {
+ struct v4l2_rect rect;
+
+ __uvc_ctrl_get_v4l2_rect(ctrl, mapping, id, &rect);
+ return copy_to_user(xctrl->p_rect, &rect, sizeof(rect));
+ }
+
+ xctrl->value = mapping->get(mapping, query, uvc_ctrl_data(ctrl, id));
+
+ return 0;
}
int uvc_ctrl_set(struct uvc_fh *handle,
@@ -1703,6 +1823,8 @@ int uvc_ctrl_set(struct uvc_fh *handle,
struct uvc_video_chain *chain = handle->chain;
struct uvc_control *ctrl;
struct uvc_control_mapping *mapping;
+ struct v4l2_rect v4l2_rect;
+ struct uvc_rect uvc_rect;
s32 value;
u32 step;
s32 min;
@@ -1774,6 +1896,16 @@ int uvc_ctrl_set(struct uvc_fh *handle,
break;
+ case V4L2_CTRL_TYPE_RECT:
+ ret = copy_from_user(&v4l2_rect, xctrl->p_rect,
+ sizeof(v4l2_rect));
+ if (ret < 0)
+ return ret;
+ ret = v4l2_to_uvc_rect(&uvc_rect, &v4l2_rect);
+ if (ret < 0)
+ return ret;
+ break;
+
default:
value = xctrl->value;
break;
@@ -1807,8 +1939,16 @@ int uvc_ctrl_set(struct uvc_fh *handle,
ctrl->info.size);
}
- mapping->set(mapping, value,
- uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
+ switch (mapping->data_type) {
+ case UVC_CTRL_DATA_TYPE_RECT:
+ memcpy(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)
+ + mapping->offset / 8,
+ &uvc_rect, sizeof(uvc_rect));
+ break;
+ default:
+ mapping->set(mapping, value,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
+ }
if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
ctrl->handle = handle;
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 711556d13d03..a88d3fe6de93 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1038,17 +1038,15 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
if (ret < 0)
return ret;
- if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL ||
+ ctrls->which == V4L2_CTRL_WHICH_MIN_VAL ||
+ ctrls->which == V4L2_CTRL_WHICH_MAX_VAL) {
for (i = 0; i < ctrls->count; ++ctrl, ++i) {
- struct v4l2_queryctrl qc = { .id = ctrl->id };
-
- ret = uvc_query_v4l2_ctrl(chain, &qc);
+ ret = uvc_ctrl_get(chain, ctrl, ctrls->which);
if (ret < 0) {
ctrls->error_idx = i;
return ret;
}
-
- ctrl->value = qc.default_value;
}
return 0;
@@ -1059,7 +1057,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
return ret;
for (i = 0; i < ctrls->count; ++ctrl, ++i) {
- ret = uvc_ctrl_get(chain, ctrl);
+ ret = uvc_ctrl_get(chain, ctrl, ctrls->which);
if (ret < 0) {
uvc_ctrl_rollback(handle);
ctrls->error_idx = i;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 143230b3275b..f414ad7d57b2 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -424,6 +424,13 @@ struct uvc_streaming_header {
u8 bTriggerUsage;
};
+struct uvc_rect {
+ u16 top;
+ u16 left;
+ u16 bottom;
+ u16 right;
+} __packed;
+
enum uvc_buffer_state {
UVC_BUF_STATE_IDLE = 0,
UVC_BUF_STATE_QUEUED = 1,
@@ -897,7 +904,8 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
return __uvc_ctrl_commit(handle, 1, NULL);
}
-int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
+int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl,
+ u32 v4l2_which);
int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
bool read);
diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
index bfdae12cdacf..9076a444758a 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/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index 8288137387c0..d7d77602a5e7 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -16,6 +16,7 @@
#define UVC_CTRL_DATA_TYPE_BOOLEAN 3
#define UVC_CTRL_DATA_TYPE_ENUM 4
#define UVC_CTRL_DATA_TYPE_BITMASK 5
+#define UVC_CTRL_DATA_TYPE_RECT 6
/* Control flags */
#define UVC_CTRL_FLAG_SET_CUR (1 << 0)
--
2.36.0.550.gb090851708-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v3 5/6] media: uvcvideo: Initialize roi to default value
2022-05-18 6:24 [PATCH v3 0/6] media: Implement UVC v1.5 ROI Yunke Cao
` (3 preceding siblings ...)
2022-05-18 6:24 ` [PATCH v3 4/6] media: uvcvideo: implement UVC v1.5 ROI Yunke Cao
@ 2022-05-18 6:24 ` Yunke Cao
2022-05-18 6:24 ` [PATCH v3 6/6] media: vivid: Add a roi rectangle control Yunke Cao
2022-05-19 18:29 ` [PATCH v3 0/6] media: Implement UVC v1.5 ROI Nicolas Dufresne
6 siblings, 0 replies; 16+ messages in thread
From: Yunke Cao @ 2022-05-18 6:24 UTC (permalink / raw)
To: Hans Verkuil, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media, Yunke Cao
For the devices we tested with, firmwares have wrong initial
ROI configuration. Initialize roi by setting to default value.
Signed-off-by: Yunke Cao <yunkec@google.com>
---
drivers/media/usb/uvc/uvc_ctrl.c | 40 ++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 0abe174231ab..f7aa97aa3772 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2506,6 +2506,40 @@ static void uvc_ctrl_prune_entity(struct uvc_device *dev,
}
}
+static int uvc_ctrl_init_roi(struct uvc_device *dev, struct uvc_control *ctrl)
+{
+ int ret;
+
+ ret = uvc_query_ctrl(dev, UVC_GET_DEF, ctrl->entity->id, dev->intfnum,
+ UVC_CT_REGION_OF_INTEREST_CONTROL,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
+ ctrl->info.size);
+ if (ret)
+ goto out;
+
+ /*
+ * Some firmwares have wrong GET_CURRENT configuration. E.g. it's
+ * below GET_MIN, or have rectangle coordinates mixed up. This
+ * causes problems sometimes, because we are unable to set
+ * auto-controls value without first setting ROI rectangle to
+ * valid configuration.
+ *
+ * We expect that default configuration is always correct and
+ * is within the GET_MIN / GET_MAX boundaries.
+ *
+ * Set current ROI configuration to GET_DEF, so that we will
+ * always have properly configured ROI.
+ */
+ ret = uvc_query_ctrl(dev, UVC_SET_CUR, 1, dev->intfnum,
+ UVC_CT_REGION_OF_INTEREST_CONTROL,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF),
+ ctrl->info.size);
+out:
+ if (ret)
+ dev_err(&dev->udev->dev, "Failed to fixup ROI (%d).\n", ret);
+ return ret;
+}
+
/*
* Add control information and hardcoded stock control mappings to the given
* device.
@@ -2518,6 +2552,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
const struct uvc_control_mapping *mapping = uvc_ctrl_mappings;
const struct uvc_control_mapping *mend =
mapping + ARRAY_SIZE(uvc_ctrl_mappings);
+ const u8 camera_entity[16] = UVC_GUID_UVC_CAMERA;
/* XU controls initialization requires querying the device for control
* information. As some buggy UVC devices will crash when queried
@@ -2538,6 +2573,11 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain,
* GET_INFO on standard controls.
*/
uvc_ctrl_get_flags(chain->dev, ctrl, &ctrl->info);
+
+ if (ctrl->info.selector ==
+ UVC_CT_REGION_OF_INTEREST_CONTROL &&
+ uvc_entity_match_guid(ctrl->entity, camera_entity))
+ uvc_ctrl_init_roi(chain->dev, ctrl);
break;
}
}
--
2.36.0.550.gb090851708-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v3 6/6] media: vivid: Add a roi rectangle control
2022-05-18 6:24 [PATCH v3 0/6] media: Implement UVC v1.5 ROI Yunke Cao
` (4 preceding siblings ...)
2022-05-18 6:24 ` [PATCH v3 5/6] media: uvcvideo: Initialize roi to default value Yunke Cao
@ 2022-05-18 6:24 ` Yunke Cao
2022-05-19 8:29 ` Hans Verkuil
2022-05-19 18:29 ` [PATCH v3 0/6] media: Implement UVC v1.5 ROI Nicolas Dufresne
6 siblings, 1 reply; 16+ messages in thread
From: Yunke Cao @ 2022-05-18 6:24 UTC (permalink / raw)
To: Hans Verkuil, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media, Yunke Cao
The control supports current, default, minimum and maximum.
Tested by calling ioctls from the user space.
Signed-off-by: Yunke Cao <yunkec@google.com>
---
.../media/test-drivers/vivid/vivid-ctrls.c | 34 +++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
index e7516dc1227b..79093882d386 100644
--- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
+++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
@@ -34,6 +34,7 @@
#define VIVID_CID_U8_4D_ARRAY (VIVID_CID_CUSTOM_BASE + 10)
#define VIVID_CID_AREA (VIVID_CID_CUSTOM_BASE + 11)
#define VIVID_CID_RO_INTEGER (VIVID_CID_CUSTOM_BASE + 12)
+#define VIVID_CID_RECT (VIVID_CID_CUSTOM_BASE + 13)
#define VIVID_CID_VIVID_BASE (0x00f00000 | 0xf000)
#define VIVID_CID_VIVID_CLASS (0x00f00000 | 1)
@@ -292,6 +293,38 @@ static const struct v4l2_ctrl_config vivid_ctrl_area = {
.p_def.p_const = &area,
};
+static const struct v4l2_rect def_rect = {
+ .left = 0,
+ .top = 0,
+ .width = 1000,
+ .height = 2000,
+};
+
+static const struct v4l2_rect min_rect = {
+ .left = 0,
+ .top = 0,
+ .width = 1,
+ .height = 2,
+};
+
+static const struct v4l2_rect max_rect = {
+ .left = 0,
+ .top = 0,
+ .width = 2000,
+ .height = 4000,
+};
+
+static const struct v4l2_ctrl_config vivid_ctrl_rect = {
+ .ops = &vivid_user_gen_ctrl_ops,
+ .id = VIVID_CID_RECT,
+ .name = "Region of Interest Rectangle",
+ .type = V4L2_CTRL_TYPE_RECT,
+ .p_def.p_const = &def_rect,
+ .p_min.p_const = &min_rect,
+ .p_max.p_const = &max_rect,
+};
+
+
static const struct v4l2_ctrl_config vivid_ctrl_ro_int32 = {
.ops = &vivid_user_gen_ctrl_ops,
.id = VIVID_CID_RO_INTEGER,
@@ -1611,6 +1644,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
dev->int_menu = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_int_menu, NULL);
dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
+ v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_rect, NULL);
v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
--
2.36.0.550.gb090851708-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 6/6] media: vivid: Add a roi rectangle control
2022-05-18 6:24 ` [PATCH v3 6/6] media: vivid: Add a roi rectangle control Yunke Cao
@ 2022-05-19 8:29 ` Hans Verkuil
0 siblings, 0 replies; 16+ messages in thread
From: Hans Verkuil @ 2022-05-19 8:29 UTC (permalink / raw)
To: Yunke Cao, Laurent Pinchart, Nicolas Dufresne
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media
On 5/18/22 08:24, Yunke Cao wrote:
> The control supports current, default, minimum and maximum.
>
> Tested by calling ioctls from the user space.
>
> Signed-off-by: Yunke Cao <yunkec@google.com>
> ---
> .../media/test-drivers/vivid/vivid-ctrls.c | 34 +++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/media/test-drivers/vivid/vivid-ctrls.c b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> index e7516dc1227b..79093882d386 100644
> --- a/drivers/media/test-drivers/vivid/vivid-ctrls.c
> +++ b/drivers/media/test-drivers/vivid/vivid-ctrls.c
> @@ -34,6 +34,7 @@
> #define VIVID_CID_U8_4D_ARRAY (VIVID_CID_CUSTOM_BASE + 10)
> #define VIVID_CID_AREA (VIVID_CID_CUSTOM_BASE + 11)
> #define VIVID_CID_RO_INTEGER (VIVID_CID_CUSTOM_BASE + 12)
> +#define VIVID_CID_RECT (VIVID_CID_CUSTOM_BASE + 13)
>
> #define VIVID_CID_VIVID_BASE (0x00f00000 | 0xf000)
> #define VIVID_CID_VIVID_CLASS (0x00f00000 | 1)
> @@ -292,6 +293,38 @@ static const struct v4l2_ctrl_config vivid_ctrl_area = {
> .p_def.p_const = &area,
> };
>
> +static const struct v4l2_rect def_rect = {
> + .left = 0,
> + .top = 0,
> + .width = 1000,
> + .height = 2000,
> +};
> +
> +static const struct v4l2_rect min_rect = {
> + .left = 0,
> + .top = 0,
> + .width = 1,
> + .height = 2,
> +};
> +
> +static const struct v4l2_rect max_rect = {
> + .left = 0,
> + .top = 0,
> + .width = 2000,
> + .height = 4000,
> +};
The max rect will need to be updated whenever the format changes since
it depends on the image size.
I would also add the V4L2_CID_REGION_OF_INTEREST_AUTO control. It wouldn't
actually do anything, but it makes for a more realistic example.
BTW, is V4L2_CID_REGION_OF_INTEREST_AUTO a required control if you have the
V4L2_CID_REGION_OF_INTEREST_RECT control? It's not clear from the documentation.
I think it should be a required control: if set to 0, then the ROI would be
unused.
I also feel that this control should be an array of ROIs. It is AFAIK not
uncommon for hardware to support multiple ROIs. The dynamic control array
patches that are part of e.g. this series:
https://patchwork.linuxtv.org/project/linux-media/cover/20220503093925.876640-1-xavier.roumegue@oss.nxp.com/
may help with that.
Note that dynamic array support is still not merged. It is used in at least two
outstanding patch series, but neither one is ready yet to be merged. For the use
of ROI it might be interesting if the dynamic array support would allow for empty
arrays, but that's not implemented at the moment (the minimum number of elements
is 1).
If it is an array, then should INTEREST_AUTO become an array as well? Or should
the bitmask be incorporated into each ROI rectangle struct (i.e. it will no longer
be a v4l2_rect but a different struct).
I think incorporating this in the ROI struct would make more sense, that way it
is an atomic operation: here is the ROI, and here is what it is to be used for.
Apologies for the somewhat rambling email, it's in part a bit of brainstorming.
I think it might be better if you post an RFC with the API proposal, rather than
a patch series. The question here is what the public API should look like, how
generic it should be, what are the use-cases, etc.
Regards,
Hans
> +
> +static const struct v4l2_ctrl_config vivid_ctrl_rect = {
> + .ops = &vivid_user_gen_ctrl_ops,
> + .id = VIVID_CID_RECT,
> + .name = "Region of Interest Rectangle",
> + .type = V4L2_CTRL_TYPE_RECT,
> + .p_def.p_const = &def_rect,
> + .p_min.p_const = &min_rect,
> + .p_max.p_const = &max_rect,
> +};
> +
> +
> static const struct v4l2_ctrl_config vivid_ctrl_ro_int32 = {
> .ops = &vivid_user_gen_ctrl_ops,
> .id = VIVID_CID_RO_INTEGER,
> @@ -1611,6 +1644,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
> dev->int_menu = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_int_menu, NULL);
> dev->ro_int32 = v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_ro_int32, NULL);
> v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_area, NULL);
> + v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_rect, NULL);
> v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u32_array, NULL);
> v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u16_matrix, NULL);
> v4l2_ctrl_new_custom(hdl_user_gen, &vivid_ctrl_u8_4d_array, NULL);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/6] media: Implement UVC v1.5 ROI
2022-05-18 6:24 [PATCH v3 0/6] media: Implement UVC v1.5 ROI Yunke Cao
` (5 preceding siblings ...)
2022-05-18 6:24 ` [PATCH v3 6/6] media: vivid: Add a roi rectangle control Yunke Cao
@ 2022-05-19 18:29 ` Nicolas Dufresne
2022-05-19 23:55 ` Yunke Cao
6 siblings, 1 reply; 16+ messages in thread
From: Nicolas Dufresne @ 2022-05-19 18:29 UTC (permalink / raw)
To: Yunke Cao, Hans Verkuil, Laurent Pinchart
Cc: Tomasz Figa, Sergey Senozhatsky, Ricardo Ribalda, linux-media
Hi Yunke Cao,
Le mercredi 18 mai 2022 à 15:24 +0900, Yunke Cao a écrit :
> This patch set implements UVC v1.5 regioin of interest using V4L2
> control API.
>
> ROI control is consisted an auto control with type bitmask and a
> rectangle control with a newly added type V4L2_CTRL_TYPE_RECT.
>
> V4L2_CTRL_WHICH_MIN/MAX_VAL is added to support the rectangle control.
>
> A rectangle control is also added to the vivid test driver.
>
> Tested on two different usb cameras using v4l2-ctl and calling ioctls.
>
> Yunke Cao (6):
> media: v4l2_ctrl: Add region of interest rectangle control
> media: v4l2_ctrl: Add region of interest auto control
> media: v4l2_ctrl: Add V4L2_CTRL_WHICH_MIN/MAX_VAL
> media: uvcvideo: implement UVC v1.5 ROI
> media: uvcvideo: Initialize roi to default value
> media: vivid: Add a roi rectangle control
A little about best practices, consider adding a change log in your revisions,
so we know what you have changed (or document what you decided not to change).
Its also nice to wait for the review comment to settle before posting a new set,
it may appear rude otherwise.
kind regards,
Nicolas
>
> .../userspace-api/media/drivers/uvcvideo.rst | 1 +
> .../media/v4l/ext-ctrls-camera.rst | 49 ++++
> .../media/v4l/vidioc-g-ext-ctrls.rst | 12 +-
> .../media/videodev2.h.rst.exceptions | 3 +
> drivers/media/i2c/imx214.c | 4 +-
> .../media/test-drivers/vivid/vivid-ctrls.c | 34 +++
> drivers/media/usb/uvc/uvc_ctrl.c | 214 ++++++++++++++++--
> drivers/media/usb/uvc/uvc_v4l2.c | 12 +-
> drivers/media/usb/uvc/uvcvideo.h | 10 +-
> drivers/media/v4l2-core/v4l2-ctrls-api.c | 51 ++++-
> drivers/media/v4l2-core/v4l2-ctrls-core.c | 155 ++++++++++++-
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 7 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 4 +-
> include/media/v4l2-ctrls.h | 32 ++-
> include/uapi/linux/usb/video.h | 1 +
> include/uapi/linux/uvcvideo.h | 1 +
> include/uapi/linux/v4l2-controls.h | 11 +
> include/uapi/linux/videodev2.h | 4 +
> 18 files changed, 553 insertions(+), 52 deletions(-)
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 0/6] media: Implement UVC v1.5 ROI
2022-05-19 18:29 ` [PATCH v3 0/6] media: Implement UVC v1.5 ROI Nicolas Dufresne
@ 2022-05-19 23:55 ` Yunke Cao
0 siblings, 0 replies; 16+ messages in thread
From: Yunke Cao @ 2022-05-19 23:55 UTC (permalink / raw)
To: Nicolas Dufresne
Cc: Hans Verkuil, Laurent Pinchart, Tomasz Figa, Sergey Senozhatsky,
Ricardo Ribalda, linux-media
Thanks Nicolas!
Noted. I'm very new to the kernel community. Will do in the future :).
Best,
Yunke
On Fri, May 20, 2022 at 3:29 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Hi Yunke Cao,
>
> Le mercredi 18 mai 2022 à 15:24 +0900, Yunke Cao a écrit :
> > This patch set implements UVC v1.5 regioin of interest using V4L2
> > control API.
> >
> > ROI control is consisted an auto control with type bitmask and a
> > rectangle control with a newly added type V4L2_CTRL_TYPE_RECT.
> >
> > V4L2_CTRL_WHICH_MIN/MAX_VAL is added to support the rectangle control.
> >
> > A rectangle control is also added to the vivid test driver.
> >
> > Tested on two different usb cameras using v4l2-ctl and calling ioctls.
> >
> > Yunke Cao (6):
> > media: v4l2_ctrl: Add region of interest rectangle control
> > media: v4l2_ctrl: Add region of interest auto control
> > media: v4l2_ctrl: Add V4L2_CTRL_WHICH_MIN/MAX_VAL
> > media: uvcvideo: implement UVC v1.5 ROI
> > media: uvcvideo: Initialize roi to default value
> > media: vivid: Add a roi rectangle control
>
> A little about best practices, consider adding a change log in your revisions,
> so we know what you have changed (or document what you decided not to change).
> Its also nice to wait for the review comment to settle before posting a new set,
> it may appear rude otherwise.
>
> kind regards,
> Nicolas
>
> >
> > .../userspace-api/media/drivers/uvcvideo.rst | 1 +
> > .../media/v4l/ext-ctrls-camera.rst | 49 ++++
> > .../media/v4l/vidioc-g-ext-ctrls.rst | 12 +-
> > .../media/videodev2.h.rst.exceptions | 3 +
> > drivers/media/i2c/imx214.c | 4 +-
> > .../media/test-drivers/vivid/vivid-ctrls.c | 34 +++
> > drivers/media/usb/uvc/uvc_ctrl.c | 214 ++++++++++++++++--
> > drivers/media/usb/uvc/uvc_v4l2.c | 12 +-
> > drivers/media/usb/uvc/uvcvideo.h | 10 +-
> > drivers/media/v4l2-core/v4l2-ctrls-api.c | 51 ++++-
> > drivers/media/v4l2-core/v4l2-ctrls-core.c | 155 ++++++++++++-
> > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 7 +
> > drivers/media/v4l2-core/v4l2-ioctl.c | 4 +-
> > include/media/v4l2-ctrls.h | 32 ++-
> > include/uapi/linux/usb/video.h | 1 +
> > include/uapi/linux/uvcvideo.h | 1 +
> > include/uapi/linux/v4l2-controls.h | 11 +
> > include/uapi/linux/videodev2.h | 4 +
> > 18 files changed, 553 insertions(+), 52 deletions(-)
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread