Message-ID: <ochkz2axakkaqv6ygqwfackxh37nmelcu45d3g6cpmh3dxxjmf@mg6jlfyb62iv>
Date: Sat, 8 Nov 2025 09:59:51 +0100
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 
	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, 
	Michael Riesch <michael.riesch@...labora.com>
Subject: Re: [PATCH v8 4/8] media: Documentation: uapi: Add V4L2 ISP
 documentation

Hi Laurent

On Sat, Nov 08, 2025 at 02:17:13AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Oct 20, 2025 at 10:24:50AM +0200, Jacopo Mondi wrote:
> > Add userspace documentation for V4L2 ISP generic parameters and
> > statistics formats.
> >
> > Reviewed-by: Daniel Scally <dan.scally@...asonboard.com>
> > Reviewed-by: Michael Riesch <michael.riesch@...labora.com>
> > Acked-by: Sakari Ailus <sakari.ailus@...ux.intel.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> > ---
> >  .../userspace-api/media/v4l/meta-formats.rst       |   1 +
> >  Documentation/userspace-api/media/v4l/v4l2-isp.rst | 120 +++++++++++++++++++++
> >  MAINTAINERS                                        |   1 +
> >  3 files changed, 122 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst b/Documentation/userspace-api/media/v4l/meta-formats.rst
> > index d9868ee88a0717c1acaa4ee477eaed96a6411f73..7b758ea9eb4ac3c4b354bf8e2f319985ed9e2b37 100644
> > --- a/Documentation/userspace-api/media/v4l/meta-formats.rst
> > +++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
> > @@ -25,3 +25,4 @@ These formats are used for the :ref:`metadata` interface only.
> >      metafmt-vivid
> >      metafmt-vsp1-hgo
> >      metafmt-vsp1-hgt
> > +    v4l2-isp
> > diff --git a/Documentation/userspace-api/media/v4l/v4l2-isp.rst b/Documentation/userspace-api/media/v4l/v4l2-isp.rst
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..b53df722ed29117c3827314e844fc4de61343f40
> > --- /dev/null
> > +++ b/Documentation/userspace-api/media/v4l/v4l2-isp.rst
> > @@ -0,0 +1,120 @@
> > +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> > +
> > +.. _v4l2-isp:
> > +
> > +************************
> > +Generic V4L2 ISP formats
> > +************************
> > +
> > +ISP configuration and statistics: theory of operations
> > +======================================================
> > +
> > +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.
> > +
> > +ISP statistics are collected at a specific time point and drivers use them to
> > +populate a *statistics buffer* which is then returned to userspace.
> > +
> > +The parameters and statistics buffers are organized in a driver-specific
> > +way, and their data layout differs between one driver and another.
> > +
> > +ISP drivers generally exchange parameters and statistics with userspace through
> > +a metadata output and capture node respectively, implementing the
> > +:c:type:`v4l2_meta_format` interface. Each ISP driver defines one metadata
> > +capture format and one metadata output format to be used on those video nodes,
> > +and the buffer content layout and organization is fixed by the format definition.
> > +
> > +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 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 new features and/or bug fixes may land in the kernel at a later
> > +stage and require changes to the metadata formats definition. This is
> > +considered an ABI breakage that is strictly forbidden by the Linux 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 output formats that describe ISP configuration parameters were
> > +typically realized by defining C structures that reflect the ISP registers
> > +layout and get 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 number of supported ISP blocks, the layout of their configuration data are
> > +fixed by the format definition, incurring in the above described uAPI/uABI
> > +problem.
> > +
> > +Generic ISP parameters
> > +----------------------
> > +
>
> Most of the text above is a design rationale that in my opinion doesn't
> belong to the UAPI documentation. You already include a design rationale

mm, you're right, I'll drop it

> in the kernel documentation, in patch 8/8. If some of the text above
> contains a more verbose explanation, it could be moved there.

I think it's fine

>
> I would shorten all this to
>
> ************************
> Generic V4L2 ISP formats
> ************************
>
> Generic ISP formats are metadata formats that define a mechanism to pass ISP
> parameters and statistics between userspace and drivers in V4L2 buffers. They
> are designed to allow extending the data in a backward-compatible way.
>
> ISP configuration
> =================

Now re-organized as

************************
Generic V4L2 ISP formats
************************

ISP parameters
=================

ISP Block enabling, disabling and configuration
-----------------------------------------------

ISP statistics
==============

V4L2 ISP uAPI data types
========================

>
> > +The generic ISP configuration parameters format is realized by a defining a
> > +single C structure that contains a header, followed by a binary buffer where
> > +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
>
> s/versioning information/extensible parameters format version/


Thanks, I tried in the last versions to avoid the term "extensible"
and I've used "generic" everywhere I could.

I have reworded this as it follows to avoid repeating 'parameters' 3
times in a single paragraph:

The :c:type:`v4l2_isp_params_buffer` structure defines the buffer header which
is followed by a binary buffer of ISP configuration data. Userspace shall
correctly populate the buffer header with the generic parameters format version
and with the size (in bytes) of the binary data buffer where it will store the
ISP blocks configuration.

>
> > +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.
>
> s/and not accepted/and returns an error/
>
> > +
> > +Any further extension to the parameters layout that happens after the ISP driver
>
> s/Any further extension/Extensions/
>
> > +has been merged in Linux can be implemented by adding new blocks definition
>
> s/after the ISP driver has been merged in Linux //

Reworded as

Extension to the parameters format can be implemented by adding new blocks
definition without invalidating the existing ones.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>

Thanks
  j

>
> > +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 d925745077f21e5a1388a30217a24beeb4fff3b5..f52237d57710cadff78b297d2b4610b508f55092 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -26856,6 +26856,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
>
> --
> Regards,
>
> Laurent Pinchart

Powered by blists - more mailing lists