Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 10 Jul 2020 16:36:37 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc:     Helen Koike <helen.koike@collabora.com>,
        linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org,
        eddie.cai.linux@gmail.com, mchehab@kernel.org, heiko@sntech.de,
        jacob2.chen@rock-chips.com, jeffy.chen@rock-chips.com,
        zyc@rock-chips.com, linux-kernel@vger.kernel.org,
        tfiga@chromium.org, hans.verkuil@cisco.com,
        sakari.ailus@linux.intel.com, kernel@collabora.com,
        ezequiel@collabora.com, linux-media@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, zhengsq@rock-chips.com
Subject: Re: [PATCH v8 03/14] media: rkisp1: Add user space ABI definitions
Message-ID: <20200710133637.GY5964@pendragon.ideasonboard.com>
References: <20190730184256.30338-1-helen.koike@collabora.com>
 <20190730184256.30338-4-helen.koike@collabora.com>
 <20190815184659.GA5011@pendragon.ideasonboard.com>
 <0614045e-ec3f-1aef-9d51-d3bd8276a47a@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <0614045e-ec3f-1aef-9d51-d3bd8276a47a@collabora.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Dafna,

On Fri, Jul 10, 2020 at 02:59:36PM +0200, Dafna Hirschfeld wrote:
> On 15.08.19 20:46, Laurent Pinchart wrote:
> > On Tue, Jul 30, 2019 at 03:42:45PM -0300, Helen Koike wrote:
> >> From: Jeffy Chen <jeffy.chen@rock-chips.com>
> >>
> >> Add the header for userspace
> >>
> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> >> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
> >> [update for upstream]
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >> Hi,
> >>
> >> I don't have the "REF_01 - ISP_user_manual, Rev 2.57" document that was
> >> mentioned in previous version, so I'm adding a TODO for now to improve
> >> the docs once we collect the information.
> > 
> > I would keep the document name in the TODO entry, for reference.
> > 
> >> If Rockchip people could help here it would be great.
> > 
> > As you don't have access to the documentation I won't focus on the
> > issues that you can't address, but in general I think we need more
> > documentation than what is provided here. In particular we need a
> > description of the pipeline, with the order of the processing blocks.
> > 
> >> Changes in v8: None
> >> Changes in v7:
> >> - Fix checkpatch errors (lines over 80 and SPDX)
> >> - Add TODO to improve docs
> >>
> >>   include/uapi/linux/rkisp1-config.h | 816 +++++++++++++++++++++++++++++
> >>   1 file changed, 816 insertions(+)
> >>   create mode 100644 include/uapi/linux/rkisp1-config.h
> >>
> >> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> >> new file mode 100644
> >> index 000000000000..9ab979bb4adb
> >> --- /dev/null
> >> +++ b/include/uapi/linux/rkisp1-config.h
> >> @@ -0,0 +1,816 @@
> >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> >> +/*
> >> + * Rockchip isp1 driver
> > 
> > Maybe "Rockchip ISP1 userspace API" ?
> > 
> >> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> >> + */
> >> +
> >> +/*
> >> + * TODO: Improve documentation, mostly regarding abbreviation and hardware
> >> + * specificities.
> >> + */
> >> +
> >> +#ifndef _UAPI_RKISP1_CONFIG_H
> >> +#define _UAPI_RKISP1_CONFIG_H
> >> +
> >> +#include <linux/types.h>
> >> +#include <linux/v4l2-controls.h>
> > 
> > Is v4l2-controls.h needed ?
> > 
> >> +
> >> +#define CIFISP_MODULE_DPCC              (1 << 0)
> >> +#define CIFISP_MODULE_BLS               (1 << 1)
> >> +#define CIFISP_MODULE_SDG               (1 << 2)
> >> +#define CIFISP_MODULE_HST               (1 << 3)
> >> +#define CIFISP_MODULE_LSC               (1 << 4)
> >> +#define CIFISP_MODULE_AWB_GAIN          (1 << 5)
> >> +#define CIFISP_MODULE_FLT               (1 << 6)
> >> +#define CIFISP_MODULE_BDM               (1 << 7)
> >> +#define CIFISP_MODULE_CTK               (1 << 8)
> >> +#define CIFISP_MODULE_GOC               (1 << 9)
> >> +#define CIFISP_MODULE_CPROC             (1 << 10)
> >> +#define CIFISP_MODULE_AFC               (1 << 11)
> >> +#define CIFISP_MODULE_AWB               (1 << 12)
> >> +#define CIFISP_MODULE_IE                (1 << 13)
> >> +#define CIFISP_MODULE_AEC               (1 << 14)
> >> +#define CIFISP_MODULE_WDR               (1 << 15)
> >> +#define CIFISP_MODULE_DPF               (1 << 16)
> >> +#define CIFISP_MODULE_DPF_STRENGTH      (1 << 17)
> > 
> > Should we use BIT() here ?
> > 
> >> +
> >> +#define CIFISP_CTK_COEFF_MAX            0x100
> >> +#define CIFISP_CTK_OFFSET_MAX           0x800
> >> +
> >> +#define CIFISP_AE_MEAN_MAX              25
> >> +#define CIFISP_HIST_BIN_N_MAX           16
> >> +#define CIFISP_AFM_MAX_WINDOWS          3
> >> +#define CIFISP_DEGAMMA_CURVE_SIZE       17
> >> +
> >> +#define CIFISP_BDM_MAX_TH               0xFF
> > 
> > Hex constants are usually expressed lower-case in the kernel.
> > 
> >> +
> >> +/*
> >> + * Black level compensation
> >> + */
> >> +/* maximum value for horizontal start address */
> >> +#define CIFISP_BLS_START_H_MAX             0x00000FFF
> >> +/* maximum value for horizontal stop address */
> >> +#define CIFISP_BLS_STOP_H_MAX              0x00000FFF
> >> +/* maximum value for vertical start address */
> >> +#define CIFISP_BLS_START_V_MAX             0x00000FFF
> >> +/* maximum value for vertical stop address */
> >> +#define CIFISP_BLS_STOP_V_MAX              0x00000FFF
> >> +/* maximum is 2^18 = 262144*/
> >> +#define CIFISP_BLS_SAMPLES_MAX             0x00000012
> >> +/* maximum value for fixed black level */
> >> +#define CIFISP_BLS_FIX_SUB_MAX             0x00000FFF
> >> +/* minimum value for fixed black level */
> >> +#define CIFISP_BLS_FIX_SUB_MIN             0xFFFFF000
> >> +/* 13 bit range (signed)*/
> >> +#define CIFISP_BLS_FIX_MASK                0x00001FFF
> >> +
> >> +/*
> >> + * Automatic white balance measurments
> >> + */
> >> +#define CIFISP_AWB_MAX_GRID                1
> >> +#define CIFISP_AWB_MAX_FRAMES              7
> >> +
> >> +/*
> >> + * Gamma out
> >> + */
> >> +/* Maximum number of color samples supported */
> >> +#define CIFISP_GAMMA_OUT_MAX_SAMPLES       17
> >> +
> >> +/*
> >> + * Lens shade correction
> >> + */
> >> +#define CIFISP_LSC_GRAD_TBL_SIZE           8
> >> +#define CIFISP_LSC_SIZE_TBL_SIZE           8
> >> +/*
> >> + * The following matches the tuning process,
> >> + * not the max capabilities of the chip.
> >> + * Last value unused.
> >> + */
> >> +#define	CIFISP_LSC_DATA_TBL_SIZE           290
> >> +
> >> +/*
> >> + * Histogram calculation
> >> + */
> >> +/* Last 3 values unused. */
> >> +#define CIFISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
> >> +
> >> +/*
> >> + * Defect Pixel Cluster Correction
> >> + */
> >> +#define CIFISP_DPCC_METHODS_MAX       3
> >> +
> >> +/*
> >> + * Denoising pre filter
> >> + */
> >> +#define CIFISP_DPF_MAX_NLF_COEFFS      17
> >> +#define CIFISP_DPF_MAX_SPATIAL_COEFFS  6
> >> +
> >> +/*
> >> + * Measurement types
> >> + */
> >> +#define CIFISP_STAT_AWB           (1 << 0)
> >> +#define CIFISP_STAT_AUTOEXP       (1 << 1)
> >> +#define CIFISP_STAT_AFM_FIN       (1 << 2)
> >> +#define CIFISP_STAT_HIST          (1 << 3)
> >> +
> >> +enum cifisp_histogram_mode {
> >> +	CIFISP_HISTOGRAM_MODE_DISABLE,
> >> +	CIFISP_HISTOGRAM_MODE_RGB_COMBINED,
> >> +	CIFISP_HISTOGRAM_MODE_R_HISTOGRAM,
> >> +	CIFISP_HISTOGRAM_MODE_G_HISTOGRAM,
> >> +	CIFISP_HISTOGRAM_MODE_B_HISTOGRAM,
> >> +	CIFISP_HISTOGRAM_MODE_Y_HISTOGRAM
> >> +};
> >> +
> >> +enum cifisp_awb_mode_type {
> >> +	CIFISP_AWB_MODE_MANUAL,
> >> +	CIFISP_AWB_MODE_RGB,
> >> +	CIFISP_AWB_MODE_YCBCR
> >> +};
> >> +
> >> +enum cifisp_flt_mode {
> >> +	CIFISP_FLT_STATIC_MODE,
> >> +	CIFISP_FLT_DYNAMIC_MODE
> >> +};
> >> +
> >> +/**
> >> + * enum cifisp_exp_ctrl_autostop - stop modes
> >> + * @CIFISP_EXP_CTRL_AUTOSTOP_0: continuous measurement
> >> + * @CIFISP_EXP_CTRL_AUTOSTOP_1: stop measuring after a complete frame
> >> + */
> >> +enum cifisp_exp_ctrl_autostop {
> >> +	CIFISP_EXP_CTRL_AUTOSTOP_0 = 0,
> >> +	CIFISP_EXP_CTRL_AUTOSTOP_1 = 1,
> >> +};
> >> +
> >> +/**
> >> + * enum cifisp_exp_meas_mode - Exposure measure mode
> >> + * @CIFISP_EXP_MEASURING_MODE_0: Y = 16 + 0.25R + 0.5G + 0.1094B
> >> + * @CIFISP_EXP_MEASURING_MODE_1: Y = (R + G + B) x (85/256)
> >> + */
> >> +enum cifisp_exp_meas_mode {
> >> +	CIFISP_EXP_MEASURING_MODE_0,
> >> +	CIFISP_EXP_MEASURING_MODE_1,
> >> +};
> >> +
> >> +/*---------- PART1: Input Parameters ------------*/
> >> +
> >> +struct cifisp_window {
> >> +	__u16 h_offs;
> >> +	__u16 v_offs;
> >> +	__u16 h_size;
> >> +	__u16 v_size;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_bls_fixed_val - BLS fixed subtraction values
> >> + *
> >> + * The values will be subtracted from the sensor
> >> + * values. Therefore a negative value means addition instead of subtraction!
> >> + *
> >> + * @r: Fixed (signed!) subtraction value for Bayer pattern R
> >> + * @gr: Fixed (signed!) subtraction value for Bayer pattern Gr
> >> + * @gb: Fixed (signed!) subtraction value for Bayer pattern Gb
> >> + * @b: Fixed (signed!) subtraction value for Bayer pattern B
> >> + */
> >> +struct cifisp_bls_fixed_val {
> >> +	__s16 r;
> >> +	__s16 gr;
> >> +	__s16 gb;
> >> +	__s16 b;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_bls_config - Configuration used by black level subtraction
> >> + *
> >> + * @enable_auto: Automatic mode activated means that the measured values
> >> + *		 are subtracted. Otherwise the fixed subtraction
> >> + *		 values will be subtracted.
> >> + * @en_windows: enabled window
> >> + * @bls_window1: Measurement window 1 size
> >> + * @bls_window2: Measurement window 2 size
> >> + * @bls_samples: Set amount of measured pixels for each Bayer position
> >> + *		 (A, B,C and D) to 2^bls_samples.
> >> + * @cifisp_bls_fixed_val: Fixed subtraction values
> >> + */
> >> +struct cifisp_bls_config {
> >> +	__u8 enable_auto;
> >> +	__u8 en_windows;
> >> +	struct cifisp_window bls_window1;
> >> +	struct cifisp_window bls_window2;
> >> +	__u8 bls_samples;
> >> +	struct cifisp_bls_fixed_val fixed_val;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_dpcc_methods_config - Methods Configuration used by DPCC
> >> + *
> >> + * Methods Configuration used by Defect Pixel Cluster Correction
> >> + *
> >> + * @method: Method enable bits
> >> + * @line_thresh: Line threshold
> >> + * @line_mad_fac: Line MAD factor
> >> + * @pg_fac: Peak gradient factor
> >> + * @rnd_thresh: Rank Neighbor Difference threshold
> >> + * @rg_fac: Rank gradient factor
> >> + */
> >> +struct cifisp_dpcc_methods_config {
> >> +	__u32 method;
> >> +	__u32 line_thresh;
> >> +	__u32 line_mad_fac;
> >> +	__u32 pg_fac;
> >> +	__u32 rnd_thresh;
> >> +	__u32 rg_fac;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_dpcc_methods_config - Configuration used by DPCC
> > 
> > Should be cifisp_dpcc_config.
> > 
> >> + *
> >> + * Configuration used by Defect Pixel Cluster Correction
> >> + *
> >> + * @mode: dpcc output mode
> >> + * @output_mode: whether use hard coded methods
> >> + * @set_use: stage1 methods set
> >> + * @methods: methods config
> >> + * @ro_limits: rank order limits
> >> + * @rnd_offs: differential rank offsets for rank neighbor difference
> >> + */
> >> +struct cifisp_dpcc_config {
> >> +	__u32 mode;
> >> +	__u32 output_mode;
> >> +	__u32 set_use;
> >> +	struct cifisp_dpcc_methods_config methods[CIFISP_DPCC_METHODS_MAX];
> >> +	__u32 ro_limits;
> >> +	__u32 rnd_offs;
> >> +} __attribute__ ((packed));
> >> +
> >> +struct cifisp_gamma_corr_curve {
> >> +	__u16 gamma_y[CIFISP_DEGAMMA_CURVE_SIZE];
> >> +} __attribute__ ((packed));
> >> +
> >> +struct cifisp_gamma_curve_x_axis_pnts {
> >> +	__u32 gamma_dx0;
> >> +	__u32 gamma_dx1;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_gamma_corr_curve - Configuration used by sensor degamma
> > 
> > Should be cifisp_sdg_config
> > 
> >> + *
> >> + * @curve_x: gamma curve point definition axis for x
> >> + * @xa_pnts: x increments
> >> + */
> >> +struct cifisp_sdg_config {
> >> +	struct cifisp_gamma_corr_curve curve_r;
> >> +	struct cifisp_gamma_corr_curve curve_g;
> >> +	struct cifisp_gamma_corr_curve curve_b;
> >> +	struct cifisp_gamma_curve_x_axis_pnts xa_pnts;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_lsc_config - Configuration used by Lens shading correction
> >> + *
> >> + * refer to REF_01 for details
> >> + */
> >> +struct cifisp_lsc_config {
> >> +	__u32 r_data_tbl[CIFISP_LSC_DATA_TBL_SIZE];
> >> +	__u32 gr_data_tbl[CIFISP_LSC_DATA_TBL_SIZE];
> >> +	__u32 gb_data_tbl[CIFISP_LSC_DATA_TBL_SIZE];
> >> +	__u32 b_data_tbl[CIFISP_LSC_DATA_TBL_SIZE];
> >> +
> >> +	__u32 x_grad_tbl[CIFISP_LSC_GRAD_TBL_SIZE];
> >> +	__u32 y_grad_tbl[CIFISP_LSC_GRAD_TBL_SIZE];
> >> +
> >> +	__u32 x_size_tbl[CIFISP_LSC_SIZE_TBL_SIZE];
> >> +	__u32 y_size_tbl[CIFISP_LSC_SIZE_TBL_SIZE];
> >> +	__u16 config_width;
> >> +	__u16 config_height;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_ie_config - Configuration used by image effects
> >> + *
> >> + * @eff_mat_1: 3x3 Matrix Coefficients for Emboss Effect 1
> >> + * @eff_mat_2: 3x3 Matrix Coefficients for Emboss Effect 2
> >> + * @eff_mat_3: 3x3 Matrix Coefficients for Emboss 3/Sketch 1
> >> + * @eff_mat_4: 3x3 Matrix Coefficients for Sketch Effect 2
> >> + * @eff_mat_5: 3x3 Matrix Coefficients for Sketch Effect 3
> >> + * @eff_tint: Chrominance increment values of tint (used for sepia effect)
> >> + */
> >> +struct cifisp_ie_config {
> >> +	__u16 effect;
> >> +	__u16 color_sel;
> >> +	__u16 eff_mat_1;
> >> +	__u16 eff_mat_2;
> >> +	__u16 eff_mat_3;
> >> +	__u16 eff_mat_4;
> >> +	__u16 eff_mat_5;
> >> +	__u16 eff_tint;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_cproc_config - Configuration used by Color Processing
> >> + *
> >> + * @c_out_range: Chrominance pixel clipping range at output.
> >> + *		 (0 for limit, 1 for full)
> >> + * @y_in_range: Luminance pixel clipping range at output.
> >> + * @y_out_range: Luminance pixel clipping range at output.
> >> + * @contrast: 00~ff, 0.0~1.992
> >> + * @brightness: 80~7F, -128~+127
> >> + * @sat: saturation, 00~FF, 0.0~1.992
> >> + * @hue: 80~7F, -90~+87.188
> >> + */
> >> +struct cifisp_cproc_config {
> >> +	__u8 c_out_range;
> >> +	__u8 y_in_range;
> >> +	__u8 y_out_range;
> >> +	__u8 contrast;
> >> +	__u8 brightness;
> >> +	__u8 sat;
> >> +	__u8 hue;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_awb_meas_config - Configuration used by auto white balance
> >> + *
> >> + * @awb_wnd: white balance measurement window (in pixels)
> >> + *	     (from enum cifisp_awb_mode_type)
> >> + * @max_y: only pixels values < max_y contribute to awb measurement, set to 0
> >> + *	   to disable this feature
> >> + * @min_y: only pixels values > min_y contribute to awb measurement
> >> + * @max_csum: Chrominance sum maximum value, only consider pixels with Cb+Cr,
> >> + *	      smaller than threshold for awb measurements
> >> + * @min_c: Chrominance minimum value, only consider pixels with Cb/Cr
> >> + *	   each greater than threshold value for awb measurements
> >> + * @frames: number of frames - 1 used for mean value calculation
> >> + *	    (ucFrames=0 means 1 Frame)
> >> + * @awb_ref_cr: reference Cr value for AWB regulation, target for AWB
> >> + * @awb_ref_cb: reference Cb value for AWB regulation, target for AWB
> >> + */
> >> +struct cifisp_awb_meas_config {
> >> +	/*
> >> +	 * Note: currently the h and v offsets are mapped to grid offsets
> >> +	 */
> >> +	struct cifisp_window awb_wnd;
> >> +	__u32 awb_mode;
> >> +	__u8 max_y;
> >> +	__u8 min_y;
> >> +	__u8 max_csum;
> >> +	__u8 min_c;
> >> +	__u8 frames;
> >> +	__u8 awb_ref_cr;
> >> +	__u8 awb_ref_cb;
> >> +	__u8 enable_ymax_cmp;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_awb_gain_config - Configuration used by auto white balance gain
> >> + *
> >> + * out_data_x = ( AWB_GEAIN_X * in_data + 128) >> 8
> >> + */
> >> +struct cifisp_awb_gain_config {
> >> +	__u16 gain_red;
> >> +	__u16 gain_green_r;
> >> +	__u16 gain_blue;
> >> +	__u16 gain_green_b;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_flt_config - Configuration used by ISP filtering
> >> + *
> >> + * @mode: ISP_FILT_MODE register fields (from enum cifisp_flt_mode)
> >> + * @grn_stage1: ISP_FILT_MODE register fields
> >> + * @chr_h_mode: ISP_FILT_MODE register fields
> >> + * @chr_v_mode: ISP_FILT_MODE register fields
> >> + *
> >> + * refer to REF_01 for details.
> >> + */
> >> +
> >> +struct cifisp_flt_config {
> >> +	__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;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_bdm_config - Configuration used by Bayer DeMosaic
> >> + *
> >> + * @demosaic_th: threshod for bayer demosaicing texture detection
> >> + */
> >> +struct cifisp_bdm_config {
> >> +	__u8 demosaic_th;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_ctk_config - Configuration used by Cross Talk correction
> >> + *
> >> + * @coeff: color correction matrix
> >> + * @ct_offset_b: offset for the crosstalk correction matrix
> >> + */
> >> +struct cifisp_ctk_config {
> >> +	__u16 coeff0;
> >> +	__u16 coeff1;
> >> +	__u16 coeff2;
> >> +	__u16 coeff3;
> >> +	__u16 coeff4;
> >> +	__u16 coeff5;
> >> +	__u16 coeff6;
> >> +	__u16 coeff7;
> >> +	__u16 coeff8;
> >> +	__u16 ct_offset_r;
> >> +	__u16 ct_offset_g;
> >> +	__u16 ct_offset_b;
> >> +} __attribute__ ((packed));
> >> +
> >> +enum cifisp_goc_mode {
> >> +	CIFISP_GOC_MODE_LOGARITHMIC,
> >> +	CIFISP_GOC_MODE_EQUIDISTANT
> >> +};
> >> +
> >> +/**
> >> + * struct cifisp_goc_config - Configuration used by Gamma Out correction
> >> + *
> >> + * @mode: goc mode (from enum cifisp_goc_mode)
> >> + * @gamma_y: gamma out curve y-axis for all color components
> >> + */
> >> +struct cifisp_goc_config {
> >> +	__u32 mode;
> >> +	__u16 gamma_y[CIFISP_GAMMA_OUT_MAX_SAMPLES];
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_hst_config - Configuration used by Histogram
> >> + *
> >> + * @mode: histogram mode (from enum cifisp_histogram_mode)
> >> + * @histogram_predivider: process every stepsize pixel, all other pixels are
> >> + *			  skipped
> >> + * @meas_window: coordinates of the measure window
> >> + * @hist_weight: weighting factor for sub-windows
> >> + */
> >> +struct cifisp_hst_config {
> >> +	__u32 mode;
> >> +	__u8 histogram_predivider;
> >> +	struct cifisp_window meas_window;
> >> +	__u8 hist_weight[CIFISP_HISTOGRAM_WEIGHT_GRIDS_SIZE];
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_aec_config - Configuration used by Auto Exposure Control
> >> + *
> >> + * @mode: Exposure measure mode (from enum cifisp_exp_meas_mode)
> >> + * @autostop: stop mode (from enum cifisp_exp_ctrl_autostop)
> >> + * @meas_window: coordinates of the measure window
> >> + */
> >> +struct cifisp_aec_config {
> >> +	__u32 mode;
> >> +	__u32 autostop;
> >> +	struct cifisp_window meas_window;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_afc_config - Configuration used by Auto Focus Control
> >> + *
> >> + * @num_afm_win: max CIFISP_AFM_MAX_WINDOWS
> >> + * @afm_win: coordinates of the meas window
> >> + * @thres: threshold used for minimizing the influence of noise
> >> + * @var_shift: the number of bits for the shift operation at the end of the
> >> + *	       calculation chain.
> >> + */
> >> +struct cifisp_afc_config {
> >> +	__u8 num_afm_win;
> >> +	struct cifisp_window afm_win[CIFISP_AFM_MAX_WINDOWS];
> >> +	__u32 thres;
> >> +	__u32 var_shift;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * enum cifisp_dpf_gain_usage - dpf gain usage
> >> + * @CIFISP_DPF_GAIN_USAGE_DISABLED: don't use any gains in preprocessing stage
> >> + * @CIFISP_DPF_GAIN_USAGE_NF_GAINS: use only the noise function gains from
> >> + *				    registers DPF_NF_GAIN_R, ...
> >> + * @CIFISP_DPF_GAIN_USAGE_LSC_GAINS:  use only the gains from LSC module
> >> + * @CIFISP_DPF_GAIN_USAGE_NF_LSC_GAINS: use the noise function gains and the
> >> + *					gains from LSC module
> >> + * @CIFISP_DPF_GAIN_USAGE_AWB_GAINS: use only the gains from AWB module
> >> + * @CIFISP_DPF_GAIN_USAGE_AWB_LSC_GAINS: use the gains from AWB and LSC module
> >> + * @CIFISP_DPF_GAIN_USAGE_MAX: upper border (only for an internal evaluation)
> >> + */
> >> +enum cifisp_dpf_gain_usage {
> >> +	CIFISP_DPF_GAIN_USAGE_DISABLED,
> >> +	CIFISP_DPF_GAIN_USAGE_NF_GAINS,
> >> +	CIFISP_DPF_GAIN_USAGE_LSC_GAINS,
> >> +	CIFISP_DPF_GAIN_USAGE_NF_LSC_GAINS,
> >> +	CIFISP_DPF_GAIN_USAGE_AWB_GAINS,
> >> +	CIFISP_DPF_GAIN_USAGE_AWB_LSC_GAINS,
> >> +	CIFISP_DPF_GAIN_USAGE_MAX
> >> +};
> >> +
> >> +/**
> >> + * enum cifisp_dpf_gain_usage - dpf gain usage
> > 
> > This should be cifisp_dpf_rb_filtersize and the description should be
> > updated.
> > 
> >> + * @CIFISP_DPF_RB_FILTERSIZE_13x9: red and blue filter kernel size 13x9
> >> + *				   (means 7x5 active pixel)
> >> + * @CIFISP_DPF_RB_FILTERSIZE_9x9: red and blue filter kernel size 9x9
> >> + *				   (means 5x5 active pixel)
> >> + */
> >> +enum cifisp_dpf_rb_filtersize {
> >> +	CIFISP_DPF_RB_FILTERSIZE_13x9,
> >> +	CIFISP_DPF_RB_FILTERSIZE_9x9,
> >> +};
> >> +
> >> +/**
> >> + * enum cifisp_dpf_nll_scale_mode - dpf noise level scale mode
> >> + * @CIFISP_NLL_SCALE_LINEAR: use a linear scaling
> >> + * @CIFISP_NLL_SCALE_LOGARITHMIC: use a logarithmic scaling
> >> + */
> >> +enum cifisp_dpf_nll_scale_mode {
> >> +	CIFISP_NLL_SCALE_LINEAR,
> >> +	CIFISP_NLL_SCALE_LOGARITHMIC,
> >> +};
> >> +
> >> +/**
> >> + * struct cifisp_dpf_nll - Noise level lookup
> >> + *
> >> + * @coeff: Noise level Lookup coefficient
> >> + * @scale_mode: dpf noise level scale mode (from enum cifisp_dpf_nll_scale_mode)
> >> + */
> >> +struct cifisp_dpf_nll {
> >> +	__u16 coeff[CIFISP_DPF_MAX_NLF_COEFFS];
> >> +	__u32 scale_mode;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_dpf_rb_flt - Red blue filter config
> >> + *
> >> + * @fltsize: The filter size for the red and blue pixels
> >> + *	     (from enum cifisp_dpf_rb_filtersize)
> >> + * @spatial_coeff: Spatial weights
> >> + * @r_enable: enable filter processing for red pixels
> >> + * @b_enable: enable filter processing for blue pixels
> >> + */
> >> +struct cifisp_dpf_rb_flt {
> >> +	__u32 fltsize;
> >> +	__u8 spatial_coeff[CIFISP_DPF_MAX_SPATIAL_COEFFS];
> >> +	__u8 r_enable;
> >> +	__u8 b_enable;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_dpf_g_flt - Green filter Configuration
> >> + *
> >> + * @spatial_coeff: Spatial weights
> >> + * @gr_enable: enable filter processing for green pixels in green/red lines
> >> + * @gb_enable: enable filter processing for green pixels in green/blue lines
> >> + */
> >> +struct cifisp_dpf_g_flt {
> >> +	__u8 spatial_coeff[CIFISP_DPF_MAX_SPATIAL_COEFFS];
> >> +	__u8 gr_enable;
> >> +	__u8 gb_enable;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_dpf_gain - Noise function Configuration
> >> + *
> >> + * @mode: dpf gain usage  (from enum cifisp_dpf_gain_usage)
> >> + * @nf_r_gain: Noise function Gain that replaces the AWB gain for red pixels
> >> + * @nf_b_gain: Noise function Gain that replaces the AWB gain for blue pixels
> >> + * @nf_gr_gain: Noise function Gain that replaces the AWB gain
> >> + *		for green pixels in a red line
> >> + * @nf_gb_gain: Noise function Gain that replaces the AWB gain
> >> + *		for green pixels in a blue line
> >> + */
> >> +struct cifisp_dpf_gain {
> >> +	__u32 mode;
> >> +	__u16 nf_r_gain;
> >> +	__u16 nf_b_gain;
> >> +	__u16 nf_gr_gain;
> >> +	__u16 nf_gb_gain;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_dpf_config - Configuration used by De-noising pre-filter
> >> + *
> >> + * @gain: noise function gain
> >> + * @g_flt: green filter config
> >> + * @rb_flt: red blue filter config
> >> + * @nll: noise level lookup
> >> + */
> >> +struct cifisp_dpf_config {
> >> +	struct cifisp_dpf_gain gain;
> >> +	struct cifisp_dpf_g_flt g_flt;
> >> +	struct cifisp_dpf_rb_flt rb_flt;
> >> +	struct cifisp_dpf_nll nll;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_dpf_strength_config - strength of the filter
> >> + *
> >> + * @r: filter strength of the RED filter
> >> + * @g: filter strength of the GREEN filter
> >> + * @b: filter strength of the BLUE filter
> >> + */
> >> +struct cifisp_dpf_strength_config {
> >> +	__u8 r;
> >> +	__u8 g;
> >> +	__u8 b;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_isp_other_cfg - Parameters for some blocks in rockchip isp1
> >> + *
> >> + * @dpcc_config: Defect Pixel Cluster Correction config
> >> + * @bls_config: Black Level Subtraction config
> >> + * @sdg_config: sensor degamma config
> >> + * @lsc_config: Lens Shade config
> >> + * @awb_gain_config: Auto White balance gain config
> >> + * @flt_config: filter config
> >> + * @bdm_config: demosaic config
> >> + * @ctk_config: cross talk config
> >> + * @goc_config: gamma out config
> >> + * @bls_config: black level subtraction config
> >> + * @dpf_config: De-noising pre-filter config
> >> + * @dpf_strength_config: dpf strength config
> >> + * @cproc_config: color process config
> >> + * @ie_config: image effects config
> >> + */
> >> +struct cifisp_isp_other_cfg {
> >> +	struct cifisp_dpcc_config dpcc_config;
> >> +	struct cifisp_bls_config bls_config;
> >> +	struct cifisp_sdg_config sdg_config;
> >> +	struct cifisp_lsc_config lsc_config;
> >> +	struct cifisp_awb_gain_config awb_gain_config;
> >> +	struct cifisp_flt_config flt_config;
> >> +	struct cifisp_bdm_config bdm_config;
> >> +	struct cifisp_ctk_config ctk_config;
> >> +	struct cifisp_goc_config goc_config;
> >> +	struct cifisp_dpf_config dpf_config;
> >> +	struct cifisp_dpf_strength_config dpf_strength_config;
> >> +	struct cifisp_cproc_config cproc_config;
> >> +	struct cifisp_ie_config ie_config;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_isp_meas_cfg - Rockchip ISP1 Measure Parameters
> >> + *
> >> + * @awb_meas_config: auto white balance config
> >> + * @hst_config: histogram config
> >> + * @aec_config: auto exposure config
> >> + * @afc_config: auto focus config
> >> + */
> >> +struct cifisp_isp_meas_cfg {
> >> +	struct cifisp_awb_meas_config awb_meas_config;
> >> +	struct cifisp_hst_config hst_config;
> >> +	struct cifisp_aec_config aec_config;
> >> +	struct cifisp_afc_config afc_config;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct rkisp1_isp_params_cfg - Rockchip ISP1 Input Parameters Meta Data
> >> + *
> >> + * @module_en_update: mask the enable bits of which module should be updated
> >> + * @module_ens: mask the enable value of each module, only update the module
> >> + *		which correspond bit was set in module_en_update
> >> + * @module_cfg_update: mask the config bits of which module should be updated
> >> + * @meas: measurement config
> >> + * @others: other config
> >> + */
> >> +struct rkisp1_isp_params_cfg {
> >> +	__u32 module_en_update;
> >> +	__u32 module_ens;
> >> +	__u32 module_cfg_update;
> >> +
> >> +	struct cifisp_isp_meas_cfg meas;
> >> +	struct cifisp_isp_other_cfg others;
> >> +} __attribute__ ((packed));
> > 
> > As far as I understand most (all ?) of the structures defined above are
> > not used directly by the hardware, is that right ? In that case, do they
> > need to be packed ?
> 
> Hi,
> I guess the 'packed' is a mean to serialize the struct since it is sent to userspace as
> a buffer.

That's no different than using structures in an ioctl, is it ?

> I see that the ipu3 also uses 'packed' for the metadata formats.

For data structures that are directly consumed (or produced) by the
hardware (or firmware) we often need the packed attribute. The
structures used to describe the stastics produced by the rkisp1 need to
be packed.

For structures that are consumed by software only, it's not necessarily
a requirement. Packing has drawbacks, for instance it means we have to
handle unaligned accesses explicitly on platforms that don't support
them.

> If we don't have the 'packed' it means we relay on that
> the userspace uses the same padding as the kernel for the struct.
> Is it ok to assume that?

Isn't that dictated by the C ABI ? We of course need to ensure that
there will be no 32/64-bit compatibility issues.

> > Some of the structures have a _cfg suffix, while others have a _config
> > suffix. Should we standardise on one of the two ?
> > 
> >> +
> >> +/*---------- PART2: Measurement Statistics ------------*/
> >> +
> >> +/**
> >> + * struct cifisp_bls_meas_val - AWB measured values
> > 
> > This should be cifisp_awb_meas
> > 
> >> + *
> >> + * @cnt: White pixel count, number of "white pixels" found during laster
> > 
> > s/laster/last/ ?
> > 
> >> + *	 measurement
> >> + * @mean_y_or_g: Mean value of Y within window and frames,
> >> + *		 Green if RGB is selected.
> >> + * @mean_cb_or_b: Mean value of Cb within window and frames,
> >> + *		  Blue if RGB is selected.
> >> + * @mean_cr_or_r: Mean value of Cr within window and frames,
> >> + *		  Red if RGB is selected.
> >> + */
> >> +struct cifisp_awb_meas {
> >> +	__u32 cnt;
> >> +	__u8 mean_y_or_g;
> >> +	__u8 mean_cb_or_b;
> >> +	__u8 mean_cr_or_r;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_awb_stat - statistics automatic white balance data
> >> + *
> >> + * @awb_mean: Mean measured data
> >> + */
> >> +struct cifisp_awb_stat {
> >> +	struct cifisp_awb_meas awb_mean[CIFISP_AWB_MAX_GRID];
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_bls_meas_val - BLS measured values
> >> + *
> >> + * @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 cifisp_bls_meas_val {
> >> +	__u16 meas_r;
> >> +	__u16 meas_gr;
> >> +	__u16 meas_gb;
> >> +	__u16 meas_b;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_ae_stat - statistics auto exposure data
> >> + *
> >> + * @exp_mean: Mean luminance value of block xx
> >> + * @bls_val:  BLS measured values
> >> + *
> >> + * Image is divided into 5x5 blocks.
> >> + */
> >> +struct cifisp_ae_stat {
> >> +	__u8 exp_mean[CIFISP_AE_MEAN_MAX];
> >> +	struct cifisp_bls_meas_val bls_val;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_af_meas_val - AF measured values
> >> + *
> >> + * @sum: sharpness, refer to REF_01 for definition
> >> + * @lum: luminance, refer to REF_01 for definition
> > 
> > That will be lovely to use without documentation...
> > 
> >> + */
> >> +struct cifisp_af_meas_val {
> >> +	__u32 sum;
> >> +	__u32 lum;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_af_stat - statistics auto focus data
> >> + *
> >> + * @window: AF measured value of window x
> >> + *
> >> + * The module measures the sharpness in 3 windows of selectable size via
> >> + * register settings(ISP_AFM_*_A/B/C)
> >> + */
> >> +struct cifisp_af_stat {
> >> +	struct cifisp_af_meas_val window[CIFISP_AFM_MAX_WINDOWS];
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct cifisp_hist_stat - statistics histogram data
> >> + *
> >> + * @hist_bins: measured bin counters
> >> + *
> >> + * Measurement window divided into 25 sub-windows, set
> >> + * with ISP_HIST_XXX
> > 
> > What is ISP_HIST_XXX ?
> > 
> >> + */
> >> +struct cifisp_hist_stat {
> >> +	__u16 hist_bins[CIFISP_HIST_BIN_N_MAX];
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct rkisp1_stat_buffer - Rockchip ISP1 Statistics Data
> >> + *
> >> + * @cifisp_awb_stat: statistics data for automatic white balance
> >> + * @cifisp_ae_stat: statistics data for auto exposure
> >> + * @cifisp_af_stat: statistics data for auto focus
> >> + * @cifisp_hist_stat: statistics histogram data
> >> + */
> >> +struct cifisp_stat {
> >> +	struct cifisp_awb_stat awb;
> >> +	struct cifisp_ae_stat ae;
> >> +	struct cifisp_af_stat af;
> >> +	struct cifisp_hist_stat hist;
> >> +} __attribute__ ((packed));
> >> +
> >> +/**
> >> + * struct rkisp1_stat_buffer - Rockchip ISP1 Statistics Meta Data
> >> + *
> >> + * @meas_type: measurement types (CIFISP_STAT_ definitions)
> >> + * @frame_id: frame ID for sync
> >> + * @params: statistics data
> >> + */
> >> +struct rkisp1_stat_buffer {
> >> +	__u32 meas_type;
> >> +	__u32 frame_id;
> >> +	struct cifisp_stat params;
> >> +} __attribute__ ((packed));
> >> +
> >> +#endif /* _UAPI_RKISP1_CONFIG_H */

-- 
Regards,

Laurent Pinchart