Re: [PATCH v6 03/17] media: rkisp1: Add user space ABI definitions

[Tomasz Figa <tfiga@chromium.org>]

On Thu, Mar 8, 2018 at 6:48 PM Jacob Chen <jacob-chen@iotwrt.com> wrote:
[snip]
> +/**
> + * 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];

Looking at the code, we only need 12 bits of each, so perhaps it could make
sense to make those __u16? Also, the natural layout for these seems to be
two-dimensional, i.e. [CIFISP_LSC_NUM_SECTORS][CIFISP_LSC_NUM_SECTORS]. I
think it wouldn't be a problem to define it this way for UAPI too.

> +       __u16 config_width;
> +       __u16 config_height;

These 2 seem unused. Just making sure. If they are part of hardware LSC
configuration, then we should keep them.

[snip]
> +struct cifisp_awb_meas_config {
> +       /*
> +        * Note: currently the h and v offsets are mapped to grid offsets
> +        */

Perhaps should be part of the kerneldoc comment above? Also, I don't seem
to understand what this means.

> +       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

typo: AWB_GAIN?

> + */
> +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

Missing documentation for remaining fields.

> + *
> + * refer to REF_01 for details.
> + */
> +
> +struct cifisp_flt_config {
> +       __u32 mode;
> +       __u8 grn_stage1;
> +       __u8 chr_h_mode;
> +       __u8 chr_v_mode;

Maybe we could move u8 below u32 to optimize the alignment?

[snip]
> +/**
> + * 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;

Perhaps could swap the two above for better alignment?

> +       __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;

Perhaps could put afm_win[] and then num_afm_win here, for better alignment?

Best regards,
Tomasz