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

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

Commit Message

Rui Wang Feb. 1, 2026, 7:16 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
  changelog since v11:
   - Simple Debug log
---
 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 Feb. 3, 2026, 2:08 p.m. UTC | #1
Rui,

On Sun, Feb 01, 2026 at 02:16:03PM -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.
>
> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
>
> ---
>   changelog :
>     --No change since v10
>   changelog since v11:
>    - Simple Debug log
> ---
>  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 9d7fcc1c..303d5cab 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;
>  }
>
> @@ -117,6 +120,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{};
> +	for (const auto &mode : noiseReductionModes_) {
> +		modes.emplace_back(mode.modeValue);
> +	}

No {}

> +	/*
> +	 * Set the default mode to the active mode.
> +	 */

Fits in 1 line

> +	context.ctrlMap[&controls::draft::NoiseReductionMode] =
> +		ControlInfo(modes, activeMode_->modeValue);

On v11 I asked the following question, and I'll paste it here with
your reply below, as I haven't replied on v11 (sorry about that).

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

I'm not sure I 100% understand why manual mode affects the available
modes map.

context.ctrlMap[&controls::draft::NoiseReductionMode] should be
populated with the entries from tuning file.

What I would expect (copying here from my v8 proposal at
https://patchwork.libcamera.org/patch/25818/#37709)

With "minimal" and "highquality" in the tuning
file:

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

With only "minimal" in the tuning file

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

etc etc

I expect manual mode to allow users to override the single parameters
that define a "mode" not to change the enumeration of available modes.

Anyway, since I might be missing the overall plan and you seem
convinced this is the way to go let's:

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

and we'll eventually rework on top if needed


> +}
> +
>  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, 7:07 p.m. UTC | #2
On 2026-02-03 09:08, Jacopo Mondi wrote:
> Rui,
>
> On Sun, Feb 01, 2026 at 02:16:03PM -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.
>>
>> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
>> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
>>
>> ---
>>    changelog :
>>      --No change since v10
>>    changelog since v11:
>>     - Simple Debug log
>> ---
>>   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 9d7fcc1c..303d5cab 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;
>>   }
>>
>> @@ -117,6 +120,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{};
>> +	for (const auto &mode : noiseReductionModes_) {
>> +		modes.emplace_back(mode.modeValue);
>> +	}
> No {}
>
>> +	/*
>> +	 * Set the default mode to the active mode.
>> +	 */
> Fits in 1 line
>
>> +	context.ctrlMap[&controls::draft::NoiseReductionMode] =
>> +		ControlInfo(modes, activeMode_->modeValue);
> On v11 I asked the following question, and I'll paste it here with
> your reply below, as I haven't replied on v11 (sorry about that).
>
> -------------------------------------------------------------------------------
>> 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
> -------------------------------------------------------------------------------
>
> I'm not sure I 100% understand why manual mode affects the available
> modes map.
>
> context.ctrlMap[&controls::draft::NoiseReductionMode] should be
> populated with the entries from tuning file.

Hello ,
Sorry I did not express clearly in v11 reply. and made you misunderstood.

I am going to exposure all dpf regsiters into controls as manual mode.

like : DomainFilter kernel value[6] nll [17] , strength [3]

and all those excute into this registerControls


  Rui

> What I would expect (copying here from my v8 proposal at
> https://patchwork.libcamera.org/patch/25818/#37709)
>
> With "minimal" and "highquality" in the tuning
> file:
>
> # cam -c1 --list-controls
> Control: [inout] draft::NoiseReductionMode:
>    - NoiseReductionModeOff (0) [default]
>    - NoiseReductionModeMinimal (3)
>    - NoiseReductionModeHighQuality (2)
>
> With only "minimal" in the tuning file
>
> # cam -c1 --list-controls
> Control: [inout] draft::NoiseReductionMode:
>    - NoiseReductionModeOff (0) [default]
>    - NoiseReductionModeMinimal (3)
>
> etc etc
>
I build and run current branch and with imx219.yaml

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

is your camera as imx219 ?



>
> I expect manual mode to allow users to override the single parameters
> that define a "mode" not to change the enumeration of available modes.
>
> Anyway, since I might be missing the overall plan and you seem
> convinced this is the way to go let's:
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> and we'll eventually rework on top if needed
>
>
>> +}
>> +
>>   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
>>
Jacopo Mondi Feb. 4, 2026, 10:26 a.m. UTC | #3
Hi Rui

On Tue, Feb 03, 2026 at 02:07:35PM -0500, rui wang wrote:
>
> On 2026-02-03 09:08, Jacopo Mondi wrote:
> > Rui,
> >
> > On Sun, Feb 01, 2026 at 02:16:03PM -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.
> > >
> > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> > > Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > >
> > > ---
> > >    changelog :
> > >      --No change since v10
> > >    changelog since v11:
> > >     - Simple Debug log
> > > ---
> > >   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 9d7fcc1c..303d5cab 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;
> > >   }
> > >
> > > @@ -117,6 +120,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{};
> > > +	for (const auto &mode : noiseReductionModes_) {
> > > +		modes.emplace_back(mode.modeValue);
> > > +	}
> > No {}
> >
> > > +	/*
> > > +	 * Set the default mode to the active mode.
> > > +	 */
> > Fits in 1 line
> >
> > > +	context.ctrlMap[&controls::draft::NoiseReductionMode] =

I just noticed it is probably worth moving NoiseReductionMode out of
draft and make it a core libcamera control

> > > +		ControlInfo(modes, activeMode_->modeValue);
> > On v11 I asked the following question, and I'll paste it here with
> > your reply below, as I haven't replied on v11 (sorry about that).
> >
> > -------------------------------------------------------------------------------
> > > 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
> > -------------------------------------------------------------------------------
> >
> > I'm not sure I 100% understand why manual mode affects the available
> > modes map.
> >
> > context.ctrlMap[&controls::draft::NoiseReductionMode] should be
> > populated with the entries from tuning file.
>
> Hello ,
> Sorry I did not express clearly in v11 reply. and made you misunderstood.
>
> I am going to exposure all dpf regsiters into controls as manual mode.
>
> like : DomainFilter kernel value[6] nll [17] , strength [3]
>
> and all those excute into this registerControls

mmm, ok, however manual controls won't be registered as values for
controls::draft::NoiseReductionMode but will go through other
controls, right ?

Also, the manual controls are platform specific ones which expose the
register interface of the RkISP1. How do we plan to expose them ?
Through a dedicated set of controls in a newly introduced
controls::rkisp1:: namespace ?

>
>
>  Rui
>
> > What I would expect (copying here from my v8 proposal at
> > https://patchwork.libcamera.org/patch/25818/#37709)
> >
> > With "minimal" and "highquality" in the tuning
> > file:
> >
> > # cam -c1 --list-controls
> > Control: [inout] draft::NoiseReductionMode:
> >    - NoiseReductionModeOff (0) [default]
> >    - NoiseReductionModeMinimal (3)
> >    - NoiseReductionModeHighQuality (2)
> >
> > With only "minimal" in the tuning file
> >
> > # cam -c1 --list-controls
> > Control: [inout] draft::NoiseReductionMode:
> >    - NoiseReductionModeOff (0) [default]
> >    - NoiseReductionModeMinimal (3)
> >
> > etc etc
> >
> I build and run current branch and with imx219.yaml
>
> # cam -c1 --list-controls
> Control: [inout] draft::NoiseReductionMode:
>   - NoiseReductionModeOff (0)
>   - NoiseReductionModeMinimal (3)
>   - NoiseReductionModeHighQuality (2) [default]
>
> is your camera as imx219 ?
>
>
>
> >
> > I expect manual mode to allow users to override the single parameters
> > that define a "mode" not to change the enumeration of available modes.
> >
> > Anyway, since I might be missing the overall plan and you seem
> > convinced this is the way to go let's:
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > and we'll eventually rework on top if needed
> >
> >
> > > +}
> > > +
> > >   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. 4, 2026, 12:29 p.m. UTC | #4
On 2026-02-04 05:26, Jacopo Mondi wrote:
> Hi Rui
>
> On Tue, Feb 03, 2026 at 02:07:35PM -0500, rui wang wrote:
>> On 2026-02-03 09:08, Jacopo Mondi wrote:
>>> Rui,
>>>
>>> On Sun, Feb 01, 2026 at 02:16:03PM -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.
>>>>
>>>> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
>>>> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
>>>>
>>>> ---
>>>>     changelog :
>>>>       --No change since v10
>>>>     changelog since v11:
>>>>      - Simple Debug log
>>>> ---
>>>>    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 9d7fcc1c..303d5cab 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;
>>>>    }
>>>>
>>>> @@ -117,6 +120,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{};
>>>> +	for (const auto &mode : noiseReductionModes_) {
>>>> +		modes.emplace_back(mode.modeValue);
>>>> +	}
>>> No {}
>>>
>>>> +	/*
>>>> +	 * Set the default mode to the active mode.
>>>> +	 */
>>> Fits in 1 line
>>>
>>>> +	context.ctrlMap[&controls::draft::NoiseReductionMode] =
> I just noticed it is probably worth moving NoiseReductionMode out of
> draft and make it a core libcamera control


Yes , I am think moving this draft controls to libcamera controls after 
those patch merged

it and will submit into seperate PR for this ,because it will change 
RPI's module as well.




>
>>>> +		ControlInfo(modes, activeMode_->modeValue);
>>> On v11 I asked the following question, and I'll paste it here with
>>> your reply below, as I haven't replied on v11 (sorry about that).
>>>
>>> -------------------------------------------------------------------------------
>>>> 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
>>> -------------------------------------------------------------------------------
>>>
>>> I'm not sure I 100% understand why manual mode affects the available
>>> modes map.
>>>
>>> context.ctrlMap[&controls::draft::NoiseReductionMode] should be
>>> populated with the entries from tuning file.
>> Hello ,
>> Sorry I did not express clearly in v11 reply. and made you misunderstood.
>>
>> I am going to exposure all dpf regsiters into controls as manual mode.
>>
>> like : DomainFilter kernel value[6] nll [17] , strength [3]
>>
>> and all those excute into this registerControls
> mmm, ok, however manual controls won't be registered as values for
> controls::draft::NoiseReductionMode but will go through other
> controls, right ?
>
> Also, the manual controls are platform specific ones which expose the
> register interface of the RkISP1. How do we plan to expose them ?
> Through a dedicated set of controls in a newly introduced
> controls::rkisp1:: namespace ?


          As my expectation ,"Manual control" will  belong to


          controls::draft::NoiseReductionMode ,

           From CamShark or any client once controls::draft::Manual


          is selected, the all DPF's register values can be edit and
          accepted


          from controls in manual mode.


            and "manual mode" would not break other


          NoiseReductionMode logic .


          all those dpf registers  will be in namespace :


          controls::rkisp1
          I had a debug version :
          https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/2/files#diff-4420c2393ceee9cd5ff9a751d04baf9ee580c739


>
>>
>>   Rui
>>
>>> What I would expect (copying here from my v8 proposal at
>>> https://patchwork.libcamera.org/patch/25818/#37709)
>>>
>>> With "minimal" and "highquality" in the tuning
>>> file:
>>>
>>> # cam -c1 --list-controls
>>> Control: [inout] draft::NoiseReductionMode:
>>>     - NoiseReductionModeOff (0) [default]
>>>     - NoiseReductionModeMinimal (3)
>>>     - NoiseReductionModeHighQuality (2)
>>>
>>> With only "minimal" in the tuning file
>>>
>>> # cam -c1 --list-controls
>>> Control: [inout] draft::NoiseReductionMode:
>>>     - NoiseReductionModeOff (0) [default]
>>>     - NoiseReductionModeMinimal (3)
>>>
>>> etc etc
>>>
>> I build and run current branch and with imx219.yaml
>>
>> # cam -c1 --list-controls
>> Control: [inout] draft::NoiseReductionMode:
>>    - NoiseReductionModeOff (0)
>>    - NoiseReductionModeMinimal (3)
>>    - NoiseReductionModeHighQuality (2) [default]
>>
>> is your camera as imx219 ?
>>
>>
>>
>>> I expect manual mode to allow users to override the single parameters
>>> that define a "mode" not to change the enumeration of available modes.
>>>
>>> Anyway, since I might be missing the overall plan and you seem
>>> convinced this is the way to go let's:
>>>
>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>
>>> and we'll eventually rework on top if needed
>>>
>>>
>>>> +}
>>>> +
>>>>    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
>>>>
Jacopo Mondi Feb. 4, 2026, 1 p.m. UTC | #5
Hi Rui

On Wed, Feb 04, 2026 at 07:29:03AM -0500, rui wang wrote:
>
> On 2026-02-04 05:26, Jacopo Mondi wrote:
> > Hi Rui
> >
> > On Tue, Feb 03, 2026 at 02:07:35PM -0500, rui wang wrote:
> > > On 2026-02-03 09:08, Jacopo Mondi wrote:
> > > > Rui,
> > > >
> > > > On Sun, Feb 01, 2026 at 02:16:03PM -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.
> > > > >
> > > > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> > > > > Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > > > >
> > > > > ---
> > > > >     changelog :
> > > > >       --No change since v10
> > > > >     changelog since v11:
> > > > >      - Simple Debug log
> > > > > ---
> > > > >    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 9d7fcc1c..303d5cab 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;
> > > > >    }
> > > > >
> > > > > @@ -117,6 +120,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{};
> > > > > +	for (const auto &mode : noiseReductionModes_) {
> > > > > +		modes.emplace_back(mode.modeValue);
> > > > > +	}
> > > > No {}
> > > >
> > > > > +	/*
> > > > > +	 * Set the default mode to the active mode.
> > > > > +	 */
> > > > Fits in 1 line
> > > >
> > > > > +	context.ctrlMap[&controls::draft::NoiseReductionMode] =
> > I just noticed it is probably worth moving NoiseReductionMode out of
> > draft and make it a core libcamera control
>
>
> Yes , I am think moving this draft controls to libcamera controls after
> those patch merged

mm, ok, I would do it before adding more users but I guess that's not
a big deal as long as it happens quickly. Otherwise if we add support
in rkisp1 for controls::draft::NoiseReductionMode to
later change it to controls::NoiseReductionMode in the next release,
it seems an ABI breakage we could easily avoid.

>
> it and will submit into seperate PR for this ,because it will change RPI's
> module as well.
>
>
>
>
> >
> > > > > +		ControlInfo(modes, activeMode_->modeValue);
> > > > On v11 I asked the following question, and I'll paste it here with
> > > > your reply below, as I haven't replied on v11 (sorry about that).
> > > >
> > > > -------------------------------------------------------------------------------
> > > > > 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
> > > > -------------------------------------------------------------------------------
> > > >
> > > > I'm not sure I 100% understand why manual mode affects the available
> > > > modes map.
> > > >
> > > > context.ctrlMap[&controls::draft::NoiseReductionMode] should be
> > > > populated with the entries from tuning file.
> > > Hello ,
> > > Sorry I did not express clearly in v11 reply. and made you misunderstood.
> > >
> > > I am going to exposure all dpf regsiters into controls as manual mode.
> > >
> > > like : DomainFilter kernel value[6] nll [17] , strength [3]
> > >
> > > and all those excute into this registerControls
> > mmm, ok, however manual controls won't be registered as values for
> > controls::draft::NoiseReductionMode but will go through other
> > controls, right ?
> >
> > Also, the manual controls are platform specific ones which expose the
> > register interface of the RkISP1. How do we plan to expose them ?
> > Through a dedicated set of controls in a newly introduced
> > controls::rkisp1:: namespace ?
>
>
>          As my expectation ,"Manual control" will  belong to
>
>
>          controls::draft::NoiseReductionMode ,
>
>           From CamShark or any client once controls::draft::Manual
>
>
>          is selected, the all DPF's register values can be edit and
>          accepted

Do you mean you plan to add a:
        controls::draft::NoiseReductionModeManual

value in the list of supported modes of
controls::draft::NoiseReductionMode ?


>
>
>          from controls in manual mode.
>
>
>            and "manual mode" would not break other
>
>
>          NoiseReductionMode logic .
>
>
>          all those dpf registers  will be in namespace :
>
>
>          controls::rkisp1
>          I had a debug version :
>          https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/2/files#diff-4420c2393ceee9cd5ff9a751d04baf9ee580c739

And once controls::draft::NoiseReductionModeManual is added the user
can control the parameters of the denoise engine throuhg the controls
you have added in the above pull request ?

That seems like a good plan, but to me NoiseReductionModeManual is
logically equivalent to NoiseReductionModeOff.

It doesn't require an entry in the tuning file. If the algorithm
implementation supports manual mode, the algorithm will add it to the
list of supported modes for controls::draft::NoiseReductionMode.

So, once manual mode support is added you can add it to the
noiseReductionModes_ list in your Dpf::parseConfig() implementation
before parsing the yaml file.

Do we agree or is your interpreatation different ?

>
>
> >
> > >
> > >   Rui
> > >
> > > > What I would expect (copying here from my v8 proposal at
> > > > https://patchwork.libcamera.org/patch/25818/#37709)
> > > >
> > > > With "minimal" and "highquality" in the tuning
> > > > file:
> > > >
> > > > # cam -c1 --list-controls
> > > > Control: [inout] draft::NoiseReductionMode:
> > > >     - NoiseReductionModeOff (0) [default]
> > > >     - NoiseReductionModeMinimal (3)
> > > >     - NoiseReductionModeHighQuality (2)
> > > >
> > > > With only "minimal" in the tuning file
> > > >
> > > > # cam -c1 --list-controls
> > > > Control: [inout] draft::NoiseReductionMode:
> > > >     - NoiseReductionModeOff (0) [default]
> > > >     - NoiseReductionModeMinimal (3)
> > > >
> > > > etc etc
> > > >
> > > I build and run current branch and with imx219.yaml
> > >
> > > # cam -c1 --list-controls
> > > Control: [inout] draft::NoiseReductionMode:
> > >    - NoiseReductionModeOff (0)
> > >    - NoiseReductionModeMinimal (3)
> > >    - NoiseReductionModeHighQuality (2) [default]
> > >
> > > is your camera as imx219 ?
> > >
> > >
> > >
> > > > I expect manual mode to allow users to override the single parameters
> > > > that define a "mode" not to change the enumeration of available modes.
> > > >
> > > > Anyway, since I might be missing the overall plan and you seem
> > > > convinced this is the way to go let's:
> > > >
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > >
> > > > and we'll eventually rework on top if needed
> > > >
> > > >
> > > > > +}
> > > > > +
> > > > >    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. 4, 2026, 1:55 p.m. UTC | #6
On 2026-02-04 08:00, Jacopo Mondi wrote:
> Hi Rui
>
> On Wed, Feb 04, 2026 at 07:29:03AM -0500, rui wang wrote:
>> On 2026-02-04 05:26, Jacopo Mondi wrote:
>>> Hi Rui
>>>
>>> On Tue, Feb 03, 2026 at 02:07:35PM -0500, rui wang wrote:
>>>> On 2026-02-03 09:08, Jacopo Mondi wrote:
>>>>> Rui,
>>>>>
>>>>> On Sun, Feb 01, 2026 at 02:16:03PM -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.
>>>>>>
>>>>>> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
>>>>>> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
>>>>>>
>>>>>> ---
>>>>>>      changelog :
>>>>>>        --No change since v10
>>>>>>      changelog since v11:
>>>>>>       - Simple Debug log
>>>>>> ---
>>>>>>     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 9d7fcc1c..303d5cab 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;
>>>>>>     }
>>>>>>
>>>>>> @@ -117,6 +120,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{};
>>>>>> +	for (const auto &mode : noiseReductionModes_) {
>>>>>> +		modes.emplace_back(mode.modeValue);
>>>>>> +	}
>>>>> No {}
>>>>>
>>>>>> +	/*
>>>>>> +	 * Set the default mode to the active mode.
>>>>>> +	 */
>>>>> Fits in 1 line
>>>>>
>>>>>> +	context.ctrlMap[&controls::draft::NoiseReductionMode] =
>>> I just noticed it is probably worth moving NoiseReductionMode out of
>>> draft and make it a core libcamera control
>>
>> Yes , I am think moving this draft controls to libcamera controls after
>> those patch merged
> mm, ok, I would do it before adding more users but I guess that's not
> a big deal as long as it happens quickly. Otherwise if we add support
> in rkisp1 for controls::draft::NoiseReductionMode to
> later change it to controls::NoiseReductionMode in the next release,
> it seems an ABI breakage we could easily avoid.

sure , once this patch series merged I will push one PR for 
draft::control rename

and will check with Kieran regarding to ABI breakage details

>
>> it and will submit into seperate PR for this ,because it will change RPI's
>> module as well.
>>
>>
>>
>>
>>>>>> +		ControlInfo(modes, activeMode_->modeValue);
>>>>> On v11 I asked the following question, and I'll paste it here with
>>>>> your reply below, as I haven't replied on v11 (sorry about that).
>>>>>
>>>>> -------------------------------------------------------------------------------
>>>>>> 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
>>>>> -------------------------------------------------------------------------------
>>>>>
>>>>> I'm not sure I 100% understand why manual mode affects the available
>>>>> modes map.
>>>>>
>>>>> context.ctrlMap[&controls::draft::NoiseReductionMode] should be
>>>>> populated with the entries from tuning file.
>>>> Hello ,
>>>> Sorry I did not express clearly in v11 reply. and made you misunderstood.
>>>>
>>>> I am going to exposure all dpf regsiters into controls as manual mode.
>>>>
>>>> like : DomainFilter kernel value[6] nll [17] , strength [3]
>>>>
>>>> and all those excute into this registerControls
>>> mmm, ok, however manual controls won't be registered as values for
>>> controls::draft::NoiseReductionMode but will go through other
>>> controls, right ?
>>>
>>> Also, the manual controls are platform specific ones which expose the
>>> register interface of the RkISP1. How do we plan to expose them ?
>>> Through a dedicated set of controls in a newly introduced
>>> controls::rkisp1:: namespace ?
>>
>>           As my expectation ,"Manual control" will  belong to
>>
>>
>>           controls::draft::NoiseReductionMode ,
>>
>>            From CamShark or any client once controls::draft::Manual
>>
>>
>>           is selected, the all DPF's register values can be edit and
>>           accepted
> Do you mean you plan to add a:
>          controls::draft::NoiseReductionModeManual
>
> value in the list of supported modes of
> controls::draft::NoiseReductionMode ?
>
>
>>
>>           from controls in manual mode.
>>
>>
>>             and "manual mode" would not break other
>>
>>
>>           NoiseReductionMode logic .
>>
>>
>>           all those dpf registers  will be in namespace :
>>
>>
>>           controls::rkisp1
>>           I had a debug version :
>>           https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/2/files#diff-4420c2393ceee9cd5ff9a751d04baf9ee580c739
> And once controls::draft::NoiseReductionModeManual is added the user
> can control the parameters of the denoise engine throuhg the controls
> you have added in the above pull request ?

Yes , all dpf regs value can by write from camshark controls.

it is valuable for manual tuning dpf peformance and also provide some 
specific

denoise  profile as user expectation

> That seems like a good plan, but to me NoiseReductionModeManual is
> logically equivalent to NoiseReductionModeOff.
yes

NoiseReductionModeManual is equivalent to "OFF" "Mininal"

>
> It doesn't require an entry in the tuning file. If the algorithm
> implementation supports manual mode, the algorithm will add it to the
> list of supported modes for controls::draft::NoiseReductionMode.
>
> So, once manual mode support is added you can add it to the
> noiseReductionModes_ list in your Dpf::parseConfig() implementation
> before parsing the yaml file.
>
> Do we agree or is your interpreatation different ?

parseConfig would parse maunual init setting from yaml as well like we discussed ever
noiseReductionModes_ will be added manual mode's config once parse successfully


>
>>
>>>>    Rui
>>>>
>>>>> What I would expect (copying here from my v8 proposal at
>>>>> https://patchwork.libcamera.org/patch/25818/#37709)
>>>>>
>>>>> With "minimal" and "highquality" in the tuning
>>>>> file:
>>>>>
>>>>> # cam -c1 --list-controls
>>>>> Control: [inout] draft::NoiseReductionMode:
>>>>>      - NoiseReductionModeOff (0) [default]
>>>>>      - NoiseReductionModeMinimal (3)
>>>>>      - NoiseReductionModeHighQuality (2)
>>>>>
>>>>> With only "minimal" in the tuning file
>>>>>
>>>>> # cam -c1 --list-controls
>>>>> Control: [inout] draft::NoiseReductionMode:
>>>>>      - NoiseReductionModeOff (0) [default]
>>>>>      - NoiseReductionModeMinimal (3)
>>>>>
>>>>> etc etc
>>>>>
>>>> I build and run current branch and with imx219.yaml
>>>>
>>>> # cam -c1 --list-controls
>>>> Control: [inout] draft::NoiseReductionMode:
>>>>     - NoiseReductionModeOff (0)
>>>>     - NoiseReductionModeMinimal (3)
>>>>     - NoiseReductionModeHighQuality (2) [default]
>>>>
>>>> is your camera as imx219 ?
>>>>
>>>>
>>>>
>>>>> I expect manual mode to allow users to override the single parameters
>>>>> that define a "mode" not to change the enumeration of available modes.
>>>>>
>>>>> Anyway, since I might be missing the overall plan and you seem
>>>>> convinced this is the way to go let's:
>>>>>
>>>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>>>
>>>>> and we'll eventually rework on top if needed
>>>>>
>>>>>
>>>>>> +}
>>>>>> +
>>>>>>     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 9d7fcc1c..303d5cab 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;
 }
 
@@ -117,6 +120,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{};
+	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 */