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

Message ID 20251208004808.1274417-4-rui.wang@ideasonboard.com
State New
Headers show
Series
  • rebase_dpf_refactory_patch_v4
Related show

Commit Message

Rui Wang Dec. 8, 2025, 12:48 a.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>
---
changelog:
 - Move V3 prepareEnabledMode/prepareDisabledMode into this patch

 src/ipa/rkisp1/algorithms/dpf.cpp | 82 ++++++++++++++++++++-----------
 src/ipa/rkisp1/algorithms/dpf.h   |  9 ++++
 2 files changed, 62 insertions(+), 29 deletions(-)

Comments

Jacopo Mondi Dec. 11, 2025, 3:46 p.m. UTC | #1 
Hi Rui

On Sun, Dec 07, 2025 at 07:48:04PM -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>
> ---
> changelog:
>  - Move V3 prepareEnabledMode/prepareDisabledMode into this patch
>
>  src/ipa/rkisp1/algorithms/dpf.cpp | 82 ++++++++++++++++++++-----------
>  src/ipa/rkisp1/algorithms/dpf.h   |  9 ++++
>  2 files changed, 62 insertions(+), 29 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> index 18f2a158..edb4c2bf 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> @@ -320,38 +320,62 @@ 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)
> +{
> +	frameContext.dpf.denoise = false;

If you get here, frameContext.dpf.denoise was false already

> +	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;

Add an empty line here

>  	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);
> +

And drop this one

> +	*strengthConfig = strengthConfig_;
>  }
>
>  REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> index 30cbaa57..99cdbdd3 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.h
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -44,6 +44,15 @@ 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);
> +

	void prepareDisabledMode(IPAContext &context, const uint32_t frame,
				 IPAFrameContext &frameContext,
				 RkISP1Params *params);
	void prepareEnabledMode(IPAContext &context, const uint32_t frame,
				IPAFrameContext &frameContext,
				RkISP1Params *params);

nits apart
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>  	struct rkisp1_cif_isp_dpf_config config_;
>  	struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
>  	std::vector<ModeConfig> noiseReductionModes_;
> --
> 2.43.0
>
Rui Wang Dec. 14, 2025, 1:41 a.m. UTC | #2 
Jacopo Mondi wrote:
> Hi Rui
> 
> On Sun, Dec 07, 2025 at 07:48:04PM -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>
> > ---
> > changelog:
> >  - Move V3 prepareEnabledMode/prepareDisabledMode into this patch
> >
> >  src/ipa/rkisp1/algorithms/dpf.cpp | 82 ++++++++++++++++++++-----------
> >  src/ipa/rkisp1/algorithms/dpf.h   |  9 ++++
> >  2 files changed, 62 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> > index 18f2a158..edb4c2bf 100644
> > --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> > @@ -320,38 +320,62 @@ 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)
> > +{
> > +	frameContext.dpf.denoise = false;
> 
> If you get here, frameContext.dpf.denoise was false already
>

Yes , looks this flag update is redudent , will remove in next series

> > +	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;
> 
> Add an empty line here
> 
> >  	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);
> > +
> 
> And drop this one
> 
will delete this blank line

> > +	*strengthConfig = strengthConfig_;
> >  }
> >
> >  REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
> > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> > index 30cbaa57..99cdbdd3 100644
> > --- a/src/ipa/rkisp1/algorithms/dpf.h
> > +++ b/src/ipa/rkisp1/algorithms/dpf.h
> > @@ -44,6 +44,15 @@ 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);
> > +
> 
> 	void prepareDisabledMode(IPAContext &context, const uint32_t frame,
> 				 IPAFrameContext &frameContext,
> 				 RkISP1Params *params);
> 	void prepareEnabledMode(IPAContext &context, const uint32_t frame,
> 				IPAFrameContext &frameContext,
> 				RkISP1Params *params);
> 
> nits apart
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>   j
> 
> >  	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 18f2a158..edb4c2bf 100644
--- a/src/ipa/rkisp1/algorithms/dpf.cpp
+++ b/src/ipa/rkisp1/algorithms/dpf.cpp
@@ -320,38 +320,62 @@  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)
+{
+	frameContext.dpf.denoise = false;
+	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 30cbaa57..99cdbdd3 100644
--- a/src/ipa/rkisp1/algorithms/dpf.h
+++ b/src/ipa/rkisp1/algorithms/dpf.h
@@ -44,6 +44,15 @@  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_;