[{"id":25004,"web_url":"https://patchwork.libcamera.org/comment/25004/","msgid":"<166362674576.3399646.15438093086514165456@Monstersaurus>","date":"2022-09-19T22:32:25","subject":"Re: [libcamera-devel] [PATCH] ipa: rkisp1: Add support for manual\n\tgain and exposure","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder via libcamera-devel (2022-09-19 07:37:43)\n> Add support for manual gain and exposure in the rkisp1 IPA.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> This patch depends on v4 of the series: \"ipa: Frame context queue, IPU3\n> & RkISP consolidation, and RkISP1 improvements\"\n\nWill you plan to add the same support to the IPU3 to keep them\nconsistent? Or is that no something you've looked at yet ? (It's fine to\nwait for this to be figured out, but I don't want IPU3 and RKISP to have\ndifferent behaviours if we can avoid it, so it might be that we should\nupdate both at the same time)\n\n\n\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 59 +++++++++++++++++++++++++++----\n> src/ipa/rkisp1/algorithms/agc.h | 4 +++\n> src/ipa/rkisp1/ipa_context.h | 13 +++++--\n> src/ipa/rkisp1/rkisp1.cpp | 3 ++\n> 4 files changed, 71 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index bc764142..d26b35ed 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \"libipa/histogram.h\"\n> @@ -73,8 +74,11 @@ Agc::Agc()\n> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> {\n> /* Configure the default exposure and gain. */\n> - context.activeState.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);\n> - context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;\n> + context.activeState.agc.automatic.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);\n> + context.activeState.agc.automatic.exposure = 10ms / context.configuration.sensor.lineDuration;\n> + context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> + context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> + context.activeState.agc.autoEnabled = true;\n> \n> /*\n> * According to the RkISP1 documentation:\n> @@ -221,8 +225,8 @@ void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,\n> << stepGain;\n> \n> /* Update the estimated exposure and gain. */\n> - activeState.agc.exposure = shutterTime / configuration.sensor.lineDuration;\n> - activeState.agc.gain = stepGain;\n> + activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> + activeState.agc.automatic.gain = stepGain;\n> }\n> \n> /**\n> @@ -331,8 +335,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> {\n> - frameContext.agc.exposure = context.activeState.agc.exposure;\n> - frameContext.agc.gain = context.activeState.agc.gain;\n> + if (frameContext.agc.autoEnabled) {\n> + frameContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> + frameContext.agc.gain = context.activeState.agc.automatic.gain;\n> + }\n\nI'm not quite sure if this is right yet. Will prepare be called at a\ntime that we know these are the exposure and gains applied to the sensor\nin time for the frame associated with frameContext?\n\nIf not, we can still do this as a transitionary change - but we'd need\nto add a todo/comment here.\n\nThe frameContext should reflect what will be/is actually configured for the\nframe. But I know that's not easy right now in regards to the\ndelayedControls implementations...\n\n\n\n\n> \n> if (frame > 0)\n> return;\n> @@ -365,6 +371,47 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> params->module_en_update |= RKISP1_CIF_ISP_MODULE_HST;\n> }\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void Agc::queueRequest(IPAContext &context,\n> + [[maybe_unused]] const uint32_t frame,\n> + RkISP1FrameContext &frameContext,\n> + const ControlList &controls)\n> +{\n> + auto &agc = context.activeState.agc;\n> +\n> + const auto &agcEnable = controls.get(controls::AeEnable);\n> + if (agcEnable && *agcEnable != agc.autoEnabled) {\n> + agc.autoEnabled = *agcEnable;\n> +\n> + LOG(RkISP1Agc, Debug)\n> + << (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> + }\n> +\n> + const auto &exposure = controls.get(controls::ExposureTime);\n> + if (exposure && !agc.autoEnabled) {\n\nI think this shouldn't check !agc.autoEnabled. If a new manual exposure\ntime is passed in, we can update the agc.manual.exposure accordingly.\n\nOr do we explicitly disallow setting manual exposures when AeEnable is\nenabled?\n\nWhat would \n\n Frame 1: AeEnabled = true; ManualExposure = 15ms; \n Frame 2: AeEnabled = false;\n\nBe expected to use for ManualExposure?\n\n\n\n> + agc.manual.exposure = *exposure;\n> +\n> + LOG(RkISP1Agc, Debug)\n> + << \"Set exposure to: \" << agc.manual.exposure;\n> + }\n> +\n> + const auto &gain = controls.get(controls::AnalogueGain);\n> + if (gain && !agc.autoEnabled) {\n\nSame here.... They will only be set to the FrameContext if autoEnabled\nis set below...\n\nBut I guess that means we are either auto AGC or manual exposure &&\ngain.\n\n\n> + agc.manual.gain = *gain;\n> +\n> + LOG(RkISP1Agc, Debug) << \"Set gain to: \" << agc.manual.gain;\n> + }\n> +\n> + frameContext.agc.autoEnabled = agc.autoEnabled;\n\nYes! The FC will certainly take the state of this at this point.\n\n\n> +\n> + if (!frameContext.agc.autoEnabled) {\n> + frameContext.agc.exposure = agc.manual.exposure;\n> + frameContext.agc.gain = agc.manual.gain;\n\nI think this shows we don't yet support automatic gain but fixed\nexposure... (or vice-versa)\n\n\nIs that something we need to consider later? or is there anything we\nneed to do now for that ?\n\n\n\n> + }\n> +}\n> +\n> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> \n> } /* namespace ipa::rkisp1::algorithms */\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index d6c6fb13..e624f101 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -32,6 +32,10 @@ public:\n> void process(IPAContext &context, const uint32_t frame,\n> RkISP1FrameContext &frameContext,\n> const rkisp1_stat_buffer *stats) override;\n> + void queueRequest(IPAContext &context,\n> + const uint32_t frame,\n> + RkISP1FrameContext &frameContext,\n> + const ControlList &controls) override;\n> \n> private:\n> void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index d0bc9090..df72ec87 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -50,8 +50,16 @@ struct IPASessionConfiguration {\n> \n> struct IPAActiveState {\n> struct {\n> - uint32_t exposure;\n> - double gain;\n> + struct {\n> + uint32_t exposure;\n> + double gain;\n> + } manual;\n> + struct {\n> + uint32_t exposure;\n> + double gain;\n> + } automatic;\n> +\n> + bool autoEnabled;\n> } agc;\n> \n> struct {\n> @@ -92,6 +100,7 @@ struct RkISP1FrameContext : public FrameContext {\n> struct {\n> uint32_t exposure;\n> double gain;\n> + bool autoEnabled;\n> } agc;\n> \n> struct {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 75370ac8..0092d0a1 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -92,8 +92,11 @@ private:\n> namespace {\n> \n> /* List of controls handled by the RkISP1 IPA */\n> +/* \\todo Report more accurate limits for exposure and gain */\n\nI'd also like to see this table get moved so that each algorithm\ngenerates the entries... which might have to be another callback on the\nmodule or handled at init() time or such perhaps. But not required for\nthis patch.\n\n\n> const ControlInfoMap::Map rkisp1Controls{\n> { &controls::AeEnable, ControlInfo(false, true) },\n> + { &controls::ExposureTime, ControlInfo(0, 66666) },\n> + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n> { &controls::AwbEnable, ControlInfo(false, true) },\n> { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n> { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n> -- \n> 2.30.2\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 75727C0DA4\n\tfor ;\n\tMon, 19 Sep 2022 22:32:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89EFA62199;\n\tTue, 20 Sep 2022 00:32:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3366061FAC\n\tfor ;\n\tTue, 20 Sep 2022 00:32:29 +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 9A4426DB;\n\tTue, 20 Sep 2022 00:32:28 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663626751;\n\tbh=526DmxXdbJtTS6vnq+jXNkGhhEnBWKtx9L0oVMcxe7g=;\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:\n\tFrom;\n\tb=p4OKiLIt1Z/SV2Fcdus470/7/7LrTMDEpBS9Z+CCOj7KnVYWdgjeiQZSYCFm0kIAX\n\tc2VbeRgljvaDY3T6VtP94ldN62qW+jQKKwi1QeNks7t06xnDxoPuQbqIRtj6c4Nxd/\n\tCwnisYtedhiglHaEMD5KutpYGfjVyAcCsLYC4JM1nVttnIU5uJJUVG1kOOo1zO3Uby\n\t3cdO9kiMZMOa6haXvahn1m3JFXUkiogmqzEhArmmcPUHEGJiWUEnMBmStdyaXbnAwE\n\tb6p1TbuQ5uBdG4UrGcWS3yDoa/hI50Ajk3O1C/2Fc3AIR+r3pnNCvGnylinRwXLjDJ\n\t9LEnV2gGLiY+A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663626748;\n\tbh=526DmxXdbJtTS6vnq+jXNkGhhEnBWKtx9L0oVMcxe7g=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=kSOs2RrGdMWOiE4I9UmgNxgywwOA2HqaJA0ljl2F4UQwteoVxxZ2SRInPFLPplCAA\n\t+4/YLlXbJXUKEDdc6w6lWyUvlZOVIfHyv3q9s+BpkJu4rRFSwCMmr3udK+0wIqw6Pi\n\tjjQTr5WcNSU+d4ZKq5IwMr0mowJ3AVhr2gTZF2y8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"kSOs2RrG\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220919063743.2753465-1-paul.elder@ideasonboard.com>","References":"<20220919063743.2753465-1-paul.elder@ideasonboard.com>","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 19 Sep 2022 23:32:25 +0100","Message-ID":"<166362674576.3399646.15438093086514165456@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] ipa: rkisp1: Add support for manual\n\tgain and exposure","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]