Re: [PATCH v1 4/5] media: rkisp1: Add feature flags for BLS and compand

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thu, Jul 04, 2024 at 10:34:20AM +0200, Jacopo Mondi wrote:
> On Thu, Jul 04, 2024 at 01:25:32AM GMT, Laurent Pinchart wrote:
> > From: Paul Elder <paul.elder@ideasonboard.com>
> >
> > Add feature flags for the dedicated black level subtraction hardware
> > block and for the compand hardware block. The companding feature flag is
> > added on its own (as opposed to "the absence of BLS") because we will
> > need it later for when we add support for the companding block.
> >
> > Skip BLS configuration when the BLS feature flag is unset, as devices
> > without the dedicated BLS block cannot configure a hardware block that
> > doesn't exist.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 4 ++++
> >  drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c    | 9 ++++++---
> >  drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 7 +++++++
> >  3 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index cdf2d30e2bb1..607e1a024d02 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -116,6 +116,8 @@ enum rkisp1_isp_pad {
> >   * @RKISP1_FEATURE_SELF_PATH: The ISP has a self path
> >   * @RKISP1_FEATURE_DUAL_CROP: The ISP has the dual crop block at the resizer input
> >   * @RKISP1_FEATURE_DMA_34BIT: The ISP uses 34-bit DMA addresses
> > + * @RKISP1_FEATURE_BLS: The ISP has a dedicated BLS block
> > + * @RKISP1_FEATURE_COMPAND: The ISP has a companding block
> >   *
> >   * The ISP features are stored in a bitmask in &rkisp1_info.features and allow
> >   * the driver to implement support for features present in some ISP versions
> > @@ -127,6 +129,8 @@ enum rkisp1_feature {
> >  	RKISP1_FEATURE_SELF_PATH = BIT(2),
> >  	RKISP1_FEATURE_DUAL_CROP = BIT(3),
> >  	RKISP1_FEATURE_DMA_34BIT = BIT(4),
> > +	RKISP1_FEATURE_BLS = BIT(5),
> > +	RKISP1_FEATURE_COMPAND = BIT(6),
> >  };
> >
> >  #define rkisp1_has_feature(rkisp1, feature) \
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > index 0535ce57e862..dd114ab77800 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > @@ -509,7 +509,8 @@ static const struct rkisp1_info px30_isp_info = {
> >  	.isp_ver = RKISP1_V12,
> >  	.features = RKISP1_FEATURE_MIPI_CSI2
> >  		  | RKISP1_FEATURE_SELF_PATH
> > -		  | RKISP1_FEATURE_DUAL_CROP,
> > +		  | RKISP1_FEATURE_DUAL_CROP
> > +		  | RKISP1_FEATURE_BLS,
> 
> Doesn't apply for me on top of [media-stage/master + my ext params
> series]
> 
> In particular, in media-stage I don't see
> >  	.max_width = 3264,
> >  	.max_height = 2448,
> 
> these

There are a few prerequisites. The cover letter lists the base commit
ID. The fact that I haven't pushed a branch anywhere doesn't help
obviously... I'll make it clearer in v2.

> >  };
> > @@ -532,7 +533,8 @@ static const struct rkisp1_info rk3399_isp_info = {
> >  	.isp_ver = RKISP1_V10,
> >  	.features = RKISP1_FEATURE_MIPI_CSI2
> >  		  | RKISP1_FEATURE_SELF_PATH
> > -		  | RKISP1_FEATURE_DUAL_CROP,
> > +		  | RKISP1_FEATURE_DUAL_CROP
> > +		  | RKISP1_FEATURE_BLS,
> >  	.max_width = 4416,
> >  	.max_height = 3312,
> >  };
> > @@ -554,7 +556,8 @@ static const struct rkisp1_info imx8mp_isp_info = {
> >  	.isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
> >  	.isp_ver = RKISP1_V_IMX8MP,
> >  	.features = RKISP1_FEATURE_MAIN_STRIDE
> > -		  | RKISP1_FEATURE_DMA_34BIT,
> > +		  | RKISP1_FEATURE_DMA_34BIT
> > +		  | RKISP1_FEATURE_COMPAND,
> >  	.max_width = 4096,
> >  	.max_height = 3072,
> >  };
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 92312b4dabf6..bac9d4972493 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -1268,6 +1268,12 @@ rkisp1_isp_isr_other_config(struct rkisp1_params *params,
> >  	module_cfg_update = new_params->module_cfg_update;
> >  	module_ens = new_params->module_ens;
> >
> > +	if (!rkisp1_has_feature(params->rkisp1, BLS)) {
> > +		module_en_update &= ~RKISP1_CIF_ISP_MODULE_BLS;
> > +		module_cfg_update &= ~RKISP1_CIF_ISP_MODULE_BLS;
> > +		module_ens &= ~RKISP1_CIF_ISP_MODULE_BLS;
> > +	}
> > +
> 
> or is it easier to read if you
> 
> 	if (rkisp1_has_feature(params->rkisp1, BLS)) {
>                 /* update bls config */
>                 if (module_cfg_update & RKISP1_CIF_ISP_MODULE_BLS)
>                         rkisp1_bls_config(params,
>                                           &new_params->others.bls_config);
> 
>                 if (module_en_update & RKISP1_CIF_ISP_MODULE_BLS) {
>                         if (module_ens & RKISP1_CIF_ISP_MODULE_BLS)
>                                 rkisp1_param_set_bits(params,
>                                                       RKISP1_CIF_ISP_BLS_CTRL,
>                                                       RKISP1_CIF_ISP_BLS_ENA);
>                         else
>                                 rkisp1_param_clear_bits(params,
>                                                         RKISP1_CIF_ISP_BLS_CTRL,
>                                                         RKISP1_CIF_ISP_BLS_ENA);
>                 }
>         }
> 
> below ?

I was considering it. Lower indentation is nice, and I thought that we
could centralize all the feature checks in one place, but I don't mind
much either way. If you have a stronger preference I can change this.

> >  	/* update dpc config */
> >  	if (module_cfg_update & RKISP1_CIF_ISP_MODULE_DPCC)
> >  		rkisp1_dpcc_config(params,
> > @@ -1851,6 +1857,7 @@ static const struct rkisp1_ext_params_handler {
> >  		.size		= sizeof(struct rkisp1_ext_params_bls_config),
> >  		.handler	= rkisp1_ext_params_bls,
> >  		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > +		.features	= RKISP1_FEATURE_BLS,
> >  	},
> >  	[RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = {
> >  		.size		= sizeof(struct rkisp1_ext_params_dpcc_config),

-- 
Regards,

Laurent Pinchart