[v1,02/11] ipa: rkisp1: algorithms: dpf: init Dpf tuning config

25169 diff mbox series

Load tuning flags and gain levels on top of the single-config helper.
The parseConfig() function wraps parseSingleConfig() to load the base
configuration, then parses optional enable/devmode flags and any
exposure index -banded overrides.

Replace the inline parsing in init() with a call to parseConfig(). This
simplifies init() and allows the parsing logic to be reused. The refactor
also adds logging of the parsed base tuning and preserves the base config
for manual mode restoration.

Rework domain filter, NLL, gain, and strength parsing into a
parseSingleConfig() helper. This extracts the parsing logic so it can be
reused for both the base tuning and per-exposure-gain-level configuration blocks.

The init method now:
- Calls parseConfig() to load tuning data
- Logs the parsed base configuration
- Caches base config for manual mode restore
- exposure levels are sorted by exposure index threshold for efficient band selection
at runtime.

Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/denoise.h |  11 +-
 src/ipa/rkisp1/algorithms/dpf.cpp   | 186 ++++++++++++++++++++++++----
 src/ipa/rkisp1/algorithms/dpf.h     |  24 +++-
 3 files changed, 195 insertions(+), 26 deletions(-)

Hi Rui

On Mon, Nov 24, 2025 at 07:08:39PM -0500, Rui Wang wrote:
> Load tuning flags and gain levels on top of the single-config helper.

what's "the single-config helper" ?

> The parseConfig() function wraps parseSingleConfig() to load the base
> configuration, then parses optional enable/devmode flags and any
> exposure index -banded overrides.

"index-banded" maybe ?

>
> Replace the inline parsing in init() with a call to parseConfig(). This
> simplifies init() and allows the parsing logic to be reused. The refactor
> also adds logging of the parsed base tuning and preserves the base config
> for manual mode restoration.
>
> Rework domain filter, NLL, gain, and strength parsing into a
> parseSingleConfig() helper. This extracts the parsing logic so it can be
> reused for both the base tuning and per-exposure-gain-level configuration blocks.

line length for commit message is 75 cols

>
> The init method now:
> - Calls parseConfig() to load tuning data
> - Logs the parsed base configuration
> - Caches base config for manual mode restore
> - exposure levels are sorted by exposure index threshold for efficient band selection
> at runtime.

ditto, please reflow

>
> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/denoise.h |  11 +-
>  src/ipa/rkisp1/algorithms/dpf.cpp   | 186 ++++++++++++++++++++++++----
>  src/ipa/rkisp1/algorithms/dpf.h     |  24 +++-
>  3 files changed, 195 insertions(+), 26 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h
> index 8907dc4e..abd08cba 100644
> --- a/src/ipa/rkisp1/algorithms/denoise.h
> +++ b/src/ipa/rkisp1/algorithms/denoise.h
> @@ -20,12 +20,21 @@ class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm
>  protected:
>  	DenoiseBaseAlgorithm() = default;
>  	~DenoiseBaseAlgorithm() = default;
> -

There usually is an empty line between constructors/destructors and
other methods (which might be grouped as well)

> +	virtual void setDevMode(bool dev) { devMode_ = dev; }
> +	virtual bool isDevMode() const { return devMode_; }

getters in libcamera usually have the name of the field

	virtual bool devMode() const { return devMode_; }

>  	virtual uint32_t computeExposureIndex(const IPAContext &context,
>  					      const IPAFrameContext &frameContext) const;
>  	template<typename LevelContainer>
>  	uint32_t selectExposureIndexBand(unsigned exposureIndex,
>  					 const LevelContainer &levels) const;
> +	virtual bool parseConfig([[maybe_unused]] const YamlObject &tuningData)
> +	{
> +		return true;
> +	}
> +
> +private:
> +	/**< Developer mode state for advanced controls */

we don't doxygen in headers, and do not doxygen private fields (I
don't think Doxygen collects documentation for those ?)

A simple comment will do

> +	bool devMode_ = false;
>  };
>
>  inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context,
> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> index 39f3e461..dc0a361c 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> @@ -8,6 +8,7 @@
>  #include "dpf.h"
>
>  #include <algorithm>
> +#include <array>

Where are arrays used in this patch ?

>  #include <string>
>  #include <vector>
>
> @@ -37,7 +38,7 @@ namespace ipa::rkisp1::algorithms {
>  LOG_DEFINE_CATEGORY(RkISP1Dpf)
>
>  Dpf::Dpf()
> -	: config_({}), strengthConfig_({})
> +	: config_({}), strengthConfig_({}), baseConfig_({}), baseStrengthConfig_({})
>  {
>  }
>
> @@ -47,14 +48,130 @@ Dpf::Dpf()
>  int Dpf::init([[maybe_unused]] IPAContext &context,
>  	      const YamlObject &tuningData)
>  {
> -	std::vector<uint8_t> values;
> +	/* Parse tuning block */

Comments should end with '.'

> +	if (!parseConfig(tuningData))
> +		return -EINVAL;
> +
> +	/* Log parsed base tuning (counts are always full-sized for base). */
> +	LOG(RkISP1Dpf, Info)
> +		<< "DPF init: base tuning parsed, G coeffs="
> +		<< RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
> +		<< ", RB fltsize="
> +		<< (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? "13x9" : "9x9")
> +		<< ", NLL coeffs=" << RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS
> +		<< ", NLL scale="
> +		<< (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? "log" : "linear")
> +		<< ", Strength (r,g,b)="
> +		<< (int)strengthConfig_.r << "," << (int)strengthConfig_.g
> +		<< "," << (int)strengthConfig_.b;
> +
> +	/* Preserve base (non-exposure index) YAML configuration for restoration after manual mode. */

when it's trivial to stay in 80 cols, please do so

> +	baseConfig_ = config_;
> +	baseStrengthConfig_ = strengthConfig_;
> +
> +	/* Optional exposure index-banded tuning */
> +	if (useExposureIndexLevels_) {
> +		LOG(RkISP1Dpf, Info)
> +			<< "DPF init: loaded " << exposureIndexLevels_.size()
> +			<< " exposureIndex level(s) from tuning";
> +	}
> +
> +	/* Optional mode tuning */
> +	if (!modes_.empty()) {
> +		LOG(RkISP1Dpf, Info)
> +			<< "DPF init: loaded " << modes_.size()
> +			<< " mode(s) from tuning";
> +	}

no {} for single statements

> +
> +	return 0;
> +}
> +
> +bool Dpf::parseConfig(const YamlObject &tuningData)
> +{
> +	/* Parse base config */
> +	if (!parseSingleConfig(tuningData, config_, strengthConfig_))
> +		return false;
> +
> +	baseConfig_ = config_;
> +	baseStrengthConfig_ = strengthConfig_;
> +
> +	/* Optional developer mode flag (default true). If false, only basic manual controls available. */

What is "developer mode" ?

Do we have anything like this in other algorithms ?
It's true by default, so applications can eventually disable it ?

> +	bool devMode = tuningData["devmode"].get<bool>().value_or(true);
> +	setDevMode(devMode);
> +
> +	/* Parse exposure index levels */
> +	if (tuningData.contains("ExposureIndexLevels")) {
> +		useExposureIndexLevels_ = true;
> +		exposureIndexLevels_.clear();
> +		for (const auto &entry : tuningData["ExposureIndexLevels"].asList()) {
> +			std::optional<uint32_t> maxExposureIndexOpt =
> +				entry["maxExposureIndex"].get<uint32_t>();
> +			if (!maxExposureIndexOpt) {
> +				LOG(RkISP1Dpf, Error) << "ExposureIndexLevels entry missing maxExposureIndex";
> +				continue;

Shouldn't this be mandatory if "ExposureIndexLevels" is specified ?

> +			}
> +			ExposureIndexLevelConfig lvl{};
> +			lvl.maxExposureIndex = *maxExposureIndexOpt;
> +			if (!parseSingleConfig(entry, lvl.dpf, lvl.strength))
> +				continue;
> +			exposureIndexLevels_.push_back(lvl);
> +		}

So each "ExposureIndexLevels" might contain the same exact
configuration of the main outer block ?

I've not yet understood how these are used, but do you think all
paramters should be repeated in each ExposureIndexLevel ?

> +		std::sort(exposureIndexLevels_.begin(), exposureIndexLevels_.end(),
> +			  [](const ExposureIndexLevelConfig &a, const ExposureIndexLevelConfig &b) {
> +				  return a.maxExposureIndex < b.maxExposureIndex;
> +			  });
> +	}
> +
> +	/* Parse modes */
> +	if (tuningData.contains("NoiseReductionMode")) {
> +		modes_.clear();
> +		for (const auto &entry : tuningData["NoiseReductionMode"].asList()) {
> +			std::optional<std::string> typeOpt =
> +				entry["type"].get<std::string>();
> +			if (!typeOpt) {
> +				LOG(RkISP1Dpf, Error) << "Modes entry missing type";
> +				continue;

I think this should be an error as well

> +			}
> +
> +			int32_t modeValue;
> +			if (*typeOpt == "minimal") {
> +				modeValue = controls::draft::NoiseReductionModeMinimal;
> +			} else if (*typeOpt == "highquality") {
> +				modeValue = controls::draft::NoiseReductionModeHighQuality;
> +			} else if (*typeOpt == "fast") {
> +				modeValue = controls::draft::NoiseReductionModeFast;
> +			} else if (*typeOpt == "zsl") {
> +				modeValue = controls::draft::NoiseReductionModeZSL;
> +			} else {
> +				LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt;
> +				continue;

Same here probably

> +			}
>
> +			ModeConfig mode{};
> +			mode.modeValue = modeValue;

Can't you assign to mode.modeValue directly ?

> +			if (!parseSingleConfig(entry, mode.dpf, mode.strength))
> +				continue;

Same question, for each mode should all the paramters be repeated ?
Does each mode require both the denoise pre-filter and the filter to
be fully configured or only some paramters will change per-mode ?

> +			modes_.push_back(mode);
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +bool Dpf::parseSingleConfig(const YamlObject &tuningData,
> +			    rkisp1_cif_isp_dpf_config &config,
> +			    rkisp1_cif_isp_dpf_strength_config &strengthConfig)
> +{
> +	std::vector<uint8_t> values;

Either an empty line here, or you can simply move the declaration
below when it is used

>  	/*
>  	 * The domain kernel is configured with a 9x9 kernel for the green
>  	 * pixels, and a 13x9 or 9x9 kernel for red and blue pixels.
>  	 */
> +	if (!tuningData.contains("DomainFilter")) {
> +		LOG(RkISP1Dpf, Error) << "DomainFilter section missing";
> +		return false;
> +	}
>  	const YamlObject &dFObject = tuningData["DomainFilter"];

I think you can avoid a double lookup by simply

	const YamlObject &dFObject = tuningData["DomainFilter"];
	if (!dFObject) {
		LOG(RkISP1Dpf, Error) << "DomainFilter section missing";
		return false;
	}

here and in all other places where this pattern is repeated in this
patch series

> -

You really don't like empty lines :)

>  	/*
>  	 * For the green component, we have the 9x9 kernel specified
>  	 * as 6 coefficients:
> @@ -78,14 +195,14 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>  			<< "Invalid 'DomainFilter:g': expected "
>  			<< RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
>  			<< " elements, got " << values.size();
> -		return -EINVAL;
> +		return false;
>  	}
>
>  	std::copy_n(values.begin(), values.size(),
> -		    std::begin(config_.g_flt.spatial_coeff));
> +		    std::begin(config.g_flt.spatial_coeff));
>
> -	config_.g_flt.gr_enable = true;
> -	config_.g_flt.gb_enable = true;
> +	config.g_flt.gr_enable = true;
> +	config.g_flt.gb_enable = true;
>
>  	/*
>  	 * For the red and blue components, we have the 13x9 kernel specified
> @@ -116,24 +233,28 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>  			<< RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1
>  			<< " or " << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
>  			<< " elements, got " << values.size();
> -		return -EINVAL;
> +		return false;
>  	}
>
> -	config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
> -			       ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
> -			       : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
> +	config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
> +					? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
> +					: RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
>
>  	std::copy_n(values.begin(), values.size(),
> -		    std::begin(config_.rb_flt.spatial_coeff));
> +		    std::begin(config.rb_flt.spatial_coeff));
>
> -	config_.rb_flt.r_enable = true;
> -	config_.rb_flt.b_enable = true;
> +	config.rb_flt.r_enable = true;
> +	config.rb_flt.b_enable = true;
>
>  	/*
>  	 * The range kernel is configured with a noise level lookup table (NLL)
>  	 * which stores a piecewise linear function that characterizes the
>  	 * sensor noise profile as a noise level function curve (NLF).
>  	 */
> +	if (!tuningData.contains("NoiseLevelFunction")) {
> +		LOG(RkISP1Dpf, Error) << "NoiseLevelFunction section missing";
> +		return false;
> +	}
>  	const YamlObject &rFObject = tuningData["NoiseLevelFunction"];

Or
        if (!rFObject)

>
>  	std::vector<uint16_t> nllValues;
> @@ -143,32 +264,49 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>  			<< "Invalid 'RangeFilter:coeff': expected "
>  			<< RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS
>  			<< " elements, got " << nllValues.size();
> -		return -EINVAL;
> +		return false;
>  	}
>
>  	std::copy_n(nllValues.begin(), nllValues.size(),
> -		    std::begin(config_.nll.coeff));
> +		    std::begin(config.nll.coeff));
>
>  	std::string scaleMode = rFObject["scale-mode"].get<std::string>("");
>  	if (scaleMode == "linear") {
> -		config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;
> +		config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;
>  	} else if (scaleMode == "logarithmic") {
> -		config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
> +		config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
>  	} else {
>  		LOG(RkISP1Dpf, Error)
>  			<< "Invalid 'RangeFilter:scale-mode': expected "
>  			<< "'linear' or 'logarithmic' value, got "
>  			<< scaleMode;
> -		return -EINVAL;
> +		return false;
>  	}
>
> +	if (!tuningData.contains("Gain")) {
> +		LOG(RkISP1Dpf, Error) << "Gain section missing";
> +		return false;
> +	}
> +	const YamlObject &gObject = tuningData["Gain"];
> +
> +	config.gain.mode =
> +		gObject["gain_mode"].get<uint32_t>().value_or(
> +			RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS);

Should we validate that gain_mode is a valid value ? Do we want to
have an integer or should we rather have strings like:
"awb", "lsc", "awblsc", "nf", "nflsc" ?

I'm looking at rkisp1_dpf_config in
drivers/media/platform/rockchip/rkisp1/rkisp1-params.c

Do you know why that function unconditionally enable
RKISP1_CIF_ISP_DPF_MODE_AWB_GAIN_COMP
whenever using NF gains ?

> +	config.gain.nf_r_gain = gObject["nf_r_gain"].get<uint16_t>().value_or(256);
> +	config.gain.nf_b_gain = gObject["nf_b_gain"].get<uint16_t>().value_or(256);
> +	config.gain.nf_gr_gain = gObject["nf_gr_gain"].get<uint16_t>().value_or(256);
> +	config.gain.nf_gb_gain = gObject["nf_gb_gain"].get<uint16_t>().value_or(256);

For my education, how do the gains work ?

We have a single value for R, Gb, Gr, B. Are these gains applied to
all pixels throughout the image ? It is not clear to me if they should
"replace" the AWB gains or add to it (as the driver as noted above
always enable RKISP1_CIF_ISP_DPF_MODE_AWB_GAIN_COMP when using NF
gains).

> +
> +	if (!tuningData.contains("FilterStrength")) {
> +		LOG(RkISP1Dpf, Error) << "FilterStrength section missing";
> +		return false;
> +	}
>  	const YamlObject &fSObject = tuningData["FilterStrength"];
>
> -	strengthConfig_.r = fSObject["r"].get<uint16_t>(64);
> -	strengthConfig_.g = fSObject["g"].get<uint16_t>(64);
> -	strengthConfig_.b = fSObject["b"].get<uint16_t>(64);
> -
> -	return 0;
> +	strengthConfig.r = fSObject["r"].get<uint8_t>().value_or(64);
> +	strengthConfig.g = fSObject["g"].get<uint8_t>().value_or(64);
> +	strengthConfig.b = fSObject["b"].get<uint8_t>().value_or(64);

So these are the filter strengths.

If my understanding is right, we have two main functions to configure:
the denoise pre-filter and the actual filter.

The tuning paramters now have the following structure (if I'm not
mistaken)

      DomainFilter:
        g:
        rb:
      NoiseLevelFunction:
        coeff: []
        scale-mode:
      Gain:
        gain_mode:
        nf_r_gain:
        nf_b_gain:
        nf_gr_gain:
        nf_gb_gain:
      FilterStrength:
        r:
        g:
        b:

Can we re-organize it in logical blocks ?

I don't know if we have a policy of being backward-compatible with
tuning files. I think it's too early to have it, but others might have
different opinions.

In case we can rework the paramters organization, wouldn't something
like:

    Dpf:
      Denoise:
        scale-mode:""
        coeff: []
        gains:
          mode: ""
          r:
          gr:
          gb:
          b:

      Filter:
        weights:
          g: []
          rb: []
        strength:
          r:
          b:
          g:

Be easier to maintain and understand ?

> +	return true;
>  }
>
>  /**
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> index 2dd8cd36..8691932d 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.h
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -10,12 +10,13 @@
>  #include <sys/types.h>
>
>  #include "algorithm.h"
> +#include "denoise.h"
>
>  namespace libcamera {
>
>  namespace ipa::rkisp1::algorithms {
>
> -class Dpf : public Algorithm
> +class Dpf : public DenoiseBaseAlgorithm
>  {
>  public:
>  	Dpf();
> @@ -32,6 +33,27 @@ public:
>  private:
>  	struct rkisp1_cif_isp_dpf_config config_;
>  	struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
> +	struct rkisp1_cif_isp_dpf_config baseConfig_;
> +	struct rkisp1_cif_isp_dpf_strength_config baseStrengthConfig_;
> +	struct ExposureIndexLevelConfig {
> +		uint32_t maxExposureIndex; /* inclusive upper bound */
> +		struct rkisp1_cif_isp_dpf_config dpf;
> +		struct rkisp1_cif_isp_dpf_strength_config strength;
> +	};
> +	struct ModeConfig {
> +		int32_t modeValue;
> +		struct rkisp1_cif_isp_dpf_config dpf;
> +		struct rkisp1_cif_isp_dpf_strength_config strength;
> +	};
> +
> +	std::vector<ExposureIndexLevelConfig> exposureIndexLevels_;
> +	std::vector<ModeConfig> modes_;
> +	bool useExposureIndexLevels_ = false;
> +
> +	bool parseConfig(const YamlObject &tuningData) override;
> +	bool parseSingleConfig(const YamlObject &tuningData,
> +			       rkisp1_cif_isp_dpf_config &config,
> +			       rkisp1_cif_isp_dpf_strength_config &strengthConfig);

Please follow:
https://google.github.io/styleguide/cppguide.html#Declaration_Order

In this case: types definition first, then functions, then data members

>  };
>
>  } /* namespace ipa::rkisp1::algorithms */
> --
> 2.43.0
>