[{"id":25599,"web_url":"https://patchwork.libcamera.org/comment/25599/","msgid":"<20221026142625.tighzhsfcchjeaad@uno.localdomain>","date":"2022-10-26T14:26:25","subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain and exposure","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Oct 24, 2022 at 03:03:47AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> From: Paul Elder \n>\n> Add support for manual gain and exposure in the rkisp1 IPA.\n>\n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 +-\n> src/ipa/rkisp1/algorithms/agc.cpp | 62 +++++++++++++++++++++---\n> src/ipa/rkisp1/algorithms/agc.h | 4 ++\n> src/ipa/rkisp1/ipa_context.h | 13 ++++-\n> src/ipa/rkisp1/rkisp1.cpp | 17 +++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> 6 files changed, 89 insertions(+), 11 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index eaf3955e4096..b526450d9949 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -10,7 +10,7 @@ import \"include/libcamera/ipa/core.mojom\";\n>\n> interface IPARkISP1Interface {\n> \tinit(libcamera.IPASettings settings,\n> -\t uint32 hwRevision)\n> +\t uint32 hwRevision, libcamera.ControlInfoMap sensorControls)\n> \t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> \tstart() => (int32 ret);\n> \tstop();\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 169afe372803..377a3e8a0efd 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -74,9 +74,14 @@ Agc::Agc()\n> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> {\n> \t/* Configure the default exposure and gain. */\n> -\tcontext.activeState.agc.gain = std::max(context.configuration.sensor.minAnalogueGain,\n> -\t\t\t\t\t\tkMinAnalogueGain);\n> -\tcontext.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;\n> +\tcontext.activeState.agc.automatic.gain =\n> +\t\tstd::max(context.configuration.sensor.minAnalogueGain,\n> +\t\t\t kMinAnalogueGain);\n> +\tcontext.activeState.agc.automatic.exposure =\n> +\t\t10ms / context.configuration.sensor.lineDuration;\n> +\tcontext.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> +\tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> +\tcontext.activeState.agc.autoEnabled = true;\n>\n> \t/*\n> \t * According to the RkISP1 documentation:\n> @@ -108,14 +113,57 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \treturn 0;\n> }\n>\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void Agc::queueRequest(IPAContext &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t const ControlList &controls)\n> +{\n> +\tauto &agc = context.activeState.agc;\n> +\n> +\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> +\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> +\t\tagc.autoEnabled = *agcEnable;\n> +\n> +\t\tLOG(RkISP1Agc, Debug)\n> +\t\t\t<< (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> +\t}\n> +\n> +\tconst auto &exposure = controls.get(controls::ExposureTime);\n> +\tif (exposure && !agc.autoEnabled) {\n> +\t\tagc.manual.exposure = *exposure;\n> +\n> +\t\tLOG(RkISP1Agc, Debug)\n> +\t\t\t<< \"Set exposure to: \" << agc.manual.exposure;\n> +\t}\n> +\n> +\tconst auto &gain = controls.get(controls::AnalogueGain);\n> +\tif (gain && !agc.autoEnabled) {\n> +\t\tagc.manual.gain = *gain;\n> +\n> +\t\tLOG(RkISP1Agc, Debug) << \"Set gain to: \" << agc.manual.gain;\n> +\t}\n> +\n> +\tframeContext.agc.autoEnabled = agc.autoEnabled;\n> +\n> +\tif (!frameContext.agc.autoEnabled) {\n> +\t\tframeContext.agc.exposure = agc.manual.exposure;\n> +\t\tframeContext.agc.gain = agc.manual.gain;\n> +\t}\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> {\n> -\tframeContext.agc.exposure = context.activeState.agc.exposure;\n> -\tframeContext.agc.gain = context.activeState.agc.gain;\n> +\tif (frameContext.agc.autoEnabled) {\n> +\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> +\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> +\t}\n>\n> \tif (frame > 0)\n> \t\treturn;\n> @@ -263,8 +311,8 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t << stepGain;\n>\n> \t/* Update the estimated exposure and gain. */\n> -\tactiveState.agc.exposure = shutterTime / configuration.sensor.lineDuration;\n> -\tactiveState.agc.gain = stepGain;\n> +\tactiveState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> +\tactiveState.agc.automatic.gain = stepGain;\n> }\n>\n> /**\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index da4d2d4e8359..a228d0c37768 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -26,6 +26,10 @@ public:\n> \t~Agc() = default;\n>\n> \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> +\tvoid queueRequest(IPAContext &context,\n> +\t\t\t const uint32_t frame,\n> +\t\t\t IPAFrameContext &frameContext,\n> +\t\t\t const ControlList &controls) override;\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t rkisp1_params_cfg *params) override;\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 3e47ac663c58..b9b2065328d6 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -54,8 +54,16 @@ struct IPASessionConfiguration {\n>\n> struct IPAActiveState {\n> \tstruct {\n> -\t\tuint32_t exposure;\n> -\t\tdouble gain;\n> +\t\tstruct {\n> +\t\t\tuint32_t exposure;\n> +\t\t\tdouble gain;\n> +\t\t} manual;\n> +\t\tstruct {\n> +\t\t\tuint32_t exposure;\n> +\t\t\tdouble gain;\n> +\t\t} automatic;\n> +\n> +\t\tbool autoEnabled;\n> \t} agc;\n>\n> \tstruct {\n> @@ -96,6 +104,7 @@ struct IPAFrameContext : public FrameContext {\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> +\t\tbool autoEnabled;\n> \t} agc;\n>\n> \tstruct {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 538e42cb6f7f..5c041ded0db4 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -50,6 +50,7 @@ public:\n> \tIPARkISP1();\n>\n> \tint init(const IPASettings &settings, unsigned int hwRevision,\n> +\t\t const ControlInfoMap &sensorControls,\n> \t\t ControlInfoMap *ipaControls) override;\n> \tint start() override;\n> \tvoid stop() override;\n> @@ -116,6 +117,7 @@ std::string IPARkISP1::logPrefix() const\n> }\n>\n> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> +\t\t const ControlInfoMap &sensorControls,\n> \t\t ControlInfoMap *ipaControls)\n> {\n> \t/* \\todo Add support for other revisions */\n> @@ -184,6 +186,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n>\n> \t/* Return the controls handled by the IPA. */\n> \tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> +\n> +\tauto exposureTimeInfo = sensorControls.find(V4L2_CID_EXPOSURE);\n> +\tif (exposureTimeInfo != sensorControls.end()) {\n> +\t\tctrlMap.emplace(&controls::ExposureTime,\n> +\t\t\t\tControlInfo(exposureTimeInfo->second.min(),\n> +\t\t\t\t\t exposureTimeInfo->second.max()));\n> +\t}\n> +\n> +\tauto analogueGainInfo = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);\n> +\tif (analogueGainInfo != sensorControls.end()) {\n> +\t\tctrlMap.emplace(&controls::AnalogueGain,\n> +\t\t\t\tControlInfo(static_cast(analogueGainInfo->second.min().get()),\n> +\t\t\t\t\t static_cast(analogueGainInfo->second.max().get())));\n> +\t}\n> +\n\nAt configure() time\n\n\tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n\tif (itExp == ctrls_.end()) {\n\t\tLOG(IPARkISP1, Error) << \"Can't find exposure control\";\n\t\treturn -EINVAL;\n\t}\n\n\tconst auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n\tif (itGain == ctrls_.end()) {\n\t\tLOG(IPARkISP1, Error) << \"Can't find gain control\";\n\t\treturn -EINVAL;\n\t}\n\nCould well fail earlier at init() time ?\n\n> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>\n> \treturn 0;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 455ee2a0a711..1418dc9a47fb 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -348,7 +348,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \t}\n>\n> \tint ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> -\t\t\t &controlInfo_);\n> +\t\t\t sensor_->controls(), &controlInfo_);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;\n> --\n> Regards,\n>\n> Laurent Pinchart\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 9F6B4BDB16\n\tfor ;\n\tWed, 26 Oct 2022 14:26:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7ADC62F55;\n\tWed, 26 Oct 2022 16:26:28 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6E9C61F4B\n\tfor ;\n\tWed, 26 Oct 2022 16:26:27 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 2E0271C0003;\n\tWed, 26 Oct 2022 14:26:26 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666794388;\n\tbh=mfIPN2kvH36xUmpEDW9DwTx0RBkqgGSE1RsMZCxs5wI=;\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=VMvJVPl1jTRaSTeMCv8/OsVIZZdnxBjD3a1L+3+Gt6jhAREtTQV4wdViJZPbc0OIc\n\to39szCHlZ4t9UUKxB4zZZ7s+E5bvwkmNVM0SgqX5tXhoQ2xk8HIwDCdsrUG0l9Cbtj\n\t8YG4dnU1MgBb8Aj4AQ/LFE9un8caMJRHck6lKAj2x7vxkrgKxIzRPgupWmE5oWsxOr\n\t9gHA1YEPMLoq0lqNwj+JnDRo7qqpyGZKcgo4/fPKNY0jD4wiQWwTuIkAtPriFMW3Er\n\tG7/VMBTFfNpsag8Btuoo3GZlgIy3irMncVZmnfd4Ki0f1qoYDpivDPMMCAj9v24Q+O\n\tu9cT1uxeceOPw==","Date":"Wed, 26 Oct 2022 16:26:25 +0200","To":"Laurent Pinchart ","Message-ID":"<20221026142625.tighzhsfcchjeaad@uno.localdomain>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221024000356.29521-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain 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":"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":25675,"web_url":"https://patchwork.libcamera.org/comment/25675/","msgid":"<166699457871.404886.12183097050519912257@Monstersaurus>","date":"2022-10-28T22:02:58","subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain and exposure","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:47)\n> From: Paul Elder \n> \n> Add support for manual gain and exposure in the rkisp1 IPA.\n> \n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 +-\n> src/ipa/rkisp1/algorithms/agc.cpp | 62 +++++++++++++++++++++---\n> src/ipa/rkisp1/algorithms/agc.h | 4 ++\n> src/ipa/rkisp1/ipa_context.h | 13 ++++-\n> src/ipa/rkisp1/rkisp1.cpp | 17 +++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> 6 files changed, 89 insertions(+), 11 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index eaf3955e4096..b526450d9949 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -10,7 +10,7 @@ import \"include/libcamera/ipa/core.mojom\";\n> \n> interface IPARkISP1Interface {\n> init(libcamera.IPASettings settings,\n> - uint32 hwRevision)\n> + uint32 hwRevision, libcamera.ControlInfoMap sensorControls)\n> => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> start() => (int32 ret);\n> stop();\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 169afe372803..377a3e8a0efd 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -74,9 +74,14 @@ 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.sensor.minAnalogueGain,\n> - kMinAnalogueGain);\n> - context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;\n> + context.activeState.agc.automatic.gain =\n> + std::max(context.configuration.sensor.minAnalogueGain,\n> + kMinAnalogueGain);\n> + context.activeState.agc.automatic.exposure =\n> + 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> @@ -108,14 +113,57 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> return 0;\n> }\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void Agc::queueRequest(IPAContext &context,\n> + [[maybe_unused]] const uint32_t frame,\n> + IPAFrameContext &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> + 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> + agc.manual.gain = *gain;\n> +\n> + LOG(RkISP1Agc, Debug) << \"Set gain to: \" << agc.manual.gain;\n> + }\n> +\n> + frameContext.agc.autoEnabled = agc.autoEnabled;\n> +\n> + if (!frameContext.agc.autoEnabled) {\n> + frameContext.agc.exposure = agc.manual.exposure;\n> + frameContext.agc.gain = agc.manual.gain;\n> + }\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> IPAFrameContext &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> \n> if (frame > 0)\n> return;\n> @@ -263,8 +311,8 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &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> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index da4d2d4e8359..a228d0c37768 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -26,6 +26,10 @@ public:\n> ~Agc() = default;\n> \n> int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> + void queueRequest(IPAContext &context,\n> + const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + const ControlList &controls) override;\n> void prepare(IPAContext &context, const uint32_t frame,\n> IPAFrameContext &frameContext,\n> rkisp1_params_cfg *params) override;\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 3e47ac663c58..b9b2065328d6 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -54,8 +54,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> @@ -96,6 +104,7 @@ struct IPAFrameContext : 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 538e42cb6f7f..5c041ded0db4 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -50,6 +50,7 @@ public:\n> IPARkISP1();\n> \n> int init(const IPASettings &settings, unsigned int hwRevision,\n> + const ControlInfoMap &sensorControls,\n> ControlInfoMap *ipaControls) override;\n> int start() override;\n> void stop() override;\n> @@ -116,6 +117,7 @@ std::string IPARkISP1::logPrefix() const\n> }\n> \n> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> + const ControlInfoMap &sensorControls,\n> ControlInfoMap *ipaControls)\n> {\n> /* \\todo Add support for other revisions */\n> @@ -184,6 +186,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \n> /* Return the controls handled by the IPA. */\n> ControlInfoMap::Map ctrlMap = rkisp1Controls;\n> +\n> + auto exposureTimeInfo = sensorControls.find(V4L2_CID_EXPOSURE);\n> + if (exposureTimeInfo != sensorControls.end()) {\n> + ctrlMap.emplace(&controls::ExposureTime,\n> + ControlInfo(exposureTimeInfo->second.min(),\n> + exposureTimeInfo->second.max()));\n> + }\n> +\n> + auto analogueGainInfo = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);\n> + if (analogueGainInfo != sensorControls.end()) {\n> + ctrlMap.emplace(&controls::AnalogueGain,\n> + ControlInfo(static_cast(analogueGainInfo->second.min().get()),\n> + static_cast(analogueGainInfo->second.max().get())));\n> + }\n\nIt's likely a yak - but this is another area I've been thinking about\nlately.\n\nWe're deciding what controls are supported by the algorithms at the top\nlevel, and I think this is probably something we should push down to\neach algorithm to report back. (and do any validation they require).\n\nYet Another Call into each algorithm for init() though?\n\nBut then we could push the algorith to validate it has the information\nit needs from the sensor (and fail early otherwise) and then the ctrlMap\nwould only get populated with controls that are supported by the\nalgorithms. \n\nI.e. - if (for some reason) AGC is disabled, no AGC controls would get\nregistered.\n\n\n> +\n> *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> \n> return 0;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 455ee2a0a711..1418dc9a47fb 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -348,7 +348,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> }\n> \n> int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> - &controlInfo_);\n> + sensor_->controls(), &controlInfo_);\n> if (ret < 0) {\n> LOG(RkISP1, Error) << \"IPA initialization failure\";\n> return ret;\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 E680DBD16B\n\tfor ;\n\tFri, 28 Oct 2022 22:03:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B11E62FED;\n\tSat, 29 Oct 2022 00:03:04 +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 BEDEB62F89\n\tfor ;\n\tSat, 29 Oct 2022 00:03:01 +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 32FB1440;\n\tSat, 29 Oct 2022 00:03:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666994584;\n\tbh=3I95L2rQFkZ0lHK5Nk7FxxC43tug31KVLP55/z4ek9o=;\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=lSNQypMTpteRKkbAYn3EF9PwyRhLXBT1ohv2mYLdUf0IWwb3m5Ph8qCnmE++LAz/w\n\t+GA8jVJ76PpMFPty62DTEIOVofrMAXd4ufsn2JjGEbo5LDJwK88v67D07yFQamObet\n\trbFy9bMDQQZ9volUk/7bnL4xtBw2dsuNbnmD8n+36cmOh9vPae4KHi4HSTYIwSUBLZ\n\tdohGhBIfRH4NSjFvNPGrOALrlVjnxq6akTa+SyivRmFrZ6tP5jePN/vNMaK+UTETum\n\tMBRdK7fepWUqWajGUC7ntY8a/0ftrpVZyCq0xmnZFljsdi8tHv7fI265zuB8TucgE4\n\tinGuYQ1fgoieg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666994581;\n\tbh=3I95L2rQFkZ0lHK5Nk7FxxC43tug31KVLP55/z4ek9o=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=OVDeMbL5cfuNei609Ij/gvzh7MHLJbQOO8NiaSFh3F5ihQLkDPcX0jOpXChrzV+00\n\tnpyxtrzA4kUuYZZ0uRmJ5tN/RbRuBLH9xGLaoamuxovjj1CZoaa1lx0r3i84TMl774\n\to4W5cyZrvd2+6sU+xzPzNjj537TtJvnMsr80VblU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"OVDeMbL5\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221024000356.29521-5-laurent.pinchart@ideasonboard.com>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-5-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 28 Oct 2022 23:02:58 +0100","Message-ID":"<166699457871.404886.12183097050519912257@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain 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\" "}},{"id":25680,"web_url":"https://patchwork.libcamera.org/comment/25680/","msgid":"","date":"2022-10-30T16:44:16","subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain and exposure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Oct 28, 2022 at 11:02:58PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:47)\n> > From: Paul Elder \n> > \n> > Add support for manual gain and exposure in the rkisp1 IPA.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 2 +-\n> > src/ipa/rkisp1/algorithms/agc.cpp | 62 +++++++++++++++++++++---\n> > src/ipa/rkisp1/algorithms/agc.h | 4 ++\n> > src/ipa/rkisp1/ipa_context.h | 13 ++++-\n> > src/ipa/rkisp1/rkisp1.cpp | 17 +++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > 6 files changed, 89 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index eaf3955e4096..b526450d9949 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -10,7 +10,7 @@ import \"include/libcamera/ipa/core.mojom\";\n> > \n> > interface IPARkISP1Interface {\n> > init(libcamera.IPASettings settings,\n> > - uint32 hwRevision)\n> > + uint32 hwRevision, libcamera.ControlInfoMap sensorControls)\n> > => (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > start() => (int32 ret);\n> > stop();\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 169afe372803..377a3e8a0efd 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -74,9 +74,14 @@ 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.sensor.minAnalogueGain,\n> > - kMinAnalogueGain);\n> > - context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;\n> > + context.activeState.agc.automatic.gain =\n> > + std::max(context.configuration.sensor.minAnalogueGain,\n> > + kMinAnalogueGain);\n> > + context.activeState.agc.automatic.exposure =\n> > + 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> > @@ -108,14 +113,57 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > return 0;\n> > }\n> > \n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > + */\n> > +void Agc::queueRequest(IPAContext &context,\n> > + [[maybe_unused]] const uint32_t frame,\n> > + IPAFrameContext &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> > + 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> > + agc.manual.gain = *gain;\n> > +\n> > + LOG(RkISP1Agc, Debug) << \"Set gain to: \" << agc.manual.gain;\n> > + }\n> > +\n> > + frameContext.agc.autoEnabled = agc.autoEnabled;\n> > +\n> > + if (!frameContext.agc.autoEnabled) {\n> > + frameContext.agc.exposure = agc.manual.exposure;\n> > + frameContext.agc.gain = agc.manual.gain;\n> > + }\n> > +}\n> > +\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > */\n> > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > IPAFrameContext &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> > \n> > if (frame > 0)\n> > return;\n> > @@ -263,8 +311,8 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &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> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index da4d2d4e8359..a228d0c37768 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -26,6 +26,10 @@ public:\n> > ~Agc() = default;\n> > \n> > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > + void queueRequest(IPAContext &context,\n> > + const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + const ControlList &controls) override;\n> > void prepare(IPAContext &context, const uint32_t frame,\n> > IPAFrameContext &frameContext,\n> > rkisp1_params_cfg *params) override;\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 3e47ac663c58..b9b2065328d6 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -54,8 +54,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> > @@ -96,6 +104,7 @@ struct IPAFrameContext : 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 538e42cb6f7f..5c041ded0db4 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -50,6 +50,7 @@ public:\n> > IPARkISP1();\n> > \n> > int init(const IPASettings &settings, unsigned int hwRevision,\n> > + const ControlInfoMap &sensorControls,\n> > ControlInfoMap *ipaControls) override;\n> > int start() override;\n> > void stop() override;\n> > @@ -116,6 +117,7 @@ std::string IPARkISP1::logPrefix() const\n> > }\n> > \n> > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > + const ControlInfoMap &sensorControls,\n> > ControlInfoMap *ipaControls)\n> > {\n> > /* \\todo Add support for other revisions */\n> > @@ -184,6 +186,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > \n> > /* Return the controls handled by the IPA. */\n> > ControlInfoMap::Map ctrlMap = rkisp1Controls;\n> > +\n> > + auto exposureTimeInfo = sensorControls.find(V4L2_CID_EXPOSURE);\n> > + if (exposureTimeInfo != sensorControls.end()) {\n> > + ctrlMap.emplace(&controls::ExposureTime,\n> > + ControlInfo(exposureTimeInfo->second.min(),\n> > + exposureTimeInfo->second.max()));\n> > + }\n> > +\n> > + auto analogueGainInfo = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);\n> > + if (analogueGainInfo != sensorControls.end()) {\n> > + ctrlMap.emplace(&controls::AnalogueGain,\n> > + ControlInfo(static_cast(analogueGainInfo->second.min().get()),\n> > + static_cast(analogueGainInfo->second.max().get())));\n> > + }\n> \n> It's likely a yak - but this is another area I've been thinking about\n> lately.\n\nIt's definitely a yak we need to shave. Any volunteer ? :-)\n\n> We're deciding what controls are supported by the algorithms at the top\n> level, and I think this is probably something we should push down to\n> each algorithm to report back. (and do any validation they require).\n> \n> Yet Another Call into each algorithm for init() though?\n\nWe could do that in Algorithm::init(), but we will likely need to update\nthe range of some of the controls at configure() time as well. Maybe\nthat can be handled in a second step ?\n\n> But then we could push the algorith to validate it has the information\n> it needs from the sensor (and fail early otherwise) and then the ctrlMap\n> would only get populated with controls that are supported by the\n> algorithms. \n> \n> I.e. - if (for some reason) AGC is disabled, no AGC controls would get\n> registered.\n> \n> > +\n> > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> > \n> > return 0;\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 455ee2a0a711..1418dc9a47fb 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -348,7 +348,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > }\n> > \n> > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > - &controlInfo_);\n> > + sensor_->controls(), &controlInfo_);\n> > if (ret < 0) {\n> > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > return ret;","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 EA661BDB16\n\tfor ;\n\tSun, 30 Oct 2022 16:44:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E1376301D;\n\tSun, 30 Oct 2022 17:44:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 393E661F47\n\tfor ;\n\tSun, 30 Oct 2022 17:44:46 +0100 (CET)","from pendragon.ideasonboard.com (85-76-1-64-nat.elisa-mobile.fi\n\t[85.76.1.64])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CB93C706;\n\tSun, 30 Oct 2022 17:44:42 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667148288;\n\tbh=3qgmAV9baAov6QvSq9hLZV+KC01wrMojiroDlClJ400=;\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=IlO1t+HwdINCRHXoM4E+x6hPUVSOiV3Ujc4+z09HHAmdv3Muj3C9w7V+kLFJi8OnI\n\t10dq5vZCptSqA9jzYfnK5KKWE2p5KBx2MIWMiVtinEFRwo7OjB1thTCkOXa+tdMCfy\n\tztmQImr+t7U3hHJji3WZmlHD94EdwZvCPI46elyBhgpc8S8ZYWRN0jcnN7IwKRXdQV\n\tYiieH/nTzr13GKVbRwCD+hoSDUiH6UMoEBj3NpurLttF89g6dhBEPLR6PN2NaUQ+sg\n\tKDtejstGVkeviLfp8sZiMitTdS6TXZptp4fNr4XjwBQT/d8j7BkM2knliNAPqoyLd4\n\tYvNIHJREknzuw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1667148285;\n\tbh=3qgmAV9baAov6QvSq9hLZV+KC01wrMojiroDlClJ400=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iC4UiLP2jyTIDO56ov3F904tYFE3g3TmtWYp+UtgBBTRcPKShza/PRfJWv7Uav4d7\n\td8/cSrTFIbSQhOVOjETQYmN4YYOkIiT3JWiK0adnp1dyALPcy8oEtXOVgnhoGlSk76\n\tueZO3i019jXUad1ScV5lurlhCwU4myGWQX+ulG+w="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"iC4UiLP2\"; dkim-atps=neutral","Date":"Sun, 30 Oct 2022 18:44:16 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-5-laurent.pinchart@ideasonboard.com>\n\t<166699457871.404886.12183097050519912257@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166699457871.404886.12183097050519912257@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25682,"web_url":"https://patchwork.libcamera.org/comment/25682/","msgid":"","date":"2022-10-30T16:50:04","subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain and exposure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Oct 26, 2022 at 04:26:25PM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 24, 2022 at 03:03:47AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > From: Paul Elder \n> >\n> > Add support for manual gain and exposure in the rkisp1 IPA.\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 2 +-\n> > src/ipa/rkisp1/algorithms/agc.cpp | 62 +++++++++++++++++++++---\n> > src/ipa/rkisp1/algorithms/agc.h | 4 ++\n> > src/ipa/rkisp1/ipa_context.h | 13 ++++-\n> > src/ipa/rkisp1/rkisp1.cpp | 17 +++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> > 6 files changed, 89 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index eaf3955e4096..b526450d9949 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -10,7 +10,7 @@ import \"include/libcamera/ipa/core.mojom\";\n> >\n> > interface IPARkISP1Interface {\n> > \tinit(libcamera.IPASettings settings,\n> > -\t uint32 hwRevision)\n> > +\t uint32 hwRevision, libcamera.ControlInfoMap sensorControls)\n> > \t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> > \tstart() => (int32 ret);\n> > \tstop();\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 169afe372803..377a3e8a0efd 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -74,9 +74,14 @@ Agc::Agc()\n> > int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > {\n> > \t/* Configure the default exposure and gain. */\n> > -\tcontext.activeState.agc.gain = std::max(context.configuration.sensor.minAnalogueGain,\n> > -\t\t\t\t\t\tkMinAnalogueGain);\n> > -\tcontext.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;\n> > +\tcontext.activeState.agc.automatic.gain =\n> > +\t\tstd::max(context.configuration.sensor.minAnalogueGain,\n> > +\t\t\t kMinAnalogueGain);\n> > +\tcontext.activeState.agc.automatic.exposure =\n> > +\t\t10ms / context.configuration.sensor.lineDuration;\n> > +\tcontext.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> > +\tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > +\tcontext.activeState.agc.autoEnabled = true;\n> >\n> > \t/*\n> > \t * According to the RkISP1 documentation:\n> > @@ -108,14 +113,57 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \treturn 0;\n> > }\n> >\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > + */\n> > +void Agc::queueRequest(IPAContext &context,\n> > +\t\t [[maybe_unused]] const uint32_t frame,\n> > +\t\t IPAFrameContext &frameContext,\n> > +\t\t const ControlList &controls)\n> > +{\n> > +\tauto &agc = context.activeState.agc;\n> > +\n> > +\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> > +\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> > +\t\tagc.autoEnabled = *agcEnable;\n> > +\n> > +\t\tLOG(RkISP1Agc, Debug)\n> > +\t\t\t<< (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> > +\t}\n> > +\n> > +\tconst auto &exposure = controls.get(controls::ExposureTime);\n> > +\tif (exposure && !agc.autoEnabled) {\n> > +\t\tagc.manual.exposure = *exposure;\n> > +\n> > +\t\tLOG(RkISP1Agc, Debug)\n> > +\t\t\t<< \"Set exposure to: \" << agc.manual.exposure;\n> > +\t}\n> > +\n> > +\tconst auto &gain = controls.get(controls::AnalogueGain);\n> > +\tif (gain && !agc.autoEnabled) {\n> > +\t\tagc.manual.gain = *gain;\n> > +\n> > +\t\tLOG(RkISP1Agc, Debug) << \"Set gain to: \" << agc.manual.gain;\n> > +\t}\n> > +\n> > +\tframeContext.agc.autoEnabled = agc.autoEnabled;\n> > +\n> > +\tif (!frameContext.agc.autoEnabled) {\n> > +\t\tframeContext.agc.exposure = agc.manual.exposure;\n> > +\t\tframeContext.agc.gain = agc.manual.gain;\n> > +\t}\n> > +}\n> > +\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > */\n> > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > \t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> > {\n> > -\tframeContext.agc.exposure = context.activeState.agc.exposure;\n> > -\tframeContext.agc.gain = context.activeState.agc.gain;\n> > +\tif (frameContext.agc.autoEnabled) {\n> > +\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> > +\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> > +\t}\n> >\n> > \tif (frame > 0)\n> > \t\treturn;\n> > @@ -263,8 +311,8 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> > \t\t\t << stepGain;\n> >\n> > \t/* Update the estimated exposure and gain. */\n> > -\tactiveState.agc.exposure = shutterTime / configuration.sensor.lineDuration;\n> > -\tactiveState.agc.gain = stepGain;\n> > +\tactiveState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> > +\tactiveState.agc.automatic.gain = stepGain;\n> > }\n> >\n> > /**\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index da4d2d4e8359..a228d0c37768 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -26,6 +26,10 @@ public:\n> > \t~Agc() = default;\n> >\n> > \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> > +\tvoid queueRequest(IPAContext &context,\n> > +\t\t\t const uint32_t frame,\n> > +\t\t\t IPAFrameContext &frameContext,\n> > +\t\t\t const ControlList &controls) override;\n> > \tvoid prepare(IPAContext &context, const uint32_t frame,\n> > \t\t IPAFrameContext &frameContext,\n> > \t\t rkisp1_params_cfg *params) override;\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 3e47ac663c58..b9b2065328d6 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -54,8 +54,16 @@ struct IPASessionConfiguration {\n> >\n> > struct IPAActiveState {\n> > \tstruct {\n> > -\t\tuint32_t exposure;\n> > -\t\tdouble gain;\n> > +\t\tstruct {\n> > +\t\t\tuint32_t exposure;\n> > +\t\t\tdouble gain;\n> > +\t\t} manual;\n> > +\t\tstruct {\n> > +\t\t\tuint32_t exposure;\n> > +\t\t\tdouble gain;\n> > +\t\t} automatic;\n> > +\n> > +\t\tbool autoEnabled;\n> > \t} agc;\n> >\n> > \tstruct {\n> > @@ -96,6 +104,7 @@ struct IPAFrameContext : public FrameContext {\n> > \tstruct {\n> > \t\tuint32_t exposure;\n> > \t\tdouble gain;\n> > +\t\tbool autoEnabled;\n> > \t} agc;\n> >\n> > \tstruct {\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 538e42cb6f7f..5c041ded0db4 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -50,6 +50,7 @@ public:\n> > \tIPARkISP1();\n> >\n> > \tint init(const IPASettings &settings, unsigned int hwRevision,\n> > +\t\t const ControlInfoMap &sensorControls,\n> > \t\t ControlInfoMap *ipaControls) override;\n> > \tint start() override;\n> > \tvoid stop() override;\n> > @@ -116,6 +117,7 @@ std::string IPARkISP1::logPrefix() const\n> > }\n> >\n> > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > +\t\t const ControlInfoMap &sensorControls,\n> > \t\t ControlInfoMap *ipaControls)\n> > {\n> > \t/* \\todo Add support for other revisions */\n> > @@ -184,6 +186,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> >\n> > \t/* Return the controls handled by the IPA. */\n> > \tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> > +\n> > +\tauto exposureTimeInfo = sensorControls.find(V4L2_CID_EXPOSURE);\n> > +\tif (exposureTimeInfo != sensorControls.end()) {\n> > +\t\tctrlMap.emplace(&controls::ExposureTime,\n> > +\t\t\t\tControlInfo(exposureTimeInfo->second.min(),\n> > +\t\t\t\t\t exposureTimeInfo->second.max()));\n> > +\t}\n> > +\n> > +\tauto analogueGainInfo = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);\n> > +\tif (analogueGainInfo != sensorControls.end()) {\n> > +\t\tctrlMap.emplace(&controls::AnalogueGain,\n> > +\t\t\t\tControlInfo(static_cast(analogueGainInfo->second.min().get()),\n> > +\t\t\t\t\t static_cast(analogueGainInfo->second.max().get())));\n> > +\t}\n> > +\n> \n> At configure() time\n> \n> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> \t\tLOG(IPARkISP1, Error) << \"Can't find exposure control\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> \tconst auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> \tif (itGain == ctrls_.end()) {\n> \t\tLOG(IPARkISP1, Error) << \"Can't find gain control\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> Could well fail earlier at init() time ?\n\nI think we should, yes.\n\n> > \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> >\n> > \treturn 0;\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 455ee2a0a711..1418dc9a47fb 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -348,7 +348,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > \t}\n> >\n> > \tint ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > -\t\t\t &controlInfo_);\n> > +\t\t\t sensor_->controls(), &controlInfo_);\n> > \tif (ret < 0) {\n> > \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> > \t\treturn ret;","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 19A52BD16B\n\tfor ;\n\tSun, 30 Oct 2022 16:50:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4343E6301F;\n\tSun, 30 Oct 2022 17:50:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1398461F47\n\tfor ;\n\tSun, 30 Oct 2022 17:50:35 +0100 (CET)","from pendragon.ideasonboard.com (85-76-1-64-nat.elisa-mobile.fi\n\t[85.76.1.64])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E3D5A7C5;\n\tSun, 30 Oct 2022 17:50:28 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667148636;\n\tbh=QVt+A60HJBDJk9PCdiy8+H+DtHm5PlaYpf72Caae0lI=;\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=SiFVZ6jL8skoSTWNhlhafzGRqh5XJy/L/Evt6q8iBZxfbpbjOOHYrvpnW4rqGFjxI\n\t4MUAmIFHtWwzAxhEreYNBvfiTLPdhId5/7gHnd6fwu7rKr/PUi1Qr33xa3K0UFVXJ9\n\tWxY180pPrDYb3LFB+AXIsP0Azvu21kDXa27O2w/oIsF0ZSdIUTBSYFwmM7uTjYt7Nq\n\t77LAdcM/BEyTgerMRh0pJndlCL30XVV45or8VBTXzpAusMsewQr2vnhzlIFf3f2U+Y\n\tJizqSClNIizP+M1L53NTASHGielqRQlMYaIORHfEaJhvfCCqx7M2FtkR2M9mpAKqB5\n\tZG5HphaFWytBA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1667148634;\n\tbh=QVt+A60HJBDJk9PCdiy8+H+DtHm5PlaYpf72Caae0lI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zl5loTPh5tLw2XvjQ9F07vrmyJazCxq/UyjS+n9JslZKs1+GYN/rkN/datEBEZJwT\n\t7TEWbESRiXO1UZJHyjpLRg1y6JbqZnrZ3eCb9p2gwEuXQ23vXwopIyzE62DuRXvsZi\n\tsV3BXG3oDfTlXPHW0oNV95tCsP8AfXbcR1trpbqM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Zl5loTPh\"; dkim-atps=neutral","Date":"Sun, 30 Oct 2022 18:50:04 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-5-laurent.pinchart@ideasonboard.com>\n\t<20221026142625.tighzhsfcchjeaad@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221026142625.tighzhsfcchjeaad@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25861,"web_url":"https://patchwork.libcamera.org/comment/25861/","msgid":"","date":"2022-11-23T09:39:54","subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain and exposure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, Oct 24, 2022 at 03:03:47AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> From: Paul Elder \n> \n> Add support for manual gain and exposure in the rkisp1 IPA.\n> \n> Signed-off-by: Paul Elder \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 +-\n> src/ipa/rkisp1/algorithms/agc.cpp | 62 +++++++++++++++++++++---\n> src/ipa/rkisp1/algorithms/agc.h | 4 ++\n> src/ipa/rkisp1/ipa_context.h | 13 ++++-\n> src/ipa/rkisp1/rkisp1.cpp | 17 +++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-\n> 6 files changed, 89 insertions(+), 11 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index eaf3955e4096..b526450d9949 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -10,7 +10,7 @@ import \"include/libcamera/ipa/core.mojom\";\n> \n> interface IPARkISP1Interface {\n> \tinit(libcamera.IPASettings settings,\n> -\t uint32 hwRevision)\n> +\t uint32 hwRevision, libcamera.ControlInfoMap sensorControls)\n> \t\t=> (int32 ret, libcamera.ControlInfoMap ipaControls);\n> \tstart() => (int32 ret);\n> \tstop();\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 169afe372803..377a3e8a0efd 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -74,9 +74,14 @@ Agc::Agc()\n> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> {\n> \t/* Configure the default exposure and gain. */\n> -\tcontext.activeState.agc.gain = std::max(context.configuration.sensor.minAnalogueGain,\n> -\t\t\t\t\t\tkMinAnalogueGain);\n> -\tcontext.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;\n> +\tcontext.activeState.agc.automatic.gain =\n> +\t\tstd::max(context.configuration.sensor.minAnalogueGain,\n> +\t\t\t kMinAnalogueGain);\n> +\tcontext.activeState.agc.automatic.exposure =\n> +\t\t10ms / context.configuration.sensor.lineDuration;\n> +\tcontext.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> +\tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> +\tcontext.activeState.agc.autoEnabled = true;\n> \n> \t/*\n> \t * According to the RkISP1 documentation:\n> @@ -108,14 +113,57 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void Agc::queueRequest(IPAContext &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t const ControlList &controls)\n> +{\n> +\tauto &agc = context.activeState.agc;\n> +\n> +\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> +\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> +\t\tagc.autoEnabled = *agcEnable;\n> +\n> +\t\tLOG(RkISP1Agc, Debug)\n> +\t\t\t<< (*agcEnable ? \"Enabling\" : \"Disabling\") << \" AGC\";\n> +\t}\n> +\n> +\tconst auto &exposure = controls.get(controls::ExposureTime);\n> +\tif (exposure && !agc.autoEnabled) {\n> +\t\tagc.manual.exposure = *exposure;\n> +\n> +\t\tLOG(RkISP1Agc, Debug)\n> +\t\t\t<< \"Set exposure to: \" << agc.manual.exposure;\n\ns/://\n\nand below too.\n\n> +\t}\n> +\n> +\tconst auto &gain = controls.get(controls::AnalogueGain);\n> +\tif (gain && !agc.autoEnabled) {\n> +\t\tagc.manual.gain = *gain;\n> +\n> +\t\tLOG(RkISP1Agc, Debug) << \"Set gain to: \" << agc.manual.gain;\n> +\t}\n> +\n> +\tframeContext.agc.autoEnabled = agc.autoEnabled;\n> +\n> +\tif (!frameContext.agc.autoEnabled) {\n> +\t\tframeContext.agc.exposure = agc.manual.exposure;\n> +\t\tframeContext.agc.gain = agc.manual.gain;\n> +\t}\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> {\n> -\tframeContext.agc.exposure = context.activeState.agc.exposure;\n> -\tframeContext.agc.gain = context.activeState.agc.gain;\n> +\tif (frameContext.agc.autoEnabled) {\n> +\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> +\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> +\t}\n> \n> \tif (frame > 0)\n> \t\treturn;\n> @@ -263,8 +311,8 @@ void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t << stepGain;\n> \n> \t/* Update the estimated exposure and gain. */\n> -\tactiveState.agc.exposure = shutterTime / configuration.sensor.lineDuration;\n> -\tactiveState.agc.gain = stepGain;\n> +\tactiveState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> +\tactiveState.agc.automatic.gain = stepGain;\n> }\n> \n> /**\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index da4d2d4e8359..a228d0c37768 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -26,6 +26,10 @@ public:\n> \t~Agc() = default;\n> \n> \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> +\tvoid queueRequest(IPAContext &context,\n> +\t\t\t const uint32_t frame,\n> +\t\t\t IPAFrameContext &frameContext,\n> +\t\t\t const ControlList &controls) override;\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t rkisp1_params_cfg *params) override;\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 3e47ac663c58..b9b2065328d6 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -54,8 +54,16 @@ struct IPASessionConfiguration {\n> \n> struct IPAActiveState {\n> \tstruct {\n> -\t\tuint32_t exposure;\n> -\t\tdouble gain;\n> +\t\tstruct {\n> +\t\t\tuint32_t exposure;\n> +\t\t\tdouble gain;\n> +\t\t} manual;\n> +\t\tstruct {\n> +\t\t\tuint32_t exposure;\n> +\t\t\tdouble gain;\n> +\t\t} automatic;\n> +\n> +\t\tbool autoEnabled;\n> \t} agc;\n> \n> \tstruct {\n> @@ -96,6 +104,7 @@ struct IPAFrameContext : public FrameContext {\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> +\t\tbool autoEnabled;\n> \t} agc;\n> \n> \tstruct {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 538e42cb6f7f..5c041ded0db4 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -50,6 +50,7 @@ public:\n> \tIPARkISP1();\n> \n> \tint init(const IPASettings &settings, unsigned int hwRevision,\n> +\t\t const ControlInfoMap &sensorControls,\n> \t\t ControlInfoMap *ipaControls) override;\n> \tint start() override;\n> \tvoid stop() override;\n> @@ -116,6 +117,7 @@ std::string IPARkISP1::logPrefix() const\n> }\n> \n> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> +\t\t const ControlInfoMap &sensorControls,\n> \t\t ControlInfoMap *ipaControls)\n> {\n> \t/* \\todo Add support for other revisions */\n> @@ -184,6 +186,21 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \n> \t/* Return the controls handled by the IPA. */\n> \tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> +\n> +\tauto exposureTimeInfo = sensorControls.find(V4L2_CID_EXPOSURE);\n> +\tif (exposureTimeInfo != sensorControls.end()) {\n\nNo need for curly braces.\n\n> +\t\tctrlMap.emplace(&controls::ExposureTime,\n> +\t\t\t\tControlInfo(exposureTimeInfo->second.min(),\n> +\t\t\t\t\t exposureTimeInfo->second.max()));\n\nTo really benefit from emplace, you should write\n\n\t\tctrlMap.emplace(std::piecewise_construct,\n\t\t\t\tstd::forward_as_tuple(&controls::ExposureTime),\n\t\t\t\tstd::forward_as_tuple(exposureTimeInfo->second.min(),\n\t\t\t\t\t\t exposureTimeInfo->second.max()));\n\nto avoid the construction of a temporary ControlInfo.\n\nThe units are not right for this control, you need to convert it from\nV4L2 units (lines) to libcamera units (time). It may be easier to handle\nthis on top of \"[PATCH 4/4] ipa: rkisp1: add FrameDurationLimits\ncontrol\" which I hope we'll merge quickly.\n\nThis applies to the AnalogueGain control below.\n\nI can handle these issues in v4.\n\n> +\t}\n> +\n> +\tauto analogueGainInfo = sensorControls.find(V4L2_CID_ANALOGUE_GAIN);\n> +\tif (analogueGainInfo != sensorControls.end()) {\n> +\t\tctrlMap.emplace(&controls::AnalogueGain,\n> +\t\t\t\tControlInfo(static_cast(analogueGainInfo->second.min().get()),\n> +\t\t\t\t\t static_cast(analogueGainInfo->second.max().get())));\n> +\t}\n> +\n> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> \n> \treturn 0;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 455ee2a0a711..1418dc9a47fb 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -348,7 +348,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \t}\n> \n> \tint ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> -\t\t\t &controlInfo_);\n> +\t\t\t sensor_->controls(), &controlInfo_);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;","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 3430CBE08B\n\tfor ;\n\tWed, 23 Nov 2022 09:40:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A88463319;\n\tWed, 23 Nov 2022 10:40:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBD536330D\n\tfor ;\n\tWed, 23 Nov 2022 10:40:10 +0100 (CET)","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 57AA988F\n\tfor ;\n\tWed, 23 Nov 2022 10:40:10 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669196411;\n\tbh=C4F780x6hIDGEs+j5db/Fi51Its0Rp9NgASIS6h02C8=;\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:\n\tFrom;\n\tb=2d5I7yBxcZ1vdGt3cehuUYUKPF2gcp1sjHcC55N/Ua0kDXgGEHsiD0lMfrICybZEr\n\tvpulWx0JYtkasohetm0y7/4c7IvYCKKL1rf7dS02zmvxpTiZAoPEIs/crHinH5nAQw\n\t3AHrjeU/D+48su2fl0zm6ji7Z73YycI+A91tUJ/Y3zlJYcDWVHnoKCM8OUlunv9r1D\n\tbPuIjGqJ9WpK9WY41d5PHpx8m5V4O1ko/jdpgbp7KchZBxbIJxRu8rg+ccJucD3epm\n\t8u2KMFFmJfGyc+7T7mbrfsrA9afP1I+O6ElNPcNiyiaJTt6AFDB7wm0634Y8gSxYbt\n\tkbafiBCPPxTlw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669196410;\n\tbh=C4F780x6hIDGEs+j5db/Fi51Its0Rp9NgASIS6h02C8=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=gRYRUsUL5qK75RBG61x/Yk29yrjsXaQschitixz8rWPnawmxT3ATdt1WCvmImLn08\n\tq1sK00uwHcKeS3rmS7MqFK+crs882mjVK0W7V4XVM+/csjexa2EnM/ystBCmU+TrdE\n\tmWnlRwgfktZ5GN5ETDzj+lYF/kHBNRnO/wGoXfys="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gRYRUsUL\"; dkim-atps=neutral","Date":"Wed, 23 Nov 2022 11:39:54 +0200","To":"libcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221024000356.29521-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 04/13] ipa: rkisp1: Add support for\n\tmanual gain 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]