Message-ID: <e82e7c1d-b4ac-49a0-9b76-d101395c7040@collabora.com>
Date: Tue, 14 Oct 2025 11:23:29 +0200
From: Michael Riesch <michael.riesch@...labora.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>,
 Dafna Hirschfeld <dafna@...tmail.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.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>
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v7 4/8] media: Documentation: uapi: Add V4L2 ISP
 documentation

Hi Jacopo,

Thanks for your efforts!

On 10/14/25 10:00, Jacopo Mondi wrote:
> [...]
> +
> +The uAPI/ABI problem
> +--------------------
> +
> +By upstreaming the metadata formats that describe the parameters and statistics
> +buffers 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.

Suggestion:

As for most peripherals, ISP driver development in Linux is often an
iterative process, in which not all of the hardware features are
supported in the first version. The support for them and/or bug fixes
may land in the kernel at a later stage.

> +
> +If any later bug-fix/improvement requires changes to the metadata formats,

s/bug-fix/bug fix

> +this is considered an ABI-breakage that is strictly forbidden by the Linux

s/ABI-breakage/ABI breakage

> +kernel policies. For this reason, any change in the ISP parameters and
> +statistics buffer layout would require defining a new metadata format.
> +
> +For these reasons Video4Linux2 has introduced support for generic ISP parameters
> +and statistics data types, 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
> +
> +ISP configuration
> +=================
> +
> +Before the introduction of generic formats
> +------------------------------------------
> +
> +Metadata cature formats that describe ISP configuration parameters were most

s/cature/capture

s/most the time/"most of the time" or "typically" or "usually" or
"normally"?

> +the time realized by defining C structures that reflect the ISP registers layout
> +and gets populated by userspace before queueing the buffer to the ISP. Each

s/gets/get

> +C structure usually corresponds to one ISP *processing block*, with each block
> +implementing one of the ISP supported features.
> +
> +The number of supported ISP blocks, the layout of their configuration data are
> +fixed by the format definition, incurring the in the above described uAPI/uABI
> +problems.

incurring the described uAPI/ABI problems described above.

> +
> +Generic ISP parameters
> +----------------------
> +
> +The generic ISP configuration parameters format is realized by a defining a
> +single C structure that contains an header, followed by a binary buffer where

s/an header/a header

> +userspace programs a variable number of ISP configuration data block, one for
> +each supported ISP feature.
> +
> +The :c:type:`v4l2_isp_params_buffer` structure defines the parameters buffer
> +header which is followed by a binary buffer of ISP configuration parameters.
> +Userspace shall correctly populate the buffer header with the versioning
> +information and with the size (in bytes) of the binary data buffer where it will
> +store the ISP blocks configuration.
> +
> +Each *ISP configuration block* is preceded by an header implemented by the
> +:c:type:`v4l2_isp_params_block_header` structure, followed by the configuration
> +parameters for that specific block, defined by the ISP driver specific data
> +types.
> +
> +Userspace applications are responsible for correctly populating each block's
> +header fields (type, flags and size) and the block-specific parameters.
> +
> +ISP Block enabling, disabling and configuration
> +-----------------------------------------------
> +
> +When userspace wants to configure and enable an ISP block it shall fully
> +populate the block configuration and set the V4L2_ISP_PARAMS_FL_BLOCK_ENABLE
> +bit in the block header's `flags` field.
> +
> +When userspace simply wants to disable an ISP block the
> +V4L2_ISP_PARAMS_FL_BLOCK_DISABLE bit should be set in block header's `flags`
> +field. Drivers accept a configuration parameters block with no additional
> +data after the header in this case.
> +
> +If the configuration of an already active ISP block has to be updated,
> +userspace shall fully populate the ISP block parameters and omit setting the
> +V4L2_ISP_PARAMS_FL_BLOCK_ENABLE and V4L2_ISP_PARAMS_FL_BLOCK_DISABLE bits in the
> +header's `flags` field.
> +
> +Setting both the V4L2_ISP_PARAMS_FL_BLOCK_ENABLE and
> +V4L2_ISP_PARAMS_FL_BLOCK_DISABLE bits in the flags field is not allowed and not
> +accepted.
> +
> +Any further extension to the parameters layout that happens after the ISP driver
> +has been merged in Linux can be implemented by adding new blocks definition
> +without invalidating the existing ones.
> +
> +ISP statistics
> +==============
> +
> +Support for generic statistics format is not yet implemented in Video4Linux2.
> +
> +V4L2 ISP uAPI data types
> +========================
> +
> +.. kernel-doc:: include/uapi/linux/media/v4l2-isp.h
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9ac834d212f88222437e8d806800b2516d44f01..340353334299cd5eebf1f72132b7e91b6f5fdbfe 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26857,6 +26857,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/v4l2-isp.rst
>  F:	include/uapi/linux/media/v4l2-isp.h
>  
>  VF610 NAND DRIVER
> 


With the comments above addressed,

Reviewed-by: Michael Riesch <michael.riesch@...labora.com>

Thanks and best regards,
Michael

Powered by blists - more mailing lists