Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210208051749.1785246-1-sergey.senozhatsky@gmail.com> <20210208051749.1785246-2-sergey.senozhatsky@gmail.com>
In-Reply-To: <20210208051749.1785246-2-sergey.senozhatsky@gmail.com>
From:   Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Date:   Tue, 16 Mar 2021 19:19:57 +0100
Message-ID: <CAPybu_19hztQQEi0H40sWZQMb-X7g7dDuW4Mz8_gRv-nG2tghw@mail.gmail.com>
Subject: Re: [PATCHv2 1/3] media: v4l UAPI docs: document ROI selection targets
To:     Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Tomasz Figa <tomasz.figa@gmail.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media <linux-media@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>


-- 
Ricardo Ribalda