Hi Laurent On Wed, Jun 12, 2024 at 03:56:00PM GMT, Laurent Pinchart wrote: > On Wed, Jun 12, 2024 at 11:02:58AM +0100, Daniel Scally wrote: > > Hi Jacopo - thanks for the patchset > > > > On 05/06/2024 17:54, Jacopo Mondi wrote: > > > Add to the rkisp1-config.h header data types and documentation of > > > the extensible parameters format. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> > > > --- > > > include/uapi/linux/rkisp1-config.h | 482 +++++++++++++++++++++++++++++ > > > 1 file changed, 482 insertions(+) > > > > > > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h > > > index 6eeaf8bf2362..9c93e536f270 100644 > > > --- a/include/uapi/linux/rkisp1-config.h > > > +++ b/include/uapi/linux/rkisp1-config.h > > > @@ -996,4 +996,486 @@ struct rkisp1_stat_buffer { > > > struct rkisp1_cif_isp_stat params; > > > }; > > > > > > +/*---------- PART3: Extensible Configuration Parameters ------------*/ > > > + > > > +/** > > > + * enum rkisp1_ext_params_block_type - RkISP1 extensible params block type > > > + * > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS: Black level subtraction > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC: Defect pixel cluster correction > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG: Sensor de-gamma > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS: Auto white balance gains > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT: ISP filtering > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM: Bayer de-mosaic > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK: Cross-talk correction > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC: Gamma out correction > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF: De-noise pre-filter > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGHT: De-noise pre-filter strength > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC: Color processing > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_IE: Image effects > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC: Lens shading correction > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS: Auto white balance statistics > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS: Histogram statistics > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS: Auto exposure statistics > > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS: Auto-focus statistics > > > + */ > > > +enum rkisp1_ext_params_block_type { > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGHT, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_IE, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS, > > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL, > > > +}; > > > > Laurent suggested referencing the enum value in the comments for each > > block in his review of the C55 series, and I think that that's a good > > idea - can we do that? > > I would like that too. > > > Otherwise, looks good to me: > > > > Reviewed-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx> > > > > > + > > > +/** > > > + * enum rkisp1_ext_params_block_state - RkISP1 extensible parameter block enable > > > + * state flags > > > + * > > > + * @RKISP1_EXT_PARAMS_BLOCK_NO_CHANGE: Do not change the block power state > > I wouldn't call that "power" state, I don't think it's about power. How would you call it ? It's about enabling the block, should I use 'enable' ? > > > > + * @RKISP1_EXT_PARAMS_BLOCK_DISABLE: Disable the HW block > > > + * @RKISP1_EXT_PARAMS_BLOCK_ENABLE: Enable the HW block > > > + */ > > > +enum rkisp1_ext_params_block_state { > > > + RKISP1_EXT_PARAMS_BLOCK_NO_CHANGE, > > > + RKISP1_EXT_PARAMS_BLOCK_DISABLE, > > > + RKISP1_EXT_PARAMS_BLOCK_ENABLE, > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_block_header - RkISP1 extensible parameter block > > > + * header > > > + * > > > + * This structure represents the common part of all the ISP configuration > > > + * blocks. Each parameters block shall embed an instance of this structure type > > > + * as its first member, followed by the block-specific configuration data. The > > > + * driver inspects this common header to discern the block type and its size and > > > + * properly handle the block content by casting it to the correct block-specific > > > + * type. > > > + * > > > + * The @type field is one of the values enumerated by > > > + * :c:type:`rkisp1_ext_params_block_type` and specifies how the data should be > > > + * interpreted by the driver. The @size field specifies the size of the > > > + * parameters block and is used by the driver for validation purposes. > > > + * > > > + * The @state field specifies if the ISP block power state should be changed, > > > + * and, if it has to, if it has to be enabled to disabled. The possible > > > + * states are enumerated by :c:type:`rkisp1_ext_params_block_state`. > > > + * When userspace needs to configure and enable an ISP block it shall fully > > > + * populate the block configuration and the @state flag shall be set to > > > + * RKISP1_EXT_PARAMS_BLOCK_ENABLE. When userspace simply wants to disable the > > > + * ISP block the @state flag shall be set to RKISP1_EXT_PARAMS_BLOCK_DISABLE. > > You should document here what happens to the rest of the parameters in > the block in that case. I think they should be ignored by the driver, > and possibly set to 0 by userspace. I'll add "they get ignored" > > Another option when disabling a block would be to include the header > only, with the size field set to the header size, and not include the > rest of the block contents. It's a bit pointless to include data that > the kernel won't use. While that would be my preference from an API > point of view (at least until someone points out to use cases that would > benefit from a different option), I haven't checked what it would imply > from a userspace and kernelspace implementation point of view. I could > agree to keeping the data part of the block in for disabled blocks if > omitting it would result in implementation issues. > I'm not sure it will add issues, but it would make it more cumbersome for userspace maybe > > If > > > + * a new configuration of an ISP block should be applied but the power state > > > + * doesn't need to be changed, userspace shall fully populate the ISP block > > > + * configuration and the @state flag shall be set to > > > + * RKISP1_EXT_PARAMS_BLOCK_NO_CHANGE. > > I don't think we need RKISP1_EXT_PARAMS_BLOCK_NO_CHANGE for that. The > state can just be set to @RKISP1_EXT_PARAMS_BLOCK_ENABLE. > The existing code does rkisp1_isp_isr_xxx_config() { if (cfg_update & MODULE_XXX) rkisp1_xxx_config() if (en_update & MODULE_XXX) if (enable & MODULE_XXX) set_bit(ENABLE, MODULE_XXX) else clear_bit(ENABLE, MODULE_XXX) } As the enablement maks is separate from the configuration mask, userspace can in the previous version set (cfg_update |= MODULE_XXX) without setting the MODULE_XXX bit in en_update. Now, with the the introduction of this new format, the 'state' field is always present and I can't express with just two values the "reconfigure but not enable" case. Think about LSC, when a new table has to be applied we have to re-program it without changing the enable bit. I mean, re-asserting the bit is probably harmless, but isn't it safer avoid doing that ? > > > + * > > > + * Userspace is responsible for correctly populating the parameters block header > > > + * fields (@type, @state and @size) and correctly populate the block-specific > > > + * parameters. > > > + * > > > + * For example: > > > + * > > > + * .. code-block:: c > > > + * > > > + * void populate_bls(struct rkisp1_ext_params_block_header *block) { > > > + * struct rkisp1_ext_params_bls_config *bls = > > > + * (struct rkisp1_ext_params_bls_config *)block; > > > + * > > > + * block->header.type = RKISP1_EXT_PARAMS_BLOCK_ID_BLS; > > > + * block->header.state = RKISP1_EXT_PARAMS_BLOCK_ENABLE; > > > + * block->header.size = sizeof(struct rkisp1_ext_params_bls_config); > > > + * > > > + * bls->bls_config.enable_auto = 0; > > > + * bls->bls_config.fixed_val.r = blackLevelRed_; > > > + * bls->bls_config.fixed_val.gr = blackLevelGreenR_; > > > + * bls->bls_config.fixed_val.gb = blackLevelGreenB_; > > > + * bls->bls_config.fixed_val.b = blackLevelBlue_; > > > + * } > > > + * > > > + * @type: The parameters block type, see > > > + * :c:type:`rkisp1_ext_params_block_type` > > > + * @state: The block enable state flag, see > > > + * :c:type:`rkisp1_ext_params_block_state` > > > + * @size: Size (in bytes) of the parameters block, including this header > > > + */ > > > +struct rkisp1_ext_params_block_header { > > > + __u32 type; > > > + __u32 state; > > > + __u64 size; > > I think a 32-bit size would be move than enough. The header should > however be 64-bit aligned. That's why size is 64 bits. > > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_bls_config - RkISP1 extensible params BLS config > > > + * > > > + * RkISP1 extensible parameters Black Level Subtraction configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @bls_config: Black Level Subtraction configuration, see > > > + * :c:type:`rkisp1_cif_isp_bls_config` > > > + */ > > > +struct rkisp1_ext_params_bls_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_bls_config bls_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_dpcc_config - RkISP1 extensible params DPCC config > > > + * > > > + * RkISP1 extensible parameters Defective Pixel Cluster Correction configuration > > > + * block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @dpcc_config: Defective Pixel Cluster Correction configuration, see > > > + * :c:type:`rkisp1_cif_isp_dpcc_config` > > > + */ > > > +struct rkisp1_ext_params_dpcc_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_dpcc_config dpcc_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_sdg_config - RkISP1 extensible params SDG config > > > + * > > > + * RkISP1 extensible parameters Sensor Degamma configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @sdg_config: Sensor Degamma configuration, see > > > + * :c:type:`rkisp1_cif_isp_sdg_config` > > > + */ > > > +struct rkisp1_ext_params_sdg_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_sdg_config sdg_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_lsc_config - RkISP1 extensible params LSC config > > > + * > > > + * RkISP1 extensible parameters Lens Shading Correction configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @lsc_config: Lens Shading Correction configuration, see > > > + * :c:type:`rkisp1_cif_isp_lsc_config` > > > + */ > > > +struct rkisp1_ext_params_lsc_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_lsc_config lsc_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_awb_gain_config - RkISP1 extensible params AWB > > > + * gain config > > > + * > > > + * RkISP1 extensible parameters Auto-White Balance Gains configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @awb_config: Auto-White Balance Gains configuration, see > > > + * :c:type:`rkisp1_cif_isp_awb_gain_config` > > > + */ > > > +struct rkisp1_ext_params_awb_gain_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_awb_gain_config awb_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_flt_config - RkISP1 extensible params FLT config > > > + * > > > + * RkISP1 extensible parameters Filter configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @flt_config: Filter configuration, see > > > + * :c:type:`rkisp1_cif_isp_flt_config` > > > + */ > > > +struct rkisp1_ext_params_flt_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_flt_config flt_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_bdm_config - RkISP1 extensible params BDM config > > > + * > > > + * RkISP1 extensible parameters Demosaicing configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @bdm_config: Demosaicing configuration, see > > > + * :c:type:`rkisp1_cif_isp_bdm_config` > > > + */ > > > +struct rkisp1_ext_params_bdm_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_bdm_config bdm_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_ctk_config - RkISP1 extensible params CTK config > > > + * > > > + * RkISP1 extensible parameters Cross-Talk configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @ctk_config: Cross-Talk configuration, see > > > + * :c:type:`rkisp1_cif_isp_ctk_config` > > > + */ > > > +struct rkisp1_ext_params_ctk_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_ctk_config ctk_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_goc_config - RkISP1 extensible params GOC config > > > + * > > > + * RkISP1 extensible parameters Gamma-Out configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @goc_config: Gamma-Out configuration, see > > > + * :c:type:`rkisp1_cif_isp_goc_config` > > > + */ > > > +struct rkisp1_ext_params_goc_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_goc_config goc_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_dpf_config - RkISP1 extensible params DPF config > > > + * > > > + * RkISP1 extensible parameters De-noise Pre-Filter configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @dpf_config: De-noise Pre-Filter configuration, see > > > + * :c:type:`rkisp1_cif_isp_dpf_config` > > > + */ > > > +struct rkisp1_ext_params_dpf_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_dpf_config dpf_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_dpf_strength_config - RkISP1 extensible params DPF > > > + * strength config > > > + * > > > + * RkISP1 extensible parameters De-noise Pre-Filter strength configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @dpf_strength_config: De-noise Pre-Filter strength configuration, see > > > + * :c:type:`rkisp1_cif_isp_dpf_strength_config` > > > + */ > > > +struct rkisp1_ext_params_dpf_strength_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_dpf_strength_config dpf_strength_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_cproc_config - RkISP1 extensible params CPROC config > > > + * > > > + * RkISP1 extensible parameters Color Processing configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @cproc_config: Color processing configuration, see > > > + * :c:type:`rkisp1_cif_isp_cproc_config` > > > + */ > > > +struct rkisp1_ext_params_cproc_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_cproc_config cproc_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_ie_config - RkISP1 extensible params IE config > > > + * > > > + * RkISP1 extensible parameters Image Effect configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @ie_config: Image Effect configuration, see > > > + * :c:type:`rkisp1_cif_isp_ie_config` > > > + */ > > > +struct rkisp1_ext_params_ie_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_ie_config ie_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_awb_meas_config - RkISP1 extensible params AWB > > > + * Meas config > > > + * > > > + * RkISP1 extensible parameters Auto-White Balance Measurement configuration > > > + * block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @awb_meas_config: Auto-White Balance measure configuration, see > > > + * :c:type:`rkisp1_cif_isp_awb_meas_config` > > > + */ > > > +struct rkisp1_ext_params_awb_meas_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_awb_meas_config awb_meas_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_hst_config - RkISP1 extensible params Histogram config > > > + * > > > + * RkISP1 extensible parameters Histogram statistics configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @hst_config: Histogram statistics configuration, see > > > + * :c:type:`rkisp1_cif_isp_hst_config` > > > + */ > > > +struct rkisp1_ext_params_hst_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_hst_config hst_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_aec_config - RkISP1 extensible params AEC config > > > + * > > > + * RkISP1 extensible parameters Auto-Exposure statistics configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @aec_config: Auto-Exposure statistics configuration, see > > > + * :c:type:`rkisp1_cif_isp_aec_config` > > > + */ > > > +struct rkisp1_ext_params_aec_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_aec_config aec_config; > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_afc_config - RkISP1 extensible params AFC config > > > + * > > > + * RkISP1 extensible parameters Auto-Focus statistics configuration block > > > + * > > > + * @header: The RkISP1 extensible parameters header, see > > > + * :c:type:`rkisp1_ext_params_block_header` > > > + * @afc_config: Auto-Focus statistics configuration, see > > > + * :c:type:`rkisp1_cif_isp_afc_config` > > > + */ > > > +struct rkisp1_ext_params_afc_config { > > > + struct rkisp1_ext_params_block_header header; > > > + struct rkisp1_cif_isp_afc_config afc_config; > > > +}; > > > + > > > +#define RKISP1_EXT_PARAMS_MAX_SIZE \ > > > + (sizeof(struct rkisp1_ext_params_bls_config) +\ > > > + sizeof(struct rkisp1_ext_params_dpcc_config) +\ > > > + sizeof(struct rkisp1_ext_params_sdg_config) +\ > > > + sizeof(struct rkisp1_ext_params_lsc_config) +\ > > > + sizeof(struct rkisp1_ext_params_awb_gain_config) +\ > > > + sizeof(struct rkisp1_ext_params_flt_config) +\ > > > + sizeof(struct rkisp1_ext_params_bdm_config) +\ > > > + sizeof(struct rkisp1_ext_params_ctk_config) +\ > > > + sizeof(struct rkisp1_ext_params_goc_config) +\ > > > + sizeof(struct rkisp1_ext_params_dpf_config) +\ > > > + sizeof(struct rkisp1_ext_params_dpf_strength_config) +\ > > > + sizeof(struct rkisp1_ext_params_cproc_config) +\ > > > + sizeof(struct rkisp1_ext_params_ie_config) +\ > > > + sizeof(struct rkisp1_ext_params_awb_meas_config) +\ > > > + sizeof(struct rkisp1_ext_params_hst_config) +\ > > > + sizeof(struct rkisp1_ext_params_aec_config) +\ > > > + sizeof(struct rkisp1_ext_params_afc_config)) > > > + > > > +/** > > > + * enum rksip1_ext_param_buffer_version - RkISP1 extensible parameters version > > > + * > > > + * @RKISP1_EXT_PARAM_BUFFER_V1: First version of RkISP1 extensible parameters > > > + */ > > > +enum rksip1_ext_param_buffer_version { > > > + RKISP1_EXT_PARAM_BUFFER_V1 = 1, > > > +}; > > > + > > > +/** > > > + * struct rkisp1_ext_params_cfg - RkISP1 extensible parameters configuration > > > + * > > > + * This struct contains the configuration parameters of the RkISP1 ISP > > > + * algorithms, serialized by userspace into a data buffer. Each configuration > > > + * parameter block is represented by a block-specific structure which contains a > > > + * :c:type:`rkisp1_ext_params_block_header` entry as first member. Userspace > > > + * populates the @data buffer with configuration parameters for the blocks that > > > + * it intends to configure. As a consequence, the data buffer effective size > > > + * changes according to the number of ISP blocks that userspace intends to > > > + * configure and is set by userspace in the @total_size field. > > > + * > > > + * The parameters buffer is versioned by the @version field to allow modifying > > > + * and extending its definition. Userspace shall populate the @version field to > > > + * inform the driver about the version it intends to use. The driver will parse > > > + * and handle the @data buffer according to the data layout specific to the > > > + * indicated version and return an error if the desired version is not > > > + * supported. > > > + * > > > + * For each ISP block that userspace wants to configure, a block-specific > > > + * structure is appended to the @data buffer, one after the other without gaps > > I think we should align all the blocks to a 64 bits boundary. Otherwise > we'll have unaligned access issues, as well as layout differences > between 32-bitand 64-bit userspace. Each block header is 64 bits aligned, what follows it's not Should I add __attribute__((aligned(8)) To each block ? > > > > + * in between nor overlaps. Userspace shall populate the @total_size field with > > > + * the effective size, in bytes, of the @data buffer. > > > + * > > > + * The expected memory layout of the parameters buffer is:: > > > + * > > > + * +-------------------- struct rkisp1_ext_params_cfg -------------------+ > > > + * | version = RKISP_EXT_PARAMS_BUFFER_V1; | > > > + * | total_size = sizeof(struct rkisp1_ext_params_bls_config) | > > > + * | sizeof(struct rkisp1_ext_params_dpcc_config); | > > > + * | +------------------------- data ---------------------------------+ | > > > + * | | +------------- struct rkisp1_ext_params_bls_config -----------+ | | > > > + * | | | +-------- struct rkisp1_ext_params_block_header ---------+ | | | > > > + * | | | | type = RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS; | | | | > > > + * | | | | state = RKISP1_EXT_PARAMS_BLOCK_ENABLE; | | | | > > > + * | | | | size = sizeof(struct rkisp1_ext_params_bls_config); | | | | > > > + * | | | +---------------------------------------------------------+ | | | > > > + * | | | +---------- struct rkisp1_cif_isp_bls_config -------------+ | | | > > > + * | | | | enable_auto = 0; | | | | > > > + * | | | | fixed_val.r = 256; | | | | > > > + * | | | | fixed_val.gr = 256; | | | | > > > + * | | | | fixed_val.gb = 256; | | | | > > > + * | | | | fixed_val.b = 256; | | | | > > > + * | | | +---------------------------------------------------------+ | | | > > > + * | | +------------ struct rkisp1_ext_params_dpcc_config -----------+ | | > > > + * | | | +-------- struct rkisp1_ext_params_block_header ---------+ | | | > > > + * | | | | type = RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC; | | | | > > > + * | | | | state = RKISP1_EXT_PARAMS_BLOCK_ENABLE; | | | | > > > + * | | | | size = sizeof(struct rkisp1_ext_params_dpcc_config); | | | | > > > + * | | | +---------------------------------------------------------+ | | | > > > + * | | | +---------- struct rkisp1_cif_isp_dpcc_config ------------+ | | | > > > + * | | | | mode = RKISP1_CIF_ISP_DPCC_MODE_STAGE1_ENABLE; | | | | > > > + * | | | | output_mode = | | | | > > > + * | | | | RKISP1_CIF_ISP_DPCC_OUTPUT_MODE_STAGE1_INCL_G_CENTER; | | | | > > > + * | | | | set_use = ... ; | | | | > > > + * | | | | ... = ... ; | | | | > > > + * | | | +---------------------------------------------------------+ | | | > > > + * | | +-------------------------------------------------------------+ | | > > > + * | +-----------------------------------------------------------------+ | > > > + * +---------------------------------------------------------------------+ > > > + * > > > + * @version: The RkISP1 extensible parameters buffer version, see > > > + * :c:type:`rksip1_ext_param_buffer_version` > > > + * @total_size: The RkISP1 configuration data effective size, excluding this > > > + * header > > > + * @data: The RkISP1 extensible configuration data blocks > > > + */ > > > +struct rkisp1_ext_params_cfg { > > > + __u32 version; > > On a 64-bit system there will be a 32-bit hole here, while on a 32-bit > system there won't. This means that a 32-bit userspace won't run on a > 64-bit kernel. You can add a a __u32 reserved field to fix that. Or make version and __u64 ? > > > > + __u64 total_size; > > > + __u8 data[RKISP1_EXT_PARAMS_MAX_SIZE]; > > > +}; > > > + > > > #endif /* _UAPI_RKISP1_CONFIG_H */ > > -- > Regards, > > Laurent Pinchart >