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;
Message-ID: <5f7873743042cb302c2cb55cdb7ea5e9b26966f0.camel@ndufresne.ca>
Subject: Re: [PATCH WIP RFC v2 0/2] media: uapi: Add V4L2_CID_VTOTAL control
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
To:     Benjamin Bara <bbara93@gmail.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Manivannan Sadhasivam <mani@kernel.org>
Cc:     laurent.pinchart@ideasonboard.com, jacopo.mondi@ideasonboard.com,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Benjamin Bara <benjamin.bara@skidata.com>
Date:   Thu, 22 Jun 2023 11:14:41 -0400
In-Reply-To: <20230609-v4l2-vtotal-v2-0-cf29925f4517@skidata.com>
References: <20230609-v4l2-vtotal-v2-0-cf29925f4517@skidata.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

Hi,

Le jeudi 22 juin 2023 =C3=A0 09:12 +0200, Benjamin Bara a =C3=A9crit=C2=A0:
> Hi!
>=20
> This series adds new controls for the vertical and horizontal total
> size. Camera sensors usually have registers regarding the total size and
> not the blanking size. Also user space prefers to calculate with total
> frame sizes. Therefore, this would simplify implementations on both
> sides and could be seen as a replacement or upgrade for V4L2_CID_VBLANK.
> Additionally, its value is independent from format changes and therefore
> simplifies calculations in user space.

The naming is getting more and more generic. We need to find a way to give =
these
controls a name that associates them to their usage field (sensors). If we =
keep
growing the control list this way, in few years it will be terribly hard to
understand what is used for what.

What about, V4L2_CID_SENSOR_VTOTAL/HTOTAL ? I'm expecting the last bit of t=
he
name to come from very well known specification, otherwise, its not a great=
 name
either in my opinion.

>=20
> For v2, I added a little bit more documentation and my personal
> expectations to 1/2. As I am fairly new to the camera world, they might
> be a little bit naive so please correct me if this is utopical. I added
> the RFC tag for that reason. However, my intention is to define a
> documented behaviour regarding the values and limits which could
> basically depend on HTOTAL. Currently, HBLANK is always set to the
> minimum in libcamera (I guess to simplify calculations). I think with
> HTOTAL, we could always use WIDTH_MAX(all modes) + HBLANK_MIN(WIDTH_MAX)
> as default to enable a constant frame size. If the current HTOTAL value
> differs from the default, we could infer that the user wants to either
> have a higher frame rate, or a higher exposure and could adapt to that.
> E.g. use the minimums to get the highest frame rate possible if the
> current values are outside of the possible range, or not limiting the
> exposure control by the current vertical blanking as it is done now. I
> guess this would help the driver to "understand" the needs of the user
> space, and therefore allow it to react in a defined and expected manner.
>=20
> 2/2 is a WIP (same as in v1) and currently implements VTOTAL for the
> imx290, basically extending the current V4L2_CID_VBLANK implementation.
>=20
> Thanks Dave for the feedback, insights and the examples (ov5647,
> ov9282). I might need some time to skim through the data sheets to learn
> why they do stuff like it is done now.
>=20
> ---
> Changes in v2:
> - 1/2: add HTOTAL
> - 1/2: add documentation about expectations
> - Link to v1: https://lore.kernel.org/r/20230609-v4l2-vtotal-v1-0-4b7dee7=
e073e@skidata.com
>=20

Do you have an link to an example user of this new API ?

Nicolas

> ---
> Benjamin Bara (2):
>       media: uapi: Add V4L2_CID_{V,H}TOTAL controls
>       media: i2c: imx290: Add support for V4L2_CID_VTOTAL
>=20
>  Documentation/driver-api/media/camera-sensor.rst   | 11 ++++-
>  .../media/v4l/ext-ctrls-image-source.rst           | 16 ++++++++
>  drivers/media/i2c/imx290.c                         | 47 ++++++++++++++++=
------
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c          |  2 +
>  include/uapi/linux/v4l2-controls.h                 |  2 +
>  5 files changed, 64 insertions(+), 14 deletions(-)
> ---
> base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
> change-id: 20230609-v4l2-vtotal-eb15d37cafea
>=20
> Best regards,