From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Dan Scally <dan.scally@ideasonboard.com>
Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com>,
Linux Media Mailing List <linux-media@vger.kernel.org>,
Sakari Ailus <sakari.ailus@iki.fi>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Stefan Klug <stefan.klug@ideasonboard.com>,
Paul Elder <paul.elder@ideasonboard.com>,
Kieran Bingham <kieran.bingham@ideasonboard.com>,
Umang Jain <umang.jain@ideasonboard.com>,
Dafna Hirschfeld <dafna@fastmail.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Heiko Stuebner <heiko@sntech.de>
Subject: Re: [PATCH 5/8] media: rkisp1: Implement extensible params support
Date: Wed, 12 Jun 2024 18:42:01 +0300 [thread overview]
Message-ID: <20240612154201.GD15991@pendragon.ideasonboard.com> (raw)
In-Reply-To: <43ba414f-96f5-47a5-81a6-26cb109b22cd@ideasonboard.com>
On Wed, Jun 12, 2024 at 02:50:34PM +0100, Daniel Scally wrote:
> On 05/06/2024 17:54, Jacopo Mondi wrote:
> > Implement support in the rkisp1-params for the extensible configuration
> > parameters format.
> >
> > Create a list of handlers for each ISP block that wraps the existing
> > configuration functions and handles the ISP block enablement state.
> >
> > Parse the configuration parameters buffer in __rkisp1_ext_params_config
> > and filter the enable blocks by group, to allow setting the 'other' and
> > 'meas' groups separately from the 'lsc' group to support the
> > pre/post-configure operations.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > .../platform/rockchip/rkisp1/rkisp1-params.c | 565 +++++++++++++++++-
> > 1 file changed, 544 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 6f99c7dad758..3d78e643d0b8 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -33,6 +33,10 @@
> > #define RKISP1_ISP_CC_COEFF(n) \
> > (RKISP1_CIF_ISP_CC_COEFF_0 + (n) * 4)
> >
> > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS BIT(0)
> > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS BIT(1)
> > +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC BIT(2)
Would "LSC" and "OTHERS" be enough ? The OTHERS and MEAS groups are
always handled together.
> > +
> > enum rkisp1_params_formats {
> > RKISP1_PARAMS_FIXED,
> > RKISP1_PARAMS_EXTENSIBLE,
> > @@ -1529,9 +1533,491 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
> > }
> > }
> >
> > +/*------------------------------------------------------------------------------
> > + * Extensible parameters format handling
> > + */
> > +
> > +static void rkisp1_ext_params_bls(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_bls_config *bls =
> > + (struct rkisp1_ext_params_bls_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_BLS_CTRL,
> > + RKISP1_CIF_ISP_BLS_ENA);
> > + return;
> > + }
> > +
> > + rkisp1_bls_config(params, &bls->bls_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_BLS_CTRL,
> > + RKISP1_CIF_ISP_BLS_ENA);
> > +}
>
> Most of the handlers have all but identical handling for the
> enable/disable parts; is it worth factoring that out perhaps? The
> register and bits could be added to struct rkisp1_ext_params_handler
> and then a common pre-handler function could be called from
> __rkisp1_ext_params_config() to set/clear the bits.
I like the idea of generalizing things :-) The devil is in the details
though, so if it causes annoying issues, we can skip it.
If we drop RKISP1_EXT_PARAMS_BLOCK_NO_CHANGE, we should also cache the
enable state. That should be easy to do in a bitmask indexed by block
type, and will fit nicely in generalized handling of the enable bits.
> > +
> > +static void rkisp1_ext_params_dpcc(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_dpcc_config *dpcc =
> > + (struct rkisp1_ext_params_dpcc_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE,
> > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE);
> > + return;
> > + }
> > +
> > + rkisp1_dpcc_config(params, &dpcc->dpcc_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPCC_MODE,
> > + RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE);
> > +}
> > +
> > +static void rkisp1_ext_params_sdg(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_sdg_config *sdg =
> > + (struct rkisp1_ext_params_sdg_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
> > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA);
> > + return;
> > + }
> > +
> > + rkisp1_sdg_config(params, &sdg->sdg_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA);
> > +}
> > +
> > +static void rkisp1_ext_params_lsc(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_lsc_config *lsc =
> > + (struct rkisp1_ext_params_lsc_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_LSC_CTRL,
> > + RKISP1_CIF_ISP_LSC_CTRL_ENA);
> > + return;
> > + }
> > +
> > + rkisp1_lsc_config(params, &lsc->lsc_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_LSC_CTRL,
> > + RKISP1_CIF_ISP_LSC_CTRL_ENA);
> > +}
> > +
> > +static void rkisp1_ext_params_awbg(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_awb_gain_config *awbg =
> > + (struct rkisp1_ext_params_awb_gain_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
> > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA);
> > + return;
> > + }
> > +
> > + params->ops->awb_gain_config(params, &awbg->awb_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > + RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA);
> > +}
> > +
> > +static void rkisp1_ext_params_flt(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_flt_config *flt =
> > + (struct rkisp1_ext_params_flt_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_FILT_MODE,
> > + RKISP1_CIF_ISP_FLT_ENA);
> > + return;
> > + }
> > +
> > + rkisp1_flt_config(params, &flt->flt_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_FILT_MODE,
> > + RKISP1_CIF_ISP_FLT_ENA);
> > +}
> > +
> > +static void rkisp1_ext_params_bdm(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_bdm_config *bdm =
> > + (struct rkisp1_ext_params_bdm_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DEMOSAIC,
> > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS);
> > + return;
> > + }
> > +
> > + rkisp1_bdm_config(params, &bdm->bdm_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DEMOSAIC,
> > + RKISP1_CIF_ISP_DEMOSAIC_BYPASS);
> > +}
> > +
> > +static void rkisp1_ext_params_ctk(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_ctk_config *ctk =
> > + (struct rkisp1_ext_params_ctk_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_ctk_enable(params, false);
> > + return;
> > + }
> > +
> > + rkisp1_ctk_config(params, &ctk->ctk_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_ctk_enable(params, true);
> > +}
> > +
> > +static void rkisp1_ext_params_goc(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_goc_config *goc =
> > + (struct rkisp1_ext_params_goc_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
> > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA);
> > + return;
> > + }
> > +
> > + params->ops->goc_config(params, &goc->goc_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > + RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA);
> > +}
> > +
> > +static void rkisp1_ext_params_dpf(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_dpf_config *dpf =
> > + (struct rkisp1_ext_params_dpf_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPF_MODE,
> > + RKISP1_CIF_ISP_DPF_MODE_EN);
> > + return;
> > + }
> > +
> > + rkisp1_dpf_config(params, &dpf->dpf_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPF_MODE,
> > + RKISP1_CIF_ISP_DPF_MODE_EN);
> > +}
> > +
> > +static void rkisp1_ext_params_dpfs(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_dpf_strength_config *dpfs =
> > + (struct rkisp1_ext_params_dpf_strength_config *)hdr;
> > +
> > + rkisp1_dpf_strength_config(params, &dpfs->dpf_strength_config);
> > +}
> > +
> > +static void rkisp1_ext_params_cproc(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_cproc_config *cproc =
> > + (struct rkisp1_ext_params_cproc_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL,
> > + RKISP1_CIF_C_PROC_CTR_ENABLE);
> > + return;
> > + }
> > +
> > + rkisp1_cproc_config(params, &cproc->cproc_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_C_PROC_CTRL,
> > + RKISP1_CIF_C_PROC_CTR_ENABLE);
> > +}
> > +
> > +static void rkisp1_ext_params_ie(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_ie_config *ie =
> > + (struct rkisp1_ext_params_ie_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_ie_enable(params, false);
> > + return;
> > + }
> > +
> > + rkisp1_ie_config(params, &ie->ie_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_ie_enable(params, true);
> > +}
> > +
> > +static void rkisp1_ext_params_awbm(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_awb_meas_config *awbm =
> > + (struct rkisp1_ext_params_awb_meas_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config,
> > + false);
> > + return;
> > + }
> > +
> > + params->ops->awb_meas_config(params, &awbm->awb_meas_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + params->ops->awb_meas_enable(params, &awbm->awb_meas_config,
> > + true);
> > +}
> > +
> > +static void rkisp1_ext_params_hstm(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_hst_config *hst =
> > + (struct rkisp1_ext_params_hst_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + params->ops->hst_enable(params, &hst->hst_config, false);
> > + return;
> > + }
> > +
> > + params->ops->hst_config(params, &hst->hst_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + params->ops->hst_enable(params, &hst->hst_config, true);
> > +}
> > +
> > +static void rkisp1_ext_params_aecm(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_aec_config *aec =
> > + (struct rkisp1_ext_params_aec_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_EXP_CTRL,
> > + RKISP1_CIF_ISP_EXP_ENA);
> > + return;
> > + }
> > +
> > + params->ops->aec_config(params, &aec->aec_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_EXP_CTRL,
> > + RKISP1_CIF_ISP_EXP_ENA);
> > +}
> > +
> > +static void rkisp1_ext_params_afcm(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr)
> > +{
> > + struct rkisp1_ext_params_afc_config *afc =
> > + (struct rkisp1_ext_params_afc_config *)hdr;
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_AFM_CTRL,
> > + RKISP1_CIF_ISP_AFM_ENA);
> > + return;
> > + }
> > +
> > + params->ops->afm_config(params, &afc->afc_config);
> > +
> > + if (hdr->state == RKISP1_EXT_PARAMS_BLOCK_ENABLE)
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL,
> > + RKISP1_CIF_ISP_AFM_ENA);
> > +}
> > +
> > +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_block_header *hdr);
> > +
> > +static const struct rkisp1_ext_params_handler {
> > + size_t size;
> > + rkisp1_block_handler handler;
> > + unsigned int group;
We'll have to extend this structure with a bitmask of the ISP versions
each block supports. That can be done later.
> > +} rkisp1_ext_params_handlers[] = {
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = {
> > + .size = sizeof(struct rkisp1_ext_params_bls_config),
> > + .handler = rkisp1_ext_params_bls,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = {
> > + .size = sizeof(struct rkisp1_ext_params_dpcc_config),
> > + .handler = rkisp1_ext_params_dpcc,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG] = {
> > + .size = sizeof(struct rkisp1_ext_params_sdg_config),
> > + .handler = rkisp1_ext_params_sdg,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAINS] = {
> > + .size =
> > + sizeof(struct rkisp1_ext_params_awb_gain_config),
> > + .handler = rkisp1_ext_params_awbg,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT] = {
> > + .size = sizeof(struct rkisp1_ext_params_flt_config),
> > + .handler = rkisp1_ext_params_flt,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM] = {
> > + .size = sizeof(struct rkisp1_ext_params_bdm_config),
> > + .handler = rkisp1_ext_params_bdm,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK] = {
> > + .size = sizeof(struct rkisp1_ext_params_ctk_config),
> > + .handler = rkisp1_ext_params_ctk,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC] = {
> > + .size = sizeof(struct rkisp1_ext_params_goc_config),
> > + .handler = rkisp1_ext_params_goc,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF] = {
> > + .size = sizeof(struct rkisp1_ext_params_dpf_config),
> > + .handler = rkisp1_ext_params_dpf,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGHT] = {
> > + .size =
> > + sizeof(struct rkisp1_ext_params_dpf_strength_config),
> > + .handler = rkisp1_ext_params_dpfs,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC] = {
> > + .size = sizeof(struct rkisp1_ext_params_cproc_config),
> > + .handler = rkisp1_ext_params_cproc,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_IE] = {
> > + .size = sizeof(struct rkisp1_ext_params_ie_config),
> > + .handler = rkisp1_ext_params_ie,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC] = {
> > + .size = sizeof(struct rkisp1_ext_params_lsc_config),
> > + .handler = rkisp1_ext_params_lsc,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS] = {
> > + .size =
> > + sizeof(struct rkisp1_ext_params_awb_meas_config),
> > + .handler = rkisp1_ext_params_awbm,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS] = {
> > + .size = sizeof(struct rkisp1_ext_params_hst_config),
> > + .handler = rkisp1_ext_params_hstm,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS] = {
> > + .size = sizeof(struct rkisp1_ext_params_aec_config),
> > + .handler = rkisp1_ext_params_aecm,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS] = {
> > + .size = sizeof(struct rkisp1_ext_params_afc_config),
> > + .handler = rkisp1_ext_params_afcm,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS
> > + },
> > +};
> > +
> > +static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_cfg *cfg,
> > + u32 block_group_mask)
> > +{
> > + size_t block_offset = 0;
> > +
> > + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) {
> > + dev_dbg(params->rkisp1->dev,
> > + "Invalid parameters buffer size %llu\n",
> > + cfg->total_size);
> > + return -EINVAL;
> > + }
> > +
> > + /* Walk the list of parameter blocks and process them. */
> > + while (block_offset < cfg->total_size) {
> > + const struct rkisp1_ext_params_handler *block_handler;
> > + struct rkisp1_ext_params_block_header *block;
> > +
> > + block = (struct rkisp1_ext_params_block_header *)
> > + &cfg->data[block_offset];
> > + block_offset += block->size;
> > +
> > + /*
> > + * Validate the block id and make sure the block group is in
> > + * the list of groups to configure.
> > + */
> > + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) {
Use ARRAY_SIZE().
> > + dev_dbg(params->rkisp1->dev,
> > + "Invalid parameters block type\n");
> > + return -EINVAL;
> > + }
> > +
> > + block_handler = &rkisp1_ext_params_handlers[block->type];
> > + if (!(block_handler->group & block_group_mask))
> > + continue;
>
>
> So maybe something like
>
>
> if (block_handler->enable_reg)
>
> __rkisp1_block_handle_enable_disable(block->state, block_handler->enable_reg,
> block_handler->enable_val);
>
>
> here to move it out of the handlers.
>
> > +
> > + if (block->size != block_handler->size) {
> > + dev_dbg(params->rkisp1->dev,
> > + "Invalid parameters block size\n");
> > + return -EINVAL;
> > + }
> > +
> > + block_handler->handler(params, block);
It would be nicer to move validation to qbuf time, and applying
parameters at runtime. Maybe I'll find that later in the series :-)
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rkisp1_ext_params_config(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_cfg *cfg)
> > +{
> > + return __rkisp1_ext_params_config(params, cfg,
> > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS |
> > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC |
> > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS);
> > +}
> > +
> > +static int
> > +rkisp1_ext_params_other_meas_config(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_cfg *cfg)
> > +{
> > + return __rkisp1_ext_params_config(params, cfg,
> > + RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS |
> > + RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS);
> > +}
> > +
> > +static int rkisp1_ext_params_lsc_config(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_cfg *cfg)
> > +{
> > + return __rkisp1_ext_params_config(params, cfg,
> > + RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC);
> > +}
I'm tempted to inline this in the callers, I think it would be more
readable.
> > +
> > static bool rkisp1_params_get_buffer(struct rkisp1_params *params,
> > struct rkisp1_buffer **buf,
> > - struct rkisp1_params_cfg **cfg)
> > + void **cfg)
> > {
> > if (list_empty(¶ms->params))
> > return false;
> > @@ -1544,28 +2030,37 @@ static bool rkisp1_params_get_buffer(struct rkisp1_params *params,
> >
> > static void rkisp1_params_complete_buffer(struct rkisp1_params *params,
> > struct rkisp1_buffer *buf,
> > - unsigned int frame_sequence)
> > + unsigned int frame_sequence,
> > + enum vb2_buffer_state state)
> > {
> > list_del(&buf->queue);
> >
> > buf->vb.sequence = frame_sequence;
> > - vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> > + vb2_buffer_done(&buf->vb.vb2_buf, state);
> > }
> >
> > void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> > {
> > struct rkisp1_params *params = &rkisp1->params;
> > - struct rkisp1_params_cfg *new_params;
> > - struct rkisp1_buffer *cur_buf;
> > + struct rkisp1_buffer *buf;
>
> Why the rename?
>
> > + int ret = 0;
> > + void *cfg;
> >
> > spin_lock(¶ms->config_lock);
> >
> > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params))
> > + if (!rkisp1_params_get_buffer(params, &buf, &cfg))
> > goto unlock;
> >
> > - rkisp1_isp_isr_other_config(params, new_params);
> > - rkisp1_isp_isr_lsc_config(params, new_params);
> > - rkisp1_isp_isr_meas_config(params, new_params);
> > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) {
> > + rkisp1_isp_isr_other_config(params, cfg);
> > + rkisp1_isp_isr_lsc_config(params, cfg);
> > + rkisp1_isp_isr_meas_config(params, cfg);
> > + } else {
> > + ret = rkisp1_ext_params_config(params, cfg);
> > + }
> > +
> > + if (ret)
> > + goto complete_and_unlock;
As validation should happen at qbuf time, I think it would be nicer to
reorder the patches to avoid introducing error handling here and
removing it later.
> >
> > /* update shadow register immediately */
> > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > @@ -1579,8 +2074,11 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> > * indicate to userspace on which frame these parameters are being
> > * applied.
> > */
> > - rkisp1_params_complete_buffer(params, cur_buf,
> > - rkisp1->isp.frame_sequence + 1);
> > +complete_and_unlock:
> > + rkisp1_params_complete_buffer(params, buf,
> > + rkisp1->isp.frame_sequence + 1,
> > + ret ? VB2_BUF_STATE_ERROR
> > + : VB2_BUF_STATE_DONE);
> >
> > unlock:
> > spin_unlock(¶ms->config_lock);
> > @@ -1631,8 +2129,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
> > enum v4l2_ycbcr_encoding ycbcr_encoding)
> > {
> > struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config;
> > - struct rkisp1_params_cfg *new_params;
> > - struct rkisp1_buffer *cur_buf;
> > + struct rkisp1_buffer *buf;
> > + int ret = 0;
> > + void *cfg;
> >
> > params->quantization = quantization;
> > params->ycbcr_encoding = ycbcr_encoding;
> > @@ -1661,11 +2160,26 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
> >
> > /* apply the first buffer if there is one already */
> >
> > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params))
> > + if (!rkisp1_params_get_buffer(params, &buf, &cfg))
> > goto unlock;
> >
> > - rkisp1_isp_isr_other_config(params, new_params);
> > - rkisp1_isp_isr_meas_config(params, new_params);
> > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) {
> > + rkisp1_isp_isr_other_config(params, cfg);
> > + rkisp1_isp_isr_meas_config(params, cfg);
> > + } else {
> > + ret = rkisp1_ext_params_other_meas_config(params, cfg);
> > + }
> > +
> > + if (ret) {
> > + /*
> > + * Complete the buffer in error state immediately. In case of no
> > + * error, the buffer will be completed in
> > + * rkisp1_params_post_configure().
> > + */
> > + rkisp1_params_complete_buffer(params, buf, 0,
> > + VB2_BUF_STATE_ERROR);
> > + goto unlock;
> > + }
> >
> > /* update shadow register immediately */
> > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > @@ -1677,8 +2191,9 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
> >
> > void rkisp1_params_post_configure(struct rkisp1_params *params)
> > {
> > - struct rkisp1_params_cfg *new_params;
> > - struct rkisp1_buffer *cur_buf;
> > + struct rkisp1_buffer *buf;
>
> And likewise here?
>
> > + int ret = 0;
> > + void *cfg;
> >
> > spin_lock_irq(¶ms->config_lock);
> >
> > @@ -1691,16 +2206,24 @@ void rkisp1_params_post_configure(struct rkisp1_params *params)
> > * unconditionally.
> > */
> >
> > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params))
> > + if (!rkisp1_params_get_buffer(params, &buf, &cfg))
> > goto unlock;
> >
> > - rkisp1_isp_isr_lsc_config(params, new_params);
> > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS)
> > + rkisp1_isp_isr_lsc_config(params, cfg);
> > + else
> > + ret = rkisp1_ext_params_lsc_config(params, cfg);
> > +
> > + if (ret)
> > + goto complete_and_unlock;
> >
> > /* update shadow register immediately */
> > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
> >
> > - rkisp1_params_complete_buffer(params, cur_buf, 0);
> > +complete_and_unlock:
> > + rkisp1_params_complete_buffer(params, buf, 0, ret ? VB2_BUF_STATE_ERROR
> > + : VB2_BUF_STATE_DONE);
> >
> > unlock:
> > spin_unlock_irq(¶ms->config_lock);
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2024-06-12 15:42 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-05 16:54 [PATCH 0/8] media: rkisp1: Implement support for extensible parameters Jacopo Mondi
2024-06-05 16:54 ` [PATCH 1/8] uapi: rkisp1-config: Add extensible parameters format Jacopo Mondi
2024-06-12 10:02 ` Dan Scally
2024-06-12 12:56 ` Laurent Pinchart
2024-06-12 13:49 ` Dan Scally
2024-06-12 14:56 ` Laurent Pinchart
2024-06-19 12:44 ` Jacopo Mondi
2024-06-19 15:49 ` Laurent Pinchart
2024-06-05 16:54 ` [PATCH 2/8] uapi: videodev2: Add V4L2_META_FMT_RK_ISP1_EXT_PARAMS Jacopo Mondi
2024-06-12 10:00 ` Dan Scally
2024-06-12 14:35 ` Laurent Pinchart
2024-06-12 15:09 ` Dan Scally
2024-06-20 9:31 ` Paul Elder
2024-06-05 16:54 ` [PATCH 3/8] media: rkisp1: Remove cached format info Jacopo Mondi
2024-06-12 10:06 ` Dan Scally
2024-06-12 14:47 ` Laurent Pinchart
2024-06-20 9:41 ` Paul Elder
2024-06-05 16:54 ` [PATCH 4/8] media: rkisp1: Add support for ext format Jacopo Mondi
2024-06-12 10:46 ` Dan Scally
2024-06-12 14:51 ` Laurent Pinchart
2024-06-05 16:54 ` [PATCH 5/8] media: rkisp1: Implement extensible params support Jacopo Mondi
2024-06-12 13:50 ` Dan Scally
2024-06-12 15:42 ` Laurent Pinchart [this message]
2024-06-19 15:46 ` Jacopo Mondi
2024-06-19 16:09 ` Laurent Pinchart
2024-06-05 16:54 ` [PATCH 6/8] media: rkisp1: Propagate pre/post-config errors Jacopo Mondi
2024-06-12 13:35 ` Dan Scally
2024-06-12 15:46 ` Laurent Pinchart
2024-06-05 16:54 ` [PATCH 7/8] media: rkisp1: Add struct rkisp1_params_buffer Jacopo Mondi
2024-06-12 13:52 ` Dan Scally
2024-06-05 16:54 ` [PATCH 8/8] media: rkisp1: Copy and validate parameters buffer Jacopo Mondi
2024-06-12 14:28 ` Dan Scally
2024-06-12 16:20 ` Laurent Pinchart
2024-06-19 14:22 ` Jacopo Mondi
2024-06-19 15:44 ` Laurent Pinchart
2024-06-19 15:55 ` Jacopo Mondi
2024-06-19 16:11 ` Laurent Pinchart
2024-06-19 16:20 ` Jacopo Mondi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240612154201.GD15991@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=dafna@fastmail.com \
--cc=dan.scally@ideasonboard.com \
--cc=heiko@sntech.de \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jacopo.mondi@ideasonboard.com \
--cc=kieran.bingham@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=paul.elder@ideasonboard.com \
--cc=sakari.ailus@iki.fi \
--cc=stefan.klug@ideasonboard.com \
--cc=umang.jain@ideasonboard.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.