Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230609-v4l2-vtotal-v1-0-4b7dee7e073e@skidata.com> <20230609-v4l2-vtotal-v1-1-4b7dee7e073e@skidata.com>
In-Reply-To: <20230609-v4l2-vtotal-v1-1-4b7dee7e073e@skidata.com>
From:   Dave Stevenson <dave.stevenson@raspberrypi.com>
Date:   Fri, 9 Jun 2023 15:03:46 +0100
Message-ID: <CAPY8ntDsUPZvN0M_7pyRfERK9V0MCkD6pp7zu6G9nDWYHwSVcw@mail.gmail.com>
Subject: Re: [PATCH 1/2] media: uapi: Add V4L2_CID_VTOTAL control
To:     Benjamin Bara <bbara93@gmail.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Manivannan Sadhasivam <mani@kernel.org>,
        laurent.pinchart@ideasonboard.com, jacopo.mondi@ideasonboard.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Benjamin Bara <benjamin.bara@skidata.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Benjamin

Thanks for the patch.

On Fri, 9 Jun 2023 at 14:16, Benjamin Bara <bbara93@gmail.com> wrote:
>
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> Currently, V4L2_CID_VBLANK can be used to control the frame duration of
> a stream. However, camera sensors usually have a register for the
> vertical total size (image data + blanking), e.g. VMAX on the imx290.
> The dependency between format height and vertical blanking results to a
> change of the vertical blanking value and limits whenever the format of
> the frame is changed and therefore makes it harder for user space to do
> calculations, e.g. the frame duration.
>
> V4L2_CID_VTOTAL does not depend on the format and therefore simplifies
> calculations. Additionally, it represents the hardware "better" and
> therefore also simplifies calculations on the driver side.
>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst | 6 ++++++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c                        | 1 +
>  include/uapi/linux/v4l2-controls.h                               | 1 +
>  3 files changed, 8 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> index 71f23f131f97..e72d1363ad85 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> @@ -59,6 +59,12 @@ Image Source Control IDs
>      non-sensitive.
>      This control is required for automatic calibration of sensors/cameras.
>
> +``V4L2_CID_VTOTAL (integer)``
> +    Number of total lines per frame, including data and idle lines (blanking).
> +    The unit of the vertical total size is a line. Every line has length of the
> +    image width plus horizontal blanking at the pixel rate defined by
> +    ``V4L2_CID_PIXEL_RATE`` control in the same sub-device.
> +
>  .. c:type:: v4l2_area
>
>  .. flat-table:: struct v4l2_area

The documentation for Frame interval configuration on Raw camera
sensors[1] needs updating to describe the new behaviour as well.

  Dave

[1] https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/media/camera-sensor.rst#L80


> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 564fedee2c88..6a0d310d5f42 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1112,6 +1112,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>         case V4L2_CID_TEST_PATTERN_BLUE:        return "Blue Pixel Value";
>         case V4L2_CID_TEST_PATTERN_GREENB:      return "Green (Blue) Pixel Value";
>         case V4L2_CID_NOTIFY_GAINS:             return "Notify Gains";
> +       case V4L2_CID_VTOTAL:                   return "Vertical Total Size";
>
>         /* Image processing controls */
>         /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 5e80daa4ffe0..99120005634e 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1117,6 +1117,7 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_TEST_PATTERN_GREENB           (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
>  #define V4L2_CID_UNIT_CELL_SIZE                        (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
>  #define V4L2_CID_NOTIFY_GAINS                  (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 9)
> +#define V4L2_CID_VTOTAL                                (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 10)
>
>
>  /* Image processing controls */
>
> --
> 2.34.1
>