[v8,3/7] ipa: rkisp1: algorithms: dpf: Refactor prepare() into helpers
diff mbox series

Message ID 20260115163318.1339354-4-rui.wang@ideasonboard.com
State Superseded
Headers show
Series
  • refactor DPF parsing and initialization
Related show

Commit Message

Rui Wang Jan. 15, 2026, 4:33 p.m. UTC 
Split the prepare() function into prepareDisabledMode() and
prepareEnabledMode() to improve readability and maintainability.

This separates the logic for disabling and enabling the DPF, making
the main prepare() function cleaner and easier to follow.

Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

---
changelog since v5i/v6:  Nothing changed

 Reviewed-by tags from v5 are carried over (no code changes).
---
 src/ipa/rkisp1/algorithms/dpf.cpp | 80 ++++++++++++++++++++-----------
 src/ipa/rkisp1/algorithms/dpf.h   |  7 +++
 2 files changed, 58 insertions(+), 29 deletions(-)

Comments

Jacopo Mondi Jan. 16, 2026, 9:42 a.m. UTC | #1 
Hi Rui
   + Stefan

On Thu, Jan 15, 2026 at 11:33:14AM -0500, Rui Wang wrote:
> Split the prepare() function into prepareDisabledMode() and
> prepareEnabledMode() to improve readability and maintainability.
>
> This separates the logic for disabling and enabling the DPF, making
> the main prepare() function cleaner and easier to follow.
>
> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> ---
> changelog since v5i/v6:  Nothing changed
>
>  Reviewed-by tags from v5 are carried over (no code changes).
> ---
>  src/ipa/rkisp1/algorithms/dpf.cpp | 80 ++++++++++++++++++++-----------
>  src/ipa/rkisp1/algorithms/dpf.h   |  7 +++
>  2 files changed, 58 insertions(+), 29 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> index 9b663831..09f2bcbd 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> @@ -316,38 +316,60 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,
>  	if (!frameContext.dpf.update && frame > 0)
>  		return;

With the new tuning file layout there is no default mode anymore, so
the algorithm starts with Dpf disabled by default and it is only
enabled once the user selects one modes at queueRequest() time.

Is my understanding correct ?

Question for Stefan and other ones who has been developing more
algorithms for RkISP1: is this aligned with how other algorithms work
? In other word, there is no way to enable the algo from tuning file
but only with Controls. I think it is fine, just checking.

>
> +	if (!frameContext.dpf.denoise) {
> +		prepareDisabledMode(context, frame, frameContext, params);
> +		return;
> +	}
> +
> +	prepareEnabledMode(context, frame, frameContext, params);
> +}
> +
> +void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,
> +			      [[maybe_unused]] const uint32_t frame,
> +			      [[maybe_unused]] IPAFrameContext &frameContext,

So why you pass them to the function if you know know you're not going
to use them ?

> +			      RkISP1Params *params)
> +{
> +	auto dpfConfig = params->block<BlockType::Dpf>();
> +	dpfConfig.setEnabled(false);
> +	auto dpfStrength = params->block<BlockType::DpfStrength>();
> +	dpfStrength.setEnabled(false);
> +}
> +
> +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> +			     [[maybe_unused]] IPAFrameContext &frameContext,

Same question, if you know you're not going to use the function
parameters, why do you have them here ?

> +			     RkISP1Params *params)
> +{
>  	auto config = params->block<BlockType::Dpf>();
> -	config.setEnabled(frameContext.dpf.denoise);
> +	config.setEnabled(true);
> +	*config = config_;
> +
> +	const auto &awb = context.configuration.awb;
> +	const auto &lsc = context.configuration.lsc;
> +
> +	auto &mode = config->gain.mode;
> +
> +	/*
> +	 * The DPF needs to take into account the total amount of
> +	 * digital gain, which comes from the AWB and LSC modules. The
> +	 * DPF hardware can be programmed with a digital gain value
> +	 * manually, but can also use the gains supplied by the AWB and
> +	 * LSC modules automatically when they are enabled. Use that
> +	 * mode of operation as it simplifies control of the DPF.
> +	 */
> +	if (awb.enabled && lsc.enabled)
> +		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
> +	else if (awb.enabled)
> +		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
> +	else if (lsc.enabled)
> +		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> +	else
> +		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> +
> +	config_.gain.mode = mode;
>
>  	auto strengthConfig = params->block<BlockType::DpfStrength>();
> -	strengthConfig.setEnabled(frameContext.dpf.denoise);
> -
> -	if (frameContext.dpf.denoise) {
> -		*config = config_;
> -		*strengthConfig = strengthConfig_;
> -
> -		const auto &awb = context.configuration.awb;
> -		const auto &lsc = context.configuration.lsc;
> -
> -		auto &mode = config->gain.mode;
> -
> -		/*
> -		 * The DPF needs to take into account the total amount of
> -		 * digital gain, which comes from the AWB and LSC modules. The
> -		 * DPF hardware can be programmed with a digital gain value
> -		 * manually, but can also use the gains supplied by the AWB and
> -		 * LSC modules automatically when they are enabled. Use that
> -		 * mode of operation as it simplifies control of the DPF.
> -		 */
> -		if (awb.enabled && lsc.enabled)
> -			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
> -		else if (awb.enabled)
> -			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
> -		else if (lsc.enabled)
> -			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> -		else
> -			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> -	}
> +	strengthConfig.setEnabled(true);
> +	*strengthConfig = strengthConfig_;
>  }
>
>  REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> index 2a2c7052..fcf121cb 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.h
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -44,6 +44,13 @@ private:
>
>  	bool loadReductionConfig(int32_t mode);
>
> +	void prepareDisabledMode(IPAContext &context, const uint32_t frame,
> +				 IPAFrameContext &frameContext,
> +				 RkISP1Params *params);
> +	void prepareEnabledMode(IPAContext &context, const uint32_t frame,
> +				IPAFrameContext &frameContext,
> +				RkISP1Params *params);
> +
>  	struct rkisp1_cif_isp_dpf_config config_;
>  	struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
>  	std::vector<ModeConfig> noiseReductionModes_;
> --
> 2.43.0
>
Stefan Klug Jan. 16, 2026, 1:38 p.m. UTC | #2 
Hi Rui, hi Jacopo,

Quoting Jacopo Mondi (2026-01-16 10:42:54)
> Hi Rui
>    + Stefan
> 
> On Thu, Jan 15, 2026 at 11:33:14AM -0500, Rui Wang wrote:
> > Split the prepare() function into prepareDisabledMode() and
> > prepareEnabledMode() to improve readability and maintainability.
> >
> > This separates the logic for disabling and enabling the DPF, making
> > the main prepare() function cleaner and easier to follow.
> >
> > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > ---
> > changelog since v5i/v6:  Nothing changed
> >
> >  Reviewed-by tags from v5 are carried over (no code changes).
> > ---
> >  src/ipa/rkisp1/algorithms/dpf.cpp | 80 ++++++++++++++++++++-----------
> >  src/ipa/rkisp1/algorithms/dpf.h   |  7 +++
> >  2 files changed, 58 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> > index 9b663831..09f2bcbd 100644
> > --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> > @@ -316,38 +316,60 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,
> >       if (!frameContext.dpf.update && frame > 0)
> >               return;
> 
> With the new tuning file layout there is no default mode anymore, so
> the algorithm starts with Dpf disabled by default and it is only
> enabled once the user selects one modes at queueRequest() time.
> 
> Is my understanding correct ?
> 
> Question for Stefan and other ones who has been developing more
> algorithms for RkISP1: is this aligned with how other algorithms work
> ? In other word, there is no way to enable the algo from tuning file
> but only with Controls. I think it is fine, just checking.

The unwritten expectation is afaik that a camera starts up with a
default good image (whatever good in that sense is). So I believe that
depending on the sensor it might very well make sense to enable a bit of
denoising.  Or to turn the question upside down. It would be frustrating
to users if they can't configure their preferred default denoising in
the tuning file. So I think we should keep that option.

Best regards,
Stefan
 
> >
> > +     if (!frameContext.dpf.denoise) {
> > +             prepareDisabledMode(context, frame, frameContext, params);
> > +             return;
> > +     }
> > +
> > +     prepareEnabledMode(context, frame, frameContext, params);
> > +}
> > +
> > +void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,
> > +                           [[maybe_unused]] const uint32_t frame,
> > +                           [[maybe_unused]] IPAFrameContext &frameContext,
> 
> So why you pass them to the function if you know know you're not going
> to use them ?
> 
> > +                           RkISP1Params *params)
> > +{
> > +     auto dpfConfig = params->block<BlockType::Dpf>();
> > +     dpfConfig.setEnabled(false);
> > +     auto dpfStrength = params->block<BlockType::DpfStrength>();
> > +     dpfStrength.setEnabled(false);
> > +}
> > +
> > +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > +                          [[maybe_unused]] IPAFrameContext &frameContext,
> 
> Same question, if you know you're not going to use the function
> parameters, why do you have them here ?
> 
> > +                          RkISP1Params *params)
> > +{
> >       auto config = params->block<BlockType::Dpf>();
> > -     config.setEnabled(frameContext.dpf.denoise);
> > +     config.setEnabled(true);
> > +     *config = config_;
> > +
> > +     const auto &awb = context.configuration.awb;
> > +     const auto &lsc = context.configuration.lsc;
> > +
> > +     auto &mode = config->gain.mode;
> > +
> > +     /*
> > +      * The DPF needs to take into account the total amount of
> > +      * digital gain, which comes from the AWB and LSC modules. The
> > +      * DPF hardware can be programmed with a digital gain value
> > +      * manually, but can also use the gains supplied by the AWB and
> > +      * LSC modules automatically when they are enabled. Use that
> > +      * mode of operation as it simplifies control of the DPF.
> > +      */
> > +     if (awb.enabled && lsc.enabled)
> > +             mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
> > +     else if (awb.enabled)
> > +             mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
> > +     else if (lsc.enabled)
> > +             mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> > +     else
> > +             mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> > +
> > +     config_.gain.mode = mode;
> >
> >       auto strengthConfig = params->block<BlockType::DpfStrength>();
> > -     strengthConfig.setEnabled(frameContext.dpf.denoise);
> > -
> > -     if (frameContext.dpf.denoise) {
> > -             *config = config_;
> > -             *strengthConfig = strengthConfig_;
> > -
> > -             const auto &awb = context.configuration.awb;
> > -             const auto &lsc = context.configuration.lsc;
> > -
> > -             auto &mode = config->gain.mode;
> > -
> > -             /*
> > -              * The DPF needs to take into account the total amount of
> > -              * digital gain, which comes from the AWB and LSC modules. The
> > -              * DPF hardware can be programmed with a digital gain value
> > -              * manually, but can also use the gains supplied by the AWB and
> > -              * LSC modules automatically when they are enabled. Use that
> > -              * mode of operation as it simplifies control of the DPF.
> > -              */
> > -             if (awb.enabled && lsc.enabled)
> > -                     mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
> > -             else if (awb.enabled)
> > -                     mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
> > -             else if (lsc.enabled)
> > -                     mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> > -             else
> > -                     mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> > -     }
> > +     strengthConfig.setEnabled(true);
> > +     *strengthConfig = strengthConfig_;
> >  }
> >
> >  REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
> > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> > index 2a2c7052..fcf121cb 100644
> > --- a/src/ipa/rkisp1/algorithms/dpf.h
> > +++ b/src/ipa/rkisp1/algorithms/dpf.h
> > @@ -44,6 +44,13 @@ private:
> >
> >       bool loadReductionConfig(int32_t mode);
> >
> > +     void prepareDisabledMode(IPAContext &context, const uint32_t frame,
> > +                              IPAFrameContext &frameContext,
> > +                              RkISP1Params *params);
> > +     void prepareEnabledMode(IPAContext &context, const uint32_t frame,
> > +                             IPAFrameContext &frameContext,
> > +                             RkISP1Params *params);
> > +
> >       struct rkisp1_cif_isp_dpf_config config_;
> >       struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
> >       std::vector<ModeConfig> noiseReductionModes_;
> > --
> > 2.43.0
> >
Jacopo Mondi Jan. 16, 2026, 2:28 p.m. UTC | #3 
Hi Stefan

On Fri, Jan 16, 2026 at 02:38:03PM +0100, Stefan Klug wrote:
> Hi Rui, hi Jacopo,
>
> Quoting Jacopo Mondi (2026-01-16 10:42:54)
> > Hi Rui
> >    + Stefan
> >
> > On Thu, Jan 15, 2026 at 11:33:14AM -0500, Rui Wang wrote:
> > > Split the prepare() function into prepareDisabledMode() and
> > > prepareEnabledMode() to improve readability and maintainability.
> > >
> > > This separates the logic for disabling and enabling the DPF, making
> > > the main prepare() function cleaner and easier to follow.
> > >
> > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >
> > > ---
> > > changelog since v5i/v6:  Nothing changed
> > >
> > >  Reviewed-by tags from v5 are carried over (no code changes).
> > > ---
> > >  src/ipa/rkisp1/algorithms/dpf.cpp | 80 ++++++++++++++++++++-----------
> > >  src/ipa/rkisp1/algorithms/dpf.h   |  7 +++
> > >  2 files changed, 58 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> > > index 9b663831..09f2bcbd 100644
> > > --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> > > @@ -316,38 +316,60 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,
> > >       if (!frameContext.dpf.update && frame > 0)
> > >               return;
> >
> > With the new tuning file layout there is no default mode anymore, so
> > the algorithm starts with Dpf disabled by default and it is only
> > enabled once the user selects one modes at queueRequest() time.
> >
> > Is my understanding correct ?
> >
> > Question for Stefan and other ones who has been developing more
> > algorithms for RkISP1: is this aligned with how other algorithms work
> > ? In other word, there is no way to enable the algo from tuning file
> > but only with Controls. I think it is fine, just checking.
>
> The unwritten expectation is afaik that a camera starts up with a
> default good image (whatever good in that sense is). So I believe that
> depending on the sensor it might very well make sense to enable a bit of
> denoising.  Or to turn the question upside down. It would be frustrating
> to users if they can't configure their preferred default denoising in
> the tuning file. So I think we should keep that option.

I see.

Originally Rui proposed a scheme for the tuning file like the one
described in https://patchwork.libcamera.org/patch/25385/#37366

I'll paste here below for convenience

--------------------------------------------------------------------------------
The tuning file layout is the following one

  - Dpf:
      filter:
      nll:
      strength:
      modes:
        - type: "minimal"
          filter:
          nll:
          strength:
        - type: "highquality"
          filter:
          nll:
          strength:
        - type: "fast"
          filter:
          nll:
          strength:
        - type: "zsl"
          filter:
          nll:
          strength:

Each entry in "modes" is .. a mode, with it's own configuration.
BUT: what do the outer configuration refers to ? What is its purpose ?
It is not associated to any "mode", how can user select it ?

  - Dpf:
      filter:           \
      nll:              | -> This one ??
      strength:         /
      modes:
      - type: ...
-------------------------------------------------------------------------------

The "external" configuration was used as a default and for manual/auto
mode switching (if I got it right).

I suggested Rui to remove it as users could only select one mode or
to disable the Dpf using Controls, so the "extenal" mode was
effectivelly not user selectable.

However if we want to start with a mode and we want to give users the
ability to decide using the tuning file if Dpf should be enable,
should we maybe add a property like "enabledMode: ". In example:

  - Dpf:
    modes:
      - type: "minimal"
        filter:
        nll:
        strength:
      - type: "highquality"
        filter:
        nll:
        strength:
      - type: "fast"
        filter:
        nll:
        strength:
      - type: "zsl"
        filter:
        nll:
        strength:
    enabledMode: "minimal"

so that:

- if "enabledMode:" is specified Dpf is started with this mode
  configured
- if it is not specified Dpf is disabled by default
- We don't need an "external"/"default" configuration which is not
  user-selectable like the previous version of the tuning file had

What do you and Rui think ?

Thanks
  j

>
> Best regards,
> Stefan
>
> > >
> > > +     if (!frameContext.dpf.denoise) {
> > > +             prepareDisabledMode(context, frame, frameContext, params);
> > > +             return;
> > > +     }
> > > +
> > > +     prepareEnabledMode(context, frame, frameContext, params);
> > > +}
> > > +
> > > +void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,
> > > +                           [[maybe_unused]] const uint32_t frame,
> > > +                           [[maybe_unused]] IPAFrameContext &frameContext,
> >
> > So why you pass them to the function if you know know you're not going
> > to use them ?
> >
> > > +                           RkISP1Params *params)
> > > +{
> > > +     auto dpfConfig = params->block<BlockType::Dpf>();
> > > +     dpfConfig.setEnabled(false);
> > > +     auto dpfStrength = params->block<BlockType::DpfStrength>();
> > > +     dpfStrength.setEnabled(false);
> > > +}
> > > +
> > > +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > +                          [[maybe_unused]] IPAFrameContext &frameContext,
> >
> > Same question, if you know you're not going to use the function
> > parameters, why do you have them here ?
> >
> > > +                          RkISP1Params *params)
> > > +{
> > >       auto config = params->block<BlockType::Dpf>();
> > > -     config.setEnabled(frameContext.dpf.denoise);
> > > +     config.setEnabled(true);
> > > +     *config = config_;
> > > +
> > > +     const auto &awb = context.configuration.awb;
> > > +     const auto &lsc = context.configuration.lsc;
> > > +
> > > +     auto &mode = config->gain.mode;
> > > +
> > > +     /*
> > > +      * The DPF needs to take into account the total amount of
> > > +      * digital gain, which comes from the AWB and LSC modules. The
> > > +      * DPF hardware can be programmed with a digital gain value
> > > +      * manually, but can also use the gains supplied by the AWB and
> > > +      * LSC modules automatically when they are enabled. Use that
> > > +      * mode of operation as it simplifies control of the DPF.
> > > +      */
> > > +     if (awb.enabled && lsc.enabled)
> > > +             mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
> > > +     else if (awb.enabled)
> > > +             mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
> > > +     else if (lsc.enabled)
> > > +             mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> > > +     else
> > > +             mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> > > +
> > > +     config_.gain.mode = mode;
> > >
> > >       auto strengthConfig = params->block<BlockType::DpfStrength>();
> > > -     strengthConfig.setEnabled(frameContext.dpf.denoise);
> > > -
> > > -     if (frameContext.dpf.denoise) {
> > > -             *config = config_;
> > > -             *strengthConfig = strengthConfig_;
> > > -
> > > -             const auto &awb = context.configuration.awb;
> > > -             const auto &lsc = context.configuration.lsc;
> > > -
> > > -             auto &mode = config->gain.mode;
> > > -
> > > -             /*
> > > -              * The DPF needs to take into account the total amount of
> > > -              * digital gain, which comes from the AWB and LSC modules. The
> > > -              * DPF hardware can be programmed with a digital gain value
> > > -              * manually, but can also use the gains supplied by the AWB and
> > > -              * LSC modules automatically when they are enabled. Use that
> > > -              * mode of operation as it simplifies control of the DPF.
> > > -              */
> > > -             if (awb.enabled && lsc.enabled)
> > > -                     mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
> > > -             else if (awb.enabled)
> > > -                     mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
> > > -             else if (lsc.enabled)
> > > -                     mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> > > -             else
> > > -                     mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> > > -     }
> > > +     strengthConfig.setEnabled(true);
> > > +     *strengthConfig = strengthConfig_;
> > >  }
> > >
> > >  REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
> > > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> > > index 2a2c7052..fcf121cb 100644
> > > --- a/src/ipa/rkisp1/algorithms/dpf.h
> > > +++ b/src/ipa/rkisp1/algorithms/dpf.h
> > > @@ -44,6 +44,13 @@ private:
> > >
> > >       bool loadReductionConfig(int32_t mode);
> > >
> > > +     void prepareDisabledMode(IPAContext &context, const uint32_t frame,
> > > +                              IPAFrameContext &frameContext,
> > > +                              RkISP1Params *params);
> > > +     void prepareEnabledMode(IPAContext &context, const uint32_t frame,
> > > +                             IPAFrameContext &frameContext,
> > > +                             RkISP1Params *params);
> > > +
> > >       struct rkisp1_cif_isp_dpf_config config_;
> > >       struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
> > >       std::vector<ModeConfig> noiseReductionModes_;
> > > --
> > > 2.43.0
> > >

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
index 9b663831..09f2bcbd 100644
--- a/src/ipa/rkisp1/algorithms/dpf.cpp
+++ b/src/ipa/rkisp1/algorithms/dpf.cpp
@@ -316,38 +316,60 @@  void Dpf::prepare(IPAContext &context, const uint32_t frame,
 	if (!frameContext.dpf.update && frame > 0)
 		return;
 
+	if (!frameContext.dpf.denoise) {
+		prepareDisabledMode(context, frame, frameContext, params);
+		return;
+	}
+
+	prepareEnabledMode(context, frame, frameContext, params);
+}
+
+void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,
+			      [[maybe_unused]] const uint32_t frame,
+			      [[maybe_unused]] IPAFrameContext &frameContext,
+			      RkISP1Params *params)
+{
+	auto dpfConfig = params->block<BlockType::Dpf>();
+	dpfConfig.setEnabled(false);
+	auto dpfStrength = params->block<BlockType::DpfStrength>();
+	dpfStrength.setEnabled(false);
+}
+
+void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,
+			     [[maybe_unused]] IPAFrameContext &frameContext,
+			     RkISP1Params *params)
+{
 	auto config = params->block<BlockType::Dpf>();
-	config.setEnabled(frameContext.dpf.denoise);
+	config.setEnabled(true);
+	*config = config_;
+
+	const auto &awb = context.configuration.awb;
+	const auto &lsc = context.configuration.lsc;
+
+	auto &mode = config->gain.mode;
+
+	/*
+	 * The DPF needs to take into account the total amount of
+	 * digital gain, which comes from the AWB and LSC modules. The
+	 * DPF hardware can be programmed with a digital gain value
+	 * manually, but can also use the gains supplied by the AWB and
+	 * LSC modules automatically when they are enabled. Use that
+	 * mode of operation as it simplifies control of the DPF.
+	 */
+	if (awb.enabled && lsc.enabled)
+		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
+	else if (awb.enabled)
+		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
+	else if (lsc.enabled)
+		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
+	else
+		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
+
+	config_.gain.mode = mode;
 
 	auto strengthConfig = params->block<BlockType::DpfStrength>();
-	strengthConfig.setEnabled(frameContext.dpf.denoise);
-
-	if (frameContext.dpf.denoise) {
-		*config = config_;
-		*strengthConfig = strengthConfig_;
-
-		const auto &awb = context.configuration.awb;
-		const auto &lsc = context.configuration.lsc;
-
-		auto &mode = config->gain.mode;
-
-		/*
-		 * The DPF needs to take into account the total amount of
-		 * digital gain, which comes from the AWB and LSC modules. The
-		 * DPF hardware can be programmed with a digital gain value
-		 * manually, but can also use the gains supplied by the AWB and
-		 * LSC modules automatically when they are enabled. Use that
-		 * mode of operation as it simplifies control of the DPF.
-		 */
-		if (awb.enabled && lsc.enabled)
-			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
-		else if (awb.enabled)
-			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
-		else if (lsc.enabled)
-			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
-		else
-			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
-	}
+	strengthConfig.setEnabled(true);
+	*strengthConfig = strengthConfig_;
 }
 
 REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
index 2a2c7052..fcf121cb 100644
--- a/src/ipa/rkisp1/algorithms/dpf.h
+++ b/src/ipa/rkisp1/algorithms/dpf.h
@@ -44,6 +44,13 @@  private:
 
 	bool loadReductionConfig(int32_t mode);
 
+	void prepareDisabledMode(IPAContext &context, const uint32_t frame,
+				 IPAFrameContext &frameContext,
+				 RkISP1Params *params);
+	void prepareEnabledMode(IPAContext &context, const uint32_t frame,
+				IPAFrameContext &frameContext,
+				RkISP1Params *params);
+
 	struct rkisp1_cif_isp_dpf_config config_;
 	struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
 	std::vector<ModeConfig> noiseReductionModes_;