Re: [PATCH v4 07/10] media: uapi: Add stats buffer and parameters buffer for c3 ISP

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

Hi Jacopo

Thanks very much for your reply.

On 2024/12/12 01:21, Jacopo Mondi wrote:
> [ EXTERNAL EMAIL ]
>
> Hi Keke
>
> On Thu, Dec 05, 2024 at 05:04:33PM +0800, Keke Li via B4 Relay wrote:
>> From: Keke Li <keke.li@amlogic.com>
>>
>> Add a header that describes the 3A statistics buffer and the
>> parameters buffer for c3 ISP
>>
>> Signed-off-by: Keke Li <keke.li@amlogic.com>
>> ---
>>   include/uapi/linux/media/amlogic/c3-isp-config.h | 555 +++++++++++++++++++++++
>>   1 file changed, 555 insertions(+)
>>
>> diff --git a/include/uapi/linux/media/amlogic/c3-isp-config.h b/include/uapi/linux/media/amlogic/c3-isp-config.h
>> new file mode 100644
>> index 000000000000..52b9b805f870
>> --- /dev/null
>> +++ b/include/uapi/linux/media/amlogic/c3-isp-config.h
>> @@ -0,0 +1,555 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>> +/*
>> + * Copyright (C) 2024 Amlogic, Inc. All rights reserved
>> + */
>> +
>> +#ifndef _UAPI_C3_ISP_CONFIG_H_
>> +#define _UAPI_C3_ISP_CONFIG_H_
>> +
>> +#include <linux/types.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
>> + * aggregated statistics per zone.
>> + */
>> +#define C3_ISP_AE_MAX_ZONES               (17 * 15)
>> +#define C3_ISP_AF_MAX_ZONES               (17 * 15)
>> +#define C3_ISP_AWB_MAX_ZONES              (32 * 24)
>> +
>> +/* The maximum number of point on the diagonal of the frame for statistics */
>> +#define C3_ISP_AE_MAX_PT_NUM              18
>> +#define C3_ISP_AF_MAX_PT_NUM              18
>> +#define C3_ISP_AWB_MAX_PT_NUM             33
>> +
>> +/**
>> + * struct c3_isp_awb_zone_stats - AWB statistics of a zone
>> + *
>> + * AWB zone stats is aligned with 8 bytes
>> + *
>> + * @rg: the ratio of R / G in a zone
>> + * @bg: the ratio of B / G in a zone
>> + * @pixel_sum: the total number of pixels used in a zone
>> + */
>> +struct c3_isp_awb_zone_stats {
>> +     __u16 rg;
>> +     __u16 bg;
>> +     __u32 pixel_sum;
>> +};
>> +
>> +/**
>> + * struct c3_isp_awb_stats - Auto white balance statistics information.
>> + *
>> + * AWB statistical information of all zones.
>> + *
>> + * @stats: array of auto white balance statistics
>> + */
>> +struct c3_isp_awb_stats {
>> +     struct c3_isp_awb_zone_stats stats[C3_ISP_AWB_MAX_ZONES];
>> +} __attribute__((aligned(16)));
> the 16 bytes alignment is to match the layout produced by the HW ?
>

Yes,  the hardware requires 16-byte alignment.

>> +
>> +/**
>> + * struct c3_isp_ae_zone_stats - AE statistics of a zone
>> + *
>> + * AE zone stats is aligned with 8 bytes.
>> + * This is a 5-bin histogram and the total sum is
>> + * normalized to 0xffff.
> Here and in many other comments text should be re-flowed to 80 cols


OK,  will re-flow to 80 cols.

>> + * So hist2 = 0xffff - (hist0 + hist1 + hist3 + hist4)
>> + *
>> + * @hist0: the global normalized pixel count for bin 0
>> + * @hist1: the global normalized pixel count for bin 1
>> + * @hist3: the global normalized pixel count for bin 3
>> + * @hist4: the global normalized pixel count for bin 4
>> + */
>> +struct c3_isp_ae_zone_stats {
>> +     __u16 hist0;
>> +     __u16 hist1;
>> +     __u16 hist3;
>> +     __u16 hist4;
>> +};
>> +
>> +/**
>> + * struct c3_isp_ae_stats - Exposure statistics information
>> + *
>> + * AE statistical information consists of
>> + * all blocks information and a 1024-bin histogram.
>> + *
>> + * @stats: array of auto exposure block statistics
>> + * @reserved: undefined buffer space
>> + * @hist: a 1024-bin histogram for the entire image
>> + */
>> +struct c3_isp_ae_stats {
>> +     struct c3_isp_ae_zone_stats stats[C3_ISP_AE_MAX_ZONES];
>> +     __u32 reserved[2];
>> +     __u32 hist[1024];
>> +} __attribute__((aligned(16)));
>> +
>> +/**
>> + * struct c3_isp_af_zone_stats - AF statistics of a zone
>> + *
>> + * AF zone stats is aligned with 8 bytes.
>> + * The zonal accumulated contrast metrics are stored
>> + * in floating point format with 16 bits mantissa and
>> + * 5 or 6 bits exponent.
>> + * Apart from contrast metrics we accumulate squared image and
>> + * quartic image data over the zone.
>> + *
>> + * @i2_mat: the mantissa of zonal squared image pixel sum
>> + * @i4_mat: the mantissa of zonal quartic image pixel sum
>> + * @e4_mat: the mantissa of zonal multi-directional quartic edge sum
>> + * @e4_exp: the exponent of zonal multi-directional quartic edge sum
>> + * @i2_exp: the exponent of zonal squared image pixel sum
>> + * @i4_exp: the exponent of zonal quartic image pixel sum
>> + */
>> +struct c3_isp_af_zone_stats {
>> +     __u16 i2_mat;
>> +     __u16 i4_mat;
>> +     __u16 e4_mat;
>> +     __u16 e4_exp: 5;
>> +     __u16 i2_exp: 5;
>> +     __u16 i4_exp: 6;
>> +};
>> +
>> +/**
>> + * struct c3_isp_af_stats - Auto Focus statistics information
>> + *
>> + * AF statistical information of each zone
>> + *
>> + * @stats: array of auto focus block statistics
>> + * @reserved: undefined buffer space
>> + */
>> +struct c3_isp_af_stats {
>> +     struct c3_isp_af_zone_stats stats[C3_ISP_AF_MAX_ZONES];
>> +     __u32 reserved[2];
>> +} __attribute__((aligned(16)));
>> +
>> +/**
>> + * struct c3_isp_stats_buffer - V4L2_META_FMT_C3ISP_STATS
>> + *
>> + * Contains ISP statistics
>> + *
>> + * @awb: auto white balance stats
>> + * @ae: auto exposure stats
>> + * @af: auto focus stats
>> + */
>> +struct c3_isp_stats_buffer {
>> +     struct c3_isp_awb_stats awb;
>> +     struct c3_isp_ae_stats ae;
>> +     struct c3_isp_af_stats af;
>> +};
>> +
>> +/**
>> + * enum c3_isp_params_buffer_version -  C3 ISP parameters block versioning
>> + *
>> + * @C3_ISP_PARAMS_BUFFER_V0: First version of C3 ISP parameters block
>> + */
>> +enum c3_isp_params_buffer_version {
>> +     C3_ISP_PARAMS_BUFFER_V0,
>> +};
>> +
>> +/**
>> + * enum c3_isp_params_block_type - Enumeration of C3 ISP parameter blocks
>> + *
>> + * Each block configures a specific processing block of the C3 ISP.
>> + * The block type allows the driver to correctly interpret
>> + * the parameters block data.
> can be reflowed


Will re-flow

>> + *
>> + * @C3_ISP_PARAMS_BLOCK_AWB_GAINS: White balance gains
>> + * @C3_ISP_PARAMS_BLOCK_AWB_CONFIG: AE statistics parameters
> This is AWB


Aha,  will use AWB

>> + * @C3_ISP_PARAMS_BLOCK_AE_CONFIG: AE statistics parameters
>> + * @C3_ISP_PARAMS_BLOCK_AF_CONFIG: AF statistics parameters
> I would say "statistic format configuration" for all blocks that
> configure how stats are generated
>

OK, will modify these comments.

>> + * @C3_ISP_PARAMS_BLOCK_PST_GAMMA: post gamma parameters
>> + * @C3_ISP_PARAMS_BLOCK_CCM: Color correction matrix parameters
>> + * @C3_ISP_PARAMS_BLOCK_CSC: Color space conversion parameters
>> + * @C3_ISP_PARAMS_BLOCK_BLC: Black level correction parameters
>> + * @C3_ISP_PARAMS_BLOCK_SENTINEL: First non-valid block index
>> + */
>> +enum c3_isp_params_block_type {
>> +     C3_ISP_PARAMS_BLOCK_AWB_GAINS,
>> +     C3_ISP_PARAMS_BLOCK_AWB_CONFIG,
>> +     C3_ISP_PARAMS_BLOCK_AE_CONFIG,
>> +     C3_ISP_PARAMS_BLOCK_AF_CONFIG,
>> +     C3_ISP_PARAMS_BLOCK_PST_GAMMA,
>> +     C3_ISP_PARAMS_BLOCK_CCM,
>> +     C3_ISP_PARAMS_BLOCK_CSC,
>> +     C3_ISP_PARAMS_BLOCK_BLC,
>> +     C3_ISP_PARAMS_BLOCK_SENTINEL
>> +};
>> +
>> +#define C3_ISP_PARAMS_BLOCK_FL_NONE          0
> mmm, in other versions of the extensible format (rkisp1) there's an
> ENABLED and DISABLED flag. It seems to me you're using FL_NONE as
> FL_ENABLED here. I'm not sure what's best, as setting a field to FL_NONE
> seems weird, but also having to specify FL_ENABLED might be
> redundant...


Will check this issue and refer to rkisp1.

>> +#define C3_ISP_PARAMS_BLOCK_FL_DISABLED              BIT(0)
>> +
>> +/**
>> + * 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
>> + * 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
>> + *
>> + *   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_NONE;
> maybe explicitly saying FL_ENABLED is better ?
>
> I guess it also depends if your driver differentiate "update the
> config without enabling" from "apply a config and enable the block".
>
> I'll find out in later patches I presume...


I  will check this flag and wait your suggestion in later patches.

>> + *           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
>> + */
>> +struct c3_isp_params_block_header {
>> +     __u16 type;
>> +     __u16 flags;
>> +     __u32 size;
>> +};
>> +
>> +/**
>> + * struct c3_isp_params_awb_gains - Gains for auto-white balance
>> + *
>> + * This struct allows users to configure the gains for white balance. There
>> + * are four gain settings corresponding to each colour channel in the bayer domain.
> easy to reflow to 80 cols


Will re-flow to 80 cols.

>> + * All of the gains are stored in Q4.8 format.
>> + *
>> + * header.type should be set to C3_ISP_PARAMS_BLOCK_AWB_GAINS
>> + * from :c:type:`c3_isp_params_block_type`
>> + *
>> + * @header: The C3 ISP parameters block header
>> + * @gr_gain: Multiplier for Gr channel (Q4.8 format)
>> + * @r_gain: Multiplier for R channel (Q4.8 format)
>> + * @b_gain: Multiplier for B channel (Q4.8 format)
>> + * @gb_gain: Multiplier for Gb channel (Q4.8 format)
>> + */
>> +struct c3_isp_params_awb_gains {
>> +     struct c3_isp_params_block_header header;
>> +     __u16 gr_gain;
>> +     __u16 r_gain;
>> +     __u16 b_gain;
>> +     __u16 gb_gain;
>> +};
>> +
>> +/*
> If you want this documented by kernel-doc use /**


Will use  /**

>> + * enum c3_isp_params_awb_tap_points - Tap points for the AWB statistics
>> + * @C3_ISP_AWB_STATS_TAP_FE: immediately after the optical frontend block
>> + * @C3_ISP_AWB_STATS_TAP_GE: immediately after the green equal block
>> + * @C3_ISP_AWB_STATS_TAP_BEFORE_WB: immediately before the white balance block
>> + * @C3_ISP_AWB_STATS_TAP_AFTER_WB: immediately after the white balance block
>> + */
>> +enum c3_isp_params_awb_tap_point {
>> +     C3_ISP_AWB_STATS_TAP_OFE = 0,
>> +     C3_ISP_AWB_STATS_TAP_GE,
>> +     C3_ISP_AWB_STATS_TAP_BEFORE_WB,
>> +     C3_ISP_AWB_STATS_TAP_AFTER_WB,
>> +};
>> +
>> +/**
>> + * struct c3_isp_params_awb_config - Stats settings for auto-white balance
>> + *
>> + * This struct allows the configuration of the statistics generated for auto
>> + * white balance.
>> + *
>> + * header.type should be set to C3_ISP_PARAMS_BLOCK_AWB_CONFIG
>> + * from :c:type:`c3_isp_params_block_type`
>> + *
>> + * @header: the C3 ISP parameters block header
>> + * @tap_point: the tap point from enum c3_isp_params_awb_tap_point
>> + * @satur_vald: AWB statistic over saturation control
>> + *           value: 0: disable, 1: enable
>> + * @horiz_zones_num: active zones number of hotizontal [0..32]
> active number of horizontal zones


Will use  "active number of horizontal zones"

>> + * @vert_zones_num: active zones number of vertical [0..24]
> ditto


Will use "active number of vertical zones"

>> + * @rg_min: minimum R/G ratio (Q4.8 format)
>> + * @rg_max: maximum R/G ratio (Q4.8 format)
>> + * @bg_min: minimum B/G ratio (Q4.8 format)
>> + * @bg_max: maximum B/G ratio (Q4.8 format)
>> + * @rg_low: R/G ratio trim low (Q4.8 format)
>> + * @rg_high: R/G ratio trim hight (Q4.8 format)
>> + * @bg_low: B/G ratio trim low (Q4.8 format)
>> + * @bg_high: B/G ratio trim high (Q4.8 format)
>> + * @zone_weight: array of weights for AWB statistics zones [0..15]
>> + * @horiz_cood: the horizontal coordinate of point on the diagonal [0..2888]
>> + * @vert_cood: the vertical coordinate of point on the diagonal [0..2240]
>> + */
>> +struct c3_isp_params_awb_config {
>> +     struct c3_isp_params_block_header header;
>> +     __u8 tap_point;
>> +     __u8 satur_vald;
>> +     __u8 horiz_zones_num;
>> +     __u8 vert_zones_num;
>> +     __u16 rg_min;
>> +     __u16 rg_max;
>> +     __u16 bg_min;
>> +     __u16 bg_max;
>> +     __u16 rg_low;
>> +     __u16 rg_high;
>> +     __u16 bg_low;
>> +     __u16 bg_high;
>> +     __u8 zone_weight[C3_ISP_AWB_MAX_ZONES];
>> +     __u16 horiz_cood[C3_ISP_AWB_MAX_PT_NUM];
>> +     __u16 vert_cood[C3_ISP_AWB_MAX_PT_NUM];
>> +};
>> +
>> +/*
> ditto, use /** if you want this documented
>

OK, will use /**

>> + * enum c3_isp_params_ae_tap_points - Tap points for the AE statistics
>> + * @C3_ISP_AE_STATS_TAP_GE: immediately after the green equal block
>> + * @C3_ISP_AE_STATS_TAP_MLS: immediately after the mesh lens shading block
>> + */
>> +enum c3_isp_params_ae_tap_point {
>> +     C3_ISP_AE_STATS_TAP_GE = 0,
>> +     C3_ISP_AE_STATS_TAP_MLS,
>> +};
>> +
>> +/**
>> + * struct c3_isp_params_ae_config - Stats settings for auto-exposure
>> + *
>> + * This struct allows the configuration of the statistics generated for
>> + * auto exposure.
>> + *
>> + * header.type should be set to C3_ISP_PARAMS_BLOCK_AE_CONFIG
>> + * from :c:type:`c3_isp_params_block_type`
>> + *
>> + * @header: the C3 ISP parameters block header
>> + * @horiz_zones_num: active zones number of hotizontal [0..17]
> s/hotizontal/horizontal
>
> same comment as per the AWB stats


Will use "active number of horizontalzones"

>> + * @vert_zones_num: active zones number of vertical [0..15]
>> + * @tap_point: the tap point from enum c3_isp_params_ae_tap_point
>> + * @zone_weight: array of weights for AE statistics zones [0..15]
>> + * @horiz_cood: the horizontal coordinate of point on the diagonal [0..2888]
>> + * @vert_cood: the vertical coordinate of point on the diagonal [0..2240]
> I'm not sure how these fields are used, so I'm not sure if this should
> be 'point' or 'points'


I think should use 'points', because here has not only one point.

>> + */
>> +struct c3_isp_params_ae_config {
>> +     struct c3_isp_params_block_header header;
>> +     __u8 tap_point;
>> +     __u8 horiz_zones_num;
>> +     __u8 vert_zones_num;
>> +     __u8 zone_weight[C3_ISP_AE_MAX_ZONES];
>> +     __u16 horiz_cood[C3_ISP_AE_MAX_PT_NUM];
>> +     __u16 vert_cood[C3_ISP_AE_MAX_PT_NUM];
>> +};
> When passed through pahole this gives me
>
>          /* size: 340, cachelines: 6, members: 7 */
>          /* padding: 2 */
>
> Because the compile aligns to the larger members in the struct which
> is a __u32.
>
> so I think you should
>
>          __u8 reserved[2];
>
> at the end of the structure to avoid the compiler insert padding
> bytes.
>
> Generally, for uAPI it's a good idea to avoid implicit alignments and
> holes, and possibly align to 8 bytes.
>
> This is a new format, and it's extensible, so it's versioned, but I
> would rather make sure all holes/fields are declared as reserved and
> the driver requires to have them set to 0 by userspace.
>
> Reason is, assume you add a field to this structure in a future kernel
> revision: existing userspace compiled against an older header version
> will set the fields to 0 and the driver can ignore them, newer
> userspace can populate them and the driver can use them. Relying on
> implicit padding means that if you have to extend this single
> structure a new version of the extensible should be declared and the
> driver prepared to handle both, while I do expect the extensible
> format version to be updated only when a major change to the data
> layout (or the header) is performed.
>
> All in all I would:
> 1) Add __attribute__((aligned(8))); to all types
> 2) Check with paholes is there are holes or padding bytes and declare
> them as reserved, documenting that those fields should be set to 0 by
> userspace.


OK,  will resolve this issue.

> The discussion we had with Hans and Laurent to provide some background
> https://lore.kernel.org/all/20240724085004.82694-1-jacopo.mondi@ideasonboard.com/T/#m5333e7b6ab4230bb32efa063ab6fd73a529fdc29
> (I hope I got all points right).
>
> For pahole, what I've done is simply
>
> -------------------- main.c -------------------------------------
> #include "c3-isp-config.h"
>
> int main(int argc, char *argv[])
> {
>          struct c3_isp_params_awb_gains awb_gains;
>          struct c3_isp_params_awb_config awb_config;
>          struct c3_isp_params_ae_config ae_config;
>          struct c3_isp_params_af_config af_config;
>          struct c3_isp_params_pst_gamma gamm;
>          struct c3_isp_params_ccm ccm;
>          struct c3_isp_params_csc csc;
>          struct c3_isp_params_blc blc;
>
>          return 0;
> }
> -------------------- main.c -------------------------------------
>
> $ gcc -g main.c
> $ pahole ./a.out


Will use pahole to test it.

>> +
>> +/*
>> + * enum c3_isp_params_af_tap_points - Tap points for the AF statistics
>> + * @C3_ISP_AF_STATS_TAP_SNR: immediately after the spatial noise reduce block
>> + * @C3_ISP_AF_STATS_TAP_DMS: immediately after the demosaic block
>> + */
>> +enum c3_isp_params_af_tap_point {
>> +     C3_ISP_AF_STATS_TAP_SNR = 0,
>> +     C3_ISP_AF_STATS_TAP_DMS,
>> +};
>> +
>> +/**
>> + * struct c3_isp_params_af_config - Stats settings for auto-focus
>> + *
>> + * This struct allows the configuration of the statistics generated for
>> + * auto focus.
>> + *
>> + * header.type should be set to C3_ISP_PARAMS_BLOCK_AF_CONFIG
>> + * from :c:type:`c3_isp_params_block_type`
>> + *
>> + * @header: the C3 ISP parameters block header
>> + * @tap_point: the tap point from enum c3_isp_params_af_tap_point
>> + * @horiz_zones_num: active zones number of hotizontal [0..17]
>> + * @vert_zones_num: active zones number of vertical [0..15]
>> + * @horiz_cood: the horizontal coordinate of point on the diagonal [0..2888]
>> + * @vert_cood: the vertical coordinate of point on the diagonal [0..2240]
> same comments as above about 'point' or 'points'
>

I think should use 'points', because here has not only one point.

>> + */
>> +struct c3_isp_params_af_config {
>> +     struct c3_isp_params_block_header header;
>> +     __u8 tap_point;
>> +     __u8 horiz_zones_num;
>> +     __u8 vert_zones_num;
> Pahole points out an hole here
>
>          /* XXX 1 byte hole, try to pack */


Will add __u8 reserved

>> +     __u16 horiz_cood[C3_ISP_AF_MAX_PT_NUM];
> Because this should be aligned to 2 bytes boundary. Add a reserved
> field in between.
>

Will add a reserved.

>> +     __u16 vert_cood[C3_ISP_AF_MAX_PT_NUM];
>> +};
>> +
>> +/**
>> + * struct c3_isp_params_pst_gamma - Post gamma configuration
>> + *
>> + * This struct allows the configuration of the look up table for
>> + * post gamma. The gamma curve consists of 129 points, so need to
>> + * set lut[129].
>> + *
>> + * header.type should be set to C3_ISP_PARAMS_BLOCK_PST_GAMMA
>> + * from :c:type:`c3_isp_params_block_type`
>> + *
>> + * @header: the C3 ISP parameters block header
>> + * @lut: look up table for P-Stitch gamma [0..1023]
> lookup


Will use 'lookup'

>> + */
>> +struct c3_isp_params_pst_gamma {
>> +     struct c3_isp_params_block_header header;
>> +     __u16 lut[129];
>> +};
> Check padding
>

Will check padding.

>> +
>> +/**
>> + * struct c3_isp_params_ccm - ISP CCM configuration
>> + *
>> + * This struct allows the configuration of the matrix for
>> + * color correction. The matrix consists of 3 x 3 points,
>> + * so need to set matrix[3][3].
>> + *
>> + * header.type should be set to C3_ISP_PARAMS_BLOCK_CCM
>> + * from :c:type:`c3_isp_params_block_type`
>> + *
>> + * @header: the C3 ISP parameters block header
>> + * @matrix: a 3 x 3 matrix used for color correction,
>> + *          the value of matrix[x][y] is orig_value x 256. [-4096..4095]
>> + */
>> +struct c3_isp_params_ccm {
>> +     struct c3_isp_params_block_header header;
>> +     __s16 matrix[3][3];
> check padding here as well
>

Will check padding.

>> +};
>> +
>> +/**
>> + * struct c3_isp_params_csc - ISP Color Space Conversion configuration
>> + *
>> + * This struct allows the configuration of the matrix for
>> + * color space conversion. The matrix consists of 3 x 3 points,
>> + * so need to set matrix[3][3].
>> + *
>> + * header.type should be set to C3_ISP_PARAMS_BLOCK_CSC
>> + * from :c:type:`c3_isp_params_block_type`
>> + *
>> + * @header: the C3 ISP parameters block header
>> + * @matrix: a 3x3 matrix used for the color space conversion,
>> + *          the value of matrix[x][y] is orig_value x 256. [-4096..4095]
>> + */
>> +struct c3_isp_params_csc {
>> +     struct c3_isp_params_block_header header;
>> +     __s16 matrix[3][3];
>> +};
> Check padding here as well
>

Will check padding.

>> +
>> +/**
>> + * struct c3_isp_params_blc - ISP Black Level Correction configuration
>> + *
>> + * This struct allows the configuration of the block level offset
>> + * for each color channel.
>> + *
>> + * header.type should be set to C3_ISP_PARAMS_BLOCK_BLC
>> + * from :c:type:`c3_isp_params_block_type`
>> + *
>> + * @header: the C3 ISP parameters block header
>> + * @gr_ofst: Gr blc offset (Q4.8 format)
>> + * @r_ofst: R blc offset (Q4.8 format)
>> + * @b_ofst: B blc offset (Q4.8 format)
>> + * @gb_ofst: Gb blc offset(Q4.8 format)
>> + */
>> +struct c3_isp_params_blc {
>> +     struct c3_isp_params_block_header header;
>> +     __u16 gr_ofst;
>> +     __u16 r_ofst;
>> +     __u16 b_ofst;
>> +     __u16 gb_ofst;
>> +};
>> +
>> +/**
>> + * define C3_ISP_PARAMS_MAX_SIZE - Maximum size of all C3 ISP Parameters
>> + *
>> + * Though the parameters for the C3 ISP are passed as optional blocks, the
>> + * driver still needs to know the absolute maximum size so that it can allocate
>> + * a buffer sized appropriately to accommodate userspace attempting to set all
>> + * possible parameters in a single frame.
>> + */
>> +#define C3_ISP_PARAMS_MAX_SIZE                           \
>> +     (sizeof(struct c3_isp_params_awb_gains) +        \
>> +     sizeof(struct c3_isp_params_awb_config) +        \
>> +     sizeof(struct c3_isp_params_ae_config) +         \
>> +     sizeof(struct c3_isp_params_af_config) +         \
>> +     sizeof(struct c3_isp_params_pst_gamma) +         \
>> +     sizeof(struct c3_isp_params_ccm) +               \
>> +     sizeof(struct c3_isp_params_csc) +               \
>> +     sizeof(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.
>> + *
>> + * The expected memory layout of the parameters buffer is::
>> + *
>> + *   +-------------------- struct c3_isp_params_cfg ---- ------------------+
>> + *   | version = C3_ISP_PARAM_BUFFER_V0;                                   |
>> + *   | data_size = sizeof(struct c3_isp_params_awb_gains) +                |
>> + *   |              sizeof(struct c3_isp_params_awb_config);       |
>> + *   | +------------------------- data  ---------------------------------+ |
>> + *   | | +------------ struct c3_isp_params_awb_gains) ------------------+ |
>> + *   | | | +---------  struct c3_isp_params_block_header header -----+ | | |
>> + *   | | | | type = C3_ISP_PARAMS_BLOCK_AWB_GAINS;                   | | | |
>> + *   | | | | flags = C3_ISP_PARAMS_BLOCK_FL_NONE;                    | | | |
>> + *   | | | | size = sizeof(struct c3_isp_params_awb_gains);          | | | |
>> + *   | | | +---------------------------------------------------------+ | | |
>> + *   | | | gr_gain = ...;                                              | | |
>> + *   | | | r_gain = ...;                                               | | |
>> + *   | | | b_gain = ...;                                               | | |
>> + *   | | | gb_gain = ...;                                              | | |
>> + *   | | +------------------ struct c3_isp_params_awb_config ----------+ | |
>> + *   | | | +---------- struct c3_isp_param_block_header header ------+ | | |
>> + *   | | | | type = C3_ISP_PARAMS_BLOCK_AWB_CONFIG;                  | | | |
>> + *   | | | | flags = C3_ISP_PARAMS_BLOCK_FL_NONE;                    | | | |
>> + *   | | | | size = sizeof(struct c3_isp_params_awb_config)          | | | |
>> + *   | | | +---------------------------------------------------------+ | | |
>> + *   | | | tap_point = ...;                                            | | |
>> + *   | | | satur_vald = ...;                                           | | |
>> + *   | | | horiz_zones_num = ...;                                      | | |
>> + *   | | | vert_zones_num = ...;                                       | | |
>> + *   | | +-------------------------------------------------------------+ | |
>> + *   | +-----------------------------------------------------------------+ |
>> + *   +---------------------------------------------------------------------+
>> + *
>> + * @version: The C3 ISP parameters buffer version
>> + * @data_size: The C3 ISP configuration data effective size,
>> + *           excluding this header
> can be reflowed.


Will re-flow

> Thanks
>    j
>
>> + * @data: The C3 ISP configuration blocks data
>> + */
>> +struct c3_isp_params_cfg {
>> +     __u8 version;
>> +     __u32 data_size;
>> +     __u8 data[C3_ISP_PARAMS_MAX_SIZE];
>> +};
>> +
>> +#endif
>>
>> --
>> 2.47.0
>>
>>
>>