Re: [PATCH v7 02/18] media: uapi: Add extensible param and stats blocks for RPPX1

[Jai Luthra]

Quoting Jacopo Mondi (2026-04-14 20:35:05)
> Hi Jai
> 
> On Fri, Apr 10, 2026 at 02:35:37PM +0530, Jai Luthra wrote:
> > Define the userspace API for the Dreamchip RPP-X1 ISP extensible
> > parameters and statistics. The RPP-X1 is functionally similar to the
> > RkISP1 already supported upstream, but operates at higher bit depths (up
> > to 24-bit precision in many blocks) and exposes additional configuration
> > options. This warrants a dedicated uAPI rather than reusing the RkISP1
> > definitions.
> >
> > The parameter blocks follow the V4L2 extensible parameters framework
> > using struct v4l2_isp_params_block_header, with each ISP functional
> > block represented as a tagged configuration structure. The statistics
> > buffer provides AWB, auto-exposure and histogram measurement results at
> > native RPP-X1 precision.
> >
> > Not all functional blocks present on the RPP-X1 hardware are included
> > yet, but the format is extensible and new blocks can be added without
> > breaking existing userspace.
> 
> Let me continue the uAPI review
> 
> >
> > Signed-off-by: Jai Luthra <jai.luthra+renesas@xxxxxxxxxxxxxxxx>
> > ---

[snip]

> > +/**
> > + * struct rppx1_lin_curve - Linearization curve for one color channel
> > + *
> > + * The RPP-X1 linearization module supports 12/20/24-bit precision depending
> > + * on hardware version. Values are provided at 24-bit precision; the driver
> > + * truncates to the hardware capability.
> > + *
> > + * @gamma_y: curve y-axis values, each up to 24 bits
> > + */
> > +struct rppx1_lin_curve {
> > +     __u32 gamma_y[RPPX1_LIN_SAMPLES_NUM];
> > +};
> > +
> > +/**
> > + * struct rppx1_lin_curve_dx - Linearization curve x-axis (sampling points)
> > + * increments.
> > + *
> > + * gamma_dx[0] is for the lower samples, so Bits 0:3 for sample 1, ... Bits
> > + * 28:31 for sample 8
> > + * gamma_dx[1] is for the higher samples, so Bits 0:3 for sample 9, ... Bits
> > + * 28:31 for sample 16
> > + *
> > + * The reset values for both fields is 0xcccccccc. This means that each sample
> > + * is 12 units away from the previous one on the x-axis.
> > + *
> > + * @gamma_dx: curve x-axis increments in 4-bit precision
> > + */
> > +struct rppx1_lin_curve_dx {
> > +     __u32 gamma_dx[2];
> > +};
> > +
> > +/**
> > + * struct rppx1_params_lin_config - Linearization (Sensor De-gamma) configuration
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_LIN)
> > + * @curve_r: linearization curve for red channel
> > + * @curve_g: linearization curve for green channel
> > + * @curve_b: linearization curve for blue channel
> > + * @xa_pnts: x axis increment definitions
> > + */
> > +struct rppx1_params_lin_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     struct rppx1_lin_curve curve_r;
> > +     struct rppx1_lin_curve curve_g;
> > +     struct rppx1_lin_curve curve_b;
> > +     struct rppx1_lin_curve_dx xa_pnts;
> > +};
> > +
> > +/**
> > + * struct rppx1_params_lsc_config - Lens Shading Correction configuration
> > + *
> 
> A little more details maybe
> 
> The correction factor are expressed as a grid of 16x16 segments that are
> mapped on the image. The size of each segment is expressed by the
> @x_size_tbl and @y_size_tbl arrays.
> 
> The correction factors are expressed per-color channel in the
> @r_data_tbl, @gr_data_tbl, @gb_data_tbl and @b_data_tbl fields in
> Q2.10 format ranging from 1 to 3.999.
> 
> Pre-calculated multiplication factors shall be provided in the
> @x_grad_tbl and @y_grad_tbl fields. Gradients are expressed as 12 bits
> integer values.
> 

Ack.

> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_LSC)
> > + * @r_data_tbl: sample table red
> 
> I would drop _tbl and use
> 
>       @r_data: Correction factors for the red channel in Q2.10 format
> 
> > + * @gr_data_tbl: sample table green (red)
> > + * @gb_data_tbl: sample table green (blue)
> > + * @b_data_tbl: sample table blue
> 
> Same for these
> 
> > + * @x_grad_tbl: gradient table x
> 
> I would drop _tbl and use
> 
>       @x_grad: Interpolation gradients for each horizontal sector
> 
> > + * @y_grad_tbl: gradient table y
> 
>       @y_grad: Interpolation gradients for each vertical sector
> 
> > + * @x_size_tbl: size table x
> 
>       @x_sect_size: Horizontal sectors sizes
> 
> > + * @y_size_tbl: size table y
> 
>       @y_sect_size: Vertical sectors sizes
> 
> > + * @config_width: reserved
> > + * @config_height: reserved
> 
> What for ?
> 

No idea :(

I copied these over from RkISP1 but can't figure out the potential use.
Will drop.

> > + */
> > +struct rppx1_params_lsc_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     __u16 r_data_tbl[RPPX1_LSC_SAMPLES_MAX][RPPX1_LSC_SAMPLES_MAX];
> > +     __u16 gr_data_tbl[RPPX1_LSC_SAMPLES_MAX][RPPX1_LSC_SAMPLES_MAX];
> > +     __u16 gb_data_tbl[RPPX1_LSC_SAMPLES_MAX][RPPX1_LSC_SAMPLES_MAX];
> > +     __u16 b_data_tbl[RPPX1_LSC_SAMPLES_MAX][RPPX1_LSC_SAMPLES_MAX];
> > +     __u16 x_grad_tbl[RPPX1_LSC_SECTORS_TBL_SIZE];
> > +     __u16 y_grad_tbl[RPPX1_LSC_SECTORS_TBL_SIZE];
> > +     __u16 x_size_tbl[RPPX1_LSC_SECTORS_TBL_SIZE];
> > +     __u16 y_size_tbl[RPPX1_LSC_SECTORS_TBL_SIZE];
> 
> Both sectors and gradients are 16 and not 8.

The change from 8 to 16 is handled in a separate SQUASH patch.

> I would name RPPX1_LSC_SECTORS_TBL_SIZE as RPPX1_LSC_NUM_SECTORS
> 
> > +     __u16 config_width;
> > +     __u16 config_height;
> 
> Drop these if not used

Ack.

> 
> > +};
> > +
> > +/**
> > + * struct rppx1_params_awb_gain_config  - AWB gain configuration
> > + *
> > + * RPP-X1 AWB gains are 18-bit with 12-bit fractional part (0x1000 = 1.0),
> 
> The White Balance (WB) module allows to specify per-color channel
> gains  WB gains are expressed as unsigned fixed-point values in
> Q6.12 format with a maximum of 63.999.
> 
> > + * giving a range of 0.0 to 64.0.
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_AWB_GAIN)
> > + * @gain_red: gain for red component, 18-bit (Q6.12)
> > + * @gain_green_r: gain for green-in-red component, 18-bit (Q6.12)
> > + * @gain_blue: gain for blue component, 18-bit (Q6.12)
> > + * @gain_green_b: gain for green-in-blue component, 18-bit (Q6.12)
> > + */
> > +struct rppx1_params_awb_gain_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     __u32 gain_red;
> > +     __u32 gain_green_r;
> > +     __u32 gain_blue;
> > +     __u32 gain_green_b;
> > +};
> > +
> > +/**
> > + * struct rppx1_params_flt_config - Filter (demosaic/denoise) configuration
> 
> The filter/sharpening block seems to be embedded in the Debayer block.
> I don't think we have exercised this one enough to be confident we
> have a suitable userspace implementation yet.
> 
> Can maybe post-pone this one ?
> 

Sure, will drop.

> > + *
> > + * RPP-X1 thresholds are 18-bit and factors are 8-bit.
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_FLT)
> > + * @mode: filter mode
> > + * @grn_stage1: green filter stage 1 select (range 0x0...0x8)
> > + * @chr_h_mode: chroma filter horizontal mode
> > + * @chr_v_mode: chroma filter vertical mode
> > + * @thresh_bl0: If thresh_bl1 < sum_grad < thresh_bl0 then fac_bl0 is selected (blurring th)
> > + * @thresh_bl1: If sum_grad < thresh_bl1 then fac_bl1 is selected (blurring th)
> > + * @thresh_sh0: If thresh_sh0 < sum_grad < thresh_sh1 then thresh_sh0 is selected (sharpening th)
> > + * @thresh_sh1: If thresh_sh1 < sum_grad then thresh_sh1 is selected (sharpening th)
> > + * @lum_weight: luminance weight, min (bits 0:11), kink (bits 12:23), gain (bits 28:30)
> > + * @fac_sh1: filter factor for sharp1 level
> > + * @fac_sh0: filter factor for sharp0 level
> > + * @fac_mid: filter factor for mid level and for static filter mode
> > + * @fac_bl0: filter factor for blur0 level
> > + * @fac_bl1: filter factor for blur1 level (max blur)
> > + */
> > +struct rppx1_params_flt_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     __u32 mode;
> > +     __u8 grn_stage1;
> > +     __u8 chr_h_mode;
> > +     __u8 chr_v_mode;
> > +     __u32 thresh_bl0;
> > +     __u32 thresh_bl1;
> > +     __u32 thresh_sh0;
> > +     __u32 thresh_sh1;
> > +     __u32 lum_weight;
> > +     __u32 fac_sh1;
> > +     __u32 fac_sh0;
> > +     __u32 fac_mid;
> > +     __u32 fac_bl0;
> > +     __u32 fac_bl1;
> > +};
> > +
> > +/**
> > + * struct rppx1_params_bdm_config - Bayer Demosaic configuration
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_BDM)
> > + * @demosaic_th: threshold for texture detection, 16-bit
> > + */
> > +struct rppx1_params_bdm_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     __u16 demosaic_th;
> > +};
> 
> We don't have an algo in libcamera, right ? This seems simple, but
> until it's not exercised by userspace I would defer introducing it.
> 

Ack.

> > +
> > +/**
> > + * struct rppx1_params_ctk_config - Color Correction (Cross-Talk) configuration
> 
> The module seems to be called CCOR (Color CORrection)
> 
> I would use that term
> 
> > + *
> > + * RPP-X1 coefficients are 16-bit signed fixed-point (Q4.12).
> > + * Range: -8.0 (0x8000) to +7.9996 (0x7FFF), 1.0 = 0x1000.
> 
> The CCOR (Color Correction) module performs color space conversion
> on a pixel-per-pixel basis using a 3x3 matrix of coefficients
> and per-color channel offsets.
> 
> The matrix coefficients are represented as signed fixed point values
> in Q4.12 format ranging from -8 to +7.999.
> 
> The per-channel color offsets are represented as 2's complement values
> stored in 25 bits ranging from -16777216 to 16777215.
> 
> > + *
> > + * RPP-X1 offsets are up to 24-bit + sign depending on hardware version.
> 
> For RPP-X1 the value seems to be fixed to 24 bits.

Indeed, it is 24 + 1 sign bit for the POST and AWB modules.

I got confused by another CCOR instance used for RGB2YUV_CCOR module which
is 12 bit unsigned.

> 
> We might want to report the ccor_version register value in stats if
> this changes for other ISP model. However I would leave it out for the
> time being, assume 24 and if we need to support a different register
> size introduce ccor_version in stats and add a comment here. We won't
> need to change the block definition.
> 

Ah, so here too the driver currently right shifts the parameters if
hardware version reg reports 12/20 bit instead of 24bit.

So I have the same question as the previous patch, does the userspace
benefit from knowing what's actually used, or can we abstract it away in
the uAPI?

> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_CTK)
> > + * @coeff: 3x3 color correction matrix, Q4.12 signed
> > + * @ct_offset: R, G, B offsets, up to 25-bit signed
> > + */
> > +struct rppx1_params_ctk_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     __u16 coeff[3][3];
> > +     __u32 ct_offset[3];
> 
> Or simply 'offset'
> 

Ack.

> > +};
> > +
> > +/**
> > + * struct rppx1_params_goc_config - Gamma Out Correction configuration
> 
> mmm, I would have used 'gamma', as that's what the manual use. But
> registers are named GAMMA_OUT so I would be fine with goc if that's
> preferred
> 

I think GAMMA_OUT would be better for this one, and GAMMA_IN for the LIN
module configuration?

> > + *
> > + * RPP-X1 gamma output values are up to 24-bit depending on hardware version.
> 
> The module allows to apply a Gamma correction curve to RGB data
> represented with a table of 16 entries @gamma_y. The 16 input sample
> points can be equidistant or segmented using a logarithmic scale
> according to the value of @mode.
> 
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_GOC)
> > + * @mode: gamma curve mode (0 = logarithmic, 1 = equidistant)
> 
> Please define an enum and refer to it here
> 
> /**
>  * enum rppx1_params_goc_mode - GOC curve segmentation mode
>  * @RPPX1_PARAMS_GOC_MODE_LOGARITHMIC: Logarithmic segmentation mode (default)
>  * @RPPX1_PARAMS_GOC_MODE_EQUIDISTANT: Equidistant segmentation mode
>  */
> enum rppx1_params_goc_mode {
>         RPPX1_PARAMS_GOC_MODE_LOGARITHMIC,
>         RPPX1_PARAMS_GOC_MODE_EQUIDISTANT
> };
> 

Ack.

> > + * @gamma_y: gamma out curve y-axis values, up to 24-bit
> > + */
> > +struct rppx1_params_goc_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     __u32 mode;
> > +     __u32 gamma_y[RPPX1_GAMMA_OUT_MAX_SAMPLES];
> > +};
> > +
> > +/**
> > + * enum rppx1_dpf_gain_usage - DPF noise function gain usage mode
> > + * @RPPX1_DPF_GAIN_USAGE_DISABLED: gain not used
> > + * @RPPX1_DPF_GAIN_USAGE_NF_GAINS: use noise function gains
> > + * @RPPX1_DPF_GAIN_USAGE_LSC_GAINS: use LSC gains
> > + * @RPPX1_DPF_GAIN_USAGE_NF_LSC_GAINS: use noise function and LSC gains
> > + * @RPPX1_DPF_GAIN_USAGE_AWB_GAINS: use AWB gains
> > + * @RPPX1_DPF_GAIN_USAGE_AWB_LSC_GAINS: use AWB and LSC gains
> > + */
> > +enum rppx1_dpf_gain_usage {
> > +     RPPX1_DPF_GAIN_USAGE_DISABLED,
> > +     RPPX1_DPF_GAIN_USAGE_NF_GAINS,
> > +     RPPX1_DPF_GAIN_USAGE_LSC_GAINS,
> > +     RPPX1_DPF_GAIN_USAGE_NF_LSC_GAINS,
> > +     RPPX1_DPF_GAIN_USAGE_AWB_GAINS,
> > +     RPPX1_DPF_GAIN_USAGE_AWB_LSC_GAINS,
> > +};
> > +
> > +/**
> > + * enum rppx1_nll_scale_mode - DPF noise level lookup scale mode
> > + * @RPPX1_NLL_SCALE_LINEAR: linear scaling
> > + * @RPPX1_NLL_SCALE_LOGARITHMIC: logarithmic scaling
> > + */
> > +enum rppx1_nll_scale_mode {
> > +     RPPX1_NLL_SCALE_LINEAR,
> > +     RPPX1_NLL_SCALE_LOGARITHMIC,
> > +};
> > +
> > +/**
> > + * enum rppx1_dpf_rb_filtersize - DPF red/blue filter kernel size
> > + * @RPPX1_DPF_RB_FILTERSIZE_13x9: 13x9 filter size
> > + * @RPPX1_DPF_RB_FILTERSIZE_9x9: 9x9 filter size
> > + */
> > +enum rppx1_dpf_rb_filtersize {
> > +     RPPX1_DPF_RB_FILTERSIZE_13x9,
> > +     RPPX1_DPF_RB_FILTERSIZE_9x9,
> > +};
> > +
> > +/**
> > + * struct rppx1_dpf_gain - DPF noise function gain configuration
> > + *
> > + * @mode: gain usage mode
> > + * @nf_r_gain: noise function gain replacing AWB gain for red
> > + * @nf_b_gain: noise function gain replacing AWB gain for blue
> > + * @nf_gr_gain: noise function gain replacing AWB gain for green-in-red
> > + * @nf_gb_gain: noise function gain replacing AWB gain for green-in-blue
> > + */
> > +struct rppx1_dpf_gain {
> > +     __u32 mode;
> > +     __u16 nf_r_gain;
> > +     __u16 nf_b_gain;
> > +     __u16 nf_gr_gain;
> > +     __u16 nf_gb_gain;
> > +};
> > +
> > +#define RPPX1_DPF_MAX_NLF_COEFFS                     17
> > +#define RPPX1_DPF_MAX_SPATIAL_COEFFS                 6
> > +
> > +/**
> > + * struct rppx1_dpf_nll - DPF noise level lookup
> > + *
> > + * @coeff: noise level lookup coefficients
> > + * @scale_mode: 0 = linear, 1 = logarithmic
> > + */
> > +struct rppx1_dpf_nll {
> > +     __u16 coeff[RPPX1_DPF_MAX_NLF_COEFFS];
> > +     __u32 scale_mode;
> > +};
> > +
> > +/**
> > + * struct rppx1_dpf_rb_flt - DPF red/blue filter configuration
> > + *
> > + * @fltsize: filter kernel size (0 = 13x9, 1 = 9x9)
> > + * @spatial_coeff: spatial weight coefficients
> > + * @r_enable: enable filter for red pixels
> > + * @b_enable: enable filter for blue pixels
> > + */
> > +struct rppx1_dpf_rb_flt {
> > +     __u32 fltsize;
> > +     __u8 spatial_coeff[RPPX1_DPF_MAX_SPATIAL_COEFFS];
> > +     __u8 r_enable;
> > +     __u8 b_enable;
> > +};
> > +
> > +/**
> > + * struct rppx1_dpf_g_flt - DPF green filter configuration
> > + *
> > + * @spatial_coeff: spatial weight coefficients
> > + * @gr_enable: enable filter for green-in-red pixels
> > + * @gb_enable: enable filter for green-in-blue pixels
> > + */
> > +struct rppx1_dpf_g_flt {
> > +     __u8 spatial_coeff[RPPX1_DPF_MAX_SPATIAL_COEFFS];
> > +     __u8 gr_enable;
> > +     __u8 gb_enable;
> > +};
> > +
> > +/**
> > + * struct rppx1_params_dpf_config - De-noising Pre-Filter configuration
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_DPF)
> > + * @gain: noise function gain
> > + * @g_flt: green filter configuration
> > + * @rb_flt: red/blue filter configuration
> > + * @nll: noise level lookup
> > + */
> > +struct rppx1_params_dpf_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     struct rppx1_dpf_gain gain;
> > +     struct rppx1_dpf_g_flt g_flt;
> > +     struct rppx1_dpf_rb_flt rb_flt;
> > +     struct rppx1_dpf_nll nll;
> > +};
> > +
> > +/**
> > + * struct rppx1_params_dpf_strength_config - DPF strength configuration
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_DPF_STRENGTH)
> > + * @r: filter strength for RED
> > + * @g: filter strength for GREEN
> > + * @b: filter strength for BLUE
> > + */
> > +struct rppx1_params_dpf_strength_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     __u8 r;
> > +     __u8 g;
> > +     __u8 b;
> > +};
> 
> This seems to be named "Bilateral Denoise".
> 
> Same reasoning goes as per the filter module.
> 
> We have an algorithm in libcamera for RkISP1 but it has been under
> heavy rework and I wouldn't consider it final or even very well
> tested.
> 
> The whole denoising part need more work and I think we can we maybe
> post-pone DPF for RPP-X1 ?
> 
> The risk is to define blocks we'll have to re-define later on.
> 

Ack.

> > +
> > +/**
> > + * enum rppx1_awb_mode_type - AWB measurement mode
> > + * @RPPX1_AWB_MODE_MANUAL: manual white balance
> 
> I can't find any reference to Manual mode.
> 
> I see
> 1: RGB measurement
> 0: YcbCr

Indeed seems to be a leftover from RKISP, where too it is unused.

But looking at the implementation in rppx1_wbmeas.c, I see we are free to
program the CCOR coefficients and offsets, unlike RKISP.

WDYT about dropping the modes altogether and exposing that table to the
userspace?

> 
> > + * @RPPX1_AWB_MODE_RGB: RGB measurement mode
> > + * @RPPX1_AWB_MODE_YCBCR: YCbCr measurement mode
> > + */
> > +enum rppx1_awb_mode_type {
> 
> Maybe let's use AWB_MEAS as a prefix
> 
> > +     RPPX1_AWB_MODE_MANUAL,
> 
> Both for the type and the fields
> 
> > +     RPPX1_AWB_MODE_RGB,
> > +     RPPX1_AWB_MODE_YCBCR,
> > +};
> > +
> > +/**
> > + * struct rppx1_params_awb_meas_config - AWB measurement configuration
> > + *
> 
>     * The auto-white balance measurement module supports two
>     * measurement modes, selected by the @awb_mode field.
>     * The measurement window is programmed through the @awb_wnd field.
>     *
>     * To support measurement in YCbCr a color conversion matrix with
>     * programmable offset is available is available in the
>     * @ccor_coeff and @ccor_offs fields.
>     */
> 
> 
> > + * RPP-X1 min_y, max_y, min_c, max_csum, awb_ref_cr, awb_ref_cb are up to
> > + * 24-bit depending on hardware version (8/20/24-bit).
> 
> As for the other similar cases, I think for RPP-X1 we can assume 24
> bits. If another version appears with a different bitwidth, we should
> report `awb_meas_version` through stats and add a comment here about
> the expected fields width.
> 
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_AWB_MEAS)
> > + * @awb_wnd: measurement window
> > + * @awb_mode: measurement mode (from enum rppx1_awb_mode_type)
> 
> Missing 'awb_max_en' with the associated enum
> 

`__u8 enable_ymax_cmp` handles that bit.

IMO an enum would be overkill for a simple disable/enable, we don't do that
elsewhere right?

> /**
>  * enum rppx1_awb_max_en - Enable max luminance threshold
>  *
>  * Luminance max threshold enable. Only pixels with a luminance value
>  * lower than @max_y are considered. Only valid in YCbCr measurement mode.
>  *
>  * @RPPX1_AWB_MEAS_Y_MAX_DISABLE: Disable luminance threshold
>  * @RPPX1_AWB_MEAS_Y_MAX_ENABLE: Enable luminance threshold
>  */
> enum rppx1_awb_max_en {
>         RPPX1_AWB_MEAS_Y_MAX_DISABLE,
>          RPPX1_AWB_MEAS_Y_MAX_ENABLE
> };
> 
> > + * @max_y: upper pixel value limit, up to 24-bit
> 
> This is worth a longer explanation.
> 
>       @max_y: luminance maximum value. Only pixels with luminance
>               value below this threshold are considered. Only valid if
>               @awb_mode is set to YCbCr and @awb_max_en is set to
>               enable.

Ack.

> 
> > + * @min_y: lower pixel value limit, up to 24-bit
> 
>       @min_y_max_g: luminance minimum value in YCbCr mode; maximum
>       green value in RGB mode
> 
> > + * @max_csum: chrominance sum maximum, up to 24-bit
> 
> Missing:
> 
>       @enable_ymax_cmp: enable Y_MAX compare
> 

Hmm, I see it in my original mail?

> > + * @min_c: chrominance minimum, up to 24-bit
> > + * @frames: number of frames for mean value calculation (0 = 1 frame)
> 
>       (0 = 1 frame, ..., 7 = 8 frames)

> 
> > + * @awb_ref_cr: reference Cr for AWB regulation, up to 24-bit
> 
>       @ref_cr_max_r: reference Cr or maximum red pixels value

Ack.

> 
> > + * @awb_ref_cb: reference Cb for AWB regulation, up to 24-bit
> 
>       @ref_cb_max_b: reference Cb or maximum blue pixels value
> 
> > + *
> 
> Missing
>       @ccor_coeffs: Color conversion matrix coefficients. The
>       coefficients have to be programmed according to the measurement
>       mode in use.
>       @ccor_offs: Color conversion matrix offsets.
> 
> As these two fields match the color conversion matrix, it might be
> worth defining a type for it where to defer the description of the
> coefficients and offset representations.

So should we drop the RGB/YCbCr Modes and use only the table, or keep the
modes, and the "Manual mode" when the user wants to specify a table?

> 
> > + */
> > +struct rppx1_params_awb_meas_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     struct rppx1_window awb_wnd;
> > +     __u32 awb_mode;
> > +     __u32 max_y;
> > +     __u32 min_y;
> > +     __u32 max_csum;
> > +     __u32 min_c;
> > +     __u8 frames;
> > +     __u32 awb_ref_cr;
> > +     __u32 awb_ref_cb;
> > +     __u8 enable_ymax_cmp;
> > +};
> > +
> > +/**
> > + * enum rppx1_histogram_mode - Histogram measurement mode
> > + * @RPPX1_HISTOGRAM_MODE_DISABLE: histogram disabled
> > + * @RPPX1_HISTOGRAM_MODE_RGB_COMBINED: combined RGB histogram
> > + * @RPPX1_HISTOGRAM_MODE_R_HISTOGRAM: red channel histogram
> > + * @RPPX1_HISTOGRAM_MODE_G_HISTOGRAM: green channel histogram
> > + * @RPPX1_HISTOGRAM_MODE_B_HISTOGRAM: blue channel histogram
> > + * @RPPX1_HISTOGRAM_MODE_Y_HISTOGRAM: luminance histogram
> > + */
> > +enum rppx1_histogram_mode {
> > +     RPPX1_HISTOGRAM_MODE_DISABLE,
> > +     RPPX1_HISTOGRAM_MODE_RGB_COMBINED,
> > +     RPPX1_HISTOGRAM_MODE_R_HISTOGRAM,
> > +     RPPX1_HISTOGRAM_MODE_G_HISTOGRAM,
> > +     RPPX1_HISTOGRAM_MODE_B_HISTOGRAM,
> > +     RPPX1_HISTOGRAM_MODE_Y_HISTOGRAM,
> > +};
> > +
> > +#define RPPX1_HISTOGRAM_WEIGHT_GRIDS_SIZE            25
> 
> Missing the tap points definitions for HIST_CHANNEL_SEL

Handled in a SQUASH patch later.

> 
> > +
> > +/**
> > + * struct rppx1_params_hst_config - Histogram measurement configuration
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_HST_MEAS)
> > + * @mode: histogram mode (from enum rppx1_histogram_mode)
> 
> Missing:
>       @tap_point
> 
> 
> > + * @histogram_predivider: process every Nth pixel
> 
> I see a separate v_stepsize and h_step_inc for the subsampling
> 

Yeah I chose to leave this particular one out because it seemed a little
more complex than others.

Will do before v8.

> > + * @meas_window: measurement window coordinates
> > + * @hist_weight: weighting factors for sub-windows (5x5 grid)
> 
> There also are three programmable coefficients, offsets and shifts

The coefficients are currently not exposed, instead we have `enum
rppx1_histogram_mode` handling RGB_COMBINED or separate R/G/B/Y
Histograms.

Same question as AWB, do we want to drop the modes and expose the
coefficients directly, or maybe have an extra "MANUAL" mode and keep the
enum?

> 
> > + */
> > +struct rppx1_params_hst_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     __u32 mode;
> > +     __u8 histogram_predivider;
> > +     struct rppx1_window meas_window;
> > +     __u8 hist_weight[RPPX1_HISTOGRAM_WEIGHT_GRIDS_SIZE];
> > +};
> > +
> > +/**
> > + * enum rppx1_exp_meas_mode - Exposure measurement mode
> > + * @RPPX1_EXP_MEASURING_MODE_0: Y = 16 + 0.25R + 0.5G + 0.1094B
> > + * @RPPX1_EXP_MEASURING_MODE_1: Y = (R + G + B) x (85/256)
> > + */
> > +enum rppx1_exp_meas_mode {
> > +     RPPX1_EXP_MEASURING_MODE_0,
> > +     RPPX1_EXP_MEASURING_MODE_1,
> 
> This doesn't match the definition of EXM_MODE I see:
> 
>         RPPX1_EXP_MEASURING_MODE_0 = Disabled
>         RPPX1_EXP_MEASURING_MODE_1 = Y/R/G/B exposure measurement
>         RPPX1_EXP_MEASURING_MODE_2 = RGB Bayer exposure measurement

This is fixed in the SQUASH patch later which also exposes programmable
coefficients and sampling point.

The "MODE_0 = Disabled" is not exposed in the enum but rather through the
V4L2_ISP_PARAMS_FL_BLOCK_DISABLE flag. Again, having only one toggle in
uAPI seems better than two.

> > +};
> > +
> 
> Missing TAP point definitions for EXM_CHANNEL_SEL
> 

Done in SQUASH patch later in the series.

> > +/**
> > + * struct rppx1_params_aec_config - Auto Exposure measurement configuration
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_AEC_MEAS)
> > + * @mode: exposure measure mode (from enum rppx1_exp_meas_mode)
> > + * @autostop: 0 = continuous, 1 = stop after one frame
> 
> Seems not to be supported
> 

The hardware documentation defines the bit, but says it's not supported.
So, it would mean that it might be supported in some later revision of the
HW?

Thus, I kept it around. But we can drop it too and define a new block
later.

> > + * @meas_window: measurement window coordinates
> > + */
> > +struct rppx1_params_aec_config {
> > +     struct v4l2_isp_params_block_header header;
> > +     __u32 mode;
> > +     __u32 autostop;
> 
> There also are coefficients
> 
>         /* Unsigned Q0.7 values ranging from 0 to 1.992 */
>         struct {
>                 r;
>                 gr;
>                 b;
>                 gb;
>         } coeffs;
> 

Handled in the SQUASH patch.

> > +     struct rppx1_window meas_window;
> > +};
> > +
> > +/**
> > + * RPPX1_PARAMS_MAX_SIZE - Maximum size of all RPP-X1 parameter blocks
> > + */
> > +#define RPPX1_PARAMS_MAX_SIZE                                                \
> > +     (sizeof(struct rppx1_params_bls_config)                 +       \
> > +     sizeof(struct rppx1_params_dpcc_config)                 +       \
> > +     sizeof(struct rppx1_params_lin_config)                  +       \
> > +     sizeof(struct rppx1_params_awb_gain_config)             +       \
> > +     sizeof(struct rppx1_params_flt_config)                  +       \
> > +     sizeof(struct rppx1_params_bdm_config)                  +       \
> > +     sizeof(struct rppx1_params_ctk_config)                  +       \
> > +     sizeof(struct rppx1_params_goc_config)                  +       \
> > +     sizeof(struct rppx1_params_dpf_config)                  +       \
> > +     sizeof(struct rppx1_params_dpf_strength_config)         +       \
> > +     sizeof(struct rppx1_params_lsc_config)                  +       \
> > +     sizeof(struct rppx1_params_awb_meas_config)             +       \
> > +     sizeof(struct rppx1_params_hst_config)                  +       \
> > +     sizeof(struct rppx1_params_aec_config))
> > +
> > +/* ---------------------------------------------------------------------------
> > + * Statistics Structures
> > + *
> > + * Native RPP-X1 precision. Fields use __u32 where the hardware provides
> > + * wider-than-8-bit results.
> > + */
> > +
> > +/**
> > + * struct rppx1_awb_meas - AWB measured values
> > + *
> > + * @cnt: white pixel count
> > + * @mean_y_or_g: mean Y (or G in RGB mode), up to 24-bit
> > + * @mean_cb_or_b: mean Cb (or B in RGB mode), up to 24-bit
> > + * @mean_cr_or_r: mean Cr (or R in RGB mode), up to 24-bit
> > + */
> > +struct rppx1_awb_meas {
> > +     __u32 cnt;
> > +     __u32 mean_y_or_g;
> > +     __u32 mean_cb_or_b;
> > +     __u32 mean_cr_or_r;
> > +};
> > +
> > +/**
> > + * struct rppx1_awb_stat - AWB statistics
> > + *
> > + * @awb_mean: measured AWB data
> > + */
> > +struct rppx1_awb_stat {
> > +     struct rppx1_awb_meas awb_mean[RPPX1_AWB_MAX_GRID];
> > +};
> > +
> > +/**
> > + * struct rppx1_bls_meas_val - BLS measured values
> > + *
> > + * RPP-X1 BLS statistics can be 8/20/24-bit depending on version.
> > + *
> > + * @meas_r: mean measured value for Bayer pattern R
> > + * @meas_gr: mean measured value for Bayer pattern Gr
> > + * @meas_gb: mean measured value for Bayer pattern Gb
> > + * @meas_b: mean measured value for Bayer pattern B
> > + */
> > +struct rppx1_bls_meas_val {
> > +     __u32 meas_r;
> > +     __u32 meas_gr;
> > +     __u32 meas_gb;
> > +     __u32 meas_b;
> > +};
> > +
> > +/**
> > + * struct rppx1_ae_stat - Auto Exposure statistics
> > + *
> > + * RPP-X1 exposure mean values are up to 20-bit depending on version.
> > + * The image is divided into a 5x5 grid (25 blocks).
> > + *
> > + * @exp_mean: mean luminance values per block, up to 20-bit
> > + * @bls_val: BLS measured values
> > + */
> > +struct rppx1_ae_stat {
> > +     __u32 exp_mean[RPPX1_EXM_MEAN_MAX];
> > +     struct rppx1_bls_meas_val bls_val;
> > +};
> > +
> > +/**
> > + * struct rppx1_hist_stat - Histogram statistics
> > + *
> > + * @hist_bins: 32 histogram bin counters, each 20-bit unsigned fixed point
> > + *          (bits 0-4 fractional, bits 5-19 integer)
> > + */
> > +struct rppx1_hist_stat {
> > +     __u32 hist_bins[RPPX1_HIST_BIN_N_MAX];
> > +};
> > +
> > +/**
> > + * struct rppx1_stat - RPP-X1 3A statistics
> > + *
> > + * @awb: auto white balance statistics
> > + * @ae: auto exposure statistics
> > + * @hist: histogram statistics
> > + */
> > +struct rppx1_stat {
> > +     struct rppx1_awb_stat awb;
> > +     struct rppx1_ae_stat ae;
> > +     struct rppx1_hist_stat hist;
> > +};
> > +
> > +/**
> > + * RPPX1_STAT_AWB - AWB measurement data available
> > + * RPPX1_STAT_AUTOEXP - Auto exposure measurement data available
> > + *
> Missing documentation of:
> 
>       RPPX1_STAT_HIST - Histogram measurement data available
> 
> > + */
> > +#define RPPX1_STAT_AWB                       (1U << 0)
> > +#define RPPX1_STAT_AUTOEXP           (1U << 1)
> > +#define RPPX1_STAT_HIST                      (1U << 2)
> > +
> > +/**
> > + * struct rppx1_stat_buffer - RPP-X1 statistics metadata buffer
> > + *
> > + * @meas_type: bitmask of available measurements (RPPX1_STAT_*)
> > + * @frame_id: frame identifier for synchronization
> > + * @params: statistics data
> > + */
> > +struct rppx1_stat_buffer {
> > +     __u32 meas_type;
> > +     __u32 frame_id;
> > +     struct rppx1_stat params;
> 
> Maybe name it stats as well ?
> 

Ack.

> Now that v2 of extensible stats is out, I would rebase this on top of
> them
> 

Will do.

Thanks,
    Jai

> Thank you!
>    j
> 
> > +};
> > +
> > +#endif /* __UAPI_RPP_X1_CONFIG_H */
> >
> > --
> > 2.53.0
> >
> >