[PATCH v2 2/2] libcamera: ipa: Add C3 ISP IPA

Keke Li keke.li at amlogic.com
Wed Feb 5 12:46:54 CET 2025


Hi Dan

Thanks for your reply.

On 2025/1/22 22:32, Dan Scally wrote:
> [ EXTERNAL EMAIL ]
>
> Hi Keke, thanks for the patch
>
> On 27/12/2024 10:58, Keke Li wrote:
>> The C3 ISP IPA is used to process 3A statistics
>> and generate parameters for ISP hardware.
>>
>> Signed-off-by: Keke Li <keke.li at amlogic.com>
>> ---
>>   include/linux/c3-isp-config.h            | 564 +++++++++++++++++++++++
>>   meson_options.txt                        |   2 +-
>>   src/ipa/c3-isp/algorithms/agc.cpp        | 260 +++++++++++
>>   src/ipa/c3-isp/algorithms/agc.h          |  50 ++
>>   src/ipa/c3-isp/algorithms/algorithm.h    |  31 ++
>>   src/ipa/c3-isp/algorithms/awb.cpp        | 257 +++++++++++
>>   src/ipa/c3-isp/algorithms/awb.h          |  42 ++
>>   src/ipa/c3-isp/algorithms/blc.cpp        | 102 ++++
>>   src/ipa/c3-isp/algorithms/blc.h          |  40 ++
>>   src/ipa/c3-isp/algorithms/ccm.cpp        |  86 ++++
>>   src/ipa/c3-isp/algorithms/ccm.h          |  40 ++
>>   src/ipa/c3-isp/algorithms/csc.cpp        |  64 +++
>>   src/ipa/c3-isp/algorithms/csc.h          |  32 ++
>>   src/ipa/c3-isp/algorithms/meson.build    |  10 +
>>   src/ipa/c3-isp/algorithms/post_gamma.cpp |  64 +++
>>   src/ipa/c3-isp/algorithms/post_gamma.h   |  33 ++
>>   src/ipa/c3-isp/c3-isp.cpp                | 386 ++++++++++++++++
>>   src/ipa/c3-isp/data/imx290.yaml          |  30 ++
>>   src/ipa/c3-isp/data/meson.build          |   9 +
>>   src/ipa/c3-isp/ipa_context.cpp           | 251 ++++++++++
>>   src/ipa/c3-isp/ipa_context.h             | 110 +++++
>>   src/ipa/c3-isp/meson.build               |  32 ++
>>   src/ipa/c3-isp/module.h                  |  28 ++
>>   src/ipa/c3-isp/params.cpp                | 127 +++++
>>   src/ipa/c3-isp/params.h                  | 133 ++++++
> This is a lot of additions for a single commit. It might be 
> administratively easier to break them
> down; the header file should certainly have its own commit, and each 
> of the algorithms could also
> very easily have their own commit. It would be nice also to have 
> additions for libtuning if its
> possible so that users can generate tuning files for this IPA 
> automatically.


OK, will break them down.

>>   25 files changed, 2782 insertions(+), 1 deletion(-)
>>   create mode 100644 include/linux/c3-isp-config.h
>>   create mode 100644 src/ipa/c3-isp/algorithms/agc.cpp
>>   create mode 100644 src/ipa/c3-isp/algorithms/agc.h
>>   create mode 100644 src/ipa/c3-isp/algorithms/algorithm.h
>>   create mode 100755 src/ipa/c3-isp/algorithms/awb.cpp
>>   create mode 100755 src/ipa/c3-isp/algorithms/awb.h
>>   create mode 100644 src/ipa/c3-isp/algorithms/blc.cpp
>>   create mode 100644 src/ipa/c3-isp/algorithms/blc.h
>>   create mode 100644 src/ipa/c3-isp/algorithms/ccm.cpp
>>   create mode 100644 src/ipa/c3-isp/algorithms/ccm.h
>>   create mode 100644 src/ipa/c3-isp/algorithms/csc.cpp
>>   create mode 100644 src/ipa/c3-isp/algorithms/csc.h
>>   create mode 100644 src/ipa/c3-isp/algorithms/meson.build
>>   create mode 100644 src/ipa/c3-isp/algorithms/post_gamma.cpp
>>   create mode 100644 src/ipa/c3-isp/algorithms/post_gamma.h
>>   create mode 100644 src/ipa/c3-isp/c3-isp.cpp
>>   create mode 100644 src/ipa/c3-isp/data/imx290.yaml
>>   create mode 100644 src/ipa/c3-isp/data/meson.build
>>   create mode 100644 src/ipa/c3-isp/ipa_context.cpp
>>   create mode 100644 src/ipa/c3-isp/ipa_context.h
>>   create mode 100644 src/ipa/c3-isp/meson.build
>>   create mode 100644 src/ipa/c3-isp/module.h
>>   create mode 100644 src/ipa/c3-isp/params.cpp
>>   create mode 100644 src/ipa/c3-isp/params.h
>>
>> diff --git a/include/linux/c3-isp-config.h 
>> b/include/linux/c3-isp-config.h
>> new file mode 100644
>> index 00000000..ee673ed0
>> --- /dev/null
>> +++ b/include/linux/c3-isp-config.h
> As above; this needs to go in its own commit. The commit message 
> should emphasise that it's a
> temporary addition - once the driver is merged to the kernel then 
> it'll be handled by a script we have.


OK, will indicate that it's a temporary addition.

>> @@ -0,0 +1,564 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
>> +/*
>> + * 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)));
>> +
>> +/**
>> + * 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.
>> + * 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_info - 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_info {
>> +     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.
>> + *
>> + * @C3_ISP_PARAMS_BLOCK_AWB_GAINS: White balance gains
>> + * @C3_ISP_PARAMS_BLOCK_AWB_CONFIG: AWB statistic format 
>> configuration for all
>> + *                                  blocks that control how stats 
>> are generated
>> + * @C3_ISP_PARAMS_BLOCK_AE_CONFIG: AE statistic format configuration 
>> for all
>> + *                                 blocks that control how stats are 
>> generated
>> + * @C3_ISP_PARAMS_BLOCK_AF_CONFIG: AF statistic format configuration 
>> for all
>> + *                                 blocks that control how stats are 
>> generated
>> + * @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_DISABLE (1U << 0)
>> +#define C3_ISP_PARAMS_BLOCK_FL_ENABLE (1U << 1)
>> +
>> +/**
>> + * 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_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
>> + */
>> +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. 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;
>> +} __attribute__((aligned(8)));
>> +
>> +/**
>> + * 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 number of hotizontal zones [0..32]
>> + * @vert_zones_num: active number of vertical zones [0..24]
>> + * @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 points on the diagonal 
>> [0..2888]
>> + * @vert_cood: the vertical coordinate of points 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];
>> +} __attribute__((aligned(8)));
>> +
>> +/**
>> + * 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 number of horizontal zones [0..17]
>> + * @vert_zones_num: active number of vertical zones [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 points on the diagonal 
>> [0..2888]
>> + * @vert_cood: the vertical coordinate of points on the diagonal 
>> [0..2240]
>> + * @reserved: applications must zero this array
>> + */
>> +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];
>> +     __u16 reserved[3];
>> +} __attribute__((aligned(8)));
>> +
>> +/**
>> + * 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 number of hotizontal zones [0..17]
>> + * @vert_zones_num: active number of vertical zones [0..15]
>> + * @reserved: applications must zero this array
>> + * @horiz_cood: the horizontal coordinate of points on the diagonal 
>> [0..2888]
>> + * @vert_cood: the vertical coordinate of points on the diagonal 
>> [0..2240]
>> + */
>> +struct c3_isp_params_af_config {
>> +     struct c3_isp_params_block_header header;
>> +     __u8 tap_point;
>> +     __u8 horiz_zones_num;
>> +     __u8 vert_zones_num;
>> +     __u8 reserved[5];
>> +     __u16 horiz_cood[C3_ISP_AF_MAX_PT_NUM];
>> +     __u16 vert_cood[C3_ISP_AF_MAX_PT_NUM];
>> +} __attribute__((aligned(8)));
>> +
>> +/**
>> + * 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: lookup table for P-Stitch gamma [0..1023]
>> + * @reserved: applications must zero this array
>> + */
>> +struct c3_isp_params_pst_gamma {
>> +     struct c3_isp_params_block_header header;
>> +     __u16 lut[129];
>> +     __u16 reserved[3];
>> +} __attribute__((aligned(8)));
>> +
>> +/**
>> + * 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]
>> + * @reserved: applications must zero this array
>> + */
>> +struct c3_isp_params_ccm {
>> +     struct c3_isp_params_block_header header;
>> +     __s16 matrix[3][3];
>> +     __u16 reserved[3];
>> +} __attribute__((aligned(8)));
>> +
>> +/**
>> + * 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]
>> + * @reserved: applications must zero this array
>> + */
>> +struct c3_isp_params_csc {
>> +     struct c3_isp_params_block_header header;
>> +     __s16 matrix[3][3];
>> +     __u16 reserved[3];
>> +} __attribute__((aligned(8)));
>> +
>> +/**
>> + * 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
>> + * @data: The C3 ISP configuration blocks data
>> + */
>> +struct c3_isp_params_cfg {
>> +     __u32 version;
>> +     __u32 data_size;
>> +     __u8 data[C3_ISP_PARAMS_MAX_SIZE];
>> +};
>> +
>> +#endif
>> diff --git a/meson_options.txt b/meson_options.txt
>> index d59f4c04..352f981b 100644
>> --- a/meson_options.txt
>> +++ b/meson_options.txt
>> @@ -32,7 +32,7 @@ option('gstreamer',
>>
>>   option('ipas',
>>           type : 'array',
>> -        choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/vc4', 
>> 'simple', 'vimc'],
>> +        choices : ['c3-isp', 'ipu3', 'mali-c55', 'rkisp1', 
>> 'rpi/vc4', 'simple', 'vimc'],
>>           description : 'Select which IPA modules to build')
>>
>>   option('lc-compliance',
>> diff --git a/src/ipa/c3-isp/algorithms/agc.cpp 
>> b/src/ipa/c3-isp/algorithms/agc.cpp
>> new file mode 100644
>> index 00000000..16eb0b46
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/agc.cpp
>> @@ -0,0 +1,260 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP AGC/AEC mean-based control algorithm
>> + */
>> +
>> +#include "agc.h"
>> +
>> +#include <algorithm>
>> +#include <chrono>
>> +#include <cmath>
>> +#include <tuple>
>> +#include <vector>
>> +
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/base/utils.h>
>> +
>> +#include <libcamera/control_ids.h>
>> +
>> +#include <libcamera/ipa/core_ipa_interface.h>
>> +
>> +#include "libcamera/internal/yaml_parser.h"
>> +
>> +#include "libipa/histogram.h"
>> +
>> +/**
>> + * \file agc.h
>> + */
>> +
>> +namespace libcamera {
>> +
>> +using namespace std::literals::chrono_literals;
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +/**
>> + * \class Agc
>> + * \brief A mean-based auto-exposure algorithm
>> + */
>> +
>> +LOG_DEFINE_CATEGORY(C3ISPAgc)
>> +
>> +Agc::Agc()
>> +     : horizonalZonesNum_(17), verticalZonesNum_(15)
>> +{
>> +}
>> +
>> +/**
>> + * \brief Initialise the AGC algorithm from tuning files
>> + * \param[in] context The shared IPA context
>> + * \param[in] tuningData The YamlObject containing Agc tuning data
>> + *
>> + * This function calls the base class' tuningData parsers to 
>> discover which
>> + * control values are supported.
>> + *
>> + * \return 0 on success or errors from the base class
>> + */
>> +int Agc::init(IPAContext &context,
>> +           const YamlObject &tuningData)
>> +{
>> +     int ret;
>> +
>> +     ret = parseTuningData(tuningData);
>> +     if (ret)
>> +             return ret;
>> +
>> +     context.ctrlMap.merge(controls());
>> +
>> +     return 0;
>> +}
> No AeEnable control? So it can only do automatic exposure, not manual?


Will add AeEnable control.

>> +
>> +/**
>> + * \brief Configure the AGC given a configInfo
>> + * \param[in] context The shared IPA context
>> + * \param[in] configInfo The IPA configuration data
>> + *
>> + * \return 0
>> + */
>> +int Agc::configure(IPAContext &context,
>> +                [[maybe_unused]] const IPACameraSensorInfo &configInfo)
>> +{
>> +     const IPASessionConfiguration &configuration = 
>> context.configuration;
>> +     IPAActiveState &activeState = context.activeState;
>> +
>> +     /* Configure the default gain and exposure */
>> +     activeState.agc.gain = configuration.sensor.minAnalogueGain;
>> +     activeState.agc.exposure = 10ms / 
>> configuration.sensor.lineDuration;
> In IPAC3ISP::updateControls() you get the actual default exposure from 
> the sensor's V4L2 controls -
> can we use that instead?


Will check this issue.

>> +
>> +     activeState.agc.constraintMode = constraintModes().begin()->first;
>> +     activeState.agc.exposureMode = 
>> exposureModeHelpers().begin()->first;
>> +
>> +     setLimits(configuration.sensor.minShutterSpeed,
>> +               configuration.sensor.maxShutterSpeed,
>> +               configuration.sensor.minAnalogueGain,
>> +               configuration.sensor.maxAnalogueGain);
>> +
>> +     resetFrameCount();
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::prepare
>> + */
>> +void Agc::prepare(IPAContext &context, const uint32_t frame,
>> +               [[maybe_unused]] IPAFrameContext &frameContext,
>> +               C3ISPParams *params)
>> +{
>> +     if (frame)
>> +             return;
>> +
>> +     auto AeCfg = params->block<BlockType::AEConfig>();
>> +     AeCfg.setEnabled(C3_ISP_PARAMS_BLOCK_FL_ENABLE);
>> +
>> +     AeCfg->tap_point = C3_ISP_AE_STATS_TAP_MLS;
>> +     AeCfg->horiz_zones_num = horizonalZonesNum_;
>> +     AeCfg->vert_zones_num = verticalZonesNum_;
>> +
>> +     for (unsigned int i = 0; i < AeCfg->horiz_zones_num * 
>> AeCfg->vert_zones_num; i++)
>> +             AeCfg->zone_weight[i] = 1;
> unsigned int numZones = horizonalZonesNum_ * verticalZonesNum_;
> Span<uint8_t> weights{ AeCfg->zone_weight, numZones };
>
> std::fill(weights.begin(), weights.end(), 1);
>
>
> Same elsewhere in the code that you fill the weights array
>

Will use this method.

>> +
>> +     Size sensorSize = context.configuration.sensor.size;
>> +     uint8_t maxPointNum = std::max(AeCfg->horiz_zones_num, 
>> AeCfg->vert_zones_num) + 1;
>> +
>> +     for (unsigned int i = 0; i < maxPointNum; i++) {
>> +             uint16_t hidx = i * sensorSize.width / 
>> AeCfg->horiz_zones_num;
>> +
>> +             /* Aligned with 2 */
>> +             hidx = hidx / 2 * 2;
>> +             AeCfg->horiz_cood[i] = std::min(hidx, 
>> (uint16_t)sensorSize.width);
> Mild preference for "coord" over "cood" - I suppose that applies to 
> the config header too.


OK, will use "coord".

>> +
>> +             uint16_t vidx = i * sensorSize.height / 
>> AeCfg->vert_zones_num;
>> +
>> +             /* Aligned with 2 */
>> +             vidx = vidx / 2 * 2;
>> +             AeCfg->vert_cood[i] = std::min(vidx, 
>> (uint16_t)sensorSize.height);
> Same here


Will use "coord".

>> +     }
>> +}
>> +
>> +Histogram Agc::parseStatistics(const c3_isp_stats_info *stats)
>> +{
>> +     const struct c3_isp_ae_stats *info = &stats->ae;
>> +     uint16_t zonesNum = horizonalZonesNum_ * verticalZonesNum_;
>> +     std::vector<uint8_t> means(zonesNum, 0);
>> +
>> +     /*
>> +      * Each zone has a 5-bin histogram and the
>> +      * total sum is normalized to 65535.
>> +      * For the convenience of calculation,
>> +      * it can be assumed that:
>> +      * hist0 represents the number of brightness 0,
> Not 32? I would normally expect the centre of the bin to be targeted


Will reconsider this issue.

>> +      * hist1 represents the number of brightness 64,
>> +      * hist2 represents the number of brightness 128,
>> +      * hist3 represents the number of brightness 192,
>> +      * hist4 represents the number of brightness 255.
>> +      *
>> +      * Finally, the average brightness of a zone can be calculated.
>> +      */
>> +     for (unsigned int i = 0; i < zonesNum; i++) {
>> +             uint16_t hist2 = 65535 - info->stats[i].hist0 - 
>> info->stats[i].hist1 - info->stats[i].hist3 - info->stats[i].hist4;
>> +
>> +             uint32_t lumaSum = info->stats[i].hist0 * 0 + 
>> info->stats[i].hist1 * 64 + hist2 * 128 + info->stats[i].hist3 * 192 
>> + info->stats[i].hist4 * 255;
>> +
>> +             means[i] = lumaSum / 65535;
>
> This could do with a touch of tidying. How about something like...
>
>
>     for (unsigned int i = 0; i < zonesNum; i++) {
>         uint16_t hist2 = 65535 - info->stats[i].hist0
>                    - info->stats[i].hist1
>                    - info->stats[i].hist3
>                    - info->stats[i].hist4;
>
>         uint32_t lumaSum = info->stats[i].hist0 * 0
>                  + info->stats[i].hist1 * 64
>                  + hist2 * 128
>                  + info->stats[i].hist3 * 192
>                  + info->stats[i].hist4 * 255;
>
>         means[i] = lumaSum / 65535;
>     }


Will tidy.

>> +     }
>> +
>> +     lumaMeans_ = means;
>> +
>> +     /* This is a 1024-bin histogram */
>> +     uint32_t *hist = const_cast<uint32_t *>(info->hist);
>> +
>> +     return Histogram(Span<uint32_t>(hist, std::size(info->hist)));
>> +}
>> +
>> +/**
>> + * \brief Estimate the relative luminance of the frame with a given 
>> gain
>> + * \param[in] gain The gain to apply in estimating luminance
>> + *
>> + * The estimation is based on the average value of the zone. Each
>> + * average value is multiplied by the gain, and then saturated to
>> + * to approximate the sensor behaviour at high brightness values.
>> + * The approximation is quite rough, as it doesn't take into account
>> + * non-linearities when approaching saturation.
>> + *
>> + * The values are normalized to the [0.0, 1.0] range, where 1.0 
>> corresponds
>> + * to a theoretical perfect reflector of 100% reference white.
>> + *
>> + * More detailed information can be found in:
>> + * https://en.wikipedia.org/wiki/Relative_luminance
>> + *
>> + * \return The relative luminance of the frame
>> + */
>> +double Agc::estimateLuminance(double gain) const
>> +{
>> +     double sum = 0.0;
>> +
>> +     for (unsigned int i = 0; i < lumaMeans_.size(); i++)
>> +             sum += std::min(lumaMeans_[i] * gain, 255.0);
>> +
>> +     return sum / lumaMeans_.size() / 255;
>> +}
>> +/**
>> + * \brief Process C3 ISP statistics, and run AGC operations
>> + * \param[in] context The shared IPA context
>> + * \param[in] frame The current frame sequence number
>> + * \param[in] frameContext The current frame context
>> + * \param[in] stats The C3 ISP statistics and ISP results
>> + * \param[out] metadata Metadata for the frame, to be filled by the 
>> algorithm
>> + *
>> + * Identify the current image brightness, and use that to estimate 
>> the optimal
>> + * new exposure and gain for the scene.
>> + */
>> +void Agc::process(IPAContext &context, [[maybe_unused]] const 
>> uint32_t frame,
>> +               IPAFrameContext &frameContext,
>> +               const c3_isp_stats_info *stats,
>> +               ControlList &metadata)
>> +{
>> +     Histogram hist = parseStatistics(stats);
>> +
>> +     utils::Duration shutterTime;
>> +     double aGain, dGain;
>> +
>> +     /*
>> +      * The Agc algorithm needs to know the effective exposure value 
>> that was
>> +      * applied to the sensor when the statistics were collected.
>> +      */
>> +     utils::Duration exposureTime = 
>> context.configuration.sensor.lineDuration * 
>> frameContext.sensor.exposure;
>> +     double analogueGain = frameContext.sensor.gain;
>> +     utils::Duration effectiveExposureValue = exposureTime * 
>> analogueGain;
>> +
>> +     std::tie(shutterTime, aGain, dGain) =
>> + calculateNewEv(context.activeState.agc.constraintMode,
>> + context.activeState.agc.exposureMode, hist,
>> +                            effectiveExposureValue);
>> +
>> +     LOG(C3ISPAgc, Debug)
>> +             << "Shutter time, analogue gain and digital gain are "
>> +             << shutterTime << ", " << aGain << " and " << dGain;
>> +
>> +     IPAActiveState &activeState = context.activeState;
>> +
>> +     activeState.agc.exposure = shutterTime / 
>> context.configuration.sensor.lineDuration;
>> +     activeState.agc.gain = aGain;
>> +
>> +     metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
>> +     metadata.set(controls::ExposureTime, 
>> exposureTime.get<std::micro>());
>> +
>> +     uint32_t vTotal = context.configuration.sensor.size.height + 
>> context.configuration.sensor.defVBlank;
>> +     utils::Duration frameDuration = 
>> context.configuration.sensor.lineDuration * vTotal;
>> +     metadata.set(controls::FrameDuration, 
>> frameDuration.get<std::micro>());
>> +
>> +     lumaMeans_ = {};
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(Agc, "Agc")
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/agc.h 
>> b/src/ipa/c3-isp/algorithms/agc.h
>> new file mode 100644
>> index 00000000..b0d1b500
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/agc.h
>> @@ -0,0 +1,50 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP AGC/AEC mean-based control algorithm
>> + */
>> +
>> +#pragma once
>> +
>> +#include <linux/c3-isp-config.h>
>> +
>> +#include <libcamera/base/span.h>
>> +#include <libcamera/base/utils.h>
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include "libipa/agc_mean_luminance.h"
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +class Agc : public Algorithm, public AgcMeanLuminance
>> +{
>> +public:
>> +     Agc();
>> +     ~Agc() = default;
>> +
>> +     int init(IPAContext &context, const YamlObject &tuningData) 
>> override;
>> +     int configure(IPAContext &context, const IPACameraSensorInfo 
>> &configInfo) override;
>> +     void prepare(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext,
>> +                  C3ISPParams *params) override;
>> +     void process(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext,
>> +                  const c3_isp_stats_info *stats,
>> +                  ControlList &metadata) override;
>> +private:
>> +     Histogram parseStatistics(const c3_isp_stats_info *stats);
>> +     double estimateLuminance(double gain) const override;
>> +
>> +     std::vector<uint8_t> lumaMeans_;
>> +     uint8_t horizonalZonesNum_;
>> +     uint8_t verticalZonesNum_;
>> +};
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/algorithm.h 
>> b/src/ipa/c3-isp/algorithms/algorithm.h
>> new file mode 100644
>> index 00000000..eb40512c
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/algorithm.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP control algorithm interface
>> + */
>> +
>> +#pragma once
>> +
>> +#include <libipa/algorithm.h>
>> +
>> +#include "module.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp {
>> +
>> +class Algorithm : public libcamera::ipa::Algorithm<Module>
>> +{
>> +public:
>> +     Algorithm()
>> +             : disabled_(false)
>> +     {
>> +     }
>> +
>> +     bool disabled_;
>> +};
>> +
>> +} /* namespace ipa::c3isp */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/awb.cpp 
>> b/src/ipa/c3-isp/algorithms/awb.cpp
>> new file mode 100755
>> index 00000000..f8ae3c1f
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/awb.cpp
>> @@ -0,0 +1,257 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP AWB control algorithm
>> + */
>> +
>> +#include "awb.h"
>> +
>> +#include <algorithm>
>> +#include <ios>
> Is this used?


Will remove this.

>> +
>> +#include <libcamera/base/log.h>
>> +
>> +#include <libcamera/control_ids.h>
>> +
>> +#include <libcamera/ipa/core_ipa_interface.h>
>> +
>> +/**
>> + * \file awb.h
>> + */
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +/**
>> + * \class Awb
>> + * \brief A Grey world white balance correction algorithm
>> + */
>> +
>> +LOG_DEFINE_CATEGORY(C3ISPAwb)
>> +
>> +Awb::Awb()
>> +     : horizonalZonesNum_(32), verticalZonesNum_(24)
>> +{
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::configure
>> + */
>> +int Awb::configure(IPAContext &context,
>> +                [[maybe_unused]] const IPACameraSensorInfo &configInfo)
>> +{
>> +     IPAActiveState &activeState = context.activeState;
>> +
>> +     activeState.awb.gains.manual.red = 1.0;
>> +     activeState.awb.gains.manual.blue = 1.0;
>> +     activeState.awb.gains.manual.green = 1.0;
> I think you can drop the explicit green, it's always 1.0 and that's 
> well understood.


OK, will drop the explicit green.

>> +
>> +     activeState.awb.gains.automatic.red = 1.0;
>> +     activeState.awb.gains.automatic.blue = 1.0;
>> +     activeState.awb.gains.automatic.green = 1.0;
>> +     activeState.awb.autoEnabled = true;
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::queueRequest
>> + */
>> +void Awb::queueRequest(IPAContext &context,
>> +                    [[maybe_unused]] const uint32_t frame,
>> +                    IPAFrameContext &frameContext,
>> +                    const ControlList &controls)
>> +{
>> +     auto &awb = context.activeState.awb;
>> +
>> +     const auto &awbEnable = controls.get(controls::AwbEnable);
>> +     if (awbEnable && *awbEnable != awb.autoEnabled) {
>> +             awb.autoEnabled = *awbEnable;
>> +
>> +             LOG(C3ISPAwb, Debug)
>> +                     << (*awbEnable ? "Enabling" : "Disabling") << " 
>> AWB";
>> +     }
>> +
>> +     const auto &colourGains = controls.get(controls::ColourGains);
>> +     if (colourGains && !awb.autoEnabled) {
>> +             awb.gains.manual.red = (*colourGains)[0];
>> +             awb.gains.manual.blue = (*colourGains)[1];
>> +
>> +             LOG(C3ISPAwb, Debug)
>> +                     << "Set colour gains to red: " << 
>> awb.gains.manual.red
>> +                     << ", blue: " << awb.gains.manual.blue;
>> +     }
>> +
>> +     frameContext.awb.autoEnabled = awb.autoEnabled;
>> +
>> +     if (!awb.autoEnabled) {
>> +             frameContext.awb.gains.red = awb.gains.manual.red;
>> +             frameContext.awb.gains.green = 1.0;
>> +             frameContext.awb.gains.blue = awb.gains.manual.blue;
>> +     }
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::prepare
>> + */
>> +void Awb::prepare(IPAContext &context, const uint32_t frame,
>> +               IPAFrameContext &frameContext, C3ISPParams *params)
>> +{
>> +     /*
>> +      * This is the latest time we can read the active state. This 
>> is the
>> +      * most up-to-date automatic values we can read.
>> +      */
>> +     if (frameContext.awb.autoEnabled) {
>> +             frameContext.awb.gains.red = 
>> context.activeState.awb.gains.automatic.red;
>> +             frameContext.awb.gains.green = 
>> context.activeState.awb.gains.automatic.green;
>> +             frameContext.awb.gains.blue = 
>> context.activeState.awb.gains.automatic.blue;
>> +     }
>> +
>> +     auto AWBGains = params->block<BlockType::AWBGains>();
>> +     AWBGains.setEnabled(C3_ISP_PARAMS_BLOCK_FL_ENABLE);
>> +
>> +     AWBGains->gr_gain = std::clamp<int>(256 * 
>> frameContext.awb.gains.green, 0, 0xfff);
>> +     AWBGains->r_gain = std::clamp<int>(256 * 
>> frameContext.awb.gains.red, 0, 0xfff);
>> +     AWBGains->b_gain = std::clamp<int>(256 * 
>> frameContext.awb.gains.blue, 0, 0xfff);
>> +     AWBGains->gb_gain = std::clamp<int>(256 * 
>> frameContext.awb.gains.green, 0, 0xfff);
> You can just specify 1.0 instead of using 
> frameContext.awb.gains.green. We also have helpers to put
> things into Q4.8 format - it's the floatingToFixedPoint() function in 
> src/ipa/libipa/fixedpoint.cpp


OK, will use 1.0 and floatingToFixedPoint().

>> +
>> +     if (frame)
>> +             return;
>> +
>> +     auto AWBCfg = params->block<BlockType::AWBConfig>();
>> +     AWBCfg.setEnabled(C3_ISP_PARAMS_BLOCK_FL_ENABLE);
>> +
>> +     AWBCfg->tap_point = C3_ISP_AWB_STATS_TAP_BEFORE_WB;
>> +     AWBCfg->satur_vald = 1;
>> +     AWBCfg->horiz_zones_num = horizonalZonesNum_;
>> +     AWBCfg->vert_zones_num = verticalZonesNum_;
>> +     AWBCfg->rg_min = 75;
>> +     AWBCfg->rg_max = 256;
>> +     AWBCfg->bg_min = 44;
>> +     AWBCfg->bg_max = 222;
>> +     AWBCfg->rg_low = 93;
>> +     AWBCfg->rg_high = 244;
>> +     AWBCfg->bg_low = 61;
>> +     AWBCfg->bg_high = 205;
>> +
>> +     for (unsigned int i = 0; i < AWBCfg->horiz_zones_num * 
>> AWBCfg->vert_zones_num; i++)
>> +             AWBCfg->zone_weight[i] = 1;
> unsigned int numZones = horizonalZonesNum_ * verticalZonesNum_;
> Span<uint8_t> weights{ AWBCfg->zone_weight, numZones };
> std::fill(weights.begin(), weights.end(), 1);


Will use this method.

>> +
>> +     Size sensorSize = context.configuration.sensor.size;
>> +     uint8_t maxPointNum = std::max(AWBCfg->horiz_zones_num, 
>> AWBCfg->vert_zones_num) + 1;
>> +
>> +     for (unsigned int i = 0; i < maxPointNum; i++) {
>> +             uint16_t hidx = i * sensorSize.width / 
>> AWBCfg->horiz_zones_num;
>> +
>> +             /* Aligned with 2 */
>> +             hidx = hidx / 2 * 2;
>> +             AWBCfg->horiz_cood[i] = std::min(hidx, 
>> (uint16_t)sensorSize.width);
>> +
>> +             uint16_t vidx = i * sensorSize.height / 
>> AWBCfg->vert_zones_num;
>> +
>> +             /* Aligned with 2 */
>> +             vidx = vidx / 2 * 2;
>> +             AWBCfg->vert_cood[i] = std::min(vidx, 
>> (uint16_t)sensorSize.height);
>> +     }
>> +}
>> +
>> +uint32_t Awb::estimateCCT(double red, double green, double blue)
>> +{
>> +     /* Convert the RGB values to CIE tristimulus values (XYZ) */
>> +     double X = (-0.14282) * (red) + (1.54924) * (green) + 
>> (-0.95641) * (blue);
>> +     double Y = (-0.32466) * (red) + (1.57837) * (green) + 
>> (-0.73191) * (blue);
>> +     double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) 
>> * (blue);
>> +
>> +     /* Calculate the normalized chromaticity values */
>> +     double x = X / (X + Y + Z);
>> +     double y = Y / (X + Y + Z);
>> +
>> +     /* Calculate CCT */
>> +     double n = (x - 0.3320) / (0.1858 - y);
>> +     return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
>> +}
> We have a helper for this in src/ipa/libipa/colours.cpp


Will check this issue.

>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::process
>> + */
>> +void Awb::process([[maybe_unused]] IPAContext &context,
> context is used, so you can drop the qualifier


Will drop [[maybe_unused]]

>> +               [[maybe_unused]] const uint32_t frame,
>> +               IPAFrameContext &frameContext,
>> +               const c3_isp_stats_info *stats,
>> +               ControlList &metadata)
>> +{
>> +     IPAActiveState &activeState = context.activeState;
>> +     const struct c3_isp_awb_stats *awb = &stats->awb;
>> +     uint16_t zoneCnt = horizonalZonesNum_ * verticalZonesNum_;
>> +     uint32_t rgSum = 0;
>> +     uint32_t bgSum = 0;
>> +     double rgMean;
>> +     double bgMean;
>> +     double greenMean;
>> +     double blueMean;
>> +     double redMean;
> We'd typically declare variables close to their use, so rather than 
> double rgMean here...


Will use float.

>> +
>> +     for (unsigned int i = 0; i < zoneCnt; i++) {
>> +             rgSum += awb->stats[i].rg;
>> +             bgSum += awb->stats[i].bg;
>> +     }
>> +
>> +     rgMean = rgSum / zoneCnt / 4096.0;
> ...It would be "double rgMean = rgSum / zoneCnt / 4096.0;" here


OK

>> +     bgMean = bgSum / zoneCnt / 4096.0;
>> +
>> +     /*
>> +      * To simplify the calculation,
>> +      * the green mean is hardcoded to 1.0
>> +      */
>> +
>> +     greenMean = 1.0;
> Similarly here I think you can drop the green variables; the 
> hard-coded 1.0 is well understood


OK. will drop the green variables.

>> +     redMean = rgMean * greenMean;
>> +     blueMean = bgMean * greenMean;
> And this "* greenMean" is unecessary since that value is only ever 
> 1.0, which means you only need
> either rg/bgMean or red/blueMean, not both


Will drop the "greenMean"

>> +
>> +     activeState.awb.temperatureK = estimateCCT(redMean, greenMean, 
>> blueMean);
>> +
>> +     /* Metadata shall contain the up to date measurement */
>> +     metadata.set(controls::ColourTemperature, 
>> activeState.awb.temperatureK);
>> +
>> +     /*
>> +      * Estimate the red and blue gains to apply in a grey world.
>> +      * The green gain is hardcoded to 1.0. Avoid division by zero
>> +      * by clamping the divisor to mininum value of 0.0625.
>> +      */
>> +     double redGain = greenMean / std::max(redMean, 0.0625);
>> +     double blueGain = greenMean / std::max(blueMean, 0.0625);
>> +
>> +     /*
>> +      * Clamp the gain values to the hardware, which express gains 
>> as Q4.8
>> +      * unsigned integer values. Set the minimum just above zero to 
>> avoid
>> +      * divisions by zero.
>> +      */
>> +     redGain = std::clamp(redGain, 1.0 / 256, 4095.0 / 256);
>> +     blueGain = std::clamp(blueGain, 1.0 / 256, 4095.0 / 256);
>
> There's a helper to convert from floating to Q4.8 in libipa - use 
> those please
>

Will use the helper to convert.

>> +
>> +     /* Filter the values to avoid oscillations. */
>> +     double speed = 0.2;
>> +     redGain = speed * redGain + (1 - speed) * 
>> activeState.awb.gains.automatic.red;
>> +     blueGain = speed * blueGain + (1 - speed) * 
>> activeState.awb.gains.automatic.blue;
>> +
>> +     activeState.awb.gains.automatic.red = redGain;
>> +     activeState.awb.gains.automatic.blue = blueGain;
>> +     activeState.awb.gains.automatic.green = 1.0;
>> +
>> +     metadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);
>> +     metadata.set(controls::ColourGains, { 
>> static_cast<float>(frameContext.awb.gains.red),
>> + static_cast<float>(frameContext.awb.gains.blue) });
>> +
>> +     LOG(C3ISPAwb, Debug) << "Gains: R " << 
>> activeState.awb.gains.automatic.red
>> +                          << ", G " << 
>> activeState.awb.gains.automatic.green
>> +                          << ", B " << 
>> activeState.awb.gains.automatic.blue
>> +                          << ", Ct " << activeState.awb.temperatureK 
>> << "K";
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(Awb, "Awb")
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/awb.h 
>> b/src/ipa/c3-isp/algorithms/awb.h
>> new file mode 100755
>> index 00000000..63ed84b2
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/awb.h
>> @@ -0,0 +1,42 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP AWB control algorithm
>> + */
>> +
>> +#pragma once
>> +
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +class Awb : public Algorithm
>> +{
>> +public:
>> +     Awb();
>> +     ~Awb() = default;
>> +
>> +     int configure(IPAContext &context, const IPACameraSensorInfo 
>> &configInfo) override;
>> +     void queueRequest(IPAContext &context, const uint32_t frame,
>> +                       IPAFrameContext &frameContext,
>> +                       const ControlList &controls) override;
>> +     void prepare(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext,
>> +                  C3ISPParams *params) override;
>> +     void process(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext,
>> +                  const c3_isp_stats_info *stats,
>> +                  ControlList &metadata) override;
>> +
>> +private:
>> +     uint32_t estimateCCT(double red, double green, double blue);
>> +     uint8_t horizonalZonesNum_;
>> +     uint8_t verticalZonesNum_;
>> +};
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/blc.cpp 
>> b/src/ipa/c3-isp/algorithms/blc.cpp
>> new file mode 100644
>> index 00000000..781c8c2f
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/blc.cpp
>> @@ -0,0 +1,102 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP Black Level Correction control
>> + */
>> +
>> +#include "blc.h"
>> +
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/control_ids.h>
>> +
>> +#include "libcamera/internal/yaml_parser.h"
>> +
>> +/**
>> + * \file blc.h
>> + */
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +/**
>> + * \class Blc
>> + * \brief C3 ISP Black Level Correction control
>> + *
>> + * The pixels output by the camera normally include a black level, 
>> because
>> + * sensors do not always report a signal level of '0' for black. 
>> Pixels at or
>> + * below this level should be considered black. To achieve that, the 
>> C3 ISP BLC
>> + * algorithm subtracts a configurable offset from all pixels.
>> + *
>> + * The black level can be measured at runtime from an optical dark 
>> region of the
>> + * camera sensor, or measured during the camera tuning process. The 
>> first option
>> + * isn't currently supported.
>> + */
>> +
>> +LOG_DEFINE_CATEGORY(C3ISPBlc)
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::init
>> + */
>> +int Blc::init([[maybe_unused]] IPAContext &context, const YamlObject 
>> &tuningData)
>> +{
>> +     std::optional<int16_t> levelRed = 
>> tuningData["BlcR"].get<int16_t>();
>> +     std::optional<int16_t> levelGreenR = 
>> tuningData["BlcGr"].get<int16_t>();
>> +     std::optional<int16_t> levelGreenB = 
>> tuningData["BlcGb"].get<int16_t>();
>> +     std::optional<int16_t> levelBlue = 
>> tuningData["BlcB"].get<int16_t>();
>> +
>> +     blackLevelRed_ = levelRed.value_or(4096);
>> +     blackLevelGreenR_ = levelGreenR.value_or(4096);
>> +     blackLevelGreenB_ = levelGreenB.value_or(4096);
>> +     blackLevelBlue_ = levelBlue.value_or(4096);
>> +
>> +     LOG(C3ISPBlc, Debug)
>> +             << "Black Levels: red " << blackLevelRed_
>> +             << ", green (red) " << blackLevelGreenR_
>> +             << ", green (blue) " << blackLevelGreenB_
>> +             << ", blue " << blackLevelBlue_;
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::prepare
>> + */
>> +void Blc::prepare([[maybe_unused]] IPAContext &context, const 
>> uint32_t frame,
>> +               [[maybe_unused]] IPAFrameContext &frameContext,
>> +               C3ISPParams *params)
>> +{
>> +     if (frame)
>> +             return;
>> +
>> +     auto blcCfg = params->block<BlockType::Blc>();
>> +     blcCfg.setEnabled(C3_ISP_PARAMS_BLOCK_FL_ENABLE);
>> +
>> +     blcCfg->gr_ofst = blackLevelGreenR_;
>> +     blcCfg->r_ofst = blackLevelRed_;
>> +     blcCfg->b_ofst = blackLevelBlue_;
>> +     blcCfg->gb_ofst = blackLevelGreenB_;
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::process
>> + */
>> +void Blc::process([[maybe_unused]] IPAContext &context,
>> +               [[maybe_unused]] const uint32_t frame,
>> +               [[maybe_unused]] IPAFrameContext &frameContext,
>> +               [[maybe_unused]] const c3_isp_stats_info *stats,
>> +               ControlList &metadata)
>> +{
>> +     metadata.set(controls::SensorBlackLevels,
>> +                  { static_cast<int32_t>(blackLevelRed_),
>> + static_cast<int32_t>(blackLevelGreenR_),
>> + static_cast<int32_t>(blackLevelGreenB_),
>> +                    static_cast<int32_t>(blackLevelBlue_) });
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(Blc, "Blc")
>
>
> This is all fine, though one addition you might consider is to check 
> if the CameraSensorHelper
> reports any black level values for the sensor, and use those if 
> there's none in the tuning data.
> Check the blc.cpp in mali-c55 IPA for an example.
>

Will refer to the blc.cpp in mali-c55 IPA

>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/blc.h 
>> b/src/ipa/c3-isp/algorithms/blc.h
>> new file mode 100644
>> index 00000000..702163f7
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/blc.h
>> @@ -0,0 +1,40 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP Black Level Correction control
>> + */
>> +
>> +#pragma once
>> +
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +class Blc : public Algorithm
>> +{
>> +public:
>> +     Blc() {}
>> +     ~Blc() = default;
>> +
>> +     int init(IPAContext &context, const YamlObject &tuningData) 
>> override;
>> +     void prepare(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext,
>> +                  C3ISPParams *params) override;
>> +     void process(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext,
>> +                  const c3_isp_stats_info *stats,
>> +                  ControlList &metadata) override;
>> +
>> +private:
>> +     int16_t blackLevelRed_;
>> +     int16_t blackLevelGreenR_;
>> +     int16_t blackLevelGreenB_;
>> +     int16_t blackLevelBlue_;
>> +};
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/ccm.cpp 
>> b/src/ipa/c3-isp/algorithms/ccm.cpp
>> new file mode 100644
>> index 00000000..43f146d6
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/ccm.cpp
>> @@ -0,0 +1,86 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP Color Correction Matrix control
>> + */
>> +
>> +#include "ccm.h"
>> +
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/base/utils.h>
>> +
>> +#include <libcamera/control_ids.h>
>> +#include <libcamera/ipa/core_ipa_interface.h>
>> +
>> +#include "libcamera/internal/yaml_parser.h"
>> +#include "libipa/interpolator.h"
>> +
>> +/**
>> + * \file ccm.h
>> + */
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +/**
>> + * \class Ccm
>> + * \brief A color correction matrix algorithm
>> + */
>> +
>> +LOG_DEFINE_CATEGORY(C3ISPCcm)
>> +
>> +Ccm::Ccm()
>> +{
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::init
>> + */
>> +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject 
>> &tuningData)
>> +{
>> +     ccmCoeff = 
>> tuningData["CcmCoeff"].getList<int>().value_or(std::vector<int>{});
>
>
> This needs some validation; you're relying on it being an array of 9 
> values below, so that needs to
> be verified.


Will verify.

>
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::prepare
>> + */
>> +void Ccm::prepare([[maybe_unused]] IPAContext &context,
>> +               [[maybe_unused]] const uint32_t frame,
>> +               [[maybe_unused]] IPAFrameContext &frameContext, 
>> C3ISPParams *params)
>> +{
>> +     auto CcmCfg = params->block<BlockType::Ccm>();
>> +     CcmCfg.setEnabled(C3_ISP_PARAMS_BLOCK_FL_ENABLE);
>> +
>> +     for (unsigned int i = 0; i < 3; i++) {
>> +             for (unsigned int j = 0; j < 3; j++) {
>> +                     CcmCfg->matrix[i][j] = ccmCoeff[i * 3 + j];
>> +             }
>> +     }
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::process
>> + */
>> +void Ccm::process([[maybe_unused]] IPAContext &context,
>> +               [[maybe_unused]] const uint32_t frame,
>> +               [[maybe_unused]] IPAFrameContext &frameContext,
>> +               [[maybe_unused]] const c3_isp_stats_info *stats,
>> +               ControlList &metadata)
>> +{
>> +     float m[9];
>> +     for (unsigned int i = 0; i < 3; i++) {
>> +             for (unsigned int j = 0; j < 3; j++)
>> +                     m[i * 3 + j] = ccmCoeff[i * 3 + j];
>> +     }
>> +     metadata.set(controls::ColourCorrectionMatrix, m);
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/ccm.h 
>> b/src/ipa/c3-isp/algorithms/ccm.h
>> new file mode 100644
>> index 00000000..9d8115d4
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/ccm.h
>> @@ -0,0 +1,40 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP Color Correction Matrix control
>> + */
>> +
>> +#pragma once
>> +
>> +#include <linux/c3-isp-config.h>
>> +
>> +#include <libipa/interpolator.h>
>> +
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +class Ccm : public Algorithm
>> +{
>> +public:
>> +     Ccm();
>> +     ~Ccm() = default;
>> +
>> +     int init(IPAContext &context, const YamlObject &tuningData) 
>> override;
>> +     void prepare(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext,
>> +                  C3ISPParams *params) override;
>> +     void process(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext, const 
>> c3_isp_stats_info *stats,
>> +                  ControlList &metadata) override;
>> +
>> +private:
>> +     std::vector<int> ccmCoeff;
>> +};
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/csc.cpp 
>> b/src/ipa/c3-isp/algorithms/csc.cpp
>> new file mode 100644
>> index 00000000..8e066f57
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/csc.cpp
>> @@ -0,0 +1,64 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP Color Space Conversion control
>> + */
>> +
>> +#include "csc.h"
>> +
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/control_ids.h>
>> +
>> +/**
>> + * \file csc.h
>> + */
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +/**
>> + * \class Csc
>> + * \brief Color Space Conversion algorithm
>> + */
>> +
>> +LOG_DEFINE_CATEGORY(C3ISPCsc)
>> +
>> +Csc::Csc()
>> +{
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::init
>> + */
>> +int Csc::init([[maybe_unused]] IPAContext &context,
>> +           [[maybe_unused]] const YamlObject &tuningData)
>> +{
>> +     cscCoeff = 
>> tuningData["CscCoeff"].getList<int>().value_or(std::vector<int>{});
> Likewise, validation please.


Will verify.

>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::prepare
>> + */
>> +void Csc::prepare([[maybe_unused]] IPAContext &context,
>> +               [[maybe_unused]] const uint32_t frame,
>> +               [[maybe_unused]] IPAFrameContext &frameContext,
>> +               C3ISPParams *params)
>> +{
>> +     auto CscCfg = params->block<BlockType::Csc>();
>> +     CscCfg.setEnabled(C3_ISP_PARAMS_BLOCK_FL_ENABLE);
>> +
>> +     for (unsigned int i = 0; i < 3; i++) {
>> +             for (unsigned int j = 0; j < 3; j++)
>> +                     CscCfg->matrix[i][j] = cscCoeff[i * 3 + j];
>> +     }
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(Csc, "Csc")
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/csc.h 
>> b/src/ipa/c3-isp/algorithms/csc.h
>> new file mode 100644
>> index 00000000..13b89dd0
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/csc.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP Color Space Conversion control
>> + */
>> +
>> +#pragma once
>> +
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +class Csc : public Algorithm
>> +{
>> +public:
>> +     Csc();
>> +     ~Csc() = default;
>> +
>> +     int init(IPAContext &context, const YamlObject &tuningData) 
>> override;
>> +     void prepare(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext,
>> +                  C3ISPParams *params) override;
>> +private:
>> +     std::vector<int> cscCoeff;
>> +};
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/meson.build 
>> b/src/ipa/c3-isp/algorithms/meson.build
>> new file mode 100644
>> index 00000000..5e7e76dd
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/meson.build
>> @@ -0,0 +1,10 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +c3isp_ipa_algorithms = files([
>> +    'agc.cpp',
>> +    'awb.cpp',
>> +    'blc.cpp',
>> +    'ccm.cpp',
>> +    'csc.cpp',
>> +    'post_gamma.cpp'
>> +])
>> diff --git a/src/ipa/c3-isp/algorithms/post_gamma.cpp 
>> b/src/ipa/c3-isp/algorithms/post_gamma.cpp
>> new file mode 100644
>> index 00000000..cd62a604
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/post_gamma.cpp
>> @@ -0,0 +1,64 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP Post Gamma control
>> + */
>> +
>> +#include "post_gamma.h"
>> +
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/control_ids.h>
>> +
>> +/**
>> + * \file post_gamma.h
>> + */
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +/**
>> + * \class PostGamma
>> + * \brief A post gamma algorithm
>> + */
>> +
>> +LOG_DEFINE_CATEGORY(C3ISPPostGamma)
>> +
>> +PostGamma::PostGamma()
>> +{
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::init
>> + */
>> +int PostGamma::init([[maybe_unused]] IPAContext &context,
>> +                 [[maybe_unused]] const YamlObject &tuningData)
>> +{
>> +     gammaLut =
>> + 
>> tuningData["GammaLut"].getList<uint16_t>().value_or(std::vector<uint16_t>{});
> And here, needs validation.


Will verify.

>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * \copydoc libcamera::ipa::Algorithm::prepare
>> + */
>> +void PostGamma::prepare([[maybe_unused]] IPAContext &context,
>> +                     [[maybe_unused]] const uint32_t frame,
>> +                     [[maybe_unused]] IPAFrameContext &frameContext,
>> +                     C3ISPParams *params)
>> +{
>> +     auto PostGammaCfg = params->block<BlockType::PostGamma>();
>> +     PostGammaCfg.setEnabled(C3_ISP_PARAMS_BLOCK_FL_ENABLE);
>> +
>> +     for (unsigned int i = 0; i < 129; i++) {
>> +             PostGammaCfg->lut[i] = gammaLut[i];
>> +     }
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(PostGamma, "PostGamma")
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/algorithms/post_gamma.h 
>> b/src/ipa/c3-isp/algorithms/post_gamma.h
>> new file mode 100644
>> index 00000000..0bfa134b
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/algorithms/post_gamma.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP Post Gamma control
>> + */
>> +
>> +#pragma once
>> +
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp::algorithms {
>> +
>> +class PostGamma : public Algorithm
>> +{
>> +public:
>> +     PostGamma();
>> +     ~PostGamma() = default;
>> +
>> +     int init(IPAContext &context, const YamlObject &tuningData) 
>> override;
>> +     void prepare(IPAContext &context, const uint32_t frame,
>> +                  IPAFrameContext &frameContext,
>> +                  C3ISPParams *params) override;
>> +
>> +private:
>> +     std::vector<uint16_t> gammaLut;
>> +};
>> +
>> +} /* namespace ipa::c3isp::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/c3-isp.cpp b/src/ipa/c3-isp/c3-isp.cpp
>> new file mode 100644
>> index 00000000..9331576d
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/c3-isp.cpp
>> @@ -0,0 +1,386 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic Inc.
>> + *
>> + * c3-isp.cpp - Amlogic Image Processing Algorithms
>> + */
>> +
>> +#include <algorithm>
>> +#include <array>
>> +#include <chrono>
>> +#include <stdint.h>
>> +#include <string.h>
>> +
>> +#include <linux/c3-isp-config.h>
>> +#include <linux/v4l2-controls.h>
>> +
>> +#include <libcamera/base/file.h>
>> +#include <libcamera/base/log.h>
>> +
>> +#include <libcamera/control_ids.h>
>> +#include <libcamera/controls.h>
>> +#include <libcamera/framebuffer.h>
>> +#include <libcamera/request.h>
>> +
>> +#include <libcamera/ipa/c3isp_ipa_interface.h>
>> +#include <libcamera/ipa/ipa_interface.h>
>> +#include <libcamera/ipa/ipa_module_info.h>
>> +
>> +#include "libcamera/internal/formats.h"
>> +#include "libcamera/internal/mapped_framebuffer.h"
>> +#include "libcamera/internal/yaml_parser.h"
>> +
>> +#include "algorithms/algorithm.h"
>> +
>> +#include "ipa_context.h"
>> +#include "params.h"
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(IPAC3ISP)
>> +
>> +using namespace std::literals::chrono_literals;
>> +
>> +namespace ipa::c3isp {
>> +
>> +static constexpr uint32_t kMaxFrameContexts = 16;
>> +
>> +class IPAC3ISP : public IPAC3ISPInterface, public Module
>> +{
>> +public:
>> +     IPAC3ISP();
>> +
>> +     int init(const IPASettings &settings, unsigned int hwRevision,
>> +              const IPACameraSensorInfo &sensorInfo,
>> +              const ControlInfoMap &sensorControls,
>> +              ControlInfoMap *ipaControls) override;
>> +     int start() override;
>> +     void stop() override;
>> +
>> +     int configure(const IPAConfigInfo &ipaConfig,
>> +                   ControlInfoMap *ipaControls) override;
>> +     void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>> +     void unmapBuffers(const std::vector<unsigned int> &ids) override;
>> +
>> +     void queueRequest(const uint32_t frame, const ControlList 
>> &controls) override;
>> +     void fillParamsBuffer(const uint32_t frame, const uint32_t 
>> bufferId) override;
>> +     void processStatsBuffer(const uint32_t frame, const uint32_t 
>> bufferId,
>> +                             const ControlList &sensorControls) 
>> override;
>> +
>> +protected:
>> +     std::string logPrefix() const override;
>> +
>> +private:
>> +     void updateControls(const IPACameraSensorInfo &sensorInfo,
>> +                         const ControlInfoMap &sensorControls,
>> +                         ControlInfoMap *ipaControls);
>> +     void setControls(unsigned int frame);
>> +
>> +     std::map<unsigned int, FrameBuffer> buffers_;
>> +     std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>> +
>> +     ControlInfoMap sensorControls_;
>> +
>> +     struct IPAContext context_;
>> +};
>> +
>> +namespace {
>> +
>> +const ControlInfoMap::Map c3ispControls{
>> +     { &controls::AwbEnable, ControlInfo(false, true) },
>> +     { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
>> +};
>
>
> It would be better to have these within the Awb algorithm and reported 
> out through a ControlInfoMap
> in the IPA context.
>

OK, will use your method.

>> +
>> +} /* namespace */
>> +
>> +IPAC3ISP::IPAC3ISP()
>> +     : context_({ {}, {}, { kMaxFrameContexts }, {}, {} })
>> +{
>> +}
>> +
>> +std::string IPAC3ISP::logPrefix() const
>> +{
>> +     return "c3isp";
>> +}
>> +
>> +int IPAC3ISP::init(const IPASettings &settings, unsigned int 
>> hwRevision,
>> +                const IPACameraSensorInfo &sensorInfo,
>> +                const ControlInfoMap &sensorControls,
>> +                ControlInfoMap *ipaControls)
>> +{
>> +     LOG(IPAC3ISP, Debug) << "Hardware revision is " << hwRevision;
> This isn't used apart from this debug print. Is it necessary to pass 
> it through to the IPA?


Will drop this parameter.

>> +
>> +     context_.camHelper = 
>> CameraSensorHelperFactoryBase::create(settings.sensorModel);
>> +     if (!context_.camHelper) {
>> +             LOG(IPAC3ISP, Error)
>> +                     << "Failed to create camera sensor helper for "
>> +                     << settings.sensorModel;
>> +             return -ENODEV;
>> +     }
>> +
>> +     context_.configuration.sensor.lineDuration =
>> +             sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;
>> +
>> +     File file(settings.configurationFile);
>> +     if (!file.open(File::OpenModeFlag::ReadOnly)) {
>> +             int ret = file.error();
>> +             LOG(IPAC3ISP, Error)
>> +                     << "Failed to open configuration file "
>> +                     << settings.configurationFile << ": " << 
>> strerror(-ret);
>> +             return ret;
>> +     }
>> +
>> +     std::unique_ptr<libcamera::YamlObject> data = 
>> YamlParser::parse(file);
>> +     if (!data)
>> +             return -EINVAL;
>> +
>> +     unsigned int version = (*data)["version"].get<uint32_t>(0);
>> +     if (version != 1) {
>> +             LOG(IPAC3ISP, Error)
>> +                     << "Invalid tuning file version " << version;
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (!data->contains("algorithms")) {
>> +             LOG(IPAC3ISP, Error)
>> +                     << "Tuning file doesn't contain any algorithm";
>> +             return -EINVAL;
>> +     }
>> +
>> +     int ret = createAlgorithms(context_, (*data)["algorithms"]);
>> +     if (ret)
>> +             return ret;
>> +
>> +     updateControls(sensorInfo, sensorControls, ipaControls);
>> +
>> +     return 0;
>> +}
>> +
>> +int IPAC3ISP::start()
>> +{
>> +     setControls(0);
>> +
>> +     return 0;
>> +}
>> +
>> +void IPAC3ISP::stop()
>> +{
>> +     context_.frameContexts.clear();
>> +}
>> +
>> +int IPAC3ISP::configure(const IPAConfigInfo &ipaConfig,
>> +                     ControlInfoMap *ipaControls)
>> +{
>> +     sensorControls_ = ipaConfig.sensorControls;
>> +
>> +     const auto itExp = sensorControls_.find(V4L2_CID_EXPOSURE);
>> +     int32_t minExposure = itExp->second.min().get<int32_t>();
>> +     int32_t maxExposure = itExp->second.max().get<int32_t>();
>> +
>> +     const auto itGain = sensorControls_.find(V4L2_CID_ANALOGUE_GAIN);
>> +     int32_t minGain = itGain->second.min().get<int32_t>();
>> +     int32_t maxGain = itGain->second.max().get<int32_t>();
>> +
>> +     LOG(IPAC3ISP, Debug)
>> +             << "Exposure: [" << minExposure << ", " << maxExposure
>> +             << "], gain: [" << minGain << ", " << maxGain << "]";
>> +
>> +     context_.configuration = {};
>> +     context_.activeState = {};
>> +     context_.frameContexts.clear();
>> +
>> +     const IPACameraSensorInfo &info = ipaConfig.sensorInfo;
>> +
>> +     const ControlInfo vBlank = 
>> sensorControls_.find(V4L2_CID_VBLANK)->second;
>> +     context_.configuration.sensor.defVBlank = 
>> vBlank.def().get<int32_t>();
>> +     context_.configuration.sensor.size = info.outputSize;
>> +     context_.configuration.sensor.lineDuration = info.minLineLength 
>> * 1.0s / info.pixelRate;
>> +
>> +     updateControls(info, sensorControls_, ipaControls);
>> +
>> +     context_.configuration.sensor.minShutterSpeed =
>> +             minExposure * context_.configuration.sensor.lineDuration;
>> +     context_.configuration.sensor.maxShutterSpeed =
>> +             maxExposure * context_.configuration.sensor.lineDuration;
>> +     context_.configuration.sensor.minAnalogueGain =
>> +             context_.camHelper->gain(minGain);
>> +     context_.configuration.sensor.maxAnalogueGain =
>> +             context_.camHelper->gain(maxGain);
>> +
>> +     for (auto const &a : algorithms()) {
>> +             Algorithm *algo = static_cast<Algorithm *>(a.get());
>> +
>> +             if (algo->disabled_)
>> +                     continue;
>> +
>> +             int ret = algo->configure(context_, info);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +void IPAC3ISP::mapBuffers(const std::vector<IPABuffer> &buffers)
>> +{
>> +     for (const IPABuffer &buffer : buffers) {
>> +             auto elem = buffers_.emplace(std::piecewise_construct,
>> + std::forward_as_tuple(buffer.id),
>> + std::forward_as_tuple(buffer.planes));
>> +             const FrameBuffer &fb = elem.first->second;
>> +
>> +             MappedFrameBuffer mappedBuffer(&fb, 
>> MappedFrameBuffer::MapFlag::ReadWrite);
>> +             if (!mappedBuffer.isValid()) {
>> +                     LOG(IPAC3ISP, Fatal) << "Failed tommap buffer: "
>> +                                          << 
>> strerror(mappedBuffer.error());
>> +             }
>> +
>> +             mappedBuffers_.emplace(buffer.id, 
>> std::move(mappedBuffer));
>> +     }
>> +}
>> +
>> +void IPAC3ISP::unmapBuffers(const std::vector<unsigned int> &ids)
>> +{
>> +     for (unsigned int id : ids) {
>> +             const auto fb = buffers_.find(id);
>> +             if (fb == buffers_.end())
>> +                     continue;
>> +
>> +             mappedBuffers_.erase(id);
>> +             buffers_.erase(id);
>> +     }
>> +}
>> +
>> +void IPAC3ISP::queueRequest(const uint32_t frame, const ControlList 
>> &controls)
>> +{
>> +     IPAFrameContext &frameContext = 
>> context_.frameContexts.alloc(frame);
>> +
>> +     for (auto const &a : algorithms()) {
>> +             Algorithm *algo = static_cast<Algorithm *>(a.get());
>> +             if (algo->disabled_)
>> +                     continue;
>> +             algo->queueRequest(context_, frame, frameContext, 
>> controls);
>> +     }
>> +}
>> +
>> +void IPAC3ISP::fillParamsBuffer(const uint32_t frame, const uint32_t 
>> bufferId)
>> +{
>> +     IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>> +
>> +     C3ISPParams params(mappedBuffers_.at(bufferId).planes()[0]);
>> +
>> +     for (auto const &algo : algorithms())
>> +             algo->prepare(context_, frame, frameContext, &params);
>> +
>> +     paramsBufferReady.emit(frame, params.size());
>> +}
>> +
>> +void IPAC3ISP::processStatsBuffer(const uint32_t frame, const 
>> uint32_t bufferId,
>> +                               const ControlList &sensorControls)
>> +{
>> +     IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>> +
>> +     const c3_isp_stats_info *stats = nullptr;
>> +
>> +     stats = reinterpret_cast<c3_isp_stats_info *>(
>> +             mappedBuffers_.at(bufferId).planes()[0].data());
>> +
>> +     frameContext.sensor.exposure =
>> + sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> +     frameContext.sensor.gain =
>> + 
>> context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> +
>> +     ControlList metadata(controls::controls);
>> +
>> +     for (auto const &a : algorithms()) {
>> +             Algorithm *algo = static_cast<Algorithm *>(a.get());
>> +             if (algo->disabled_)
>> +                     continue;
>> +             algo->process(context_, frame, frameContext, stats, 
>> metadata);
>> +     }
>> +
>> +     setControls(frame);
>> +
>> +     metadataReady.emit(frame, metadata);
>> +}
>> +
>> +void IPAC3ISP::updateControls(const IPACameraSensorInfo &sensorInfo,
>> +                           const ControlInfoMap &sensorControls,
>> +                           ControlInfoMap *ipaControls)
>> +{
>> +     ControlInfoMap::Map ctrlMap = c3ispControls;
>> +
>> +     double lineDuration = 
>> context_.configuration.sensor.lineDuration.get<std::micro>();
>> +     const ControlInfo &v4l2Exposure = 
>> sensorControls.find(V4L2_CID_EXPOSURE)->second;
>> +     int32_t minExposure = v4l2Exposure.min().get<int32_t>() * 
>> lineDuration;
>> +     int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * 
>> lineDuration;
>> +     int32_t defExposure = v4l2Exposure.def().get<int32_t>() * 
>> lineDuration;
>> +     ctrlMap.emplace(std::piecewise_construct,
>> + std::forward_as_tuple(&controls::ExposureTime),
>> +                     std::forward_as_tuple(minExposure, maxExposure, 
>> defExposure));
>> +
>> +     const ControlInfo &v4l2Gain = 
>> sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
>> +     float minGain = 
>> context_.camHelper->gain(v4l2Gain.min().get<int32_t>());
>> +     float maxGain = 
>> context_.camHelper->gain(v4l2Gain.max().get<int32_t>());
>> +     float defGain = 
>> context_.camHelper->gain(v4l2Gain.def().get<int32_t>());
>> +     ctrlMap.emplace(std::piecewise_construct,
>> + std::forward_as_tuple(&controls::AnalogueGain),
>> +                     std::forward_as_tuple(minGain, maxGain, defGain));
>> +
>> +     const ControlInfo &v4l2HBlank = 
>> sensorControls.find(V4L2_CID_HBLANK)->second;
>> +     uint32_t hblank = v4l2HBlank.def().get<int32_t>();
>> +     uint32_t lineLength = sensorInfo.outputSize.width + hblank;
>> +
>> +     const ControlInfo &v4l2VBlank = 
>> sensorControls.find(V4L2_CID_VBLANK)->second;
>> +     std::array<uint32_t, 3> frameHeights{
>> +             v4l2VBlank.min().get<int32_t>() + 
>> sensorInfo.outputSize.height,
>> +             v4l2VBlank.max().get<int32_t>() + 
>> sensorInfo.outputSize.height,
>> +             v4l2VBlank.def().get<int32_t>() + 
>> sensorInfo.outputSize.height,
>> +     };
>> +
>> +     std::array<int64_t, 3> frameDurations;
>> +     for (unsigned int i = 0; i < frameHeights.size(); ++i) {
>> +             uint64_t frameSize = lineLength * frameHeights[i];
>> +             frameDurations[i] = frameSize / (sensorInfo.pixelRate / 
>> 1000000U);
>> +     }
>> +
>> +     ctrlMap[&controls::FrameDurationLimits] = 
>> ControlInfo(frameDurations[0],
>> + frameDurations[1],
>> + frameDurations[2]);
>> +     ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());
>> +     *ipaControls = ControlInfoMap(std::move(ctrlMap), 
>> controls::controls);
>> +}
>> +
>> +void IPAC3ISP::setControls(unsigned int frame)
>> +{
>> +     uint32_t exposure = context_.activeState.agc.exposure;
>> +     uint32_t gain = 
>> context_.camHelper->gainCode(context_.activeState.agc.gain);
>> +
>> +     ControlList ctrls(sensorControls_);
>> +     ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>> +     ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
>> +
>> +     setSensorControls.emit(frame, ctrls);
>> +}
>> +
>> +} /* namespace ipa::c3isp */
>> +
>> +/*
>> + * External IPA module interface
>> + */
>> +
>> +extern "C" {
>> +const struct IPAModuleInfo ipaModuleInfo = {
>> +     IPA_MODULE_API_VERSION,
>> +     1,
>> +     "c3isp",
>> +     "c3isp",
>> +};
>> +
>> +IPAInterface *ipaCreate()
>> +{
>> +     return new ipa::c3isp::IPAC3ISP();
>> +}
>> +}
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/data/imx290.yaml 
>> b/src/ipa/c3-isp/data/imx290.yaml
>> new file mode 100644
>> index 00000000..498a3684
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/data/imx290.yaml
>> @@ -0,0 +1,30 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +%YAML 1.1
>> +---
>> +version: 1
>> +algorithms:
>> +  - Agc:
>> +  - Awb:
>> +  - PostGamma:
>> +      GammaLut: [
>> +                   0,   86,  134,  169,  198,  223,  245, 265,  283, 
>> 300, 316, 331,
>> +                   346, 359, 373,  385,  397,  409,  420, 431,  441, 
>> 452, 462, 471,
>> +                   481, 490, 499,  508,  516,  525,  533, 541,  549, 
>> 557, 565, 572,
>> +                   580, 587, 594,  601,  608,  615,  622, 629,  635, 
>> 642, 648, 655,
>> +                   661, 667, 673,  679,  685,  691,  697, 703,  708, 
>> 714, 720, 725,
>> +                   731, 736, 742,  747,  752,  758,  763, 768,  773, 
>> 778, 783, 788,
>> +                   793, 798, 803,  808,  812,  817,  822, 827,  831, 
>> 836, 840, 845,
>> +                   849, 854, 858,  863,  867,  872,  876, 880,  884, 
>> 889, 893, 897,
>> +                   901, 905, 910,  914,  918,  922,  926, 930,  934, 
>> 938, 942, 946,
>> +                   950, 953, 957,  961,  965,  969,  972, 976,  980, 
>> 984, 987, 991,
>> +                   995, 998, 1002, 1006, 1009, 1013, 1016, 1020, 1023
>> +                ]
>> +  - Ccm:
>> +      CcmCoeff: [ 533, -191, -86, -147, 474, -71, 23, -208, 441 ]
>> +  - Csc:
>> +      CscCoeff: [ 54, 183, 19, -29, -99, 128, 128, -116, -12 ]
>> +  - Blc:
>> +      BlcR: 20089
>> +      BlcGr: 20090
>> +      BlcGb: 20081
>> +      BlcB: 20084
> Separate commit please


Will separate commit.

>> diff --git a/src/ipa/c3-isp/data/meson.build 
>> b/src/ipa/c3-isp/data/meson.build
>> new file mode 100644
>> index 00000000..e9ecfc2c
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/data/meson.build
>> @@ -0,0 +1,9 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +conf_files = files([
>> +    'imx290.yaml',
>> +])
>> +
>> +install_data(conf_files,
>> +             install_dir : ipa_data_dir / 'c3isp',
>> +             install_tag : 'runtime')
>> diff --git a/src/ipa/c3-isp/ipa_context.cpp 
>> b/src/ipa/c3-isp/ipa_context.cpp
>> new file mode 100644
>> index 00000000..3d087460
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/ipa_context.cpp
>> @@ -0,0 +1,251 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic Inc.
>> + *
>> + * C3ISP IPA Context
>> + */
>> +
>> +#include "ipa_context.h"
>> +
>> +/**
>> + * \file ipa_context.h
>> + * \brief Context and state information shared between the algorithms
>> + */
>> +
>> +namespace libcamera::ipa::c3isp {
>> +
>> +/**
>> + * \struct IPASessionConfiguration
>> + * \brief Session configuration for the IPA module
>> + *
>> + * The session configuration contains all IPA configuration 
>> parameters that
>> + * remain constant during the capture session, from IPA module start 
>> to stop.
>> + * It is typically set during the configure() operation of the IPA 
>> module, but
>> + * may also be updated in the start() operation.
>> + */
>> +
>> +/**
>> + * \var IPASessionConfiguration::sensor
>> + * \brief Sensor-specific configuration of the IPA
>> + *
>> + * \var IPASessionConfiguration::sensor.minShutterSpeed
>> + * \brief Minimum shutter speed supported with the sensor
>> + *
>> + * \var IPASessionConfiguration::sensor.maxShutterSpeed
>> + * \brief Maximum shutter speed supported with the sensor
>> + *
>> + * \var IPASessionConfiguration::sensor.minAnalogueGain
>> + * \brief Minimum analogue gain supported with the sensor
>> + *
>> + * \var IPASessionConfiguration::sensor.maxAnalogueGain
>> + * \brief Maximum analogue gain supported with the sensor
>> + *
>> + * \var IPASessionConfiguration::sensor.defVBlank
>> + * \brief The default vblank value of the sensor
>> + *
>> + * \var IPASessionConfiguration::sensor.lineDuration
>> + * \brief Line duration in microseconds
>> + *
>> + * \var IPASessionConfiguration::sensor.size
>> + * \brief Sensor output resolution
>> + */
>> +
>> +/**
>> + * \struct IPAActiveState
>> + * \brief Active state for algorithms
>> + *
>> + * The active state contains all algorithm-specific data that needs 
>> to be
>> + * maintained by algorithms across frames. Unlike the session 
>> configuration,
>> + * the active state is mutable and constantly updated by algorithms. 
>> The active
>> + * state is accessible through the IPAContext structure.
>> + *
>> + * The active state stores two distinct categories of information:
>> + *
>> + *  - The consolidated value of all algorithm controls. Requests 
>> passed to
>> + *    the queueRequest() function store values for controls that the
>> + *    application wants to modify for that particular frame, and the
>> + *    queueRequest() function updates the active state with those 
>> values.
>> + *    The active state thus contains a consolidated view of the 
>> value of all
>> + *    controls handled by the algorithm.
>> + *
>> + *  - The value of parameters computed by the algorithm when running 
>> in auto
>> + *    mode. Algorithms running in auto mode compute new parameters 
>> every
>> + *    time statistics buffers are received (either synchronously, or
>> + *    possibly in a background thread). The latest computed value of 
>> those
>> + *    parameters is stored in the active state in the process() 
>> function.
>> + *
>> + * Each of the members in the active state belongs to a specific 
>> algorithm. A
>> + * member may be read by any algorithm, but shall only be written by 
>> its owner.
>> + */
>> +
>> +/**
>> + * \var IPAActiveState::agc
>> + * \brief State for the Automatic Gain Control algorithm
>> + *
>> + * The \a automatic variables track the latest values computed by 
>> algorithm
>> + * based on the latest processed statistics. All other variables 
>> track the
>> + * consolidated controls requested in queued requests.
>> + *
>> + * \var IPAActiveState::agc.exposure
>> + * \brief Automatic exposure time expressed as a number of lines
>> + *
>> + * \var IPAActiveState::agc.gain
>> + * \brief Automatic analogue gain multiplier
>> + *
>> + * \var IPAActiveState::agc.constraintMode
>> + * \brief Constraint mode as set by the AeConstraintMode control
>> + *
>> + * \var IPAActiveState::agc.exposureMode
>> + * \brief Exposure mode as set by the AeExposureMode control
>> + */
>> +
>> +/**
>> + * \var IPAActiveState::awb
>> + * \brief State for the Automatic White Balance algorithm
>> + *
>> + * \struct IPAActiveState::awb.gains
>> + * \brief White balance gains
>> + *
>> + * \struct IPAActiveState::awb.gains.manual
>> + * \brief Manual white balance gains (set through requests)
>> + *
>> + * \var IPAActiveState::awb.gains.manual.red
>> + * \brief Manual white balance gain for R channel
>> + *
>> + * \var IPAActiveState::awb.gains.manual.green
>> + * \brief Manual white balance gain for G channel
>> + *
>> + * \var IPAActiveState::awb.gains.manual.blue
>> + * \brief Manual white balance gain for B channel
>> + *
>> + * \struct IPAActiveState::awb.gains.automatic
>> + * \brief Automatic white balance gains (computed by the algorithm)
>> + *
>> + * \var IPAActiveState::awb.gains.automatic.red
>> + * \brief Automatic white balance gain for R channel
>> + *
>> + * \var IPAActiveState::awb.gains.automatic.green
>> + * \brief Automatic white balance gain for G channel
>> + *
>> + * \var IPAActiveState::awb.gains.automatic.blue
>> + * \brief Automatic white balance gain for B channel
>> + *
>> + * \var IPAActiveState::awb.temperatureK
>> + * \brief Estimated color temperature
>> + *
>> + * \var IPAActiveState::awb.autoEnabled
>> + * \brief Whether the Auto White Balance algorithm is enabled
>> + */
>> +
>> +/**
>> + * \struct IPAFrameContext
>> + * \brief Per-frame context for algorithms
>> + *
>> + * The frame context stores two distinct categories of information:
>> + *
>> + * - The value of the controls to be applied to the frame. These 
>> values are
>> + *   typically set in the queueRequest() function, from the 
>> consolidated
>> + *   control values stored in the active state. The frame context 
>> thus stores
>> + *   values for all controls related to the algorithm, not limited 
>> to the
>> + *   controls specified in the corresponding request, but 
>> consolidated from all
>> + *   requests that have been queued so far.
>> + *
>> + *   For controls that can be set manually or computed by an algorithm
>> + *   (depending on the algorithm operation mode), such as for 
>> instance the
>> + *   colour gains for the AWB algorithm, the control value will be 
>> stored in
>> + *   the frame context in the queueRequest() function only when 
>> operating in
>> + *   manual mode. When operating in auto mode, the values are 
>> computed by the
>> + *   algorithm in process(), stored in the active state, and copied 
>> to the
>> + *   frame context in prepare(), just before being stored in the ISP 
>> parameters
>> + *   buffer.
>> + *
>> + *   The queueRequest() function can also store ancillary data in 
>> the frame
>> + *   context, such as flags to indicate if (and what) control values 
>> have
>> + *   changed compared to the previous request.
>> + *
>> + * - Status information computed by the algorithm for a frame. For 
>> instance,
>> + *   the colour temperature estimated by the AWB algorithm from ISP 
>> statistics
>> + *   calculated on a frame is stored in the frame context for that 
>> frame in
>> + *   the process() function.
>> + */
>> +
>> +/**
>> + * \var IPAFrameContext::agc
>> + * \brief Automatic Gain Control parameters for this frame
>> + *
>> + * The exposure and gain are provided by the AGC algorithm, and are 
>> to be
>> + * applied to the sensor in order to take effect for this frame.
>> + *
>> + * \var IPAFrameContext::agc.exposure
>> + * \brief Exposure time expressed as a number of lines computed by 
>> the algorithm
>> + *
>> + * \var IPAFrameContext::agc.gain
>> + * \brief Analogue gain multiplier computed by the algorithm
>> + *
>> + * The gain should be adapted to the sensor specific gain code 
>> before applying.
>> + *
>> + * \var IPAFrameContext::agc.autoEnabled
>> + * \brief Manual/automatic AGC state as set by the AeEnable control
>> + *
>> + * \var IPAFrameContext::agc.constraintMode
>> + * \brief Constraint mode as set by the AeConstraintMode control
>> + *
>> + * \var IPAFrameContext::agc.exposureMode
>> + * \brief Exposure mode as set by the AeExposureMode control
>> + *
>> + * \var IPAFrameContext::agc.meteringMode
>> + * \brief Metering mode as set by the AeMeteringMode control
>> + *
>> + * \var IPAFrameContext::agc.maxFrameDuration
>> + * \brief Maximum frame duration as set by the FrameDurationLimits 
>> control
>> + *
>> + * \var IPAFrameContext::agc.updateMetering
>> + * \brief Indicate if new ISP AGC metering parameters need to be 
>> applied
>> + */
>> +
>> +/**
>> + * \var IPAFrameContext::awb
>> + * \brief Automatic White Balance parameters for this frame
>> + *
>> + * \struct IPAFrameContext::awb.gains
>> + * \brief White balance gains
>> + *
>> + * \var IPAFrameContext::awb.gains.red
>> + * \brief White balance gain for R channel
>> + *
>> + * \var IPAFrameContext::awb.gains.green
>> + * \brief White balance gain for G channel
>> + *
>> + * \var IPAFrameContext::awb.gains.blue
>> + * \brief White balance gain for B channel
>> + *
>> + * \var IPAFrameContext::awb.autoEnabled
>> + * \brief Whether the Auto White Balance algorithm is enabled
>> + */
>> +
>> +/**
>> + * \var IPAFrameContext::sensor
>> + * \brief Sensor configuration that used been used for this frame
>> + *
>> + * \var IPAFrameContext::sensor.exposure
>> + * \brief Exposure time expressed as a number of lines
>> + *
>> + * \var IPAFrameContext::sensor.gain
>> + * \brief Analogue gain multiplier
>> + */
>> +
>> +/**
>> + * \struct IPAContext
>> + * \brief Global IPA context data shared between all algorithms
>> + *
>> + * \var IPAContext::configuration
>> + * \brief The IPA session configuration, immutable during the session
>> + *
>> + * \var IPAContext::activeState
>> + * \brief The IPA active state, storing the latest state for all 
>> algorithms
>> + *
>> + * \var IPAContext::frameContexts
>> + * \brief Ring buffer of per-frame contexts
>> + */
>> +
>> +} /* namespace libcamera::ipa::c3isp */
>> diff --git a/src/ipa/c3-isp/ipa_context.h b/src/ipa/c3-isp/ipa_context.h
>> new file mode 100644
>> index 00000000..f1519c4c
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/ipa_context.h
>> @@ -0,0 +1,110 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic Inc.
>> + *
>> + * C3ISP IPA Context
>> + */
>> +
>> +#pragma once
>> +
>> +#include <memory>
>> +
>> +#include <linux/c3-isp-config.h>
>> +
>> +#include <libcamera/base/utils.h>
>> +
>> +#include <libcamera/control_ids.h>
>> +#include <libcamera/controls.h>
>> +#include <libcamera/geometry.h>
>> +#include <libcamera/ipa/core_ipa_interface.h>
>> +
>> +#include <libipa/camera_sensor_helper.h>
>> +#include <libipa/fc_queue.h>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp {
>> +
>> +struct IPASessionConfiguration {
>> +     struct {
>> +             utils::Duration minShutterSpeed;
>> +             utils::Duration maxShutterSpeed;
>> +             double minAnalogueGain;
>> +             double maxAnalogueGain;
>> +
>> +             int32_t defVBlank;
>> +             utils::Duration lineDuration;
>> +             Size size;
>> +     } sensor;
>> +};
>> +
>> +struct IPAActiveState {
>> +     struct {
>> +             uint32_t exposure;
>> +             double gain;
>> +             uint32_t constraintMode;
>> +             uint32_t exposureMode;
> If you include the control_ids header you can use 
> controls::AeConstraintModeEnum /
> AeExposureModeEnum here which is a bit more explicit


Will check this issue.

>> +     } agc;
>> +
>> +     struct {
>> +             struct {
>> +                     struct {
>> +                             double red;
>> +                             double green;
>> +                             double blue;
>> +                     } manual;
>> +                     struct {
>> +                             double red;
>> +                             double green;
>> +                             double blue;
>> +                     } automatic;
>> +             } gains;
>> +
>> +             unsigned int temperatureK;
>> +             bool autoEnabled;
>> +     } awb;
>> +};
>> +
>> +struct IPAFrameContext : public FrameContext {
>> +     struct {
>> +             uint32_t exposure;
>> +             double gain;
>> +             bool autoEnabled;
>> +             controls::AeConstraintModeEnum constraintMode;
>> +             controls::AeExposureModeEnum exposureMode;
>> +             controls::AeMeteringModeEnum meteringMode;
> Ah - like this!


OK.

>> +             utils::Duration maxFrameDuration;
>> +             bool updateMetering;
>> +     } agc;
>> +
>> +     struct {
>> +             struct {
>> +                     double red;
>> +                     double green;
>> +                     double blue;
>> +             } gains;
>> +
>> +             bool autoEnabled;
>> +     } awb;
>> +
>> +     struct {
>> +             uint32_t exposure;
>> +             double gain;
>> +     } sensor;
>> +};
>> +
>> +struct IPAContext {
>> +     IPASessionConfiguration configuration;
>> +     IPAActiveState activeState;
>> +
>> +     FCQueue<IPAFrameContext> frameContexts;
>> +
>> +     ControlInfoMap::Map ctrlMap;
>> +
>> +     /* Interface to the Camera Helper */
>> +     std::unique_ptr<CameraSensorHelper> camHelper;
>> +};
>> +
>> +} /* namespace ipa::c3isp */
>> +
>> +} /* namespace libcamera*/
>> diff --git a/src/ipa/c3-isp/meson.build b/src/ipa/c3-isp/meson.build
>> new file mode 100644
>> index 00000000..fa5c6be0
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/meson.build
>> @@ -0,0 +1,32 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +subdir('algorithms')
>> +subdir('data')
>> +
>> +ipa_name = 'ipa_c3isp'
>> +
>> +c3isp_ipa_sources = files([
>> +    'ipa_context.cpp',
>> +    'params.cpp',
>> +    'c3-isp.cpp',
>> +])
>> +
>> +c3isp_ipa_sources += c3isp_ipa_algorithms
>> +
>> +mod = shared_module(ipa_name, c3isp_ipa_sources,
>> +                    name_prefix : '',
>> +                    include_directories : [ipa_includes],
>> +                    dependencies : [libcamera_private, libipa_dep],
>> +                    install : true,
>> +                    install_dir : ipa_install_dir)
>> +
>> +if ipa_sign_module
>> +    custom_target(ipa_name + '.so.sign',
>> +                  input : mod,
>> +                  output : ipa_name + '.so.sign',
>> +                  command : [ipa_sign, ipa_priv_key, '@INPUT@', 
>> '@OUTPUT@'],
>> +                  install : false,
>> +                  build_by_default : true)
>> +endif
>> +
>> +ipa_names += ipa_name
>> diff --git a/src/ipa/c3-isp/module.h b/src/ipa/c3-isp/module.h
>> new file mode 100644
>> index 00000000..1a66c5d4
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/module.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic
>> + *
>> + * C3ISP IPA Module
>> + */
>> +
>> +#pragma once
>> +
>> +#include <linux/c3-isp-config.h>
>> +
>> +#include <libcamera/ipa/c3isp_ipa_interface.h>
>> +
>> +#include <libipa/module.h>
>> +
>> +#include "ipa_context.h"
>> +#include "params.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp {
>> +
>> +using Module = ipa::Module<IPAContext, IPAFrameContext, 
>> IPACameraSensorInfo,
>> +                        C3ISPParams, c3_isp_stats_info>;
>> +
>> +} /* namespace ipa::c3isp */
>> +
>> +} /* namespace libcamera*/
>> diff --git a/src/ipa/c3-isp/params.cpp b/src/ipa/c3-isp/params.cpp
>> new file mode 100644
>> index 00000000..fd682b53
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/params.cpp
>> @@ -0,0 +1,127 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic Inc.
>> + *
>> + * C3ISP ISP Parameters
>> + */
>> +
>> +#include "params.h"
>> +
>> +#include <map>
>> +#include <stddef.h>
>> +#include <string.h>
>> +
>> +#include <linux/c3-isp-config.h>
>> +#include <linux/videodev2.h>
>> +
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/base/utils.h>
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(C3ISPParams)
>> +
>> +namespace ipa::c3isp {
>> +
>> +namespace {
>> +
>> +struct BlockTypeInfo {
>> +     enum c3_isp_params_block_type type;
>> +     size_t size;
>> +};
>> +
>> +#define C3ISP_BLOCK_TYPE_ENTRY(block, id, type)                      \
>> + {                                                            \
>> + BlockType::block,                                    \
>> + {                                                    \
>> + C3_ISP_PARAMS_BLOCK_##id,                    \
>> +                             sizeof(struct c3_isp_params_##type), \
>> + }                                                    \
>> +     }
>> +
>> +const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {
>> +     C3ISP_BLOCK_TYPE_ENTRY(AWBGains, AWB_GAINS, awb_gains),
>> +     C3ISP_BLOCK_TYPE_ENTRY(AWBConfig, AWB_CONFIG, awb_config),
>> +     C3ISP_BLOCK_TYPE_ENTRY(AEConfig, AE_CONFIG, ae_config),
>> +     C3ISP_BLOCK_TYPE_ENTRY(AFConfig, AF_CONFIG, af_config),
>> +     C3ISP_BLOCK_TYPE_ENTRY(PostGamma, PST_GAMMA, pst_gamma),
>> +     C3ISP_BLOCK_TYPE_ENTRY(Ccm, CCM, ccm),
>> +     C3ISP_BLOCK_TYPE_ENTRY(Csc, CSC, csc),
>> +     C3ISP_BLOCK_TYPE_ENTRY(Blc, BLC, blc),
>> +};
>> +
>> +} /* namespace */
>> +
>> +C3ISPParamsBlockBase::C3ISPParamsBlockBase(BlockType type,
>> +                                        const Span<uint8_t> &data)
>> +     : type_(type), data_(data)
>> +{
>> +     header_ = data.subspan(0, sizeof(c3_isp_params_block_header));
>> +}
>> +
>> +void C3ISPParamsBlockBase::setEnabled(uint16_t flags)
>> +{
>> +     struct c3_isp_params_block_header *header =
>> +             reinterpret_cast<struct c3_isp_params_block_header 
>> *>(header_.data());
>> +
>> +     header->flags = flags;
>> +}
>> +
>> +C3ISPParams::C3ISPParams(Span<uint8_t> data)
>> +     : data_(data), used_(0)
>> +{
>> +     struct c3_isp_params_cfg *buffer =
>> +             reinterpret_cast<struct c3_isp_params_cfg *>(data.data());
>> +
>> +     buffer->version = C3_ISP_PARAMS_BUFFER_V0;
>> +     buffer->data_size = 0;
>> +
>> +     used_ += offsetof(struct c3_isp_params_cfg, data);
>> +}
>> +
>> +Span<uint8_t> C3ISPParams::block(BlockType type)
>> +{
>> +     auto infoIt = kBlockTypeInfo.find(type);
>> +     if (infoIt == kBlockTypeInfo.end()) {
>> +             LOG(C3ISPParams, Error)
>> +                     << "Invalid parameters type "
>> +                     << utils::to_underlying(type);
>> +             return {};
>> +     }
>> +
>> +     const BlockTypeInfo &info = infoIt->second;
>> +
>> +     auto cacheIt = blocks_.find(type);
>> +     if (cacheIt != blocks_.end())
>> +             return cacheIt->second;
>> +
>> +     size_t size = info.size;
>> +     if (size > data_.size() - used_) {
>> +             LOG(C3ISPParams, Error)
>> +                     << "Out of memory to allocate block type "
> Not really an _allocation_ per se. I think I'd re-word this to make 
> clear that the buffer ran out of
> space.


Will re-word this.

>> +                     << utils::to_underlying(type);
>> +             return {};
>> +     }
>> +
>> +     Span<uint8_t> block = data_.subspan(used_, info.size);
>> +     used_ += block.size();
>> +
>> +     struct c3_isp_params_cfg *buffer =
>> +             reinterpret_cast<struct c3_isp_params_cfg 
>> *>(data_.data());
>> +     buffer->data_size += block.size();
>> +
>> +     memset(block.data(), 0, block.size());
>> +
>> +     struct c3_isp_params_block_header *header =
>> +             reinterpret_cast<struct c3_isp_params_block_header 
>> *>(block.data());
>> +     header->type = info.type;
>> +     header->size = block.size();
>> +
>> +     blocks_[type] = block;
>> +
>> +     return block;
>> +}
>> +
>> +} /* namespace ipa::c3isp */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/c3-isp/params.h b/src/ipa/c3-isp/params.h
>> new file mode 100644
>> index 00000000..9bb3877b
>> --- /dev/null
>> +++ b/src/ipa/c3-isp/params.h
>> @@ -0,0 +1,133 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Amlogic Inc.
>> + *
>> + * C3ISP ISP Parameters
>> + */
>> +
>> +#pragma once
>> +
>> +#include <map>
>> +#include <stdint.h>
>> +
>> +#include <linux/c3-isp-config.h>
>> +
>> +#include <libcamera/base/class.h>
>> +#include <libcamera/base/span.h>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::c3isp {
>> +
>> +enum class BlockType {
>> +     AWBGains,
>> +     AWBConfig,
>> +     AEConfig,
>> +     AFConfig,
>> +     PostGamma,
>> +     Ccm,
>> +     Csc,
>> +     Blc,
>> +};
>> +
>> +namespace details {
>> +
>> +template<BlockType B>
>> +struct block_type {
>> +};
>> +
>> +#define C3ISP_DEFINE_BLOCK_TYPE(blocktype, blockStruct)          \
>> + template<>                                               \
>> +     struct block_type<BlockType::blocktype> {                \
>> +             using type = struct c3_isp_params_##blockStruct; \
>> +     };
>> +
>> +C3ISP_DEFINE_BLOCK_TYPE(AWBGains, awb_gains)
>> +C3ISP_DEFINE_BLOCK_TYPE(AWBConfig, awb_config)
>> +C3ISP_DEFINE_BLOCK_TYPE(AEConfig, ae_config)
>> +C3ISP_DEFINE_BLOCK_TYPE(AFConfig, af_config)
>> +C3ISP_DEFINE_BLOCK_TYPE(PostGamma, pst_gamma)
>> +C3ISP_DEFINE_BLOCK_TYPE(Ccm, ccm)
>> +C3ISP_DEFINE_BLOCK_TYPE(Csc, csc)
>> +C3ISP_DEFINE_BLOCK_TYPE(Blc, blc)
>> +
>> +} /* namespace details */
>> +
>> +class C3ISPParams;
>> +
>> +class C3ISPParamsBlockBase
>> +{
>> +public:
>> +     C3ISPParamsBlockBase(BlockType type, const Span<uint8_t> &data);
>> +
>> +     Span<uint8_t> data() const { return data_; }
>> +
>> +     void setEnabled(uint16_t flags);
>> +
>> +private:
>> +     LIBCAMERA_DISABLE_COPY(C3ISPParamsBlockBase)
>> +
>> +     BlockType type_;
>> +     Span<uint8_t> header_;
>> +     Span<uint8_t> data_;
>> +};
>> +
>> +template<BlockType B>
>> +class C3ISPParamsBlock : public C3ISPParamsBlockBase
>> +{
>> +public:
>> +     using Type = typename details::block_type<B>::type;
>> +
>> +     C3ISPParamsBlock(const Span<uint8_t> &data)
>> +             : C3ISPParamsBlockBase(B, data)
>> +     {
>> +     }
>> +
>> +     const Type *operator->() const
>> +     {
>> +             return reinterpret_cast<const Type *>(data().data());
>> +     }
>> +
>> +     Type *operator->()
>> +     {
>> +             return reinterpret_cast<Type *>(data().data());
>> +     }
>> +
>> +     const Type &operator*() const &
>> +     {
>> +             return *reinterpret_cast<const Type *>(data().data());
>> +     }
>> +
>> +     const Type &operator*() &
>> +     {
>> +             return *reinterpret_cast<Type *>(data().data());
>> +     }
>> +};
>> +
>> +class C3ISPParams
>> +{
>> +public:
>> +     C3ISPParams(Span<uint8_t> data);
>> +
>> +     template<BlockType B>
>> +     C3ISPParamsBlock<B> block()
>> +     {
>> +             return C3ISPParamsBlock<B>(block(B));
>> +     }
>> +
>> +     size_t size() const { return used_; }
>> +
>> +private:
>> +     friend class C3ISPParamsBlockBase;
>> +
>> +     Span<uint8_t> block(BlockType type);
>> +
>> +     Span<uint8_t> data_;
>> +     size_t used_;
>> +
>> +     std::map<BlockType, Span<uint8_t>> blocks_;
>> +};
>> +
>> +} /* namespace ipa::c3isp */
>> +
>> +} /* namespace libcamera */


More information about the libcamera-devel mailing list