Re: [PATCH 3/8] media: uapi: Convert Amlogic C3 to V4L2 extensible params

[Keke Li <keke.li@amlogic.com>]

Hi Jacopo

On 2025/7/10 14:44, Jacopo Mondi wrote:
> [ EXTERNAL EMAIL ]
>
> Hi Keke
>
>     thanks for the comment and for testing
>
> On Thu, Jul 10, 2025 at 10:32:01AM +0800, Keke Li wrote:
>> 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@ideasonboard.com>


Reviewed-by: Keke Li <keke.li@amlogic.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?
> The types of the Amlogic C3 and RkISP1 headers are already part of the
> kernel userspace API and we can't change them without breaking
> existing users I'm afraid. In the C3 case the kernel support has been
> collected in the v6.16 merge window, and v6.16 has not been released yet,
> so one could argue there are no "existing users" yet.
>
> It's however too late in my opinion to make modifications that depend
> on this patch series which will, be collected for v6.18 at the
> earliest.


OK

> Thanks
>    j
>
>> 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
>>>