Thanks, I will add detailed behavior in the follow up version.
On Tue, May 17, 2022 at 10:23 PM Nicolas Dufresne <nicolas@xxxxxxxxxxxx> wrote:
>
> 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)
The ROI controls are in V4L2_CID_CAMERA_CLASS
"(V4L2_CID_CAMERA_CLASS_BASE+36)" unless I miss anything.
Correct me if I'm wrong but I don't see "CAMERA" included in the name
of other camera controls.
> > +#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 */
> >
>
