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@xxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > 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
