[v8,2/7] ipa: rkisp1: algorithms: dpf: Implement mode switching
diff mbox series

Message ID 20260115163318.1339354-3-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 
Add support for switching between different noise reduction modes.

Introduce `noiseReductionModes_` to store mode-specific configs.

LoadReductionConfig() select specific config from configs.

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

---
changelog since v5:
 - Update log verbos from Info to Debug in loadReductionConfig
 - Update log mode value to string to in loadReductionConfig
 - improving the return value changed like :
       if (ret != 0)  -> if (ret)

 Reviewed-by tags from v5 are carried over (no function changes).
changelog since v6:
 - add { controls::draft::NoiseReductionModeOff, "off" }, to fix
   out_of_range issue

changelog since v7:
 - Delete base config parse from parseConfig
---
 src/ipa/rkisp1/algorithms/dpf.cpp | 90 +++++++++++++++++++++++++++----
 src/ipa/rkisp1/algorithms/dpf.h   | 11 ++++
 2 files changed, 92 insertions(+), 9 deletions(-)

Comments

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

On Thu, Jan 15, 2026 at 11:33:13AM -0500, Rui Wang wrote:
> Add support for switching between different noise reduction modes.
>
> Introduce `noiseReductionModes_` to store mode-specific configs.
>
> LoadReductionConfig() select specific config from configs.
>
> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> ---
> changelog since v5:
>  - Update log verbos from Info to Debug in loadReductionConfig
>  - Update log mode value to string to in loadReductionConfig
>  - improving the return value changed like :
>        if (ret != 0)  -> if (ret)
>
>  Reviewed-by tags from v5 are carried over (no function changes).
> changelog since v6:
>  - add { controls::draft::NoiseReductionModeOff, "off" }, to fix
>    out_of_range issue
>
> changelog since v7:
>  - Delete base config parse from parseConfig
> ---
>  src/ipa/rkisp1/algorithms/dpf.cpp | 90 +++++++++++++++++++++++++++----
>  src/ipa/rkisp1/algorithms/dpf.h   | 11 ++++
>  2 files changed, 92 insertions(+), 9 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> index dd3effa1..9b663831 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 <map>
>  #include <string>
>  #include <vector>
>
> @@ -36,8 +37,21 @@ namespace ipa::rkisp1::algorithms {
>
>  LOG_DEFINE_CATEGORY(RkISP1Dpf)
>
> +namespace {
> +
> +const std::map<int32_t, std::string> kModesMap = {
> +	{ controls::draft::NoiseReductionModeMinimal, "minimal" },
> +	{ controls::draft::NoiseReductionModeFast, "fast" },
> +	{ controls::draft::NoiseReductionModeHighQuality, "highquality" },
> +	{ controls::draft::NoiseReductionModeZSL, "zsl" },
> +	{ controls::draft::NoiseReductionModeOff, "off" },
> +};
> +
> +} /* namespace */
> +
>  Dpf::Dpf()
> -	: config_({}), strengthConfig_({})
> +	: config_({}), strengthConfig_({}), noiseReductionModes_({}),
> +	  runningMode_(controls::draft::NoiseReductionModeOff)
>  {
>  }
>
> @@ -57,10 +71,43 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>
>  int Dpf::parseConfig(const YamlObject &tuningData)
>  {
> -	/* Parse base config. */
> -	int ret = parseSingleConfig(tuningData, config_, strengthConfig_);
> -	if (ret)
> -		return ret;
> +	/* Parse modes. */
> +	return parseModes(tuningData);

This should be called by init() removing the need for parseConfig() as
suggested in the review of the previous patch

> +}
> +
> +int Dpf::parseModes(const YamlObject &tuningData)
> +{
> +	/* Parse noise reduction modes. */
> +	if (!tuningData.contains("modes"))

Should we LOG(Error) now that modes are mandatory ?

> +		return -EINVAL;
> +
> +	noiseReductionModes_.clear();
> +	for (const auto &entry : tuningData["modes"].asList()) {
> +		std::optional<std::string> typeOpt =
> +			entry["type"].get<std::string>();
> +		if (!typeOpt) {
> +			LOG(RkISP1Dpf, Error) << "Modes entry missing type";
> +			return -EINVAL;
> +		}
> +
> +		ModeConfig mode;
> +		auto it = std::find_if(kModesMap.begin(), kModesMap.end(),
> +				       [&typeOpt](const auto &pair) {
> +					       return pair.second == *typeOpt;
> +				       });
> +
> +		if (it == kModesMap.end()) {
> +			LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt;
> +			return -EINVAL;
> +		}
> +
> +		mode.modeValue = it->first;
> +		auto ret = parseSingleConfig(entry, mode.dpf, mode.strength);

                int ret;

auto gives you nothing here

> +		if (ret)
> +			return ret;
> +
> +		noiseReductionModes_.push_back(mode);

I think it was pointed out already in the previous versions: you
should populate context.ctrlMap[controls::draft::NoiseReductionMode]
with one entry for each supported mode.

See https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rkisp1/algorithms/agc.cpp#n76

Will it happen later on in the series and I've missed it ?


> +	}
>
>  	return 0;
>  }
> @@ -193,6 +240,28 @@ int Dpf::parseSingleConfig(const YamlObject &tuningData,
>  	return 0;
>  }
>
> +bool Dpf::loadReductionConfig(int32_t mode)

             loadModeConfig(int32_t mode)

> +{
> +	auto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(),
> +			       [mode](const ModeConfig &m) {
> +				       return m.modeValue == mode;
> +			       });
> +	if (it == noiseReductionModes_.end()) {
> +		LOG(RkISP1Dpf, Warning)
> +			<< "No DPF config for reduction mode: " << kModesMap.at(mode);
			<< "No DPF config for mode: " << kModesMap.at(mode);


> +		return false;
> +	}
> +
> +	config_ = it->dpf;
> +	strengthConfig_ = it->strength;

So now the config_ and strengthConfig_ member variables point to the
'active' mode configuration. Why are you copying them instead of just
keeping a pointer to the active mode in noiseReductionModes_ ?

> +
> +	LOG(RkISP1Dpf, Debug)
> +		<< "DPF mode=Reduction (config loaded)"
> +		<< " mode=" << kModesMap.at(mode);

                   " mode = "
> +
> +	return true;
> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::queueRequest
>   */
> @@ -206,8 +275,6 @@ void Dpf::queueRequest(IPAContext &context,
>
>  	const auto &denoise = controls.get(controls::draft::NoiseReductionMode);
>  	if (denoise) {
> -		LOG(RkISP1Dpf, Debug) << "Set denoise to " << *denoise;
> -
>  		switch (*denoise) {
>  		case controls::draft::NoiseReductionModeOff:
>  			if (dpf.denoise) {
> @@ -218,9 +285,10 @@ void Dpf::queueRequest(IPAContext &context,
>  		case controls::draft::NoiseReductionModeMinimal:
>  		case controls::draft::NoiseReductionModeHighQuality:
>  		case controls::draft::NoiseReductionModeFast:
> -			if (!dpf.denoise) {
> -				dpf.denoise = true;
> +		case controls::draft::NoiseReductionModeZSL:
> +			if (loadReductionConfig(*denoise)) {
>  				update = true;
> +				dpf.denoise = true;
>  			}
>  			break;
>  		default:
> @@ -229,6 +297,10 @@ void Dpf::queueRequest(IPAContext &context,
>  				<< *denoise;
>  			break;
>  		}
> +		if (update) {
> +			runningMode_ = *denoise;

So the member variable runningMode_ simply keeps track of the index of
the enabled mode, right ? And I see it only used in logConfig()
introduced in the next patch.

If you accept the above suggestion about keeping a
pointer/refernce/iterator to the active mode on the
noiseReductionModes_ vector instead of copying its content to config_
and strengthConfig_ you can use the 'modeValue' index instead of
duplicating the information in runnigMode_

> +			LOG(RkISP1Dpf, Debug) << "Set denoise to " << kModesMap.at(runningMode_);
> +		}
>  	}
>
>  	frameContext.dpf.denoise = dpf.denoise;
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> index 39186c55..2a2c7052 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.h
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -30,13 +30,24 @@ public:
>  		     RkISP1Params *params) override;
>
>  private:
> +	struct ModeConfig {
> +		int32_t modeValue;
> +		rkisp1_cif_isp_dpf_config dpf;
> +		rkisp1_cif_isp_dpf_strength_config strength;
> +	};
> +
>  	int parseConfig(const YamlObject &tuningData);
> +	int parseModes(const YamlObject &tuningData);
>  	int parseSingleConfig(const YamlObject &tuningData,
>  			      rkisp1_cif_isp_dpf_config &config,
>  			      rkisp1_cif_isp_dpf_strength_config &strengthConfig);
>
> +	bool loadReductionConfig(int32_t mode);
> +
>  	struct rkisp1_cif_isp_dpf_config config_;
>  	struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
> +	std::vector<ModeConfig> noiseReductionModes_;
> +	int32_t runningMode_;
>  };
>
>  } /* namespace ipa::rkisp1::algorithms */
> --
> 2.43.0
>
Rui Wang Jan. 17, 2026, 3:13 a.m. UTC | #2 
On 2026-01-16 04:21, Jacopo Mondi wrote:
> Hi Rui
>
> On Thu, Jan 15, 2026 at 11:33:13AM -0500, Rui Wang wrote:
>> Add support for switching between different noise reduction modes.
>>
>> Introduce `noiseReductionModes_` to store mode-specific configs.
>>
>> LoadReductionConfig() select specific config from configs.
>>
>> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>
>> ---
>> changelog since v5:
>>   - Update log verbos from Info to Debug in loadReductionConfig
>>   - Update log mode value to string to in loadReductionConfig
>>   - improving the return value changed like :
>>         if (ret != 0)  -> if (ret)
>>
>>   Reviewed-by tags from v5 are carried over (no function changes).
>> changelog since v6:
>>   - add { controls::draft::NoiseReductionModeOff, "off" }, to fix
>>     out_of_range issue
>>
>> changelog since v7:
>>   - Delete base config parse from parseConfig
>> ---
>>   src/ipa/rkisp1/algorithms/dpf.cpp | 90 +++++++++++++++++++++++++++----
>>   src/ipa/rkisp1/algorithms/dpf.h   | 11 ++++
>>   2 files changed, 92 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
>> index dd3effa1..9b663831 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 <map>
>>   #include <string>
>>   #include <vector>
>>
>> @@ -36,8 +37,21 @@ namespace ipa::rkisp1::algorithms {
>>
>>   LOG_DEFINE_CATEGORY(RkISP1Dpf)
>>
>> +namespace {
>> +
>> +const std::map<int32_t, std::string> kModesMap = {
>> +	{ controls::draft::NoiseReductionModeMinimal, "minimal" },
>> +	{ controls::draft::NoiseReductionModeFast, "fast" },
>> +	{ controls::draft::NoiseReductionModeHighQuality, "highquality" },
>> +	{ controls::draft::NoiseReductionModeZSL, "zsl" },
>> +	{ controls::draft::NoiseReductionModeOff, "off" },
>> +};
>> +
>> +} /* namespace */
>> +
>>   Dpf::Dpf()
>> -	: config_({}), strengthConfig_({})
>> +	: config_({}), strengthConfig_({}), noiseReductionModes_({}),
>> +	  runningMode_(controls::draft::NoiseReductionModeOff)
>>   {
>>   }
>>
>> @@ -57,10 +71,43 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>>
>>   int Dpf::parseConfig(const YamlObject &tuningData)
>>   {
>> -	/* Parse base config. */
>> -	int ret = parseSingleConfig(tuningData, config_, strengthConfig_);
>> -	if (ret)
>> -		return ret;
>> +	/* Parse modes. */
>> +	return parseModes(tuningData);
> This should be called by init() removing the need for parseConfig() as
> suggested in the review of the previous patch

I am thinking about  add parseActiveMode into parseConfig , and all YAML 
parsing into this function in future.

currently it has only parseModes inside parseConfig.

    - enableMode: ''

>
>> +}
>> +
>> +int Dpf::parseModes(const YamlObject &tuningData)
>> +{
>> +	/* Parse noise reduction modes. */
>> +	if (!tuningData.contains("modes"))
> Should we LOG(Error) now that modes are mandatory ?
>
>> +		return -EINVAL;
>> +
>> +	noiseReductionModes_.clear();
>> +	for (const auto &entry : tuningData["modes"].asList()) {
>> +		std::optional<std::string> typeOpt =
>> +			entry["type"].get<std::string>();
>> +		if (!typeOpt) {
>> +			LOG(RkISP1Dpf, Error) << "Modes entry missing type";
>> +			return -EINVAL;
>> +		}
>> +
>> +		ModeConfig mode;
>> +		auto it = std::find_if(kModesMap.begin(), kModesMap.end(),
>> +				       [&typeOpt](const auto &pair) {
>> +					       return pair.second == *typeOpt;
>> +				       });
>> +
>> +		if (it == kModesMap.end()) {
>> +			LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt;
>> +			return -EINVAL;
>> +		}
>> +
>> +		mode.modeValue = it->first;
>> +		auto ret = parseSingleConfig(entry, mode.dpf, mode.strength);
>                  int ret;
>
> auto gives you nothing here
>
>> +		if (ret)
>> +			return ret;
>> +
>> +		noiseReductionModes_.push_back(mode);
> I think it was pointed out already in the previous versions: you
> should populate context.ctrlMap[controls::draft::NoiseReductionMode]
> with one entry for each supported mode.
>
> See https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rkisp1/algorithms/agc.cpp#n76
>
> Will it happen later on in the series and I've missed it ?
>
auto it = std::find_if(kModesMap.begin(), kModesMap.end(),

[&typeOpt](const auto &pair) {

return pair.second == *typeOpt;

});


kModesMap contains all NoiseReductionMode value . so I can traverse each mode in YAML
and then add into  noiseReductionModes_.


>> +	}
>>
>>   	return 0;
>>   }
>> @@ -193,6 +240,28 @@ int Dpf::parseSingleConfig(const YamlObject &tuningData,
>>   	return 0;
>>   }
>>
>> +bool Dpf::loadReductionConfig(int32_t mode)
>               loadModeConfig(int32_t mode)
>
>> +{
>> +	auto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(),
>> +			       [mode](const ModeConfig &m) {
>> +				       return m.modeValue == mode;
>> +			       });
>> +	if (it == noiseReductionModes_.end()) {
>> +		LOG(RkISP1Dpf, Warning)
>> +			<< "No DPF config for reduction mode: " << kModesMap.at(mode);
> 			<< "No DPF config for mode: " << kModesMap.at(mode);
>
> will remove this 'reduction' in v9.
>> +		return false;
>> +	}
>> +
>> +	config_ = it->dpf;
>> +	strengthConfig_ = it->strength;
> So now the config_ and strengthConfig_ member variables point to the
> 'active' mode configuration. Why are you copying them instead of just
> keeping a pointer to the active mode in noiseReductionModes_ ?
good idea, Introduce a const interator to point active mode in

noiseReductionModes_ it can save strengthConfig_,config_, runningMode.
will implement in v9.

>
>> +
>> +	LOG(RkISP1Dpf, Debug)
>> +		<< "DPF mode=Reduction (config loaded)"
>> +		<< " mode=" << kModesMap.at(mode);
>                     " mode = "
will update in v9.
>> +
>> +	return true;
>> +}
>> +
>>   /**
>>    * \copydoc libcamera::ipa::Algorithm::queueRequest
>>    */
>> @@ -206,8 +275,6 @@ void Dpf::queueRequest(IPAContext &context,
>>
>>   	const auto &denoise = controls.get(controls::draft::NoiseReductionMode);
>>   	if (denoise) {
>> -		LOG(RkISP1Dpf, Debug) << "Set denoise to " << *denoise;
>> -
>>   		switch (*denoise) {
>>   		case controls::draft::NoiseReductionModeOff:
>>   			if (dpf.denoise) {
>> @@ -218,9 +285,10 @@ void Dpf::queueRequest(IPAContext &context,
>>   		case controls::draft::NoiseReductionModeMinimal:
>>   		case controls::draft::NoiseReductionModeHighQuality:
>>   		case controls::draft::NoiseReductionModeFast:
>> -			if (!dpf.denoise) {
>> -				dpf.denoise = true;
>> +		case controls::draft::NoiseReductionModeZSL:
>> +			if (loadReductionConfig(*denoise)) {
>>   				update = true;
>> +				dpf.denoise = true;
>>   			}
>>   			break;
>>   		default:
>> @@ -229,6 +297,10 @@ void Dpf::queueRequest(IPAContext &context,
>>   				<< *denoise;
>>   			break;
>>   		}
>> +		if (update) {
>> +			runningMode_ = *denoise;
> So the member variable runningMode_ simply keeps track of the index of
> the enabled mode, right ? And I see it only used in logConfig()
> introduced in the next patch.
>
> If you accept the above suggestion about keeping a
> pointer/refernce/iterator to the active mode on the
> noiseReductionModes_ vector instead of copying its content to config_
> and strengthConfig_ you can use the 'modeValue' index instead of
> duplicating the information in runnigMode_

yes will add const_iterator activeMode_ to point noiseReductionModes_


>
>> +			LOG(RkISP1Dpf, Debug) << "Set denoise to " << kModesMap.at(runningMode_);
>> +		}
>>   	}
>>
>>   	frameContext.dpf.denoise = dpf.denoise;
>> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
>> index 39186c55..2a2c7052 100644
>> --- a/src/ipa/rkisp1/algorithms/dpf.h
>> +++ b/src/ipa/rkisp1/algorithms/dpf.h
>> @@ -30,13 +30,24 @@ public:
>>   		     RkISP1Params *params) override;
>>
>>   private:
>> +	struct ModeConfig {
>> +		int32_t modeValue;
>> +		rkisp1_cif_isp_dpf_config dpf;
>> +		rkisp1_cif_isp_dpf_strength_config strength;
>> +	};
>> +
>>   	int parseConfig(const YamlObject &tuningData);
>> +	int parseModes(const YamlObject &tuningData);
>>   	int parseSingleConfig(const YamlObject &tuningData,
>>   			      rkisp1_cif_isp_dpf_config &config,
>>   			      rkisp1_cif_isp_dpf_strength_config &strengthConfig);
>>
>> +	bool loadReductionConfig(int32_t mode);
>> +
>>   	struct rkisp1_cif_isp_dpf_config config_;
>>   	struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
>> +	std::vector<ModeConfig> noiseReductionModes_;
>> +	int32_t runningMode_;
>>   };
>>
>>   } /* namespace ipa::rkisp1::algorithms */
>> --
>> 2.43.0
>>
Jacopo Mondi Jan. 17, 2026, 9:38 a.m. UTC | #3 
Hi Rui

On Fri, Jan 16, 2026 at 10:13:35PM -0500, rui wang wrote:
>
> On 2026-01-16 04:21, Jacopo Mondi wrote:
> > Hi Rui
> >
> > On Thu, Jan 15, 2026 at 11:33:13AM -0500, Rui Wang wrote:
> > > Add support for switching between different noise reduction modes.
> > >
> > > Introduce `noiseReductionModes_` to store mode-specific configs.
> > >
> > > LoadReductionConfig() select specific config from configs.
> > >
> > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >
> > > ---
> > > changelog since v5:
> > >   - Update log verbos from Info to Debug in loadReductionConfig
> > >   - Update log mode value to string to in loadReductionConfig
> > >   - improving the return value changed like :
> > >         if (ret != 0)  -> if (ret)
> > >
> > >   Reviewed-by tags from v5 are carried over (no function changes).
> > > changelog since v6:
> > >   - add { controls::draft::NoiseReductionModeOff, "off" }, to fix
> > >     out_of_range issue
> > >
> > > changelog since v7:
> > >   - Delete base config parse from parseConfig
> > > ---
> > >   src/ipa/rkisp1/algorithms/dpf.cpp | 90 +++++++++++++++++++++++++++----
> > >   src/ipa/rkisp1/algorithms/dpf.h   | 11 ++++
> > >   2 files changed, 92 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> > > index dd3effa1..9b663831 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 <map>
> > >   #include <string>
> > >   #include <vector>
> > >
> > > @@ -36,8 +37,21 @@ namespace ipa::rkisp1::algorithms {
> > >
> > >   LOG_DEFINE_CATEGORY(RkISP1Dpf)
> > >
> > > +namespace {
> > > +
> > > +const std::map<int32_t, std::string> kModesMap = {
> > > +	{ controls::draft::NoiseReductionModeMinimal, "minimal" },
> > > +	{ controls::draft::NoiseReductionModeFast, "fast" },
> > > +	{ controls::draft::NoiseReductionModeHighQuality, "highquality" },
> > > +	{ controls::draft::NoiseReductionModeZSL, "zsl" },
> > > +	{ controls::draft::NoiseReductionModeOff, "off" },
> > > +};
> > > +
> > > +} /* namespace */
> > > +
> > >   Dpf::Dpf()
> > > -	: config_({}), strengthConfig_({})
> > > +	: config_({}), strengthConfig_({}), noiseReductionModes_({}),
> > > +	  runningMode_(controls::draft::NoiseReductionModeOff)
> > >   {
> > >   }
> > >
> > > @@ -57,10 +71,43 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
> > >
> > >   int Dpf::parseConfig(const YamlObject &tuningData)
> > >   {
> > > -	/* Parse base config. */
> > > -	int ret = parseSingleConfig(tuningData, config_, strengthConfig_);
> > > -	if (ret)
> > > -		return ret;
> > > +	/* Parse modes. */
> > > +	return parseModes(tuningData);
> > This should be called by init() removing the need for parseConfig() as
> > suggested in the review of the previous patch
>
> I am thinking about  add parseActiveMode into parseConfig , and all YAML
> parsing into this function in future.
>
> currently it has only parseModes inside parseConfig.
>
>    - enableMode: ''
>
> >
> > > +}
> > > +
> > > +int Dpf::parseModes(const YamlObject &tuningData)
> > > +{
> > > +	/* Parse noise reduction modes. */
> > > +	if (!tuningData.contains("modes"))
> > Should we LOG(Error) now that modes are mandatory ?
> >
> > > +		return -EINVAL;
> > > +
> > > +	noiseReductionModes_.clear();
> > > +	for (const auto &entry : tuningData["modes"].asList()) {
> > > +		std::optional<std::string> typeOpt =
> > > +			entry["type"].get<std::string>();
> > > +		if (!typeOpt) {
> > > +			LOG(RkISP1Dpf, Error) << "Modes entry missing type";
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		ModeConfig mode;
> > > +		auto it = std::find_if(kModesMap.begin(), kModesMap.end(),
> > > +				       [&typeOpt](const auto &pair) {
> > > +					       return pair.second == *typeOpt;
> > > +				       });
> > > +
> > > +		if (it == kModesMap.end()) {
> > > +			LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt;
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		mode.modeValue = it->first;
> > > +		auto ret = parseSingleConfig(entry, mode.dpf, mode.strength);
> >                  int ret;
> >
> > auto gives you nothing here
> >
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		noiseReductionModes_.push_back(mode);
> > I think it was pointed out already in the previous versions: you
> > should populate context.ctrlMap[controls::draft::NoiseReductionMode]
> > with one entry for each supported mode.
> >
> > See https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rkisp1/algorithms/agc.cpp#n76
> >
> > Will it happen later on in the series and I've missed it ?
> >
> auto it = std::find_if(kModesMap.begin(), kModesMap.end(),
>
> [&typeOpt](const auto &pair) {
>
> return pair.second == *typeOpt;
>
> });
>
>
> kModesMap contains all NoiseReductionMode value . so I can traverse each mode in YAML
> and then add into  noiseReductionModes_.

I'm sorry but maybe we're not getting each other.

Could you please read the link I provided above where the AGC
algorithm populates context.ctrlMap[] ?

You should populate that map as well with only the modes enabled in
config file.

Please apply the following patch to your series:

------------------------------------------------------------------------------
--- a/src/ipa/rkisp1/algorithms/dpf.cpp
+++ b/src/ipa/rkisp1/algorithms/dpf.cpp
@@ -58,30 +58,33 @@ Dpf::Dpf()
 /**
  * \copydoc libcamera::ipa::Algorithm::init
  */
-int Dpf::init([[maybe_unused]] IPAContext &context,
-             const YamlObject &tuningData)
+int Dpf::init(IPAContext &context, const YamlObject &tuningData)
 {
        /* Parse tuning block. */
-       int ret = parseConfig(tuningData);
+       int ret = parseConfig(context, tuningData);
        if (ret)
                return ret;

        return 0;
 }

-int Dpf::parseConfig(const YamlObject &tuningData)
+int Dpf::parseConfig(IPAContext &context, const YamlObject &tuningData)
 {
        /* Parse modes. */
-       return parseModes(tuningData);
+       return parseModes(context, tuningData);
 }

-int Dpf::parseModes(const YamlObject &tuningData)
+int Dpf::parseModes(IPAContext &context, const YamlObject &tuningData)
 {
        /* Parse noise reduction modes. */
        if (!tuningData.contains("modes"))
                return -EINVAL;

+       std::vector<ControlValue> enabledModes{
+               ControlValue(controls::draft::NoiseReductionModeOff),
+       };
        noiseReductionModes_.clear();
+
        for (const auto &entry : tuningData["modes"].asList()) {
                std::optional<std::string> typeOpt =
                        entry["type"].get<std::string>();
@@ -107,8 +110,12 @@ int Dpf::parseModes(const YamlObject &tuningData)
                        return ret;

                noiseReductionModes_.push_back(mode);
+               enabledModes.push_back(ControlValue(mode.modeValue));
        }

+       context.ctrlMap[&controls::draft::NoiseReductionMode] =
+                                               ControlInfo(enabledModes);
+
        return 0;
 }

diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
index 1821da396cd7..4deedb838d4d 100644
--- a/src/ipa/rkisp1/algorithms/dpf.h
+++ b/src/ipa/rkisp1/algorithms/dpf.h
@@ -36,8 +36,8 @@ private:
                rkisp1_cif_isp_dpf_strength_config strength;
        };

-       int parseConfig(const YamlObject &tuningData);
-       int parseModes(const YamlObject &tuningData);
+       int parseConfig(IPAContext &context, const YamlObject &tuningData);
+       int parseModes(IPAContext &context, const YamlObject &tuningData);
        int parseSingleConfig(const YamlObject &tuningData,
                              rkisp1_cif_isp_dpf_config &config,
                              rkisp1_cif_isp_dpf_strength_config &strengthConfig);
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index fbcc39103d4b..402ed62ceaa9 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -120,7 +120,6 @@ const IPAHwSettings ipaHwSettingsV12{
 /* List of controls handled by the RkISP1 IPA */
 const ControlInfoMap::Map rkisp1Controls{
        { &controls::DebugMetadataEnable, ControlInfo(false, true, false) },
-       { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
 };

 } /* namespace */
------------------------------------------------------------------------------

So that only modes enabled in the tuning file are reported in
Camera::controls().

Without the patch:

# cam -c1 --list-controls
Control: [inout] draft::NoiseReductionMode:
  - NoiseReductionModeOff (0) [default]
  - NoiseReductionModeFast (1)
  - NoiseReductionModeHighQuality (2)
  - NoiseReductionModeMinimal (3)
  - NoiseReductionModeZSL (4)

With the patch and modes "minimal" and "highquality" in the tuning
file as per this series on imx219

# cam -c1 --list-controls
Control: [inout] draft::NoiseReductionMode:
  - NoiseReductionModeOff (0) [default]
  - NoiseReductionModeMinimal (3)
  - NoiseReductionModeHighQuality (2)

With the patch and mode "highquality" removed from the tuning file

# cam -c1 --list-controls
Control: [inout] draft::NoiseReductionMode:
  - NoiseReductionModeOff (0) [default]
  - NoiseReductionModeMinimal (3)

>
>
> > > +	}
> > >
> > >   	return 0;
> > >   }
> > > @@ -193,6 +240,28 @@ int Dpf::parseSingleConfig(const YamlObject &tuningData,
> > >   	return 0;
> > >   }
> > >
> > > +bool Dpf::loadReductionConfig(int32_t mode)
> >               loadModeConfig(int32_t mode)
> >
> > > +{
> > > +	auto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(),
> > > +			       [mode](const ModeConfig &m) {
> > > +				       return m.modeValue == mode;
> > > +			       });
> > > +	if (it == noiseReductionModes_.end()) {
> > > +		LOG(RkISP1Dpf, Warning)
> > > +			<< "No DPF config for reduction mode: " << kModesMap.at(mode);
> > 			<< "No DPF config for mode: " << kModesMap.at(mode);
> >
> > will remove this 'reduction' in v9.
> > > +		return false;
> > > +	}
> > > +
> > > +	config_ = it->dpf;
> > > +	strengthConfig_ = it->strength;
> > So now the config_ and strengthConfig_ member variables point to the
> > 'active' mode configuration. Why are you copying them instead of just
> > keeping a pointer to the active mode in noiseReductionModes_ ?
> good idea, Introduce a const interator to point active mode in
>
> noiseReductionModes_ it can save strengthConfig_,config_, runningMode.
> will implement in v9.
>
> >
> > > +
> > > +	LOG(RkISP1Dpf, Debug)
> > > +		<< "DPF mode=Reduction (config loaded)"
> > > +		<< " mode=" << kModesMap.at(mode);
> >                     " mode = "
> will update in v9.
> > > +
> > > +	return true;
> > > +}
> > > +
> > >   /**
> > >    * \copydoc libcamera::ipa::Algorithm::queueRequest
> > >    */
> > > @@ -206,8 +275,6 @@ void Dpf::queueRequest(IPAContext &context,
> > >
> > >   	const auto &denoise = controls.get(controls::draft::NoiseReductionMode);
> > >   	if (denoise) {
> > > -		LOG(RkISP1Dpf, Debug) << "Set denoise to " << *denoise;
> > > -
> > >   		switch (*denoise) {
> > >   		case controls::draft::NoiseReductionModeOff:
> > >   			if (dpf.denoise) {
> > > @@ -218,9 +285,10 @@ void Dpf::queueRequest(IPAContext &context,
> > >   		case controls::draft::NoiseReductionModeMinimal:
> > >   		case controls::draft::NoiseReductionModeHighQuality:
> > >   		case controls::draft::NoiseReductionModeFast:
> > > -			if (!dpf.denoise) {
> > > -				dpf.denoise = true;
> > > +		case controls::draft::NoiseReductionModeZSL:
> > > +			if (loadReductionConfig(*denoise)) {
> > >   				update = true;
> > > +				dpf.denoise = true;
> > >   			}
> > >   			break;
> > >   		default:
> > > @@ -229,6 +297,10 @@ void Dpf::queueRequest(IPAContext &context,
> > >   				<< *denoise;
> > >   			break;
> > >   		}
> > > +		if (update) {
> > > +			runningMode_ = *denoise;
> > So the member variable runningMode_ simply keeps track of the index of
> > the enabled mode, right ? And I see it only used in logConfig()
> > introduced in the next patch.
> >
> > If you accept the above suggestion about keeping a
> > pointer/refernce/iterator to the active mode on the
> > noiseReductionModes_ vector instead of copying its content to config_
> > and strengthConfig_ you can use the 'modeValue' index instead of
> > duplicating the information in runnigMode_
>
> yes will add const_iterator activeMode_ to point noiseReductionModes_
>
>
> >
> > > +			LOG(RkISP1Dpf, Debug) << "Set denoise to " << kModesMap.at(runningMode_);
> > > +		}
> > >   	}
> > >
> > >   	frameContext.dpf.denoise = dpf.denoise;
> > > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> > > index 39186c55..2a2c7052 100644
> > > --- a/src/ipa/rkisp1/algorithms/dpf.h
> > > +++ b/src/ipa/rkisp1/algorithms/dpf.h
> > > @@ -30,13 +30,24 @@ public:
> > >   		     RkISP1Params *params) override;
> > >
> > >   private:
> > > +	struct ModeConfig {
> > > +		int32_t modeValue;
> > > +		rkisp1_cif_isp_dpf_config dpf;
> > > +		rkisp1_cif_isp_dpf_strength_config strength;
> > > +	};
> > > +
> > >   	int parseConfig(const YamlObject &tuningData);
> > > +	int parseModes(const YamlObject &tuningData);
> > >   	int parseSingleConfig(const YamlObject &tuningData,
> > >   			      rkisp1_cif_isp_dpf_config &config,
> > >   			      rkisp1_cif_isp_dpf_strength_config &strengthConfig);
> > >
> > > +	bool loadReductionConfig(int32_t mode);
> > > +
> > >   	struct rkisp1_cif_isp_dpf_config config_;
> > >   	struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
> > > +	std::vector<ModeConfig> noiseReductionModes_;
> > > +	int32_t runningMode_;
> > >   };
> > >
> > >   } /* namespace ipa::rkisp1::algorithms */
> > > --
> > > 2.43.0
> > >

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
index dd3effa1..9b663831 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 <map>
 #include <string>
 #include <vector>
 
@@ -36,8 +37,21 @@  namespace ipa::rkisp1::algorithms {
 
 LOG_DEFINE_CATEGORY(RkISP1Dpf)
 
+namespace {
+
+const std::map<int32_t, std::string> kModesMap = {
+	{ controls::draft::NoiseReductionModeMinimal, "minimal" },
+	{ controls::draft::NoiseReductionModeFast, "fast" },
+	{ controls::draft::NoiseReductionModeHighQuality, "highquality" },
+	{ controls::draft::NoiseReductionModeZSL, "zsl" },
+	{ controls::draft::NoiseReductionModeOff, "off" },
+};
+
+} /* namespace */
+
 Dpf::Dpf()
-	: config_({}), strengthConfig_({})
+	: config_({}), strengthConfig_({}), noiseReductionModes_({}),
+	  runningMode_(controls::draft::NoiseReductionModeOff)
 {
 }
 
@@ -57,10 +71,43 @@  int Dpf::init([[maybe_unused]] IPAContext &context,
 
 int Dpf::parseConfig(const YamlObject &tuningData)
 {
-	/* Parse base config. */
-	int ret = parseSingleConfig(tuningData, config_, strengthConfig_);
-	if (ret)
-		return ret;
+	/* Parse modes. */
+	return parseModes(tuningData);
+}
+
+int Dpf::parseModes(const YamlObject &tuningData)
+{
+	/* Parse noise reduction modes. */
+	if (!tuningData.contains("modes"))
+		return -EINVAL;
+
+	noiseReductionModes_.clear();
+	for (const auto &entry : tuningData["modes"].asList()) {
+		std::optional<std::string> typeOpt =
+			entry["type"].get<std::string>();
+		if (!typeOpt) {
+			LOG(RkISP1Dpf, Error) << "Modes entry missing type";
+			return -EINVAL;
+		}
+
+		ModeConfig mode;
+		auto it = std::find_if(kModesMap.begin(), kModesMap.end(),
+				       [&typeOpt](const auto &pair) {
+					       return pair.second == *typeOpt;
+				       });
+
+		if (it == kModesMap.end()) {
+			LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt;
+			return -EINVAL;
+		}
+
+		mode.modeValue = it->first;
+		auto ret = parseSingleConfig(entry, mode.dpf, mode.strength);
+		if (ret)
+			return ret;
+
+		noiseReductionModes_.push_back(mode);
+	}
 
 	return 0;
 }
@@ -193,6 +240,28 @@  int Dpf::parseSingleConfig(const YamlObject &tuningData,
 	return 0;
 }
 
+bool Dpf::loadReductionConfig(int32_t mode)
+{
+	auto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(),
+			       [mode](const ModeConfig &m) {
+				       return m.modeValue == mode;
+			       });
+	if (it == noiseReductionModes_.end()) {
+		LOG(RkISP1Dpf, Warning)
+			<< "No DPF config for reduction mode: " << kModesMap.at(mode);
+		return false;
+	}
+
+	config_ = it->dpf;
+	strengthConfig_ = it->strength;
+
+	LOG(RkISP1Dpf, Debug)
+		<< "DPF mode=Reduction (config loaded)"
+		<< " mode=" << kModesMap.at(mode);
+
+	return true;
+}
+
 /**
  * \copydoc libcamera::ipa::Algorithm::queueRequest
  */
@@ -206,8 +275,6 @@  void Dpf::queueRequest(IPAContext &context,
 
 	const auto &denoise = controls.get(controls::draft::NoiseReductionMode);
 	if (denoise) {
-		LOG(RkISP1Dpf, Debug) << "Set denoise to " << *denoise;
-
 		switch (*denoise) {
 		case controls::draft::NoiseReductionModeOff:
 			if (dpf.denoise) {
@@ -218,9 +285,10 @@  void Dpf::queueRequest(IPAContext &context,
 		case controls::draft::NoiseReductionModeMinimal:
 		case controls::draft::NoiseReductionModeHighQuality:
 		case controls::draft::NoiseReductionModeFast:
-			if (!dpf.denoise) {
-				dpf.denoise = true;
+		case controls::draft::NoiseReductionModeZSL:
+			if (loadReductionConfig(*denoise)) {
 				update = true;
+				dpf.denoise = true;
 			}
 			break;
 		default:
@@ -229,6 +297,10 @@  void Dpf::queueRequest(IPAContext &context,
 				<< *denoise;
 			break;
 		}
+		if (update) {
+			runningMode_ = *denoise;
+			LOG(RkISP1Dpf, Debug) << "Set denoise to " << kModesMap.at(runningMode_);
+		}
 	}
 
 	frameContext.dpf.denoise = dpf.denoise;
diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
index 39186c55..2a2c7052 100644
--- a/src/ipa/rkisp1/algorithms/dpf.h
+++ b/src/ipa/rkisp1/algorithms/dpf.h
@@ -30,13 +30,24 @@  public:
 		     RkISP1Params *params) override;
 
 private:
+	struct ModeConfig {
+		int32_t modeValue;
+		rkisp1_cif_isp_dpf_config dpf;
+		rkisp1_cif_isp_dpf_strength_config strength;
+	};
+
 	int parseConfig(const YamlObject &tuningData);
+	int parseModes(const YamlObject &tuningData);
 	int parseSingleConfig(const YamlObject &tuningData,
 			      rkisp1_cif_isp_dpf_config &config,
 			      rkisp1_cif_isp_dpf_strength_config &strengthConfig);
 
+	bool loadReductionConfig(int32_t mode);
+
 	struct rkisp1_cif_isp_dpf_config config_;
 	struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;
+	std::vector<ModeConfig> noiseReductionModes_;
+	int32_t runningMode_;
 };
 
 } /* namespace ipa::rkisp1::algorithms */