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