Message-ID: <20251005002453.GC13055@pendragon.ideasonboard.com>
Date: Sun, 5 Oct 2025 03:24:53 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Dafna Hirschfeld <dafna@...tmail.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 v5 3/8] media: uapi: Convert Amlogic C3 to V4L2
 extensible params

Hi Jacopo,

Thank you for the patch.

On Mon, Sep 15, 2025 at 07:18:12PM +0200, Jacopo Mondi wrote:
> With the introduction of common types for extensible parameters
> format, convert the c3-isp-config.h header to use the new types.
> 
> Factor-out the documentation that is now part of the common header
> and only keep the driver-specific on in place.
> 
> The conversion to use common types doesn't impact userspace as the
> new types are either identical to the ones already existing in the
> C3 ISP uAPI or are 1-to-1 type convertible.

You can reflow the commit message.

> Reviewed-by: Daniel Scally <dan.scally@...asonboard.com>
> Reviewed-by: Keke Li <keke.li@...ogic.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> ---
>  include/uapi/linux/media/amlogic/c3-isp-config.h | 86 ++++++------------------
>  1 file changed, 20 insertions(+), 66 deletions(-)
> 
> diff --git a/include/uapi/linux/media/amlogic/c3-isp-config.h b/include/uapi/linux/media/amlogic/c3-isp-config.h
> index ed085ea62a574932c7ad8d59d34b2c5c74a597d8..bf6de55b27a7d4d15effcca5525865650d9070fb 100644
> --- a/include/uapi/linux/media/amlogic/c3-isp-config.h
> +++ b/include/uapi/linux/media/amlogic/c3-isp-config.h
> @@ -6,8 +6,13 @@
>  #ifndef _UAPI_C3_ISP_CONFIG_H_
>  #define _UAPI_C3_ISP_CONFIG_H_
>  
> +#ifdef __KERNEL__
> +#include <linux/build_bug.h>
> +#endif /* __KERNEL__ */
>  #include <linux/types.h>
>  
> +#include <linux/media/v4l2-isp.h>
> +
>  /*
>   * Frames are split into zones of almost equal width and height - a zone is a
>   * rectangular tile of a frame. The metering blocks within the ISP collect
> @@ -176,62 +181,22 @@ enum c3_isp_params_block_type {
>  	C3_ISP_PARAMS_BLOCK_SENTINEL
>  };
>  
> -#define C3_ISP_PARAMS_BLOCK_FL_DISABLE (1U << 0)
> -#define C3_ISP_PARAMS_BLOCK_FL_ENABLE (1U << 1)
> +/* For backward compatibility */
> +#define C3_ISP_PARAMS_BLOCK_FL_DISABLE	V4L2_PARAMS_FL_BLOCK_DISABLE
> +#define C3_ISP_PARAMS_BLOCK_FL_ENABLE	V4L2_PARAMS_FL_BLOCK_ENABLE
>  
>  /**
>   * struct c3_isp_params_block_header - C3 ISP parameter block 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. The
> - * driver inspects this common header to discern the block type and its size and
> - * properly handle the block content by casting it to the correct block-specific
> - * type.

+ * blocks and is identical to :c:type:`v4l2_params_block_header`.

>   *
>   * The @type field is one of the values enumerated by

s/@...e/type/

Please compile the documentation for the next version. I expect the
'struct c3_isp_params_block_header' above to not be appreciated by
sphinx as there's no corresponding structure.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>

>   * :c:type:`c3_isp_params_block_type` and specifies how the data should be
> - * interpreted by the driver. The @size field specifies the size of the
> - * parameters block and is used by the driver for validation purposes. The
> - * @flags field is a bitmask of per-block flags C3_ISP_PARAMS_FL*.
> - *
> - * When userspace wants to disable an ISP block the
> - * C3_ISP_PARAMS_BLOCK_FL_DISABLED bit should be set in the @flags field. In
> - * this case userspace may optionally omit the remainder of the configuration
> - * block, which will be ignored by the driver.
> - *
> - * When a new configuration of an ISP block needs to be applied userspace
> - * shall fully populate the ISP block and omit setting the
> - * C3_ISP_PARAMS_BLOCK_FL_DISABLED bit in the @flags field.
> - *
> - * Userspace is responsible for correctly populating the parameters block header
> - * fields (@type, @flags and @size) and the block-specific parameters.
> - *
> - * For example:
> - *
> - * .. code-block:: c
> + * interpreted by the driver.
>   *
> - *	void populate_pst_gamma(struct c3_isp_params_block_header *block) {
> - *		struct c3_isp_params_pst_gamma *gamma =
> - *			(struct c3_isp_params_pst_gamma *)block;
> - *
> - *		gamma->header.type = C3_ISP_PARAMS_BLOCK_PST_GAMMA;
> - *		gamma->header.flags = C3_ISP_PARAMS_BLOCK_FL_ENABLE;
> - *		gamma->header.size = sizeof(*gamma);
> - *
> - *		for (unsigned int i = 0; i < 129; i++)
> - *			gamma->pst_gamma_lut[i] = i;
> - *	}
> - *
> - * @type: The parameters block type from :c:type:`c3_isp_params_block_type`
> - * @flags: A bitmask of block flags
> - * @size: Size (in bytes) of the parameters block, including this header
> + * The flags field is a bitmask of per-block flags C3_ISP_PARAMS_FL_*.
>   */
> -struct c3_isp_params_block_header {
> -	__u16 type;
> -	__u16 flags;
> -	__u32 size;
> -};
> +#define c3_isp_params_block_header v4l2_params_block_header
>  
>  /**
>   * struct c3_isp_params_awb_gains - Gains for auto-white balance
> @@ -498,26 +463,9 @@ struct c3_isp_params_blc {
>  /**
>   * struct c3_isp_params_cfg - C3 ISP configuration parameters
>   *
> - * This struct contains the configuration parameters of the C3 ISP
> - * algorithms, serialized by userspace into an opaque data buffer. Each
> - * configuration parameter block is represented by a block-specific structure
> - * which contains a :c:type:`c3_isp_param_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.
> - *
> - * The parameters buffer is versioned by the @version field to allow modifying
> - * and extending its definition. Userspace should 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 revision and return an error if the desired revision is not
> - * supported.
> - *
> - * 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 nor overlaps. Userspace shall populate the @total_size field with
> - * the effective size, in bytes, of the @data buffer.
> + * This is the driver-specific implementation of :c:type:`v4l2_params_buffer`.
> + *
> + * Currently only C3_ISP_PARAM_BUFFER_V0 is supported.
>   *
>   * The expected memory layout of the parameters buffer is::
>   *
> @@ -561,4 +509,10 @@ struct c3_isp_params_cfg {
>  	__u8 data[C3_ISP_PARAMS_MAX_SIZE];
>  };
>  
> +#ifdef __KERNEL__
> +/* Make sure the header is type-convertible to the generic v4l2 params one */
> +static_assert((sizeof(struct c3_isp_params_cfg) - C3_ISP_PARAMS_MAX_SIZE) ==
> +	      sizeof(struct v4l2_params_buffer));
> +#endif /* __KERNEL__ */
> +
>  #endif
> 

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists