[v4,6/7] ipa: rkisp1: algorithms: dpf: Add detailed config logging
diff mbox series

Message ID 20251208004808.1274417-7-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 
Add logConfigIfChanged() helper function to log DPF configuration
updates when they occur. This provides visibility into the active
DPF parameters including:

- Control mode and denoise enable state
- Filter sizes (9x9 vs 13x9 for rb)
- NLL scale mode (linear vs logarithmic)
- Gain mode
- Strength values (r, g, b)
- Spatial filter coefficients (g and rb arrays)
- Noise level lookup table coefficients

The logging is triggered in prepareEnabledMode() whenever the
configuration is updated, helping with debugging and tuning.

Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
---

changelog:
 -Move log information into a seperate patch in series

 src/ipa/rkisp1/algorithms/dpf.cpp | 49 +++++++++++++++++++++++++++++++
 src/ipa/rkisp1/algorithms/dpf.h   |  1 +
 2 files changed, 50 insertions(+)

Comments

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

On Sun, Dec 07, 2025 at 07:48:07PM -0500, Rui Wang wrote:
> Add logConfigIfChanged() helper function to log DPF configuration
> updates when they occur. This provides visibility into the active
> DPF parameters including:
>
> - Control mode and denoise enable state
> - Filter sizes (9x9 vs 13x9 for rb)
> - NLL scale mode (linear vs logarithmic)
> - Gain mode
> - Strength values (r, g, b)
> - Spatial filter coefficients (g and rb arrays)
> - Noise level lookup table coefficients
>
> The logging is triggered in prepareEnabledMode() whenever the
> configuration is updated, helping with debugging and tuning.
>
> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> ---
>
> changelog:
>  -Move log information into a seperate patch in series
>
>  src/ipa/rkisp1/algorithms/dpf.cpp | 49 +++++++++++++++++++++++++++++++
>  src/ipa/rkisp1/algorithms/dpf.h   |  1 +
>  2 files changed, 50 insertions(+)
>
> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> index 68a94afe..e2e5c7ca 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> @@ -274,6 +274,54 @@ bool Dpf::loadReductionConfig(int32_t mode)
>  	return true;
>  }
>
> +void Dpf::logConfigIfChanged(const IPAFrameContext &frameContext)

I would just name it "logConfig()

> +{
> +	if (!frameContext.dpf.update) {
> +		return;
> +	}

you know already

> +
> +	std::ostringstream ss;
> +
> +	ss << "DPF config update: ";
> +	ss << " control mode=" << static_cast<int>(runningMode_);
> +	ss << ", denoise=" << (frameContext.dpf.denoise ? "enabled" : "disabled, ");
> +
> +	ss << "rb_fltsize="
> +	   << (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? "13x9" : "9x9");
> +	ss << ", nll_scale="
> +	   << (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? "log" : "linear");
> +	ss << ", gain_mode=" << config_.gain.mode;
> +	ss << ", strength=" << int(strengthConfig_.r) << ',' << int(strengthConfig_.g) << ',' << int(strengthConfig_.b);
> +
> +	ss << ", g=[";
> +	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
> +		if (i) {
> +			ss << ',';
> +		}
> +		ss << int(config_.g_flt.spatial_coeff[i]);
> +	}
> +	ss << "]";
> +
> +	ss << ", rb=[";
> +	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
> +		if (i) {
> +			ss << ',';
> +		}
> +		ss << int(config_.rb_flt.spatial_coeff[i]);
> +	}
> +	ss << "]";
> +
> +	ss << ", nll=[";
> +	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS; ++i) {
> +		if (i) {
> +			ss << ',';
> +		}
> +		ss << int(config_.nll.coeff[i]);
> +	}
> +	ss << "]";
> +	LOG(RkISP1Dpf, Info) << ss.str();

Not sure if this is worth an Info or just Debug ?

Do you have a printout example ?

> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::queueRequest
>   */
> @@ -386,6 +434,7 @@ void Dpf::prepareEnabledMode(IPAContext &context,
>  	strengthConfig.setEnabled(true);
>
>  	*strengthConfig = strengthConfig_;
> +	logConfigIfChanged(frameContext);
>  }
>
>  REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> index 99cdbdd3..6586256d 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.h
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -43,6 +43,7 @@ private:
>  			       rkisp1_cif_isp_dpf_strength_config &strengthConfig);
>
>  	bool loadReductionConfig(int32_t mode);
> +	void logConfigIfChanged(const IPAFrameContext &frameContext);
>
>  	void prepareDisabledMode(IPAContext &context,
>  				 const uint32_t frame,
> --
> 2.43.0
>
Rui Wang Dec. 12, 2025, 11:54 p.m. UTC | #2 
Jacopo Mondi wrote:
> Hi Rui
> 
> On Sun, Dec 07, 2025 at 07:48:07PM -0500, Rui Wang wrote:
> > Add logConfigIfChanged() helper function to log DPF configuration
> > updates when they occur. This provides visibility into the active
> > DPF parameters including:
> >
> > - Control mode and denoise enable state
> > - Filter sizes (9x9 vs 13x9 for rb)
> > - NLL scale mode (linear vs logarithmic)
> > - Gain mode
> > - Strength values (r, g, b)
> > - Spatial filter coefficients (g and rb arrays)
> > - Noise level lookup table coefficients
> >
> > The logging is triggered in prepareEnabledMode() whenever the
> > configuration is updated, helping with debugging and tuning.
> >
> > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> > ---
> >
> > changelog:
> >  -Move log information into a seperate patch in series
> >
> >  src/ipa/rkisp1/algorithms/dpf.cpp | 49 +++++++++++++++++++++++++++++++
> >  src/ipa/rkisp1/algorithms/dpf.h   |  1 +
> >  2 files changed, 50 insertions(+)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> > index 68a94afe..e2e5c7ca 100644
> > --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> > @@ -274,6 +274,54 @@ bool Dpf::loadReductionConfig(int32_t mode)
> >  	return true;
> >  }
> >
> > +void Dpf::logConfigIfChanged(const IPAFrameContext &frameContext)
> 
> I would just name it "logConfig()
yes if that  it will  call like :
if(updated)
	logConfig(**);
> 
> > +{
> > +	if (!frameContext.dpf.update) {
> > +		return;
> > +	}
> 
> you know already
> 
> > +
> > +	std::ostringstream ss;
> > +
> > +	ss << "DPF config update: ";
> > +	ss << " control mode=" << static_cast<int>(runningMode_);
> > +	ss << ", denoise=" << (frameContext.dpf.denoise ? "enabled" : "disabled, ");
> > +
> > +	ss << "rb_fltsize="
> > +	   << (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? "13x9" : "9x9");
> > +	ss << ", nll_scale="
> > +	   << (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? "log" : "linear");
> > +	ss << ", gain_mode=" << config_.gain.mode;
> > +	ss << ", strength=" << int(strengthConfig_.r) << ',' << int(strengthConfig_.g) << ',' << int(strengthConfig_.b);
> > +
> > +	ss << ", g=[";
> > +	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
> > +		if (i) {
> > +			ss << ',';
> > +		}
> > +		ss << int(config_.g_flt.spatial_coeff[i]);
> > +	}
> > +	ss << "]";
> > +
> > +	ss << ", rb=[";
> > +	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
> > +		if (i) {
> > +			ss << ',';
> > +		}
> > +		ss << int(config_.rb_flt.spatial_coeff[i]);
> > +	}
> > +	ss << "]";
> > +
> > +	ss << ", nll=[";
> > +	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS; ++i) {
> > +		if (i) {
> > +			ss << ',';
> > +		}
> > +		ss << int(config_.nll.coeff[i]);
> > +	}
> > +	ss << "]";
> > +	LOG(RkISP1Dpf, Info) << ss.str();
> 
> Not sure if this is worth an Info or just Debug ?
>
Those log message should belong to debug level as expected.
just I found lots debug log print out each frame.
> Do you have a printout example ?
>
[0:06:34.799242625] [393]  INFO RkISP1Dpf dpf.cpp:322 DPF config update:  control mode=2, denoise=enabledrb_fltsize=13x9, nll_scale=linear, gain_mode=5, strength=130,130,130, g=[22,18,13,8,5,2], rb=[20,18,16,11,7,3], nll=[0,26,52,78,106,138,172,208,248,292,340,392,448,508,572,640,712]

> > +}
> > +
> >  /**
> >   * \copydoc libcamera::ipa::Algorithm::queueRequest
> >   */
> > @@ -386,6 +434,7 @@ void Dpf::prepareEnabledMode(IPAContext &context,
> >  	strengthConfig.setEnabled(true);
> >
> >  	*strengthConfig = strengthConfig_;
> > +	logConfigIfChanged(frameContext);
> >  }
> >
> >  REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
> > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> > index 99cdbdd3..6586256d 100644
> > --- a/src/ipa/rkisp1/algorithms/dpf.h
> > +++ b/src/ipa/rkisp1/algorithms/dpf.h
> > @@ -43,6 +43,7 @@ private:
> >  			       rkisp1_cif_isp_dpf_strength_config &strengthConfig);
> >
> >  	bool loadReductionConfig(int32_t mode);
> > +	void logConfigIfChanged(const IPAFrameContext &frameContext);
> >
> >  	void prepareDisabledMode(IPAContext &context,
> >  				 const uint32_t frame,
> > --
> > 2.43.0
> >

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
index 68a94afe..e2e5c7ca 100644
--- a/src/ipa/rkisp1/algorithms/dpf.cpp
+++ b/src/ipa/rkisp1/algorithms/dpf.cpp
@@ -274,6 +274,54 @@  bool Dpf::loadReductionConfig(int32_t mode)
 	return true;
 }
 
+void Dpf::logConfigIfChanged(const IPAFrameContext &frameContext)
+{
+	if (!frameContext.dpf.update) {
+		return;
+	}
+
+	std::ostringstream ss;
+
+	ss << "DPF config update: ";
+	ss << " control mode=" << static_cast<int>(runningMode_);
+	ss << ", denoise=" << (frameContext.dpf.denoise ? "enabled" : "disabled, ");
+
+	ss << "rb_fltsize="
+	   << (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? "13x9" : "9x9");
+	ss << ", nll_scale="
+	   << (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? "log" : "linear");
+	ss << ", gain_mode=" << config_.gain.mode;
+	ss << ", strength=" << int(strengthConfig_.r) << ',' << int(strengthConfig_.g) << ',' << int(strengthConfig_.b);
+
+	ss << ", g=[";
+	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
+		if (i) {
+			ss << ',';
+		}
+		ss << int(config_.g_flt.spatial_coeff[i]);
+	}
+	ss << "]";
+
+	ss << ", rb=[";
+	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
+		if (i) {
+			ss << ',';
+		}
+		ss << int(config_.rb_flt.spatial_coeff[i]);
+	}
+	ss << "]";
+
+	ss << ", nll=[";
+	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS; ++i) {
+		if (i) {
+			ss << ',';
+		}
+		ss << int(config_.nll.coeff[i]);
+	}
+	ss << "]";
+	LOG(RkISP1Dpf, Info) << ss.str();
+}
+
 /**
  * \copydoc libcamera::ipa::Algorithm::queueRequest
  */
@@ -386,6 +434,7 @@  void Dpf::prepareEnabledMode(IPAContext &context,
 	strengthConfig.setEnabled(true);
 
 	*strengthConfig = strengthConfig_;
+	logConfigIfChanged(frameContext);
 }
 
 REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
index 99cdbdd3..6586256d 100644
--- a/src/ipa/rkisp1/algorithms/dpf.h
+++ b/src/ipa/rkisp1/algorithms/dpf.h
@@ -43,6 +43,7 @@  private:
 			       rkisp1_cif_isp_dpf_strength_config &strengthConfig);
 
 	bool loadReductionConfig(int32_t mode);
+	void logConfigIfChanged(const IPAFrameContext &frameContext);
 
 	void prepareDisabledMode(IPAContext &context,
 				 const uint32_t frame,