diff mbox series

[PATCHv2,1/3] media: v4l UAPI docs: document ROI selection targets

Message ID 20210208051749.1785246-2-sergey.senozhatsky@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add UVC 1.5 Region Of Interest control to uvcvideo | expand

Commit Message

Sergey Senozhatsky Feb. 8, 2021, 5:17 a.m. UTC 
From: Sergey Senozhatsky <senozhatsky@chromium.org>

Document new v4l2-selection target which will be used for the
Region of Interest v4l2 control.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../media/v4l/selection-api-configuration.rst | 23 +++++++++++++++++++
 .../media/v4l/v4l2-selection-targets.rst      | 21 +++++++++++++++++
 include/uapi/linux/v4l2-common.h              |  8 +++++++
 3 files changed, 52 insertions(+)

Comments

Ricardo Ribalda Delgado March 16, 2021, 6:19 p.m. UTC | #1 
Hi Sergey

Thanks for the patch!

On Mon, Feb 8, 2021 at 6:21 AM Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
>
> From: Sergey Senozhatsky <senozhatsky@chromium.org>
>
> Document new v4l2-selection target which will be used for the
> Region of Interest v4l2 control.
>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  .../media/v4l/selection-api-configuration.rst | 23 +++++++++++++++++++
>  .../media/v4l/v4l2-selection-targets.rst      | 21 +++++++++++++++++
>  include/uapi/linux/v4l2-common.h              |  8 +++++++
>  3 files changed, 52 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
> index fee49bf1a1c0..9f69d71803f6 100644
> --- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
> +++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
> @@ -135,3 +135,26 @@ and the height of rectangles obtained using ``V4L2_SEL_TGT_CROP`` and
>  ``V4L2_SEL_TGT_COMPOSE`` targets. If these are not equal then the
>  scaling is applied. The application can compute the scaling ratios using
>  these values.
> +
> +Configuration of Region of Interest (ROI)
> +=========================================
> +
> +The range of coordinates of the top left corner, width and height of
> +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target.
> +It is recommended for the driver developers to put the top/left corner
> +at position ``(0,0)``. The rectangle's coordinates are in global sensor
> +coordinates. The units are in pixels and independent of the field of view.
> +They are not impacted by any cropping or scaling that is currently being
> +used.

Can we also mention binning here?

> +
> +The top left corner, width and height of the Region of Interest area
> +currently being employed by the device is given by the
> +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system
> +as ``V4L2_SEL_TGT_ROI_BOUNDS``.

Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ?

> +
> +In order to change active ROI top left, width and height coordinates
> +use ``V4L2_SEL_TGT_ROI`` target.
> +
> +Each capture device has a default ROI rectangle, given by the
> +``V4L2_SEL_TGT_ROI_DEFAULT`` target. Drivers shall set the ROI rectangle
> +to the default when the driver is first loaded, but not later.
> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> index e877ebbdb32e..cb3809418fa6 100644
> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> @@ -69,3 +69,24 @@ of the two interfaces they are used.
>         modified by hardware.
>        - Yes
>        - No
> +    * - ``V4L2_SEL_TGT_ROI_CURRENT``
> +      - 0x0200
> +      - Current Region of Interest rectangle.
> +      - Yes
> +      - No
> +    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
> +      - 0x0201
> +      - Suggested Region of Interest rectangle.
> +      - Yes
> +      - No
> +    * - ``V4L2_SEL_TGT_ROI_BOUNDS``
> +      - 0x0202
> +      - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit
> +       inside the ROI bounds rectangle.
> +      - Yes
> +      - No
> +    * - ``V4L2_SEL_TGT_ROI``
> +      - 0x0203
> +      - Sets the new Region of Interest rectangle.
> +      - Yes
> +      - No
As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI


> diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> index 7d21c1634b4d..d0c108fba638 100644
> --- a/include/uapi/linux/v4l2-common.h
> +++ b/include/uapi/linux/v4l2-common.h
> @@ -78,6 +78,14 @@
>  #define V4L2_SEL_TGT_COMPOSE_BOUNDS    0x0102
>  /* Current composing area plus all padding pixels */
>  #define V4L2_SEL_TGT_COMPOSE_PADDED    0x0103
> +/* Current Region of Interest area */
> +#define V4L2_SEL_TGT_ROI_CURRENT       0x0200
> +/* Default Region of Interest area */
> +#define V4L2_SEL_TGT_ROI_DEFAULT       0x0201
> +/* Region of Interest bounds */
> +#define V4L2_SEL_TGT_ROI_BOUNDS        0x0202
> +/* Set Region of Interest area */
> +#define V4L2_SEL_TGT_ROI               0x0203

Nit: Maybe it could be a good idea to split doc and code. This way the
backports/fixes are easier.

>
>  /* Selection flags */
>  #define V4L2_SEL_FLAG_GE               (1 << 0)
> --
> 2.30.0
>
Sergey Senozhatsky March 17, 2021, 1:31 a.m. UTC | #2 
On (21/03/16 19:19), Ricardo Ribalda Delgado wrote:
> > +Configuration of Region of Interest (ROI)
> > +=========================================
> > +
> > +The range of coordinates of the top left corner, width and height of
> > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target.
> > +It is recommended for the driver developers to put the top/left corner
> > +at position ``(0,0)``. The rectangle's coordinates are in global sensor
> > +coordinates. The units are in pixels and independent of the field of view.
> > +They are not impacted by any cropping or scaling that is currently being
> > +used.
> 
> Can we also mention binning here?

What's binning? Is it in the UVC spec?

> > +The top left corner, width and height of the Region of Interest area
> > +currently being employed by the device is given by the
> > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system
> > +as ``V4L2_SEL_TGT_ROI_BOUNDS``.
> 
> Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ?

We don't. Will remove it.

> > +    * - ``V4L2_SEL_TGT_ROI_CURRENT``
> > +      - 0x0200
> > +      - Current Region of Interest rectangle.
> > +      - Yes
> > +      - No
> > +    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
> > +      - 0x0201
> > +      - Suggested Region of Interest rectangle.
> > +      - Yes
> > +      - No
> > +    * - ``V4L2_SEL_TGT_ROI_BOUNDS``
> > +      - 0x0202
> > +      - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit
> > +       inside the ROI bounds rectangle.
> > +      - Yes
> > +      - No
> > +    * - ``V4L2_SEL_TGT_ROI``
> > +      - 0x0203
> > +      - Sets the new Region of Interest rectangle.
> > +      - Yes
> > +      - No
> As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI

Agreed.

> > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> > index 7d21c1634b4d..d0c108fba638 100644
> > --- a/include/uapi/linux/v4l2-common.h
> > +++ b/include/uapi/linux/v4l2-common.h
> > @@ -78,6 +78,14 @@
> >  #define V4L2_SEL_TGT_COMPOSE_BOUNDS    0x0102
> >  /* Current composing area plus all padding pixels */
> >  #define V4L2_SEL_TGT_COMPOSE_PADDED    0x0103
> > +/* Current Region of Interest area */
> > +#define V4L2_SEL_TGT_ROI_CURRENT       0x0200
> > +/* Default Region of Interest area */
> > +#define V4L2_SEL_TGT_ROI_DEFAULT       0x0201
> > +/* Region of Interest bounds */
> > +#define V4L2_SEL_TGT_ROI_BOUNDS        0x0202
> > +/* Set Region of Interest area */
> > +#define V4L2_SEL_TGT_ROI               0x0203
> 
> Nit: Maybe it could be a good idea to split doc and code. This way the
> backports/fixes are easier.

I'm quite sure this is the first time I'm being asked to split code
and documentation :) I'm usually asked to do the opposite - merge code
and documentation.
Ricardo Ribalda Delgado March 17, 2021, 8:04 a.m. UTC | #3 
Hi

On Wed, Mar 17, 2021 at 2:31 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (21/03/16 19:19), Ricardo Ribalda Delgado wrote:
> > > +Configuration of Region of Interest (ROI)
> > > +=========================================
> > > +
> > > +The range of coordinates of the top left corner, width and height of
> > > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target.
> > > +It is recommended for the driver developers to put the top/left corner
> > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor
> > > +coordinates. The units are in pixels and independent of the field of view.
> > > +They are not impacted by any cropping or scaling that is currently being
> > > +used.
> >
> > Can we also mention binning here?
>
> What's binning? Is it in the UVC spec?

Binning is when you reduce an image by adding up surrounding pixels.

So you have a 100x100 image that you convert to a 50x50 but showing
the same area of interest.


>
> > > +The top left corner, width and height of the Region of Interest area
> > > +currently being employed by the device is given by the
> > > +``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system
> > > +as ``V4L2_SEL_TGT_ROI_BOUNDS``.
> >
> > Why do we need current? Cant we just read back V4L2_SEL_TGT_ROI ?
>
> We don't. Will remove it.
>
> > > +    * - ``V4L2_SEL_TGT_ROI_CURRENT``
> > > +      - 0x0200
> > > +      - Current Region of Interest rectangle.
> > > +      - Yes
> > > +      - No
> > > +    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
> > > +      - 0x0201
> > > +      - Suggested Region of Interest rectangle.
> > > +      - Yes
> > > +      - No
> > > +    * - ``V4L2_SEL_TGT_ROI_BOUNDS``
> > > +      - 0x0202
> > > +      - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit
> > > +       inside the ROI bounds rectangle.
> > > +      - Yes
> > > +      - No
> > > +    * - ``V4L2_SEL_TGT_ROI``
> > > +      - 0x0203
> > > +      - Sets the new Region of Interest rectangle.
> > > +      - Yes
> > > +      - No
> > As mentioned before I think we should not have TGT_ROI_CURRENT and TGT_ROI
>
> Agreed.
>
> > > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> > > index 7d21c1634b4d..d0c108fba638 100644
> > > --- a/include/uapi/linux/v4l2-common.h
> > > +++ b/include/uapi/linux/v4l2-common.h
> > > @@ -78,6 +78,14 @@
> > >  #define V4L2_SEL_TGT_COMPOSE_BOUNDS    0x0102
> > >  /* Current composing area plus all padding pixels */
> > >  #define V4L2_SEL_TGT_COMPOSE_PADDED    0x0103
> > > +/* Current Region of Interest area */
> > > +#define V4L2_SEL_TGT_ROI_CURRENT       0x0200
> > > +/* Default Region of Interest area */
> > > +#define V4L2_SEL_TGT_ROI_DEFAULT       0x0201
> > > +/* Region of Interest bounds */
> > > +#define V4L2_SEL_TGT_ROI_BOUNDS        0x0202
> > > +/* Set Region of Interest area */
> > > +#define V4L2_SEL_TGT_ROI               0x0203
> >
> > Nit: Maybe it could be a good idea to split doc and code. This way the
> > backports/fixes are easier.
>
> I'm quite sure this is the first time I'm being asked to split code
> and documentation :) I'm usually asked to do the opposite - merge code
> and documentation.

I got answered in both directions.  I prefer to split it because the
doc can go to different audience than the code, and then it makes my
life easier when backporting.

But if you or Laurent prefer  otherwise I am of course happy with any option ;)
Sergey Senozhatsky March 17, 2021, 8:08 a.m. UTC | #4 
On (21/03/17 09:04), Ricardo Ribalda Delgado wrote:
> On Wed, Mar 17, 2021 at 2:31 AM Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> >
> > On (21/03/16 19:19), Ricardo Ribalda Delgado wrote:
> > > > +Configuration of Region of Interest (ROI)
> > > > +=========================================
> > > > +
> > > > +The range of coordinates of the top left corner, width and height of
> > > > +areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target.
> > > > +It is recommended for the driver developers to put the top/left corner
> > > > +at position ``(0,0)``. The rectangle's coordinates are in global sensor
> > > > +coordinates. The units are in pixels and independent of the field of view.
> > > > +They are not impacted by any cropping or scaling that is currently being
> > > > +used.
> > >
> > > Can we also mention binning here?
> >
> > What's binning? Is it in the UVC spec?
> 
> Binning is when you reduce an image by adding up surrounding pixels.
> 
> So you have a 100x100 image that you convert to a 50x50 but showing
> the same area of interest.

I see. Hmm, not sure if I can comment on this. It's not in the spec,
so it might be up to the firmware, maybe. What do you think?

> > > > diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
> > > > index 7d21c1634b4d..d0c108fba638 100644
> > > > --- a/include/uapi/linux/v4l2-common.h
> > > > +++ b/include/uapi/linux/v4l2-common.h
> > > > @@ -78,6 +78,14 @@
> > > >  #define V4L2_SEL_TGT_COMPOSE_BOUNDS    0x0102
> > > >  /* Current composing area plus all padding pixels */
> > > >  #define V4L2_SEL_TGT_COMPOSE_PADDED    0x0103
> > > > +/* Current Region of Interest area */
> > > > +#define V4L2_SEL_TGT_ROI_CURRENT       0x0200
> > > > +/* Default Region of Interest area */
> > > > +#define V4L2_SEL_TGT_ROI_DEFAULT       0x0201
> > > > +/* Region of Interest bounds */
> > > > +#define V4L2_SEL_TGT_ROI_BOUNDS        0x0202
> > > > +/* Set Region of Interest area */
> > > > +#define V4L2_SEL_TGT_ROI               0x0203
> > >
> > > Nit: Maybe it could be a good idea to split doc and code. This way the
> > > backports/fixes are easier.
> >
> > I'm quite sure this is the first time I'm being asked to split code
> > and documentation :) I'm usually asked to do the opposite - merge code
> > and documentation.
> 
> I got answered in both directions.  I prefer to split it because the
> doc can go to different audience than the code, and then it makes my
> life easier when backporting.
> 
> But if you or Laurent prefer  otherwise I am of course happy with any option ;)

Either way works for me. Laurent, any preferences?

	-ss
diff mbox series

Patch

diff --git a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
index fee49bf1a1c0..9f69d71803f6 100644
--- a/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
+++ b/Documentation/userspace-api/media/v4l/selection-api-configuration.rst
@@ -135,3 +135,26 @@  and the height of rectangles obtained using ``V4L2_SEL_TGT_CROP`` and
 ``V4L2_SEL_TGT_COMPOSE`` targets. If these are not equal then the
 scaling is applied. The application can compute the scaling ratios using
 these values.
+
+Configuration of Region of Interest (ROI)
+=========================================
+
+The range of coordinates of the top left corner, width and height of
+areas that can be ROI is given by the ``V4L2_SEL_TGT_ROI_BOUNDS`` target.
+It is recommended for the driver developers to put the top/left corner
+at position ``(0,0)``. The rectangle's coordinates are in global sensor
+coordinates. The units are in pixels and independent of the field of view.
+They are not impacted by any cropping or scaling that is currently being
+used.
+
+The top left corner, width and height of the Region of Interest area
+currently being employed by the device is given by the
+``V4L2_SEL_TGT_ROI_CURRENT`` target. It uses the same coordinate system
+as ``V4L2_SEL_TGT_ROI_BOUNDS``.
+
+In order to change active ROI top left, width and height coordinates
+use ``V4L2_SEL_TGT_ROI`` target.
+
+Each capture device has a default ROI rectangle, given by the
+``V4L2_SEL_TGT_ROI_DEFAULT`` target. Drivers shall set the ROI rectangle
+to the default when the driver is first loaded, but not later.
diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
index e877ebbdb32e..cb3809418fa6 100644
--- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
+++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
@@ -69,3 +69,24 @@  of the two interfaces they are used.
 	modified by hardware.
       - Yes
       - No
+    * - ``V4L2_SEL_TGT_ROI_CURRENT``
+      - 0x0200
+      - Current Region of Interest rectangle.
+      - Yes
+      - No
+    * - ``V4L2_SEL_TGT_ROI_DEFAULT``
+      - 0x0201
+      - Suggested Region of Interest rectangle.
+      - Yes
+      - No
+    * - ``V4L2_SEL_TGT_ROI_BOUNDS``
+      - 0x0202
+      - Bounds of the Region of Interest rectangle. All valid ROI rectangles fit
+	inside the ROI bounds rectangle.
+      - Yes
+      - No
+    * - ``V4L2_SEL_TGT_ROI``
+      - 0x0203
+      - Sets the new Region of Interest rectangle.
+      - Yes
+      - No
diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
index 7d21c1634b4d..d0c108fba638 100644
--- a/include/uapi/linux/v4l2-common.h
+++ b/include/uapi/linux/v4l2-common.h
@@ -78,6 +78,14 @@ 
 #define V4L2_SEL_TGT_COMPOSE_BOUNDS	0x0102
 /* Current composing area plus all padding pixels */
 #define V4L2_SEL_TGT_COMPOSE_PADDED	0x0103
+/* Current Region of Interest area */
+#define V4L2_SEL_TGT_ROI_CURRENT	0x0200
+/* Default Region of Interest area */
+#define V4L2_SEL_TGT_ROI_DEFAULT	0x0201
+/* Region of Interest bounds */
+#define V4L2_SEL_TGT_ROI_BOUNDS	0x0202
+/* Set Region of Interest area */
+#define V4L2_SEL_TGT_ROI		0x0203
 
 /* Selection flags */
 #define V4L2_SEL_FLAG_GE		(1 << 0)