Re: [PATCH v7 02/18] media: uapi: Add extensible param and stats blocks for RPPX1
From: Jai Luthra
Date: Wed Apr 15 2026 - 02:22:44 EST
Hi Jacopo
Thanks for the review.
Quoting Jacopo Mondi (2026-04-10 21:40:26)
> 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.
> >
> > Signed-off-by: Jai Luthra <jai.luthra+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > include/uapi/linux/media/dreamchip/rppx1-config.h | 728 ++++++++++++++++++++++
> > 1 file changed, 728 insertions(+)
> >
> > diff --git a/include/uapi/linux/media/dreamchip/rppx1-config.h b/include/uapi/linux/media/dreamchip/rppx1-config.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..b9083e6f32b15329333eb13491b50c0aea8d1a32
> > --- /dev/null
> > +++ b/include/uapi/linux/media/dreamchip/rppx1-config.h
> > @@ -0,0 +1,728 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Dreamchip RPP-X1 ISP Driver - Userspace API
> > + *
> > + * Copyright (C) 2026 Renesas Electronics Corp.
> > + * Copyright (C) 2026 Ideas on Board Oy
> > + * Copyright (C) 2026 Ragnatech AB
> > + */
> > +
> > +#ifndef __UAPI_RPP_X1_CONFIG_H
> > +#define __UAPI_RPP_X1_CONFIG_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/media/v4l2-isp.h>
> > +
> > +/*
> > + * Defect Pixel Cluster Correction
> > + */
> > +#define RPPX1_DPCC_METHODS_MAX 3
> > +
> > +/* Linearization (Sensor De-gamma) */
> > +#define RPPX1_LIN_SAMPLES_NUM 17
> > +
> > +/* Gamma Out */
> > +#define RPPX1_GAMMA_OUT_MAX_SAMPLES 17
> > +
> > +/* Lens Shade Correction */
> > +#define RPPX1_LSC_SECTORS_TBL_SIZE 8
> > +#define RPPX1_LSC_SAMPLES_MAX 17
> > +
> > +/* Histogram */
> > +#define RPPX1_HIST_BIN_N_MAX 32
> > +
> > +/* Exposure Measurement */
> > +#define RPPX1_EXM_MEAN_MAX 25
> > +
> > +/* AWB Measurement */
> > +#define RPPX1_AWB_MAX_GRID 1
> > +
> > +/* Color Correction Matrix */
> > +#define RPPX1_CTK_COEFF_MAX 0x8000
> > +#define RPPX1_CTK_OFFSET_MAX 0x800000
> > +
> > +/* Filter */
> > +#define RPPX1_BDM_MAX_TH 0xffff
> > +
> > +/**
> > + * enum rppx1_params_block_type - RPP-X1 extensible params block types
> > + *
> > + * @RPPX1_PARAMS_BLOCK_TYPE_BLS: Black Level Subtraction
> > + * @RPPX1_PARAMS_BLOCK_TYPE_DPCC: Defect Pixel Cluster Correction
> > + * @RPPX1_PARAMS_BLOCK_TYPE_LIN: Linearization (Sensor De-gamma)
> > + * @RPPX1_PARAMS_BLOCK_TYPE_AWB_GAIN: Auto White Balance Gains
> > + * @RPPX1_PARAMS_BLOCK_TYPE_FLT: ISP Filtering
> > + * @RPPX1_PARAMS_BLOCK_TYPE_BDM: Bayer Demosaic
> > + * @RPPX1_PARAMS_BLOCK_TYPE_CTK: Color Correction (Cross-Talk)
> > + * @RPPX1_PARAMS_BLOCK_TYPE_GOC: Gamma Out Correction
> > + * @RPPX1_PARAMS_BLOCK_TYPE_DPF: De-noise Pre-Filter
> > + * @RPPX1_PARAMS_BLOCK_TYPE_DPF_STRENGTH: De-noise Pre-Filter Strength
> > + * @RPPX1_PARAMS_BLOCK_TYPE_LSC: Lens Shading Correction
> > + * @RPPX1_PARAMS_BLOCK_TYPE_AWB_MEAS: AWB Measurement Configuration
> > + * @RPPX1_PARAMS_BLOCK_TYPE_HST_MEAS: Histogram Measurement Configuration
> > + * @RPPX1_PARAMS_BLOCK_TYPE_AEC_MEAS: Auto Exposure Measurement Configuration
> > + */
> > +enum rppx1_params_block_type {
> > + RPPX1_PARAMS_BLOCK_TYPE_BLS,
> > + RPPX1_PARAMS_BLOCK_TYPE_DPCC,
> > + RPPX1_PARAMS_BLOCK_TYPE_LIN,
> > + RPPX1_PARAMS_BLOCK_TYPE_AWB_GAIN,
> > + RPPX1_PARAMS_BLOCK_TYPE_FLT,
> > + RPPX1_PARAMS_BLOCK_TYPE_BDM,
> > + RPPX1_PARAMS_BLOCK_TYPE_CTK,
> > + RPPX1_PARAMS_BLOCK_TYPE_GOC,
> > + RPPX1_PARAMS_BLOCK_TYPE_DPF,
> > + RPPX1_PARAMS_BLOCK_TYPE_DPF_STRENGTH,
> > + RPPX1_PARAMS_BLOCK_TYPE_LSC,
> > + RPPX1_PARAMS_BLOCK_TYPE_AWB_MEAS,
> > + RPPX1_PARAMS_BLOCK_TYPE_HST_MEAS,
> > + RPPX1_PARAMS_BLOCK_TYPE_AEC_MEAS,
> > +};
>
> Let me start with a review of some of the blocks, we'll go through
> them one by one.
>
> > +
> > +/**
> > + * struct rppx1_window - Measurement window
> > + *
> > + * @h_offs: horizontal offset from the left of the frame in pixels
>
> I think it's relevant saying these are 14 bits values
>
> > + * @v_offs: vertical offset from the top of the frame in pixels
> > + * @h_size: horizontal size of the window in pixels
> > + * @v_size: vertical size of the window in pixels
> > + */
> > +struct rppx1_window {
> > + __u16 h_offs;
> > + __u16 v_offs;
> > + __u16 h_size;
> > + __u16 v_size;
> > +};
> > +
> > +/**
> > + * struct rppx1_bls_fixed_val - BLS fixed subtraction values
> > + *
> > + * Fixed black level values subtracted from sensor data per Bayer channel.
> > + * Negative values result in addition. Each value is a 24-bit + sign
> > + * (25-bit signed) fixed-point number stored in a __s32.
>
> I think these should be described as
>
> "Each value is is stored as a signed 2's complement representation
> ranging from -2^24 to 2^24-1."
>
> As the fixed point representation in 2's complement allows to
> represent a negative number with an integer I think the type of the
> fields should be __u32.
Ack, will fix.
>
> > + *
> > + * RPP-X1 supports 12/20/24-bit + sign depending on hardware version.
>
> The ISP reports this through the "bls_version" register field.
>
> I would introduce an enumeration for this and reference it here.
>
> > + * Userspace should provide values at full 24-bit precision; the driver
> > + * truncates to match the hardware.
>
> If you're looking at "bls_version" I didn't find where it is said that
> it impacts the fixed values, I only read it impacts the measured
> values. Have I missed that ?
>
Yes, the BLS_VERSION register doesn't talk about the fixed values, only the
measured values.
But the PRE1_BLS_A_FIXED is 24 bits while PRE2_BLS_A_FIXED is 12 bits,
which matches the measured values described in the BLS_VERSION register.
> > + *
> > + * @r: subtraction value for Bayer pattern R
> > + * @gr: subtraction value for Bayer pattern Gr
> > + * @gb: subtraction value for Bayer pattern Gb
> > + * @b: subtraction value for Bayer pattern B
>
> The manual describes the values as "A", "B", "C" and "D".
>
> These values are matched with the Bayer components according to the
> cropping configuration on the input port. The pipeline should
> carefully crop to the macro-pixel boundary so that A B C and D
> correspond to the sensor's native Bayer ordering.
>
That's a good point, will add a note in the comments. Are you also implying
that we should use A/B/C/D for the names of these parameters here?
> Also, I wouldn't say "subtraction" but simply "Fixed black level for
> channel ..."
Ack.
>
> > + */
> > +struct rppx1_bls_fixed_val {
> > + __s32 r;
> > + __s32 gr;
> > + __s32 gb;
> > + __s32 b;
> > +};
> > +
> > +/**
> > + * struct rppx1_params_bls_config - Black Level Subtraction configuration
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_BLS)
> > + * @enable_auto: 1 = use measured values, 0 = use fixed_val
>
> I would call this "bls_mode" and create and enum for it
>
> enum rppx1_bls_mode {
> RPPX1_BLS_MODE_FIXED,
> RPPX1_BLS_MODE_MEAS,
> };
>
> > + * @en_windows: enabled measurement windows bitmask
>
> For this one as well
>
> enum rppx1_bls_win_en {
> RPPX1_BLS_WIN_EN_OFF,
> RPPX1_BLS_WIN_EN_WIN1,
> RPPX1_BLS_WIN_EN_WIN2,
> RPPX1_BLS_WIN_EN_WIN12,
> };
>
Ack.
> > + * @bls_window1: measurement window 1
> > + * @bls_window2: measurement window 2
> > + * @bls_samples: log2 of the number of measured pixels per Bayer position
> > + * @fixed_val: fixed subtraction values (24-bit + sign)
>
> Let's defer the field length description to the documentation of
> rppx1_bls_fixed_val.
>
> Also, I would document these as "fixed black level values"
>
> > + */
> > +struct rppx1_params_bls_config {
> > + struct v4l2_isp_params_block_header header;
> > + __u8 enable_auto;
> > + __u8 en_windows;
>
> I was about to complaint that you're missing the enable bit but we can
> use the block header maybe ?
>
Indeed, that's what is done in all the module implementations. IMO that's
cleaner than having two different enable toggles.
> > + struct rppx1_window bls_window1;
> > + struct rppx1_window bls_window2;
> > + __u8 bls_samples;
> > + struct rppx1_bls_fixed_val fixed_val;
> > +};
> > +
> > +/**
> > + * struct rppx1_dpcc_methods_config - DPCC methods set configuration
> > + *
> > + * This structure stores the configuration of one set of methods for the DPCC
> > + * algorithm. Multiple methods can be selected in each set (independently for
> > + * the Green and Red/Blue components) through the @method field, the result is
> > + * the logical AND of all enabled methods. The remaining fields set thresholds
> > + * and factors for each method.
> > + *
> > + * @method: method enable bits (RPPX1_DPCC_METHODS_SET_*)
> > + * @line_thresh: line threshold (RPPX1_DPCC_LINE_THRESH_*)
> > + * @line_mad_fac: line MAD factor (RPPX1_DPCC_LINE_MAD_FAC_*)
> > + * @pg_fac: peak gradient factor (RPPX1_DPCC_PG_FAC_*)
> > + * @rnd_thresh: rank neighbor difference threshold (RPPX1_DPCC_RND_THRESH_*)
> > + * @rg_fac: rank gradient factor (RPPX1_DPCC_RG_FAC_*)
> > + */
> > +struct rppx1_dpcc_methods_config {
> > + __u32 method;
>
> Shoulf we define each bit ?
>
> #define RPPX1_DPCC_METHODS_RG_RB_EN (1 << 12)
> #define RPPX1_DPCC_METHODS_RND_RB_EN (1 << 11)
> #define RPPX1_DPCC_METHODS_RO_RB_EN (1 << 10)
> #define RPPX1_DPCC_METHODS_LC_RB_EN (1 << 9)
> #define RPPX1_DPCC_METHODS_PG_RB_EN (1 << 8)
> #define RPPX1_DPCC_METHODS_RG_G_EN (1 << 4)
> #define RPPX1_DPCC_METHODS_RND_G_EN (1 << 3)
> #define RPPX1_DPCC_METHODS_RO_G_EN (1 << 2)
> #define RPPX1_DPCC_METHODS_LC_G_EN (1 << 1)
> #define RPPX1_DPCC_METHODS_PG_G_EN (1 << 0)
>
DPCC implementation is currently stubbed out. Maybe we can just drop it for
this first iteration of the uAPI?
In case we want to implement it before v8, I agree on defining these bits
in the header.
> > + __u32 line_thresh;
> > + __u32 line_mad_fac;
>
> For these and other fields you can define bitshits like the RkISP1
> uAPI header does
>
> > + __u32 pg_fac;
> > + __u32 rnd_thresh;
> > + __u32 rg_fac;
> > +};
> > +
> > +/**
> > + * struct rppx1_params_dpcc_config - Defect Pixel Cluster Correction configuration
> > + *
> > + * @header: block header (type = RPPX1_PARAMS_BLOCK_TYPE_DPCC)
> > + * @mode: DPCC mode (RPPX1_DPCC_MODE_*)
> > + * @output_mode: interpolation output mode (RPPX1_DPCC_OUTPUT_MODE_*)
> > + * @set_use: methods sets selection (RPPX1_DPCC_SET_USE_*)
> > + * @methods: methods sets configuration
> > + * @ro_limits: rank order limits (RPPX1_DPCC_RO_LIMITS_*)
> > + * @rnd_offs: differential rank offsets (RPPX1_DPCC_RND_OFFS_*)
> > + */
> > +struct rppx1_params_dpcc_config {
> > + struct v4l2_isp_params_block_header header;
> > + __u32 mode;
>
> Here as well it might be useful to define
>
> #define RPPX1_DPCC_MODE_STAGE1_EN (1 << 2)
> #define RPPX1_DPCC_MODE_GRAYSCALE (1 << 1)
> #define RPPX1_DPPC_MODE_ENABLE (1 << 0)
>
> > + __u32 output_mode;
> > + __u32 set_use;
>
> The same applies to these two registers
Ack. I missed them because they were unused.
Will go through other modules too before v8.
>
> > + struct rppx1_dpcc_methods_config methods[RPPX1_DPCC_METHODS_MAX];
> > + __u32 ro_limits;
> > + __u32 rnd_offs;
> > +};
>
> DPPC reports a version number that allows to identify the bit width.
> Should it be communicated to userspace through statistics ?
>
Ah, so throughout this series I have picked the largest bit width for the
uAPI. The module driver implementation handles shifting the data before
reading from (stats) and writing to (config) registers. Abstracting away
that detail from userspace.
I thought that would make life easy for libcamera tuning, where it is done
only once for the highest bitdepth.
Am I missing some case where this information is important for tuning?
> > +
> > +/**
> > + * 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
>
> The hardware version should be reported through stats with proper
> defines
>
> Did you get what the difference is between:
> 0x006: 24 bit version
> 0x009: 24 bit, 4 bit dxi
>
> It seems to me dxi samples are 4 bits, aren't they ?
>
I believe in the other version dxi samples will be 3 bits, like they're in
RkISP.
> > + *
> > + * @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;
> > +};
>
> Ack, I'll stop here for the time being :)
>
Thanks,
Jai
[snip]