Hi, Tomasz, Thank you for the time spent to review this long file.
> -----Original Message----- > From: Tomasz Figa [mailto:tf...@chromium.org] > Sent: Sunday, June 17, 2018 11:09 PM > To: Zhi, Yong <yong....@intel.com> > Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus > <sakari.ai...@linux.intel.com>; Mani, Rajmohan > <rajmohan.m...@intel.com>; Toivonen, Tuukka > <tuukka.toivo...@intel.com>; Hu, Jerry W <jerry.w...@intel.com>; Zheng, > Jian Xu <jian.xu.zh...@intel.com> > Subject: Re: [PATCH v6 02/12] intel-ipu3: Add user space API definitions > > Hi Yong, > > On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong....@intel.com> wrote: > > > > Define the structures and macros to be used by public. > > > > Signed-off-by: Yong Zhi <yong....@intel.com> > > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com> > > --- > > include/uapi/linux/intel-ipu3.h | 1403 > > +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 1403 insertions(+) > > create mode 100644 include/uapi/linux/intel-ipu3.h > > > > Since we'll need 1 more resend with latest fixes from Chromium tree and > recently posted documentation anyway, let me do some more nitpicking > inline, so we can end up with slightly cleaner code. :) > > > diff --git a/include/uapi/linux/intel-ipu3.h > > b/include/uapi/linux/intel-ipu3.h new file mode 100644 index > > 000000000000..694ef0c8d7a7 > > --- /dev/null > > +++ b/include/uapi/linux/intel-ipu3.h > > @@ -0,0 +1,1403 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* Copyright (C) 2018 Intel Corporation */ > > + > > +#ifndef __IPU3_UAPI_H > > +#define __IPU3_UAPI_H > > + > > +#include <linux/types.h> > > + > > +#define IPU3_UAPI_ISP_VEC_ELEMS 64 > > + > > +#define IMGU_ABI_PAD __aligned(IPU3_UAPI_ISP_WORD_BYTES) > > This seems unused. Ack, will remove. > > > +#define IPU3_ALIGN > __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES))) > > Any reason to mix both __aligned() and __attribute__((aligned()))? > > > + > > +#define IPU3_UAPI_ISP_WORD_BYTES 32 > > It would make sense to define this above IPU3_ALIGN(), which references it. > Sure. > > +#define IPU3_UAPI_MAX_STRIPES 2 > > + > > +/******************* ipu3_uapi_stats_3a *******************/ > > + > > +#define IPU3_UAPI_MAX_BUBBLE_SIZE 10 > > + > > +#define IPU3_UAPI_AE_COLORS 4 > > +#define IPU3_UAPI_AE_BINS 256 > > + > > +#define IPU3_UAPI_AWB_MD_ITEM_SIZE 8 > > +#define IPU3_UAPI_AWB_MAX_SETS 60 > > +#define IPU3_UAPI_AWB_SET_SIZE 0x500 > > Why not just decimal 1280? Ok, will change above and similar places to decimal expression. > > > +#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \ > > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \ > > + IPU3_UAPI_AWB_MD_ITEM_SIZE) > > +#define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \ > > + (IPU3_UAPI_AWB_MAX_SETS * \ > > + (IPU3_UAPI_AWB_SET_SIZE + > IPU3_UAPI_AWB_SPARE_FOR_BUBBLES)) > > + > > +#define IPU3_UAPI_AF_MAX_SETS 24 > > +#define IPU3_UAPI_AF_MD_ITEM_SIZE 4 > > +#define IPU3_UAPI_AF_SPARE_FOR_BUBBLES \ > > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \ > > + IPU3_UAPI_AF_MD_ITEM_SIZE) > > +#define IPU3_UAPI_AF_Y_TABLE_SET_SIZE 0x80 > > Why not just decimal 128? Ack. > > > +#define IPU3_UAPI_AF_Y_TABLE_MAX_SIZE \ > > + (IPU3_UAPI_AF_MAX_SETS * \ > > + (IPU3_UAPI_AF_Y_TABLE_SET_SIZE + > IPU3_UAPI_AF_SPARE_FOR_BUBBLES) * \ > > + IPU3_UAPI_MAX_STRIPES) > > + > > +#define IPU3_UAPI_AWB_FR_MAX_SETS 24 > > +#define IPU3_UAPI_AWB_FR_MD_ITEM_SIZE 8 > > +#define IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE 0x100 > > Why not just decimal 256? Ack. > > > +#define IPU3_UAPI_AWB_FR_SPARE_FOR_BUBBLES \ > > + (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \ > > + IPU3_UAPI_AWB_FR_MD_ITEM_SIZE) #define > > +IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE \ > > + (IPU3_UAPI_AWB_FR_MAX_SETS * \ > > + (IPU3_UAPI_AWB_FR_BAYER_TBL_SIZE + \ > > + IPU3_UAPI_AWB_FR_SPARE_FOR_BUBBLES) * > IPU3_UAPI_MAX_STRIPES) > [snip] > > +struct ipu3_uapi_af_filter_config { > > + struct { > > + __u8 a1; > > + __u8 a2; > > + __u8 a3; > > + __u8 a4; > > + } y1_coeff_0; > > + struct { > > + __u8 a5; > > + __u8 a6; > > + __u8 a7; > > + __u8 a8; > > + } y1_coeff_1; > > + struct { > > + __u8 a9; > > + __u8 a10; > > + __u8 a11; > > + __u8 a12; > > + } y1_coeff_2; > > Why these aren't just __u8 y1_coeff[12]? Yes, we can combine them together. > > > + > > + __u32 y1_sign_vec; > > + > > + struct { > > + __u8 a1; > > + __u8 a2; > > + __u8 a3; > > + __u8 a4; > > + } y2_coeff_0; > > + struct { > > + __u8 a5; > > + __u8 a6; > > + __u8 a7; > > + __u8 a8; > > + } y2_coeff_1; > > + struct { > > + __u8 a9; > > + __u8 a10; > > + __u8 a11; > > + __u8 a12; > > + } y2_coeff_2; > > Ditto. > Ack. > > + > > + __u32 y2_sign_vec; > > + > > + struct { > > + __u8 y_gen_rate_gr; /* 6 bits */ > > + __u8 y_gen_rate_r; > > + __u8 y_gen_rate_b; > > + __u8 y_gen_rate_gb; > > + } y_calc; > > Why not just call the struct "y_gen_rate" and then fields "gr", "r", "b", > "gb"? > It would make any code referencing them much shorter. > Good idea, thanks!! > > + > > + struct { > > + __u32 __reserved0:8; > > + __u32 y1_nf:4; > > + __u32 __reserved1:4; > > + __u32 y2_nf:4; > > + __u32 __reserved2:12; > > + } nf; > > Similarly here, is there any need to repeat "nf" inside the struct? > Ack. > > +} __packed; > > + > > +struct ipu3_uapi_af_meta_data { > > + __u8 y_table[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE] IPU3_ALIGN; } > > +__packed; > [snip] > > +/******************* ipu3_uapi_acc_param *******************/ > > + > > +#define IPU3_UAPI_BNR_LUT_SIZE 32 > > + > > +/* number of elements in gamma correction LUT */ > > +#define IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES 256 > > + > > +#define IPU3_UAPI_SHD_MAX_CELLS_PER_SET 146 > > +/* largest grid is 73x56 */ > > The relation between the comment above and the values defined here is > not clear to me. > Will add some explanation to the macros in next update. > > +#define IPU3_UAPI_SHD_MAX_CFG_SETS 28 > [snip] > > +struct ipu3_uapi_bnr_static_config { > > + struct ipu3_uapi_bnr_static_config_wb_gains_config wb_gains; > > + struct ipu3_uapi_bnr_static_config_wb_gains_thr_config > wb_gains_thr; > > + struct ipu3_uapi_bnr_static_config_thr_coeffs_config thr_coeffs; > > + struct ipu3_uapi_bnr_static_config_thr_ctrl_shd_config thr_ctrl_shd; > > + struct ipu3_uapi_bnr_static_config_opt_center_config opt_center; > > + struct ipu3_uapi_bnr_static_config_lut_config lut; > > + struct ipu3_uapi_bnr_static_config_bp_ctrl_config bp_ctrl; > > + struct ipu3_uapi_bnr_static_config_dn_detect_ctrl_config > dn_detect_ctrl; > > + __u32 column_size; /* 0x44 */ > > What's the meaning of this number? > Will do some cleanup to keep the meaningful only. > > + struct ipu3_uapi_bnr_static_config_opt_center_sqr_config > > +opt_center_sqr; } __packed; > > + > > +struct ipu3_uapi_bnr_static_config_green_disparity { > > + __u32 gd_red:6; > > + __u32 __reserved0:2; > > + __u32 gd_green:6; > > + __u32 __reserved1:2; > > + __u32 gd_blue:6; > > + __u32 __reserved2:10; > > + __u32 gd_black:14; > > + __u32 __reserved3:2; > > + __u32 gd_shading:7; > > + __u32 __reserved4:1; > > + __u32 gd_support:2; > > + __u32 __reserved5:1; > > + __u32 gd_clip:1; /* central weights > > variables */ > > + __u32 gd_central_weight:4; > > +} __packed; > > + > > +struct ipu3_uapi_dm_config { > > + /* DWORD0 */ > > Is there any meaning behind this comment? > Will remove all non-relevant info in next update. > > + __u32 dm_en:1; > > + __u32 ch_ar_en:1; > > + __u32 fcc_en:1; > > + __u32 __reserved0:13; > > + __u32 frame_width:16; > > + > > + /* DWORD1 */ > > Ditto. > > > + __u32 gamma_sc:5; > > + __u32 __reserved1:3; > > + __u32 lc_ctrl:5; > > + __u32 __reserved2:3; > > + __u32 cr_param1:5; > > + __u32 __reserved3:3; > > + __u32 cr_param2:5; > > + __u32 __reserved4:3; > > + > > + /* DWORD2 */ > > Ditto. > > > + __u32 coring_param:5; > > + __u32 __reserved5:27; > > +} __packed; > [snip] > > +/* Bayer shading correction */ > > + > > +struct ipu3_uapi_shd_grid_config { > > + /* reg 0 */ > > What's the meaning behind this comment? > Will replace them with more relevant info. > > + __u8 width; > > + __u8 height; > > + __u8 block_width_log2:3; > > + __u8 __reserved0:1; > > + __u8 block_height_log2:3; > > + __u8 __reserved1:1; > > + __u8 grid_height_per_slice; > > + /* reg 1 */ > > Ditto. > > > + __s16 x_start; /* 13 bits */ > > + __s16 y_start; > > +} __packed; > > + > > +struct ipu3_uapi_shd_general_config { > > + __u32 init_set_vrt_offst_ul:8; > > + __u32 shd_enable:1; > > + /* aka 'gf' */ > > What's the meaning of this comment? gain_factor, will remove the un-needed history notes/marks in next update. > > > + __u32 gain_factor:2; > > + __u32 __reserved:21; > > +} __packed; > > + > > +struct ipu3_uapi_shd_black_level_config { > > + __s16 bl_r; /* 12 bits */ > > + __s16 bl_gr; > > +#define IPU3_UAPI_SHD_BLGR_NF_SHIFT 13 /* Normalization shift > aka nf */ > > +#define IPU3_UAPI_SHD_BLGR_NF_MASK 0x7 > > + __s16 bl_gb; /* 12 bits */ > > + __s16 bl_b; > > +} __packed; > > + > > +struct ipu3_uapi_shd_config_static { > > + /* B0: Fixed order: one transfer to GAC */ > > It's not clear to me what this comment means. > Ack, will clean up/update places like this. > > + struct ipu3_uapi_shd_grid_config grid; > > + struct ipu3_uapi_shd_general_config general; > > + struct ipu3_uapi_shd_black_level_config black_level; } > > +__packed; > > + > > +struct ipu3_uapi_shd_lut { > > + struct { > > + struct { > > + __u16 r; > > + __u16 gr; > > + } r_and_gr[IPU3_UAPI_SHD_MAX_CELLS_PER_SET]; > > + __u8 __reserved1[24]; > > + struct { > > + __u16 gb; > > + __u16 b; > > + } gb_and_b[IPU3_UAPI_SHD_MAX_CELLS_PER_SET]; > > + __u8 __reserved2[24]; > > + } sets[IPU3_UAPI_SHD_MAX_CFG_SETS]; } __packed; > > + > > +struct ipu3_uapi_shd_config { > > + struct ipu3_uapi_shd_config_static shd IPU3_ALIGN; > > + struct ipu3_uapi_shd_lut shd_lut IPU3_ALIGN; } __packed; > > + > > +/* Image Enhancement Filter and Denoise */ > > + > > +struct ipu3_uapi_iefd_cux2 { > > + __u32 x0:9; > > + __u32 x1:9; > > + __u32 a01:9; > > + __u32 b01:5; /* NOTE: hardcoded to zero > > */ > > No need for such long spacing to the comment. > Ack. > > +} __packed; > [snip] > > +struct ipu3_uapi_unsharp_coef0 { > > + __u32 c00:9; /* Coeff11 */ > > + __u32 c01:9; /* Coeff12 */ > > + __u32 c02:9; /* Coeff13 */ > > No need for such wide spacing. > Ack. > > + __u32 __reserved:5; > > +} __packed; > > + > > +struct ipu3_uapi_unsharp_coef1 { > > + __u32 c11:9; /* Coeff22 */ > > + __u32 c12:9; /* Coeff23 */ > > + __u32 c22:9; /* Coeff33 */ > > Ditto. > Ack. > > + __u32 __reserved:5; > > +} __packed; > [snip] > > +struct ipu3_uapi_bds_hor_ctrl1 { > > + __u32 hor_crop_start:13; > > + __u32 __reserved0:3; > > + __u32 hor_crop_end:13; > > + __u32 __reserved1:1; > > + __u32 hor_crop_en:1; > > + __u32 __reserved2:1; > > +} __packed; > > The struct name includes "hor" already, so no need to include it in fields > names. > Ack. > > + > > +struct ipu3_uapi_bds_hor_ctrl2 { > > + __u32 input_frame_height:13; > > + __u32 __reserved0:19; > > +} __packed; > > + > > +struct ipu3_uapi_bds_hor { > > + struct ipu3_uapi_bds_hor_ctrl0 hor_ctrl0; > > + struct ipu3_uapi_bds_ptrn_arr hor_ptrn_arr; > > + struct ipu3_uapi_bds_phase_arr hor_phase_arr; > > + struct ipu3_uapi_bds_hor_ctrl1 hor_ctrl1; > > + struct ipu3_uapi_bds_hor_ctrl2 hor_ctrl2; } __packed; > > Same here. Code that wants to access an example field needs to do it like > "[...]bds.hor.hor_ctrl1.hor_crop_en". Without repeating "hor" at every level, > it could be just "[...]bds.hor.ctrl1.crop_en". > Ack, will remove the repeated text in such cases. > > + > > +struct ipu3_uapi_bds_ver_ctrl0 { > > + __u32 sample_patrn_length:9; > > + __u32 __reserved0:3; > > + __u32 ver_ds_en:1; > > Is there a need to include "ver" in the name? Ack. > > > + __u32 min_clip_val:1; > > + __u32 max_clip_val:2; > > + __u32 __reserved1:16; > > +} __packed; > > + > > +struct ipu3_uapi_bds_ver_ctrl1 { > > + __u32 out_frame_width:13; > > + __u32 __reserved0:3; > > + __u32 out_frame_height:13; > > + __u32 __reserved1:3; > > +} __packed; > > + > > +struct ipu3_uapi_bds_ver { > > + struct ipu3_uapi_bds_ver_ctrl0 ver_ctrl0; > > + struct ipu3_uapi_bds_ptrn_arr ver_ptrn_arr; > > + struct ipu3_uapi_bds_phase_arr ver_phase_arr; > > + struct ipu3_uapi_bds_ver_ctrl1 ver_ctrl1; > > Is there a need to include "ver" in field names? > Ack. > > + > > Unnecessary blank line. > Ack. > > +} __packed; > > + > [snip] > > +struct ipu3_uapi_anr_beta { > > + __u16 beta_gr; /* 11 bits */ > > + __u16 beta_r; > > + __u16 beta_b; > > + __u16 beta_gb; > > +} __packed; > > Is there a need to include "beta" in field names? > Ack. > [snip] > > +struct ipu3_uapi_isp_lin_vmem_params { > > + __s16 lin_lutlow_gr[IPU3_UAPI_LIN_LUT_SIZE]; > > + __s16 lin_lutlow_r[IPU3_UAPI_LIN_LUT_SIZE]; > > + __s16 lin_lutlow_b[IPU3_UAPI_LIN_LUT_SIZE]; > > + __s16 lin_lutlow_gb[IPU3_UAPI_LIN_LUT_SIZE]; > > + __s16 lin_lutdif_gr[IPU3_UAPI_LIN_LUT_SIZE]; > > + __s16 lin_lutdif_r[IPU3_UAPI_LIN_LUT_SIZE]; > > + __s16 lin_lutdif_b[IPU3_UAPI_LIN_LUT_SIZE]; > > + __s16 lin_lutdif_gb[IPU3_UAPI_LIN_LUT_SIZE]; > > Is there a need to include "lin" in field names? > Ack. > > +} __packed; > > + > > +/* Temporal Noise Reduction VMEM parameters */ > > + > > +#define IPU3_UAPI_ISP_TNR3_VMEM_LEN 9 > > + > > +struct ipu3_uapi_isp_tnr3_vmem_params { > > + __u16 slope[IPU3_UAPI_ISP_TNR3_VMEM_LEN]; > > + __u16 __reserved1[IPU3_UAPI_ISP_VEC_ELEMS > > + - > > +IPU3_UAPI_ISP_TNR3_VMEM_LEN]; > > Something wrong with indentation here. Maybe it could make sense to > define the length of padding as a macro, in addition to > IPU3_UAPI_ISP_TNR3_VMEM_LEN? Will think about it. > > > + __u16 sigma[IPU3_UAPI_ISP_TNR3_VMEM_LEN]; > > + __u16 __reserved2[IPU3_UAPI_ISP_VEC_ELEMS > > + - > > + IPU3_UAPI_ISP_TNR3_VMEM_LEN]; > > Ditto. > Ack, thanks!! > Best regards, > Tomasz
