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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing backward compatibility macro for V4L2_ISP_PARAMS_FL_DRIVER_FLAGS.
- [Medium] The VALID and INVALID flags described in the commit message for the statistics buffer are entirely missing from the code.
--

--- Patch [2]: [PATCH v1 02/11] media: uapi: v4l2-isp: Add v4l2 ISP extensible statistics definitions ---
commit 6a949d8a5a3f44c2ff93fc6087ed1842a9325479
Author: Antoine Bouyer <[email protected]>

media: uapi: v4l2-isp: Add v4l2 ISP extensible statistics definitions

This commit extends the v4l2-isp extensible format to support statistics
buffers. It introduces v4l2_isp_stats_buffer reusing the container definitions
from v4l2_isp_params_buffer, and adds V0 and V1 versions for compatibility.

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

[Severity: Medium]
Are the VALID and INVALID flags missing from this patch?

The commit message states they are introduced to force the ISP driver to
validate the buffer, but they do not appear in the code. Furthermore, the
added documentation for v4l2_isp_stats_block_header states there are no
generic flags for statistics.

> diff --git a/include/uapi/linux/media/v4l2-isp.h b/include/uapi/linux/media/v4l2-isp.h
> index 779168f9058e3..e84476280d431 100644
> --- a/include/uapi/linux/media/v4l2-isp.h
> +++ b/include/uapi/linux/media/v4l2-isp.h
[ ... ]
> @@ -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)

[Severity: Critical]
Does this rename break UAPI compatibility?

Unlike v4l2_isp_params_version and V4L2_ISP_PARAMS_VERSION_V0 which received
compatibility macros earlier in the file, V4L2_ISP_PARAMS_FL_DRIVER_FLAGS is
renamed without a fallback alias. This will cause existing userspace builds
relying on the old macro to fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2