[{"id":37687,"web_url":"https://patchwork.libcamera.org/comment/37687/","msgid":"","date":"2026-01-16T09:42:54","subject":"Re: [PATCH v8 3/7] ipa: rkisp1: algorithms: dpf: Refactor prepare()\n\tinto helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Rui\n + Stefan\n\nOn Thu, Jan 15, 2026 at 11:33:14AM -0500, Rui Wang wrote:\n> Split the prepare() function into prepareDisabledMode() and\n> prepareEnabledMode() to improve readability and maintainability.\n>\n> This separates the logic for disabling and enabling the DPF, making\n> the main prepare() function cleaner and easier to follow.\n>\n> Signed-off-by: Rui Wang \n> Reviewed-by: Jacopo Mondi \n>\n> ---\n> changelog since v5i/v6: Nothing changed\n>\n> Reviewed-by tags from v5 are carried over (no code changes).\n> ---\n> src/ipa/rkisp1/algorithms/dpf.cpp | 80 ++++++++++++++++++++-----------\n> src/ipa/rkisp1/algorithms/dpf.h | 7 +++\n> 2 files changed, 58 insertions(+), 29 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> index 9b663831..09f2bcbd 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> @@ -316,38 +316,60 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> \tif (!frameContext.dpf.update && frame > 0)\n> \t\treturn;\n\nWith the new tuning file layout there is no default mode anymore, so\nthe algorithm starts with Dpf disabled by default and it is only\nenabled once the user selects one modes at queueRequest() time.\n\nIs my understanding correct ?\n\nQuestion for Stefan and other ones who has been developing more\nalgorithms for RkISP1: is this aligned with how other algorithms work\n? In other word, there is no way to enable the algo from tuning file\nbut only with Controls. I think it is fine, just checking.\n\n>\n> +\tif (!frameContext.dpf.denoise) {\n> +\t\tprepareDisabledMode(context, frame, frameContext, params);\n> +\t\treturn;\n> +\t}\n> +\n> +\tprepareEnabledMode(context, frame, frameContext, params);\n> +}\n> +\n> +void Dpf::prepareDisabledMode([[maybe_unused]] IPAContext &context,\n> +\t\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n\nSo why you pass them to the function if you know know you're not going\nto use them ?\n\n> +\t\t\t RkISP1Params *params)\n> +{\n> +\tauto dpfConfig = params->block();\n> +\tdpfConfig.setEnabled(false);\n> +\tauto dpfStrength = params->block();\n> +\tdpfStrength.setEnabled(false);\n> +}\n> +\n> +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> +\t\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n\nSame question, if you know you're not going to use the function\nparameters, why do you have them here ?\n\n> +\t\t\t RkISP1Params *params)\n> +{\n> \tauto config = params->block();\n> -\tconfig.setEnabled(frameContext.dpf.denoise);\n> +\tconfig.setEnabled(true);\n> +\t*config = config_;\n> +\n> +\tconst auto &awb = context.configuration.awb;\n> +\tconst auto &lsc = context.configuration.lsc;\n> +\n> +\tauto &mode = config->gain.mode;\n> +\n> +\t/*\n> +\t * The DPF needs to take into account the total amount of\n> +\t * digital gain, which comes from the AWB and LSC modules. The\n> +\t * DPF hardware can be programmed with a digital gain value\n> +\t * manually, but can also use the gains supplied by the AWB and\n> +\t * LSC modules automatically when they are enabled. Use that\n> +\t * mode of operation as it simplifies control of the DPF.\n> +\t */\n> +\tif (awb.enabled && lsc.enabled)\n> +\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;\n> +\telse if (awb.enabled)\n> +\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;\n> +\telse if (lsc.enabled)\n> +\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n> +\telse\n> +\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n> +\n> +\tconfig_.gain.mode = mode;\n>\n> \tauto strengthConfig = params->block();\n> -\tstrengthConfig.setEnabled(frameContext.dpf.denoise);\n> -\n> -\tif (frameContext.dpf.denoise) {\n> -\t\t*config = config_;\n> -\t\t*strengthConfig = strengthConfig_;\n> -\n> -\t\tconst auto &awb = context.configuration.awb;\n> -\t\tconst auto &lsc = context.configuration.lsc;\n> -\n> -\t\tauto &mode = config->gain.mode;\n> -\n> -\t\t/*\n> -\t\t * The DPF needs to take into account the total amount of\n> -\t\t * digital gain, which comes from the AWB and LSC modules. The\n> -\t\t * DPF hardware can be programmed with a digital gain value\n> -\t\t * manually, but can also use the gains supplied by the AWB and\n> -\t\t * LSC modules automatically when they are enabled. Use that\n> -\t\t * mode of operation as it simplifies control of the DPF.\n> -\t\t */\n> -\t\tif (awb.enabled && lsc.enabled)\n> -\t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_LSC_GAINS;\n> -\t\telse if (awb.enabled)\n> -\t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_AWB_GAINS;\n> -\t\telse if (lsc.enabled)\n> -\t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n> -\t\telse\n> -\t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n> -\t}\n> +\tstrengthConfig.setEnabled(true);\n> +\t*strengthConfig = strengthConfig_;\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 2a2c7052..fcf121cb 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.h\n> +++ b/src/ipa/rkisp1/algorithms/dpf.h\n> @@ -44,6 +44,13 @@ private:\n>\n> \tbool loadReductionConfig(int32_t mode);\n>\n> +\tvoid prepareDisabledMode(IPAContext &context, const uint32_t frame,\n> +\t\t\t\t IPAFrameContext &frameContext,\n> +\t\t\t\t RkISP1Params *params);\n> +\tvoid prepareEnabledMode(IPAContext &context, const uint32_t frame,\n> +\t\t\t\tIPAFrameContext &frameContext,\n> +\t\t\t\tRkISP1Params *params);\n> +\n> \tstruct rkisp1_cif_isp_dpf_config config_;\n> \tstruct rkisp1_cif_isp_dpf_strength_config strengthConfig_;\n> \tstd::vector noiseReductionModes_;\n> --\n> 2.43.0\n>","headers":{"Return-Path":"","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 6EA76C3226\n\tfor ;\n\tFri, 16 Jan 2026 09:42:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 876A461FBC;\n\tFri, 16 Jan 2026 10:42:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F174A61A35\n\tfor ;\n\tFri, 16 Jan 2026 10:42:56 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:6462:5de2:520d:d7a3:63ca:99e8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BBEC24B3;\n\tFri, 16 Jan 2026 10:42:28 +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=\"QhsOUa92\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768556548;\n\tbh=YJHRabv9TxjqI5q/7WmnGphhg2B8p5a+eiix0XlPzUg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QhsOUa92Zy2ytuRRsX9bibcDDoJV9gWRAqfopqyc3C88Ad7lQM2u3tylY9YW1hikr\n\tLUwMt1+Gwc4hul991BXu2CN8ywF1MFtzH3QZtN7pRACTg2kU3fmBlhJdjOdMd0+Gh7\n\twv8g7occo5h4y2B6nNyE7ubA5oXC/83AFS8VhNYE=","Date":"Fri, 16 Jan 2026 10:42:54 +0100","From":"Jacopo Mondi ","To":"Rui Wang , \n\tStefan Klug ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ","Subject":"Re: [PATCH v8 3/7] ipa: rkisp1: algorithms: dpf: Refactor prepare()\n\tinto helpers","Message-ID":"","References":"<20260115163318.1339354-1-rui.wang@ideasonboard.com>\n\t<20260115163318.1339354-4-rui.wang@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20260115163318.1339354-4-rui.wang@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37693,"web_url":"https://patchwork.libcamera.org/comment/37693/","msgid":"<176857068344.216397.2644393977613535059@localhost>","date":"2026-01-16T13:38:03","subject":"Re: [PATCH v8 3/7] ipa: rkisp1: algorithms: dpf: Refactor prepare()\n\tinto helpers","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Rui, hi Jacopo,\n\nQuoting Jacopo Mondi (2026-01-16 10:42:54)\n> Hi Rui\n> + Stefan\n> \n> On Thu, Jan 15, 2026 at 11:33:14AM -0500, Rui Wang wrote:\n> > Split the prepare() function into prepareDisabledMode() and\n> > prepareEnabledMode() to improve readability and maintainability.\n> >\n> > This separates the logic for disabling and enabling the DPF, making\n> > the main prepare() function cleaner and easier to follow.\n> >\n> > Signed-off-by: Rui Wang \n> > Reviewed-by: Jacopo Mondi \n> >\n> > ---\n> > changelog since v5i/v6: Nothing changed\n> >\n> > Reviewed-by tags from v5 are carried over (no code changes).\n> > ---\n> > src/ipa/rkisp1/algorithms/dpf.cpp | 80 ++++++++++++++++++++-----------\n> > src/ipa/rkisp1/algorithms/dpf.h | 7 +++\n> > 2 files changed, 58 insertions(+), 29 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > index 9b663831..09f2bcbd 100644\n> > --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > @@ -316,38 +316,60 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> > if (!frameContext.dpf.update && frame > 0)\n> > return;\n> \n> With the new tuning file layout there is no default mode anymore, so\n> the algorithm starts with Dpf disabled by default and it is only\n> enabled once the user selects one modes at queueRequest() time.\n> \n> Is my understanding correct ?\n> \n> Question for Stefan and other ones who has been developing more\n> algorithms for RkISP1: is this aligned with how other algorithms work\n> ? In other word, there is no way to enable the algo from tuning file\n> but only with Controls. I think it is fine, just checking.\n\nThe unwritten expectation is afaik that a camera starts up with a\ndefault good image (whatever good in that sense is). So I believe that\ndepending on the sensor it might very well make sense to enable a bit of\ndenoising. Or to turn the question upside down. It would be frustrating\nto users if they can't configure their preferred default denoising in\nthe tuning file. So I think we should keep that option.\n\nBest regards,\nStefan\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> \n> So why you pass them to the function if you know know you're not going\n> to use them ?\n> \n> > + RkISP1Params *params)\n> > +{\n> > + auto dpfConfig = params->block();\n> > + dpfConfig.setEnabled(false);\n> > + auto dpfStrength = params->block();\n> > + dpfStrength.setEnabled(false);\n> > +}\n> > +\n> > +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > + [[maybe_unused]] IPAFrameContext &frameContext,\n> \n> Same question, if you know you're not going to use the function\n> parameters, why do you have them here ?\n> \n> > + RkISP1Params *params)\n> > +{\n> > auto config = params->block();\n> > - config.setEnabled(frameContext.dpf.denoise);\n> > + config.setEnabled(true);\n> > + *config = config_;\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> > + config_.gain.mode = mode;\n> >\n> > auto strengthConfig = params->block();\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> > + strengthConfig.setEnabled(true);\n> > + *strengthConfig = strengthConfig_;\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 2a2c7052..fcf121cb 100644\n> > --- a/src/ipa/rkisp1/algorithms/dpf.h\n> > +++ b/src/ipa/rkisp1/algorithms/dpf.h\n> > @@ -44,6 +44,13 @@ private:\n> >\n> > bool loadReductionConfig(int32_t mode);\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,\n> > + RkISP1Params *params);\n> > +\n> > struct rkisp1_cif_isp_dpf_config config_;\n> > struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;\n> > std::vector noiseReductionModes_;\n> > --\n> > 2.43.0\n> >","headers":{"Return-Path":"","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 1D82FBDCBF\n\tfor ;\n\tFri, 16 Jan 2026 13:38:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56BFF61FC3;\n\tFri, 16 Jan 2026 14:38:08 +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 97C79615B2\n\tfor ;\n\tFri, 16 Jan 2026 14:38:06 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:cc1c:cb5b:bc84:204])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 5A3EC594;\n\tFri, 16 Jan 2026 14:37:38 +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=\"mk3sVC/c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768570658;\n\tbh=x6V/RkPQXc3M13t7DddCW7mY9Yy0ZpjfATBbhrC/Yy8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=mk3sVC/cInkjQQMU7heh9LtQUpY1eyyIulydGExtqDFRpw52Qz4IdIEbxdsYOqpqt\n\tgh9j6bCHm7uSUOSExyX8soRKLp425yS6A1k+zzw4g750W/2HobXisfVINBWclKR+se\n\ttiN6byvEP3qBnYqugsujth90gZMyvb+hvG2gcgj0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20260115163318.1339354-1-rui.wang@ideasonboard.com>\n\t<20260115163318.1339354-4-rui.wang@ideasonboard.com>\n\t","Subject":"Re: [PATCH v8 3/7] ipa: rkisp1: algorithms: dpf: Refactor prepare()\n\tinto helpers","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","To":"Jacopo Mondi ,\n\tRui Wang ","Date":"Fri, 16 Jan 2026 14:38:03 +0100","Message-ID":"<176857068344.216397.2644393977613535059@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37695,"web_url":"https://patchwork.libcamera.org/comment/37695/","msgid":"","date":"2026-01-16T14:28:03","subject":"Re: [PATCH v8 3/7] ipa: rkisp1: algorithms: dpf: Refactor prepare()\n\tinto helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Fri, Jan 16, 2026 at 02:38:03PM +0100, Stefan Klug wrote:\n> Hi Rui, hi Jacopo,\n>\n> Quoting Jacopo Mondi (2026-01-16 10:42:54)\n> > Hi Rui\n> > + Stefan\n> >\n> > On Thu, Jan 15, 2026 at 11:33:14AM -0500, Rui Wang wrote:\n> > > Split the prepare() function into prepareDisabledMode() and\n> > > prepareEnabledMode() to improve readability and maintainability.\n> > >\n> > > This separates the logic for disabling and enabling the DPF, making\n> > > the main prepare() function cleaner and easier to follow.\n> > >\n> > > Signed-off-by: Rui Wang \n> > > Reviewed-by: Jacopo Mondi \n> > >\n> > > ---\n> > > changelog since v5i/v6: Nothing changed\n> > >\n> > > Reviewed-by tags from v5 are carried over (no code changes).\n> > > ---\n> > > src/ipa/rkisp1/algorithms/dpf.cpp | 80 ++++++++++++++++++++-----------\n> > > src/ipa/rkisp1/algorithms/dpf.h | 7 +++\n> > > 2 files changed, 58 insertions(+), 29 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > > index 9b663831..09f2bcbd 100644\n> > > --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > > @@ -316,38 +316,60 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> > > if (!frameContext.dpf.update && frame > 0)\n> > > return;\n> >\n> > With the new tuning file layout there is no default mode anymore, so\n> > the algorithm starts with Dpf disabled by default and it is only\n> > enabled once the user selects one modes at queueRequest() time.\n> >\n> > Is my understanding correct ?\n> >\n> > Question for Stefan and other ones who has been developing more\n> > algorithms for RkISP1: is this aligned with how other algorithms work\n> > ? In other word, there is no way to enable the algo from tuning file\n> > but only with Controls. I think it is fine, just checking.\n>\n> The unwritten expectation is afaik that a camera starts up with a\n> default good image (whatever good in that sense is). So I believe that\n> depending on the sensor it might very well make sense to enable a bit of\n> denoising. Or to turn the question upside down. It would be frustrating\n> to users if they can't configure their preferred default denoising in\n> the tuning file. So I think we should keep that option.\n\nI see.\n\nOriginally Rui proposed a scheme for the tuning file like the one\ndescribed in https://patchwork.libcamera.org/patch/25385/#37366\n\nI'll paste here below for convenience\n\n--------------------------------------------------------------------------------\nThe tuning file layout is the following one\n\n - Dpf:\n filter:\n nll:\n strength:\n modes:\n - type: \"minimal\"\n filter:\n nll:\n strength:\n - type: \"highquality\"\n filter:\n nll:\n strength:\n - type: \"fast\"\n filter:\n nll:\n strength:\n - type: \"zsl\"\n filter:\n nll:\n strength:\n\nEach entry in \"modes\" is .. a mode, with it's own configuration.\nBUT: what do the outer configuration refers to ? What is its purpose ?\nIt is not associated to any \"mode\", how can user select it ?\n\n - Dpf:\n filter: \\\n nll: | -> This one ??\n strength: /\n modes:\n - type: ...\n-------------------------------------------------------------------------------\n\nThe \"external\" configuration was used as a default and for manual/auto\nmode switching (if I got it right).\n\nI suggested Rui to remove it as users could only select one mode or\nto disable the Dpf using Controls, so the \"extenal\" mode was\neffectivelly not user selectable.\n\nHowever if we want to start with a mode and we want to give users the\nability to decide using the tuning file if Dpf should be enable,\nshould we maybe add a property like \"enabledMode: \". In example:\n\n - Dpf:\n modes:\n - type: \"minimal\"\n filter:\n nll:\n strength:\n - type: \"highquality\"\n filter:\n nll:\n strength:\n - type: \"fast\"\n filter:\n nll:\n strength:\n - type: \"zsl\"\n filter:\n nll:\n strength:\n enabledMode: \"minimal\"\n\nso that:\n\n- if \"enabledMode:\" is specified Dpf is started with this mode\n configured\n- if it is not specified Dpf is disabled by default\n- We don't need an \"external\"/\"default\" configuration which is not\n user-selectable like the previous version of the tuning file had\n\nWhat do you and Rui think ?\n\nThanks\n j\n\n>\n> Best regards,\n> Stefan\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> >\n> > So why you pass them to the function if you know know you're not going\n> > to use them ?\n> >\n> > > + RkISP1Params *params)\n> > > +{\n> > > + auto dpfConfig = params->block();\n> > > + dpfConfig.setEnabled(false);\n> > > + auto dpfStrength = params->block();\n> > > + dpfStrength.setEnabled(false);\n> > > +}\n> > > +\n> > > +void Dpf::prepareEnabledMode(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > + [[maybe_unused]] IPAFrameContext &frameContext,\n> >\n> > Same question, if you know you're not going to use the function\n> > parameters, why do you have them here ?\n> >\n> > > + RkISP1Params *params)\n> > > +{\n> > > auto config = params->block();\n> > > - config.setEnabled(frameContext.dpf.denoise);\n> > > + config.setEnabled(true);\n> > > + *config = config_;\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> > > + config_.gain.mode = mode;\n> > >\n> > > auto strengthConfig = params->block();\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> > > + strengthConfig.setEnabled(true);\n> > > + *strengthConfig = strengthConfig_;\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 2a2c7052..fcf121cb 100644\n> > > --- a/src/ipa/rkisp1/algorithms/dpf.h\n> > > +++ b/src/ipa/rkisp1/algorithms/dpf.h\n> > > @@ -44,6 +44,13 @@ private:\n> > >\n> > > bool loadReductionConfig(int32_t mode);\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,\n> > > + RkISP1Params *params);\n> > > +\n> > > struct rkisp1_cif_isp_dpf_config config_;\n> > > struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;\n> > > std::vector noiseReductionModes_;\n> > > --\n> > > 2.43.0\n> > >","headers":{"Return-Path":"","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 C5A00BDCBF\n\tfor ;\n\tFri, 16 Jan 2026 14:28:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDB0D61FA3;\n\tFri, 16 Jan 2026 15:28:08 +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 205E3615B2\n\tfor ;\n\tFri, 16 Jan 2026 15:28:07 +0100 (CET)","from ideasonboard.com (static.170.20.224.46.clients.your-server.de\n\t[46.224.20.170])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BCCED4B3;\n\tFri, 16 Jan 2026 15:27:38 +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=\"uDA7gN2R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768573658;\n\tbh=2ue6cASuSuJlNLzydIWAT7ELhY3OXeFxJFdHWmQ7buo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uDA7gN2RVWwgUco4DDa9L0rwh+ZzEL+/aE9R5YjStNpg9hK3QMmTXVwfTLSf1fghw\n\tsGo4iUZ0Bt1h+nRcA6AtQz1N5oB3l6NRziz1nbymoNDN9+zaVg0sjPDNppQ1VCE3C0\n\t5kLF1jAC/0RyUSEvYRG0Je/1d5iJXUvz71Lw1Nmw=","Date":"Fri, 16 Jan 2026 15:28:03 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tRui Wang , libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v8 3/7] ipa: rkisp1: algorithms: dpf: Refactor prepare()\n\tinto helpers","Message-ID":"","References":"<20260115163318.1339354-1-rui.wang@ideasonboard.com>\n\t<20260115163318.1339354-4-rui.wang@ideasonboard.com>\n\t\n\t<176857068344.216397.2644393977613535059@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176857068344.216397.2644393977613535059@localhost>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]