| Message ID | 20240621145406.119088-7-jacopo.mondi@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: rkisp1: Implement support for extensible parameters | expand |
Hi Jacopo, Thank you for the patch. On Fri, Jun 21, 2024 at 04:54:04PM +0200, Jacopo Mondi wrote: > Implement support in 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. > > Parse the configuration parameters buffer in rkisp1_ext_params_config > and filter the enable blocks by group, to allow setting the 'other' > groups separately from the 'lsc' group to support the pre/post-configure > operations. > > Implement the vb2 buf_out_validate() operation to validate the > extensible format buffer content at qbuf time. Is there a particular reason to do so in .buf_out_validate() instead of .buf_prepare() ? > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > .../platform/rockchip/rkisp1/rkisp1-common.h | 3 + > .../platform/rockchip/rkisp1/rkisp1-params.c | 553 +++++++++++++++++- > 2 files changed, 540 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > index ccd2065351b4..bffd936f989a 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > @@ -390,6 +390,7 @@ struct rkisp1_params_ops { > * @quantization: the quantization configured on the isp's src pad > * @ycbcr_encoding the YCbCr encoding > * @raw_type: the bayer pattern on the isp video sink pad > + * @enabled_blocks: bitmask of enabled ISP blocks > */ > struct rkisp1_params { > struct rkisp1_vdev_node vnode; > @@ -404,6 +405,8 @@ struct rkisp1_params { > enum v4l2_quantization quantization; > enum v4l2_ycbcr_encoding ycbcr_encoding; > enum rkisp1_fmt_raw_pat_type raw_type; > + > + u32 enabled_blocks; > }; > > /* > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > index e38d2da994f5..f3ea70c7e0c1 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > @@ -35,6 +35,9 @@ > #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_LSC BIT(1) > + > enum rkisp1_params_formats { > RKISP1_PARAMS_FIXED, > RKISP1_PARAMS_EXTENSIBLE, > @@ -1529,6 +1532,454 @@ rkisp1_params_get_buffer(struct rkisp1_params *params) > queue); > } > > +/*------------------------------------------------------------------------------ > + * 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS))) > + 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->enable == 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 (!(params->enabled_blocks & > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC))) > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG))) > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC))) > + 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->enable == 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 (!(params->enabled_blocks & > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS))) > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT))) > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM))) > + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_ctk_enable(params, false); > + return; > + } > + > + rkisp1_ctk_config(params, &ctk->ctk_config); > + > + if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK))) > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC))) > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF))) > + 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->enable == 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 (!(params->enabled_blocks & > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC))) > + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + rkisp1_ie_enable(params, false); > + return; > + } > + > + rkisp1_ie_config(params, &ie->ie_config); > + > + if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_IE))) > + 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->enable == 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 (!(params->enabled_blocks & > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS))) > + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > + params->ops->hst_enable(params, &hst->hst_config, false); > + return; > + } > + > + params->ops->hst_config(params, &hst->hst_config); > + > + if (!(params->enabled_blocks & > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS))) > + 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->enable == 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 (!(params->enabled_blocks & > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS))) > + 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->enable == 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 (!(params->enabled_blocks & > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS))) > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > + RKISP1_CIF_ISP_AFM_ENA); > +} I still think we could get rid of most of these functions by adding .enable(), .configure() and .disable() operations to rkisp1_ext_params_handler. That can be done later. > + > +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params, > + struct rkisp1_ext_params_block_header *hdr); If it doesn't cause any issue, I'd make the hdr const. > + > +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_STRENGTH] = { > + .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_OTHERS > + }, > + [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_OTHERS > + }, > + [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_OTHERS > + }, > + [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_OTHERS > + }, > +}; > + > +static void rkisp1_ext_params_config(struct rkisp1_params *params, > + struct rkisp1_ext_params_cfg *cfg, > + u32 block_group_mask) > +{ > + size_t block_offset = 0; > + > + if (WARN_ON(!cfg)) > + return; > + > + /* 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; const > + > + block = (struct rkisp1_ext_params_block_header *) > + &cfg->data[block_offset]; > + block_offset += block->size; > + > + /* Make sure the block is in the list of groups to configure. */ > + block_handler = &rkisp1_ext_params_handlers[block->type]; > + if (!(block_handler->group & block_group_mask)) > + continue; > + > + block_handler->handler(params, block); > + > + if (block->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) > + params->enabled_blocks &= ~BIT(block->type); > + else > + params->enabled_blocks |= BIT(block->type); > + } > +} > + > static void rkisp1_params_complete_buffer(struct rkisp1_params *params, > struct rkisp1_params_buffer *buf, > unsigned int frame_sequence) > @@ -1550,9 +2001,15 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) > if (!cur_buf) > goto unlock; > > - rkisp1_isp_isr_other_config(params, cur_buf->cfg); > - rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > - rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > + rkisp1_isp_isr_other_config(params, cur_buf->cfg); > + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > + rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > + } else { > + rkisp1_ext_params_config(params, cur_buf->cfg, > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > + } > > /* update shadow register immediately */ > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > @@ -1651,8 +2108,13 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > if (!cur_buf) > goto unlock; > > - rkisp1_isp_isr_other_config(params, cur_buf->cfg); > - rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > + rkisp1_isp_isr_other_config(params, cur_buf->cfg); > + rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > + } else { > + rkisp1_ext_params_config(params, cur_buf->cfg, > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS); > + } > > /* update shadow register immediately */ > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > @@ -1680,7 +2142,11 @@ void rkisp1_params_post_configure(struct rkisp1_params *params) > if (!cur_buf) > goto unlock; > > - rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > + else > + rkisp1_ext_params_config(params, cur_buf->cfg, > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > > /* update shadow register immediately */ > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > @@ -1874,10 +2340,6 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb) > > static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb) > { > - struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > - struct rkisp1_params_buffer *params_buf = > - container_of(vbuf, struct rkisp1_params_buffer, vb); > - void *cfg = vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0); > struct rkisp1_params *params = vb->vb2_queue->drv_priv; > > if (vb2_plane_size(vb, 0) < params->metafmt->buffersize) > @@ -1885,12 +2347,6 @@ static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb) > > vb2_set_plane_payload(vb, 0, params->metafmt->buffersize); > > - /* > - * Copy the parameters buffer to the internal scratch buffer to avoid > - * userspace modifying the buffer content while the driver processes it. > - */ > - memcpy(params_buf->cfg, cfg, params->metafmt->buffersize); > - > return 0; > } > > @@ -1911,12 +2367,77 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq) > > list_for_each_entry(buf, &tmp_list, queue) > vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); > + > + params->enabled_blocks = 0; > +} > + > +static int rkisp1_params_validate_ext_params(struct rkisp1_params *params, > + struct rkisp1_ext_params_cfg *cfg) > +{ > + size_t block_offset = 0; > + > + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) { > + dev_dbg(params->rkisp1->dev, > + "Invalid parameters buffer size %u\n", > + cfg->total_size); > + return -EINVAL; > + } Following my review comments on patch 5/7, you should also validate that offsetof(struct rkisp1_ext_params_cfg, data) + cfg->total_size >= plane payload. > + > + /* Walk the list of parameter blocks and validate them. */ > + while (block_offset < cfg->total_size) { while (cfg->total_size - block_offset >= sizeof(struct rkisp1_ext_params_block_header)) { Otherwise there could be just one byte left, and dereferencing block below would read uninitialized memory. > + const struct rkisp1_ext_params_handler *hdlr; Maybe handler, it's not much longer, and is more readable. > + struct rkisp1_ext_params_block_header *block; const > + > + block = (struct rkisp1_ext_params_block_header *) > + &cfg->data[block_offset]; > + > + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) { if (block->type >= ARRAY_SIZE(rkisp1_ext_params_handlers)) { which allows you to drop RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL from the UAPI header. > + dev_dbg(params->rkisp1->dev, > + "Invalid parameters block type\n"); > + return -EINVAL; > + } > + > + hdlr = &rkisp1_ext_params_handlers[block->type]; > + if (block->size != hdlr->size) { > + dev_dbg(params->rkisp1->dev, > + "Invalid parameters block size\n"); > + return -EINVAL; > + } You need to test here that block->size >= cfg->total_size - block_offset. It may be easier to add a new local variable at the top of the function size_t remaining_size = cfg->total_size; The loop would then become while (remaining_size >= sizeof(struct rkisp1_ext_params_block_header)) { const struct rkisp1_ext_params_block_header *block; const struct rkisp1_ext_params_handler *handler; block = (struct rkisp1_ext_params_block_header *) &cfg->data[block_offset]; if (block->type >= ARRAY_SIZE(rkisp1_ext_params_handlers)) { dev_dbg(params->rkisp1->dev, "Invalid parameters block type\n"); return -EINVAL; } if (block->size > remaining_size) { dev_dbg(params->rkisp1->dev, "Premature end of parameters data\n"); return -EINVAL; } handler = &rkisp1_ext_params_handlers[block->type]; if (block->size != handler->size) { dev_dbg(params->rkisp1->dev, "Invalid parameters block size\n"); return -EINVAL; } block_offset += block->size; reamining_size -= block->size; } > + > + block_offset += block->size; > + } > + > + return 0; > +} > + > +static int rkisp1_params_vb2_buf_out_validate(struct vb2_buffer *vb) > +{ > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > + struct rkisp1_params_buffer *params_buf = > + container_of(vbuf, struct rkisp1_params_buffer, vb); > + struct vb2_queue *vq = vb->vb2_queue; > + struct rkisp1_params *params = vq->drv_priv; > + struct rkisp1_ext_params_cfg *cfg = > + vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0); > + > + /* > + * Copy the parameters buffer to the internal scratch buffer to avoid > + * userspace modifying the buffer content while the driver processes it. > + */ > + memcpy(params_buf->cfg, cfg, params->metafmt->buffersize); > + > + /* Fixed parameters format doesn't require validation. */ > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > + return 0; > + > + return rkisp1_params_validate_ext_params(params, cfg); > } > > static const struct vb2_ops rkisp1_params_vb2_ops = { > .queue_setup = rkisp1_params_vb2_queue_setup, > .buf_init = rkisp1_params_vb2_buf_init, > .buf_cleanup = rkisp1_params_vb2_buf_cleanup, > + .buf_out_validate = rkisp1_params_vb2_buf_out_validate, > .wait_prepare = vb2_ops_wait_prepare, > .wait_finish = vb2_ops_wait_finish, > .buf_queue = rkisp1_params_vb2_buf_queue,
Hi Laurent On Sat, Jun 29, 2024 at 05:36:07PM GMT, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Jun 21, 2024 at 04:54:04PM +0200, Jacopo Mondi wrote: > > Implement support in 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. > > > > Parse the configuration parameters buffer in rkisp1_ext_params_config > > and filter the enable blocks by group, to allow setting the 'other' > > groups separately from the 'lsc' group to support the pre/post-configure > > operations. > > > > Implement the vb2 buf_out_validate() operation to validate the > > extensible format buffer content at qbuf time. > > Is there a particular reason to do so in .buf_out_validate() instead of > .buf_prepare() ? > It felt like it's the right place where to perform validation * @buf_out_validate: called when the output buffer is prepared or queued * to a request; drivers can use this to validate * userspace-provided information; this is required only * for OUTPUT queues. out buffers are validated -before- buf_prepare() is called. I admit it might have no practical differences though. Would you prefer to collapse everything into _prepare ? If I drop vb2_set_plane_payload(vb, 0, params->metafmt->buffersize); Not much will remain in _prepare() and I could even drop it. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > .../platform/rockchip/rkisp1/rkisp1-common.h | 3 + > > .../platform/rockchip/rkisp1/rkisp1-params.c | 553 +++++++++++++++++- > > 2 files changed, 540 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > index ccd2065351b4..bffd936f989a 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > @@ -390,6 +390,7 @@ struct rkisp1_params_ops { > > * @quantization: the quantization configured on the isp's src pad > > * @ycbcr_encoding the YCbCr encoding > > * @raw_type: the bayer pattern on the isp video sink pad > > + * @enabled_blocks: bitmask of enabled ISP blocks > > */ > > struct rkisp1_params { > > struct rkisp1_vdev_node vnode; > > @@ -404,6 +405,8 @@ struct rkisp1_params { > > enum v4l2_quantization quantization; > > enum v4l2_ycbcr_encoding ycbcr_encoding; > > enum rkisp1_fmt_raw_pat_type raw_type; > > + > > + u32 enabled_blocks; > > }; > > > > /* > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > index e38d2da994f5..f3ea70c7e0c1 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > @@ -35,6 +35,9 @@ > > #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_LSC BIT(1) > > + > > enum rkisp1_params_formats { > > RKISP1_PARAMS_FIXED, > > RKISP1_PARAMS_EXTENSIBLE, > > @@ -1529,6 +1532,454 @@ rkisp1_params_get_buffer(struct rkisp1_params *params) > > queue); > > } > > > > +/*------------------------------------------------------------------------------ > > + * 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS))) > > + 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->enable == 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 (!(params->enabled_blocks & > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC))) > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG))) > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC))) > > + 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->enable == 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 (!(params->enabled_blocks & > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS))) > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT))) > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM))) > > + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_ctk_enable(params, false); > > + return; > > + } > > + > > + rkisp1_ctk_config(params, &ctk->ctk_config); > > + > > + if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK))) > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC))) > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF))) > > + 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->enable == 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 (!(params->enabled_blocks & > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC))) > > + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + rkisp1_ie_enable(params, false); > > + return; > > + } > > + > > + rkisp1_ie_config(params, &ie->ie_config); > > + > > + if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_IE))) > > + 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->enable == 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 (!(params->enabled_blocks & > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS))) > > + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > + params->ops->hst_enable(params, &hst->hst_config, false); > > + return; > > + } > > + > > + params->ops->hst_config(params, &hst->hst_config); > > + > > + if (!(params->enabled_blocks & > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS))) > > + 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->enable == 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 (!(params->enabled_blocks & > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS))) > > + 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->enable == 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 (!(params->enabled_blocks & > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS))) > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > > + RKISP1_CIF_ISP_AFM_ENA); > > +} > > I still think we could get rid of most of these functions by adding > .enable(), .configure() and .disable() operations to > rkisp1_ext_params_handler. That can be done later. > > > + > > +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params, > > + struct rkisp1_ext_params_block_header *hdr); > > If it doesn't cause any issue, I'd make the hdr const. > > > + > > +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_STRENGTH] = { > > + .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_OTHERS > > + }, > > + [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_OTHERS > > + }, > > + [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_OTHERS > > + }, > > + [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_OTHERS > > + }, > > +}; > > + > > +static void rkisp1_ext_params_config(struct rkisp1_params *params, > > + struct rkisp1_ext_params_cfg *cfg, > > + u32 block_group_mask) > > +{ > > + size_t block_offset = 0; > > + > > + if (WARN_ON(!cfg)) > > + return; > > + > > + /* 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; > > const > > > + > > + block = (struct rkisp1_ext_params_block_header *) > > + &cfg->data[block_offset]; > > + block_offset += block->size; > > + > > + /* Make sure the block is in the list of groups to configure. */ > > + block_handler = &rkisp1_ext_params_handlers[block->type]; > > + if (!(block_handler->group & block_group_mask)) > > + continue; > > + > > + block_handler->handler(params, block); > > + > > + if (block->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) > > + params->enabled_blocks &= ~BIT(block->type); > > + else > > + params->enabled_blocks |= BIT(block->type); > > + } > > +} > > + > > static void rkisp1_params_complete_buffer(struct rkisp1_params *params, > > struct rkisp1_params_buffer *buf, > > unsigned int frame_sequence) > > @@ -1550,9 +2001,15 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) > > if (!cur_buf) > > goto unlock; > > > > - rkisp1_isp_isr_other_config(params, cur_buf->cfg); > > - rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > > - rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > + rkisp1_isp_isr_other_config(params, cur_buf->cfg); > > + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > > + rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > > + } else { > > + rkisp1_ext_params_config(params, cur_buf->cfg, > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > > + } > > > > /* update shadow register immediately */ > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > @@ -1651,8 +2108,13 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > if (!cur_buf) > > goto unlock; > > > > - rkisp1_isp_isr_other_config(params, cur_buf->cfg); > > - rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > + rkisp1_isp_isr_other_config(params, cur_buf->cfg); > > + rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > > + } else { > > + rkisp1_ext_params_config(params, cur_buf->cfg, > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS); > > + } > > > > /* update shadow register immediately */ > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > @@ -1680,7 +2142,11 @@ void rkisp1_params_post_configure(struct rkisp1_params *params) > > if (!cur_buf) > > goto unlock; > > > > - rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > > + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > > + else > > + rkisp1_ext_params_config(params, cur_buf->cfg, > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > > > > /* update shadow register immediately */ > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > @@ -1874,10 +2340,6 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb) > > > > static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb) > > { > > - struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > > - struct rkisp1_params_buffer *params_buf = > > - container_of(vbuf, struct rkisp1_params_buffer, vb); > > - void *cfg = vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0); > > struct rkisp1_params *params = vb->vb2_queue->drv_priv; > > > > if (vb2_plane_size(vb, 0) < params->metafmt->buffersize) > > @@ -1885,12 +2347,6 @@ static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb) > > > > vb2_set_plane_payload(vb, 0, params->metafmt->buffersize); > > > > - /* > > - * Copy the parameters buffer to the internal scratch buffer to avoid > > - * userspace modifying the buffer content while the driver processes it. > > - */ > > - memcpy(params_buf->cfg, cfg, params->metafmt->buffersize); > > - > > return 0; > > } > > > > @@ -1911,12 +2367,77 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq) > > > > list_for_each_entry(buf, &tmp_list, queue) > > vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); > > + > > + params->enabled_blocks = 0; > > +} > > + > > +static int rkisp1_params_validate_ext_params(struct rkisp1_params *params, > > + struct rkisp1_ext_params_cfg *cfg) > > +{ > > + size_t block_offset = 0; > > + > > + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) { > > + dev_dbg(params->rkisp1->dev, > > + "Invalid parameters buffer size %u\n", > > + cfg->total_size); > > + return -EINVAL; > > + } > > Following my review comments on patch 5/7, you should also validate that > offsetof(struct rkisp1_ext_params_cfg, data) + cfg->total_size >= plane payload. > > > + > > + /* Walk the list of parameter blocks and validate them. */ > > + while (block_offset < cfg->total_size) { > > while (cfg->total_size - block_offset >= > sizeof(struct rkisp1_ext_params_block_header)) { > > Otherwise there could be just one byte left, and dereferencing block > below would read uninitialized memory. > > > + const struct rkisp1_ext_params_handler *hdlr; > > Maybe handler, it's not much longer, and is more readable. > > > + struct rkisp1_ext_params_block_header *block; > > const > > > + > > + block = (struct rkisp1_ext_params_block_header *) > > + &cfg->data[block_offset]; > > + > > + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) { > > if (block->type >= ARRAY_SIZE(rkisp1_ext_params_handlers)) { > > which allows you to drop RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL from the > UAPI header. > > > + dev_dbg(params->rkisp1->dev, > > + "Invalid parameters block type\n"); > > + return -EINVAL; > > + } > > + > > + hdlr = &rkisp1_ext_params_handlers[block->type]; > > + if (block->size != hdlr->size) { > > + dev_dbg(params->rkisp1->dev, > > + "Invalid parameters block size\n"); > > + return -EINVAL; > > + } > > You need to test here that block->size >= cfg->total_size - > block_offset. It may be easier to add a new local variable at the top of > the function > > size_t remaining_size = cfg->total_size; > > The loop would then become > > while (remaining_size >= sizeof(struct rkisp1_ext_params_block_header)) { > const struct rkisp1_ext_params_block_header *block; > const struct rkisp1_ext_params_handler *handler; > > block = (struct rkisp1_ext_params_block_header *) > &cfg->data[block_offset]; > > if (block->type >= ARRAY_SIZE(rkisp1_ext_params_handlers)) { > dev_dbg(params->rkisp1->dev, > "Invalid parameters block type\n"); > return -EINVAL; > } > > if (block->size > remaining_size) { > dev_dbg(params->rkisp1->dev, > "Premature end of parameters data\n"); > return -EINVAL; > } > > handler = &rkisp1_ext_params_handlers[block->type]; > if (block->size != handler->size) { > dev_dbg(params->rkisp1->dev, > "Invalid parameters block size\n"); > return -EINVAL; > } > > block_offset += block->size; > reamining_size -= block->size; > } > > > + > > + block_offset += block->size; > > + } > > + > > + return 0; > > +} > > + > > +static int rkisp1_params_vb2_buf_out_validate(struct vb2_buffer *vb) > > +{ > > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > > + struct rkisp1_params_buffer *params_buf = > > + container_of(vbuf, struct rkisp1_params_buffer, vb); > > + struct vb2_queue *vq = vb->vb2_queue; > > + struct rkisp1_params *params = vq->drv_priv; > > + struct rkisp1_ext_params_cfg *cfg = > > + vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0); > > + > > + /* > > + * Copy the parameters buffer to the internal scratch buffer to avoid > > + * userspace modifying the buffer content while the driver processes it. > > + */ > > + memcpy(params_buf->cfg, cfg, params->metafmt->buffersize); > > + > > + /* Fixed parameters format doesn't require validation. */ > > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > > + return 0; > > + > > + return rkisp1_params_validate_ext_params(params, cfg); > > } > > > > static const struct vb2_ops rkisp1_params_vb2_ops = { > > .queue_setup = rkisp1_params_vb2_queue_setup, > > .buf_init = rkisp1_params_vb2_buf_init, > > .buf_cleanup = rkisp1_params_vb2_buf_cleanup, > > + .buf_out_validate = rkisp1_params_vb2_buf_out_validate, > > .wait_prepare = vb2_ops_wait_prepare, > > .wait_finish = vb2_ops_wait_finish, > > .buf_queue = rkisp1_params_vb2_buf_queue, > > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Mon, Jul 01, 2024 at 12:43:14PM +0200, Jacopo Mondi wrote: > On Sat, Jun 29, 2024 at 05:36:07PM GMT, Laurent Pinchart wrote: > > On Fri, Jun 21, 2024 at 04:54:04PM +0200, Jacopo Mondi wrote: > > > Implement support in 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. > > > > > > Parse the configuration parameters buffer in rkisp1_ext_params_config > > > and filter the enable blocks by group, to allow setting the 'other' > > > groups separately from the 'lsc' group to support the pre/post-configure > > > operations. > > > > > > Implement the vb2 buf_out_validate() operation to validate the > > > extensible format buffer content at qbuf time. > > > > Is there a particular reason to do so in .buf_out_validate() instead of > > .buf_prepare() ? > > > > It felt like it's the right place where to perform validation > > * @buf_out_validate: called when the output buffer is prepared or queued > * to a request; drivers can use this to validate > * userspace-provided information; this is required only > * for OUTPUT queues. > > out buffers are validated -before- buf_prepare() is called. I admit it > might have no practical differences though. Would you prefer to > collapse everything into _prepare ? If I drop > > vb2_set_plane_payload(vb, 0, params->metafmt->buffersize); > > Not much will remain in _prepare() and I could even drop it. I think I'd collapse it, yes. .buf_out_validate() seems to have been introduced in codecs and I'm not sure how well-defined its semantics are. The documentation also states "to a request". > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > .../platform/rockchip/rkisp1/rkisp1-common.h | 3 + > > > .../platform/rockchip/rkisp1/rkisp1-params.c | 553 +++++++++++++++++- > > > 2 files changed, 540 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > > index ccd2065351b4..bffd936f989a 100644 > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > > @@ -390,6 +390,7 @@ struct rkisp1_params_ops { > > > * @quantization: the quantization configured on the isp's src pad > > > * @ycbcr_encoding the YCbCr encoding > > > * @raw_type: the bayer pattern on the isp video sink pad > > > + * @enabled_blocks: bitmask of enabled ISP blocks > > > */ > > > struct rkisp1_params { > > > struct rkisp1_vdev_node vnode; > > > @@ -404,6 +405,8 @@ struct rkisp1_params { > > > enum v4l2_quantization quantization; > > > enum v4l2_ycbcr_encoding ycbcr_encoding; > > > enum rkisp1_fmt_raw_pat_type raw_type; > > > + > > > + u32 enabled_blocks; > > > }; > > > > > > /* > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > > index e38d2da994f5..f3ea70c7e0c1 100644 > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c > > > @@ -35,6 +35,9 @@ > > > #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_LSC BIT(1) > > > + > > > enum rkisp1_params_formats { > > > RKISP1_PARAMS_FIXED, > > > RKISP1_PARAMS_EXTENSIBLE, > > > @@ -1529,6 +1532,454 @@ rkisp1_params_get_buffer(struct rkisp1_params *params) > > > queue); > > > } > > > > > > +/*------------------------------------------------------------------------------ > > > + * 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS))) > > > + 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->enable == 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 (!(params->enabled_blocks & > > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC))) > > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG))) > > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC))) > > > + 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->enable == 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 (!(params->enabled_blocks & > > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS))) > > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT))) > > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM))) > > > + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_ctk_enable(params, false); > > > + return; > > > + } > > > + > > > + rkisp1_ctk_config(params, &ctk->ctk_config); > > > + > > > + if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK))) > > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC))) > > > + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF))) > > > + 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->enable == 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 (!(params->enabled_blocks & > > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC))) > > > + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + rkisp1_ie_enable(params, false); > > > + return; > > > + } > > > + > > > + rkisp1_ie_config(params, &ie->ie_config); > > > + > > > + if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_IE))) > > > + 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->enable == 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 (!(params->enabled_blocks & > > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS))) > > > + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { > > > + params->ops->hst_enable(params, &hst->hst_config, false); > > > + return; > > > + } > > > + > > > + params->ops->hst_config(params, &hst->hst_config); > > > + > > > + if (!(params->enabled_blocks & > > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS))) > > > + 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->enable == 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 (!(params->enabled_blocks & > > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS))) > > > + 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->enable == 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 (!(params->enabled_blocks & > > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS))) > > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL, > > > + RKISP1_CIF_ISP_AFM_ENA); > > > +} > > > > I still think we could get rid of most of these functions by adding > > .enable(), .configure() and .disable() operations to > > rkisp1_ext_params_handler. That can be done later. > > > > > + > > > +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_block_header *hdr); > > > > If it doesn't cause any issue, I'd make the hdr const. > > > > > + > > > +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_STRENGTH] = { > > > + .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_OTHERS > > > + }, > > > + [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_OTHERS > > > + }, > > > + [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_OTHERS > > > + }, > > > + [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_OTHERS > > > + }, > > > +}; > > > + > > > +static void rkisp1_ext_params_config(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_cfg *cfg, > > > + u32 block_group_mask) > > > +{ > > > + size_t block_offset = 0; > > > + > > > + if (WARN_ON(!cfg)) > > > + return; > > > + > > > + /* 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; > > > > const > > > > > + > > > + block = (struct rkisp1_ext_params_block_header *) > > > + &cfg->data[block_offset]; > > > + block_offset += block->size; > > > + > > > + /* Make sure the block is in the list of groups to configure. */ > > > + block_handler = &rkisp1_ext_params_handlers[block->type]; > > > + if (!(block_handler->group & block_group_mask)) > > > + continue; > > > + > > > + block_handler->handler(params, block); > > > + > > > + if (block->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) > > > + params->enabled_blocks &= ~BIT(block->type); > > > + else > > > + params->enabled_blocks |= BIT(block->type); > > > + } > > > +} > > > + > > > static void rkisp1_params_complete_buffer(struct rkisp1_params *params, > > > struct rkisp1_params_buffer *buf, > > > unsigned int frame_sequence) > > > @@ -1550,9 +2001,15 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) > > > if (!cur_buf) > > > goto unlock; > > > > > > - rkisp1_isp_isr_other_config(params, cur_buf->cfg); > > > - rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > > > - rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > > > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > > + rkisp1_isp_isr_other_config(params, cur_buf->cfg); > > > + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > > > + rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > > > + } else { > > > + rkisp1_ext_params_config(params, cur_buf->cfg, > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > > > + } > > > > > > /* update shadow register immediately */ > > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > @@ -1651,8 +2108,13 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, > > > if (!cur_buf) > > > goto unlock; > > > > > > - rkisp1_isp_isr_other_config(params, cur_buf->cfg); > > > - rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > > > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { > > > + rkisp1_isp_isr_other_config(params, cur_buf->cfg); > > > + rkisp1_isp_isr_meas_config(params, cur_buf->cfg); > > > + } else { > > > + rkisp1_ext_params_config(params, cur_buf->cfg, > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS); > > > + } > > > > > > /* update shadow register immediately */ > > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > @@ -1680,7 +2142,11 @@ void rkisp1_params_post_configure(struct rkisp1_params *params) > > > if (!cur_buf) > > > goto unlock; > > > > > > - rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > > > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > > > + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); > > > + else > > > + rkisp1_ext_params_config(params, cur_buf->cfg, > > > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); > > > > > > /* update shadow register immediately */ > > > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, > > > @@ -1874,10 +2340,6 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb) > > > > > > static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb) > > > { > > > - struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > > > - struct rkisp1_params_buffer *params_buf = > > > - container_of(vbuf, struct rkisp1_params_buffer, vb); > > > - void *cfg = vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0); > > > struct rkisp1_params *params = vb->vb2_queue->drv_priv; > > > > > > if (vb2_plane_size(vb, 0) < params->metafmt->buffersize) > > > @@ -1885,12 +2347,6 @@ static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb) > > > > > > vb2_set_plane_payload(vb, 0, params->metafmt->buffersize); > > > > > > - /* > > > - * Copy the parameters buffer to the internal scratch buffer to avoid > > > - * userspace modifying the buffer content while the driver processes it. > > > - */ > > > - memcpy(params_buf->cfg, cfg, params->metafmt->buffersize); > > > - > > > return 0; > > > } > > > > > > @@ -1911,12 +2367,77 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq) > > > > > > list_for_each_entry(buf, &tmp_list, queue) > > > vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); > > > + > > > + params->enabled_blocks = 0; > > > +} > > > + > > > +static int rkisp1_params_validate_ext_params(struct rkisp1_params *params, > > > + struct rkisp1_ext_params_cfg *cfg) > > > +{ > > > + size_t block_offset = 0; > > > + > > > + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) { > > > + dev_dbg(params->rkisp1->dev, > > > + "Invalid parameters buffer size %u\n", > > > + cfg->total_size); > > > + return -EINVAL; > > > + } > > > > Following my review comments on patch 5/7, you should also validate that > > offsetof(struct rkisp1_ext_params_cfg, data) + cfg->total_size >= plane payload. > > > > > + > > > + /* Walk the list of parameter blocks and validate them. */ > > > + while (block_offset < cfg->total_size) { > > > > while (cfg->total_size - block_offset >= > > sizeof(struct rkisp1_ext_params_block_header)) { > > > > Otherwise there could be just one byte left, and dereferencing block > > below would read uninitialized memory. > > > > > + const struct rkisp1_ext_params_handler *hdlr; > > > > Maybe handler, it's not much longer, and is more readable. > > > > > + struct rkisp1_ext_params_block_header *block; > > > > const > > > > > + > > > + block = (struct rkisp1_ext_params_block_header *) > > > + &cfg->data[block_offset]; > > > + > > > + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) { > > > > if (block->type >= ARRAY_SIZE(rkisp1_ext_params_handlers)) { > > > > which allows you to drop RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL from the > > UAPI header. > > > > > + dev_dbg(params->rkisp1->dev, > > > + "Invalid parameters block type\n"); > > > + return -EINVAL; > > > + } > > > + > > > + hdlr = &rkisp1_ext_params_handlers[block->type]; > > > + if (block->size != hdlr->size) { > > > + dev_dbg(params->rkisp1->dev, > > > + "Invalid parameters block size\n"); > > > + return -EINVAL; > > > + } > > > > You need to test here that block->size >= cfg->total_size - > > block_offset. It may be easier to add a new local variable at the top of > > the function > > > > size_t remaining_size = cfg->total_size; > > > > The loop would then become > > > > while (remaining_size >= sizeof(struct rkisp1_ext_params_block_header)) { > > const struct rkisp1_ext_params_block_header *block; > > const struct rkisp1_ext_params_handler *handler; > > > > block = (struct rkisp1_ext_params_block_header *) > > &cfg->data[block_offset]; > > > > if (block->type >= ARRAY_SIZE(rkisp1_ext_params_handlers)) { > > dev_dbg(params->rkisp1->dev, > > "Invalid parameters block type\n"); > > return -EINVAL; > > } > > > > if (block->size > remaining_size) { > > dev_dbg(params->rkisp1->dev, > > "Premature end of parameters data\n"); > > return -EINVAL; > > } > > > > handler = &rkisp1_ext_params_handlers[block->type]; > > if (block->size != handler->size) { > > dev_dbg(params->rkisp1->dev, > > "Invalid parameters block size\n"); > > return -EINVAL; > > } > > > > block_offset += block->size; > > reamining_size -= block->size; > > } > > > > > + > > > + block_offset += block->size; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int rkisp1_params_vb2_buf_out_validate(struct vb2_buffer *vb) > > > +{ > > > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > > > + struct rkisp1_params_buffer *params_buf = > > > + container_of(vbuf, struct rkisp1_params_buffer, vb); > > > + struct vb2_queue *vq = vb->vb2_queue; > > > + struct rkisp1_params *params = vq->drv_priv; > > > + struct rkisp1_ext_params_cfg *cfg = > > > + vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0); > > > + > > > + /* > > > + * Copy the parameters buffer to the internal scratch buffer to avoid > > > + * userspace modifying the buffer content while the driver processes it. > > > + */ > > > + memcpy(params_buf->cfg, cfg, params->metafmt->buffersize); > > > + > > > + /* Fixed parameters format doesn't require validation. */ > > > + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) > > > + return 0; > > > + > > > + return rkisp1_params_validate_ext_params(params, cfg); > > > } > > > > > > static const struct vb2_ops rkisp1_params_vb2_ops = { > > > .queue_setup = rkisp1_params_vb2_queue_setup, > > > .buf_init = rkisp1_params_vb2_buf_init, > > > .buf_cleanup = rkisp1_params_vb2_buf_cleanup, > > > + .buf_out_validate = rkisp1_params_vb2_buf_out_validate, > > > .wait_prepare = vb2_ops_wait_prepare, > > > .wait_finish = vb2_ops_wait_finish, > > > .buf_queue = rkisp1_params_vb2_buf_queue,
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h index ccd2065351b4..bffd936f989a 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h @@ -390,6 +390,7 @@ struct rkisp1_params_ops { * @quantization: the quantization configured on the isp's src pad * @ycbcr_encoding the YCbCr encoding * @raw_type: the bayer pattern on the isp video sink pad + * @enabled_blocks: bitmask of enabled ISP blocks */ struct rkisp1_params { struct rkisp1_vdev_node vnode; @@ -404,6 +405,8 @@ struct rkisp1_params { enum v4l2_quantization quantization; enum v4l2_ycbcr_encoding ycbcr_encoding; enum rkisp1_fmt_raw_pat_type raw_type; + + u32 enabled_blocks; }; /* diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c index e38d2da994f5..f3ea70c7e0c1 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c @@ -35,6 +35,9 @@ #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_LSC BIT(1) + enum rkisp1_params_formats { RKISP1_PARAMS_FIXED, RKISP1_PARAMS_EXTENSIBLE, @@ -1529,6 +1532,454 @@ rkisp1_params_get_buffer(struct rkisp1_params *params) queue); } +/*------------------------------------------------------------------------------ + * 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS))) + 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->enable == 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 (!(params->enabled_blocks & + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC))) + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG))) + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC))) + 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->enable == 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 (!(params->enabled_blocks & + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS))) + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT))) + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM))) + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_ctk_enable(params, false); + return; + } + + rkisp1_ctk_config(params, &ctk->ctk_config); + + if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK))) + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC))) + 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->enable == 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 (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF))) + 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->enable == 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 (!(params->enabled_blocks & + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC))) + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + rkisp1_ie_enable(params, false); + return; + } + + rkisp1_ie_config(params, &ie->ie_config); + + if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_IE))) + 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->enable == 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 (!(params->enabled_blocks & + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS))) + 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->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) { + params->ops->hst_enable(params, &hst->hst_config, false); + return; + } + + params->ops->hst_config(params, &hst->hst_config); + + if (!(params->enabled_blocks & + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS))) + 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->enable == 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 (!(params->enabled_blocks & + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS))) + 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->enable == 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 (!(params->enabled_blocks & + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS))) + 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_STRENGTH] = { + .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_OTHERS + }, + [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_OTHERS + }, + [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_OTHERS + }, + [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_OTHERS + }, +}; + +static void rkisp1_ext_params_config(struct rkisp1_params *params, + struct rkisp1_ext_params_cfg *cfg, + u32 block_group_mask) +{ + size_t block_offset = 0; + + if (WARN_ON(!cfg)) + return; + + /* 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; + + /* Make sure the block is in the list of groups to configure. */ + block_handler = &rkisp1_ext_params_handlers[block->type]; + if (!(block_handler->group & block_group_mask)) + continue; + + block_handler->handler(params, block); + + if (block->enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) + params->enabled_blocks &= ~BIT(block->type); + else + params->enabled_blocks |= BIT(block->type); + } +} + static void rkisp1_params_complete_buffer(struct rkisp1_params *params, struct rkisp1_params_buffer *buf, unsigned int frame_sequence) @@ -1550,9 +2001,15 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1) if (!cur_buf) goto unlock; - rkisp1_isp_isr_other_config(params, cur_buf->cfg); - rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); - rkisp1_isp_isr_meas_config(params, cur_buf->cfg); + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { + rkisp1_isp_isr_other_config(params, cur_buf->cfg); + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); + rkisp1_isp_isr_meas_config(params, cur_buf->cfg); + } else { + rkisp1_ext_params_config(params, cur_buf->cfg, + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS | + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); + } /* update shadow register immediately */ rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, @@ -1651,8 +2108,13 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params, if (!cur_buf) goto unlock; - rkisp1_isp_isr_other_config(params, cur_buf->cfg); - rkisp1_isp_isr_meas_config(params, cur_buf->cfg); + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) { + rkisp1_isp_isr_other_config(params, cur_buf->cfg); + rkisp1_isp_isr_meas_config(params, cur_buf->cfg); + } else { + rkisp1_ext_params_config(params, cur_buf->cfg, + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS); + } /* update shadow register immediately */ rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, @@ -1680,7 +2142,11 @@ void rkisp1_params_post_configure(struct rkisp1_params *params) if (!cur_buf) goto unlock; - rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg); + else + rkisp1_ext_params_config(params, cur_buf->cfg, + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC); /* update shadow register immediately */ rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, @@ -1874,10 +2340,6 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb) static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb) { - struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); - struct rkisp1_params_buffer *params_buf = - container_of(vbuf, struct rkisp1_params_buffer, vb); - void *cfg = vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0); struct rkisp1_params *params = vb->vb2_queue->drv_priv; if (vb2_plane_size(vb, 0) < params->metafmt->buffersize) @@ -1885,12 +2347,6 @@ static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb) vb2_set_plane_payload(vb, 0, params->metafmt->buffersize); - /* - * Copy the parameters buffer to the internal scratch buffer to avoid - * userspace modifying the buffer content while the driver processes it. - */ - memcpy(params_buf->cfg, cfg, params->metafmt->buffersize); - return 0; } @@ -1911,12 +2367,77 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq) list_for_each_entry(buf, &tmp_list, queue) vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR); + + params->enabled_blocks = 0; +} + +static int rkisp1_params_validate_ext_params(struct rkisp1_params *params, + struct rkisp1_ext_params_cfg *cfg) +{ + size_t block_offset = 0; + + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) { + dev_dbg(params->rkisp1->dev, + "Invalid parameters buffer size %u\n", + cfg->total_size); + return -EINVAL; + } + + /* Walk the list of parameter blocks and validate them. */ + while (block_offset < cfg->total_size) { + const struct rkisp1_ext_params_handler *hdlr; + struct rkisp1_ext_params_block_header *block; + + block = (struct rkisp1_ext_params_block_header *) + &cfg->data[block_offset]; + + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) { + dev_dbg(params->rkisp1->dev, + "Invalid parameters block type\n"); + return -EINVAL; + } + + hdlr = &rkisp1_ext_params_handlers[block->type]; + if (block->size != hdlr->size) { + dev_dbg(params->rkisp1->dev, + "Invalid parameters block size\n"); + return -EINVAL; + } + + block_offset += block->size; + } + + return 0; +} + +static int rkisp1_params_vb2_buf_out_validate(struct vb2_buffer *vb) +{ + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); + struct rkisp1_params_buffer *params_buf = + container_of(vbuf, struct rkisp1_params_buffer, vb); + struct vb2_queue *vq = vb->vb2_queue; + struct rkisp1_params *params = vq->drv_priv; + struct rkisp1_ext_params_cfg *cfg = + vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0); + + /* + * Copy the parameters buffer to the internal scratch buffer to avoid + * userspace modifying the buffer content while the driver processes it. + */ + memcpy(params_buf->cfg, cfg, params->metafmt->buffersize); + + /* Fixed parameters format doesn't require validation. */ + if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) + return 0; + + return rkisp1_params_validate_ext_params(params, cfg); } static const struct vb2_ops rkisp1_params_vb2_ops = { .queue_setup = rkisp1_params_vb2_queue_setup, .buf_init = rkisp1_params_vb2_buf_init, .buf_cleanup = rkisp1_params_vb2_buf_cleanup, + .buf_out_validate = rkisp1_params_vb2_buf_out_validate, .wait_prepare = vb2_ops_wait_prepare, .wait_finish = vb2_ops_wait_finish, .buf_queue = rkisp1_params_vb2_buf_queue,
Implement support in 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. Parse the configuration parameters buffer in rkisp1_ext_params_config and filter the enable blocks by group, to allow setting the 'other' groups separately from the 'lsc' group to support the pre/post-configure operations. Implement the vb2 buf_out_validate() operation to validate the extensible format buffer content at qbuf time. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- .../platform/rockchip/rkisp1/rkisp1-common.h | 3 + .../platform/rockchip/rkisp1/rkisp1-params.c | 553 +++++++++++++++++- 2 files changed, 540 insertions(+), 16 deletions(-)