Message-ID: <20251006011716.GD13055@pendragon.ideasonboard.com>
Date: Mon, 6 Oct 2025 04:17:16 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Dafna Hirschfeld <dafna@...tmail.com>, Keke Li <keke.li@...ogic.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Dan Scally <dan.scally@...asonboard.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Antoine Bouyer <antoine.bouyer@....com>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	linux-rockchip@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 4/8] media: Documentation: uapi: Add V4L2 extensible
 parameters

Hi Jacopo,

Thank you for the patch.

On Mon, Sep 15, 2025 at 07:18:13PM +0200, Jacopo Mondi wrote:
> Add documentation for extensible parameters format to the V4L2
> userspace API documentation.
> 
> Reviewed-by: Daniel Scally <dan.scally@...asonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> ---
>  .../media/v4l/extensible-parameters.rst            | 97 ++++++++++++++++++++++
>  .../userspace-api/media/v4l/meta-formats.rst       |  1 +
>  MAINTAINERS                                        |  1 +
>  3 files changed, 99 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/extensible-parameters.rst b/Documentation/userspace-api/media/v4l/extensible-parameters.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..e95c84f90c1b472360306d97c9b27123cd4bb6af
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/extensible-parameters.rst
> @@ -0,0 +1,97 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +
> +.. _extensible-parameters:
> +
> +*************************************
> +V4L2 extensible ISP parameters format
> +*************************************
> +
> +ISP configuration
> +=================
> +
> +ISP configuration parameters are computed by userspace and programmed into a
> +*parameters buffer* which is queued to the ISP driver on a per-frame basis. The
> +layout of the *parameters buffer* generally reflects the ISP peripheral
> +registers layout and is, for this reason, platform specific.

I wouldn't talk about registers here, you can keep this more generic:

The parameters buffer contains ISP-specific parameters, its layout is
therefore driver-specific.

Except that the goal of this API is to standardize the layout...

> +
> +The ISP configuration parameters are passed to the ISP driver through a metadata
> +output video node, using the :c:type:`v4l2_meta_format` interface. Each ISP
> +driver defines a metadata format that implements the configuration parameters
> +layout.
> +
> +Metadata output formats that describe ISP configuration parameters are most of
> +the time realized by implementing C structures that reflect the registers layout
> +and gets populated by userspace before queueing the buffer to the ISP. Each
> +C structure usually corresponds to one ISP *processing block*, with each block
> +implementing one of the ISP supported features.
> +
> +The uAPI/ABI problem
> +--------------------
> +
> +By upstreaming data types that describe the configuration parameters layout,
> +driver developers make them part of the Linux kernel ABI. As it sometimes
> +happens for most peripherals in Linux, ISP drivers development is often an
> +iterative process, where sometimes not all the hardware features are supported
> +in the first version that lands in the kernel, and some parts of the interface
> +have to later be modified for bug-fixes or improvements.
> +
> +If any later bug-fix/improvement requires changes to the metadata output format,
> +this is considered an ABI-breakage that is strictly forbidden by the Linux
> +kernel policies. For this reason, each new iteration of an ISP driver support
> +would require defining a new metadata output format, implying that drivers have
> +to be made ready to handle several different configuration formats.
> +
> +Support for generic ISP parameters buffer has been designed with the goal of
> +being:
> +
> +- Extensible: new features can be added later on without breaking the existing
> +  interface
> +- Versioned: different versions of the format can be defined without
> +  breaking the existing interface

All this sounds more like material for a commit message, although I
suppose some design rationale could make sense in the documentation.

> +
> +The extensible parameters format
> +================================
> +
> +Extensible configuration parameters formats are realized by a defining a single
> +C structure that contains a few control parameters and a binary buffer where

"control parameters" is confusing here.

> +userspace programs a variable number of *ISP configuration blocks* data.
> +
> +The generic :c:type:`v4l2_params_buffer` defines a base type that each driver
> +can use by properly sizing the data buffer array by providing a definition of
> +maximum supported parameters buffer size.
> +
> +Each *ISP configuration block* is identified by an header and contains the

s/an header/a header/

> +parameters for that specific block.
> +
> +The generic :c:type:`v4l2_params_block_header` defines a base type that each
> +driver can re-use as it is or extend appropriately.

I'm sorry, extend how ?

> +
> +Userspace applications are responsible for correctly populating the parameters
> +block header fields (type, flags and size) and the block-specific parameters.
> +
> +When userspace wants to configure and enable an ISP block it shall fully
> +populate the block configuration and set the V4L2_PARAMS_FL_BLOCK_ENABLE
> +bit in the flags field.
> +
> +When userspace simply wants to disable an ISP block the
> +V4L2_PARAMS_FL_BLOCK_DISABLE bit should be set in flags field. The driver
> +ignores the rest of the block configuration structure in this case.

The implementation accepts a header with no data in that case. That
should either be documented here, or not supported.

> +
> +If a new configuration of an ISP block has to be applied, userspace shall fully
> +populate the ISP block configuration and omit setting the
> +V4L2_PARAMS_FL_BLOCK_ENABLE and V4L2_PARAMS_FL_BLOCK_DISABLE bits in
> +the flags field.
> +
> +Setting both the V4L2_PARAMS_FL_BLOCK_ENABLE and V4L2_PARAMS_FL_BLOCK_DISABLE
> +bits in the flags field is not allowed and not accepted.
> +
> +Any further development that happens after the ISP driver has been merged in
> +Linux and which requires supporting new ISP features can be implemented by
> +adding new blocks definition without invalidating the existing ones. Similarly,
> +any change to the existing ISP configuration blocks can be handled by versioning
> +them, again without invalidating the existing ones.

That versioning is unrelated to the version field in the buffer header,
it would be implemented by defining a new block type. This should be
clarified here.

There's lots of room for improvement here, the documentation is very
hard to follow for someone who is not deeply familiar with the concepts
already. With the comments above addressed there won't be anything
fundamentally incorrect though, so this file could be rewritten later.

> +
> +V4L2 extensible parameters uAPI data types
> +==========================================
> +
> +.. kernel-doc:: include/uapi/linux/media/v4l2-isp.h
> diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst b/Documentation/userspace-api/media/v4l/meta-formats.rst
> index 0de80328c36bf148051a19abe9e5241234ddfe5c..b900ed6af7bd9ad49baf7b5a9eef9423f8abfbcb 100644
> --- a/Documentation/userspace-api/media/v4l/meta-formats.rst
> +++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
> @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata` interface only.
>  .. toctree::
>      :maxdepth: 1
>  
> +    extensible-parameters
>      metafmt-c3-isp
>      metafmt-d4xx
>      metafmt-generic
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e82c3d0758d6033fe8fcd56ffde2c03c4319fd11..abba872cb63f1430a49a2afbace4b9f9958c3991 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26414,6 +26414,7 @@ V4L2 GENERIC ISP PARAMETERS AND STATISTIC FORMATS
>  M:	Jacopo Mondi <jacopo.mondi@...asonboard.com>
>  L:	linux-media@...r.kernel.org
>  S:	Maintained
> +F:	Documentation/userspace-api/media/v4l/extensible-parameters.rst
>  F:	include/uapi/linux/media/v4l2-isp.h
>  
>  VF610 NAND DRIVER

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists