| Message ID | 20240605165434.432230-6-jacopo.mondi@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/8] uapi: rkisp1-config: Add extensible parameters format | expand |
Hi Jacopo On 05/06/2024 17:54, Jacopo Mondi wrote: > Implement support in the rkisp1-params for the extensible configuration > parameters format. > > Create a list of handlers for each ISP block that wraps the existing > configuration functions and handles the ISP block enablement state. > > Parse the configuration parameters buffer in __rkisp1_ext_params_config > and filter the enable blocks by group, to allow setting the 'other' and > 'meas' groups separately from the 'lsc' group to support the > pre/post-configure operations. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > .../platform/rockchip/rkisp1/rkisp1-params.c | 565 +++++++++++++++++- > 1 file changed, 544 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > index 6f99c7dad758..3d78e643d0b8 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > @@ -33,6 +33,10 @@ > #define RKISP1_ISP_CC_COEFF(n) \ > (RKISP1_CIF_ISP_CC_COEFF_0 + (n) * 4) > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS BIT(0) > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS BIT(1) > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC BIT(2) > + > enum rkisp1_params_formats { > RKISP1_PARAMS_FIXED, > RKISP1_PARAMS_EXTENSIBLE, > @@ -1529,9 +1533,491 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params, > } > } > > +/*------------------------------------------------------------------------------ > + * Extensible parameters format handling > + */ > + > +static void rkisp1_ext_params_bls(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_bls_config *bls = > + (struct rkisp1_ext_params_bls_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_BLS_CTRL, > + RKISP1_CIF_ISP_BLS_ENA); > + return; > + } > + > + rkisp1_bls_config(params, &bls->bls_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_BLS_CTRL, > + RKISP1_CIF_ISP_BLS_ENA); > +} Most of the handlers have all but identical handling for the enable/disable parts; is it worth factoring that out perhaps? The register and bits could be added to struct rkisp1_ext_params_handler and then a common pre-handler function could be called from __rkisp1_ext_params_config() to set/clear the bits. > + > +static void rkisp1_ext_params_dpcc(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_dpcc_config *dpcc = > + (struct rkisp1_ext_params_dpcc_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE, > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); > + return; > + } > + > + rkisp1_dpcc_config(params, &dpcc->dpcc_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPCC_MODE, > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); > +} > + > +static void rkisp1_ext_params_sdg(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_sdg_config *sdg = > + (struct rkisp1_ext_params_sdg_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); > + return; > + } > + > + rkisp1_sdg_config(params, &sdg->sdg_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); > +} > + > +static void rkisp1_ext_params_lsc(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_lsc_config *lsc = > + (struct rkisp1_ext_params_lsc_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_LSC_CTRL, > + RKISP1_CIF_ISP_LSC_CTRL_ENA); > + return; > + } > + > + rkisp1_lsc_config(params, &lsc->lsc_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_LSC_CTRL, > + RKISP1_CIF_ISP_LSC_CTRL_ENA); > +} > + > +static void rkisp1_ext_params_awbg(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_awb_gain_config *awbg = > + (struct rkisp1_ext_params_awb_gain_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); > + return; > + } > + > + params->ops->awb_gain_config(params, &awbg->awb_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); > +} > + > +static void rkisp1_ext_params_flt(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_flt_config *flt = > + (struct rkisp1_ext_params_flt_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_FILT_MODE, > + RKISP1_CIF_ISP_FLT_ENA); > + return; > + } > + > + rkisp1_flt_config(params, &flt->flt_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_FILT_MODE, > + RKISP1_CIF_ISP_FLT_ENA); > +} > + > +static void rkisp1_ext_params_bdm(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_bdm_config *bdm = > + (struct rkisp1_ext_params_bdm_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DEMOSAIC, > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); > + return; > + } > + > + rkisp1_bdm_config(params, &bdm->bdm_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DEMOSAIC, > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); > +} > + > +static void rkisp1_ext_params_ctk(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_ctk_config *ctk = > + (struct rkisp1_ext_params_ctk_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_ctk_enable(params, false); > + return; > + } > + > + rkisp1_ctk_config(params, &ctk->ctk_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_ctk_enable(params, true); > +} > + > +static void rkisp1_ext_params_goc(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_goc_config *goc = > + (struct rkisp1_ext_params_goc_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); > + return; > + } > + > + params->ops->goc_config(params, &goc->goc_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); > +} > + > +static void rkisp1_ext_params_dpf(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_dpf_config *dpf = > + (struct rkisp1_ext_params_dpf_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPF_MODE, > + RKISP1_CIF_ISP_DPF_MODE_EN); > + return; > + } > + > + rkisp1_dpf_config(params, &dpf->dpf_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPF_MODE, > + RKISP1_CIF_ISP_DPF_MODE_EN); > +} > + > +static void rkisp1_ext_params_dpfs(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_dpf_strength_config *dpfs = > + (struct rkisp1_ext_params_dpf_strength_config *)hdr; > + > + rkisp1_dpf_strength_config(params, &dpfs->dpf_strength_config); > +} > + > +static void rkisp1_ext_params_cproc(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_cproc_config *cproc = > + (struct rkisp1_ext_params_cproc_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL, > + RKISP1_CIF_C_PROC_CTR_ENABLE); > + return; > + } > + > + rkisp1_cproc_config(params, &cproc->cproc_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_C_PROC_CTRL, > + RKISP1_CIF_C_PROC_CTR_ENABLE); > +} > + > +static void rkisp1_ext_params_ie(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_ie_config *ie = > + (struct rkisp1_ext_params_ie_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_ie_enable(params, false); > + return; > + } > + > + rkisp1_ie_config(params, &ie->ie_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_ie_enable(params, true); > +} > + > +static void rkisp1_ext_params_awbm(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_awb_meas_config *awbm = > + (struct rkisp1_ext_params_awb_meas_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, > + false); > + return; > + } > + > + params->ops->awb_meas_config(params, &awbm->awb_meas_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, > + true); > +} > + > +static void rkisp1_ext_params_hstm(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_hst_config *hst = > + (struct rkisp1_ext_params_hst_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + params->ops->hst_enable(params, &hst->hst_config, false); > + return; > + } > + > + params->ops->hst_config(params, &hst->hst_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + params->ops->hst_enable(params, &hst->hst_config, true); > +} > + > +static void rkisp1_ext_params_aecm(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_aec_config *aec = > + (struct rkisp1_ext_params_aec_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_EXP_CTRL, > + RKISP1_CIF_ISP_EXP_ENA); > + return; > + } > + > + params->ops->aec_config(params, &aec->aec_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_EXP_CTRL, > + RKISP1_CIF_ISP_EXP_ENA); > +} > + > +static void rkisp1_ext_params_afcm(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr) > +{ > + struct rkisp1_ext_params_afc_config *afc = > + (struct rkisp1_ext_params_afc_config *)hdr; > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > + RKISP1_CIF_ISP_AFM_ENA); > + return; > + } > + > + params->ops->afm_config(params, &afc->afc_config); > + > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > + RKISP1_CIF_ISP_AFM_ENA); > +} > + > +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr); > + > +static const struct rkisp1_ext_params_handler { > + size_t size; > + rkisp1_block_handler handler; > + unsigned int group; > +} rkisp1_ext_params_handlers[] = { > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = { > + .size = sizeof(struct rkisp1_ext_params_bls_config), > + .handler = rkisp1_ext_params_bls, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = { > + .size = sizeof(struct rkisp1_ext_params_dpcc_config), > + .handler = rkisp1_ext_params_dpcc, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG] = { > + .size = sizeof(struct rkisp1_ext_params_sdg_config), > + .handler = rkisp1_ext_params_sdg, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS] = { > + .size = > + sizeof(struct rkisp1_ext_params_awb_gain_config), > + .handler = rkisp1_ext_params_awbg, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT] = { > + .size = sizeof(struct rkisp1_ext_params_flt_config), > + .handler = rkisp1_ext_params_flt, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM] = { > + .size = sizeof(struct rkisp1_ext_params_bdm_config), > + .handler = rkisp1_ext_params_bdm, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK] = { > + .size = sizeof(struct rkisp1_ext_params_ctk_config), > + .handler = rkisp1_ext_params_ctk, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC] = { > + .size = sizeof(struct rkisp1_ext_params_goc_config), > + .handler = rkisp1_ext_params_goc, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF] = { > + .size = sizeof(struct rkisp1_ext_params_dpf_config), > + .handler = rkisp1_ext_params_dpf, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGHT] = { > + .size = > + sizeof(struct rkisp1_ext_params_dpf_strength_config), > + .handler = rkisp1_ext_params_dpfs, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC] = { > + .size = sizeof(struct rkisp1_ext_params_cproc_config), > + .handler = rkisp1_ext_params_cproc, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_IE] = { > + .size = sizeof(struct rkisp1_ext_params_ie_config), > + .handler = rkisp1_ext_params_ie, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC] = { > + .size = sizeof(struct rkisp1_ext_params_lsc_config), > + .handler = rkisp1_ext_params_lsc, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS] = { > + .size = > + sizeof(struct rkisp1_ext_params_awb_meas_config), > + .handler = rkisp1_ext_params_awbm, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS] = { > + .size = sizeof(struct rkisp1_ext_params_hst_config), > + .handler = rkisp1_ext_params_hstm, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS] = { > + .size = sizeof(struct rkisp1_ext_params_aec_config), > + .handler = rkisp1_ext_params_aecm, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > + }, > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS] = { > + .size = sizeof(struct rkisp1_ext_params_afc_config), > + .handler = rkisp1_ext_params_afcm, > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > + }, > +}; > + > +static int __rkisp1_ext_params_config(struct rkisp1_params *params, > + struct rkisp1_ext_params_cfg *cfg, > + u32 block_group_mask) > +{ > + size_t block_offset = 0; > + > + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) { > + dev_dbg(params->rkisp1->dev, > + "Invalid parameters buffer size %llu\n", > + cfg->total_size); > + return -EINVAL; > + } > + > + /* Walk the list of parameter blocks and process them. */ > + while (block_offset < cfg->total_size) { > + const struct rkisp1_ext_params_handler *block_handler; > + struct rkisp1_ext_params_block_header *block; > + > + block = (struct rkisp1_ext_params_block_header *) > + &cfg->data[block_offset]; > + block_offset += block->size; > + > + /* > + * Validate the block id and make sure the block group is in > + * the list of groups to configure. > + */ > + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) { > + dev_dbg(params->rkisp1->dev, > + "Invalid parameters block type\n"); > + return -EINVAL; > + } > + > + block_handler = &rkisp1_ext_params_handlers[block->type]; > + if (!(block_handler->group & block_group_mask)) > + continue; So maybe something like if (block_handler->enable_reg) __rkisp1_block_handle_enable_disable(block->state, block_handler->enable_reg, block_handler->enable_val); here to move it out of the handlers. > + > + if (block->size != block_handler->size) { > + dev_dbg(params->rkisp1->dev, > + "Invalid parameters block size\n"); > + return -EINVAL; > + } > + > + block_handler->handler(params, block); > + } > + > + return 0; > +} > + > +static int rkisp1_ext_params_config(struct rkisp1_params *params, > + struct rkisp1_ext_params_cfg *cfg) > +{ > + return __rkisp1_ext_params_config(params, cfg, > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC | > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); > +} > + > +static int > +rkisp1_ext_params_other_meas_config(struct rkisp1_params *params, > + struct rkisp1_ext_params_cfg *cfg) > +{ > + return __rkisp1_ext_params_config(params, cfg, > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); > +} > + > +static int rkisp1_ext_params_lsc_config(struct rkisp1_params *params, > + struct rkisp1_ext_params_cfg *cfg) > +{ > + return __rkisp1_ext_params_config(params, cfg, > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > +} > + > static bool rkisp1_params_get_buffer(struct rkisp1_params *params, > struct rkisp1_buffer **buf, > - struct rkisp1_params_cfg **cfg) > + void **cfg) > { > if (list_empty(¶ms->params)) > return false; > @@ -1544,28 +2030,37 @@ static bool rkisp1_params_get_buffer(struct rkisp1_params *params, > > static void rkisp1_params_complete_buffer(struct rkisp1_params *params, > struct rkisp1_buffer *buf, > - unsigned int frame_sequence) > + unsigned int frame_sequence, > + enum vb2_buffer_state state) > { > list_del(&buf->queue); > > buf->vb.sequence = frame_sequence; > - vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE); > + vb2_buffer_done(&buf->vb.vb2_buf, state); > } > > void rkisp1_params_isr(struct rkisp1_device *rkisp1) > { > struct rkisp1_params *params = &rkisp1->params; > - struct rkisp1_params_cfg *new_params; > - struct rkisp1_buffer *cur_buf; > + struct rkisp1_buffer *buf; Why the rename? > + int ret = 0; > + void *cfg; > > spin_lock(¶ms->config_lock); > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > goto unlock; > > - rkisp1_isp_isr_other_config(params, new_params); > - rkisp1_isp_isr_lsc_config(params, new_params); > - rkisp1_isp_isr_meas_config(params, new_params); > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > + rkisp1_isp_isr_other_config(params, cfg); > + rkisp1_isp_isr_lsc_config(params, cfg); > + rkisp1_isp_isr_meas_config(params, cfg); > + } else { > + ret = rkisp1_ext_params_config(params, cfg); > + } > + > + if (ret) > + goto complete_and_unlock; > > /* update shadow register immediately */ > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > @@ -1579,8 +2074,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) > * indicate to userspace on which frame these parameters are being > * applied. > */ > - rkisp1_params_complete_buffer(params, cur_buf, > - rkisp1->isp.frame_sequence + 1); > +complete_and_unlock: > + rkisp1_params_complete_buffer(params, buf, > + rkisp1->isp.frame_sequence + 1, > + ret ? VB2_BUF_STATE_ERROR > + : VB2_BUF_STATE_DONE); > > unlock: > spin_unlock(¶ms->config_lock); > @@ -1631,8 +2129,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > enum v4l2_ycbcr_encoding ycbcr_encoding) > { > struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config; > - struct rkisp1_params_cfg *new_params; > - struct rkisp1_buffer *cur_buf; > + struct rkisp1_buffer *buf; > + int ret = 0; > + void *cfg; > > params->quantization = quantization; > params->ycbcr_encoding = ycbcr_encoding; > @@ -1661,11 +2160,26 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > /* apply the first buffer if there is one already */ > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > goto unlock; > > - rkisp1_isp_isr_other_config(params, new_params); > - rkisp1_isp_isr_meas_config(params, new_params); > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > + rkisp1_isp_isr_other_config(params, cfg); > + rkisp1_isp_isr_meas_config(params, cfg); > + } else { > + ret = rkisp1_ext_params_other_meas_config(params, cfg); > + } > + > + if (ret) { > + /* > + * Complete the buffer in error state immediately. In case of no > + * error, the buffer will be completed in > + * rkisp1_params_post_configure(). > + */ > + rkisp1_params_complete_buffer(params, buf, 0, > + VB2_BUF_STATE_ERROR); > + goto unlock; > + } > > /* update shadow register immediately */ > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > @@ -1677,8 +2191,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > void rkisp1_params_post_configure(struct rkisp1_params *params) > { > - struct rkisp1_params_cfg *new_params; > - struct rkisp1_buffer *cur_buf; > + struct rkisp1_buffer *buf; And likewise here? > + int ret = 0; > + void *cfg; > > spin_lock_irq(¶ms->config_lock); > > @@ -1691,16 +2206,24 @@ void rkisp1_params_post_configure(struct rkisp1_params *params) > * unconditionally. > */ > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > goto unlock; > > - rkisp1_isp_isr_lsc_config(params, new_params); > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > + rkisp1_isp_isr_lsc_config(params, cfg); > + else > + ret = rkisp1_ext_params_lsc_config(params, cfg); > + > + if (ret) > + goto complete_and_unlock; > > /* update shadow register immediately */ > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD); > > - rkisp1_params_complete_buffer(params, cur_buf, 0); > +complete_and_unlock: > + rkisp1_params_complete_buffer(params, buf, 0, ret ? VB2_BUF_STATE_ERROR > + : VB2_BUF_STATE_DONE); > > unlock: > spin_unlock_irq(¶ms->config_lock);
On Wed, Jun 12, 2024 at 02:50:34PM +0100, Daniel Scally wrote: > On 05/06/2024 17:54, Jacopo Mondi wrote: > > Implement support in the rkisp1-params for the extensible configuration > > parameters format. > > > > Create a list of handlers for each ISP block that wraps the existing > > configuration functions and handles the ISP block enablement state. > > > > Parse the configuration parameters buffer in __rkisp1_ext_params_config > > and filter the enable blocks by group, to allow setting the 'other' and > > 'meas' groups separately from the 'lsc' group to support the > > pre/post-configure operations. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > .../platform/rockchip/rkisp1/rkisp1-params.c | 565 +++++++++++++++++- > > 1 file changed, 544 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > index 6f99c7dad758..3d78e643d0b8 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > @@ -33,6 +33,10 @@ > > #define RKISP1_ISP_CC_COEFF(n) \ > > (RKISP1_CIF_ISP_CC_COEFF_0 + (n) * 4) > > > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS BIT(0) > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS BIT(1) > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC BIT(2) Would "LSC" and "OTHERS" be enough ? The OTHERS and MEAS groups are always handled together. > > + > > enum rkisp1_params_formats { > > RKISP1_PARAMS_FIXED, > > RKISP1_PARAMS_EXTENSIBLE, > > @@ -1529,9 +1533,491 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params, > > } > > } > > > > +/*------------------------------------------------------------------------------ > > + * Extensible parameters format handling > > + */ > > + > > +static void rkisp1_ext_params_bls(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_bls_config *bls = > > + (struct rkisp1_ext_params_bls_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_BLS_CTRL, > > + RKISP1_CIF_ISP_BLS_ENA); > > + return; > > + } > > + > > + rkisp1_bls_config(params, &bls->bls_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_BLS_CTRL, > > + RKISP1_CIF_ISP_BLS_ENA); > > +} > > Most of the handlers have all but identical handling for the > enable/disable parts; is it worth factoring that out perhaps? The > register and bits could be added to struct rkisp1_ext_params_handler > and then a common pre-handler function could be called from > __rkisp1_ext_params_config() to set/clear the bits. I like the idea of generalizing things :-) The devil is in the details though, so if it causes annoying issues, we can skip it. If we drop RKISP1_EXT_PARAMS_BLOCK_NO_CHANGE, we should also cache the enable state. That should be easy to do in a bitmask indexed by block type, and will fit nicely in generalized handling of the enable bits. > > + > > +static void rkisp1_ext_params_dpcc(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_dpcc_config *dpcc = > > + (struct rkisp1_ext_params_dpcc_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE, > > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); > > + return; > > + } > > + > > + rkisp1_dpcc_config(params, &dpcc->dpcc_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPCC_MODE, > > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); > > +} > > + > > +static void rkisp1_ext_params_sdg(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_sdg_config *sdg = > > + (struct rkisp1_ext_params_sdg_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); > > + return; > > + } > > + > > + rkisp1_sdg_config(params, &sdg->sdg_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); > > +} > > + > > +static void rkisp1_ext_params_lsc(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_lsc_config *lsc = > > + (struct rkisp1_ext_params_lsc_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_LSC_CTRL, > > + RKISP1_CIF_ISP_LSC_CTRL_ENA); > > + return; > > + } > > + > > + rkisp1_lsc_config(params, &lsc->lsc_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_LSC_CTRL, > > + RKISP1_CIF_ISP_LSC_CTRL_ENA); > > +} > > + > > +static void rkisp1_ext_params_awbg(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_awb_gain_config *awbg = > > + (struct rkisp1_ext_params_awb_gain_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); > > + return; > > + } > > + > > + params->ops->awb_gain_config(params, &awbg->awb_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); > > +} > > + > > +static void rkisp1_ext_params_flt(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_flt_config *flt = > > + (struct rkisp1_ext_params_flt_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_FILT_MODE, > > + RKISP1_CIF_ISP_FLT_ENA); > > + return; > > + } > > + > > + rkisp1_flt_config(params, &flt->flt_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_FILT_MODE, > > + RKISP1_CIF_ISP_FLT_ENA); > > +} > > + > > +static void rkisp1_ext_params_bdm(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_bdm_config *bdm = > > + (struct rkisp1_ext_params_bdm_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DEMOSAIC, > > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); > > + return; > > + } > > + > > + rkisp1_bdm_config(params, &bdm->bdm_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DEMOSAIC, > > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); > > +} > > + > > +static void rkisp1_ext_params_ctk(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_ctk_config *ctk = > > + (struct rkisp1_ext_params_ctk_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_ctk_enable(params, false); > > + return; > > + } > > + > > + rkisp1_ctk_config(params, &ctk->ctk_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_ctk_enable(params, true); > > +} > > + > > +static void rkisp1_ext_params_goc(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_goc_config *goc = > > + (struct rkisp1_ext_params_goc_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); > > + return; > > + } > > + > > + params->ops->goc_config(params, &goc->goc_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); > > +} > > + > > +static void rkisp1_ext_params_dpf(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_dpf_config *dpf = > > + (struct rkisp1_ext_params_dpf_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPF_MODE, > > + RKISP1_CIF_ISP_DPF_MODE_EN); > > + return; > > + } > > + > > + rkisp1_dpf_config(params, &dpf->dpf_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPF_MODE, > > + RKISP1_CIF_ISP_DPF_MODE_EN); > > +} > > + > > +static void rkisp1_ext_params_dpfs(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_dpf_strength_config *dpfs = > > + (struct rkisp1_ext_params_dpf_strength_config *)hdr; > > + > > + rkisp1_dpf_strength_config(params, &dpfs->dpf_strength_config); > > +} > > + > > +static void rkisp1_ext_params_cproc(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_cproc_config *cproc = > > + (struct rkisp1_ext_params_cproc_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL, > > + RKISP1_CIF_C_PROC_CTR_ENABLE); > > + return; > > + } > > + > > + rkisp1_cproc_config(params, &cproc->cproc_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_C_PROC_CTRL, > > + RKISP1_CIF_C_PROC_CTR_ENABLE); > > +} > > + > > +static void rkisp1_ext_params_ie(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_ie_config *ie = > > + (struct rkisp1_ext_params_ie_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_ie_enable(params, false); > > + return; > > + } > > + > > + rkisp1_ie_config(params, &ie->ie_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_ie_enable(params, true); > > +} > > + > > +static void rkisp1_ext_params_awbm(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_awb_meas_config *awbm = > > + (struct rkisp1_ext_params_awb_meas_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, > > + false); > > + return; > > + } > > + > > + params->ops->awb_meas_config(params, &awbm->awb_meas_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, > > + true); > > +} > > + > > +static void rkisp1_ext_params_hstm(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_hst_config *hst = > > + (struct rkisp1_ext_params_hst_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + params->ops->hst_enable(params, &hst->hst_config, false); > > + return; > > + } > > + > > + params->ops->hst_config(params, &hst->hst_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + params->ops->hst_enable(params, &hst->hst_config, true); > > +} > > + > > +static void rkisp1_ext_params_aecm(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_aec_config *aec = > > + (struct rkisp1_ext_params_aec_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_EXP_CTRL, > > + RKISP1_CIF_ISP_EXP_ENA); > > + return; > > + } > > + > > + params->ops->aec_config(params, &aec->aec_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_EXP_CTRL, > > + RKISP1_CIF_ISP_EXP_ENA); > > +} > > + > > +static void rkisp1_ext_params_afcm(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr) > > +{ > > + struct rkisp1_ext_params_afc_config *afc = > > + (struct rkisp1_ext_params_afc_config *)hdr; > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > > + RKISP1_CIF_ISP_AFM_ENA); > > + return; > > + } > > + > > + params->ops->afm_config(params, &afc->afc_config); > > + > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > > + RKISP1_CIF_ISP_AFM_ENA); > > +} > > + > > +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr); > > + > > +static const struct rkisp1_ext_params_handler { > > + size_t size; > > + rkisp1_block_handler handler; > > + unsigned int group; We'll have to extend this structure with a bitmask of the ISP versions each block supports. That can be done later. > > +} rkisp1_ext_params_handlers[] = { > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = { > > + .size = sizeof(struct rkisp1_ext_params_bls_config), > > + .handler = rkisp1_ext_params_bls, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = { > > + .size = sizeof(struct rkisp1_ext_params_dpcc_config), > > + .handler = rkisp1_ext_params_dpcc, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG] = { > > + .size = sizeof(struct rkisp1_ext_params_sdg_config), > > + .handler = rkisp1_ext_params_sdg, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS] = { > > + .size = > > + sizeof(struct rkisp1_ext_params_awb_gain_config), > > + .handler = rkisp1_ext_params_awbg, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT] = { > > + .size = sizeof(struct rkisp1_ext_params_flt_config), > > + .handler = rkisp1_ext_params_flt, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM] = { > > + .size = sizeof(struct rkisp1_ext_params_bdm_config), > > + .handler = rkisp1_ext_params_bdm, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK] = { > > + .size = sizeof(struct rkisp1_ext_params_ctk_config), > > + .handler = rkisp1_ext_params_ctk, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC] = { > > + .size = sizeof(struct rkisp1_ext_params_goc_config), > > + .handler = rkisp1_ext_params_goc, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF] = { > > + .size = sizeof(struct rkisp1_ext_params_dpf_config), > > + .handler = rkisp1_ext_params_dpf, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGHT] = { > > + .size = > > + sizeof(struct rkisp1_ext_params_dpf_strength_config), > > + .handler = rkisp1_ext_params_dpfs, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC] = { > > + .size = sizeof(struct rkisp1_ext_params_cproc_config), > > + .handler = rkisp1_ext_params_cproc, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_IE] = { > > + .size = sizeof(struct rkisp1_ext_params_ie_config), > > + .handler = rkisp1_ext_params_ie, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC] = { > > + .size = sizeof(struct rkisp1_ext_params_lsc_config), > > + .handler = rkisp1_ext_params_lsc, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS] = { > > + .size = > > + sizeof(struct rkisp1_ext_params_awb_meas_config), > > + .handler = rkisp1_ext_params_awbm, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS] = { > > + .size = sizeof(struct rkisp1_ext_params_hst_config), > > + .handler = rkisp1_ext_params_hstm, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS] = { > > + .size = sizeof(struct rkisp1_ext_params_aec_config), > > + .handler = rkisp1_ext_params_aecm, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > + }, > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS] = { > > + .size = sizeof(struct rkisp1_ext_params_afc_config), > > + .handler = rkisp1_ext_params_afcm, > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > + }, > > +}; > > + > > +static int __rkisp1_ext_params_config(struct rkisp1_params *params, > > + struct rkisp1_ext_params_cfg *cfg, > > + u32 block_group_mask) > > +{ > > + size_t block_offset = 0; > > + > > + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) { > > + dev_dbg(params->rkisp1->dev, > > + "Invalid parameters buffer size %llu\n", > > + cfg->total_size); > > + return -EINVAL; > > + } > > + > > + /* Walk the list of parameter blocks and process them. */ > > + while (block_offset < cfg->total_size) { > > + const struct rkisp1_ext_params_handler *block_handler; > > + struct rkisp1_ext_params_block_header *block; > > + > > + block = (struct rkisp1_ext_params_block_header *) > > + &cfg->data[block_offset]; > > + block_offset += block->size; > > + > > + /* > > + * Validate the block id and make sure the block group is in > > + * the list of groups to configure. > > + */ > > + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) { Use ARRAY_SIZE(). > > + dev_dbg(params->rkisp1->dev, > > + "Invalid parameters block type\n"); > > + return -EINVAL; > > + } > > + > > + block_handler = &rkisp1_ext_params_handlers[block->type]; > > + if (!(block_handler->group & block_group_mask)) > > + continue; > > > So maybe something like > > > if (block_handler->enable_reg) > > __rkisp1_block_handle_enable_disable(block->state, block_handler->enable_reg, > block_handler->enable_val); > > > here to move it out of the handlers. > > > + > > + if (block->size != block_handler->size) { > > + dev_dbg(params->rkisp1->dev, > > + "Invalid parameters block size\n"); > > + return -EINVAL; > > + } > > + > > + block_handler->handler(params, block); It would be nicer to move validation to qbuf time, and applying parameters at runtime. Maybe I'll find that later in the series :-) > > + } > > + > > + return 0; > > +} > > + > > +static int rkisp1_ext_params_config(struct rkisp1_params *params, > > + struct rkisp1_ext_params_cfg *cfg) > > +{ > > + return __rkisp1_ext_params_config(params, cfg, > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC | > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); > > +} > > + > > +static int > > +rkisp1_ext_params_other_meas_config(struct rkisp1_params *params, > > + struct rkisp1_ext_params_cfg *cfg) > > +{ > > + return __rkisp1_ext_params_config(params, cfg, > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); > > +} > > + > > +static int rkisp1_ext_params_lsc_config(struct rkisp1_params *params, > > + struct rkisp1_ext_params_cfg *cfg) > > +{ > > + return __rkisp1_ext_params_config(params, cfg, > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > > +} I'm tempted to inline this in the callers, I think it would be more readable. > > + > > static bool rkisp1_params_get_buffer(struct rkisp1_params *params, > > struct rkisp1_buffer **buf, > > - struct rkisp1_params_cfg **cfg) > > + void **cfg) > > { > > if (list_empty(¶ms->params)) > > return false; > > @@ -1544,28 +2030,37 @@ static bool rkisp1_params_get_buffer(struct rkisp1_params *params, > > > > static void rkisp1_params_complete_buffer(struct rkisp1_params *params, > > struct rkisp1_buffer *buf, > > - unsigned int frame_sequence) > > + unsigned int frame_sequence, > > + enum vb2_buffer_state state) > > { > > list_del(&buf->queue); > > > > buf->vb.sequence = frame_sequence; > > - vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE); > > + vb2_buffer_done(&buf->vb.vb2_buf, state); > > } > > > > void rkisp1_params_isr(struct rkisp1_device *rkisp1) > > { > > struct rkisp1_params *params = &rkisp1->params; > > - struct rkisp1_params_cfg *new_params; > > - struct rkisp1_buffer *cur_buf; > > + struct rkisp1_buffer *buf; > > Why the rename? > > > + int ret = 0; > > + void *cfg; > > > > spin_lock(¶ms->config_lock); > > > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > > goto unlock; > > > > - rkisp1_isp_isr_other_config(params, new_params); > > - rkisp1_isp_isr_lsc_config(params, new_params); > > - rkisp1_isp_isr_meas_config(params, new_params); > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > + rkisp1_isp_isr_other_config(params, cfg); > > + rkisp1_isp_isr_lsc_config(params, cfg); > > + rkisp1_isp_isr_meas_config(params, cfg); > > + } else { > > + ret = rkisp1_ext_params_config(params, cfg); > > + } > > + > > + if (ret) > > + goto complete_and_unlock; As validation should happen at qbuf time, I think it would be nicer to reorder the patches to avoid introducing error handling here and removing it later. > > > > /* update shadow register immediately */ > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > @@ -1579,8 +2074,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) > > * indicate to userspace on which frame these parameters are being > > * applied. > > */ > > - rkisp1_params_complete_buffer(params, cur_buf, > > - rkisp1->isp.frame_sequence + 1); > > +complete_and_unlock: > > + rkisp1_params_complete_buffer(params, buf, > > + rkisp1->isp.frame_sequence + 1, > > + ret ? VB2_BUF_STATE_ERROR > > + : VB2_BUF_STATE_DONE); > > > > unlock: > > spin_unlock(¶ms->config_lock); > > @@ -1631,8 +2129,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > enum v4l2_ycbcr_encoding ycbcr_encoding) > > { > > struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config; > > - struct rkisp1_params_cfg *new_params; > > - struct rkisp1_buffer *cur_buf; > > + struct rkisp1_buffer *buf; > > + int ret = 0; > > + void *cfg; > > > > params->quantization = quantization; > > params->ycbcr_encoding = ycbcr_encoding; > > @@ -1661,11 +2160,26 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > > > /* apply the first buffer if there is one already */ > > > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > > goto unlock; > > > > - rkisp1_isp_isr_other_config(params, new_params); > > - rkisp1_isp_isr_meas_config(params, new_params); > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > + rkisp1_isp_isr_other_config(params, cfg); > > + rkisp1_isp_isr_meas_config(params, cfg); > > + } else { > > + ret = rkisp1_ext_params_other_meas_config(params, cfg); > > + } > > + > > + if (ret) { > > + /* > > + * Complete the buffer in error state immediately. In case of no > > + * error, the buffer will be completed in > > + * rkisp1_params_post_configure(). > > + */ > > + rkisp1_params_complete_buffer(params, buf, 0, > > + VB2_BUF_STATE_ERROR); > > + goto unlock; > > + } > > > > /* update shadow register immediately */ > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > @@ -1677,8 +2191,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > > > void rkisp1_params_post_configure(struct rkisp1_params *params) > > { > > - struct rkisp1_params_cfg *new_params; > > - struct rkisp1_buffer *cur_buf; > > + struct rkisp1_buffer *buf; > > And likewise here? > > > + int ret = 0; > > + void *cfg; > > > > spin_lock_irq(¶ms->config_lock); > > > > @@ -1691,16 +2206,24 @@ void rkisp1_params_post_configure(struct rkisp1_params *params) > > * unconditionally. > > */ > > > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > > goto unlock; > > > > - rkisp1_isp_isr_lsc_config(params, new_params); > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > > + rkisp1_isp_isr_lsc_config(params, cfg); > > + else > > + ret = rkisp1_ext_params_lsc_config(params, cfg); > > + > > + if (ret) > > + goto complete_and_unlock; > > > > /* update shadow register immediately */ > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD); > > > > - rkisp1_params_complete_buffer(params, cur_buf, 0); > > +complete_and_unlock: > > + rkisp1_params_complete_buffer(params, buf, 0, ret ? VB2_BUF_STATE_ERROR > > + : VB2_BUF_STATE_DONE); > > > > unlock: > > spin_unlock_irq(¶ms->config_lock);
Hi Laurent On Wed, Jun 12, 2024 at 06:42:01PM GMT, Laurent Pinchart wrote: > On Wed, Jun 12, 2024 at 02:50:34PM +0100, Daniel Scally wrote: > > On 05/06/2024 17:54, Jacopo Mondi wrote: > > > Implement support in the rkisp1-params for the extensible configuration > > > parameters format. > > > > > > Create a list of handlers for each ISP block that wraps the existing > > > configuration functions and handles the ISP block enablement state. > > > > > > Parse the configuration parameters buffer in __rkisp1_ext_params_config > > > and filter the enable blocks by group, to allow setting the 'other' and > > > 'meas' groups separately from the 'lsc' group to support the > > > pre/post-configure operations. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > .../platform/rockchip/rkisp1/rkisp1-params.c | 565 +++++++++++++++++- > > > 1 file changed, 544 insertions(+), 21 deletions(-) > > > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > > index 6f99c7dad758..3d78e643d0b8 100644 > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > > @@ -33,6 +33,10 @@ > > > #define RKISP1_ISP_CC_COEFF(n) \ > > > (RKISP1_CIF_ISP_CC_COEFF_0 + (n) * 4) > > > > > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS BIT(0) > > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS BIT(1) > > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC BIT(2) > > Would "LSC" and "OTHERS" be enough ? The OTHERS and MEAS groups are > always handled together. > The existing code handles the three separately but I actually program OTHER and MEAS together. Then we're left with "OTHER" and "LSC". I would like a more descriptive name for "OTHER" > > > + > > > enum rkisp1_params_formats { > > > RKISP1_PARAMS_FIXED, > > > RKISP1_PARAMS_EXTENSIBLE, > > > @@ -1529,9 +1533,491 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params, > > > } > > > } > > > > > > +/*------------------------------------------------------------------------------ > > > + * Extensible parameters format handling > > > + */ > > > + > > > +static void rkisp1_ext_params_bls(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_bls_config *bls = > > > + (struct rkisp1_ext_params_bls_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_BLS_CTRL, > > > + RKISP1_CIF_ISP_BLS_ENA); > > > + return; > > > + } > > > + > > > + rkisp1_bls_config(params, &bls->bls_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_BLS_CTRL, > > > + RKISP1_CIF_ISP_BLS_ENA); > > > +} > > > > Most of the handlers have all but identical handling for the > > enable/disable parts; is it worth factoring that out perhaps? The > > register and bits could be added to struct rkisp1_ext_params_handler > > and then a common pre-handler function could be called from > > __rkisp1_ext_params_config() to set/clear the bits. > > I like the idea of generalizing things :-) The devil is in the details > though, so if it causes annoying issues, we can skip it. There are slight differences in how blocks are enabled, most blocks are enabled setting a bit, other have a dedicated function. I would keep them in the handlers for clarify (or I should add an .enable and .disable function, but this seems more cumbersome than what I have here) > > If we drop RKISP1_EXT_PARAMS_BLOCK_NO_CHANGE, we should also cache the > enable state. That should be easy to do in a bitmask indexed by block > type, and will fit nicely in generalized handling of the enable bits. > Done > > > + > > > +static void rkisp1_ext_params_dpcc(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_dpcc_config *dpcc = > > > + (struct rkisp1_ext_params_dpcc_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE, > > > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); > > > + return; > > > + } > > > + > > > + rkisp1_dpcc_config(params, &dpcc->dpcc_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPCC_MODE, > > > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); > > > +} > > > + > > > +static void rkisp1_ext_params_sdg(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_sdg_config *sdg = > > > + (struct rkisp1_ext_params_sdg_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); > > > + return; > > > + } > > > + > > > + rkisp1_sdg_config(params, &sdg->sdg_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); > > > +} > > > + > > > +static void rkisp1_ext_params_lsc(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_lsc_config *lsc = > > > + (struct rkisp1_ext_params_lsc_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_LSC_CTRL, > > > + RKISP1_CIF_ISP_LSC_CTRL_ENA); > > > + return; > > > + } > > > + > > > + rkisp1_lsc_config(params, &lsc->lsc_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_LSC_CTRL, > > > + RKISP1_CIF_ISP_LSC_CTRL_ENA); > > > +} > > > + > > > +static void rkisp1_ext_params_awbg(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_awb_gain_config *awbg = > > > + (struct rkisp1_ext_params_awb_gain_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > > > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); > > > + return; > > > + } > > > + > > > + params->ops->awb_gain_config(params, &awbg->awb_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); > > > +} > > > + > > > +static void rkisp1_ext_params_flt(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_flt_config *flt = > > > + (struct rkisp1_ext_params_flt_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_FILT_MODE, > > > + RKISP1_CIF_ISP_FLT_ENA); > > > + return; > > > + } > > > + > > > + rkisp1_flt_config(params, &flt->flt_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_FILT_MODE, > > > + RKISP1_CIF_ISP_FLT_ENA); > > > +} > > > + > > > +static void rkisp1_ext_params_bdm(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_bdm_config *bdm = > > > + (struct rkisp1_ext_params_bdm_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DEMOSAIC, > > > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); > > > + return; > > > + } > > > + > > > + rkisp1_bdm_config(params, &bdm->bdm_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DEMOSAIC, > > > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); > > > +} > > > + > > > +static void rkisp1_ext_params_ctk(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_ctk_config *ctk = > > > + (struct rkisp1_ext_params_ctk_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_ctk_enable(params, false); > > > + return; > > > + } > > > + > > > + rkisp1_ctk_config(params, &ctk->ctk_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_ctk_enable(params, true); > > > +} > > > + > > > +static void rkisp1_ext_params_goc(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_goc_config *goc = > > > + (struct rkisp1_ext_params_goc_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); > > > + return; > > > + } > > > + > > > + params->ops->goc_config(params, &goc->goc_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); > > > +} > > > + > > > +static void rkisp1_ext_params_dpf(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_dpf_config *dpf = > > > + (struct rkisp1_ext_params_dpf_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPF_MODE, > > > + RKISP1_CIF_ISP_DPF_MODE_EN); > > > + return; > > > + } > > > + > > > + rkisp1_dpf_config(params, &dpf->dpf_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPF_MODE, > > > + RKISP1_CIF_ISP_DPF_MODE_EN); > > > +} > > > + > > > +static void rkisp1_ext_params_dpfs(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_dpf_strength_config *dpfs = > > > + (struct rkisp1_ext_params_dpf_strength_config *)hdr; > > > + > > > + rkisp1_dpf_strength_config(params, &dpfs->dpf_strength_config); > > > +} > > > + > > > +static void rkisp1_ext_params_cproc(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_cproc_config *cproc = > > > + (struct rkisp1_ext_params_cproc_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL, > > > + RKISP1_CIF_C_PROC_CTR_ENABLE); > > > + return; > > > + } > > > + > > > + rkisp1_cproc_config(params, &cproc->cproc_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_C_PROC_CTRL, > > > + RKISP1_CIF_C_PROC_CTR_ENABLE); > > > +} > > > + > > > +static void rkisp1_ext_params_ie(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_ie_config *ie = > > > + (struct rkisp1_ext_params_ie_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_ie_enable(params, false); > > > + return; > > > + } > > > + > > > + rkisp1_ie_config(params, &ie->ie_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_ie_enable(params, true); > > > +} > > > + > > > +static void rkisp1_ext_params_awbm(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_awb_meas_config *awbm = > > > + (struct rkisp1_ext_params_awb_meas_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, > > > + false); > > > + return; > > > + } > > > + > > > + params->ops->awb_meas_config(params, &awbm->awb_meas_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, > > > + true); > > > +} > > > + > > > +static void rkisp1_ext_params_hstm(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_hst_config *hst = > > > + (struct rkisp1_ext_params_hst_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + params->ops->hst_enable(params, &hst->hst_config, false); > > > + return; > > > + } > > > + > > > + params->ops->hst_config(params, &hst->hst_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + params->ops->hst_enable(params, &hst->hst_config, true); > > > +} > > > + > > > +static void rkisp1_ext_params_aecm(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_aec_config *aec = > > > + (struct rkisp1_ext_params_aec_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_EXP_CTRL, > > > + RKISP1_CIF_ISP_EXP_ENA); > > > + return; > > > + } > > > + > > > + params->ops->aec_config(params, &aec->aec_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_EXP_CTRL, > > > + RKISP1_CIF_ISP_EXP_ENA); > > > +} > > > + > > > +static void rkisp1_ext_params_afcm(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr) > > > +{ > > > + struct rkisp1_ext_params_afc_config *afc = > > > + (struct rkisp1_ext_params_afc_config *)hdr; > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > > > + RKISP1_CIF_ISP_AFM_ENA); > > > + return; > > > + } > > > + > > > + params->ops->afm_config(params, &afc->afc_config); > > > + > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > > > + RKISP1_CIF_ISP_AFM_ENA); > > > +} > > > + > > > +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr); > > > + > > > +static const struct rkisp1_ext_params_handler { > > > + size_t size; > > > + rkisp1_block_handler handler; > > > + unsigned int group; > > We'll have to extend this structure with a bitmask of the ISP versions > each block supports. That can be done later. > > > > +} rkisp1_ext_params_handlers[] = { > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = { > > > + .size = sizeof(struct rkisp1_ext_params_bls_config), > > > + .handler = rkisp1_ext_params_bls, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = { > > > + .size = sizeof(struct rkisp1_ext_params_dpcc_config), > > > + .handler = rkisp1_ext_params_dpcc, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG] = { > > > + .size = sizeof(struct rkisp1_ext_params_sdg_config), > > > + .handler = rkisp1_ext_params_sdg, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS] = { > > > + .size = > > > + sizeof(struct rkisp1_ext_params_awb_gain_config), > > > + .handler = rkisp1_ext_params_awbg, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT] = { > > > + .size = sizeof(struct rkisp1_ext_params_flt_config), > > > + .handler = rkisp1_ext_params_flt, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM] = { > > > + .size = sizeof(struct rkisp1_ext_params_bdm_config), > > > + .handler = rkisp1_ext_params_bdm, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK] = { > > > + .size = sizeof(struct rkisp1_ext_params_ctk_config), > > > + .handler = rkisp1_ext_params_ctk, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC] = { > > > + .size = sizeof(struct rkisp1_ext_params_goc_config), > > > + .handler = rkisp1_ext_params_goc, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF] = { > > > + .size = sizeof(struct rkisp1_ext_params_dpf_config), > > > + .handler = rkisp1_ext_params_dpf, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGHT] = { > > > + .size = > > > + sizeof(struct rkisp1_ext_params_dpf_strength_config), > > > + .handler = rkisp1_ext_params_dpfs, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC] = { > > > + .size = sizeof(struct rkisp1_ext_params_cproc_config), > > > + .handler = rkisp1_ext_params_cproc, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_IE] = { > > > + .size = sizeof(struct rkisp1_ext_params_ie_config), > > > + .handler = rkisp1_ext_params_ie, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC] = { > > > + .size = sizeof(struct rkisp1_ext_params_lsc_config), > > > + .handler = rkisp1_ext_params_lsc, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS] = { > > > + .size = > > > + sizeof(struct rkisp1_ext_params_awb_meas_config), > > > + .handler = rkisp1_ext_params_awbm, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS] = { > > > + .size = sizeof(struct rkisp1_ext_params_hst_config), > > > + .handler = rkisp1_ext_params_hstm, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS] = { > > > + .size = sizeof(struct rkisp1_ext_params_aec_config), > > > + .handler = rkisp1_ext_params_aecm, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > > + }, > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS] = { > > > + .size = sizeof(struct rkisp1_ext_params_afc_config), > > > + .handler = rkisp1_ext_params_afcm, > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > > + }, > > > +}; > > > + > > > +static int __rkisp1_ext_params_config(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_cfg *cfg, > > > + u32 block_group_mask) > > > +{ > > > + size_t block_offset = 0; > > > + > > > + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) { > > > + dev_dbg(params->rkisp1->dev, > > > + "Invalid parameters buffer size %llu\n", > > > + cfg->total_size); > > > + return -EINVAL; > > > + } > > > + > > > + /* Walk the list of parameter blocks and process them. */ > > > + while (block_offset < cfg->total_size) { > > > + const struct rkisp1_ext_params_handler *block_handler; > > > + struct rkisp1_ext_params_block_header *block; > > > + > > > + block = (struct rkisp1_ext_params_block_header *) > > > + &cfg->data[block_offset]; > > > + block_offset += block->size; > > > + > > > + /* > > > + * Validate the block id and make sure the block group is in > > > + * the list of groups to configure. > > > + */ > > > + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) { > > Use ARRAY_SIZE(). > I don't think I can use ARRAY_SIZE() on a enum type > > > + dev_dbg(params->rkisp1->dev, > > > + "Invalid parameters block type\n"); > > > + return -EINVAL; > > > + } > > > + > > > + block_handler = &rkisp1_ext_params_handlers[block->type]; > > > + if (!(block_handler->group & block_group_mask)) > > > + continue; > > > > > > So maybe something like > > > > > > if (block_handler->enable_reg) > > > > __rkisp1_block_handle_enable_disable(block->state, block_handler->enable_reg, > > block_handler->enable_val); > > > > > > here to move it out of the handlers. > > > > > + > > > + if (block->size != block_handler->size) { > > > + dev_dbg(params->rkisp1->dev, > > > + "Invalid parameters block size\n"); > > > + return -EINVAL; > > > + } > > > + > > > + block_handler->handler(params, block); > > It would be nicer to move validation to qbuf time, and applying > parameters at runtime. Maybe I'll find that later in the series :-) > > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int rkisp1_ext_params_config(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_cfg *cfg) > > > +{ > > > + return __rkisp1_ext_params_config(params, cfg, > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC | > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); > > > +} > > > + > > > +static int > > > +rkisp1_ext_params_other_meas_config(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_cfg *cfg) > > > +{ > > > + return __rkisp1_ext_params_config(params, cfg, > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); > > > +} > > > + > > > +static int rkisp1_ext_params_lsc_config(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_cfg *cfg) > > > +{ > > > + return __rkisp1_ext_params_config(params, cfg, > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > > > +} > > I'm tempted to inline this in the callers, I think it would be more > readable. > > > > + > > > static bool rkisp1_params_get_buffer(struct rkisp1_params *params, > > > struct rkisp1_buffer **buf, > > > - struct rkisp1_params_cfg **cfg) > > > + void **cfg) > > > { > > > if (list_empty(¶ms->params)) > > > return false; > > > @@ -1544,28 +2030,37 @@ static bool rkisp1_params_get_buffer(struct rkisp1_params *params, > > > > > > static void rkisp1_params_complete_buffer(struct rkisp1_params *params, > > > struct rkisp1_buffer *buf, > > > - unsigned int frame_sequence) > > > + unsigned int frame_sequence, > > > + enum vb2_buffer_state state) > > > { > > > list_del(&buf->queue); > > > > > > buf->vb.sequence = frame_sequence; > > > - vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE); > > > + vb2_buffer_done(&buf->vb.vb2_buf, state); > > > } > > > > > > void rkisp1_params_isr(struct rkisp1_device *rkisp1) > > > { > > > struct rkisp1_params *params = &rkisp1->params; > > > - struct rkisp1_params_cfg *new_params; > > > - struct rkisp1_buffer *cur_buf; > > > + struct rkisp1_buffer *buf; > > > > Why the rename? > > Because "cur" implies there's a "next" or a "prev". I can drop it though. > > > + int ret = 0; > > > + void *cfg; > > > > > > spin_lock(¶ms->config_lock); > > > > > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > > > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > > > goto unlock; > > > > > > - rkisp1_isp_isr_other_config(params, new_params); > > > - rkisp1_isp_isr_lsc_config(params, new_params); > > > - rkisp1_isp_isr_meas_config(params, new_params); > > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > > + rkisp1_isp_isr_other_config(params, cfg); > > > + rkisp1_isp_isr_lsc_config(params, cfg); > > > + rkisp1_isp_isr_meas_config(params, cfg); > > > + } else { > > > + ret = rkisp1_ext_params_config(params, cfg); > > > + } > > > + > > > + if (ret) > > > + goto complete_and_unlock; > > As validation should happen at qbuf time, I think it would be nicer to > reorder the patches to avoid introducing error handling here and > removing it later. > > > > > > > /* update shadow register immediately */ > > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > @@ -1579,8 +2074,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) > > > * indicate to userspace on which frame these parameters are being > > > * applied. > > > */ > > > - rkisp1_params_complete_buffer(params, cur_buf, > > > - rkisp1->isp.frame_sequence + 1); > > > +complete_and_unlock: > > > + rkisp1_params_complete_buffer(params, buf, > > > + rkisp1->isp.frame_sequence + 1, > > > + ret ? VB2_BUF_STATE_ERROR > > > + : VB2_BUF_STATE_DONE); > > > > > > unlock: > > > spin_unlock(¶ms->config_lock); > > > @@ -1631,8 +2129,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > > enum v4l2_ycbcr_encoding ycbcr_encoding) > > > { > > > struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config; > > > - struct rkisp1_params_cfg *new_params; > > > - struct rkisp1_buffer *cur_buf; > > > + struct rkisp1_buffer *buf; > > > + int ret = 0; > > > + void *cfg; > > > > > > params->quantization = quantization; > > > params->ycbcr_encoding = ycbcr_encoding; > > > @@ -1661,11 +2160,26 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > > > > > /* apply the first buffer if there is one already */ > > > > > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > > > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > > > goto unlock; > > > > > > - rkisp1_isp_isr_other_config(params, new_params); > > > - rkisp1_isp_isr_meas_config(params, new_params); > > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > > + rkisp1_isp_isr_other_config(params, cfg); > > > + rkisp1_isp_isr_meas_config(params, cfg); > > > + } else { > > > + ret = rkisp1_ext_params_other_meas_config(params, cfg); > > > + } > > > + > > > + if (ret) { > > > + /* > > > + * Complete the buffer in error state immediately. In case of no > > > + * error, the buffer will be completed in > > > + * rkisp1_params_post_configure(). > > > + */ > > > + rkisp1_params_complete_buffer(params, buf, 0, > > > + VB2_BUF_STATE_ERROR); > > > + goto unlock; > > > + } > > > > > > /* update shadow register immediately */ > > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > @@ -1677,8 +2191,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > > > > > void rkisp1_params_post_configure(struct rkisp1_params *params) > > > { > > > - struct rkisp1_params_cfg *new_params; > > > - struct rkisp1_buffer *cur_buf; > > > + struct rkisp1_buffer *buf; > > > > And likewise here? > > > > > + int ret = 0; > > > + void *cfg; > > > > > > spin_lock_irq(¶ms->config_lock); > > > > > > @@ -1691,16 +2206,24 @@ void rkisp1_params_post_configure(struct rkisp1_params *params) > > > * unconditionally. > > > */ > > > > > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > > > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > > > goto unlock; > > > > > > - rkisp1_isp_isr_lsc_config(params, new_params); > > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > > > + rkisp1_isp_isr_lsc_config(params, cfg); > > > + else > > > + ret = rkisp1_ext_params_lsc_config(params, cfg); > > > + > > > + if (ret) > > > + goto complete_and_unlock; > > > > > > /* update shadow register immediately */ > > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD); > > > > > > - rkisp1_params_complete_buffer(params, cur_buf, 0); > > > +complete_and_unlock: > > > + rkisp1_params_complete_buffer(params, buf, 0, ret ? VB2_BUF_STATE_ERROR > > > + : VB2_BUF_STATE_DONE); > > > > > > unlock: > > > spin_unlock_irq(¶ms->config_lock); > > -- > Regards, > > Laurent Pinchart
On Wed, Jun 19, 2024 at 05:46:59PM +0200, Jacopo Mondi wrote: > On Wed, Jun 12, 2024 at 06:42:01PM GMT, Laurent Pinchart wrote: > > On Wed, Jun 12, 2024 at 02:50:34PM +0100, Daniel Scally wrote: > > > On 05/06/2024 17:54, Jacopo Mondi wrote: > > > > Implement support in the rkisp1-params for the extensible configuration > > > > parameters format. > > > > > > > > Create a list of handlers for each ISP block that wraps the existing > > > > configuration functions and handles the ISP block enablement state. > > > > > > > > Parse the configuration parameters buffer in __rkisp1_ext_params_config > > > > and filter the enable blocks by group, to allow setting the 'other' and > > > > 'meas' groups separately from the 'lsc' group to support the > > > > pre/post-configure operations. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > .../platform/rockchip/rkisp1/rkisp1-params.c | 565 +++++++++++++++++- > > > > 1 file changed, 544 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > > > index 6f99c7dad758..3d78e643d0b8 100644 > > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > > > @@ -33,6 +33,10 @@ > > > > #define RKISP1_ISP_CC_COEFF(n) \ > > > > (RKISP1_CIF_ISP_CC_COEFF_0 + (n) * 4) > > > > > > > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS BIT(0) > > > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS BIT(1) > > > > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC BIT(2) > > > > Would "LSC" and "OTHERS" be enough ? The OTHERS and MEAS groups are > > always handled together. > > The existing code handles the three separately but I actually program > OTHER and MEAS together. > > Then we're left with "OTHER" and "LSC". I would like a more > descriptive name for "OTHER" Yes "other" doesn't sound great. At the same time, it's really everythink but LSC :-) > > > > + > > > > enum rkisp1_params_formats { > > > > RKISP1_PARAMS_FIXED, > > > > RKISP1_PARAMS_EXTENSIBLE, > > > > @@ -1529,9 +1533,491 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params, > > > > } > > > > } > > > > > > > > +/*------------------------------------------------------------------------------ > > > > + * Extensible parameters format handling > > > > + */ > > > > + > > > > +static void rkisp1_ext_params_bls(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_bls_config *bls = > > > > + (struct rkisp1_ext_params_bls_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_BLS_CTRL, > > > > + RKISP1_CIF_ISP_BLS_ENA); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_bls_config(params, &bls->bls_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_BLS_CTRL, > > > > + RKISP1_CIF_ISP_BLS_ENA); > > > > +} > > > > > > Most of the handlers have all but identical handling for the > > > enable/disable parts; is it worth factoring that out perhaps? The > > > register and bits could be added to struct rkisp1_ext_params_handler > > > and then a common pre-handler function could be called from > > > __rkisp1_ext_params_config() to set/clear the bits. > > > > I like the idea of generalizing things :-) The devil is in the details > > though, so if it causes annoying issues, we can skip it. > > There are slight differences in how blocks are enabled, most blocks > are enabled setting a bit, other have a dedicated function. I would True, I missed that. > keep them in the handlers for clarify (or I should add an .enable and > .disable function, but this seems more cumbersome than what I have > here) Hmmmm... That's tempting actually, you wouldn't have to add all the functions below if you did that. You could use the existing enable/disable handlers when they exist, and add a single implementation for the blocks that just set or clear a bit, with the register address and bit being fields in the rkisp1_ext_params_handler structure. > > If we drop RKISP1_EXT_PARAMS_BLOCK_NO_CHANGE, we should also cache the > > enable state. That should be easy to do in a bitmask indexed by block > > type, and will fit nicely in generalized handling of the enable bits. > > Done > > > > > + > > > > +static void rkisp1_ext_params_dpcc(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_dpcc_config *dpcc = > > > > + (struct rkisp1_ext_params_dpcc_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE, > > > > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_dpcc_config(params, &dpcc->dpcc_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPCC_MODE, > > > > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_sdg(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_sdg_config *sdg = > > > > + (struct rkisp1_ext_params_sdg_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > > > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_sdg_config(params, &sdg->sdg_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_lsc(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_lsc_config *lsc = > > > > + (struct rkisp1_ext_params_lsc_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_LSC_CTRL, > > > > + RKISP1_CIF_ISP_LSC_CTRL_ENA); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_lsc_config(params, &lsc->lsc_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_LSC_CTRL, > > > > + RKISP1_CIF_ISP_LSC_CTRL_ENA); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_awbg(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_awb_gain_config *awbg = > > > > + (struct rkisp1_ext_params_awb_gain_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > > > > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); > > > > + return; > > > > + } > > > > + > > > > + params->ops->awb_gain_config(params, &awbg->awb_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_flt(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_flt_config *flt = > > > > + (struct rkisp1_ext_params_flt_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_FILT_MODE, > > > > + RKISP1_CIF_ISP_FLT_ENA); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_flt_config(params, &flt->flt_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_FILT_MODE, > > > > + RKISP1_CIF_ISP_FLT_ENA); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_bdm(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_bdm_config *bdm = > > > > + (struct rkisp1_ext_params_bdm_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DEMOSAIC, > > > > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_bdm_config(params, &bdm->bdm_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DEMOSAIC, > > > > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_ctk(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_ctk_config *ctk = > > > > + (struct rkisp1_ext_params_ctk_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_ctk_enable(params, false); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_ctk_config(params, &ctk->ctk_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_ctk_enable(params, true); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_goc(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_goc_config *goc = > > > > + (struct rkisp1_ext_params_goc_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, > > > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); > > > > + return; > > > > + } > > > > + > > > > + params->ops->goc_config(params, &goc->goc_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_dpf(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_dpf_config *dpf = > > > > + (struct rkisp1_ext_params_dpf_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPF_MODE, > > > > + RKISP1_CIF_ISP_DPF_MODE_EN); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_dpf_config(params, &dpf->dpf_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPF_MODE, > > > > + RKISP1_CIF_ISP_DPF_MODE_EN); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_dpfs(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_dpf_strength_config *dpfs = > > > > + (struct rkisp1_ext_params_dpf_strength_config *)hdr; > > > > + > > > > + rkisp1_dpf_strength_config(params, &dpfs->dpf_strength_config); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_cproc(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_cproc_config *cproc = > > > > + (struct rkisp1_ext_params_cproc_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL, > > > > + RKISP1_CIF_C_PROC_CTR_ENABLE); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_cproc_config(params, &cproc->cproc_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_C_PROC_CTRL, > > > > + RKISP1_CIF_C_PROC_CTR_ENABLE); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_ie(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_ie_config *ie = > > > > + (struct rkisp1_ext_params_ie_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_ie_enable(params, false); > > > > + return; > > > > + } > > > > + > > > > + rkisp1_ie_config(params, &ie->ie_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_ie_enable(params, true); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_awbm(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_awb_meas_config *awbm = > > > > + (struct rkisp1_ext_params_awb_meas_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, > > > > + false); > > > > + return; > > > > + } > > > > + > > > > + params->ops->awb_meas_config(params, &awbm->awb_meas_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, > > > > + true); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_hstm(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_hst_config *hst = > > > > + (struct rkisp1_ext_params_hst_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + params->ops->hst_enable(params, &hst->hst_config, false); > > > > + return; > > > > + } > > > > + > > > > + params->ops->hst_config(params, &hst->hst_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + params->ops->hst_enable(params, &hst->hst_config, true); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_aecm(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_aec_config *aec = > > > > + (struct rkisp1_ext_params_aec_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_EXP_CTRL, > > > > + RKISP1_CIF_ISP_EXP_ENA); > > > > + return; > > > > + } > > > > + > > > > + params->ops->aec_config(params, &aec->aec_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_EXP_CTRL, > > > > + RKISP1_CIF_ISP_EXP_ENA); > > > > +} > > > > + > > > > +static void rkisp1_ext_params_afcm(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr) > > > > +{ > > > > + struct rkisp1_ext_params_afc_config *afc = > > > > + (struct rkisp1_ext_params_afc_config *)hdr; > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > > > > + RKISP1_CIF_ISP_AFM_ENA); > > > > + return; > > > > + } > > > > + > > > > + params->ops->afm_config(params, &afc->afc_config); > > > > + > > > > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) > > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > > > > + RKISP1_CIF_ISP_AFM_ENA); > > > > +} > > > > + > > > > +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_block_header *hdr); > > > > + > > > > +static const struct rkisp1_ext_params_handler { > > > > + size_t size; > > > > + rkisp1_block_handler handler; > > > > + unsigned int group; > > > > We'll have to extend this structure with a bitmask of the ISP versions > > each block supports. That can be done later. > > > > > > +} rkisp1_ext_params_handlers[] = { > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = { > > > > + .size = sizeof(struct rkisp1_ext_params_bls_config), > > > > + .handler = rkisp1_ext_params_bls, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = { > > > > + .size = sizeof(struct rkisp1_ext_params_dpcc_config), > > > > + .handler = rkisp1_ext_params_dpcc, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG] = { > > > > + .size = sizeof(struct rkisp1_ext_params_sdg_config), > > > > + .handler = rkisp1_ext_params_sdg, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS] = { > > > > + .size = > > > > + sizeof(struct rkisp1_ext_params_awb_gain_config), > > > > + .handler = rkisp1_ext_params_awbg, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT] = { > > > > + .size = sizeof(struct rkisp1_ext_params_flt_config), > > > > + .handler = rkisp1_ext_params_flt, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM] = { > > > > + .size = sizeof(struct rkisp1_ext_params_bdm_config), > > > > + .handler = rkisp1_ext_params_bdm, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK] = { > > > > + .size = sizeof(struct rkisp1_ext_params_ctk_config), > > > > + .handler = rkisp1_ext_params_ctk, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC] = { > > > > + .size = sizeof(struct rkisp1_ext_params_goc_config), > > > > + .handler = rkisp1_ext_params_goc, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF] = { > > > > + .size = sizeof(struct rkisp1_ext_params_dpf_config), > > > > + .handler = rkisp1_ext_params_dpf, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGHT] = { > > > > + .size = > > > > + sizeof(struct rkisp1_ext_params_dpf_strength_config), > > > > + .handler = rkisp1_ext_params_dpfs, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC] = { > > > > + .size = sizeof(struct rkisp1_ext_params_cproc_config), > > > > + .handler = rkisp1_ext_params_cproc, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_IE] = { > > > > + .size = sizeof(struct rkisp1_ext_params_ie_config), > > > > + .handler = rkisp1_ext_params_ie, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC] = { > > > > + .size = sizeof(struct rkisp1_ext_params_lsc_config), > > > > + .handler = rkisp1_ext_params_lsc, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS] = { > > > > + .size = > > > > + sizeof(struct rkisp1_ext_params_awb_meas_config), > > > > + .handler = rkisp1_ext_params_awbm, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS] = { > > > > + .size = sizeof(struct rkisp1_ext_params_hst_config), > > > > + .handler = rkisp1_ext_params_hstm, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS] = { > > > > + .size = sizeof(struct rkisp1_ext_params_aec_config), > > > > + .handler = rkisp1_ext_params_aecm, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > > > + }, > > > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS] = { > > > > + .size = sizeof(struct rkisp1_ext_params_afc_config), > > > > + .handler = rkisp1_ext_params_afcm, > > > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS > > > > + }, > > > > +}; > > > > + > > > > +static int __rkisp1_ext_params_config(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_cfg *cfg, > > > > + u32 block_group_mask) > > > > +{ > > > > + size_t block_offset = 0; > > > > + > > > > + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) { > > > > + dev_dbg(params->rkisp1->dev, > > > > + "Invalid parameters buffer size %llu\n", > > > > + cfg->total_size); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + /* Walk the list of parameter blocks and process them. */ > > > > + while (block_offset < cfg->total_size) { > > > > + const struct rkisp1_ext_params_handler *block_handler; > > > > + struct rkisp1_ext_params_block_header *block; > > > > + > > > > + block = (struct rkisp1_ext_params_block_header *) > > > > + &cfg->data[block_offset]; > > > > + block_offset += block->size; > > > > + > > > > + /* > > > > + * Validate the block id and make sure the block group is in > > > > + * the list of groups to configure. > > > > + */ > > > > + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) { > > > > Use ARRAY_SIZE(). > > I don't think I can use ARRAY_SIZE() on a enum type No, but you can on rkisp1_ext_params_handlers, which is the array you index by block->type just below. > > > > + dev_dbg(params->rkisp1->dev, > > > > + "Invalid parameters block type\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + block_handler = &rkisp1_ext_params_handlers[block->type]; > > > > + if (!(block_handler->group & block_group_mask)) > > > > + continue; > > > > > > > > > So maybe something like > > > > > > > > > if (block_handler->enable_reg) > > > > > > __rkisp1_block_handle_enable_disable(block->state, block_handler->enable_reg, > > > block_handler->enable_val); > > > > > > > > > here to move it out of the handlers. > > > > > > > + > > > > + if (block->size != block_handler->size) { > > > > + dev_dbg(params->rkisp1->dev, > > > > + "Invalid parameters block size\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + block_handler->handler(params, block); > > > > It would be nicer to move validation to qbuf time, and applying > > parameters at runtime. Maybe I'll find that later in the series :-) > > > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int rkisp1_ext_params_config(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_cfg *cfg) > > > > +{ > > > > + return __rkisp1_ext_params_config(params, cfg, > > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC | > > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); > > > > +} > > > > + > > > > +static int > > > > +rkisp1_ext_params_other_meas_config(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_cfg *cfg) > > > > +{ > > > > + return __rkisp1_ext_params_config(params, cfg, > > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); > > > > +} > > > > + > > > > +static int rkisp1_ext_params_lsc_config(struct rkisp1_params *params, > > > > + struct rkisp1_ext_params_cfg *cfg) > > > > +{ > > > > + return __rkisp1_ext_params_config(params, cfg, > > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > > > > +} > > > > I'm tempted to inline this in the callers, I think it would be more > > readable. > > > > > > + > > > > static bool rkisp1_params_get_buffer(struct rkisp1_params *params, > > > > struct rkisp1_buffer **buf, > > > > - struct rkisp1_params_cfg **cfg) > > > > + void **cfg) > > > > { > > > > if (list_empty(¶ms->params)) > > > > return false; > > > > @@ -1544,28 +2030,37 @@ static bool rkisp1_params_get_buffer(struct rkisp1_params *params, > > > > > > > > static void rkisp1_params_complete_buffer(struct rkisp1_params *params, > > > > struct rkisp1_buffer *buf, > > > > - unsigned int frame_sequence) > > > > + unsigned int frame_sequence, > > > > + enum vb2_buffer_state state) > > > > { > > > > list_del(&buf->queue); > > > > > > > > buf->vb.sequence = frame_sequence; > > > > - vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE); > > > > + vb2_buffer_done(&buf->vb.vb2_buf, state); > > > > } > > > > > > > > void rkisp1_params_isr(struct rkisp1_device *rkisp1) > > > > { > > > > struct rkisp1_params *params = &rkisp1->params; > > > > - struct rkisp1_params_cfg *new_params; > > > > - struct rkisp1_buffer *cur_buf; > > > > + struct rkisp1_buffer *buf; > > > > > > Why the rename? > > Because "cur" implies there's a "next" or a "prev". > I can drop it though. > > > > > + int ret = 0; > > > > + void *cfg; > > > > > > > > spin_lock(¶ms->config_lock); > > > > > > > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > > > > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > > > > goto unlock; > > > > > > > > - rkisp1_isp_isr_other_config(params, new_params); > > > > - rkisp1_isp_isr_lsc_config(params, new_params); > > > > - rkisp1_isp_isr_meas_config(params, new_params); > > > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > > > + rkisp1_isp_isr_other_config(params, cfg); > > > > + rkisp1_isp_isr_lsc_config(params, cfg); > > > > + rkisp1_isp_isr_meas_config(params, cfg); > > > > + } else { > > > > + ret = rkisp1_ext_params_config(params, cfg); > > > > + } > > > > + > > > > + if (ret) > > > > + goto complete_and_unlock; > > > > As validation should happen at qbuf time, I think it would be nicer to > > reorder the patches to avoid introducing error handling here and > > removing it later. > > > > > > > > > > /* update shadow register immediately */ > > > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > > @@ -1579,8 +2074,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) > > > > * indicate to userspace on which frame these parameters are being > > > > * applied. > > > > */ > > > > - rkisp1_params_complete_buffer(params, cur_buf, > > > > - rkisp1->isp.frame_sequence + 1); > > > > +complete_and_unlock: > > > > + rkisp1_params_complete_buffer(params, buf, > > > > + rkisp1->isp.frame_sequence + 1, > > > > + ret ? VB2_BUF_STATE_ERROR > > > > + : VB2_BUF_STATE_DONE); > > > > > > > > unlock: > > > > spin_unlock(¶ms->config_lock); > > > > @@ -1631,8 +2129,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > > > enum v4l2_ycbcr_encoding ycbcr_encoding) > > > > { > > > > struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config; > > > > - struct rkisp1_params_cfg *new_params; > > > > - struct rkisp1_buffer *cur_buf; > > > > + struct rkisp1_buffer *buf; > > > > + int ret = 0; > > > > + void *cfg; > > > > > > > > params->quantization = quantization; > > > > params->ycbcr_encoding = ycbcr_encoding; > > > > @@ -1661,11 +2160,26 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > > > > > > > /* apply the first buffer if there is one already */ > > > > > > > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > > > > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > > > > goto unlock; > > > > > > > > - rkisp1_isp_isr_other_config(params, new_params); > > > > - rkisp1_isp_isr_meas_config(params, new_params); > > > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > > > + rkisp1_isp_isr_other_config(params, cfg); > > > > + rkisp1_isp_isr_meas_config(params, cfg); > > > > + } else { > > > > + ret = rkisp1_ext_params_other_meas_config(params, cfg); > > > > + } > > > > + > > > > + if (ret) { > > > > + /* > > > > + * Complete the buffer in error state immediately. In case of no > > > > + * error, the buffer will be completed in > > > > + * rkisp1_params_post_configure(). > > > > + */ > > > > + rkisp1_params_complete_buffer(params, buf, 0, > > > > + VB2_BUF_STATE_ERROR); > > > > + goto unlock; > > > > + } > > > > > > > > /* update shadow register immediately */ > > > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > > @@ -1677,8 +2191,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > > > > > > > void rkisp1_params_post_configure(struct rkisp1_params *params) > > > > { > > > > - struct rkisp1_params_cfg *new_params; > > > > - struct rkisp1_buffer *cur_buf; > > > > + struct rkisp1_buffer *buf; > > > > > > And likewise here? > > > > > > > + int ret = 0; > > > > + void *cfg; > > > > > > > > spin_lock_irq(¶ms->config_lock); > > > > > > > > @@ -1691,16 +2206,24 @@ void rkisp1_params_post_configure(struct rkisp1_params *params) > > > > * unconditionally. > > > > */ > > > > > > > > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) > > > > + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) > > > > goto unlock; > > > > > > > > - rkisp1_isp_isr_lsc_config(params, new_params); > > > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > > > > + rkisp1_isp_isr_lsc_config(params, cfg); > > > > + else > > > > + ret = rkisp1_ext_params_lsc_config(params, cfg); > > > > + > > > > + if (ret) > > > > + goto complete_and_unlock; > > > > > > > > /* update shadow register immediately */ > > > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > > RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD); > > > > > > > > - rkisp1_params_complete_buffer(params, cur_buf, 0); > > > > +complete_and_unlock: > > > > + rkisp1_params_complete_buffer(params, buf, 0, ret ? VB2_BUF_STATE_ERROR > > > > + : VB2_BUF_STATE_DONE); > > > > > > > > unlock: > > > > spin_unlock_irq(¶ms->config_lock);
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c index 6f99c7dad758..3d78e643d0b8 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c @@ -33,6 +33,10 @@ #define RKISP1_ISP_CC_COEFF(n) \ (RKISP1_CIF_ISP_CC_COEFF_0 + (n) * 4) +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS BIT(0) +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS BIT(1) +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC BIT(2) + enum rkisp1_params_formats { RKISP1_PARAMS_FIXED, RKISP1_PARAMS_EXTENSIBLE, @@ -1529,9 +1533,491 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params, } } +/*------------------------------------------------------------------------------ + * Extensible parameters format handling + */ + +static void rkisp1_ext_params_bls(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_bls_config *bls = + (struct rkisp1_ext_params_bls_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_BLS_CTRL, + RKISP1_CIF_ISP_BLS_ENA); + return; + } + + rkisp1_bls_config(params, &bls->bls_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_BLS_CTRL, + RKISP1_CIF_ISP_BLS_ENA); +} + +static void rkisp1_ext_params_dpcc(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_dpcc_config *dpcc = + (struct rkisp1_ext_params_dpcc_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE, + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); + return; + } + + rkisp1_dpcc_config(params, &dpcc->dpcc_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPCC_MODE, + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE); +} + +static void rkisp1_ext_params_sdg(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_sdg_config *sdg = + (struct rkisp1_ext_params_sdg_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); + return; + } + + rkisp1_sdg_config(params, &sdg->sdg_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA); +} + +static void rkisp1_ext_params_lsc(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_lsc_config *lsc = + (struct rkisp1_ext_params_lsc_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_LSC_CTRL, + RKISP1_CIF_ISP_LSC_CTRL_ENA); + return; + } + + rkisp1_lsc_config(params, &lsc->lsc_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_LSC_CTRL, + RKISP1_CIF_ISP_LSC_CTRL_ENA); +} + +static void rkisp1_ext_params_awbg(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_awb_gain_config *awbg = + (struct rkisp1_ext_params_awb_gain_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); + return; + } + + params->ops->awb_gain_config(params, &awbg->awb_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA); +} + +static void rkisp1_ext_params_flt(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_flt_config *flt = + (struct rkisp1_ext_params_flt_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_FILT_MODE, + RKISP1_CIF_ISP_FLT_ENA); + return; + } + + rkisp1_flt_config(params, &flt->flt_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_FILT_MODE, + RKISP1_CIF_ISP_FLT_ENA); +} + +static void rkisp1_ext_params_bdm(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_bdm_config *bdm = + (struct rkisp1_ext_params_bdm_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DEMOSAIC, + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); + return; + } + + rkisp1_bdm_config(params, &bdm->bdm_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DEMOSAIC, + RKISP1_CIF_ISP_DEMOSAIC_BYPASS); +} + +static void rkisp1_ext_params_ctk(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_ctk_config *ctk = + (struct rkisp1_ext_params_ctk_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_ctk_enable(params, false); + return; + } + + rkisp1_ctk_config(params, &ctk->ctk_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_ctk_enable(params, true); +} + +static void rkisp1_ext_params_goc(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_goc_config *goc = + (struct rkisp1_ext_params_goc_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL, + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); + return; + } + + params->ops->goc_config(params, &goc->goc_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA); +} + +static void rkisp1_ext_params_dpf(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_dpf_config *dpf = + (struct rkisp1_ext_params_dpf_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPF_MODE, + RKISP1_CIF_ISP_DPF_MODE_EN); + return; + } + + rkisp1_dpf_config(params, &dpf->dpf_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPF_MODE, + RKISP1_CIF_ISP_DPF_MODE_EN); +} + +static void rkisp1_ext_params_dpfs(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_dpf_strength_config *dpfs = + (struct rkisp1_ext_params_dpf_strength_config *)hdr; + + rkisp1_dpf_strength_config(params, &dpfs->dpf_strength_config); +} + +static void rkisp1_ext_params_cproc(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_cproc_config *cproc = + (struct rkisp1_ext_params_cproc_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL, + RKISP1_CIF_C_PROC_CTR_ENABLE); + return; + } + + rkisp1_cproc_config(params, &cproc->cproc_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_C_PROC_CTRL, + RKISP1_CIF_C_PROC_CTR_ENABLE); +} + +static void rkisp1_ext_params_ie(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_ie_config *ie = + (struct rkisp1_ext_params_ie_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_ie_enable(params, false); + return; + } + + rkisp1_ie_config(params, &ie->ie_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_ie_enable(params, true); +} + +static void rkisp1_ext_params_awbm(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_awb_meas_config *awbm = + (struct rkisp1_ext_params_awb_meas_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, + false); + return; + } + + params->ops->awb_meas_config(params, &awbm->awb_meas_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + params->ops->awb_meas_enable(params, &awbm->awb_meas_config, + true); +} + +static void rkisp1_ext_params_hstm(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_hst_config *hst = + (struct rkisp1_ext_params_hst_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + params->ops->hst_enable(params, &hst->hst_config, false); + return; + } + + params->ops->hst_config(params, &hst->hst_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + params->ops->hst_enable(params, &hst->hst_config, true); +} + +static void rkisp1_ext_params_aecm(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_aec_config *aec = + (struct rkisp1_ext_params_aec_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_EXP_CTRL, + RKISP1_CIF_ISP_EXP_ENA); + return; + } + + params->ops->aec_config(params, &aec->aec_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_EXP_CTRL, + RKISP1_CIF_ISP_EXP_ENA); +} + +static void rkisp1_ext_params_afcm(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr) +{ + struct rkisp1_ext_params_afc_config *afc = + (struct rkisp1_ext_params_afc_config *)hdr; + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_AFM_CTRL, + RKISP1_CIF_ISP_AFM_ENA); + return; + } + + params->ops->afm_config(params, &afc->afc_config); + + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE) + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL, + RKISP1_CIF_ISP_AFM_ENA); +} + +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params, + struct rkisp1_ext_params_block_header *hdr); + +static const struct rkisp1_ext_params_handler { + size_t size; + rkisp1_block_handler handler; + unsigned int group; +} rkisp1_ext_params_handlers[] = { + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = { + .size = sizeof(struct rkisp1_ext_params_bls_config), + .handler = rkisp1_ext_params_bls, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = { + .size = sizeof(struct rkisp1_ext_params_dpcc_config), + .handler = rkisp1_ext_params_dpcc, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG] = { + .size = sizeof(struct rkisp1_ext_params_sdg_config), + .handler = rkisp1_ext_params_sdg, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS] = { + .size = + sizeof(struct rkisp1_ext_params_awb_gain_config), + .handler = rkisp1_ext_params_awbg, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT] = { + .size = sizeof(struct rkisp1_ext_params_flt_config), + .handler = rkisp1_ext_params_flt, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM] = { + .size = sizeof(struct rkisp1_ext_params_bdm_config), + .handler = rkisp1_ext_params_bdm, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK] = { + .size = sizeof(struct rkisp1_ext_params_ctk_config), + .handler = rkisp1_ext_params_ctk, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC] = { + .size = sizeof(struct rkisp1_ext_params_goc_config), + .handler = rkisp1_ext_params_goc, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF] = { + .size = sizeof(struct rkisp1_ext_params_dpf_config), + .handler = rkisp1_ext_params_dpf, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGHT] = { + .size = + sizeof(struct rkisp1_ext_params_dpf_strength_config), + .handler = rkisp1_ext_params_dpfs, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC] = { + .size = sizeof(struct rkisp1_ext_params_cproc_config), + .handler = rkisp1_ext_params_cproc, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_IE] = { + .size = sizeof(struct rkisp1_ext_params_ie_config), + .handler = rkisp1_ext_params_ie, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC] = { + .size = sizeof(struct rkisp1_ext_params_lsc_config), + .handler = rkisp1_ext_params_lsc, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS] = { + .size = + sizeof(struct rkisp1_ext_params_awb_meas_config), + .handler = rkisp1_ext_params_awbm, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS] = { + .size = sizeof(struct rkisp1_ext_params_hst_config), + .handler = rkisp1_ext_params_hstm, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS] = { + .size = sizeof(struct rkisp1_ext_params_aec_config), + .handler = rkisp1_ext_params_aecm, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS + }, + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS] = { + .size = sizeof(struct rkisp1_ext_params_afc_config), + .handler = rkisp1_ext_params_afcm, + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS + }, +}; + +static int __rkisp1_ext_params_config(struct rkisp1_params *params, + struct rkisp1_ext_params_cfg *cfg, + u32 block_group_mask) +{ + size_t block_offset = 0; + + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) { + dev_dbg(params->rkisp1->dev, + "Invalid parameters buffer size %llu\n", + cfg->total_size); + return -EINVAL; + } + + /* Walk the list of parameter blocks and process them. */ + while (block_offset < cfg->total_size) { + const struct rkisp1_ext_params_handler *block_handler; + struct rkisp1_ext_params_block_header *block; + + block = (struct rkisp1_ext_params_block_header *) + &cfg->data[block_offset]; + block_offset += block->size; + + /* + * Validate the block id and make sure the block group is in + * the list of groups to configure. + */ + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) { + dev_dbg(params->rkisp1->dev, + "Invalid parameters block type\n"); + return -EINVAL; + } + + block_handler = &rkisp1_ext_params_handlers[block->type]; + if (!(block_handler->group & block_group_mask)) + continue; + + if (block->size != block_handler->size) { + dev_dbg(params->rkisp1->dev, + "Invalid parameters block size\n"); + return -EINVAL; + } + + block_handler->handler(params, block); + } + + return 0; +} + +static int rkisp1_ext_params_config(struct rkisp1_params *params, + struct rkisp1_ext_params_cfg *cfg) +{ + return __rkisp1_ext_params_config(params, cfg, + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC | + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); +} + +static int +rkisp1_ext_params_other_meas_config(struct rkisp1_params *params, + struct rkisp1_ext_params_cfg *cfg) +{ + return __rkisp1_ext_params_config(params, cfg, + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS); +} + +static int rkisp1_ext_params_lsc_config(struct rkisp1_params *params, + struct rkisp1_ext_params_cfg *cfg) +{ + return __rkisp1_ext_params_config(params, cfg, + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); +} + static bool rkisp1_params_get_buffer(struct rkisp1_params *params, struct rkisp1_buffer **buf, - struct rkisp1_params_cfg **cfg) + void **cfg) { if (list_empty(¶ms->params)) return false; @@ -1544,28 +2030,37 @@ static bool rkisp1_params_get_buffer(struct rkisp1_params *params, static void rkisp1_params_complete_buffer(struct rkisp1_params *params, struct rkisp1_buffer *buf, - unsigned int frame_sequence) + unsigned int frame_sequence, + enum vb2_buffer_state state) { list_del(&buf->queue); buf->vb.sequence = frame_sequence; - vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE); + vb2_buffer_done(&buf->vb.vb2_buf, state); } void rkisp1_params_isr(struct rkisp1_device *rkisp1) { struct rkisp1_params *params = &rkisp1->params; - struct rkisp1_params_cfg *new_params; - struct rkisp1_buffer *cur_buf; + struct rkisp1_buffer *buf; + int ret = 0; + void *cfg; spin_lock(¶ms->config_lock); - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) goto unlock; - rkisp1_isp_isr_other_config(params, new_params); - rkisp1_isp_isr_lsc_config(params, new_params); - rkisp1_isp_isr_meas_config(params, new_params); + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { + rkisp1_isp_isr_other_config(params, cfg); + rkisp1_isp_isr_lsc_config(params, cfg); + rkisp1_isp_isr_meas_config(params, cfg); + } else { + ret = rkisp1_ext_params_config(params, cfg); + } + + if (ret) + goto complete_and_unlock; /* update shadow register immediately */ rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, @@ -1579,8 +2074,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) * indicate to userspace on which frame these parameters are being * applied. */ - rkisp1_params_complete_buffer(params, cur_buf, - rkisp1->isp.frame_sequence + 1); +complete_and_unlock: + rkisp1_params_complete_buffer(params, buf, + rkisp1->isp.frame_sequence + 1, + ret ? VB2_BUF_STATE_ERROR + : VB2_BUF_STATE_DONE); unlock: spin_unlock(¶ms->config_lock); @@ -1631,8 +2129,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, enum v4l2_ycbcr_encoding ycbcr_encoding) { struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config; - struct rkisp1_params_cfg *new_params; - struct rkisp1_buffer *cur_buf; + struct rkisp1_buffer *buf; + int ret = 0; + void *cfg; params->quantization = quantization; params->ycbcr_encoding = ycbcr_encoding; @@ -1661,11 +2160,26 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, /* apply the first buffer if there is one already */ - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) goto unlock; - rkisp1_isp_isr_other_config(params, new_params); - rkisp1_isp_isr_meas_config(params, new_params); + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { + rkisp1_isp_isr_other_config(params, cfg); + rkisp1_isp_isr_meas_config(params, cfg); + } else { + ret = rkisp1_ext_params_other_meas_config(params, cfg); + } + + if (ret) { + /* + * Complete the buffer in error state immediately. In case of no + * error, the buffer will be completed in + * rkisp1_params_post_configure(). + */ + rkisp1_params_complete_buffer(params, buf, 0, + VB2_BUF_STATE_ERROR); + goto unlock; + } /* update shadow register immediately */ rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, @@ -1677,8 +2191,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, void rkisp1_params_post_configure(struct rkisp1_params *params) { - struct rkisp1_params_cfg *new_params; - struct rkisp1_buffer *cur_buf; + struct rkisp1_buffer *buf; + int ret = 0; + void *cfg; spin_lock_irq(¶ms->config_lock); @@ -1691,16 +2206,24 @@ void rkisp1_params_post_configure(struct rkisp1_params *params) * unconditionally. */ - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params)) + if (!rkisp1_params_get_buffer(params, &buf, &cfg)) goto unlock; - rkisp1_isp_isr_lsc_config(params, new_params); + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) + rkisp1_isp_isr_lsc_config(params, cfg); + else + ret = rkisp1_ext_params_lsc_config(params, cfg); + + if (ret) + goto complete_and_unlock; /* update shadow register immediately */ rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD); - rkisp1_params_complete_buffer(params, cur_buf, 0); +complete_and_unlock: + rkisp1_params_complete_buffer(params, buf, 0, ret ? VB2_BUF_STATE_ERROR + : VB2_BUF_STATE_DONE); unlock: spin_unlock_irq(¶ms->config_lock);
Implement support in the rkisp1-params for the extensible configuration parameters format. Create a list of handlers for each ISP block that wraps the existing configuration functions and handles the ISP block enablement state. Parse the configuration parameters buffer in __rkisp1_ext_params_config and filter the enable blocks by group, to allow setting the 'other' and 'meas' groups separately from the 'lsc' group to support the pre/post-configure operations. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- .../platform/rockchip/rkisp1/rkisp1-params.c | 565 +++++++++++++++++- 1 file changed, 544 insertions(+), 21 deletions(-)