linux-kernel - Re: [PATCH v7 4/8] media: Documentation: uapi: Add V4L2 ISP documentation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bprg66hg3xoetosl7dwt2rcs6mpcksfalymmyidla6qvdrnm7u@fpn6237j25ir>
Date: Mon, 20 Oct 2025 10:09:59 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Michael Riesch <michael.riesch@...labora.com>
Cc: 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>, 
	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 Michael

  thanks for review. I took all comments in but..


On Tue, Oct 14, 2025 at 11:23:29AM +0200, Michael Riesch wrote:
> 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.
>

.. this one, for which I think the correct form is

 > +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.

Thanks
  j

> > +
> > +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
>
>