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

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

Hi Jacopo

On 2/3/26 5:15 PM, Jacopo Mondi wrote:
> 
> 
> Hi Antoine
>    thanks a lot for extendable stats
> 
> 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.

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

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 ?

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

BR
Antoine

> 
> Thanks!
> 
>>   #endif /* _UAPI_V4L2_ISP_H_ */
>> --
>> 2.52.0
>>
>>