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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hello Antoine,
   thanks for the update

On Mon, Apr 13, 2026 at 06:03:22PM +0200, 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
> 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.

Is this a leftover ?

I don't see VALID and INVALID in this patch and unless I've missed it
badly I don't see them in the next patches.

I'm fine without them, I'm not sure how you intend to use them to
force drivers to validate a statistics buffer.

>
> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
> ---
>  include/uapi/linux/media/v4l2-isp.h | 148 +++++++++++++++++++---------
>  1 file changed, 100 insertions(+), 48 deletions(-)
>
> diff --git a/include/uapi/linux/media/v4l2-isp.h b/include/uapi/linux/media/v4l2-isp.h
> index 779168f9058e..e84476280d43 100644
> --- a/include/uapi/linux/media/v4l2-isp.h
> +++ b/include/uapi/linux/media/v4l2-isp.h
> @@ -13,25 +13,33 @@
>  #include <linux/types.h>
>
>  /**
> - * enum v4l2_isp_params_version - V4L2 ISP parameters versioning
> + * enum v4l2_isp_version - V4L2 ISP serialization format versioning
>   *
> - * @V4L2_ISP_PARAMS_VERSION_V0: First version of the V4L2 ISP parameters format
> - *				(for compatibility)
> - * @V4L2_ISP_PARAMS_VERSION_V1: First version of the V4L2 ISP parameters format
> + * @V4L2_ISP_VERSION_V0: First version of the V4L2 ISP serialization format
> + *                       (for compatibility)
> + * @V4L2_ISP_VERSION_V1: First version of the V4L2 ISP serialization format
>   *
>   * V0 and V1 are identical in order to support drivers compatible with the V4L2
> - * ISP parameters format already upstreamed which use either 0 or 1 as their
> - * versioning identifier. Both V0 and V1 refers to the first version of the
> - * V4L2 ISP parameters format.
> + * ISP format already upstreamed which use either 0 or 1 as their versioning
> + * identifier. Both V0 and V1 refers to the first version of the V4L2 ISP
> + * serialization format.
>   *
> - * Future revisions of the V4L2 ISP parameters format should start from the
> + * Future revisions of the V4L2 ISP serialization format should start from the
>   * value of 2.
>   */
> -enum v4l2_isp_params_version {
> -	V4L2_ISP_PARAMS_VERSION_V0 = 0,
> -	V4L2_ISP_PARAMS_VERSION_V1
> +enum v4l2_isp_version {
> +	V4L2_ISP_VERSION_V0 = 0,
> +	V4L2_ISP_VERSION_V1
>  };
>
> +/*
> + * Compatibility with existing users of v4l2_isp_params which pre-date the
> + * introduction of v4l2_isp_stats.
> + */
> +#define v4l2_isp_params_version			v4l2_isp_version
> +#define V4L2_ISP_PARAMS_VERSION_V0		V4L2_ISP_VERSION_V0
> +#define V4L2_ISP_PARAMS_VERSION_V1		V4L2_ISP_VERSION_V1
> +
>  #define V4L2_ISP_PARAMS_FL_BLOCK_DISABLE	(1U << 0)
>  #define V4L2_ISP_PARAMS_FL_BLOCK_ENABLE		(1U << 1)
>
> @@ -39,64 +47,108 @@ enum v4l2_isp_params_version {
>   * Reserve the first 8 bits for V4L2_ISP_PARAMS_FL_* flag.
>   *
>   * Driver-specific flags should be defined as:
> - * #define DRIVER_SPECIFIC_FLAG0     ((1U << V4L2_ISP_PARAMS_FL_DRIVER_FLAGS(0))
> - * #define DRIVER_SPECIFIC_FLAG1     ((1U << V4L2_ISP_PARAMS_FL_DRIVER_FLAGS(1))
> + * #define DRIVER_SPECIFIC_FLAG0     ((1U << V4L2_ISP_FL_DRIVER_FLAGS(0))
> + * #define DRIVER_SPECIFIC_FLAG1     ((1U << V4L2_ISP_FL_DRIVER_FLAGS(1))
>   */
> -#define V4L2_ISP_PARAMS_FL_DRIVER_FLAGS(n)       ((n) + 8)
> +#define V4L2_ISP_FL_DRIVER_FLAGS(n)		((n) + 8)
>
>  /**
> - * struct v4l2_isp_params_block_header - V4L2 extensible parameters block header
> - * @type: The parameters block type (driver-specific)
> + * struct v4l2_isp_block_header - V4L2 extensible block header
> + * @type: The parameters or statistics block type (driver-specific)
>   * @flags: A bitmask of block flags (driver-specific)
> - * @size: Size (in bytes) of the parameters block, including this header
> + * @size: Size (in bytes) of the block, including this header
>   *
> - * This structure represents the common part of all the ISP configuration
> - * blocks. Each parameters block shall embed an instance of this structure type
> - * as its first member, followed by the block-specific configuration data.
> + * This structure represents the common part of all the ISP configuration or
> + * statistic blocks. Each block shall embed an instance of this structure type
> + * as its first member, followed by the block-specific configuration or
> + * statistic 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_PARAMS_ISP_FL_* and
> - * driver-specific flags specified by the driver header.
> + * type. The @size field specifies the size of the block, including this
> + * header.
>   */
> -struct v4l2_isp_params_block_header {
> +struct v4l2_isp_block_header {
>  	__u16 type;
>  	__u16 flags;
>  	__u32 size;
>  } __attribute__((aligned(8)));
>
>  /**
> - * struct v4l2_isp_params_buffer - V4L2 extensible parameters configuration
> - * @version: The parameters buffer version (driver-specific)
> - * @data_size: The configuration data effective size, excluding this header
> - * @data: The configuration data
> + * v4l2_isp_params_block_header - V4L2 extensible parameters block header
>   *
> - * This structure contains the configuration parameters of the ISP algorithms,
> - * serialized by userspace into a data buffer. Each configuration parameter
> - * block is represented by a block-specific structure which contains a
> - * :c:type:`v4l2_isp_params_block_header` entry as first member. Userspace
> - * populates the @data buffer with configuration parameters for the blocks that
> - * it intends to configure. As a consequence, the data buffer effective size
> - * changes according to the number of ISP blocks that userspace intends to
> - * configure and is set by userspace in the @data_size field.
> - *
> - * The parameters buffer is versioned by the @version field to allow modifying
> - * and extending its definition. Userspace shall populate the @version field to
> - * inform the driver about the version it intends to use. The driver will parse
> - * and handle the @data buffer according to the data layout specific to the
> - * indicated version and return an error if the desired version is not
> + * This structure represents the common part of all the ISP configuration blocks
> + * and is identical to :c:type:`v4l2_isp_block_header`.
> + *
> + * The @flags field is a bitmask of per-block flags V4L2_ISP_PARAMS_FL_* and
> + * driver-specific flags specified by the driver header.

What if we move this to the documentation of struct v4l2_isp_block_header
and we only document the macro for compatibility reasons like you did
for `v4l2_isp_params_version` ?

We could add the above to the documentation of `struct
v4l2_isp_block_header`:

 * The @flags field is a bitmask of per-block flags. If a block is used for
 * configuration parameters this field can be a combination of
 * V4L2_ISP_PARAMS_FL_ and driver-specific flags.

Depending on the answer on VALID/INVALID we can document the usage of
flags for stats as:

 * The @flags field is a bitmask of per-block flags. If a block is used for
 * configuration parameters this field can be a combination of
 * V4L2_ISP_PARAMS_FL_ and driver-specific flags. If a block is used
 * for statistics this fields is used to report optional
 * driver-specific flags, if any.


> + */
> +#define v4l2_isp_params_block_header v4l2_isp_block_header

If you accept the above suggestion we can simply document this

/**
 * v4l2_isp_params_block_header - V4L2 extensible parameters compatibility
 *
 * Compatibility with existing users of v4l2_isp_params_block_header
 * which pre-date the introduction of v4l2_isp_block_header.
 *.

> +
> +/**
> + * v4l2_isp_stats_block_header - V4L2 extensible statistics block header
> + *
> + * This structure represents the common part of all the ISP statistics blocks
> + * and is identical to :c:type:`v4l2_isp_block_header`.
> + *
> + * The @flags field is a bitmask of driver-specific flags specified by the
> + * driver header, as there is no generic flags for statistics.
> + */
> +#define v4l2_isp_stats_block_header v4l2_isp_block_header

Do we need this or should we use v4l2_isp_block_header unconditionally ?

> +
> +/**
> + * struct v4l2_isp_buffer - V4L2 extensible buffer
> + * @version: The extensible buffer version (driver-specific)
> + * @data_size: The data effective size, excluding this header
> + * @data: The configuration or statistics data
> + *
> + * This structure contains ISP configuration parameters or ISP hardware
> + * statistics serialized into a data buffer. Each block is represented by a
> + * block-specific structure which contains a :c:type:`v4l2_isp_block_header`
> + * entry as first member.
> + *
> + * For a parameters block, userspace populates the @data buffer with

Or:

    * When used for ISP parameters userspace ..

> + * configuration parameters for the blocks that it intends to configure.
> + * As a consequence, the data buffer effective size changes according to the
> + * number of ISP blocks that userspace intends to configure and is set by
> + * userspace in the @data_size field.
> + *
> + * For a statistics block, behavior is the same as for parameters, except that

Or:

    * When used to report ISP statistics the driver populates the
    * @data buffer with statistics for each supported measurement
    * block. The buffer effective size is set by the driver in the
    * @data_size field.

> + * buffer is filled by the ISP driver.
> + *
> + * The buffer is versioned by the @version field to allow modifying
> + * and extending its definition. The writer shall populate the @version field
> + * to inform the reader about the version it intends to use. The reader will
> + * parse and handle the @data buffer according to the data layout specific to
> + * the indicated version and return an error if the desired version is not
>   * supported.

Ack, I think using "writer" and "reader" is clear enough to support
both the params and stats use case. If we want more clarity we can add
to "driver" and "userspace" a "(role)" in the two previous paragraph.
Something like:

    * When used for ISP parameters userspace (the writer) populates
    * the @data buffer ...

>   *
> - * For each ISP block that userspace wants to configure, a block-specific
> - * structure is appended to the @data buffer, one after the other without gaps
> - * in between. Userspace shall populate the @data_size field with the effective
> - * size, in bytes, of the @data buffer.
> + * For each ISP block, a block-specific structure is appended to the @data
> + * buffer, one after the other without gaps in between. The writer shall
> + * populate the @data_size field with the effective size, in bytes, of the
> + * @data buffer.

If we want to describe @data_size here we can remove it from the above
two paragraphs maybe. I think it's fine to have it here only.

>   */
> -struct v4l2_isp_params_buffer {
> +struct v4l2_isp_buffer {
>  	__u32 version;
>  	__u32 data_size;
>  	__u8 data[] __counted_by(data_size);
>  };
>
> +/**
> + * v4l2_isp_params_buffer - V4L2 extensible parameters configuration

s/configuration/compatibility

> + *
> + * This structure contains the configuration parameters of the ISP algorithms,
> + * serialized into a data buffer. It is identical to
> + * :c:type:`v4l2_isp_buffer`.

And here only

 * Compatibility with existing users of v4l2_isp_params_buffer which
 * pre-date the introduction of v4l2_isp_buffer

> + */
> +#define v4l2_isp_params_buffer v4l2_isp_buffer
> +
> +/**
> + * v4l2_isp_stats_buffer - V4L2 extensible statistics buffer
> + *
> + * This structure contains the statistics data from the ISP hardware,
> + * serialized into a data buffer. It is identical to
> + * :c:type:`v4l2_isp_buffer`.
> + */
> +#define v4l2_isp_stats_buffer v4l2_isp_buffer

Same question as per `v4l2_isp_stats_block_header`. Do we need it ?

Thanks
  j

> +
>  #endif /* _UAPI_V4L2_ISP_H_ */
> --
> 2.51.0
>