Message-ID: <f5df8dfd-4bd4-412b-b5e7-a7fba7885c1a@collabora.com>
Date: Mon, 20 Oct 2025 11:02:15 +0200
From: Michael Riesch <michael.riesch@...labora.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: 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 Jacopo,

On 10/20/25 10:09, Jacopo Mondi wrote:
> Hi Michael
> 
>   thanks for review. I took all comments in but..

Cool, thanks!

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

Maybe it's just me, but the sentence still does not sound correct to my
ears. First, you enumerate two items, so they should be joined with
"and", right? And then I understand that the two items have the
uAPI/uABI problem as consequence, correct? "to result in" or "to lead
to" seem to be better choices.

That said, don't hesitate to point out things that I misunderstood.

Best regards,
Michael

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

Powered by blists - more mailing lists