[v3,1/2] ipa/rkisp1: refactory DPF parsing and initialization
diff mbox series

Message ID 20251204213744.1110922-2-rui.wang@ideasonboard.com
State Superseded
Headers show
Series
  • rebase_dpf_refactory_patch_v3
Related show

Commit Message

Rui Wang Dec. 4, 2025, 9:37 p.m. UTC 
Split DPF configuration parsing and initialization into clearer,
self-contained helpers and modernized initialization patterns.

Add parseConfig, parseModes and loadReductionConfig to factor
parsing logic and mode lookups.
Introduce ModeConfig and store mode entries in
noiseReductionModes_ to decouple parsing from runtime selection.
Move sentinel member initializers into the constructor initializer
list (runningMode_) and declare vectors
without in-class initializers for default construction.
Replace ad-hoc string handling with value_or and const auto
where appropriate for clearer and safer parsing.
Replace domain/range/strength YAML keys mapping and error returns
(use filter, nll, strength keys and return false from parse
helpers instead of -EINVAL).
Add loadReductionConfig to centralize loading of DPF configs for
reduction modes and preserve logging and failure behavior.
Adjust queueRequest, prepare, and helpers to use the new
parsing/initialization flow while preserving existing behavior.
This refactor improves separation of concerns, makes parsing easier
to maintain, and reduces duplicated logic. No functional behaviour is
intended to be changed.

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

---
changelog :
 1. fix config issue and some format
---
 Makefile                          | 125 +++++++++++
 src/ipa/rkisp1/algorithms/dpf.cpp | 359 +++++++++++++++++++++++-------
 src/ipa/rkisp1/algorithms/dpf.h   |  22 ++
 3 files changed, 420 insertions(+), 86 deletions(-)
 create mode 100644 Makefile

Comments

Kieran Bingham Dec. 6, 2025, 9:07 p.m. UTC | #1 
Hi Rui,

In $SUBJECT s#ipa/rkisp1: refactory#ipa: rkisp1: refactor#

V2, then v3 came through in quick succession. What's the changelog
between them ?

What was v1 in this series? I'm afraid it's getting hard to track so
many submissions with inconsistent versions and no changelogs.

Is this still a continuation of
https://patchwork.libcamera.org/project/libcamera/list/?series=5612 ?

Or is it a whole rewrite with a new direction?

Quoting Rui Wang (2025-12-04 21:37:43)
> Split DPF configuration parsing and initialization into clearer,
> self-contained helpers and modernized initialization patterns.
> 

This following block of text is very terse. Please at least put a blank
line in between paragraphs to make it easier to read, but it seems to be
a sign that we're doing too much in a single patch. 

> Add parseConfig, parseModes and loadReductionConfig to factor
> parsing logic and mode lookups.
> Introduce ModeConfig and store mode entries in
> noiseReductionModes_ to decouple parsing from runtime selection.


> Move sentinel member initializers into the constructor initializer
> list (runningMode_) and declare vectors
> without in-class initializers for default construction.
> Replace ad-hoc string handling with value_or and const auto
> where appropriate for clearer and safer parsing.

> Replace domain/range/strength YAML keys mapping and error returns
> (use filter, nll, strength keys and return false from parse
> helpers instead of -EINVAL).


If you rename tuning file keys - that's essentially a tuning file API
breakage, so the commit message should be /really/ clear on this update.

The commit message up here could or rather should at least be declaring
that the following tuning file keys have been renamed:

  - DomainFilter -> filter
  - NoiseLevelFunction -> nll
  - FilterStrength -> gain
  - ... ?

I think the renames themselves could be an independent patch so they can
be considered alone, before making each one defined per mode.

> Add loadReductionConfig to centralize loading of DPF configs for
> reduction modes and preserve logging and failure behavior.
> Adjust queueRequest, prepare, and helpers to use the new
> parsing/initialization flow while preserving existing behavior.

> This refactor improves separation of concerns, makes parsing easier
> to maintain, and reduces duplicated logic. No functional behaviour is
> intended to be changed.

But - doesn't it have lots of functional behaviour changes? The entire
tuning file layout has now changed as far as I can tell.

And that now they are subkeys and can be defined explicitly for each of
the control modes that can be selected.

It seems that pulling the parameters out to be per-control mode option
is actually the main purpose of the patch - but nothing above really
tells me that?

> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
> 
> ---
> changelog :
>  1. fix config issue and some format
> ---
>  Makefile                          | 125 +++++++++++
>  src/ipa/rkisp1/algorithms/dpf.cpp | 359 +++++++++++++++++++++++-------
>  src/ipa/rkisp1/algorithms/dpf.h   |  22 ++
>  3 files changed, 420 insertions(+), 86 deletions(-)
>  create mode 100644 Makefile
> 
> diff --git a/Makefile b/Makefile
> new file mode 100644
> index 00000000..07f8f0fb
> --- /dev/null
> +++ b/Makefile
> @@ -0,0 +1,125 @@
> +# Support execution in a docker environment

Please remove this file addition from the patch.

This has been attached to multiple submissions now, please be careful to
review your patches yourself before posting to make sure there are no
unexpected attachments or incorrect squashes.

<snip Makefile>

> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> index 39f3e461..7e2da7b1 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> @@ -37,7 +37,8 @@ namespace ipa::rkisp1::algorithms {
>  LOG_DEFINE_CATEGORY(RkISP1Dpf)
>  
>  Dpf::Dpf()
> -       : config_({}), strengthConfig_({})
> +       : config_({}), strengthConfig_({}),
> +         runningMode_(controls::draft::NoiseReductionModeOff)
>  {
>  }
>  
> @@ -46,6 +47,69 @@ Dpf::Dpf()
>   */
>  int Dpf::init([[maybe_unused]] IPAContext &context,
>               const YamlObject &tuningData)
> +{
> +       /* Parse tuning block. */
> +       if (!parseConfig(tuningData)) {
> +               return -EINVAL;
> +       }


Single line conditionals do not have { } in our coding style, so this
can be just 

	if (!parseConfig(tuningData))
		return -EINVAL;

Same in all the other locations.


> +
> +       return 0;
> +}
> +bool Dpf::parseConfig(const YamlObject &tuningData)
> +{
> +       /* Parse base config. */
> +       if (!parseSingleConfig(tuningData, config_, strengthConfig_)) {
> +               return false;
> +       }
> +       if (!parseModes(tuningData)) {
> +               return false;
> +       }
> +       return true;
> +}
> +
> +bool Dpf::parseModes(const YamlObject &tuningData)
> +{
> +       /* Parse noise reduction modes. */
> +       if (!tuningData.contains("modes")) {
> +               return true;
> +       }
> +
> +       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 false;
> +               }
> +
> +               int32_t modeValue = controls::draft::NoiseReductionModeOff;
> +               if (*typeOpt == "minimal") {
> +                       modeValue = controls::draft::NoiseReductionModeMinimal;
> +               } else if (*typeOpt == "fast") {
> +                       modeValue = controls::draft::NoiseReductionModeFast;
> +               } else if (*typeOpt == "highquality") {
> +                       modeValue = controls::draft::NoiseReductionModeHighQuality;
> +               } else if (*typeOpt == "zsl") {
> +                       modeValue = controls::draft::NoiseReductionModeZSL;
> +               } else {
> +                       LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt;
> +                       return false;
> +               }
> +
> +               ModeConfig mode{};
> +               mode.modeValue = modeValue;
> +               if (!parseSingleConfig(entry, mode.dpf, mode.strength)) {
> +                       return false;
> +               }
> +               noiseReductionModes_.push_back(mode);
> +       }
> +
> +       return true;
> +}
> +bool Dpf::parseSingleConfig(const YamlObject &tuningData,

There should always be a blank line separating functions.

> +                           rkisp1_cif_isp_dpf_config &config,
> +                           rkisp1_cif_isp_dpf_strength_config &strengthConfig)
>  {
>         std::vector<uint8_t> values;
>  
> @@ -53,7 +117,11 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>          * The domain kernel is configured with a 9x9 kernel for the green
>          * pixels, and a 13x9 or 9x9 kernel for red and blue pixels.
>          */
> -       const YamlObject &dFObject = tuningData["DomainFilter"];
> +       const YamlObject &dFObject = tuningData["filter"];
> +       if (!dFObject) {
> +               LOG(RkISP1Dpf, Error) << "filter section missing";
> +               return false;
> +       }
>  
>         /*
>          * For the green component, we have the 9x9 kernel specified
> @@ -75,17 +143,17 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>         values = dFObject["g"].getList<uint8_t>().value_or(std::vector<uint8_t>{});
>         if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS) {
>                 LOG(RkISP1Dpf, Error)
> -                       << "Invalid 'DomainFilter:g': expected "
> +                       << "Invalid 'filter:g': expected "
>                         << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
>                         << " elements, got " << values.size();
> -               return -EINVAL;
> +               return false;
>         }
>  
>         std::copy_n(values.begin(), values.size(),
> -                   std::begin(config_.g_flt.spatial_coeff));
> +                   std::begin(config.g_flt.spatial_coeff));
>  
> -       config_.g_flt.gr_enable = true;
> -       config_.g_flt.gb_enable = true;
> +       config.g_flt.gr_enable = true;
> +       config.g_flt.gb_enable = true;
>  
>         /*
>          * For the red and blue components, we have the 13x9 kernel specified
> @@ -112,65 +180,155 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>         if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS &&
>             values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1) {
>                 LOG(RkISP1Dpf, Error)
> -                       << "Invalid 'DomainFilter:rb': expected "
> +                       << "Invalid 'filter:rb': expected "
>                         << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1
>                         << " or " << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
>                         << " elements, got " << values.size();
> -               return -EINVAL;
> +               return false;
>         }
>  
> -       config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
> -                              ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
> -                              : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
> +       config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
> +                                       ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
> +                                       : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
>  
>         std::copy_n(values.begin(), values.size(),
> -                   std::begin(config_.rb_flt.spatial_coeff));
> +                   std::begin(config.rb_flt.spatial_coeff));
>  
> -       config_.rb_flt.r_enable = true;
> -       config_.rb_flt.b_enable = true;
> +       config.rb_flt.r_enable = true;
> +       config.rb_flt.b_enable = true;
>  
>         /*
>          * The range kernel is configured with a noise level lookup table (NLL)
>          * which stores a piecewise linear function that characterizes the

Should this be using the PWL helpers at all ?


>          * sensor noise profile as a noise level function curve (NLF).
>          */
> -       const YamlObject &rFObject = tuningData["NoiseLevelFunction"];
> +       const YamlObject &rFObject = tuningData["nll"];
> +       if (!rFObject) {
> +               LOG(RkISP1Dpf, Error) << "nll section missing";
> +               return false;
> +       }
>  
> -       std::vector<uint16_t> nllValues;
> -       nllValues = rFObject["coeff"].getList<uint16_t>().value_or(std::vector<uint16_t>{});
> +       const auto nllValues =
> +               rFObject["coeff"].getList<uint16_t>().value_or(std::vector<uint16_t>{});

If it's valid to use the Pwl type - then we can potentially use the
YamlObject::Getter too if it helps?

libipa/pwl.cpp:YamlObject::Getter<ipa::Pwl>::get(const YamlObject &obj) const


>         if (nllValues.size() != RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS) {
>                 LOG(RkISP1Dpf, Error)
> -                       << "Invalid 'RangeFilter:coeff': expected "
> +                       << "Invalid 'nll:coeff': expected "
>                         << RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS
>                         << " elements, got " << nllValues.size();
> -               return -EINVAL;
> +               return false;
>         }
>  
>         std::copy_n(nllValues.begin(), nllValues.size(),
> -                   std::begin(config_.nll.coeff));
> +                   std::begin(config.nll.coeff));
>  
> -       std::string scaleMode = rFObject["scale-mode"].get<std::string>("");
> +       const auto scaleMode = rFObject["scale-mode"].get<std::string>("");
>         if (scaleMode == "linear") {
> -               config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;
> +               config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;
>         } else if (scaleMode == "logarithmic") {
> -               config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
> +               config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
>         } else {
>                 LOG(RkISP1Dpf, Error)
> -                       << "Invalid 'RangeFilter:scale-mode': expected "
> +                       << "Invalid 'nll:scale-mode': expected "
>                         << "'linear' or 'logarithmic' value, got "
>                         << scaleMode;
> -               return -EINVAL;
> +               return false;
>         }
>  
> -       const YamlObject &fSObject = tuningData["FilterStrength"];
> +       const YamlObject &gObject = tuningData["gain"];
> +       if (!gObject) {
> +               LOG(RkISP1Dpf, Error) << "gain section missing";
> +               return false;
> +       }
>  
> -       strengthConfig_.r = fSObject["r"].get<uint16_t>(64);
> -       strengthConfig_.g = fSObject["g"].get<uint16_t>(64);
> -       strengthConfig_.b = fSObject["b"].get<uint16_t>(64);
> +       config.gain.mode =
> +               gObject["gain_mode"].get<uint32_t>().value_or(
> +                       RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS);
> +       config.gain.nf_r_gain = gObject["r"].get<uint16_t>().value_or(256);
> +       config.gain.nf_b_gain = gObject["b"].get<uint16_t>().value_or(256);
> +       config.gain.nf_gr_gain = gObject["gr"].get<uint16_t>().value_or(256);
> +       config.gain.nf_gb_gain = gObject["gb"].get<uint16_t>().value_or(256);
> +
> +       const YamlObject &fSObject = tuningData["strength"];
> +       if (!fSObject) {
> +               LOG(RkISP1Dpf, Error) << "strength section missing";
> +               return false;
> +       }
>  
> -       return 0;
> +       strengthConfig.r = fSObject["r"].get<uint8_t>().value_or(64);
> +       strengthConfig.g = fSObject["g"].get<uint8_t>().value_or(64);
> +       strengthConfig.b = fSObject["b"].get<uint8_t>().value_or(64);
> +       return true;
> +}
> +
> +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 "
> +                       << static_cast<int>(mode);
> +               return false;
> +       }
> +
> +       config_ = it->dpf;
> +       strengthConfig_ = it->strength;
> +
> +       LOG(RkISP1Dpf, Info)
> +               << "DPF mode=Reduction (config loaded)"
> +               << " mode=" << static_cast<int>(mode);
> +
> +       return true;
>  }

Blank line required here.

> +void Dpf::logConfigIfChanged(const IPAFrameContext &frameContext)
> +{
> +       if (!frameContext.dpf.update) {
> +               return;
> +       }
> +
> +       std::ostringstream ss;
> +
> +       ss << "DPF config update: ";
> +       ss << ", control mode=" << static_cast<int>(runningMode_);
> +       ss << ", denoise=" << (frameContext.dpf.denoise ? "enabled" : "disabled, ");
> +
> +       ss << "rb_fltsize="
> +          << (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? "13x9" : "9x9");
> +       ss << ", nll_scale="
> +          << (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? "log" : "linear");
> +       ss << ", gain_mode=" << config_.gain.mode;
> +       ss << ", strength=" << int(strengthConfig_.r) << ',' << int(strengthConfig_.g) << ',' << int(strengthConfig_.b);
> +
> +       ss << ", g=[";
> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
> +               if (i) {
> +                       ss << ',';
> +               }
> +               ss << int(config_.g_flt.spatial_coeff[i]);
> +       }
> +       ss << "]";
>  
> +       ss << ", rb=[";
> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
> +               if (i) {
> +                       ss << ',';
> +               }
> +               ss << int(config_.rb_flt.spatial_coeff[i]);
> +       }
> +       ss << "]";
> +
> +       ss << ", nll=[";
> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS; ++i) {
> +               if (i) {
> +                       ss << ',';
> +               }
> +               ss << int(config_.nll.coeff[i]);
> +       }
> +       ss << "]";
> +       LOG(RkISP1Dpf, Info) << ss.str();
> +}
>  /**
>   * \copydoc libcamera::ipa::Algorithm::queueRequest
>   */
> @@ -183,30 +341,34 @@ void Dpf::queueRequest(IPAContext &context,
>         bool update = false;
>  
>         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) {
> -                               dpf.denoise = false;
> -                               update = true;
> -                       }
> -                       break;
> -               case controls::draft::NoiseReductionModeMinimal:
> -               case controls::draft::NoiseReductionModeHighQuality:
> -               case controls::draft::NoiseReductionModeFast:
> -                       if (!dpf.denoise) {
> -                               dpf.denoise = true;
> -                               update = true;
> -                       }
> -                       break;
> -               default:
> -                       LOG(RkISP1Dpf, Error)
> -                               << "Unsupported denoise value "
> -                               << *denoise;
> -                       break;
> +       if (!denoise) {
> +               return;
> +       }
> +       runningMode_ = *denoise;
> +       switch (runningMode_) {
> +       case controls::draft::NoiseReductionModeOff:
> +               if (dpf.denoise) {
> +                       dpf.denoise = false;
> +                       update = true;
> +               }
> +               break;
> +       case controls::draft::NoiseReductionModeMinimal:
> +       case controls::draft::NoiseReductionModeHighQuality:
> +       case controls::draft::NoiseReductionModeFast:
> +       case controls::draft::NoiseReductionModeZSL:
> +               if (loadReductionConfig(runningMode_)) {
> +                       update = true;
> +                       dpf.denoise = true;
> +               } else {
> +                       dpf.denoise = false;
> +                       update = true;
>                 }
> +               break;
> +       default:
> +               LOG(RkISP1Dpf, Error)
> +                       << "Unsupported denoise value "
> +                       << *denoise;
> +               break;
>         }
>  
>         frameContext.dpf.denoise = dpf.denoise;
> @@ -219,41 +381,66 @@ void Dpf::queueRequest(IPAContext &context,
>  void Dpf::prepare(IPAContext &context, const uint32_t frame,
>                   IPAFrameContext &frameContext, RkISP1Params *params)
>  {
> -       if (!frameContext.dpf.update && frame > 0)
> +       if (!frameContext.dpf.update && frame > 0) {
>                 return;
> +       }
> +
> +       if (!frameContext.dpf.denoise) {
> +               prepareDisabledMode(context, frame, frameContext, params);
> +               return;
> +       }
> +
> +       prepareEnabledMode(context, frame, frameContext, params);
> +}
> +
> +void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,
> +                             [[maybe_unused]] const uint32_t frame,
> +                             [[maybe_unused]] IPAFrameContext &frameContext,
> +                             RkISP1Params *params)
> +{
> +       frameContext.dpf.denoise = false;
> +       auto dpfConfig = params->block<BlockType::Dpf>();
> +       dpfConfig.setEnabled(false);
> +       auto dpfStrength = params->block<BlockType::DpfStrength>();
> +       dpfStrength.setEnabled(false);
> +}
> +
> +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> +                            [[maybe_unused]] IPAFrameContext &frameContext, RkISP1Params *params)
> +{
> +       auto dpfConfig = params->block<BlockType::Dpf>();
> +       dpfConfig.setEnabled(true);
> +       *dpfConfig = config_;
>  
> -       auto config = params->block<BlockType::Dpf>();
> -       config.setEnabled(frameContext.dpf.denoise);
> -
> -       auto strengthConfig = params->block<BlockType::DpfStrength>();
> -       strengthConfig.setEnabled(frameContext.dpf.denoise);
> -
> -       if (frameContext.dpf.denoise) {
> -               *config = config_;
> -               *strengthConfig = strengthConfig_;
> -
> -               const auto &awb = context.configuration.awb;
> -               const auto &lsc = context.configuration.lsc;
> -
> -               auto &mode = config->gain.mode;
> -
> -               /*
> -                * The DPF needs to take into account the total amount of
> -                * digital gain, which comes from the AWB and LSC modules. The
> -                * DPF hardware can be programmed with a digital gain value
> -                * manually, but can also use the gains supplied by the AWB and
> -                * LSC modules automatically when they are enabled. Use that
> -                * mode of operation as it simplifies control of the DPF.
> -                */
> -               if (awb.enabled && lsc.enabled)
> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
> -               else if (awb.enabled)
> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
> -               else if (lsc.enabled)
> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> -               else
> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> +       /*
> +        * The DPF needs to take into account the total amount of
> +        * digital gain, which comes from the AWB and LSC modules. The
> +        * DPF hardware can be programmed with a digital gain value
> +        * manually, but can also use the gains supplied by the AWB and
> +        * LSC modules automatically when they are enabled. Use that
> +        * mode of operation as it simplifies control of the DPF.
> +        */
> +       const auto &awb = context.configuration.awb;
> +       const auto &lsc = context.configuration.lsc;
> +       auto &mode = dpfConfig->gain.mode;
> +
> +       if (awb.enabled && lsc.enabled) {
> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
> +       } else if (awb.enabled) {
> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
> +       } else if (lsc.enabled) {
> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> +       } else {
> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
>         }
> +       config_.gain.mode = mode;
> +
> +       auto dpfStrength = params->block<BlockType::DpfStrength>();
> +       dpfStrength.setEnabled(true);
> +
> +       *dpfStrength = strengthConfig_;
> +
> +       logConfigIfChanged(frameContext);
>  }
>  
>  REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> index 2dd8cd36..d4bfb2dc 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.h
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -30,8 +30,30 @@ public:
>                      RkISP1Params *params) override;
>  
>  private:
> +       struct ModeConfig {
> +               int32_t modeValue;
> +               rkisp1_cif_isp_dpf_config dpf;
> +               rkisp1_cif_isp_dpf_strength_config strength;
> +       };
> +       bool parseConfig(const YamlObject &tuningData);
> +       bool parseModes(const YamlObject &tuningData);
> +       bool parseSingleConfig(const YamlObject &tuningData,
> +                              rkisp1_cif_isp_dpf_config &config,
> +                              rkisp1_cif_isp_dpf_strength_config &strengthConfig);
> +
> +       bool loadReductionConfig(int32_t mode);
> +       void logConfigIfChanged(const IPAFrameContext &frameContext);
> +
> +       void prepareDisabledMode(IPAContext &context, const uint32_t frame,
> +                                IPAFrameContext &frameContext,
> +                                RkISP1Params *params);
> +       void prepareEnabledMode(IPAContext &context, const uint32_t frame,
> +                               IPAFrameContext &frameContext, RkISP1Params *params);
> +
>         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 Dec. 7, 2025, 2:34 a.m. UTC | #2 
On 2025-12-06 16:07, Kieran Bingham wrote:
> Hi Rui,
>
> In $SUBJECT s#ipa/rkisp1: refactory#ipa: rkisp1: refactor#
>
> V2, then v3 came through in quick succession. What's the changelog
> between them ?
>
> What was v1 in this series? I'm afraid it's getting hard to track so
> many submissions with inconsistent versions and no changelogs.
>
> Is this still a continuation of
> https://patchwork.libcamera.org/project/libcamera/list/?series=5612 ?
>
> Or is it a whole rewrite with a new direction?

Yes , this patch series are the following of the 5612 , as discussed 
with Jacopo,  he also suggested

to split 
https://patchwork.libcamera.org/project/libcamera/list/?series=5612 ?

so I created this patch series , just refactor the dpf as first part.
then will apply auto mode and manual mode as part II and part II

Quoting Rui Wang (2025-12-04 21:37:43)

>> Split DPF configuration parsing and initialization into clearer,
>> self-contained helpers and modernized initialization patterns.
>>
> This following block of text is very terse. Please at least put a blank
> line in between paragraphs to make it easier to read, but it seems to be
> a sign that we're doing too much in a single patch.
thanks , I will rewrite the commit message with more clear change 
information.
>
>> Add parseConfig, parseModes and loadReductionConfig to factor
>> parsing logic and mode lookups.
>> Introduce ModeConfig and store mode entries in
>> noiseReductionModes_ to decouple parsing from runtime selection.
>
>> Move sentinel member initializers into the constructor initializer
>> list (runningMode_) and declare vectors
>> without in-class initializers for default construction.
>> Replace ad-hoc string handling with value_or and const auto
>> where appropriate for clearer and safer parsing.
>> Replace domain/range/strength YAML keys mapping and error returns
>> (use filter, nll, strength keys and return false from parse
>> helpers instead of -EINVAL).
>
> If you rename tuning file keys - that's essentially a tuning file API
> breakage, so the commit message should be /really/ clear on this update.
>
> The commit message up here could or rather should at least be declaring
> that the following tuning file keys have been renamed:
>
>    - DomainFilter -> filter
>    - NoiseLevelFunction -> nll
>    - FilterStrength -> gain
>    - ... ?
>
> I think the renames themselves could be an independent patch so they can
> be considered alone, before making each one defined per mode.

in next series, I will split single one patch into 5 patches

  1. Refactor dpf  yaml parser implementation into parsSingleConfig() ,  
and introduce parseConfig into init()

  2.rename yaml dpf key name :

   - DomainFilter -> filter
   - NoiseLevelFunction -> nll
   - FilterStrength -> gain
   - etc

  3.Refactor dpf prepare logic , split into enable/disable, and add some 
debug information

  4.Add reduction mode parser funntion  for different 
mode(highquality/zsl/fast..) and log information

  5.Add dpf config into rkisp/imx219.yaml

>
>> Add loadReductionConfig to centralize loading of DPF configs for
>> reduction modes and preserve logging and failure behavior.
>> Adjust queueRequest, prepare, and helpers to use the new
>> parsing/initialization flow while preserving existing behavior.
>> This refactor improves separation of concerns, makes parsing easier
>> to maintain, and reduces duplicated logic. No functional behaviour is
>> intended to be changed.
> But - doesn't it have lots of functional behaviour changes? The entire
> tuning file layout has now changed as far as I can tell.
  I will add one patch in following series to highlight refactory code 
and new logic added in commit message
>
> And that now they are subkeys and can be defined explicitly for each of
> the control modes that can be selected.
>
> It seems that pulling the parameters out to be per-control mode option
> is actually the main purpose of the patch - but nothing above really
> tells me that?
>
>> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>
>>
>> ---
>> changelog :
>>   1. fix config issue and some format
>> ---
>>   Makefile                          | 125 +++++++++++
>>   src/ipa/rkisp1/algorithms/dpf.cpp | 359 +++++++++++++++++++++++-------
>>   src/ipa/rkisp1/algorithms/dpf.h   |  22 ++
>>   3 files changed, 420 insertions(+), 86 deletions(-)
>>   create mode 100644 Makefile
>>
>> diff --git a/Makefile b/Makefile
>> new file mode 100644
>> index 00000000..07f8f0fb
>> --- /dev/null
>> +++ b/Makefile
>> @@ -0,0 +1,125 @@
>> +# Support execution in a docker environment
> Please remove this file addition from the patch.
>
> This has been attached to multiple submissions now, please be careful to
> review your patches yourself before posting to make sure there are no
> unexpected attachments or incorrect squashes.
>
> <snip Makefile>
I will do git format cautiously and check all patch files.
>
>> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
>> index 39f3e461..7e2da7b1 100644
>> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
>> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
>> @@ -37,7 +37,8 @@ namespace ipa::rkisp1::algorithms {
>>   LOG_DEFINE_CATEGORY(RkISP1Dpf)
>>   
>>   Dpf::Dpf()
>> -       : config_({}), strengthConfig_({})
>> +       : config_({}), strengthConfig_({}),
>> +         runningMode_(controls::draft::NoiseReductionModeOff)
>>   {
>>   }
>>   
>> @@ -46,6 +47,69 @@ Dpf::Dpf()
>>    */
>>   int Dpf::init([[maybe_unused]] IPAContext &context,
>>                const YamlObject &tuningData)
>> +{
>> +       /* Parse tuning block. */
>> +       if (!parseConfig(tuningData)) {
>> +               return -EINVAL;
>> +       }
>
> Single line conditionals do not have { } in our coding style, so this
> can be just
>
> 	if (!parseConfig(tuningData))
> 		return -EINVAL;
>
> Same in all the other locations.
I will format them back as coding style.
>
>
>> +
>> +       return 0;
>> +}
>> +bool Dpf::parseConfig(const YamlObject &tuningData)
>> +{
>> +       /* Parse base config. */
>> +       if (!parseSingleConfig(tuningData, config_, strengthConfig_)) {
>> +               return false;
>> +       }
>> +       if (!parseModes(tuningData)) {
>> +               return false;
>> +       }
>> +       return true;
>> +}
>> +
>> +bool Dpf::parseModes(const YamlObject &tuningData)
>> +{
>> +       /* Parse noise reduction modes. */
>> +       if (!tuningData.contains("modes")) {
>> +               return true;
>> +       }
>> +
>> +       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 false;
>> +               }
>> +
>> +               int32_t modeValue = controls::draft::NoiseReductionModeOff;
>> +               if (*typeOpt == "minimal") {
>> +                       modeValue = controls::draft::NoiseReductionModeMinimal;
>> +               } else if (*typeOpt == "fast") {
>> +                       modeValue = controls::draft::NoiseReductionModeFast;
>> +               } else if (*typeOpt == "highquality") {
>> +                       modeValue = controls::draft::NoiseReductionModeHighQuality;
>> +               } else if (*typeOpt == "zsl") {
>> +                       modeValue = controls::draft::NoiseReductionModeZSL;
>> +               } else {
>> +                       LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt;
>> +                       return false;
>> +               }
>> +
>> +               ModeConfig mode{};
>> +               mode.modeValue = modeValue;
>> +               if (!parseSingleConfig(entry, mode.dpf, mode.strength)) {
>> +                       return false;
>> +               }
>> +               noiseReductionModes_.push_back(mode);
>> +       }
>> +
>> +       return true;
>> +}
>> +bool Dpf::parseSingleConfig(const YamlObject &tuningData,
> There should always be a blank line separating functions.
will update in following patch series
>
>> +                           rkisp1_cif_isp_dpf_config &config,
>> +                           rkisp1_cif_isp_dpf_strength_config &strengthConfig)
>>   {
>>          std::vector<uint8_t> values;
>>   
>> @@ -53,7 +117,11 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>>           * The domain kernel is configured with a 9x9 kernel for the green
>>           * pixels, and a 13x9 or 9x9 kernel for red and blue pixels.
>>           */
>> -       const YamlObject &dFObject = tuningData["DomainFilter"];
>> +       const YamlObject &dFObject = tuningData["filter"];
>> +       if (!dFObject) {
>> +               LOG(RkISP1Dpf, Error) << "filter section missing";
>> +               return false;
>> +       }
>>   
>>          /*
>>           * For the green component, we have the 9x9 kernel specified
>> @@ -75,17 +143,17 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>>          values = dFObject["g"].getList<uint8_t>().value_or(std::vector<uint8_t>{});
>>          if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS) {
>>                  LOG(RkISP1Dpf, Error)
>> -                       << "Invalid 'DomainFilter:g': expected "
>> +                       << "Invalid 'filter:g': expected "
>>                          << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
>>                          << " elements, got " << values.size();
>> -               return -EINVAL;
>> +               return false;
>>          }
>>   
>>          std::copy_n(values.begin(), values.size(),
>> -                   std::begin(config_.g_flt.spatial_coeff));
>> +                   std::begin(config.g_flt.spatial_coeff));
>>   
>> -       config_.g_flt.gr_enable = true;
>> -       config_.g_flt.gb_enable = true;
>> +       config.g_flt.gr_enable = true;
>> +       config.g_flt.gb_enable = true;
>>   
>>          /*
>>           * For the red and blue components, we have the 13x9 kernel specified
>> @@ -112,65 +180,155 @@ int Dpf::init([[maybe_unused]] IPAContext &context,
>>          if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS &&
>>              values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1) {
>>                  LOG(RkISP1Dpf, Error)
>> -                       << "Invalid 'DomainFilter:rb': expected "
>> +                       << "Invalid 'filter:rb': expected "
>>                          << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1
>>                          << " or " << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
>>                          << " elements, got " << values.size();
>> -               return -EINVAL;
>> +               return false;
>>          }
>>   
>> -       config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
>> -                              ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
>> -                              : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
>> +       config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
>> +                                       ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
>> +                                       : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
>>   
>>          std::copy_n(values.begin(), values.size(),
>> -                   std::begin(config_.rb_flt.spatial_coeff));
>> +                   std::begin(config.rb_flt.spatial_coeff));
>>   
>> -       config_.rb_flt.r_enable = true;
>> -       config_.rb_flt.b_enable = true;
>> +       config.rb_flt.r_enable = true;
>> +       config.rb_flt.b_enable = true;
>>   
>>          /*
>>           * The range kernel is configured with a noise level lookup table (NLL)
>>           * which stores a piecewise linear function that characterizes the
> Should this be using the PWL helpers at all ?
> Will check whether can move to PWL helper, if yes will migrate to PWL helper in next series
>
>>           * sensor noise profile as a noise level function curve (NLF).
>>           */
>> -       const YamlObject &rFObject = tuningData["NoiseLevelFunction"];
>> +       const YamlObject &rFObject = tuningData["nll"];
>> +       if (!rFObject) {
>> +               LOG(RkISP1Dpf, Error) << "nll section missing";
>> +               return false;
>> +       }
>>   
>> -       std::vector<uint16_t> nllValues;
>> -       nllValues = rFObject["coeff"].getList<uint16_t>().value_or(std::vector<uint16_t>{});
>> +       const auto nllValues =
>> +               rFObject["coeff"].getList<uint16_t>().value_or(std::vector<uint16_t>{});
> If it's valid to use the Pwl type - then we can potentially use the
> YamlObject::Getter too if it helps?
>
> libipa/pwl.cpp:YamlObject::Getter<ipa::Pwl>::get(const YamlObject &obj) const

Will check whether can move to PWL helper, if yes will migrate to PWL helper in next series

>
>
>>          if (nllValues.size() != RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS) {
>>                  LOG(RkISP1Dpf, Error)
>> -                       << "Invalid 'RangeFilter:coeff': expected "
>> +                       << "Invalid 'nll:coeff': expected "
>>                          << RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS
>>                          << " elements, got " << nllValues.size();
>> -               return -EINVAL;
>> +               return false;
>>          }
>>   
>>          std::copy_n(nllValues.begin(), nllValues.size(),
>> -                   std::begin(config_.nll.coeff));
>> +                   std::begin(config.nll.coeff));
>>   
>> -       std::string scaleMode = rFObject["scale-mode"].get<std::string>("");
>> +       const auto scaleMode = rFObject["scale-mode"].get<std::string>("");
>>          if (scaleMode == "linear") {
>> -               config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;
>> +               config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;
>>          } else if (scaleMode == "logarithmic") {
>> -               config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
>> +               config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
>>          } else {
>>                  LOG(RkISP1Dpf, Error)
>> -                       << "Invalid 'RangeFilter:scale-mode': expected "
>> +                       << "Invalid 'nll:scale-mode': expected "
>>                          << "'linear' or 'logarithmic' value, got "
>>                          << scaleMode;
>> -               return -EINVAL;
>> +               return false;
>>          }
>>   
>> -       const YamlObject &fSObject = tuningData["FilterStrength"];
>> +       const YamlObject &gObject = tuningData["gain"];
>> +       if (!gObject) {
>> +               LOG(RkISP1Dpf, Error) << "gain section missing";
>> +               return false;
>> +       }
>>   
>> -       strengthConfig_.r = fSObject["r"].get<uint16_t>(64);
>> -       strengthConfig_.g = fSObject["g"].get<uint16_t>(64);
>> -       strengthConfig_.b = fSObject["b"].get<uint16_t>(64);
>> +       config.gain.mode =
>> +               gObject["gain_mode"].get<uint32_t>().value_or(
>> +                       RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS);
>> +       config.gain.nf_r_gain = gObject["r"].get<uint16_t>().value_or(256);
>> +       config.gain.nf_b_gain = gObject["b"].get<uint16_t>().value_or(256);
>> +       config.gain.nf_gr_gain = gObject["gr"].get<uint16_t>().value_or(256);
>> +       config.gain.nf_gb_gain = gObject["gb"].get<uint16_t>().value_or(256);
>> +
>> +       const YamlObject &fSObject = tuningData["strength"];
>> +       if (!fSObject) {
>> +               LOG(RkISP1Dpf, Error) << "strength section missing";
>> +               return false;
>> +       }
>>   
>> -       return 0;
>> +       strengthConfig.r = fSObject["r"].get<uint8_t>().value_or(64);
>> +       strengthConfig.g = fSObject["g"].get<uint8_t>().value_or(64);
>> +       strengthConfig.b = fSObject["b"].get<uint8_t>().value_or(64);
>> +       return true;
>> +}
>> +
>> +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 "
>> +                       << static_cast<int>(mode);
>> +               return false;
>> +       }
>> +
>> +       config_ = it->dpf;
>> +       strengthConfig_ = it->strength;
>> +
>> +       LOG(RkISP1Dpf, Info)
>> +               << "DPF mode=Reduction (config loaded)"
>> +               << " mode=" << static_cast<int>(mode);
>> +
>> +       return true;
>>   }
> Blank line required here.
will update in next series
>> +void Dpf::logConfigIfChanged(const IPAFrameContext &frameContext)
>> +{
>> +       if (!frameContext.dpf.update) {
>> +               return;
>> +       }
>> +
>> +       std::ostringstream ss;
>> +
>> +       ss << "DPF config update: ";
>> +       ss << ", control mode=" << static_cast<int>(runningMode_);
>> +       ss << ", denoise=" << (frameContext.dpf.denoise ? "enabled" : "disabled, ");
>> +
>> +       ss << "rb_fltsize="
>> +          << (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? "13x9" : "9x9");
>> +       ss << ", nll_scale="
>> +          << (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? "log" : "linear");
>> +       ss << ", gain_mode=" << config_.gain.mode;
>> +       ss << ", strength=" << int(strengthConfig_.r) << ',' << int(strengthConfig_.g) << ',' << int(strengthConfig_.b);
>> +
>> +       ss << ", g=[";
>> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
>> +               if (i) {
>> +                       ss << ',';
>> +               }
>> +               ss << int(config_.g_flt.spatial_coeff[i]);
>> +       }
>> +       ss << "]";
>>   
>> +       ss << ", rb=[";
>> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
>> +               if (i) {
>> +                       ss << ',';
>> +               }
>> +               ss << int(config_.rb_flt.spatial_coeff[i]);
>> +       }
>> +       ss << "]";
>> +
>> +       ss << ", nll=[";
>> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS; ++i) {
>> +               if (i) {
>> +                       ss << ',';
>> +               }
>> +               ss << int(config_.nll.coeff[i]);
>> +       }
>> +       ss << "]";
>> +       LOG(RkISP1Dpf, Info) << ss.str();
>> +}
>>   /**
>>    * \copydoc libcamera::ipa::Algorithm::queueRequest
>>    */
>> @@ -183,30 +341,34 @@ void Dpf::queueRequest(IPAContext &context,
>>          bool update = false;
>>   
>>          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) {
>> -                               dpf.denoise = false;
>> -                               update = true;
>> -                       }
>> -                       break;
>> -               case controls::draft::NoiseReductionModeMinimal:
>> -               case controls::draft::NoiseReductionModeHighQuality:
>> -               case controls::draft::NoiseReductionModeFast:
>> -                       if (!dpf.denoise) {
>> -                               dpf.denoise = true;
>> -                               update = true;
>> -                       }
>> -                       break;
>> -               default:
>> -                       LOG(RkISP1Dpf, Error)
>> -                               << "Unsupported denoise value "
>> -                               << *denoise;
>> -                       break;
>> +       if (!denoise) {
>> +               return;
>> +       }
>> +       runningMode_ = *denoise;
>> +       switch (runningMode_) {
>> +       case controls::draft::NoiseReductionModeOff:
>> +               if (dpf.denoise) {
>> +                       dpf.denoise = false;
>> +                       update = true;
>> +               }
>> +               break;
>> +       case controls::draft::NoiseReductionModeMinimal:
>> +       case controls::draft::NoiseReductionModeHighQuality:
>> +       case controls::draft::NoiseReductionModeFast:
>> +       case controls::draft::NoiseReductionModeZSL:
>> +               if (loadReductionConfig(runningMode_)) {
>> +                       update = true;
>> +                       dpf.denoise = true;
>> +               } else {
>> +                       dpf.denoise = false;
>> +                       update = true;
>>                  }
>> +               break;
>> +       default:
>> +               LOG(RkISP1Dpf, Error)
>> +                       << "Unsupported denoise value "
>> +                       << *denoise;
>> +               break;
>>          }
>>   
>>          frameContext.dpf.denoise = dpf.denoise;
>> @@ -219,41 +381,66 @@ void Dpf::queueRequest(IPAContext &context,
>>   void Dpf::prepare(IPAContext &context, const uint32_t frame,
>>                    IPAFrameContext &frameContext, RkISP1Params *params)
>>   {
>> -       if (!frameContext.dpf.update && frame > 0)
>> +       if (!frameContext.dpf.update && frame > 0) {
>>                  return;
>> +       }
>> +
>> +       if (!frameContext.dpf.denoise) {
>> +               prepareDisabledMode(context, frame, frameContext, params);
>> +               return;
>> +       }
>> +
>> +       prepareEnabledMode(context, frame, frameContext, params);
>> +}
>> +
>> +void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,
>> +                             [[maybe_unused]] const uint32_t frame,
>> +                             [[maybe_unused]] IPAFrameContext &frameContext,
>> +                             RkISP1Params *params)
>> +{
>> +       frameContext.dpf.denoise = false;
>> +       auto dpfConfig = params->block<BlockType::Dpf>();
>> +       dpfConfig.setEnabled(false);
>> +       auto dpfStrength = params->block<BlockType::DpfStrength>();
>> +       dpfStrength.setEnabled(false);
>> +}
>> +
>> +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>> +                            [[maybe_unused]] IPAFrameContext &frameContext, RkISP1Params *params)
>> +{
>> +       auto dpfConfig = params->block<BlockType::Dpf>();
>> +       dpfConfig.setEnabled(true);
>> +       *dpfConfig = config_;
>>   
>> -       auto config = params->block<BlockType::Dpf>();
>> -       config.setEnabled(frameContext.dpf.denoise);
>> -
>> -       auto strengthConfig = params->block<BlockType::DpfStrength>();
>> -       strengthConfig.setEnabled(frameContext.dpf.denoise);
>> -
>> -       if (frameContext.dpf.denoise) {
>> -               *config = config_;
>> -               *strengthConfig = strengthConfig_;
>> -
>> -               const auto &awb = context.configuration.awb;
>> -               const auto &lsc = context.configuration.lsc;
>> -
>> -               auto &mode = config->gain.mode;
>> -
>> -               /*
>> -                * The DPF needs to take into account the total amount of
>> -                * digital gain, which comes from the AWB and LSC modules. The
>> -                * DPF hardware can be programmed with a digital gain value
>> -                * manually, but can also use the gains supplied by the AWB and
>> -                * LSC modules automatically when they are enabled. Use that
>> -                * mode of operation as it simplifies control of the DPF.
>> -                */
>> -               if (awb.enabled && lsc.enabled)
>> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
>> -               else if (awb.enabled)
>> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
>> -               else if (lsc.enabled)
>> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
>> -               else
>> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
>> +       /*
>> +        * The DPF needs to take into account the total amount of
>> +        * digital gain, which comes from the AWB and LSC modules. The
>> +        * DPF hardware can be programmed with a digital gain value
>> +        * manually, but can also use the gains supplied by the AWB and
>> +        * LSC modules automatically when they are enabled. Use that
>> +        * mode of operation as it simplifies control of the DPF.
>> +        */
>> +       const auto &awb = context.configuration.awb;
>> +       const auto &lsc = context.configuration.lsc;
>> +       auto &mode = dpfConfig->gain.mode;
>> +
>> +       if (awb.enabled && lsc.enabled) {
>> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
>> +       } else if (awb.enabled) {
>> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
>> +       } else if (lsc.enabled) {
>> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
>> +       } else {
>> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
>>          }
>> +       config_.gain.mode = mode;
>> +
>> +       auto dpfStrength = params->block<BlockType::DpfStrength>();
>> +       dpfStrength.setEnabled(true);
>> +
>> +       *dpfStrength = strengthConfig_;
>> +
>> +       logConfigIfChanged(frameContext);
>>   }
>>   
>>   REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
>> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
>> index 2dd8cd36..d4bfb2dc 100644
>> --- a/src/ipa/rkisp1/algorithms/dpf.h
>> +++ b/src/ipa/rkisp1/algorithms/dpf.h
>> @@ -30,8 +30,30 @@ public:
>>                       RkISP1Params *params) override;
>>   
>>   private:
>> +       struct ModeConfig {
>> +               int32_t modeValue;
>> +               rkisp1_cif_isp_dpf_config dpf;
>> +               rkisp1_cif_isp_dpf_strength_config strength;
>> +       };
>> +       bool parseConfig(const YamlObject &tuningData);
>> +       bool parseModes(const YamlObject &tuningData);
>> +       bool parseSingleConfig(const YamlObject &tuningData,
>> +                              rkisp1_cif_isp_dpf_config &config,
>> +                              rkisp1_cif_isp_dpf_strength_config &strengthConfig);
>> +
>> +       bool loadReductionConfig(int32_t mode);
>> +       void logConfigIfChanged(const IPAFrameContext &frameContext);
>> +
>> +       void prepareDisabledMode(IPAContext &context, const uint32_t frame,
>> +                                IPAFrameContext &frameContext,
>> +                                RkISP1Params *params);
>> +       void prepareEnabledMode(IPAContext &context, const uint32_t frame,
>> +                               IPAFrameContext &frameContext, RkISP1Params *params);
>> +
>>          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/Makefile b/Makefile
new file mode 100644
index 00000000..07f8f0fb
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,125 @@ 
+# Support execution in a docker environment
+
+define run-in-docker
+	# CONTAINER=$1
+	# COMMAND=$2
+	# OPTIONS=$3
+	docker run \
+		-v "${PWD}":"${PWD}" \
+		-w "${PWD}" \
+		${3} \
+		--rm -i ${1} ${2}
+
+		#--privileged=true \
+		#--cap-add=SYS_PTRACE \
+		#--security-opt seccomp=unconfined \
+		#-v "${HOME}":"${HOME}" \
+		#-v /var/run/docker.sock:/var/run/docker.sock \
+
+endef
+
+define git-uk-bookworm-arm64-cross
+	$(call run-in-docker,git.uk.ideasonboard.com/camera/containers:debian-bookworm-cross-arm64,$1,$2)
+endef
+
+define bookworm-arm64
+	$(call run-in-docker,debian-bookworm-cross-arm64,$1,$2)
+endef
+
+# Helper function to run clang-tidy with proper setup
+# $1 = git command to list files
+# $2 = output log file
+define run-tidy
+	@echo "Running clang-tidy and saving to $(2)..."
+	@$(1) > /tmp/tidy-files.txt
+	@if [ -s /tmp/tidy-files.txt ]; then \
+		echo "Checking files:"; cat /tmp/tidy-files.txt; \
+		cat /tmp/tidy-files.txt | docker run -v "${PWD}":"${PWD}" -w "${PWD}" --rm -i debian-bookworm-cross-arm64 sh -c "mkdir -p $(RESULTS_DIR) && chmod 777 $(RESULTS_DIR) && xargs -r -I{} clang-tidy {} -p $(BUILD) || true" > $(2) 2>&1 || touch $(2); \
+		echo "Done. Output saved to $(2)"; \
+		python3 utils/log_to_html.py $(2) $(basename $(2)).html || true; \
+	else \
+		echo "No files to check"; \
+		touch $(2); \
+	fi
+	@rm -f /tmp/tidy-files.txt
+endef
+
+BUILD=build/bookworm-arm64
+RESULTS_DIR=$(BUILD)/clang-tidy-results
+
+libcamera: $(BUILD)/build.ninja
+	$(call bookworm-arm64,ninja -C $(BUILD))
+
+# /usr/share/meson/arm64-cross is provided by the debian-bookworm-cross-arm64 container
+configure $(BUILD)/build.ninja:
+	$(call bookworm-arm64,meson setup $(BUILD) \
+		$(RECONFIGURE) \
+		--cross-file /usr/share/meson/arm64-cross \
+		-Dprefix=/usr \
+		-Dpycamera=enabled \
+		-Ddocumentation=disabled \
+		-Dlibdir=/lib/aarch64-linux-gnu \
+		)
+
+reconfigure: RECONFIGURE=--reconfigure
+reconfigure: configure
+
+PYTHON_DIST=usr/lib/python3.11/dist-packages
+PACKAGE_TGZ=libcamera-aarch64.tgz
+.PHONY: package
+package:
+	$(call bookworm-arm64,rm -rf $(BUILD)/install)
+	$(call bookworm-arm64,mkdir -p $(BUILD)/install)
+	$(call bookworm-arm64,ninja -C $(BUILD) install,--env DESTDIR=install)
+	$(call bookworm-arm64,mkdir -p $(BUILD)/install/$(PYTHON_DIST))
+# 	$(call bookworm-arm64,mv \
+# 		$(BUILD)/install/usr/lib/python3/dist-packages/libcamera \
+# 		$(BUILD)/install/$(PYTHON_DIST))
+	tar -C $(BUILD)/install -czf $(PACKAGE_TGZ) ./
+
+.PHONY: tmp_install
+tmp_install:
+	$(call bookworm-arm64,rm -rf $(BUILD)/install)
+	$(call bookworm-arm64,mkdir -p $(BUILD)/install)
+	$(call bookworm-arm64,ninja -C $(BUILD) install,--env DESTDIR=install)
+	$(call bookworm-arm64,mkdir -p $(BUILD)/install/$(PYTHON_DIST))
+	$(call bookworm-arm64,mv \
+		$(BUILD)/install/usr/lib/python3/dist-packages/libcamera \
+		$(BUILD)/install/$(PYTHON_DIST))
+
+# Directly install on a target
+kastor kakip debix-som debix-model-A: libcamera tmp_install
+	rsync --keep-dirlinks -rav --progress build/bookworm-arm64/install/ rui@192.168.31.73:~/libcamera-install/
+
+
+beehive: package
+	scp $(PACKAGE_TGZ) rui@192.168.31.73:~/python3-disp/
+	ssh $@ -tC "cd / && sudo tar --keep-directory-symlink --no-same-owner -vxhf ~/python3-disp/$(PACKAGE_TGZ)"
+rui: package
+	scp $(PACKAGE_TGZ) rui@192.168.31.73:/tmp/
+	ssh rui@192.168.31.73 -tC "cd / && sudo tar --keep-directory-symlink --no-same-owner -vxhf /tmp/$(PACKAGE_TGZ)"
+
+.PHONY: tidy-diff
+tidy-diff:
+	$(call run-tidy,git diff --name-only -- '*.c' '*.cpp' '*.cc' '*.h',$(RESULTS_DIR)/tidy-diff.log)
+
+.PHONY: tidy
+tidy:
+	$(call run-tidy,git ls-files -- '*.c' '*.cpp' '*.cc' '*.h',$(RESULTS_DIR)/tidy.log)
+
+.PHONY: tidy-last-commit
+tidy-last-commit:
+	$(call run-tidy,git diff --name-only HEAD~1 HEAD -- '*.c' '*.cpp' '*.cc' '*.h',$(RESULTS_DIR)/tidy-last-commit.log)
+
+.PHONY: tidy-branch
+tidy-branch:
+	$(call run-tidy,git diff --name-only master...HEAD -- '*.c' '*.cpp' '*.cc' '*.h',$(RESULTS_DIR)/tidy-branch.log)
+
+.PHONY: tidy-staged
+tidy-staged:
+	$(call run-tidy,git diff --name-only --cached -- '*.c' '*.cpp' '*.cc' '*.h',$(RESULTS_DIR)/tidy-staged.log)
+
+.PHONY: build-and-check-branch
+build-and-check-branch:
+	git diff master...HEAD
+
diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
index 39f3e461..7e2da7b1 100644
--- a/src/ipa/rkisp1/algorithms/dpf.cpp
+++ b/src/ipa/rkisp1/algorithms/dpf.cpp
@@ -37,7 +37,8 @@  namespace ipa::rkisp1::algorithms {
 LOG_DEFINE_CATEGORY(RkISP1Dpf)
 
 Dpf::Dpf()
-	: config_({}), strengthConfig_({})
+	: config_({}), strengthConfig_({}),
+	  runningMode_(controls::draft::NoiseReductionModeOff)
 {
 }
 
@@ -46,6 +47,69 @@  Dpf::Dpf()
  */
 int Dpf::init([[maybe_unused]] IPAContext &context,
 	      const YamlObject &tuningData)
+{
+	/* Parse tuning block. */
+	if (!parseConfig(tuningData)) {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+bool Dpf::parseConfig(const YamlObject &tuningData)
+{
+	/* Parse base config. */
+	if (!parseSingleConfig(tuningData, config_, strengthConfig_)) {
+		return false;
+	}
+	if (!parseModes(tuningData)) {
+		return false;
+	}
+	return true;
+}
+
+bool Dpf::parseModes(const YamlObject &tuningData)
+{
+	/* Parse noise reduction modes. */
+	if (!tuningData.contains("modes")) {
+		return true;
+	}
+
+	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 false;
+		}
+
+		int32_t modeValue = controls::draft::NoiseReductionModeOff;
+		if (*typeOpt == "minimal") {
+			modeValue = controls::draft::NoiseReductionModeMinimal;
+		} else if (*typeOpt == "fast") {
+			modeValue = controls::draft::NoiseReductionModeFast;
+		} else if (*typeOpt == "highquality") {
+			modeValue = controls::draft::NoiseReductionModeHighQuality;
+		} else if (*typeOpt == "zsl") {
+			modeValue = controls::draft::NoiseReductionModeZSL;
+		} else {
+			LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt;
+			return false;
+		}
+
+		ModeConfig mode{};
+		mode.modeValue = modeValue;
+		if (!parseSingleConfig(entry, mode.dpf, mode.strength)) {
+			return false;
+		}
+		noiseReductionModes_.push_back(mode);
+	}
+
+	return true;
+}
+bool Dpf::parseSingleConfig(const YamlObject &tuningData,
+			    rkisp1_cif_isp_dpf_config &config,
+			    rkisp1_cif_isp_dpf_strength_config &strengthConfig)
 {
 	std::vector<uint8_t> values;
 
@@ -53,7 +117,11 @@  int Dpf::init([[maybe_unused]] IPAContext &context,
 	 * The domain kernel is configured with a 9x9 kernel for the green
 	 * pixels, and a 13x9 or 9x9 kernel for red and blue pixels.
 	 */
-	const YamlObject &dFObject = tuningData["DomainFilter"];
+	const YamlObject &dFObject = tuningData["filter"];
+	if (!dFObject) {
+		LOG(RkISP1Dpf, Error) << "filter section missing";
+		return false;
+	}
 
 	/*
 	 * For the green component, we have the 9x9 kernel specified
@@ -75,17 +143,17 @@  int Dpf::init([[maybe_unused]] IPAContext &context,
 	values = dFObject["g"].getList<uint8_t>().value_or(std::vector<uint8_t>{});
 	if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS) {
 		LOG(RkISP1Dpf, Error)
-			<< "Invalid 'DomainFilter:g': expected "
+			<< "Invalid 'filter:g': expected "
 			<< RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
 			<< " elements, got " << values.size();
-		return -EINVAL;
+		return false;
 	}
 
 	std::copy_n(values.begin(), values.size(),
-		    std::begin(config_.g_flt.spatial_coeff));
+		    std::begin(config.g_flt.spatial_coeff));
 
-	config_.g_flt.gr_enable = true;
-	config_.g_flt.gb_enable = true;
+	config.g_flt.gr_enable = true;
+	config.g_flt.gb_enable = true;
 
 	/*
 	 * For the red and blue components, we have the 13x9 kernel specified
@@ -112,65 +180,155 @@  int Dpf::init([[maybe_unused]] IPAContext &context,
 	if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS &&
 	    values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1) {
 		LOG(RkISP1Dpf, Error)
-			<< "Invalid 'DomainFilter:rb': expected "
+			<< "Invalid 'filter:rb': expected "
 			<< RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1
 			<< " or " << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
 			<< " elements, got " << values.size();
-		return -EINVAL;
+		return false;
 	}
 
-	config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
-			       ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
-			       : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
+	config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS
+					? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9
+					: RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;
 
 	std::copy_n(values.begin(), values.size(),
-		    std::begin(config_.rb_flt.spatial_coeff));
+		    std::begin(config.rb_flt.spatial_coeff));
 
-	config_.rb_flt.r_enable = true;
-	config_.rb_flt.b_enable = true;
+	config.rb_flt.r_enable = true;
+	config.rb_flt.b_enable = true;
 
 	/*
 	 * The range kernel is configured with a noise level lookup table (NLL)
 	 * which stores a piecewise linear function that characterizes the
 	 * sensor noise profile as a noise level function curve (NLF).
 	 */
-	const YamlObject &rFObject = tuningData["NoiseLevelFunction"];
+	const YamlObject &rFObject = tuningData["nll"];
+	if (!rFObject) {
+		LOG(RkISP1Dpf, Error) << "nll section missing";
+		return false;
+	}
 
-	std::vector<uint16_t> nllValues;
-	nllValues = rFObject["coeff"].getList<uint16_t>().value_or(std::vector<uint16_t>{});
+	const auto nllValues =
+		rFObject["coeff"].getList<uint16_t>().value_or(std::vector<uint16_t>{});
 	if (nllValues.size() != RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS) {
 		LOG(RkISP1Dpf, Error)
-			<< "Invalid 'RangeFilter:coeff': expected "
+			<< "Invalid 'nll:coeff': expected "
 			<< RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS
 			<< " elements, got " << nllValues.size();
-		return -EINVAL;
+		return false;
 	}
 
 	std::copy_n(nllValues.begin(), nllValues.size(),
-		    std::begin(config_.nll.coeff));
+		    std::begin(config.nll.coeff));
 
-	std::string scaleMode = rFObject["scale-mode"].get<std::string>("");
+	const auto scaleMode = rFObject["scale-mode"].get<std::string>("");
 	if (scaleMode == "linear") {
-		config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;
+		config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;
 	} else if (scaleMode == "logarithmic") {
-		config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
+		config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;
 	} else {
 		LOG(RkISP1Dpf, Error)
-			<< "Invalid 'RangeFilter:scale-mode': expected "
+			<< "Invalid 'nll:scale-mode': expected "
 			<< "'linear' or 'logarithmic' value, got "
 			<< scaleMode;
-		return -EINVAL;
+		return false;
 	}
 
-	const YamlObject &fSObject = tuningData["FilterStrength"];
+	const YamlObject &gObject = tuningData["gain"];
+	if (!gObject) {
+		LOG(RkISP1Dpf, Error) << "gain section missing";
+		return false;
+	}
 
-	strengthConfig_.r = fSObject["r"].get<uint16_t>(64);
-	strengthConfig_.g = fSObject["g"].get<uint16_t>(64);
-	strengthConfig_.b = fSObject["b"].get<uint16_t>(64);
+	config.gain.mode =
+		gObject["gain_mode"].get<uint32_t>().value_or(
+			RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS);
+	config.gain.nf_r_gain = gObject["r"].get<uint16_t>().value_or(256);
+	config.gain.nf_b_gain = gObject["b"].get<uint16_t>().value_or(256);
+	config.gain.nf_gr_gain = gObject["gr"].get<uint16_t>().value_or(256);
+	config.gain.nf_gb_gain = gObject["gb"].get<uint16_t>().value_or(256);
+
+	const YamlObject &fSObject = tuningData["strength"];
+	if (!fSObject) {
+		LOG(RkISP1Dpf, Error) << "strength section missing";
+		return false;
+	}
 
-	return 0;
+	strengthConfig.r = fSObject["r"].get<uint8_t>().value_or(64);
+	strengthConfig.g = fSObject["g"].get<uint8_t>().value_or(64);
+	strengthConfig.b = fSObject["b"].get<uint8_t>().value_or(64);
+	return true;
+}
+
+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 "
+			<< static_cast<int>(mode);
+		return false;
+	}
+
+	config_ = it->dpf;
+	strengthConfig_ = it->strength;
+
+	LOG(RkISP1Dpf, Info)
+		<< "DPF mode=Reduction (config loaded)"
+		<< " mode=" << static_cast<int>(mode);
+
+	return true;
 }
+void Dpf::logConfigIfChanged(const IPAFrameContext &frameContext)
+{
+	if (!frameContext.dpf.update) {
+		return;
+	}
+
+	std::ostringstream ss;
+
+	ss << "DPF config update: ";
+	ss << ", control mode=" << static_cast<int>(runningMode_);
+	ss << ", denoise=" << (frameContext.dpf.denoise ? "enabled" : "disabled, ");
+
+	ss << "rb_fltsize="
+	   << (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? "13x9" : "9x9");
+	ss << ", nll_scale="
+	   << (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? "log" : "linear");
+	ss << ", gain_mode=" << config_.gain.mode;
+	ss << ", strength=" << int(strengthConfig_.r) << ',' << int(strengthConfig_.g) << ',' << int(strengthConfig_.b);
+
+	ss << ", g=[";
+	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
+		if (i) {
+			ss << ',';
+		}
+		ss << int(config_.g_flt.spatial_coeff[i]);
+	}
+	ss << "]";
 
+	ss << ", rb=[";
+	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {
+		if (i) {
+			ss << ',';
+		}
+		ss << int(config_.rb_flt.spatial_coeff[i]);
+	}
+	ss << "]";
+
+	ss << ", nll=[";
+	for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS; ++i) {
+		if (i) {
+			ss << ',';
+		}
+		ss << int(config_.nll.coeff[i]);
+	}
+	ss << "]";
+	LOG(RkISP1Dpf, Info) << ss.str();
+}
 /**
  * \copydoc libcamera::ipa::Algorithm::queueRequest
  */
@@ -183,30 +341,34 @@  void Dpf::queueRequest(IPAContext &context,
 	bool update = false;
 
 	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) {
-				dpf.denoise = false;
-				update = true;
-			}
-			break;
-		case controls::draft::NoiseReductionModeMinimal:
-		case controls::draft::NoiseReductionModeHighQuality:
-		case controls::draft::NoiseReductionModeFast:
-			if (!dpf.denoise) {
-				dpf.denoise = true;
-				update = true;
-			}
-			break;
-		default:
-			LOG(RkISP1Dpf, Error)
-				<< "Unsupported denoise value "
-				<< *denoise;
-			break;
+	if (!denoise) {
+		return;
+	}
+	runningMode_ = *denoise;
+	switch (runningMode_) {
+	case controls::draft::NoiseReductionModeOff:
+		if (dpf.denoise) {
+			dpf.denoise = false;
+			update = true;
+		}
+		break;
+	case controls::draft::NoiseReductionModeMinimal:
+	case controls::draft::NoiseReductionModeHighQuality:
+	case controls::draft::NoiseReductionModeFast:
+	case controls::draft::NoiseReductionModeZSL:
+		if (loadReductionConfig(runningMode_)) {
+			update = true;
+			dpf.denoise = true;
+		} else {
+			dpf.denoise = false;
+			update = true;
 		}
+		break;
+	default:
+		LOG(RkISP1Dpf, Error)
+			<< "Unsupported denoise value "
+			<< *denoise;
+		break;
 	}
 
 	frameContext.dpf.denoise = dpf.denoise;
@@ -219,41 +381,66 @@  void Dpf::queueRequest(IPAContext &context,
 void Dpf::prepare(IPAContext &context, const uint32_t frame,
 		  IPAFrameContext &frameContext, RkISP1Params *params)
 {
-	if (!frameContext.dpf.update && frame > 0)
+	if (!frameContext.dpf.update && frame > 0) {
 		return;
+	}
+
+	if (!frameContext.dpf.denoise) {
+		prepareDisabledMode(context, frame, frameContext, params);
+		return;
+	}
+
+	prepareEnabledMode(context, frame, frameContext, params);
+}
+
+void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,
+			      [[maybe_unused]] const uint32_t frame,
+			      [[maybe_unused]] IPAFrameContext &frameContext,
+			      RkISP1Params *params)
+{
+	frameContext.dpf.denoise = false;
+	auto dpfConfig = params->block<BlockType::Dpf>();
+	dpfConfig.setEnabled(false);
+	auto dpfStrength = params->block<BlockType::DpfStrength>();
+	dpfStrength.setEnabled(false);
+}
+
+void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,
+			     [[maybe_unused]] IPAFrameContext &frameContext, RkISP1Params *params)
+{
+	auto dpfConfig = params->block<BlockType::Dpf>();
+	dpfConfig.setEnabled(true);
+	*dpfConfig = config_;
 
-	auto config = params->block<BlockType::Dpf>();
-	config.setEnabled(frameContext.dpf.denoise);
-
-	auto strengthConfig = params->block<BlockType::DpfStrength>();
-	strengthConfig.setEnabled(frameContext.dpf.denoise);
-
-	if (frameContext.dpf.denoise) {
-		*config = config_;
-		*strengthConfig = strengthConfig_;
-
-		const auto &awb = context.configuration.awb;
-		const auto &lsc = context.configuration.lsc;
-
-		auto &mode = config->gain.mode;
-
-		/*
-		 * The DPF needs to take into account the total amount of
-		 * digital gain, which comes from the AWB and LSC modules. The
-		 * DPF hardware can be programmed with a digital gain value
-		 * manually, but can also use the gains supplied by the AWB and
-		 * LSC modules automatically when they are enabled. Use that
-		 * mode of operation as it simplifies control of the DPF.
-		 */
-		if (awb.enabled && lsc.enabled)
-			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
-		else if (awb.enabled)
-			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
-		else if (lsc.enabled)
-			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
-		else
-			mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
+	/*
+	 * The DPF needs to take into account the total amount of
+	 * digital gain, which comes from the AWB and LSC modules. The
+	 * DPF hardware can be programmed with a digital gain value
+	 * manually, but can also use the gains supplied by the AWB and
+	 * LSC modules automatically when they are enabled. Use that
+	 * mode of operation as it simplifies control of the DPF.
+	 */
+	const auto &awb = context.configuration.awb;
+	const auto &lsc = context.configuration.lsc;
+	auto &mode = dpfConfig->gain.mode;
+
+	if (awb.enabled && lsc.enabled) {
+		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;
+	} else if (awb.enabled) {
+		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;
+	} else if (lsc.enabled) {
+		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
+	} else {
+		mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
 	}
+	config_.gain.mode = mode;
+
+	auto dpfStrength = params->block<BlockType::DpfStrength>();
+	dpfStrength.setEnabled(true);
+
+	*dpfStrength = strengthConfig_;
+
+	logConfigIfChanged(frameContext);
 }
 
 REGISTER_IPA_ALGORITHM(Dpf, "Dpf")
diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
index 2dd8cd36..d4bfb2dc 100644
--- a/src/ipa/rkisp1/algorithms/dpf.h
+++ b/src/ipa/rkisp1/algorithms/dpf.h
@@ -30,8 +30,30 @@  public:
 		     RkISP1Params *params) override;
 
 private:
+	struct ModeConfig {
+		int32_t modeValue;
+		rkisp1_cif_isp_dpf_config dpf;
+		rkisp1_cif_isp_dpf_strength_config strength;
+	};
+	bool parseConfig(const YamlObject &tuningData);
+	bool parseModes(const YamlObject &tuningData);
+	bool parseSingleConfig(const YamlObject &tuningData,
+			       rkisp1_cif_isp_dpf_config &config,
+			       rkisp1_cif_isp_dpf_strength_config &strengthConfig);
+
+	bool loadReductionConfig(int32_t mode);
+	void logConfigIfChanged(const IPAFrameContext &frameContext);
+
+	void prepareDisabledMode(IPAContext &context, const uint32_t frame,
+				 IPAFrameContext &frameContext,
+				 RkISP1Params *params);
+	void prepareEnabledMode(IPAContext &context, const uint32_t frame,
+				IPAFrameContext &frameContext, RkISP1Params *params);
+
 	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 */