[v11,3/7] ipa: rkisp1: algorithms: register noise reduction controls
diff mbox series

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

Commit Message

Rui Wang Jan. 22, 2026, 11:47 p.m. UTC 
Register NoiseReductionMode controls based on the tuning data and default
to the active DPF mode. Remove the static NoiseReductionMode entry from
the IPA control map now that DPF owns its registration.

Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>

---
  changelog :
    --No change since v10
---
 src/ipa/rkisp1/algorithms/dpf.cpp | 21 +++++++++++++++++++++
 src/ipa/rkisp1/algorithms/dpf.h   |  1 +
 src/ipa/rkisp1/rkisp1.cpp         |  1 -
 3 files changed, 22 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Jan. 23, 2026, 8:02 a.m. UTC | #1 
Hi Rui

On Thu, Jan 22, 2026 at 06:47:05PM -0500, Rui Wang wrote:
> Register NoiseReductionMode controls based on the tuning data and default
> to the active DPF mode. Remove the static NoiseReductionMode entry from
> the IPA control map now that DPF owns its registration.

I might have missed why you need a separate function to re-loop over
modes instead of doing this when parsing modes.

Was the patch I sent not functional ? Do you prefer a different
approach ? Can you share your design decisions or simply reply to that
email if you didn't agree with the suggestion explaining why ?

>
> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
>
> ---
>   changelog :
>     --No change since v10
> ---
>  src/ipa/rkisp1/algorithms/dpf.cpp | 21 +++++++++++++++++++++
>  src/ipa/rkisp1/algorithms/dpf.h   |  1 +
>  src/ipa/rkisp1/rkisp1.cpp         |  1 -
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> index 78a79fa5..282b8672 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> @@ -52,6 +52,9 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>  	if (ret)
>  		return ret;
>
> +	/* Register available controls. */
> +	registerControls(context);
> +
>  	return 0;
>  }
>
> @@ -112,6 +115,24 @@ int Dpf::parseConfig(const YamlObject &tuningData)
>  	return 0;
>  }
>
> +void Dpf::registerControls(IPAContext &context)
> +{
> +	/*
> +	 * Populate the control map with the available noise reduction modes.
> +	 * This allows applications to query and select from the modes defined
> +	 * in the tuning data.
> +	 */
> +	std::vector<ControlValue> modes{ controls::draft::NoiseReductionModeOff };
> +	for (const auto &mode : noiseReductionModes_) {
> +		modes.emplace_back(mode.modeValue);
> +	}
> +	/*
> +	 * Set the default mode to the active mode.
> +	 */
> +	context.ctrlMap[&controls::draft::NoiseReductionMode] =
> +		ControlInfo(modes, activeMode_->modeValue);
> +}
> +
>  int Dpf::parseSingleConfig(const YamlObject &tuningData,
>  			   rkisp1_cif_isp_dpf_config &config,
>  			   rkisp1_cif_isp_dpf_strength_config &strengthConfig)
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> index 11fc88e4..43effcbe 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.h
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -37,6 +37,7 @@ private:
>  	};
>
>  	int parseConfig(const YamlObject &tuningData);
> +	void registerControls(IPAContext &context);
>  	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 fbcc3910..402ed62c 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 */
> --
> 2.43.0
>
Rui Wang Jan. 23, 2026, 4:56 p.m. UTC | #2 
On 2026-01-23 03:02, Jacopo Mondi wrote:
> Hi Rui
>
> On Thu, Jan 22, 2026 at 06:47:05PM -0500, Rui Wang wrote:
>> Register NoiseReductionMode controls based on the tuning data and default
>> to the active DPF mode. Remove the static NoiseReductionMode entry from
>> the IPA control map now that DPF owns its registration.
> I might have missed why you need a separate function to re-loop over
> modes instead of doing this when parsing modes.
>
> Was the patch I sent not functional ? Do you prefer a different
> approach ? Can you share your design decisions or simply reply to that
> email if you didn't agree with the suggestion explaining why ?

Hello Jacopo,


The patch you sharing of add ctrlMap is working ,

I am thinking all controls can be declare in registerControls , and in 
future manual mode

would explicit many controls will add here. but actually , it will 
introduce a looper to push back

each element from noiseReductionModes_ to control map.and it can also 
decouple the ctrlMap

with tuning config

Rui

>
>> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
>> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
>>
>> ---
>>    changelog :
>>      --No change since v10
>> ---
>>   src/ipa/rkisp1/algorithms/dpf.cpp | 21 +++++++++++++++++++++
>>   src/ipa/rkisp1/algorithms/dpf.h   |  1 +
>>   src/ipa/rkisp1/rkisp1.cpp         |  1 -
>>   3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
>> index 78a79fa5..282b8672 100644
>> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
>> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
>> @@ -52,6 +52,9 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>>   	if (ret)
>>   		return ret;
>>
>> +	/* Register available controls. */
>> +	registerControls(context);
>> +
>>   	return 0;
>>   }
>>
>> @@ -112,6 +115,24 @@ int Dpf::parseConfig(const YamlObject &tuningData)
>>   	return 0;
>>   }
>>
>> +void Dpf::registerControls(IPAContext &context)
>> +{
>> +	/*
>> +	 * Populate the control map with the available noise reduction modes.
>> +	 * This allows applications to query and select from the modes defined
>> +	 * in the tuning data.
>> +	 */
>> +	std::vector<ControlValue> modes{ controls::draft::NoiseReductionModeOff };
>> +	for (const auto &mode : noiseReductionModes_) {
>> +		modes.emplace_back(mode.modeValue);
>> +	}
>> +	/*
>> +	 * Set the default mode to the active mode.
>> +	 */
>> +	context.ctrlMap[&controls::draft::NoiseReductionMode] =
>> +		ControlInfo(modes, activeMode_->modeValue);
>> +}
>> +
>>   int Dpf::parseSingleConfig(const YamlObject &tuningData,
>>   			   rkisp1_cif_isp_dpf_config &config,
>>   			   rkisp1_cif_isp_dpf_strength_config &strengthConfig)
>> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
>> index 11fc88e4..43effcbe 100644
>> --- a/src/ipa/rkisp1/algorithms/dpf.h
>> +++ b/src/ipa/rkisp1/algorithms/dpf.h
>> @@ -37,6 +37,7 @@ private:
>>   	};
>>
>>   	int parseConfig(const YamlObject &tuningData);
>> +	void registerControls(IPAContext &context);
>>   	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 fbcc3910..402ed62c 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 */
>> --
>> 2.43.0
>>
Rui Wang Feb. 3, 2026, 6:30 p.m. UTC | #3 
On 2026-01-23 03:02, Jacopo Mondi wrote:
> Hi Rui
>
> On Thu, Jan 22, 2026 at 06:47:05PM -0500, Rui Wang wrote:
>> Register NoiseReductionMode controls based on the tuning data and default
>> to the active DPF mode. Remove the static NoiseReductionMode entry from
>> the IPA control map now that DPF owns its registration.
> I might have missed why you need a separate function to re-loop over
> modes instead of doing this when parsing modes.
>
> Was the patch I sent not functional ? Do you prefer a different
> approach ? Can you share your design decisions or simply reply to that
> email if you didn't agree with the suggestion explaining why ?

Hello Jacopo,

your shared patch work in agc component.

  yes , registerControl would loop the map  again

I am considering about adding all dpf regs into control map for manual 
mode , that's why I add this function here.

and all controls register would be in one function.

and if you think it is redudant in current stage. I will move it into 
map parse function  to reduce one time loop.



>
>> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
>> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
>>
>> ---
>>    changelog :
>>      --No change since v10
>> ---
>>   src/ipa/rkisp1/algorithms/dpf.cpp | 21 +++++++++++++++++++++
>>   src/ipa/rkisp1/algorithms/dpf.h   |  1 +
>>   src/ipa/rkisp1/rkisp1.cpp         |  1 -
>>   3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
>> index 78a79fa5..282b8672 100644
>> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
>> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
>> @@ -52,6 +52,9 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>>   	if (ret)
>>   		return ret;
>>
>> +	/* Register available controls. */
>> +	registerControls(context);
>> +
>>   	return 0;
>>   }
>>
>> @@ -112,6 +115,24 @@ int Dpf::parseConfig(const YamlObject &tuningData)
>>   	return 0;
>>   }
>>
>> +void Dpf::registerControls(IPAContext &context)
>> +{
>> +	/*
>> +	 * Populate the control map with the available noise reduction modes.
>> +	 * This allows applications to query and select from the modes defined
>> +	 * in the tuning data.
>> +	 */
>> +	std::vector<ControlValue> modes{ controls::draft::NoiseReductionModeOff };
>> +	for (const auto &mode : noiseReductionModes_) {
>> +		modes.emplace_back(mode.modeValue);
>> +	}
>> +	/*
>> +	 * Set the default mode to the active mode.
>> +	 */
>> +	context.ctrlMap[&controls::draft::NoiseReductionMode] =
>> +		ControlInfo(modes, activeMode_->modeValue);
>> +}
>> +
>>   int Dpf::parseSingleConfig(const YamlObject &tuningData,
>>   			   rkisp1_cif_isp_dpf_config &config,
>>   			   rkisp1_cif_isp_dpf_strength_config &strengthConfig)
>> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
>> index 11fc88e4..43effcbe 100644
>> --- a/src/ipa/rkisp1/algorithms/dpf.h
>> +++ b/src/ipa/rkisp1/algorithms/dpf.h
>> @@ -37,6 +37,7 @@ private:
>>   	};
>>
>>   	int parseConfig(const YamlObject &tuningData);
>> +	void registerControls(IPAContext &context);
>>   	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 fbcc3910..402ed62c 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 */
>> --
>> 2.43.0
>>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
index 78a79fa5..282b8672 100644
--- a/src/ipa/rkisp1/algorithms/dpf.cpp
+++ b/src/ipa/rkisp1/algorithms/dpf.cpp
@@ -52,6 +52,9 @@  int Dpf::init([[maybe_unused]] IPAContext &context,
 	if (ret)
 		return ret;
 
+	/* Register available controls. */
+	registerControls(context);
+
 	return 0;
 }
 
@@ -112,6 +115,24 @@  int Dpf::parseConfig(const YamlObject &tuningData)
 	return 0;
 }
 
+void Dpf::registerControls(IPAContext &context)
+{
+	/*
+	 * Populate the control map with the available noise reduction modes.
+	 * This allows applications to query and select from the modes defined
+	 * in the tuning data.
+	 */
+	std::vector<ControlValue> modes{ controls::draft::NoiseReductionModeOff };
+	for (const auto &mode : noiseReductionModes_) {
+		modes.emplace_back(mode.modeValue);
+	}
+	/*
+	 * Set the default mode to the active mode.
+	 */
+	context.ctrlMap[&controls::draft::NoiseReductionMode] =
+		ControlInfo(modes, activeMode_->modeValue);
+}
+
 int Dpf::parseSingleConfig(const YamlObject &tuningData,
 			   rkisp1_cif_isp_dpf_config &config,
 			   rkisp1_cif_isp_dpf_strength_config &strengthConfig)
diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
index 11fc88e4..43effcbe 100644
--- a/src/ipa/rkisp1/algorithms/dpf.h
+++ b/src/ipa/rkisp1/algorithms/dpf.h
@@ -37,6 +37,7 @@  private:
 	};
 
 	int parseConfig(const YamlObject &tuningData);
+	void registerControls(IPAContext &context);
 	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 fbcc3910..402ed62c 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 */