[{"id":23887,"web_url":"https://patchwork.libcamera.org/comment/23887/","msgid":"<20220714184652.rb2nflafhtoydmug@uno.localdomain>","date":"2022-07-14T18:46:52","subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel\n\nOn Wed, Jul 13, 2022 at 10:43:12AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Pass the controls set by top level API to the AF algorithm if it\n> was enabled.\n>\n> Signed-off-by: Daniel Semkowicz \n> ---\n> src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 50 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 01bb54fb..53b53f12 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -28,6 +28,7 @@\n> #include \"libcamera/internal/mapped_framebuffer.h\"\n> #include \"libcamera/internal/yaml_parser.h\"\n>\n> +#include \"algorithms/af.h\"\n> #include \"algorithms/agc.h\"\n> #include \"algorithms/algorithm.h\"\n> #include \"algorithms/awb.h\"\n> @@ -295,9 +296,56 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> }\n>\n> void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,\n> -\t\t\t [[maybe_unused]] const ControlList &controls)\n> +\t\t\t const ControlList &controls)\n> {\n> -\t/* \\todo Start processing for 'frame' based on 'controls'. */\n> +\tusing namespace algorithms;\n> +\n> +\tfor (auto const &ctrl : controls) {\n\nYou can use structured bindings, they're nicer :)\n\n\tfor (auto const &[id, val] : controls) {\n\n }\n\n> +\t\tunsigned int ctrlEnum = ctrl.first;\n> +\t\tconst ControlValue &ctrlValue = ctrl.second;\n\nAnd drop these\n> +\n> +\t\tLOG(IPARkISP1, Debug) << \"Request ctrl: \"\n> +\t\t\t\t << controls::controls.at(ctrlEnum)->name()\n> +\t\t\t\t << \" = \" << ctrlValue.toString();\n> +\n> +\t\tswitch (ctrlEnum) {\n> +\t\tcase controls::AF_MODE: {\n> +\t\t\tAf *af = getAlgorithm();\n> +\t\t\tif (!af) {\n> +\t\t\t\tLOG(IPARkISP1, Warning) << \"Could not set AF_MODE - no AF algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nYou can get *af once outside of the switch.\n\nAlso, as the failure in getting *af is not related to the control,\nthere's not much value in duplicating the error message, should\ngetAlgorithm<>() be made loud on failure so that the caller can skip\ndoing the same, if not required ?\n\n> +\n> +\t\t\taf->setMode(static_cast(ctrlValue.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase controls::AF_TRIGGER: {\n> +\t\t\tAf *af = getAlgorithm();\n> +\t\t\tif (!af) {\n> +\t\t\t\tLOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\taf->setTrigger(static_cast(ctrlValue.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase controls::AF_PAUSE: {\n> +\t\t\tAf *af = getAlgorithm();\n> +\t\t\tif (!af) {\n> +\t\t\t\tLOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\taf->setPause(static_cast(ctrlValue.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tdefault:\n> +\t\t\tLOG(IPARkISP1, Warning)\n> +\t\t\t\t<< \"Ctrl \" << controls::controls.at(ctrlEnum)->name()\n> +\t\t\t\t<< \" is not handled.\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> }\n>\n> void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> --\n> 2.34.1\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 130F3BE173\n\tfor ;\n\tThu, 14 Jul 2022 18:46:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A9D96330E;\n\tThu, 14 Jul 2022 20:46:57 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::225])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DBD76330D\n\tfor ;\n\tThu, 14 Jul 2022 20:46:55 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 870B51C0004;\n\tThu, 14 Jul 2022 18:46:54 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657824417;\n\tbh=2O/9pg+TncHUpS9N2yUUl8y8VDVv+1HGXTjctPCc0jY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Vm/I/b2d9m5bJV6fkDPy4lfdmeB+F72CkXpnfGHj0jTC4h0VFI40owsU9K+J1hqp1\n\tPehG9oRpU/PLlT+I6EopNteCZzSyGhjJo01a+aYBzo/4hSt7/ccTbOs+6lXHzQpzP+\n\tJIfNb53ZrgBwnCtBVJhZS5NrBQtR5DsESRXRqQO62MZ4LLGanmSZNdmoGJusFTrJ4O\n\trVQCnmj7sN3/oX73kEt+9Tfi2e4QiDjMVVleR5JvVUuwYchxUeQXuLDXnsGYC7+mZh\n\tAurqt1VBThd1TFdgdDcIBodke2fS0SuAsePFPsQzjlqdmtKDxxKbi6akeuptMppcHF\n\tDJHImLMRZ6QRQ==","Date":"Thu, 14 Jul 2022 20:46:52 +0200","To":"Daniel Semkowicz ","Message-ID":"<20220714184652.rb2nflafhtoydmug@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-7-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220713084317.24268-7-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23892,"web_url":"https://patchwork.libcamera.org/comment/23892/","msgid":"<165783167565.3811604.18393336649491601828@Monstersaurus>","date":"2022-07-14T20:47:55","subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Daniel,\n\nQuoting Jacopo Mondi via libcamera-devel (2022-07-14 19:46:52)\n> Hi Daniel\n> \n> On Wed, Jul 13, 2022 at 10:43:12AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > Pass the controls set by top level API to the AF algorithm if it\n> > was enabled.\n> >\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++--\n> > 1 file changed, 50 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 01bb54fb..53b53f12 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -28,6 +28,7 @@\n> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > #include \"libcamera/internal/yaml_parser.h\"\n> >\n> > +#include \"algorithms/af.h\"\n> > #include \"algorithms/agc.h\"\n> > #include \"algorithms/algorithm.h\"\n> > #include \"algorithms/awb.h\"\n> > @@ -295,9 +296,56 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> > }\n> >\n> > void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,\n> > - [[maybe_unused]] const ControlList &controls)\n> > + const ControlList &controls)\n> > {\n> > - /* \\todo Start processing for 'frame' based on 'controls'. */\n> > + using namespace algorithms;\n> > +\n> > + for (auto const &ctrl : controls) {\n> \n> You can use structured bindings, they're nicer :)\n> \n> for (auto const &[id, val] : controls) {\n> \n> }\n> \n> > + unsigned int ctrlEnum = ctrl.first;\n> > + const ControlValue &ctrlValue = ctrl.second;\n> \n> And drop these\n> > +\n> > + LOG(IPARkISP1, Debug) << \"Request ctrl: \"\n> > + << controls::controls.at(ctrlEnum)->name()\n> > + << \" = \" << ctrlValue.toString();\n> > +\n> > + switch (ctrlEnum) {\n> > + case controls::AF_MODE: {\n> > + Af *af = getAlgorithm();\n> > + if (!af) {\n> > + LOG(IPARkISP1, Warning) << \"Could not set AF_MODE - no AF algorithm\";\n> > + break;\n> > + }\n> \n> You can get *af once outside of the switch.\n> \n> Also, as the failure in getting *af is not related to the control,\n> there's not much value in duplicating the error message, should\n> getAlgorithm<>() be made loud on failure so that the caller can skip\n> doing the same, if not required ?\n\nI had actually envisaged handling controls differently, adding a hook to\neach algorithm called either queueRequest() or processControls() or\nsuch, and let each algorithm handle only the controls it uses.\n\nThe drawback there is that if a control goes unhandled, it would be\ndifficult for us to detect or report that, so I think I like this\napproach too.\n\nEspecially as you cover that exact condition in the default case below!\n\nMy only worry is that switch table could get really large though, and\nthe algorithms will have to have a public API to handle all each control\nspecifically which could get extensive.\n\n\nThe only alternative I can think of off the top of my head to consider\ncould be:\n\nControlList afControls;\nControlList agcControls;\n\nfor (auto const control : controls) {\n\tcase controls::AF_TRIGGER:\n\tcase controls::AF_PAUSE:\n\t\tafControls.emplace(control);\n\t\tbreak;\n\n\tcase controls::AE_ENABLE:\n\tcase controls::AE_METERING_MODE:\n\t\tagcControls.emplace(control);\n\t\tbreak;\n\n\tdefault:\n\t\t// unhandled control error\n\t\tbreak;\n}\n\n\nif (afControls.size()) {\n\t// I would probably store all the instantiated algorithms\n\t// as a named private pointer variable in the IPARkISP1 class.\n\n\tif (!af_) {\n\t\tLOG(IPARkISP1, Warning) << \"Unhandled AF controls\";\n\t} else {\n\t\taf_->setControls(afControls);\n\t}\n}\n\nif (agcControls.size()) {\n\n\tif (!agc_) {\n\t\tLOG(IPARkISP1, Warning) << \"Unhandled AGC controls\";\n\t} else {\n\t\tagc_->setControls(agcControls);\n\t}\n}\n\nPerhaps some of that boilerplate for each algorithm could get mapped\ndown in a template. And also perhaps this introduces more iteration and\ncopying than would be desired too - it's only sketching out an idea to\nsee if it's easier to keep the definition of how controls are handled by\nalgorithms managed by the algorithms themselves.\n\n--\nKieran\n\n\n\n> > +\n> > + af->setMode(static_cast(ctrlValue.get()));\n> > + break;\n> > + }\n> > + case controls::AF_TRIGGER: {\n> > + Af *af = getAlgorithm();\n> > + if (!af) {\n> > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > + break;\n> > + }\n> > +\n> > + af->setTrigger(static_cast(ctrlValue.get()));\n> > + break;\n> > + }\n> > + case controls::AF_PAUSE: {\n> > + Af *af = getAlgorithm();\n> > + if (!af) {\n> > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > + break;\n> > + }\n> > +\n> > + af->setPause(static_cast(ctrlValue.get()));\n> > + break;\n> > + }\n> > + default:\n> > + LOG(IPARkISP1, Warning)\n> > + << \"Ctrl \" << controls::controls.at(ctrlEnum)->name()\n> > + << \" is not handled.\";\n> > + break;\n> > + }\n> > + }\n> > }\n> >\n> > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > --\n> > 2.34.1\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 E9D8ABE173\n\tfor ;\n\tThu, 14 Jul 2022 20:48:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5854C6330D;\n\tThu, 14 Jul 2022 22:48:00 +0200 (CEST)","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 DBC266330D\n\tfor ;\n\tThu, 14 Jul 2022 22:47:58 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 61D46383;\n\tThu, 14 Jul 2022 22:47:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657831680;\n\tbh=DwzHJ21ldq5xodYO2GKGl1yKrHa4kv2Xh4fPZVNkI3M=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=qAmfZBM5/dfg6j0dxtE0dkZJCdgvtaiUJOn7WQauMDKq2CEnwDSFb80doONkG2dI1\n\trJ/l0Vmcjw+7mCwGAbnhc3P32i+vBxQDPOg+OUZ6p2QhlP8Ct7+kWWNz63w3/WmmnT\n\tCXWUgi1YKCbRZuIxnLslGzBqfPBSSVnsdYtgj6vB8XAiNBryG5GLLH9IP8s+JqxX8P\n\tz8XyKF2hTG4rPgiXu665kVzhIuIpc8eKldSWRcbYihSVFGbPKV12slUSeCoS2CI8SF\n\tXI5yZdISTdXxT9cZJOKHWAr+p5ZzyyiYZZb5gM+e08c2HhtNI6tc/6wvreghfXqJyS\n\tzI51YRDJPtmyQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657831678;\n\tbh=DwzHJ21ldq5xodYO2GKGl1yKrHa4kv2Xh4fPZVNkI3M=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=dKXDLc6+wcG5eX30YcswrqiBUgFWCyMRALD+wUnF+SQl40R8cjbj5SBQ1CrUKvz62\n\tYS1Na4GiHE0r5Hgh8mAENRuP9AqAKXH+a9u7WTwJxpfVciMX3/eRIIhVvqSUvUl8yz\n\tMg4VcABtnrN1UqERQfmy9uoDHi1nvxmIrEGTtkSs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dKXDLc6+\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220714184652.rb2nflafhtoydmug@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-7-dse@thaumatec.com>\n\t<20220714184652.rb2nflafhtoydmug@uno.localdomain>","To":"Daniel Semkowicz , Jacopo Mondi , \n\tJacopo Mondi via libcamera-devel ","Date":"Thu, 14 Jul 2022 21:47:55 +0100","Message-ID":"<165783167565.3811604.18393336649491601828@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23902,"web_url":"https://patchwork.libcamera.org/comment/23902/","msgid":"","date":"2022-07-15T00:38:50","subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Jul 14, 2022 at 09:47:55PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:46:52)\n> > On Wed, Jul 13, 2022 at 10:43:12AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > Pass the controls set by top level API to the AF algorithm if it\n> > > was enabled.\n> > >\n> > > Signed-off-by: Daniel Semkowicz \n> > > ---\n> > > src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++--\n> > > 1 file changed, 50 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 01bb54fb..53b53f12 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -28,6 +28,7 @@\n> > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > #include \"libcamera/internal/yaml_parser.h\"\n> > >\n> > > +#include \"algorithms/af.h\"\n> > > #include \"algorithms/agc.h\"\n> > > #include \"algorithms/algorithm.h\"\n> > > #include \"algorithms/awb.h\"\n> > > @@ -295,9 +296,56 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> > > }\n> > >\n> > > void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,\n> > > - [[maybe_unused]] const ControlList &controls)\n> > > + const ControlList &controls)\n> > > {\n> > > - /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > + using namespace algorithms;\n> > > +\n> > > + for (auto const &ctrl : controls) {\n> > \n> > You can use structured bindings, they're nicer :)\n> > \n> > for (auto const &[id, val] : controls) {\n> > \n> > }\n> > \n> > > + unsigned int ctrlEnum = ctrl.first;\n> > > + const ControlValue &ctrlValue = ctrl.second;\n> > \n> > And drop these\n> > > +\n> > > + LOG(IPARkISP1, Debug) << \"Request ctrl: \"\n> > > + << controls::controls.at(ctrlEnum)->name()\n> > > + << \" = \" << ctrlValue.toString();\n> > > +\n> > > + switch (ctrlEnum) {\n> > > + case controls::AF_MODE: {\n> > > + Af *af = getAlgorithm();\n> > > + if (!af) {\n> > > + LOG(IPARkISP1, Warning) << \"Could not set AF_MODE - no AF algorithm\";\n> > > + break;\n> > > + }\n> > \n> > You can get *af once outside of the switch.\n> > \n> > Also, as the failure in getting *af is not related to the control,\n> > there's not much value in duplicating the error message, should\n> > getAlgorithm<>() be made loud on failure so that the caller can skip\n> > doing the same, if not required ?\n> \n> I had actually envisaged handling controls differently, adding a hook to\n> each algorithm called either queueRequest() or processControls() or\n> such, and let each algorithm handle only the controls it uses.\n\nDo you mean https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031931.html\nand https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031932.html ? :-)\n\n> The drawback there is that if a control goes unhandled, it would be\n> difficult for us to detect or report that, so I think I like this\n> approach too.\n\nWe could let the Algorithm::queueRequest() function indicate which\ncontrol(s) it has handled, and verify at the end that all controls have\nbeen handled.\n\n> Especially as you cover that exact condition in the default case below!\n> \n> My only worry is that switch table could get really large though, and\n> the algorithms will have to have a public API to handle all each control\n> specifically which could get extensive.\n\nThat's what the RPi IPA module does, and I'm not a big fan of the end\nresult.\n\n> The only alternative I can think of off the top of my head to consider\n> could be:\n> \n> ControlList afControls;\n> ControlList agcControls;\n> \n> for (auto const control : controls) {\n> \tcase controls::AF_TRIGGER:\n> \tcase controls::AF_PAUSE:\n> \t\tafControls.emplace(control);\n> \t\tbreak;\n> \n> \tcase controls::AE_ENABLE:\n> \tcase controls::AE_METERING_MODE:\n> \t\tagcControls.emplace(control);\n> \t\tbreak;\n> \n> \tdefault:\n> \t\t// unhandled control error\n> \t\tbreak;\n> }\n> \n> \n> if (afControls.size()) {\n> \t// I would probably store all the instantiated algorithms\n> \t// as a named private pointer variable in the IPARkISP1 class.\n> \n> \tif (!af_) {\n> \t\tLOG(IPARkISP1, Warning) << \"Unhandled AF controls\";\n> \t} else {\n> \t\taf_->setControls(afControls);\n> \t}\n> }\n> \n> if (agcControls.size()) {\n> \n> \tif (!agc_) {\n> \t\tLOG(IPARkISP1, Warning) << \"Unhandled AGC controls\";\n> \t} else {\n> \t\tagc_->setControls(agcControls);\n> \t}\n> }\n> \n> Perhaps some of that boilerplate for each algorithm could get mapped\n> down in a template. And also perhaps this introduces more iteration and\n> copying than would be desired too - it's only sketching out an idea to\n> see if it's easier to keep the definition of how controls are handled by\n> algorithms managed by the algorithms themselves.\n> \n> > > +\n> > > + af->setMode(static_cast(ctrlValue.get()));\n> > > + break;\n> > > + }\n> > > + case controls::AF_TRIGGER: {\n> > > + Af *af = getAlgorithm();\n> > > + if (!af) {\n> > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > + break;\n> > > + }\n> > > +\n> > > + af->setTrigger(static_cast(ctrlValue.get()));\n> > > + break;\n> > > + }\n> > > + case controls::AF_PAUSE: {\n> > > + Af *af = getAlgorithm();\n> > > + if (!af) {\n> > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > + break;\n> > > + }\n> > > +\n> > > + af->setPause(static_cast(ctrlValue.get()));\n> > > + break;\n> > > + }\n> > > + default:\n> > > + LOG(IPARkISP1, Warning)\n> > > + << \"Ctrl \" << controls::controls.at(ctrlEnum)->name()\n> > > + << \" is not handled.\";\n> > > + break;\n> > > + }\n> > > + }\n> > > }\n> > >\n> > > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)","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 CAF9CBD1F1\n\tfor ;\n\tFri, 15 Jul 2022 00:39:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C6A660489;\n\tFri, 15 Jul 2022 02:39:24 +0200 (CEST)","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 2EDC260489\n\tfor ;\n\tFri, 15 Jul 2022 02:39:22 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8BAD09DA;\n\tFri, 15 Jul 2022 02:39:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657845564;\n\tbh=9ErzKXw6hAzL0VsMGQ0OqJ4R3zGH8X+a7R0lRJFZBRg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=IiyOJaowQK8TcjPKFHjBr8QBZ1DQBKAok+FSM9JkRvNV4SoLlNjF6CSkVZRVtGCGe\n\tcP3QyKsnBizWaGguNP0kOg3SVhQ5p1IeHZ8CuIBAF3NPQkZTWLsiw2NPxU9vpeQSxD\n\tMQNPmrxP9Gc5pj29/XtgcPHShYLM190yT+2mn9RSAmxRw19AeaMg7FSFCqiAU3U0CQ\n\tkUq1CBRzTF3C3ssK72BIf8/k1B7g1PBvuHorntSeyhbrigdr6azWMQA10Cp4JD5yMX\n\t3A1w6Xn289hVaMjUabZzCx3mLKfABINX8/TNkYemJfpTql9tUQ63K145/3gyNFeWQB\n\tnheOc9DQoLEnQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657845561;\n\tbh=9ErzKXw6hAzL0VsMGQ0OqJ4R3zGH8X+a7R0lRJFZBRg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cJ5YFjUm0tkazRcLUmZl7a0Za1tkhr5wZLNHJdJFuXs36se29st4sVdpzkNQMvuwj\n\tuw+K+IkLWgvllahWhfJ7OLmbe21ibIlbYZqDFy56x9k1Vqox8JqZXzbUD7Kkufp5ha\n\tdVLeefy5LvZfd56ieGyTIt7zSdmpsvdQvWJxUMIk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cJ5YFjUm\"; dkim-atps=neutral","Date":"Fri, 15 Jul 2022 03:38:50 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-7-dse@thaumatec.com>\n\t<20220714184652.rb2nflafhtoydmug@uno.localdomain>\n\t<165783167565.3811604.18393336649491601828@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165783167565.3811604.18393336649491601828@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23905,"web_url":"https://patchwork.libcamera.org/comment/23905/","msgid":"<20220715062831.qubtq4qvnboqrfix@uno.localdomain>","date":"2022-07-15T06:28:31","subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Fri, Jul 15, 2022 at 03:38:50AM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> On Thu, Jul 14, 2022 at 09:47:55PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:46:52)\n> > > On Wed, Jul 13, 2022 at 10:43:12AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > Pass the controls set by top level API to the AF algorithm if it\n> > > > was enabled.\n> > > >\n> > > > Signed-off-by: Daniel Semkowicz \n> > > > ---\n> > > > src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++--\n> > > > 1 file changed, 50 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 01bb54fb..53b53f12 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -28,6 +28,7 @@\n> > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > #include \"libcamera/internal/yaml_parser.h\"\n> > > >\n> > > > +#include \"algorithms/af.h\"\n> > > > #include \"algorithms/agc.h\"\n> > > > #include \"algorithms/algorithm.h\"\n> > > > #include \"algorithms/awb.h\"\n> > > > @@ -295,9 +296,56 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> > > > }\n> > > >\n> > > > void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,\n> > > > - [[maybe_unused]] const ControlList &controls)\n> > > > + const ControlList &controls)\n> > > > {\n> > > > - /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > > + using namespace algorithms;\n> > > > +\n> > > > + for (auto const &ctrl : controls) {\n> > >\n> > > You can use structured bindings, they're nicer :)\n> > >\n> > > for (auto const &[id, val] : controls) {\n> > >\n> > > }\n> > >\n> > > > + unsigned int ctrlEnum = ctrl.first;\n> > > > + const ControlValue &ctrlValue = ctrl.second;\n> > >\n> > > And drop these\n> > > > +\n> > > > + LOG(IPARkISP1, Debug) << \"Request ctrl: \"\n> > > > + << controls::controls.at(ctrlEnum)->name()\n> > > > + << \" = \" << ctrlValue.toString();\n> > > > +\n> > > > + switch (ctrlEnum) {\n> > > > + case controls::AF_MODE: {\n> > > > + Af *af = getAlgorithm();\n> > > > + if (!af) {\n> > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_MODE - no AF algorithm\";\n> > > > + break;\n> > > > + }\n> > >\n> > > You can get *af once outside of the switch.\n> > >\n> > > Also, as the failure in getting *af is not related to the control,\n> > > there's not much value in duplicating the error message, should\n> > > getAlgorithm<>() be made loud on failure so that the caller can skip\n> > > doing the same, if not required ?\n> >\n> > I had actually envisaged handling controls differently, adding a hook to\n> > each algorithm called either queueRequest() or processControls() or\n> > such, and let each algorithm handle only the controls it uses.\n>\n> Do you mean https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031931.html\n> and https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031932.html ? :-)\n>\n\nOh that's nice. I have never been in love with the switch(controls)\npattern but I didn't want to throw too many thins at Daniel, as after\nall this is what the RPi IPA does. If we can find something different,\nI'm all for it.\n\n> > The drawback there is that if a control goes unhandled, it would be\n> > difficult for us to detect or report that, so I think I like this\n> > approach too.\n>\n> We could let the Algorithm::queueRequest() function indicate which\n> control(s) it has handled, and verify at the end that all controls have\n> been handled.\n>\n> > Especially as you cover that exact condition in the default case below!\n> >\n> > My only worry is that switch table could get really large though, and\n> > the algorithms will have to have a public API to handle all each control\n> > specifically which could get extensive.\n>\n> That's what the RPi IPA module does, and I'm not a big fan of the end\n> result.\n>\n> > The only alternative I can think of off the top of my head to consider\n> > could be:\n> >\n> > ControlList afControls;\n> > ControlList agcControls;\n> >\n> > for (auto const control : controls) {\n> > \tcase controls::AF_TRIGGER:\n> > \tcase controls::AF_PAUSE:\n> > \t\tafControls.emplace(control);\n> > \t\tbreak;\n> >\n> > \tcase controls::AE_ENABLE:\n> > \tcase controls::AE_METERING_MODE:\n> > \t\tagcControls.emplace(control);\n> > \t\tbreak;\n> >\n> > \tdefault:\n> > \t\t// unhandled control error\n> > \t\tbreak;\n> > }\n> >\n> >\n> > if (afControls.size()) {\n> > \t// I would probably store all the instantiated algorithms\n> > \t// as a named private pointer variable in the IPARkISP1 class.\n> >\n> > \tif (!af_) {\n> > \t\tLOG(IPARkISP1, Warning) << \"Unhandled AF controls\";\n> > \t} else {\n> > \t\taf_->setControls(afControls);\n> > \t}\n> > }\n> >\n> > if (agcControls.size()) {\n> >\n> > \tif (!agc_) {\n> > \t\tLOG(IPARkISP1, Warning) << \"Unhandled AGC controls\";\n> > \t} else {\n> > \t\tagc_->setControls(agcControls);\n> > \t}\n> > }\n> >\n> > Perhaps some of that boilerplate for each algorithm could get mapped\n> > down in a template. And also perhaps this introduces more iteration and\n> > copying than would be desired too - it's only sketching out an idea to\n> > see if it's easier to keep the definition of how controls are handled by\n> > algorithms managed by the algorithms themselves.\n> >\n> > > > +\n> > > > + af->setMode(static_cast(ctrlValue.get()));\n> > > > + break;\n> > > > + }\n> > > > + case controls::AF_TRIGGER: {\n> > > > + Af *af = getAlgorithm();\n> > > > + if (!af) {\n> > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + af->setTrigger(static_cast(ctrlValue.get()));\n> > > > + break;\n> > > > + }\n> > > > + case controls::AF_PAUSE: {\n> > > > + Af *af = getAlgorithm();\n> > > > + if (!af) {\n> > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + af->setPause(static_cast(ctrlValue.get()));\n> > > > + break;\n> > > > + }\n> > > > + default:\n> > > > + LOG(IPARkISP1, Warning)\n> > > > + << \"Ctrl \" << controls::controls.at(ctrlEnum)->name()\n> > > > + << \" is not handled.\";\n> > > > + break;\n> > > > + }\n> > > > + }\n> > > > }\n> > > >\n> > > > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 E83AABD1F1\n\tfor ;\n\tFri, 15 Jul 2022 06:28:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 20C4D63312;\n\tFri, 15 Jul 2022 08:28:36 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::223])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D48E60489\n\tfor ;\n\tFri, 15 Jul 2022 08:28:34 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 3721A60005;\n\tFri, 15 Jul 2022 06:28:32 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657866516;\n\tbh=srXunq3fbsBkBgwjDfm9ZpibBpZjB9f9J5EPRUUE2Ho=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=RHFvpRfjJkh4rCHddLd4Lrlcq4GVpydZ86Yt9kYbgrNMjPbWHvTj48tZ3LQwe+prF\n\tJwF2Fvlpe1JK5UPcwjwBvfYoEIJAl92Sgftng9XIln5mzNlnCZgfZRAEXFXHXmMeg9\n\tPHV27BQU/33A705FMmEaKJloSMB5fMBy+b+2GQ859Ko1eLTybloqIDWU39gMn+TbAl\n\tGK2dhXPLw1kfIRDXwvA3DVOVnMBms20B6MGN5Z7FQjZh5D3cIlg49xJpwyVzSTJAra\n\tdmJuKotJYj/Zm4gqjhBQR4qS/JxcP1XNUcoeePq/8RM3680Tc3fdFGu+Qg3UmliIif\n\tood7E4VReyclA==","Date":"Fri, 15 Jul 2022 08:28:31 +0200","To":"Laurent Pinchart ","Message-ID":"<20220715062831.qubtq4qvnboqrfix@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-7-dse@thaumatec.com>\n\t<20220714184652.rb2nflafhtoydmug@uno.localdomain>\n\t<165783167565.3811604.18393336649491601828@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23927,"web_url":"https://patchwork.libcamera.org/comment/23927/","msgid":"<165788219322.2021905.16479347266969922720@Monstersaurus>","date":"2022-07-15T10:49:53","subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-07-15 01:38:50)\n> Hello,\n> \n> On Thu, Jul 14, 2022 at 09:47:55PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:46:52)\n> > > On Wed, Jul 13, 2022 at 10:43:12AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > Pass the controls set by top level API to the AF algorithm if it\n> > > > was enabled.\n> > > >\n> > > > Signed-off-by: Daniel Semkowicz \n> > > > ---\n> > > > src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++--\n> > > > 1 file changed, 50 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 01bb54fb..53b53f12 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -28,6 +28,7 @@\n> > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > #include \"libcamera/internal/yaml_parser.h\"\n> > > >\n> > > > +#include \"algorithms/af.h\"\n> > > > #include \"algorithms/agc.h\"\n> > > > #include \"algorithms/algorithm.h\"\n> > > > #include \"algorithms/awb.h\"\n> > > > @@ -295,9 +296,56 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> > > > }\n> > > >\n> > > > void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,\n> > > > - [[maybe_unused]] const ControlList &controls)\n> > > > + const ControlList &controls)\n> > > > {\n> > > > - /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > > + using namespace algorithms;\n> > > > +\n> > > > + for (auto const &ctrl : controls) {\n> > > \n> > > You can use structured bindings, they're nicer :)\n> > > \n> > > for (auto const &[id, val] : controls) {\n> > > \n> > > }\n> > > \n> > > > + unsigned int ctrlEnum = ctrl.first;\n> > > > + const ControlValue &ctrlValue = ctrl.second;\n> > > \n> > > And drop these\n> > > > +\n> > > > + LOG(IPARkISP1, Debug) << \"Request ctrl: \"\n> > > > + << controls::controls.at(ctrlEnum)->name()\n> > > > + << \" = \" << ctrlValue.toString();\n> > > > +\n> > > > + switch (ctrlEnum) {\n> > > > + case controls::AF_MODE: {\n> > > > + Af *af = getAlgorithm();\n> > > > + if (!af) {\n> > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_MODE - no AF algorithm\";\n> > > > + break;\n> > > > + }\n> > > \n> > > You can get *af once outside of the switch.\n> > > \n> > > Also, as the failure in getting *af is not related to the control,\n> > > there's not much value in duplicating the error message, should\n> > > getAlgorithm<>() be made loud on failure so that the caller can skip\n> > > doing the same, if not required ?\n> > \n> > I had actually envisaged handling controls differently, adding a hook to\n> > each algorithm called either queueRequest() or processControls() or\n> > such, and let each algorithm handle only the controls it uses.\n> \n> Do you mean https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031931.html\n> and https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031932.html ? :-)\n\nAha - Yes! I hadn't seen these patches yet.\n\nI'll head over and find those now. Merging that would help IPU3 already\ntoo I believe (and RKISP here of course).\n\n\n> > The drawback there is that if a control goes unhandled, it would be\n> > difficult for us to detect or report that, so I think I like this\n> > approach too.\n> \n> We could let the Algorithm::queueRequest() function indicate which\n> control(s) it has handled, and verify at the end that all controls have\n> been handled.\n\nYes, I think I can see workable solutions with that in mind.\n\n\n\n> > Especially as you cover that exact condition in the default case below!\n> > \n> > My only worry is that switch table could get really large though, and\n> > the algorithms will have to have a public API to handle all each control\n> > specifically which could get extensive.\n> \n> That's what the RPi IPA module does, and I'm not a big fan of the end\n> result.\n\n'A really big list' terrifies me of ending up like the Android HAL layer\nrequest processing. I would want to avoid that too.\n\n--\nKieran\n\n> > The only alternative I can think of off the top of my head to consider\n> > could be:\n> > \n> > ControlList afControls;\n> > ControlList agcControls;\n> > \n> > for (auto const control : controls) {\n> > case controls::AF_TRIGGER:\n> > case controls::AF_PAUSE:\n> > afControls.emplace(control);\n> > break;\n> > \n> > case controls::AE_ENABLE:\n> > case controls::AE_METERING_MODE:\n> > agcControls.emplace(control);\n> > break;\n> > \n> > default:\n> > // unhandled control error\n> > break;\n> > }\n> > \n> > \n> > if (afControls.size()) {\n> > // I would probably store all the instantiated algorithms\n> > // as a named private pointer variable in the IPARkISP1 class.\n> > \n> > if (!af_) {\n> > LOG(IPARkISP1, Warning) << \"Unhandled AF controls\";\n> > } else {\n> > af_->setControls(afControls);\n> > }\n> > }\n> > \n> > if (agcControls.size()) {\n> > \n> > if (!agc_) {\n> > LOG(IPARkISP1, Warning) << \"Unhandled AGC controls\";\n> > } else {\n> > agc_->setControls(agcControls);\n> > }\n> > }\n> > \n> > Perhaps some of that boilerplate for each algorithm could get mapped\n> > down in a template. And also perhaps this introduces more iteration and\n> > copying than would be desired too - it's only sketching out an idea to\n> > see if it's easier to keep the definition of how controls are handled by\n> > algorithms managed by the algorithms themselves.\n> > \n> > > > +\n> > > > + af->setMode(static_cast(ctrlValue.get()));\n> > > > + break;\n> > > > + }\n> > > > + case controls::AF_TRIGGER: {\n> > > > + Af *af = getAlgorithm();\n> > > > + if (!af) {\n> > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + af->setTrigger(static_cast(ctrlValue.get()));\n> > > > + break;\n> > > > + }\n> > > > + case controls::AF_PAUSE: {\n> > > > + Af *af = getAlgorithm();\n> > > > + if (!af) {\n> > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + af->setPause(static_cast(ctrlValue.get()));\n> > > > + break;\n> > > > + }\n> > > > + default:\n> > > > + LOG(IPARkISP1, Warning)\n> > > > + << \"Ctrl \" << controls::controls.at(ctrlEnum)->name()\n> > > > + << \" is not handled.\";\n> > > > + break;\n> > > > + }\n> > > > + }\n> > > > }\n> > > >\n> > > > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 8DC93BD1F1\n\tfor ;\n\tFri, 15 Jul 2022 10:49:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59D8A63315;\n\tFri, 15 Jul 2022 12:49:57 +0200 (CEST)","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 ACD396330A\n\tfor ;\n\tFri, 15 Jul 2022 12:49:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 495E7993;\n\tFri, 15 Jul 2022 12:49:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657882197;\n\tbh=+OoisDjSC8QLg2iN3uefip2u8Js+M3qpaqDNC56IQRo=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=C3Nx8CONSbm+3QHIeeXK2Y5/aPNbLTk8uRUCO1nFzZkLkb4P2PoFgaKrFGWPauaII\n\tFhkrG+LsQYbnRsh034n5X/YG0JZKO8q2lbDazTArP/Z48q5rL7jBJwJ0A0RMGQsJrS\n\tgaBKLzoLo0L6UW1IjjpLD/moBk/lu0VIpK8b+Em++chXlUOkvAJEa8rMOvxXEoMA8G\n\tJaxEELbpGAesPaX58jmaKGD2zhj/+Py0HVvB/xCBIaLiSGgo8VUMAo3g4doIayZp5g\n\tjfxxPN0Pp9mZnwO7lkIJZnbOWbO8+zPu1lu5KTxoTD6Rg29JrFDPq0Jus4TmdAC+G9\n\tW6NFixE2vzDUQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657882195;\n\tbh=+OoisDjSC8QLg2iN3uefip2u8Js+M3qpaqDNC56IQRo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=N2l64CtzI/46UIjlSEtFNMe4KdfTz5Rze9evcySMWrl8RxEFZmYsyNkrcAz+Fx/tV\n\tjC+ebjQgoSyZUye+LLJg5TxP9VPtyhC/UOsHnEDBTfidrKpQ1LjyGRTIllEFSPRQxU\n\tflk/UdbVAeX9SnbssI9KVBb3HTFXDVRJWuZpWHd0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"N2l64Ctz\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-7-dse@thaumatec.com>\n\t<20220714184652.rb2nflafhtoydmug@uno.localdomain>\n\t<165783167565.3811604.18393336649491601828@Monstersaurus>\n\t","To":"Laurent Pinchart ","Date":"Fri, 15 Jul 2022 11:49:53 +0100","Message-ID":"<165788219322.2021905.16479347266969922720@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23930,"web_url":"https://patchwork.libcamera.org/comment/23930/","msgid":"","date":"2022-07-15T11:19:36","subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jul 15, 2022 at 11:49:53AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2022-07-15 01:38:50)\n> > On Thu, Jul 14, 2022 at 09:47:55PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:46:52)\n> > > > On Wed, Jul 13, 2022 at 10:43:12AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > Pass the controls set by top level API to the AF algorithm if it\n> > > > > was enabled.\n> > > > >\n> > > > > Signed-off-by: Daniel Semkowicz \n> > > > > ---\n> > > > > src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++--\n> > > > > 1 file changed, 50 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > index 01bb54fb..53b53f12 100644\n> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > @@ -28,6 +28,7 @@\n> > > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > > #include \"libcamera/internal/yaml_parser.h\"\n> > > > >\n> > > > > +#include \"algorithms/af.h\"\n> > > > > #include \"algorithms/agc.h\"\n> > > > > #include \"algorithms/algorithm.h\"\n> > > > > #include \"algorithms/awb.h\"\n> > > > > @@ -295,9 +296,56 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> > > > > }\n> > > > >\n> > > > > void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,\n> > > > > - [[maybe_unused]] const ControlList &controls)\n> > > > > + const ControlList &controls)\n> > > > > {\n> > > > > - /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > > > + using namespace algorithms;\n> > > > > +\n> > > > > + for (auto const &ctrl : controls) {\n> > > > \n> > > > You can use structured bindings, they're nicer :)\n> > > > \n> > > > for (auto const &[id, val] : controls) {\n> > > > \n> > > > }\n> > > > \n> > > > > + unsigned int ctrlEnum = ctrl.first;\n> > > > > + const ControlValue &ctrlValue = ctrl.second;\n> > > > \n> > > > And drop these\n> > > > > +\n> > > > > + LOG(IPARkISP1, Debug) << \"Request ctrl: \"\n> > > > > + << controls::controls.at(ctrlEnum)->name()\n> > > > > + << \" = \" << ctrlValue.toString();\n> > > > > +\n> > > > > + switch (ctrlEnum) {\n> > > > > + case controls::AF_MODE: {\n> > > > > + Af *af = getAlgorithm();\n> > > > > + if (!af) {\n> > > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_MODE - no AF algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > \n> > > > You can get *af once outside of the switch.\n> > > > \n> > > > Also, as the failure in getting *af is not related to the control,\n> > > > there's not much value in duplicating the error message, should\n> > > > getAlgorithm<>() be made loud on failure so that the caller can skip\n> > > > doing the same, if not required ?\n> > > \n> > > I had actually envisaged handling controls differently, adding a hook to\n> > > each algorithm called either queueRequest() or processControls() or\n> > > such, and let each algorithm handle only the controls it uses.\n> > \n> > Do you mean https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031931.html\n> > and https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031932.html ? :-)\n> \n> Aha - Yes! I hadn't seen these patches yet.\n> \n> I'll head over and find those now. Merging that would help IPU3 already\n> too I believe (and RKISP here of course).\n\nI'll prioritize the series.\n\n> > > The drawback there is that if a control goes unhandled, it would be\n> > > difficult for us to detect or report that, so I think I like this\n> > > approach too.\n> > \n> > We could let the Algorithm::queueRequest() function indicate which\n> > control(s) it has handled, and verify at the end that all controls have\n> > been handled.\n> \n> Yes, I think I can see workable solutions with that in mind.\n\nAnd will do this on top if/when needed.\n\n> > > Especially as you cover that exact condition in the default case below!\n> > > \n> > > My only worry is that switch table could get really large though, and\n> > > the algorithms will have to have a public API to handle all each control\n> > > specifically which could get extensive.\n> > \n> > That's what the RPi IPA module does, and I'm not a big fan of the end\n> > result.\n> \n> 'A really big list' terrifies me of ending up like the Android HAL layer\n> request processing. I would want to avoid that too.\n> \n> > > The only alternative I can think of off the top of my head to consider\n> > > could be:\n> > > \n> > > ControlList afControls;\n> > > ControlList agcControls;\n> > > \n> > > for (auto const control : controls) {\n> > > case controls::AF_TRIGGER:\n> > > case controls::AF_PAUSE:\n> > > afControls.emplace(control);\n> > > break;\n> > > \n> > > case controls::AE_ENABLE:\n> > > case controls::AE_METERING_MODE:\n> > > agcControls.emplace(control);\n> > > break;\n> > > \n> > > default:\n> > > // unhandled control error\n> > > break;\n> > > }\n> > > \n> > > \n> > > if (afControls.size()) {\n> > > // I would probably store all the instantiated algorithms\n> > > // as a named private pointer variable in the IPARkISP1 class.\n> > > \n> > > if (!af_) {\n> > > LOG(IPARkISP1, Warning) << \"Unhandled AF controls\";\n> > > } else {\n> > > af_->setControls(afControls);\n> > > }\n> > > }\n> > > \n> > > if (agcControls.size()) {\n> > > \n> > > if (!agc_) {\n> > > LOG(IPARkISP1, Warning) << \"Unhandled AGC controls\";\n> > > } else {\n> > > agc_->setControls(agcControls);\n> > > }\n> > > }\n> > > \n> > > Perhaps some of that boilerplate for each algorithm could get mapped\n> > > down in a template. And also perhaps this introduces more iteration and\n> > > copying than would be desired too - it's only sketching out an idea to\n> > > see if it's easier to keep the definition of how controls are handled by\n> > > algorithms managed by the algorithms themselves.\n> > > \n> > > > > +\n> > > > > + af->setMode(static_cast(ctrlValue.get()));\n> > > > > + break;\n> > > > > + }\n> > > > > + case controls::AF_TRIGGER: {\n> > > > > + Af *af = getAlgorithm();\n> > > > > + if (!af) {\n> > > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > > +\n> > > > > + af->setTrigger(static_cast(ctrlValue.get()));\n> > > > > + break;\n> > > > > + }\n> > > > > + case controls::AF_PAUSE: {\n> > > > > + Af *af = getAlgorithm();\n> > > > > + if (!af) {\n> > > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > > +\n> > > > > + af->setPause(static_cast(ctrlValue.get()));\n> > > > > + break;\n> > > > > + }\n> > > > > + default:\n> > > > > + LOG(IPARkISP1, Warning)\n> > > > > + << \"Ctrl \" << controls::controls.at(ctrlEnum)->name()\n> > > > > + << \" is not handled.\";\n> > > > > + break;\n> > > > > + }\n> > > > > + }\n> > > > > }\n> > > > >\n> > > > > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)","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 905FFBD1F1\n\tfor ;\n\tFri, 15 Jul 2022 11:20:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF9506330A;\n\tFri, 15 Jul 2022 13:20:09 +0200 (CEST)","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 6B16E6330A\n\tfor ;\n\tFri, 15 Jul 2022 13:20:08 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B04B4993;\n\tFri, 15 Jul 2022 13:20:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657884010;\n\tbh=3/ZcUAsVyN/DPBEtJ+NiDfP1CWJwisnDfmeC25vk3Ac=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kkPWv6VuhhQIPVU1zGZrXMgz2NhehqH9+LNZZPS3DgvWCR5Uke9wgFIuNpEkQDc0I\n\tm9WF0l1ViN/hSGU7i7J+qDzRY7OzZi9KfxBEXLRWZfAHqZeMFHAM1+zV2r9JM1lHj/\n\tfep3KQXutsHOp72T+NJnY9E9zu83lU3PqZ9KaSfSXBQkTS4izPtN9+23+zinW7USiv\n\tGsefrC1LdRQ7x16Tr/gHuSPU+4bfscswsNt1epjLeegxU9x3pLgN15NW5UncBwJY1m\n\t5bf6bij8rtuugSWC8pDLJAI4TBO/3rmPRlGdn1yrRSqYZUIqPADrttKHGbF1oMeJkz\n\t6/GSXx/XmjcBg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657884007;\n\tbh=3/ZcUAsVyN/DPBEtJ+NiDfP1CWJwisnDfmeC25vk3Ac=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uctif4zwArwT+EFXPvWqoq+Dz823PbJwtZxaIlhfP5emRFZf8q8tRof2kfEDq/A99\n\tfAklHFgho6Fz+ZSeGwelEqJ0+jcTIxDENydYNn0CLO/1fepgQk1uwpJ3EfRF3Gsnqr\n\tNWGAXlnNMdmaFzaUxmUbpXWLOsawjlF1hMzishfI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uctif4zw\"; dkim-atps=neutral","Date":"Fri, 15 Jul 2022 14:19:36 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-7-dse@thaumatec.com>\n\t<20220714184652.rb2nflafhtoydmug@uno.localdomain>\n\t<165783167565.3811604.18393336649491601828@Monstersaurus>\n\t\n\t<165788219322.2021905.16479347266969922720@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165788219322.2021905.16479347266969922720@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23954,"web_url":"https://patchwork.libcamera.org/comment/23954/","msgid":"","date":"2022-07-18T15:45:53","subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi,\n\nOn Fri, Jul 15, 2022 at 1:20 PM Laurent Pinchart\n wrote:\n>\n> On Fri, Jul 15, 2022 at 11:49:53AM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2022-07-15 01:38:50)\n> > > On Thu, Jul 14, 2022 at 09:47:55PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > > Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:46:52)\n> > > > > On Wed, Jul 13, 2022 at 10:43:12AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > Pass the controls set by top level API to the AF algorithm if it\n> > > > > > was enabled.\n> > > > > >\n> > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > ---\n> > > > > > src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++--\n> > > > > > 1 file changed, 50 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > index 01bb54fb..53b53f12 100644\n> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > @@ -28,6 +28,7 @@\n> > > > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > > > #include \"libcamera/internal/yaml_parser.h\"\n> > > > > >\n> > > > > > +#include \"algorithms/af.h\"\n> > > > > > #include \"algorithms/agc.h\"\n> > > > > > #include \"algorithms/algorithm.h\"\n> > > > > > #include \"algorithms/awb.h\"\n> > > > > > @@ -295,9 +296,56 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> > > > > > }\n> > > > > >\n> > > > > > void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,\n> > > > > > - [[maybe_unused]] const ControlList &controls)\n> > > > > > + const ControlList &controls)\n> > > > > > {\n> > > > > > - /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > > > > + using namespace algorithms;\n> > > > > > +\n> > > > > > + for (auto const &ctrl : controls) {\n> > > > >\n> > > > > You can use structured bindings, they're nicer :)\n> > > > >\n> > > > > for (auto const &[id, val] : controls) {\n> > > > >\n> > > > > }\n> > > > >\n> > > > > > + unsigned int ctrlEnum = ctrl.first;\n> > > > > > + const ControlValue &ctrlValue = ctrl.second;\n> > > > >\n> > > > > And drop these\n> > > > > > +\n> > > > > > + LOG(IPARkISP1, Debug) << \"Request ctrl: \"\n> > > > > > + << controls::controls.at(ctrlEnum)->name()\n> > > > > > + << \" = \" << ctrlValue.toString();\n> > > > > > +\n> > > > > > + switch (ctrlEnum) {\n> > > > > > + case controls::AF_MODE: {\n> > > > > > + Af *af = getAlgorithm();\n> > > > > > + if (!af) {\n> > > > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_MODE - no AF algorithm\";\n> > > > > > + break;\n> > > > > > + }\n> > > > >\n> > > > > You can get *af once outside of the switch.\n> > > > >\n> > > > > Also, as the failure in getting *af is not related to the control,\n> > > > > there's not much value in duplicating the error message, should\n> > > > > getAlgorithm<>() be made loud on failure so that the caller can skip\n> > > > > doing the same, if not required ?\n> > > >\n> > > > I had actually envisaged handling controls differently, adding a hook to\n> > > > each algorithm called either queueRequest() or processControls() or\n> > > > such, and let each algorithm handle only the controls it uses.\n> > >\n> > > Do you mean https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031931.html\n> > > and https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031932.html ? :-)\n> >\n> > Aha - Yes! I hadn't seen these patches yet.\n> >\n> > I'll head over and find those now. Merging that would help IPU3 already\n> > too I believe (and RKISP here of course).\n>\n> I'll prioritize the series.\n\nI really like the idea of passing controls to each algorithm! It would\nbe great if at the end We could control the algorithms, by just enabling\nthem in the tuning file, without additional changes in the source code.\nWaiting for these changes to be merged!\n\nBest regards\nDaniel\n\n>\n> > > > The drawback there is that if a control goes unhandled, it would be\n> > > > difficult for us to detect or report that, so I think I like this\n> > > > approach too.\n> > >\n> > > We could let the Algorithm::queueRequest() function indicate which\n> > > control(s) it has handled, and verify at the end that all controls have\n> > > been handled.\n> >\n> > Yes, I think I can see workable solutions with that in mind.\n>\n> And will do this on top if/when needed.\n>\n> > > > Especially as you cover that exact condition in the default case below!\n> > > >\n> > > > My only worry is that switch table could get really large though, and\n> > > > the algorithms will have to have a public API to handle all each control\n> > > > specifically which could get extensive.\n> > >\n> > > That's what the RPi IPA module does, and I'm not a big fan of the end\n> > > result.\n> >\n> > 'A really big list' terrifies me of ending up like the Android HAL layer\n> > request processing. I would want to avoid that too.\n> >\n> > > > The only alternative I can think of off the top of my head to consider\n> > > > could be:\n> > > >\n> > > > ControlList afControls;\n> > > > ControlList agcControls;\n> > > >\n> > > > for (auto const control : controls) {\n> > > > case controls::AF_TRIGGER:\n> > > > case controls::AF_PAUSE:\n> > > > afControls.emplace(control);\n> > > > break;\n> > > >\n> > > > case controls::AE_ENABLE:\n> > > > case controls::AE_METERING_MODE:\n> > > > agcControls.emplace(control);\n> > > > break;\n> > > >\n> > > > default:\n> > > > // unhandled control error\n> > > > break;\n> > > > }\n> > > >\n> > > >\n> > > > if (afControls.size()) {\n> > > > // I would probably store all the instantiated algorithms\n> > > > // as a named private pointer variable in the IPARkISP1 class.\n> > > >\n> > > > if (!af_) {\n> > > > LOG(IPARkISP1, Warning) << \"Unhandled AF controls\";\n> > > > } else {\n> > > > af_->setControls(afControls);\n> > > > }\n> > > > }\n> > > >\n> > > > if (agcControls.size()) {\n> > > >\n> > > > if (!agc_) {\n> > > > LOG(IPARkISP1, Warning) << \"Unhandled AGC controls\";\n> > > > } else {\n> > > > agc_->setControls(agcControls);\n> > > > }\n> > > > }\n> > > >\n> > > > Perhaps some of that boilerplate for each algorithm could get mapped\n> > > > down in a template. And also perhaps this introduces more iteration and\n> > > > copying than would be desired too - it's only sketching out an idea to\n> > > > see if it's easier to keep the definition of how controls are handled by\n> > > > algorithms managed by the algorithms themselves.\n> > > >\n> > > > > > +\n> > > > > > + af->setMode(static_cast(ctrlValue.get()));\n> > > > > > + break;\n> > > > > > + }\n> > > > > > + case controls::AF_TRIGGER: {\n> > > > > > + Af *af = getAlgorithm();\n> > > > > > + if (!af) {\n> > > > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > > > + break;\n> > > > > > + }\n> > > > > > +\n> > > > > > + af->setTrigger(static_cast(ctrlValue.get()));\n> > > > > > + break;\n> > > > > > + }\n> > > > > > + case controls::AF_PAUSE: {\n> > > > > > + Af *af = getAlgorithm();\n> > > > > > + if (!af) {\n> > > > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > > > + break;\n> > > > > > + }\n> > > > > > +\n> > > > > > + af->setPause(static_cast(ctrlValue.get()));\n> > > > > > + break;\n> > > > > > + }\n> > > > > > + default:\n> > > > > > + LOG(IPARkISP1, Warning)\n> > > > > > + << \"Ctrl \" << controls::controls.at(ctrlEnum)->name()\n> > > > > > + << \" is not handled.\";\n> > > > > > + break;\n> > > > > > + }\n> > > > > > + }\n> > > > > > }\n> > > > > >\n> > > > > > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 812B0BE173\n\tfor ;\n\tMon, 18 Jul 2022 15:46:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB9A963312;\n\tMon, 18 Jul 2022 17:46:06 +0200 (CEST)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13B536048B\n\tfor ;\n\tMon, 18 Jul 2022 17:46:05 +0200 (CEST)","by mail-lf1-x129.google.com with SMTP id a9so20018985lfk.11\n\tfor ;\n\tMon, 18 Jul 2022 08:46:05 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658159166;\n\tbh=P+t1tWreGkqhfvGNkMWNEg4s9fH7gbuHoOAo1lGssM8=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Bibm5J3mbDdqv4R18V2xP3rcQmj8XrITxzcOpud//uvG0TDyOxJKB3qGm+86WOqCb\n\tfrzOGmaftI7pDdpf1cW3x7AGfUUniQwwxsmvmyksHFRuru46iOIbjnFxLlqeLvgwJg\n\t1qCzMTH19RI7pq8Dobd4x70uvES/csDOJKjtz1XtX73UmTqMbx9QzL7wd79WPxi3Rm\n\toWUTrDQZrf1nEKk5FvYFgVkOV2yDOHqMWw1hTRovIvglIEw8StSsfiBbnxR6q7tqK8\n\trzpaynDMHzv2XvGogC0kgyQRGFYd7xagBSxWP1y8ilAFQggMMd3xikOPwZjJ8ThWsI\n\tJvpuyuhlcY3GA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=YBgMlV+p1WSrdArAOf+UqDBc+vS9aB4LyzWWUUk3KYQ=;\n\tb=oFHwK+9qSY6z62+I/ggUs+DNT5nwAJus3kYS+th8IJeDjH38flubSe3ixyN+yEVAEe\n\tqX9c8yj4YN/tGL08PKM3BAUi4KdUTrTcY/CSbHwT3DNhnytNmdkG7DLPo5IJMO+y4o9Q\n\t64VH1defjX++IBMGUxfCGyDOpIPBhGGlN102fEAxdE0JSEoCxaONKI+ZGZ2JjLfQqkCX\n\tiXyOl1Y7+mFpBjM7T8CA4hKPzusAwd5H16LIBdtdeR51uGxRbBWacj8sncnL8JBf4J+4\n\tH8pjUJnn0fyrBJyTTXh1iI1yclC1RjEaChzVLxqX+44CGagjCS11vIfNxG8tIjDSngB/\n\tlgYA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20210112.gappssmtp.com\n\theader.i=@thaumatec-com.20210112.gappssmtp.com header.b=\"oFHwK+9q\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=YBgMlV+p1WSrdArAOf+UqDBc+vS9aB4LyzWWUUk3KYQ=;\n\tb=Q8XLDOdyTKvwgikwZ6HdDca0Esb8t47w++C+12FeU7f2tDLgBcAR25d4dQSxsOPAVI\n\tjDZXqCgl6PeY++ev/Oq+LKl8s1ZBdLYVRAZKlZq1UR3O6gNnsHvatxB/mMABulF2/w2F\n\tHBM+5u7aLdcwP4XCvg0vjIJdH5LuZ2g46aZNxzr9TF1SUN+B2he8mfeaVu+wF78g5xJV\n\t1McscX7B/8PmQ3KhkxEgMC0H/zs+0lOMP2CKG6kD0yXRPKTA42/FZL9iSkeV/Dnhj/Uo\n\tIKylAzhOeJeCFZOB5LnryCagsRTDLA41B5zN3ra38yJoJpbKBMOPxRg2ReOOcAIIXGfn\n\twmGg==","X-Gm-Message-State":"AJIora84XExWPZdjMPxv9qHDmcngnARKmeQWG6bFG6YJvDe7PyyyG1eD\n\tzhydJcSw1BDG9QdzlBIuGcPL4zS9kTgJHDuHcKs6Dw==","X-Google-Smtp-Source":"AGRyM1uXpezGTbJf4bXBeAmjbdq3WEh5gadfwza/ZTixTkSVAxdGYAD9yxEqTxPSuGYFeYTONvGisZKISA9HeG1Rzxk=","X-Received":"by 2002:a05:6512:1684:b0:47f:5f27:b006 with SMTP id\n\tbu4-20020a056512168400b0047f5f27b006mr15329266lfb.225.1658159164324;\n\tMon, 18 Jul 2022 08:46:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-7-dse@thaumatec.com>\n\t<20220714184652.rb2nflafhtoydmug@uno.localdomain>\n\t<165783167565.3811604.18393336649491601828@Monstersaurus>\n\t\n\t<165788219322.2021905.16479347266969922720@Monstersaurus>\n\t","In-Reply-To":"","Date":"Mon, 18 Jul 2022 17:45:53 +0200","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","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","From":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24048,"web_url":"https://patchwork.libcamera.org/comment/24048/","msgid":"","date":"2022-07-22T00:00:54","subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Daniel,\n\nOn Mon, Jul 18, 2022 at 05:45:53PM +0200, Daniel Semkowicz wrote:\n> On Fri, Jul 15, 2022 at 1:20 PM Laurent Pinchart wrote:\n> > On Fri, Jul 15, 2022 at 11:49:53AM +0100, Kieran Bingham wrote:\n> > > Quoting Laurent Pinchart (2022-07-15 01:38:50)\n> > > > On Thu, Jul 14, 2022 at 09:47:55PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > > > Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:46:52)\n> > > > > > On Wed, Jul 13, 2022 at 10:43:12AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > > Pass the controls set by top level API to the AF algorithm if it\n> > > > > > > was enabled.\n> > > > > > >\n> > > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > > ---\n> > > > > > > src/ipa/rkisp1/rkisp1.cpp | 52 +++++++++++++++++++++++++++++++++++++--\n> > > > > > > 1 file changed, 50 insertions(+), 2 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > index 01bb54fb..53b53f12 100644\n> > > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > @@ -28,6 +28,7 @@\n> > > > > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > > > > #include \"libcamera/internal/yaml_parser.h\"\n> > > > > > >\n> > > > > > > +#include \"algorithms/af.h\"\n> > > > > > > #include \"algorithms/agc.h\"\n> > > > > > > #include \"algorithms/algorithm.h\"\n> > > > > > > #include \"algorithms/awb.h\"\n> > > > > > > @@ -295,9 +296,56 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> > > > > > > }\n> > > > > > >\n> > > > > > > void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,\n> > > > > > > - [[maybe_unused]] const ControlList &controls)\n> > > > > > > + const ControlList &controls)\n> > > > > > > {\n> > > > > > > - /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > > > > > + using namespace algorithms;\n> > > > > > > +\n> > > > > > > + for (auto const &ctrl : controls) {\n> > > > > >\n> > > > > > You can use structured bindings, they're nicer :)\n> > > > > >\n> > > > > > for (auto const &[id, val] : controls) {\n> > > > > >\n> > > > > > }\n> > > > > >\n> > > > > > > + unsigned int ctrlEnum = ctrl.first;\n> > > > > > > + const ControlValue &ctrlValue = ctrl.second;\n> > > > > >\n> > > > > > And drop these\n> > > > > > > +\n> > > > > > > + LOG(IPARkISP1, Debug) << \"Request ctrl: \"\n> > > > > > > + << controls::controls.at(ctrlEnum)->name()\n> > > > > > > + << \" = \" << ctrlValue.toString();\n> > > > > > > +\n> > > > > > > + switch (ctrlEnum) {\n> > > > > > > + case controls::AF_MODE: {\n> > > > > > > + Af *af = getAlgorithm();\n> > > > > > > + if (!af) {\n> > > > > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_MODE - no AF algorithm\";\n> > > > > > > + break;\n> > > > > > > + }\n> > > > > >\n> > > > > > You can get *af once outside of the switch.\n> > > > > >\n> > > > > > Also, as the failure in getting *af is not related to the control,\n> > > > > > there's not much value in duplicating the error message, should\n> > > > > > getAlgorithm<>() be made loud on failure so that the caller can skip\n> > > > > > doing the same, if not required ?\n> > > > >\n> > > > > I had actually envisaged handling controls differently, adding a hook to\n> > > > > each algorithm called either queueRequest() or processControls() or\n> > > > > such, and let each algorithm handle only the controls it uses.\n> > > >\n> > > > Do you mean https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031931.html\n> > > > and https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031932.html ? :-)\n> > >\n> > > Aha - Yes! I hadn't seen these patches yet.\n> > >\n> > > I'll head over and find those now. Merging that would help IPU3 already\n> > > too I believe (and RKISP here of course).\n> >\n> > I'll prioritize the series.\n> \n> I really like the idea of passing controls to each algorithm! It would\n> be great if at the end We could control the algorithms, by just enabling\n> them in the tuning file, without additional changes in the source code.\n> Waiting for these changes to be merged!\n\nDone :-) I've pushed the two patches that add the\nAlgorithm::queueRequest() function and call it from the top level of the\nIPA module. Could you give it a try to see if it solves your problem ?\n\n> > > > > The drawback there is that if a control goes unhandled, it would be\n> > > > > difficult for us to detect or report that, so I think I like this\n> > > > > approach too.\n> > > >\n> > > > We could let the Algorithm::queueRequest() function indicate which\n> > > > control(s) it has handled, and verify at the end that all controls have\n> > > > been handled.\n> > >\n> > > Yes, I think I can see workable solutions with that in mind.\n> >\n> > And will do this on top if/when needed.\n> >\n> > > > > Especially as you cover that exact condition in the default case below!\n> > > > >\n> > > > > My only worry is that switch table could get really large though, and\n> > > > > the algorithms will have to have a public API to handle all each control\n> > > > > specifically which could get extensive.\n> > > >\n> > > > That's what the RPi IPA module does, and I'm not a big fan of the end\n> > > > result.\n> > >\n> > > 'A really big list' terrifies me of ending up like the Android HAL layer\n> > > request processing. I would want to avoid that too.\n> > >\n> > > > > The only alternative I can think of off the top of my head to consider\n> > > > > could be:\n> > > > >\n> > > > > ControlList afControls;\n> > > > > ControlList agcControls;\n> > > > >\n> > > > > for (auto const control : controls) {\n> > > > > case controls::AF_TRIGGER:\n> > > > > case controls::AF_PAUSE:\n> > > > > afControls.emplace(control);\n> > > > > break;\n> > > > >\n> > > > > case controls::AE_ENABLE:\n> > > > > case controls::AE_METERING_MODE:\n> > > > > agcControls.emplace(control);\n> > > > > break;\n> > > > >\n> > > > > default:\n> > > > > // unhandled control error\n> > > > > break;\n> > > > > }\n> > > > >\n> > > > >\n> > > > > if (afControls.size()) {\n> > > > > // I would probably store all the instantiated algorithms\n> > > > > // as a named private pointer variable in the IPARkISP1 class.\n> > > > >\n> > > > > if (!af_) {\n> > > > > LOG(IPARkISP1, Warning) << \"Unhandled AF controls\";\n> > > > > } else {\n> > > > > af_->setControls(afControls);\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > if (agcControls.size()) {\n> > > > >\n> > > > > if (!agc_) {\n> > > > > LOG(IPARkISP1, Warning) << \"Unhandled AGC controls\";\n> > > > > } else {\n> > > > > agc_->setControls(agcControls);\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > Perhaps some of that boilerplate for each algorithm could get mapped\n> > > > > down in a template. And also perhaps this introduces more iteration and\n> > > > > copying than would be desired too - it's only sketching out an idea to\n> > > > > see if it's easier to keep the definition of how controls are handled by\n> > > > > algorithms managed by the algorithms themselves.\n> > > > >\n> > > > > > > +\n> > > > > > > + af->setMode(static_cast(ctrlValue.get()));\n> > > > > > > + break;\n> > > > > > > + }\n> > > > > > > + case controls::AF_TRIGGER: {\n> > > > > > > + Af *af = getAlgorithm();\n> > > > > > > + if (!af) {\n> > > > > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > > > > + break;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + af->setTrigger(static_cast(ctrlValue.get()));\n> > > > > > > + break;\n> > > > > > > + }\n> > > > > > > + case controls::AF_PAUSE: {\n> > > > > > > + Af *af = getAlgorithm();\n> > > > > > > + if (!af) {\n> > > > > > > + LOG(IPARkISP1, Warning) << \"Could not set AF_TRIGGER - no AF algorithm\";\n> > > > > > > + break;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + af->setPause(static_cast(ctrlValue.get()));\n> > > > > > > + break;\n> > > > > > > + }\n> > > > > > > + default:\n> > > > > > > + LOG(IPARkISP1, Warning)\n> > > > > > > + << \"Ctrl \" << controls::controls.at(ctrlEnum)->name()\n> > > > > > > + << \" is not handled.\";\n> > > > > > > + break;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > }\n> > > > > > >\n> > > > > > > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)","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 6D339BE173\n\tfor ;\n\tFri, 22 Jul 2022 00:01:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CEFA63311;\n\tFri, 22 Jul 2022 02:00:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83E2560487\n\tfor ;\n\tFri, 22 Jul 2022 02:00:57 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D2C7E6D5;\n\tFri, 22 Jul 2022 02:00:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658448059;\n\tbh=ra8qR4ovQhTyqOGUnz5X4aVeap2pUf2/1agt65eRLhY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=blGYEvUKwBTpyrK/tIhHdfEMqST5W+S4Q9d0O4ymamJXiIJtlqfk2NFtwSKw8YOnz\n\tA+5wAUNZPHDBNeSXY7YzjoxHi68VI4I+eldU0zfNl53BOWtKkGsQRACD/BKLG8HeXa\n\tUnWwVgRzqZ6+Qwp8xsOWjw8kTxOqnMcunNUvMiNXXbxqebZkF3MtQtzrbjwvkI0kRs\n\t+Wv8az/DMofai6qMbThpDdKK3F81sXIvFqv5jfz2npEJ9efEj+UGW+vU1imwHVku3m\n\twAi2hhiKoIciek7I2MlWVTPs9mBqS8DVNULBgxdjXpuFYuf7l/lzyBOzPrywQDzGEU\n\tqGokwapyVS7hQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658448057;\n\tbh=ra8qR4ovQhTyqOGUnz5X4aVeap2pUf2/1agt65eRLhY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r/zq4TVk0N7xtQ3j37yCxXzPsCZkAYGWGP7ejT+KTNazga1QT7OT/QUDXa57+t7Gn\n\tn6feXk2XWHIo0mnEhY5kive/PVeNg2KIo80oZnbMl/8JTcZ1JhOfMmeUA7R021y0mF\n\tgYJjZSvo6kLwuHGgmiomENuAB5S1AbTCk5QiLKAU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"r/zq4TVk\"; dkim-atps=neutral","Date":"Fri, 22 Jul 2022 03:00:54 +0300","To":"Daniel Semkowicz ","Message-ID":"","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-7-dse@thaumatec.com>\n\t<20220714184652.rb2nflafhtoydmug@uno.localdomain>\n\t<165783167565.3811604.18393336649491601828@Monstersaurus>\n\t\n\t<165788219322.2021905.16479347266969922720@Monstersaurus>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 06/11] ipa: rkisp1: Pass requests\n\tsetting AF controls to the AF algorithm","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]