Message-ID: <968ffcce-9fef-4336-b83d-f9cd7ade6ea0@amlogic.com>
Date: Thu, 10 Jul 2025 10:32:01 +0800
From: Keke Li <keke.li@...ogic.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>,
 Dafna Hirschfeld <dafna@...tmail.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.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>
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 3/8] media: uapi: Convert Amlogic C3 to V4L2 extensible
 params

Hi Jacopo

On 2025/7/8 18:40, Jacopo Mondi wrote:
> [ EXTERNAL EMAIL ]
>
> 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.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> ---
>   include/uapi/linux/media/amlogic/c3-isp-config.h | 45 +++++++-----------------
>   1 file changed, 12 insertions(+), 33 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..203116cdfb89356301c16c98cb40e5b83efe71d6 100644
> --- a/include/uapi/linux/media/amlogic/c3-isp-config.h
> +++ b/include/uapi/linux/media/amlogic/c3-isp-config.h
> @@ -6,8 +6,12 @@
>   #ifndef _UAPI_C3_ISP_CONFIG_H_
>   #define _UAPI_C3_ISP_CONFIG_H_
>
> +#include <linux/build_bug.h>
>   #include <linux/types.h>
>
> +#define _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> +#include <linux/media/v4l2-extensible-params.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
> @@ -183,11 +187,6 @@ enum c3_isp_params_block_type {
>    * 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.
>    *
>    * The @type field is one of the values enumerated by
>    * :c:type:`c3_isp_params_block_type` and specifies how the data should be
> @@ -223,15 +222,8 @@ enum c3_isp_params_block_type {
>    *                     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
>    */
> -struct c3_isp_params_block_header {
> -       __u16 type;
> -       __u16 flags;
> -       __u32 size;
> -};
> +#define c3_isp_params_block_header v4l2_params_block

Why not use v4l2_params_block directly?

Thanks

Keke

>
>   /**
>    * struct c3_isp_params_awb_gains - Gains for auto-white balance
> @@ -498,26 +490,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 +536,8 @@ struct c3_isp_params_cfg {
>          __u8 data[C3_ISP_PARAMS_MAX_SIZE];
>   };
>
> +/* 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
>
> --
> 2.49.0
>

Powered by blists - more mailing lists