Re: Re: [RFC v1 01/11] media: uapi: v4l2-isp: Add v4l2 ISP extensible statistics definitions

[Antoine Bouyer <antoine.bouyer@nxp.com>]

Hi Laurent, Jacopo

On 2/10/26 12:00 AM, Laurent Pinchart wrote:
> 
> 
> On Wed, Feb 04, 2026 at 02:14:50PM +0100, Jacopo Mondi wrote:
>> On Wed, Feb 04, 2026 at 12:07:41PM +0100, Antoine Bouyer wrote:
>>> On 2/3/26 5:15 PM, Jacopo Mondi wrote:
>>>> On Fri, Jan 23, 2026 at 09:09:28AM +0100, Antoine Bouyer wrote:
>>>>> Extend the v4l2-isp extensible format introduced for isp parameters buffer
>>>>> to the statistics buffer as well.
>>>>>
>>>>> Like for ISP configuration purpose, that will help supporting various ISP
>>>>> hardware versions reporting different statistics data with less impact on
>>>>> userspace.
>>>>>
>>>>> The `v4l2_isp_stats_buffer` reuses the `v4l2_isp_params_buffer` container
>>>>> definitions, with similar header, versions and flags. V0 and V1 versions
>>>>
>>>> Why do you need two flags ?
>>>>
>>>> Params had to introduce two because we had two drivers already
>>>> mainlined using the pre-v4l2-isp version of extensible params which
>>>> had defined their version identifier as 1 and 0 and we didn't want to
>>>> break existing userspace using those identifiers. So we had to accept
>>>> both V0 and V1 as "first version of the v4l2-isp extensible parameters
>>>> format".
>>>>
>>>> For stats we don't have users, so I guess we can start with V1 == 0 ?
>>>
>>> I wanted to keep it aligned with params, so that any driver/userspace can
>>> use the same API version value for both params and stats buffers, and limit
>>> headache.

Seems this topic is not yet clarified. Should I use same version values 
for both params and stats as in current patch ? Or use different values 
V1 == 0 as suggested ?

 From userspace perspective, I feel it would be easier to align params 
and stats versions, so we don't need to maintain different versions for 
same purpose; and if a new version V2 comes and is applied to both 
params and stats buffers, then we can use same value too. What do you 
think ?

>>>
>>>>> are provided to match with params versions. On the other side, ENABLE and
>>>>> DISABLE flags are not really meaningfull for statistics purpose. So VALID
>>>>> and INVALID flags are introduced. Purpose is to force ISP driver to
>>>>> validate a statistics buffer, before it is consumed by userspace.
>>>>
>>>> Interesting. What do you mean with "validate a statistics buffer" ?
>>>> And if a driver has to do validation, why would it send upstream a
>>>> non-validated buffer ?
>>>
>>> Like for version, I wanted to keep same header structure, including flags.
>>> Since ENABLE/DISABLE is not relevant for statistics, I thought about using a
>>> "validation" flag, to force driver confirming statistics blocks are valid or
>>> not.
>>
>> See the question on the documentation patches.
>>
>>> If you feel it is useless, I'm fine with removing it. Should I keep a flag
>>> field anyway to stay aligned with params then ?
>>
>> RkISP1 has support for both "legacy" and "extensible" formats because
>> it has been mainline for a long time with the legacy format only. We
>> couldn't simply replace the existing format with the new one because
>> we would break existing users.
>>
>> All the other drivers that have been upstreamed with extensible only
>> (Amlogic C3 and Mali C55) do not expose a legacy format as there was
>> not prior version in mainline on which userspace might depend on.
>>
>> Unless you have very convincing reason, I would certainly drop the
>> legacy format and only use extensible.
> 
> I agree with that, for upstream we shouldn't carry legacy formats in new
> drivers. I've read elsewhere in this thread that it won't cause issues,
> otherwise I would have recommended carrying an extra patch in the BSP
> kernel to implement legacy formats, and only use extensible formats
> upstream.

ok. I'm fine with removing legacy format from neo driver, and keep it 
only in downstream for some time.

> 
>>>>> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
>>>>> ---
>>>>>    include/uapi/linux/media/v4l2-isp.h | 85 +++++++++++++++++++++++++++++
>>>>>    1 file changed, 85 insertions(+)
>>>>>
>>>>> diff --git a/include/uapi/linux/media/v4l2-isp.h b/include/uapi/linux/media/v4l2-isp.h
>>>>> index 779168f9058e..ed1279b86694 100644
>>>>> --- a/include/uapi/linux/media/v4l2-isp.h
>>>>> +++ b/include/uapi/linux/media/v4l2-isp.h
>>>>> @@ -99,4 +99,89 @@ struct v4l2_isp_params_buffer {
>>>>>         __u8 data[] __counted_by(data_size);
>>>>>    };
>>>>>
>>>>> +/**
>>>>> + * enum v4l2_isp_stats_version - V4L2 ISP statistics versioning
>>>>> + *
>>>>> + * @V4L2_ISP_STATS_VERSION_V0: First version of the V4L2 ISP statistics format
>>>>> + *                          (for compatibility)
>>>>> + * @V4L2_ISP_STATS_VERSION_V1: First version of the V4L2 ISP statistics format
>>>>> + *
>>>>> + * V0 and V1 are identical, and comply with V4l2 ISP parameters versions. So
>>>>> + * both V0 and V1 refers to the first version of the V4L2 ISP statistics
>>>>> + * format.
>>>>> + *
>>>>> + * Future revisions of the V4L2 ISP statistics format should start from the
>>>>> + * value of 2.
>>>>> + */
>>>>> +enum v4l2_isp_stats_version {
>>>>> +     V4L2_ISP_STATS_VERSION_V0 = 0,
>>>>> +     V4L2_ISP_STATS_VERSION_V1,
>>>>
>>>> As suggested I would make V1 == 0
>>>>
>>>>> +};
>>>>> +
>>>>> +#define V4L2_ISP_PARAMS_FL_BLOCK_VALID               (1U << 0)
>>>>> +#define V4L2_ISP_PARAMS_FL_BLOCK_INVALID     (1U << 1)
>>>>> +
>>>>> +/*
>>>>> + * Reserve the first 8 bits for V4L2_ISP_STATS_FL_* flag.
>>>>> + *
>>>>> + * Driver-specific flags should be defined as:
>>>>> + * #define DRIVER_SPECIFIC_FLAG0     ((1U << V4L2_ISP_STATS_FL_DRIVER_FLAGS(0))
>>>>> + * #define DRIVER_SPECIFIC_FLAG1     ((1U << V4L2_ISP_STATS_FL_DRIVER_FLAGS(1))
>>>>> + */
>>>>> +#define V4L2_ISP_STATS_FL_DRIVER_FLAGS(n)       ((n) + 8)
>>>>
>>>> Currently we have no users of V4L2_ISP_PARAMS_FL_DRIVER_FLAGS so we
>>>> could even consider making it a V4L2_ISP_FL_DRIVER_FLAGS
>>>>
>>>> Or do you think it is worth creating a new symbol ?
>>>
>>> To limit impact on potential on-going development, and future conflict,
>>> creating new symbol may be safer IMO. But I'm fine with using a single
>>> symbol if you prefer. Most probably this flag customization is not used yet
>>> by any driver.
>>>
>>>>> +
>>>>> +/**
>>>>> + * struct v4l2_isp_stats_block_header - V4L2 extensible statistics block header
>>>>> + * @type: The statistics block type (driver-specific)
>>>>> + * @flags: A bitmask of block flags (driver-specific)
>>>>> + * @size: Size (in bytes) of the statistics block, including this header
>>>>> + *
>>>>> + * This structure represents the common part of all the ISP statistics blocks.
>>>>> + * Each statistics block shall embed an instance of this structure type as its
>>>>> + * first member, followed by the block-specific statistics data.
>>>>> + *
>>>>> + * The @type field is an ISP driver-specific value that identifies the block
>>>>> + * type. The @size field specifies the size of the parameters block.
>>>>> + *
>>>>> + * The @flags field is a bitmask of per-block flags V4L2_STATS_ISP_FL_* and
>>>>> + * driver-specific flags specified by the driver header.
>>>>> + */
>>>>> +struct v4l2_isp_stats_block_header {
>>>>> +     __u16 type;
>>>>> +     __u16 flags;
>>>>> +     __u32 size;
>>>>> +} __attribute__((aligned(8)));
>>>>> +
>>>>
>>>> This is currently identical to v4l2_isp_params_block_header.
>>>>
>>>> Can we create a single header for both stats and params and provide a
>>>>
>>>> #define v4l2_isp_params_block_header v4l2_isp_block_header
>>>>
>>>> for maintaining compatibility with existing users ?
>>>>
>>>> Or do you expect stats and params to eventually need different headers ?
>>>
>>> Current approach is to use same structure definitions as for params. So I'm
>>> fine with creating a single header as suggested, and provide symbols to keep
>>> compatibility.
>>>
>>>>> +/**
>>>>> + * struct v4l2_isp_stats_buffer - V4L2 extensible statistics data
>>>>> + * @version: The statistics buffer version (driver-specific)
>>>>> + * @data_size: The statistics data effective size, excluding this header
>>>>> + * @data: The statistics data
>>>>> + *
>>>>> + * This structure contains the statistics information of the ISP hardware,
>>>>> + * serialized for userspace into a data buffer. Each statistics block is
>>>>> + * represented by a block-specific structure which contains a
>>>>> + * :c:type:`v4l2_isp_stats_block_header` entry as first member. Driver
>>>>> + * populates the @data buffer with statistics information of the ISP blocks it
>>>>> + * intends to share to userspace. As a consequence, the data buffer effective
>>>>> + * size changes according to the number of ISP blocks that driver intends to
>>>>> + * provide and is set by the driver in the @data_size field.
>>>>> + *
>>>>> + * The statistics buffer is versioned by the @version field to allow modifying
>>>>> + * and extending its definition. Driver shall populate the @version field to
>>>>> + * inform the userpsace about the version it intends to use. The userspace will
>>>>> + * parse and handle the @data buffer according to the data layout specific to
>>>>> + * the indicated version.
>>>>> + *
>>>>> + * For each ISP block that driver wants to report, a block-specific structure
>>>>> + * is appended to the @data buffer, one after the other without gaps in
>>>>> + * between. Driver shall populate the @data_size field with the effective
>>>>> + * size, in bytes, of the @data buffer.
>>>>> + */
>>>>> +struct v4l2_isp_stats_buffer {
>>>>> +     __u32 version;
>>>>> +     __u32 data_size;
>>>>> +     __u8 data[] __counted_by(data_size);
>>>>> +};
>>>>> +
>>>>
>>>> Same question. Should we introduce a struct v4l2_isp_buffer ?
>>>
>>> Yes, sounds reasonable.
> 
> That seems to make sense. Once we'll have a driver using
> v4l2_isp_stats_buffer the structure will become ABI. If it then is an
> exact copy of v4l2_isp_params_buffer, it would make sense to unify them.
> Let's see what will happen after a few review rounds, if we end up
> requiring separate fields in the stats buffer header.

ok to use same struct for both.

> 
> It would also be nice to implement support for extensible stats in a
> second driver to test the API.

What is your preferred approach then ?

Should I "split" v4l2_isp rework in different patchset BUT with another 
driver using it (with userspace changes I guess). Not something I'm 
really comfortable with since I only focused on i.MX95 isp so far. But 
if this is the only way, that could be evaluated.

Or should I "keep" v4l2_isp changes together with neoisp introduction ?

Thanks

Best regards
Antoine

> 
>>>>>    #endif /* _UAPI_V4L2_ISP_H_ */
> 
> --
> Regards,
> 
> Laurent Pinchart