Le lundi 16 mai 2022 à 23:04 +0900, Yunke Cao a écrit :
> Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxx>
> ---
> .../media/v4l/ext-ctrls-camera.rst | 25 +++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 2 ++
> include/uapi/linux/v4l2-controls.h | 9 +++++++
> 3 files changed, 36 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 86a1f09a8a1c..3da66e1e1fc7 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -665,3 +665,28 @@ enum v4l2_scene_mode -
> ``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.
> +
> +``V4L2_CID_REGION_OF_INTEREST_AUTO (bitmask)``
> + This determines which, if any, on board features should track to the
> + Region of Interest.
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_EXPOSURE``
> + - Auto Exposure.
> + * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS``
> + - Auto Iris.
> + * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE``
> + - Auto White Balance.
> + * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS``
> + - Auto Focus.
> + * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT``
> + - Auto Face Detect.
> + * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK``
> + - Auto Detect and Track.
> + * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION``
> + - Image Stabilization.
> + * - ``V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY``
> + - Higher Quality.
Now I see the usage, the control is missing cross-reference and behaviour.
Consider that someone may have to use or implement your feature on different HW
and in different context in the future. Right now you aren't writing a
specification, but barely listing things that are already encoded in the item
names. For each of this, add human readable prose that explain what is expected
behaviour when the bit is set. This way, future implementation can check their
behaviour and cross-over with the documentation to make sure it is a fit, or if
another bit need to be allocated.
I still believe REGION_OF_INTEREST is too generic of a name for the purpose, as
in the context of the V4L2 API, we also support video encoders, and none of this
(perhaps except QUALITY, but with encoder you have to specify a delta for that).
The name really needs to be specialized to be implemented this way. Otherwise,
it create confusion, and makes the V4L2 uAPI poorer over time. Naming is hard,
but I need to make a suggestion, perhaps CAMERA_ROI ? We have classes, perhaps a
class for the CAMERA controls is needed ?
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 95f39a2d2ad2..f28763bf95e9 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1043,6 +1043,7 @@ 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 +1416,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..f6938e4de129 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_CID_REGION_OF_INTEREST_AUTO_EXPOSURE (1 << 0)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IRIS (1 << 1)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_WHITE_BALANCE (1 << 2)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FOCUS (1 << 3)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_FACE_DETECT (1 << 4)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_DETECT_AND_TRACK (1 << 5)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_IMAGE_STABILIZATION (1 << 6)
> +#define V4L2_CID_REGION_OF_INTEREST_AUTO_HIGHER_QUALITY (1 << 7)
>
> /* FM Modulator class control IDs */
>
