diff mbox series

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

Message ID 20240703222533.1662-5-laurent.pinchart@ideasonboard.com (mailing list archive)
State New
Headers show
Series media: rkisp1: Add support for the companding block | expand

Commit Message

Laurent Pinchart July 3, 2024, 10:25 p.m. UTC 
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(-)

Comments

Jacopo Mondi July 4, 2024, 8:34 a.m. UTC | #1 
Hi Laurent

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

>  };
> @@ -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 ?


>  	/* 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
>
Laurent Pinchart July 4, 2024, 9 a.m. UTC | #2 
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),
diff mbox series

Patch

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,
 	.max_width = 3264,
 	.max_height = 2448,
 };
@@ -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;
+	}
+
 	/* 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),