[{"id":37219,"web_url":"https://patchwork.libcamera.org/comment/37219/","msgid":"<176505524011.710079.18294885782413601787@ping.linuxembedded.co.uk>","date":"2025-12-06T21:07:20","subject":"Re: [PATCH v3 1/2] ipa/rkisp1: refactory DPF parsing and\n\tinitialization","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Rui,\n\nIn $SUBJECT s#ipa/rkisp1: refactory#ipa: rkisp1: refactor#\n\nV2, then v3 came through in quick succession. What's the changelog\nbetween them ?\n\nWhat was v1 in this series? I'm afraid it's getting hard to track so\nmany submissions with inconsistent versions and no changelogs.\n\nIs this still a continuation of\nhttps://patchwork.libcamera.org/project/libcamera/list/?series=5612 ?\n\nOr is it a whole rewrite with a new direction?\n\nQuoting Rui Wang (2025-12-04 21:37:43)\n> Split DPF configuration parsing and initialization into clearer,\n> self-contained helpers and modernized initialization patterns.\n> \n\nThis following block of text is very terse. Please at least put a blank\nline in between paragraphs to make it easier to read, but it seems to be\na sign that we're doing too much in a single patch. \n\n> Add parseConfig, parseModes and loadReductionConfig to factor\n> parsing logic and mode lookups.\n> Introduce ModeConfig and store mode entries in\n> noiseReductionModes_ to decouple parsing from runtime selection.\n\n\n> Move sentinel member initializers into the constructor initializer\n> list (runningMode_) and declare vectors\n> without in-class initializers for default construction.\n> Replace ad-hoc string handling with value_or and const auto\n> where appropriate for clearer and safer parsing.\n\n> Replace domain/range/strength YAML keys mapping and error returns\n> (use filter, nll, strength keys and return false from parse\n> helpers instead of -EINVAL).\n\n\nIf you rename tuning file keys - that's essentially a tuning file API\nbreakage, so the commit message should be /really/ clear on this update.\n\nThe commit message up here could or rather should at least be declaring\nthat the following tuning file keys have been renamed:\n\n  - DomainFilter -> filter\n  - NoiseLevelFunction -> nll\n  - FilterStrength -> gain\n  - ... ?\n\nI think the renames themselves could be an independent patch so they can\nbe considered alone, before making each one defined per mode.\n\n> Add loadReductionConfig to centralize loading of DPF configs for\n> reduction modes and preserve logging and failure behavior.\n> Adjust queueRequest, prepare, and helpers to use the new\n> parsing/initialization flow while preserving existing behavior.\n\n> This refactor improves separation of concerns, makes parsing easier\n> to maintain, and reduces duplicated logic. No functional behaviour is\n> intended to be changed.\n\nBut - doesn't it have lots of functional behaviour changes? The entire\ntuning file layout has now changed as far as I can tell.\n\nAnd that now they are subkeys and can be defined explicitly for each of\nthe control modes that can be selected.\n\nIt seems that pulling the parameters out to be per-control mode option\nis actually the main purpose of the patch - but nothing above really\ntells me that?\n\n> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>\n> \n> ---\n> changelog :\n>  1. fix config issue and some format\n> ---\n>  Makefile                          | 125 +++++++++++\n>  src/ipa/rkisp1/algorithms/dpf.cpp | 359 +++++++++++++++++++++++-------\n>  src/ipa/rkisp1/algorithms/dpf.h   |  22 ++\n>  3 files changed, 420 insertions(+), 86 deletions(-)\n>  create mode 100644 Makefile\n> \n> diff --git a/Makefile b/Makefile\n> new file mode 100644\n> index 00000000..07f8f0fb\n> --- /dev/null\n> +++ b/Makefile\n> @@ -0,0 +1,125 @@\n> +# Support execution in a docker environment\n\nPlease remove this file addition from the patch.\n\nThis has been attached to multiple submissions now, please be careful to\nreview your patches yourself before posting to make sure there are no\nunexpected attachments or incorrect squashes.\n\n<snip Makefile>\n\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> index 39f3e461..7e2da7b1 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> @@ -37,7 +37,8 @@ namespace ipa::rkisp1::algorithms {\n>  LOG_DEFINE_CATEGORY(RkISP1Dpf)\n>  \n>  Dpf::Dpf()\n> -       : config_({}), strengthConfig_({})\n> +       : config_({}), strengthConfig_({}),\n> +         runningMode_(controls::draft::NoiseReductionModeOff)\n>  {\n>  }\n>  \n> @@ -46,6 +47,69 @@ Dpf::Dpf()\n>   */\n>  int Dpf::init([[maybe_unused]] IPAContext &context,\n>               const YamlObject &tuningData)\n> +{\n> +       /* Parse tuning block. */\n> +       if (!parseConfig(tuningData)) {\n> +               return -EINVAL;\n> +       }\n\n\nSingle line conditionals do not have { } in our coding style, so this\ncan be just \n\n\tif (!parseConfig(tuningData))\n\t\treturn -EINVAL;\n\nSame in all the other locations.\n\n\n> +\n> +       return 0;\n> +}\n> +bool Dpf::parseConfig(const YamlObject &tuningData)\n> +{\n> +       /* Parse base config. */\n> +       if (!parseSingleConfig(tuningData, config_, strengthConfig_)) {\n> +               return false;\n> +       }\n> +       if (!parseModes(tuningData)) {\n> +               return false;\n> +       }\n> +       return true;\n> +}\n> +\n> +bool Dpf::parseModes(const YamlObject &tuningData)\n> +{\n> +       /* Parse noise reduction modes. */\n> +       if (!tuningData.contains(\"modes\")) {\n> +               return true;\n> +       }\n> +\n> +       noiseReductionModes_.clear();\n> +       for (const auto &entry : tuningData[\"modes\"].asList()) {\n> +               std::optional<std::string> typeOpt =\n> +                       entry[\"type\"].get<std::string>();\n> +               if (!typeOpt) {\n> +                       LOG(RkISP1Dpf, Error) << \"Modes entry missing type\";\n> +                       return false;\n> +               }\n> +\n> +               int32_t modeValue = controls::draft::NoiseReductionModeOff;\n> +               if (*typeOpt == \"minimal\") {\n> +                       modeValue = controls::draft::NoiseReductionModeMinimal;\n> +               } else if (*typeOpt == \"fast\") {\n> +                       modeValue = controls::draft::NoiseReductionModeFast;\n> +               } else if (*typeOpt == \"highquality\") {\n> +                       modeValue = controls::draft::NoiseReductionModeHighQuality;\n> +               } else if (*typeOpt == \"zsl\") {\n> +                       modeValue = controls::draft::NoiseReductionModeZSL;\n> +               } else {\n> +                       LOG(RkISP1Dpf, Error) << \"Unknown mode type: \" << *typeOpt;\n> +                       return false;\n> +               }\n> +\n> +               ModeConfig mode{};\n> +               mode.modeValue = modeValue;\n> +               if (!parseSingleConfig(entry, mode.dpf, mode.strength)) {\n> +                       return false;\n> +               }\n> +               noiseReductionModes_.push_back(mode);\n> +       }\n> +\n> +       return true;\n> +}\n> +bool Dpf::parseSingleConfig(const YamlObject &tuningData,\n\nThere should always be a blank line separating functions.\n\n> +                           rkisp1_cif_isp_dpf_config &config,\n> +                           rkisp1_cif_isp_dpf_strength_config &strengthConfig)\n>  {\n>         std::vector<uint8_t> values;\n>  \n> @@ -53,7 +117,11 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n>          * The domain kernel is configured with a 9x9 kernel for the green\n>          * pixels, and a 13x9 or 9x9 kernel for red and blue pixels.\n>          */\n> -       const YamlObject &dFObject = tuningData[\"DomainFilter\"];\n> +       const YamlObject &dFObject = tuningData[\"filter\"];\n> +       if (!dFObject) {\n> +               LOG(RkISP1Dpf, Error) << \"filter section missing\";\n> +               return false;\n> +       }\n>  \n>         /*\n>          * For the green component, we have the 9x9 kernel specified\n> @@ -75,17 +143,17 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n>         values = dFObject[\"g\"].getList<uint8_t>().value_or(std::vector<uint8_t>{});\n>         if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS) {\n>                 LOG(RkISP1Dpf, Error)\n> -                       << \"Invalid 'DomainFilter:g': expected \"\n> +                       << \"Invalid 'filter:g': expected \"\n>                         << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n>                         << \" elements, got \" << values.size();\n> -               return -EINVAL;\n> +               return false;\n>         }\n>  \n>         std::copy_n(values.begin(), values.size(),\n> -                   std::begin(config_.g_flt.spatial_coeff));\n> +                   std::begin(config.g_flt.spatial_coeff));\n>  \n> -       config_.g_flt.gr_enable = true;\n> -       config_.g_flt.gb_enable = true;\n> +       config.g_flt.gr_enable = true;\n> +       config.g_flt.gb_enable = true;\n>  \n>         /*\n>          * For the red and blue components, we have the 13x9 kernel specified\n> @@ -112,65 +180,155 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n>         if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS &&\n>             values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1) {\n>                 LOG(RkISP1Dpf, Error)\n> -                       << \"Invalid 'DomainFilter:rb': expected \"\n> +                       << \"Invalid 'filter:rb': expected \"\n>                         << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1\n>                         << \" or \" << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n>                         << \" elements, got \" << values.size();\n> -               return -EINVAL;\n> +               return false;\n>         }\n>  \n> -       config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n> -                              ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9\n> -                              : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;\n> +       config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n> +                                       ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9\n> +                                       : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;\n>  \n>         std::copy_n(values.begin(), values.size(),\n> -                   std::begin(config_.rb_flt.spatial_coeff));\n> +                   std::begin(config.rb_flt.spatial_coeff));\n>  \n> -       config_.rb_flt.r_enable = true;\n> -       config_.rb_flt.b_enable = true;\n> +       config.rb_flt.r_enable = true;\n> +       config.rb_flt.b_enable = true;\n>  \n>         /*\n>          * The range kernel is configured with a noise level lookup table (NLL)\n>          * which stores a piecewise linear function that characterizes the\n\nShould this be using the PWL helpers at all ?\n\n\n>          * sensor noise profile as a noise level function curve (NLF).\n>          */\n> -       const YamlObject &rFObject = tuningData[\"NoiseLevelFunction\"];\n> +       const YamlObject &rFObject = tuningData[\"nll\"];\n> +       if (!rFObject) {\n> +               LOG(RkISP1Dpf, Error) << \"nll section missing\";\n> +               return false;\n> +       }\n>  \n> -       std::vector<uint16_t> nllValues;\n> -       nllValues = rFObject[\"coeff\"].getList<uint16_t>().value_or(std::vector<uint16_t>{});\n> +       const auto nllValues =\n> +               rFObject[\"coeff\"].getList<uint16_t>().value_or(std::vector<uint16_t>{});\n\nIf it's valid to use the Pwl type - then we can potentially use the\nYamlObject::Getter too if it helps?\n\nlibipa/pwl.cpp:YamlObject::Getter<ipa::Pwl>::get(const YamlObject &obj) const\n\n\n>         if (nllValues.size() != RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS) {\n>                 LOG(RkISP1Dpf, Error)\n> -                       << \"Invalid 'RangeFilter:coeff': expected \"\n> +                       << \"Invalid 'nll:coeff': expected \"\n>                         << RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS\n>                         << \" elements, got \" << nllValues.size();\n> -               return -EINVAL;\n> +               return false;\n>         }\n>  \n>         std::copy_n(nllValues.begin(), nllValues.size(),\n> -                   std::begin(config_.nll.coeff));\n> +                   std::begin(config.nll.coeff));\n>  \n> -       std::string scaleMode = rFObject[\"scale-mode\"].get<std::string>(\"\");\n> +       const auto scaleMode = rFObject[\"scale-mode\"].get<std::string>(\"\");\n>         if (scaleMode == \"linear\") {\n> -               config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;\n> +               config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;\n>         } else if (scaleMode == \"logarithmic\") {\n> -               config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;\n> +               config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;\n>         } else {\n>                 LOG(RkISP1Dpf, Error)\n> -                       << \"Invalid 'RangeFilter:scale-mode': expected \"\n> +                       << \"Invalid 'nll:scale-mode': expected \"\n>                         << \"'linear' or 'logarithmic' value, got \"\n>                         << scaleMode;\n> -               return -EINVAL;\n> +               return false;\n>         }\n>  \n> -       const YamlObject &fSObject = tuningData[\"FilterStrength\"];\n> +       const YamlObject &gObject = tuningData[\"gain\"];\n> +       if (!gObject) {\n> +               LOG(RkISP1Dpf, Error) << \"gain section missing\";\n> +               return false;\n> +       }\n>  \n> -       strengthConfig_.r = fSObject[\"r\"].get<uint16_t>(64);\n> -       strengthConfig_.g = fSObject[\"g\"].get<uint16_t>(64);\n> -       strengthConfig_.b = fSObject[\"b\"].get<uint16_t>(64);\n> +       config.gain.mode =\n> +               gObject[\"gain_mode\"].get<uint32_t>().value_or(\n> +                       RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS);\n> +       config.gain.nf_r_gain = gObject[\"r\"].get<uint16_t>().value_or(256);\n> +       config.gain.nf_b_gain = gObject[\"b\"].get<uint16_t>().value_or(256);\n> +       config.gain.nf_gr_gain = gObject[\"gr\"].get<uint16_t>().value_or(256);\n> +       config.gain.nf_gb_gain = gObject[\"gb\"].get<uint16_t>().value_or(256);\n> +\n> +       const YamlObject &fSObject = tuningData[\"strength\"];\n> +       if (!fSObject) {\n> +               LOG(RkISP1Dpf, Error) << \"strength section missing\";\n> +               return false;\n> +       }\n>  \n> -       return 0;\n> +       strengthConfig.r = fSObject[\"r\"].get<uint8_t>().value_or(64);\n> +       strengthConfig.g = fSObject[\"g\"].get<uint8_t>().value_or(64);\n> +       strengthConfig.b = fSObject[\"b\"].get<uint8_t>().value_or(64);\n> +       return true;\n> +}\n> +\n> +bool Dpf::loadReductionConfig(int32_t mode)\n> +{\n> +       auto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(),\n> +                              [mode](const ModeConfig &m) {\n> +                                      return m.modeValue == mode;\n> +                              });\n> +       if (it == noiseReductionModes_.end()) {\n> +               LOG(RkISP1Dpf, Warning)\n> +                       << \"No DPF config for reduction mode \"\n> +                       << static_cast<int>(mode);\n> +               return false;\n> +       }\n> +\n> +       config_ = it->dpf;\n> +       strengthConfig_ = it->strength;\n> +\n> +       LOG(RkISP1Dpf, Info)\n> +               << \"DPF mode=Reduction (config loaded)\"\n> +               << \" mode=\" << static_cast<int>(mode);\n> +\n> +       return true;\n>  }\n\nBlank line required here.\n\n> +void Dpf::logConfigIfChanged(const IPAFrameContext &frameContext)\n> +{\n> +       if (!frameContext.dpf.update) {\n> +               return;\n> +       }\n> +\n> +       std::ostringstream ss;\n> +\n> +       ss << \"DPF config update: \";\n> +       ss << \", control mode=\" << static_cast<int>(runningMode_);\n> +       ss << \", denoise=\" << (frameContext.dpf.denoise ? \"enabled\" : \"disabled, \");\n> +\n> +       ss << \"rb_fltsize=\"\n> +          << (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? \"13x9\" : \"9x9\");\n> +       ss << \", nll_scale=\"\n> +          << (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? \"log\" : \"linear\");\n> +       ss << \", gain_mode=\" << config_.gain.mode;\n> +       ss << \", strength=\" << int(strengthConfig_.r) << ',' << int(strengthConfig_.g) << ',' << int(strengthConfig_.b);\n> +\n> +       ss << \", g=[\";\n> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {\n> +               if (i) {\n> +                       ss << ',';\n> +               }\n> +               ss << int(config_.g_flt.spatial_coeff[i]);\n> +       }\n> +       ss << \"]\";\n>  \n> +       ss << \", rb=[\";\n> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {\n> +               if (i) {\n> +                       ss << ',';\n> +               }\n> +               ss << int(config_.rb_flt.spatial_coeff[i]);\n> +       }\n> +       ss << \"]\";\n> +\n> +       ss << \", nll=[\";\n> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS; ++i) {\n> +               if (i) {\n> +                       ss << ',';\n> +               }\n> +               ss << int(config_.nll.coeff[i]);\n> +       }\n> +       ss << \"]\";\n> +       LOG(RkISP1Dpf, Info) << ss.str();\n> +}\n>  /**\n>   * \\copydoc libcamera::ipa::Algorithm::queueRequest\n>   */\n> @@ -183,30 +341,34 @@ void Dpf::queueRequest(IPAContext &context,\n>         bool update = false;\n>  \n>         const auto &denoise = controls.get(controls::draft::NoiseReductionMode);\n> -       if (denoise) {\n> -               LOG(RkISP1Dpf, Debug) << \"Set denoise to \" << *denoise;\n> -\n> -               switch (*denoise) {\n> -               case controls::draft::NoiseReductionModeOff:\n> -                       if (dpf.denoise) {\n> -                               dpf.denoise = false;\n> -                               update = true;\n> -                       }\n> -                       break;\n> -               case controls::draft::NoiseReductionModeMinimal:\n> -               case controls::draft::NoiseReductionModeHighQuality:\n> -               case controls::draft::NoiseReductionModeFast:\n> -                       if (!dpf.denoise) {\n> -                               dpf.denoise = true;\n> -                               update = true;\n> -                       }\n> -                       break;\n> -               default:\n> -                       LOG(RkISP1Dpf, Error)\n> -                               << \"Unsupported denoise value \"\n> -                               << *denoise;\n> -                       break;\n> +       if (!denoise) {\n> +               return;\n> +       }\n> +       runningMode_ = *denoise;\n> +       switch (runningMode_) {\n> +       case controls::draft::NoiseReductionModeOff:\n> +               if (dpf.denoise) {\n> +                       dpf.denoise = false;\n> +                       update = true;\n> +               }\n> +               break;\n> +       case controls::draft::NoiseReductionModeMinimal:\n> +       case controls::draft::NoiseReductionModeHighQuality:\n> +       case controls::draft::NoiseReductionModeFast:\n> +       case controls::draft::NoiseReductionModeZSL:\n> +               if (loadReductionConfig(runningMode_)) {\n> +                       update = true;\n> +                       dpf.denoise = true;\n> +               } else {\n> +                       dpf.denoise = false;\n> +                       update = true;\n>                 }\n> +               break;\n> +       default:\n> +               LOG(RkISP1Dpf, Error)\n> +                       << \"Unsupported denoise value \"\n> +                       << *denoise;\n> +               break;\n>         }\n>  \n>         frameContext.dpf.denoise = dpf.denoise;\n> @@ -219,41 +381,66 @@ void Dpf::queueRequest(IPAContext &context,\n>  void Dpf::prepare(IPAContext &context, const uint32_t frame,\n>                   IPAFrameContext &frameContext, RkISP1Params *params)\n>  {\n> -       if (!frameContext.dpf.update && frame > 0)\n> +       if (!frameContext.dpf.update && frame > 0) {\n>                 return;\n> +       }\n> +\n> +       if (!frameContext.dpf.denoise) {\n> +               prepareDisabledMode(context, frame, frameContext, params);\n> +               return;\n> +       }\n> +\n> +       prepareEnabledMode(context, frame, frameContext, params);\n> +}\n> +\n> +void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,\n> +                             [[maybe_unused]] const uint32_t frame,\n> +                             [[maybe_unused]] IPAFrameContext &frameContext,\n> +                             RkISP1Params *params)\n> +{\n> +       frameContext.dpf.denoise = false;\n> +       auto dpfConfig = params->block<BlockType::Dpf>();\n> +       dpfConfig.setEnabled(false);\n> +       auto dpfStrength = params->block<BlockType::DpfStrength>();\n> +       dpfStrength.setEnabled(false);\n> +}\n> +\n> +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> +                            [[maybe_unused]] IPAFrameContext &frameContext, RkISP1Params *params)\n> +{\n> +       auto dpfConfig = params->block<BlockType::Dpf>();\n> +       dpfConfig.setEnabled(true);\n> +       *dpfConfig = config_;\n>  \n> -       auto config = params->block<BlockType::Dpf>();\n> -       config.setEnabled(frameContext.dpf.denoise);\n> -\n> -       auto strengthConfig = params->block<BlockType::DpfStrength>();\n> -       strengthConfig.setEnabled(frameContext.dpf.denoise);\n> -\n> -       if (frameContext.dpf.denoise) {\n> -               *config = config_;\n> -               *strengthConfig = strengthConfig_;\n> -\n> -               const auto &awb = context.configuration.awb;\n> -               const auto &lsc = context.configuration.lsc;\n> -\n> -               auto &mode = config->gain.mode;\n> -\n> -               /*\n> -                * The DPF needs to take into account the total amount of\n> -                * digital gain, which comes from the AWB and LSC modules. The\n> -                * DPF hardware can be programmed with a digital gain value\n> -                * manually, but can also use the gains supplied by the AWB and\n> -                * LSC modules automatically when they are enabled. Use that\n> -                * mode of operation as it simplifies control of the DPF.\n> -                */\n> -               if (awb.enabled && lsc.enabled)\n> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;\n> -               else if (awb.enabled)\n> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;\n> -               else if (lsc.enabled)\n> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n> -               else\n> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n> +       /*\n> +        * The DPF needs to take into account the total amount of\n> +        * digital gain, which comes from the AWB and LSC modules. The\n> +        * DPF hardware can be programmed with a digital gain value\n> +        * manually, but can also use the gains supplied by the AWB and\n> +        * LSC modules automatically when they are enabled. Use that\n> +        * mode of operation as it simplifies control of the DPF.\n> +        */\n> +       const auto &awb = context.configuration.awb;\n> +       const auto &lsc = context.configuration.lsc;\n> +       auto &mode = dpfConfig->gain.mode;\n> +\n> +       if (awb.enabled && lsc.enabled) {\n> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;\n> +       } else if (awb.enabled) {\n> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;\n> +       } else if (lsc.enabled) {\n> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n> +       } else {\n> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n>         }\n> +       config_.gain.mode = mode;\n> +\n> +       auto dpfStrength = params->block<BlockType::DpfStrength>();\n> +       dpfStrength.setEnabled(true);\n> +\n> +       *dpfStrength = strengthConfig_;\n> +\n> +       logConfigIfChanged(frameContext);\n>  }\n>  \n>  REGISTER_IPA_ALGORITHM(Dpf, \"Dpf\")\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h\n> index 2dd8cd36..d4bfb2dc 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.h\n> +++ b/src/ipa/rkisp1/algorithms/dpf.h\n> @@ -30,8 +30,30 @@ public:\n>                      RkISP1Params *params) override;\n>  \n>  private:\n> +       struct ModeConfig {\n> +               int32_t modeValue;\n> +               rkisp1_cif_isp_dpf_config dpf;\n> +               rkisp1_cif_isp_dpf_strength_config strength;\n> +       };\n> +       bool parseConfig(const YamlObject &tuningData);\n> +       bool parseModes(const YamlObject &tuningData);\n> +       bool parseSingleConfig(const YamlObject &tuningData,\n> +                              rkisp1_cif_isp_dpf_config &config,\n> +                              rkisp1_cif_isp_dpf_strength_config &strengthConfig);\n> +\n> +       bool loadReductionConfig(int32_t mode);\n> +       void logConfigIfChanged(const IPAFrameContext &frameContext);\n> +\n> +       void prepareDisabledMode(IPAContext &context, const uint32_t frame,\n> +                                IPAFrameContext &frameContext,\n> +                                RkISP1Params *params);\n> +       void prepareEnabledMode(IPAContext &context, const uint32_t frame,\n> +                               IPAFrameContext &frameContext, RkISP1Params *params);\n> +\n>         struct rkisp1_cif_isp_dpf_config config_;\n>         struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;\n> +       std::vector<ModeConfig> noiseReductionModes_;\n> +       int32_t runningMode_;\n>  };\n>  \n>  } /* namespace ipa::rkisp1::algorithms */\n> -- \n> 2.43.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C24F9BDDFC\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat,  6 Dec 2025 21:07:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDC74613E4;\n\tSat,  6 Dec 2025 22:07:25 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A63776069A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  6 Dec 2025 22:07:24 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC030C73;\n\tSat,  6 Dec 2025 22:05:06 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Uws9l1T2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765055107;\n\tbh=Mp+gAE0Wo65cglh0T+h3VCtshiKicq6pYj2xzqCQRuo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Uws9l1T2t4rVEJ5cEq5LsdIL7F+79uXpwwdukA4e8sENYx+krSwUQTf1e2IlOkr+/\n\tB/0r6scSpkE+BYzzr9V9Fo7nalytaJnwDyXpz3JJJjcI9fQBTqO+01nH2teGOYMjcc\n\txuCYTraclD/jPRIO8PIzAUyO4sQPp/XllmyHpxb0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251204213744.1110922-2-rui.wang@ideasonboard.com>","References":"<20251204213744.1110922-1-rui.wang@ideasonboard.com>\n\t<20251204213744.1110922-2-rui.wang@ideasonboard.com>","Subject":"Re: [PATCH v3 1/2] ipa/rkisp1: refactory DPF parsing and\n\tinitialization","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Rui Wang <rui.wang@ideasonboard.com>","To":"Rui Wang <rui.wang@ideasonboard.com>, libcamera-devel@lists.libcamera.org","Date":"Sat, 06 Dec 2025 21:07:20 +0000","Message-ID":"<176505524011.710079.18294885782413601787@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":37220,"web_url":"https://patchwork.libcamera.org/comment/37220/","msgid":"<73e96320-332f-412b-809c-5c155cf3a9a8@ideasonboard.com>","date":"2025-12-07T02:34:18","subject":"Re: [PATCH v3 1/2] ipa/rkisp1: refactory DPF parsing and\n\tinitialization","submitter":{"id":241,"url":"https://patchwork.libcamera.org/api/people/241/","name":"Rui Wang","email":"rui.wang@ideasonboard.com"},"content":"On 2025-12-06 16:07, Kieran Bingham wrote:\n> Hi Rui,\n>\n> In $SUBJECT s#ipa/rkisp1: refactory#ipa: rkisp1: refactor#\n>\n> V2, then v3 came through in quick succession. What's the changelog\n> between them ?\n>\n> What was v1 in this series? I'm afraid it's getting hard to track so\n> many submissions with inconsistent versions and no changelogs.\n>\n> Is this still a continuation of\n> https://patchwork.libcamera.org/project/libcamera/list/?series=5612 ?\n>\n> Or is it a whole rewrite with a new direction?\n\nYes , this patch series are the following of the 5612 , as discussed \nwith Jacopo,  he also suggested\n\nto split \nhttps://patchwork.libcamera.org/project/libcamera/list/?series=5612 ?\n\nso I created this patch series , just refactor the dpf as first part.\nthen will apply auto mode and manual mode as part II and part II\n\nQuoting Rui Wang (2025-12-04 21:37:43)\n\n>> Split DPF configuration parsing and initialization into clearer,\n>> self-contained helpers and modernized initialization patterns.\n>>\n> This following block of text is very terse. Please at least put a blank\n> line in between paragraphs to make it easier to read, but it seems to be\n> a sign that we're doing too much in a single patch.\nthanks , I will rewrite the commit message with more clear change \ninformation.\n>\n>> Add parseConfig, parseModes and loadReductionConfig to factor\n>> parsing logic and mode lookups.\n>> Introduce ModeConfig and store mode entries in\n>> noiseReductionModes_ to decouple parsing from runtime selection.\n>\n>> Move sentinel member initializers into the constructor initializer\n>> list (runningMode_) and declare vectors\n>> without in-class initializers for default construction.\n>> Replace ad-hoc string handling with value_or and const auto\n>> where appropriate for clearer and safer parsing.\n>> Replace domain/range/strength YAML keys mapping and error returns\n>> (use filter, nll, strength keys and return false from parse\n>> helpers instead of -EINVAL).\n>\n> If you rename tuning file keys - that's essentially a tuning file API\n> breakage, so the commit message should be /really/ clear on this update.\n>\n> The commit message up here could or rather should at least be declaring\n> that the following tuning file keys have been renamed:\n>\n>    - DomainFilter -> filter\n>    - NoiseLevelFunction -> nll\n>    - FilterStrength -> gain\n>    - ... ?\n>\n> I think the renames themselves could be an independent patch so they can\n> be considered alone, before making each one defined per mode.\n\nin next series, I will split single one patch into 5 patches\n\n  1. Refactor dpf  yaml parser implementation into parsSingleConfig() ,  \nand introduce parseConfig into init()\n\n  2.rename yaml dpf key name :\n\n   - DomainFilter -> filter\n   - NoiseLevelFunction -> nll\n   - FilterStrength -> gain\n   - etc\n\n  3.Refactor dpf prepare logic , split into enable/disable, and add some \ndebug information\n\n  4.Add reduction mode parser funntion  for different \nmode(highquality/zsl/fast..) and log information\n\n  5.Add dpf config into rkisp/imx219.yaml\n\n>\n>> Add loadReductionConfig to centralize loading of DPF configs for\n>> reduction modes and preserve logging and failure behavior.\n>> Adjust queueRequest, prepare, and helpers to use the new\n>> parsing/initialization flow while preserving existing behavior.\n>> This refactor improves separation of concerns, makes parsing easier\n>> to maintain, and reduces duplicated logic. No functional behaviour is\n>> intended to be changed.\n> But - doesn't it have lots of functional behaviour changes? The entire\n> tuning file layout has now changed as far as I can tell.\n  I will add one patch in following series to highlight refactory code \nand new logic added in commit message\n>\n> And that now they are subkeys and can be defined explicitly for each of\n> the control modes that can be selected.\n>\n> It seems that pulling the parameters out to be per-control mode option\n> is actually the main purpose of the patch - but nothing above really\n> tells me that?\n>\n>> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com>\n>>\n>> ---\n>> changelog :\n>>   1. fix config issue and some format\n>> ---\n>>   Makefile                          | 125 +++++++++++\n>>   src/ipa/rkisp1/algorithms/dpf.cpp | 359 +++++++++++++++++++++++-------\n>>   src/ipa/rkisp1/algorithms/dpf.h   |  22 ++\n>>   3 files changed, 420 insertions(+), 86 deletions(-)\n>>   create mode 100644 Makefile\n>>\n>> diff --git a/Makefile b/Makefile\n>> new file mode 100644\n>> index 00000000..07f8f0fb\n>> --- /dev/null\n>> +++ b/Makefile\n>> @@ -0,0 +1,125 @@\n>> +# Support execution in a docker environment\n> Please remove this file addition from the patch.\n>\n> This has been attached to multiple submissions now, please be careful to\n> review your patches yourself before posting to make sure there are no\n> unexpected attachments or incorrect squashes.\n>\n> <snip Makefile>\nI will do git format cautiously and check all patch files.\n>\n>> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n>> index 39f3e461..7e2da7b1 100644\n>> --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n>> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n>> @@ -37,7 +37,8 @@ namespace ipa::rkisp1::algorithms {\n>>   LOG_DEFINE_CATEGORY(RkISP1Dpf)\n>>   \n>>   Dpf::Dpf()\n>> -       : config_({}), strengthConfig_({})\n>> +       : config_({}), strengthConfig_({}),\n>> +         runningMode_(controls::draft::NoiseReductionModeOff)\n>>   {\n>>   }\n>>   \n>> @@ -46,6 +47,69 @@ Dpf::Dpf()\n>>    */\n>>   int Dpf::init([[maybe_unused]] IPAContext &context,\n>>                const YamlObject &tuningData)\n>> +{\n>> +       /* Parse tuning block. */\n>> +       if (!parseConfig(tuningData)) {\n>> +               return -EINVAL;\n>> +       }\n>\n> Single line conditionals do not have { } in our coding style, so this\n> can be just\n>\n> \tif (!parseConfig(tuningData))\n> \t\treturn -EINVAL;\n>\n> Same in all the other locations.\nI will format them back as coding style.\n>\n>\n>> +\n>> +       return 0;\n>> +}\n>> +bool Dpf::parseConfig(const YamlObject &tuningData)\n>> +{\n>> +       /* Parse base config. */\n>> +       if (!parseSingleConfig(tuningData, config_, strengthConfig_)) {\n>> +               return false;\n>> +       }\n>> +       if (!parseModes(tuningData)) {\n>> +               return false;\n>> +       }\n>> +       return true;\n>> +}\n>> +\n>> +bool Dpf::parseModes(const YamlObject &tuningData)\n>> +{\n>> +       /* Parse noise reduction modes. */\n>> +       if (!tuningData.contains(\"modes\")) {\n>> +               return true;\n>> +       }\n>> +\n>> +       noiseReductionModes_.clear();\n>> +       for (const auto &entry : tuningData[\"modes\"].asList()) {\n>> +               std::optional<std::string> typeOpt =\n>> +                       entry[\"type\"].get<std::string>();\n>> +               if (!typeOpt) {\n>> +                       LOG(RkISP1Dpf, Error) << \"Modes entry missing type\";\n>> +                       return false;\n>> +               }\n>> +\n>> +               int32_t modeValue = controls::draft::NoiseReductionModeOff;\n>> +               if (*typeOpt == \"minimal\") {\n>> +                       modeValue = controls::draft::NoiseReductionModeMinimal;\n>> +               } else if (*typeOpt == \"fast\") {\n>> +                       modeValue = controls::draft::NoiseReductionModeFast;\n>> +               } else if (*typeOpt == \"highquality\") {\n>> +                       modeValue = controls::draft::NoiseReductionModeHighQuality;\n>> +               } else if (*typeOpt == \"zsl\") {\n>> +                       modeValue = controls::draft::NoiseReductionModeZSL;\n>> +               } else {\n>> +                       LOG(RkISP1Dpf, Error) << \"Unknown mode type: \" << *typeOpt;\n>> +                       return false;\n>> +               }\n>> +\n>> +               ModeConfig mode{};\n>> +               mode.modeValue = modeValue;\n>> +               if (!parseSingleConfig(entry, mode.dpf, mode.strength)) {\n>> +                       return false;\n>> +               }\n>> +               noiseReductionModes_.push_back(mode);\n>> +       }\n>> +\n>> +       return true;\n>> +}\n>> +bool Dpf::parseSingleConfig(const YamlObject &tuningData,\n> There should always be a blank line separating functions.\nwill update in following patch series\n>\n>> +                           rkisp1_cif_isp_dpf_config &config,\n>> +                           rkisp1_cif_isp_dpf_strength_config &strengthConfig)\n>>   {\n>>          std::vector<uint8_t> values;\n>>   \n>> @@ -53,7 +117,11 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n>>           * The domain kernel is configured with a 9x9 kernel for the green\n>>           * pixels, and a 13x9 or 9x9 kernel for red and blue pixels.\n>>           */\n>> -       const YamlObject &dFObject = tuningData[\"DomainFilter\"];\n>> +       const YamlObject &dFObject = tuningData[\"filter\"];\n>> +       if (!dFObject) {\n>> +               LOG(RkISP1Dpf, Error) << \"filter section missing\";\n>> +               return false;\n>> +       }\n>>   \n>>          /*\n>>           * For the green component, we have the 9x9 kernel specified\n>> @@ -75,17 +143,17 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n>>          values = dFObject[\"g\"].getList<uint8_t>().value_or(std::vector<uint8_t>{});\n>>          if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS) {\n>>                  LOG(RkISP1Dpf, Error)\n>> -                       << \"Invalid 'DomainFilter:g': expected \"\n>> +                       << \"Invalid 'filter:g': expected \"\n>>                          << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n>>                          << \" elements, got \" << values.size();\n>> -               return -EINVAL;\n>> +               return false;\n>>          }\n>>   \n>>          std::copy_n(values.begin(), values.size(),\n>> -                   std::begin(config_.g_flt.spatial_coeff));\n>> +                   std::begin(config.g_flt.spatial_coeff));\n>>   \n>> -       config_.g_flt.gr_enable = true;\n>> -       config_.g_flt.gb_enable = true;\n>> +       config.g_flt.gr_enable = true;\n>> +       config.g_flt.gb_enable = true;\n>>   \n>>          /*\n>>           * For the red and blue components, we have the 13x9 kernel specified\n>> @@ -112,65 +180,155 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n>>          if (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS &&\n>>              values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1) {\n>>                  LOG(RkISP1Dpf, Error)\n>> -                       << \"Invalid 'DomainFilter:rb': expected \"\n>> +                       << \"Invalid 'filter:rb': expected \"\n>>                          << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1\n>>                          << \" or \" << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n>>                          << \" elements, got \" << values.size();\n>> -               return -EINVAL;\n>> +               return false;\n>>          }\n>>   \n>> -       config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n>> -                              ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9\n>> -                              : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;\n>> +       config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n>> +                                       ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9\n>> +                                       : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;\n>>   \n>>          std::copy_n(values.begin(), values.size(),\n>> -                   std::begin(config_.rb_flt.spatial_coeff));\n>> +                   std::begin(config.rb_flt.spatial_coeff));\n>>   \n>> -       config_.rb_flt.r_enable = true;\n>> -       config_.rb_flt.b_enable = true;\n>> +       config.rb_flt.r_enable = true;\n>> +       config.rb_flt.b_enable = true;\n>>   \n>>          /*\n>>           * The range kernel is configured with a noise level lookup table (NLL)\n>>           * which stores a piecewise linear function that characterizes the\n> Should this be using the PWL helpers at all ?\n> Will check whether can move to PWL helper, if yes will migrate to PWL helper in next series\n>\n>>           * sensor noise profile as a noise level function curve (NLF).\n>>           */\n>> -       const YamlObject &rFObject = tuningData[\"NoiseLevelFunction\"];\n>> +       const YamlObject &rFObject = tuningData[\"nll\"];\n>> +       if (!rFObject) {\n>> +               LOG(RkISP1Dpf, Error) << \"nll section missing\";\n>> +               return false;\n>> +       }\n>>   \n>> -       std::vector<uint16_t> nllValues;\n>> -       nllValues = rFObject[\"coeff\"].getList<uint16_t>().value_or(std::vector<uint16_t>{});\n>> +       const auto nllValues =\n>> +               rFObject[\"coeff\"].getList<uint16_t>().value_or(std::vector<uint16_t>{});\n> If it's valid to use the Pwl type - then we can potentially use the\n> YamlObject::Getter too if it helps?\n>\n> libipa/pwl.cpp:YamlObject::Getter<ipa::Pwl>::get(const YamlObject &obj) const\n\nWill check whether can move to PWL helper, if yes will migrate to PWL helper in next series\n\n>\n>\n>>          if (nllValues.size() != RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS) {\n>>                  LOG(RkISP1Dpf, Error)\n>> -                       << \"Invalid 'RangeFilter:coeff': expected \"\n>> +                       << \"Invalid 'nll:coeff': expected \"\n>>                          << RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS\n>>                          << \" elements, got \" << nllValues.size();\n>> -               return -EINVAL;\n>> +               return false;\n>>          }\n>>   \n>>          std::copy_n(nllValues.begin(), nllValues.size(),\n>> -                   std::begin(config_.nll.coeff));\n>> +                   std::begin(config.nll.coeff));\n>>   \n>> -       std::string scaleMode = rFObject[\"scale-mode\"].get<std::string>(\"\");\n>> +       const auto scaleMode = rFObject[\"scale-mode\"].get<std::string>(\"\");\n>>          if (scaleMode == \"linear\") {\n>> -               config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;\n>> +               config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;\n>>          } else if (scaleMode == \"logarithmic\") {\n>> -               config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;\n>> +               config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;\n>>          } else {\n>>                  LOG(RkISP1Dpf, Error)\n>> -                       << \"Invalid 'RangeFilter:scale-mode': expected \"\n>> +                       << \"Invalid 'nll:scale-mode': expected \"\n>>                          << \"'linear' or 'logarithmic' value, got \"\n>>                          << scaleMode;\n>> -               return -EINVAL;\n>> +               return false;\n>>          }\n>>   \n>> -       const YamlObject &fSObject = tuningData[\"FilterStrength\"];\n>> +       const YamlObject &gObject = tuningData[\"gain\"];\n>> +       if (!gObject) {\n>> +               LOG(RkISP1Dpf, Error) << \"gain section missing\";\n>> +               return false;\n>> +       }\n>>   \n>> -       strengthConfig_.r = fSObject[\"r\"].get<uint16_t>(64);\n>> -       strengthConfig_.g = fSObject[\"g\"].get<uint16_t>(64);\n>> -       strengthConfig_.b = fSObject[\"b\"].get<uint16_t>(64);\n>> +       config.gain.mode =\n>> +               gObject[\"gain_mode\"].get<uint32_t>().value_or(\n>> +                       RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS);\n>> +       config.gain.nf_r_gain = gObject[\"r\"].get<uint16_t>().value_or(256);\n>> +       config.gain.nf_b_gain = gObject[\"b\"].get<uint16_t>().value_or(256);\n>> +       config.gain.nf_gr_gain = gObject[\"gr\"].get<uint16_t>().value_or(256);\n>> +       config.gain.nf_gb_gain = gObject[\"gb\"].get<uint16_t>().value_or(256);\n>> +\n>> +       const YamlObject &fSObject = tuningData[\"strength\"];\n>> +       if (!fSObject) {\n>> +               LOG(RkISP1Dpf, Error) << \"strength section missing\";\n>> +               return false;\n>> +       }\n>>   \n>> -       return 0;\n>> +       strengthConfig.r = fSObject[\"r\"].get<uint8_t>().value_or(64);\n>> +       strengthConfig.g = fSObject[\"g\"].get<uint8_t>().value_or(64);\n>> +       strengthConfig.b = fSObject[\"b\"].get<uint8_t>().value_or(64);\n>> +       return true;\n>> +}\n>> +\n>> +bool Dpf::loadReductionConfig(int32_t mode)\n>> +{\n>> +       auto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(),\n>> +                              [mode](const ModeConfig &m) {\n>> +                                      return m.modeValue == mode;\n>> +                              });\n>> +       if (it == noiseReductionModes_.end()) {\n>> +               LOG(RkISP1Dpf, Warning)\n>> +                       << \"No DPF config for reduction mode \"\n>> +                       << static_cast<int>(mode);\n>> +               return false;\n>> +       }\n>> +\n>> +       config_ = it->dpf;\n>> +       strengthConfig_ = it->strength;\n>> +\n>> +       LOG(RkISP1Dpf, Info)\n>> +               << \"DPF mode=Reduction (config loaded)\"\n>> +               << \" mode=\" << static_cast<int>(mode);\n>> +\n>> +       return true;\n>>   }\n> Blank line required here.\nwill update in next series\n>> +void Dpf::logConfigIfChanged(const IPAFrameContext &frameContext)\n>> +{\n>> +       if (!frameContext.dpf.update) {\n>> +               return;\n>> +       }\n>> +\n>> +       std::ostringstream ss;\n>> +\n>> +       ss << \"DPF config update: \";\n>> +       ss << \", control mode=\" << static_cast<int>(runningMode_);\n>> +       ss << \", denoise=\" << (frameContext.dpf.denoise ? \"enabled\" : \"disabled, \");\n>> +\n>> +       ss << \"rb_fltsize=\"\n>> +          << (config_.rb_flt.fltsize == RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 ? \"13x9\" : \"9x9\");\n>> +       ss << \", nll_scale=\"\n>> +          << (config_.nll.scale_mode == RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC ? \"log\" : \"linear\");\n>> +       ss << \", gain_mode=\" << config_.gain.mode;\n>> +       ss << \", strength=\" << int(strengthConfig_.r) << ',' << int(strengthConfig_.g) << ',' << int(strengthConfig_.b);\n>> +\n>> +       ss << \", g=[\";\n>> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {\n>> +               if (i) {\n>> +                       ss << ',';\n>> +               }\n>> +               ss << int(config_.g_flt.spatial_coeff[i]);\n>> +       }\n>> +       ss << \"]\";\n>>   \n>> +       ss << \", rb=[\";\n>> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS; ++i) {\n>> +               if (i) {\n>> +                       ss << ',';\n>> +               }\n>> +               ss << int(config_.rb_flt.spatial_coeff[i]);\n>> +       }\n>> +       ss << \"]\";\n>> +\n>> +       ss << \", nll=[\";\n>> +       for (size_t i = 0; i < RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS; ++i) {\n>> +               if (i) {\n>> +                       ss << ',';\n>> +               }\n>> +               ss << int(config_.nll.coeff[i]);\n>> +       }\n>> +       ss << \"]\";\n>> +       LOG(RkISP1Dpf, Info) << ss.str();\n>> +}\n>>   /**\n>>    * \\copydoc libcamera::ipa::Algorithm::queueRequest\n>>    */\n>> @@ -183,30 +341,34 @@ void Dpf::queueRequest(IPAContext &context,\n>>          bool update = false;\n>>   \n>>          const auto &denoise = controls.get(controls::draft::NoiseReductionMode);\n>> -       if (denoise) {\n>> -               LOG(RkISP1Dpf, Debug) << \"Set denoise to \" << *denoise;\n>> -\n>> -               switch (*denoise) {\n>> -               case controls::draft::NoiseReductionModeOff:\n>> -                       if (dpf.denoise) {\n>> -                               dpf.denoise = false;\n>> -                               update = true;\n>> -                       }\n>> -                       break;\n>> -               case controls::draft::NoiseReductionModeMinimal:\n>> -               case controls::draft::NoiseReductionModeHighQuality:\n>> -               case controls::draft::NoiseReductionModeFast:\n>> -                       if (!dpf.denoise) {\n>> -                               dpf.denoise = true;\n>> -                               update = true;\n>> -                       }\n>> -                       break;\n>> -               default:\n>> -                       LOG(RkISP1Dpf, Error)\n>> -                               << \"Unsupported denoise value \"\n>> -                               << *denoise;\n>> -                       break;\n>> +       if (!denoise) {\n>> +               return;\n>> +       }\n>> +       runningMode_ = *denoise;\n>> +       switch (runningMode_) {\n>> +       case controls::draft::NoiseReductionModeOff:\n>> +               if (dpf.denoise) {\n>> +                       dpf.denoise = false;\n>> +                       update = true;\n>> +               }\n>> +               break;\n>> +       case controls::draft::NoiseReductionModeMinimal:\n>> +       case controls::draft::NoiseReductionModeHighQuality:\n>> +       case controls::draft::NoiseReductionModeFast:\n>> +       case controls::draft::NoiseReductionModeZSL:\n>> +               if (loadReductionConfig(runningMode_)) {\n>> +                       update = true;\n>> +                       dpf.denoise = true;\n>> +               } else {\n>> +                       dpf.denoise = false;\n>> +                       update = true;\n>>                  }\n>> +               break;\n>> +       default:\n>> +               LOG(RkISP1Dpf, Error)\n>> +                       << \"Unsupported denoise value \"\n>> +                       << *denoise;\n>> +               break;\n>>          }\n>>   \n>>          frameContext.dpf.denoise = dpf.denoise;\n>> @@ -219,41 +381,66 @@ void Dpf::queueRequest(IPAContext &context,\n>>   void Dpf::prepare(IPAContext &context, const uint32_t frame,\n>>                    IPAFrameContext &frameContext, RkISP1Params *params)\n>>   {\n>> -       if (!frameContext.dpf.update && frame > 0)\n>> +       if (!frameContext.dpf.update && frame > 0) {\n>>                  return;\n>> +       }\n>> +\n>> +       if (!frameContext.dpf.denoise) {\n>> +               prepareDisabledMode(context, frame, frameContext, params);\n>> +               return;\n>> +       }\n>> +\n>> +       prepareEnabledMode(context, frame, frameContext, params);\n>> +}\n>> +\n>> +void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,\n>> +                             [[maybe_unused]] const uint32_t frame,\n>> +                             [[maybe_unused]] IPAFrameContext &frameContext,\n>> +                             RkISP1Params *params)\n>> +{\n>> +       frameContext.dpf.denoise = false;\n>> +       auto dpfConfig = params->block<BlockType::Dpf>();\n>> +       dpfConfig.setEnabled(false);\n>> +       auto dpfStrength = params->block<BlockType::DpfStrength>();\n>> +       dpfStrength.setEnabled(false);\n>> +}\n>> +\n>> +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>> +                            [[maybe_unused]] IPAFrameContext &frameContext, RkISP1Params *params)\n>> +{\n>> +       auto dpfConfig = params->block<BlockType::Dpf>();\n>> +       dpfConfig.setEnabled(true);\n>> +       *dpfConfig = config_;\n>>   \n>> -       auto config = params->block<BlockType::Dpf>();\n>> -       config.setEnabled(frameContext.dpf.denoise);\n>> -\n>> -       auto strengthConfig = params->block<BlockType::DpfStrength>();\n>> -       strengthConfig.setEnabled(frameContext.dpf.denoise);\n>> -\n>> -       if (frameContext.dpf.denoise) {\n>> -               *config = config_;\n>> -               *strengthConfig = strengthConfig_;\n>> -\n>> -               const auto &awb = context.configuration.awb;\n>> -               const auto &lsc = context.configuration.lsc;\n>> -\n>> -               auto &mode = config->gain.mode;\n>> -\n>> -               /*\n>> -                * The DPF needs to take into account the total amount of\n>> -                * digital gain, which comes from the AWB and LSC modules. The\n>> -                * DPF hardware can be programmed with a digital gain value\n>> -                * manually, but can also use the gains supplied by the AWB and\n>> -                * LSC modules automatically when they are enabled. Use that\n>> -                * mode of operation as it simplifies control of the DPF.\n>> -                */\n>> -               if (awb.enabled && lsc.enabled)\n>> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;\n>> -               else if (awb.enabled)\n>> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;\n>> -               else if (lsc.enabled)\n>> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n>> -               else\n>> -                       mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n>> +       /*\n>> +        * The DPF needs to take into account the total amount of\n>> +        * digital gain, which comes from the AWB and LSC modules. The\n>> +        * DPF hardware can be programmed with a digital gain value\n>> +        * manually, but can also use the gains supplied by the AWB and\n>> +        * LSC modules automatically when they are enabled. Use that\n>> +        * mode of operation as it simplifies control of the DPF.\n>> +        */\n>> +       const auto &awb = context.configuration.awb;\n>> +       const auto &lsc = context.configuration.lsc;\n>> +       auto &mode = dpfConfig->gain.mode;\n>> +\n>> +       if (awb.enabled && lsc.enabled) {\n>> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;\n>> +       } else if (awb.enabled) {\n>> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;\n>> +       } else if (lsc.enabled) {\n>> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n>> +       } else {\n>> +               mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n>>          }\n>> +       config_.gain.mode = mode;\n>> +\n>> +       auto dpfStrength = params->block<BlockType::DpfStrength>();\n>> +       dpfStrength.setEnabled(true);\n>> +\n>> +       *dpfStrength = strengthConfig_;\n>> +\n>> +       logConfigIfChanged(frameContext);\n>>   }\n>>   \n>>   REGISTER_IPA_ALGORITHM(Dpf, \"Dpf\")\n>> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h\n>> index 2dd8cd36..d4bfb2dc 100644\n>> --- a/src/ipa/rkisp1/algorithms/dpf.h\n>> +++ b/src/ipa/rkisp1/algorithms/dpf.h\n>> @@ -30,8 +30,30 @@ public:\n>>                       RkISP1Params *params) override;\n>>   \n>>   private:\n>> +       struct ModeConfig {\n>> +               int32_t modeValue;\n>> +               rkisp1_cif_isp_dpf_config dpf;\n>> +               rkisp1_cif_isp_dpf_strength_config strength;\n>> +       };\n>> +       bool parseConfig(const YamlObject &tuningData);\n>> +       bool parseModes(const YamlObject &tuningData);\n>> +       bool parseSingleConfig(const YamlObject &tuningData,\n>> +                              rkisp1_cif_isp_dpf_config &config,\n>> +                              rkisp1_cif_isp_dpf_strength_config &strengthConfig);\n>> +\n>> +       bool loadReductionConfig(int32_t mode);\n>> +       void logConfigIfChanged(const IPAFrameContext &frameContext);\n>> +\n>> +       void prepareDisabledMode(IPAContext &context, const uint32_t frame,\n>> +                                IPAFrameContext &frameContext,\n>> +                                RkISP1Params *params);\n>> +       void prepareEnabledMode(IPAContext &context, const uint32_t frame,\n>> +                               IPAFrameContext &frameContext, RkISP1Params *params);\n>> +\n>>          struct rkisp1_cif_isp_dpf_config config_;\n>>          struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;\n>> +       std::vector<ModeConfig> noiseReductionModes_;\n>> +       int32_t runningMode_;\n>>   };\n>>   \n>>   } /* namespace ipa::rkisp1::algorithms */\n>> -- \n>> 2.43.0\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E0481C3257\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  7 Dec 2025 02:34:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C4B99613E5;\n\tSun,  7 Dec 2025 03:34:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 705ED610A6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  7 Dec 2025 03:34:34 +0100 (CET)","from [192.168.31.114] (unknown [209.216.103.65])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7794A1781;\n\tSun,  7 Dec 2025 03:32:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ivNQ31Oi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765074736;\n\tbh=/r5Vn8jWO+i9seA7iNJjUmwQNs445UFRjUHlUWAE5LA=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=ivNQ31OiOFYHGbAHOc3u2/zRM4q07kudhx4dw8ALn9HWmc317mxNv5sSFDH0b/ovE\n\tJqBvn/pF2fsimu75PMpqbgS71ES+8BlpWOc+7cdfp1dcQlOGHZq0EoNt3901BcOeqX\n\tNckW0jzYKA58BSuu+f3CKTDoQcTE1bxz1icUHslM=","Message-ID":"<73e96320-332f-412b-809c-5c155cf3a9a8@ideasonboard.com>","Date":"Sat, 6 Dec 2025 21:34:18 -0500","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 1/2] ipa/rkisp1: refactory DPF parsing and\n\tinitialization","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251204213744.1110922-1-rui.wang@ideasonboard.com>\n\t<20251204213744.1110922-2-rui.wang@ideasonboard.com>\n\t<176505524011.710079.18294885782413601787@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"rui wang <rui.wang@ideasonboard.com>","In-Reply-To":"<176505524011.710079.18294885782413601787@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]