[{"id":25455,"web_url":"https://patchwork.libcamera.org/comment/25455/","msgid":"<20221019085946.djzbneswo44zotg2@uno.localdomain>","date":"2022-10-19T08:59:46","subject":"Re: [libcamera-devel] [PATCH v3] 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 Paul\n\nOn Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:\n> Add support for manual gain and exposure in the rkisp1 IPA.\n>\n\nI wish we could base this on the new AEGC controls\n\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v3:\n> - Report the exposure time and analogue gain in metadata\n>\n> Changes in v2:\n> - set the min and max values of ExposureTime and AnalogueGain properly\n> - to that end, plumb the sensorControls through the pipeline handler's\n> loadIPA() and the IPA's init()\n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 +-\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 | 21 +++++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--\n> 6 files changed, 95 insertions(+), 13 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index eaf3955e..b526450d 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 04062a36..09ec6ac4 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> \t/* Configure the default exposure and gain. */\n> -\tcontext.activeState.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);\n> -\tcontext.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;\n> +\tcontext.activeState.agc.automatic.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);\n> +\tcontext.activeState.agc.automatic.exposure = 10ms / 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> @@ -221,8 +225,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> @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> @@ -373,6 +379,47 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \tparams->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> +\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\nThis seems correct to me!\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 9ad5c32f..ebceeef4 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -32,6 +32,10 @@ public:\n> \tvoid process(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t const rkisp1_stat_buffer *stats) 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>\n> private:\n> \tvoid computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index c85d8abe..035d67e2 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> \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> @@ -92,6 +100,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 3f5c1a58..33692e5d 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -49,6 +49,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> @@ -183,6 +185,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\nWe have a list of mandatory controls in camera_sensor.cpp, among which\nwe have CID_EXPOSURE but not CID_ANALOGUE_GAIN.\n\nI wonder if we shouldn't make it mandatory too.\n\nApart from that, I wonder if the IPA can assume the pipeline handler\nhas already validated the sensor driver by using CameraSensor or it\nshould re-check here.\n\n> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>\n> \treturn 0;\n> @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)\n>\n> void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> {\n> +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \tControlList ctrls(controls::controls);\n>\n> \tif (aeState)\n> \t\tctrls.set(controls::AeLocked, aeState == 2);\n>\n> +\tctrls.set(controls::ExposureTime, frameContext.agc.exposure);\n> +\tctrls.set(controls::AnalogueGain, frameContext.agc.gain);\n> +\n> \tmetadataReady.emit(frame, ctrls);\n> }\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 455ee2a0..4f8e467a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -91,7 +91,7 @@ public:\n> \t}\n>\n> \tPipelineHandlerRkISP1 *pipe();\n> -\tint loadIPA(unsigned int hwRevision);\n> +\tint loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);\n>\n> \tStream mainPathStream_;\n> \tStream selfPathStream_;\n> @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> \treturn static_cast(Camera::Private::pipe());\n> }\n>\n> -int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> +int RkISP1CameraData::loadIPA(unsigned int hwRevision,\n> +\t\t\t const ControlInfoMap &sensorControls)\n\nDo you need to pass the controls list in or can you retrieve it from\nthe RkISP1CameraData::sensor_ class member ?\n\n> {\n> \tipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> \tif (!ipa_)\n> @@ -348,7 +349,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 sensorControls, &controlInfo_);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;\n> @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tisp_->frameStart.connect(data->delayedCtrls_.get(),\n> \t\t\t\t &DelayedControls::applyControls);\n>\n> -\tret = data->loadIPA(media_->hwRevision());\n> +\tret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());\n> \tif (ret)\n> \t\treturn ret;\n>\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 45A42C0DA4\n\tfor ;\n\tWed, 19 Oct 2022 08:59:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 857FF62E3D;\n\tWed, 19 Oct 2022 10:59:52 +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 9E44D62DD7\n\tfor ;\n\tWed, 19 Oct 2022 10:59:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4D07F1C0003;\n\tWed, 19 Oct 2022 08:59:48 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666169992;\n\tbh=Dq78+F3p5BuPNYJd+CvxOtu9hBsaV5O38anZ3VfkIq0=;\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=v/UDH+qTvhbnnWxcNwL52h14dPeBCaXU/TKWUMdXn/OKd2xU4KokiCnehCnrOAIOu\n\tqzQXFicC+t9OdaQ8x0Q5he7yACnudrz5n/GV4wk2BUtOK1YmFbfTrG5uAr9LHPEET1\n\t1DgpC9BcLM/bI2IHgeiau2IqYnTGqdgG60LZ2AALpsfUBbqS/txfYMxvJlXjeCdQtw\n\tZkNb32venBurV7vUPUhmPHfmvF1OLEH7ZU9lQDzniVb6yBRVSQ0YyYKMk11bsX3XFz\n\tvvHgMeDeGD+SWLkvzYyD82vHUUn1Lqf1vai9e0kqYJK19vHsfu7huiXEnyMv/BAg3Q\n\tBugxDkbFuR4HQ==","Date":"Wed, 19 Oct 2022 10:59:46 +0200","To":"Paul Elder ","Message-ID":"<20221019085946.djzbneswo44zotg2@uno.localdomain>","References":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3] 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":25456,"web_url":"https://patchwork.libcamera.org/comment/25456/","msgid":"<166617091299.2560709.932874122358043735@Monstersaurus>","date":"2022-10-19T09:15:12","subject":"Re: [libcamera-devel] [PATCH v3] 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 Jacopo Mondi via libcamera-devel (2022-10-19 09:59:46)\n> Hi Paul\n> \n> On Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:\n> > Add support for manual gain and exposure in the rkisp1 IPA.\n> >\n> \n> I wish we could base this on the new AEGC controls\n\nLikewise.\n\n> \n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v3:\n> > - Report the exposure time and analogue gain in metadata\n> >\n> > Changes in v2:\n> > - set the min and max values of ExposureTime and AnalogueGain properly\n> > - to that end, plumb the sensorControls through the pipeline handler's\n> > loadIPA() and the IPA's init()\n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 2 +-\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 | 21 +++++++++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--\n> > 6 files changed, 95 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index eaf3955e..b526450d 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 04062a36..09ec6ac4 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, 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> > @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> > @@ -373,6 +379,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> > + 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\nI've wondered if a helpful pattern on these might be:\n\n\tbool agcEnable = controls.get(controls::AeEnable)\n\t\t\t\t .value_or(agc.autoEnabled);\n\tif (agcEnable != agc.autoEnabled) {\n\t\tagc.autoEnabled = agcEnable;\n\t\tLOG(...)\n\t}\n\nBut it's not specifically better than what you have.\n\n\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\nBut this looks good, and can't use the .value_or() anyway.\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> This seems correct to me!\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 9ad5c32f..ebceeef4 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> > IPAFrameContext &frameContext,\n> > const rkisp1_stat_buffer *stats) override;\n> > + void queueRequest(IPAContext &context,\n> > + const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + const ControlList &controls) override;\n> >\n> > private:\n> > void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index c85d8abe..035d67e2 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 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 3f5c1a58..33692e5d 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -49,6 +49,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> > @@ -183,6 +185,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> \n> We have a list of mandatory controls in camera_sensor.cpp, among which\n> we have CID_EXPOSURE but not CID_ANALOGUE_GAIN.\n> \n> I wonder if we shouldn't make it mandatory too.\n> \n> Apart from that, I wonder if the IPA can assume the pipeline handler\n> has already validated the sensor driver by using CameraSensor or it\n> should re-check here.\n> \n> > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> >\n> > return 0;\n> > @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)\n> >\n> > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > {\n> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > ControlList ctrls(controls::controls);\n> >\n> > if (aeState)\n> > ctrls.set(controls::AeLocked, aeState == 2);\n\nNot your patch - but I don't like seeing aeState==2 ... What is 2 ?\n\n\n> >\n> > + ctrls.set(controls::ExposureTime, frameContext.agc.exposure);\n> > + ctrls.set(controls::AnalogueGain, frameContext.agc.gain);\n> > +\n> > metadataReady.emit(frame, ctrls);\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 455ee2a0..4f8e467a 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -91,7 +91,7 @@ public:\n> > }\n> >\n> > PipelineHandlerRkISP1 *pipe();\n> > - int loadIPA(unsigned int hwRevision);\n> > + int loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);\n> >\n> > Stream mainPathStream_;\n> > Stream selfPathStream_;\n> > @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> > return static_cast(Camera::Private::pipe());\n> > }\n> >\n> > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > +int RkISP1CameraData::loadIPA(unsigned int hwRevision,\n> > + const ControlInfoMap &sensorControls)\n> \n> Do you need to pass the controls list in or can you retrieve it from\n> the RkISP1CameraData::sensor_ class member ?\n> \n> > {\n> > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > if (!ipa_)\n> > @@ -348,7 +349,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > }\n> >\n> > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > - &controlInfo_);\n> > + sensorControls, &controlInfo_);\n> > if (ret < 0) {\n> > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > return ret;\n> > @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > &DelayedControls::applyControls);\n> >\n> > - ret = data->loadIPA(media_->hwRevision());\n> > + ret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());\n> > if (ret)\n> > return ret;\n> >\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 F2449BD16B\n\tfor ;\n\tWed, 19 Oct 2022 09:15:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 510FA62E3F;\n\tWed, 19 Oct 2022 11:15:17 +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 23CF162E37\n\tfor ;\n\tWed, 19 Oct 2022 11:15:16 +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 5EC2C5A4;\n\tWed, 19 Oct 2022 11:15:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666170917;\n\tbh=kwncpZUEB81ikK3TL7XbYlPEeF/icQ1TrDLgkpt/f7E=;\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=lcr2eOYOZxy96E8OaZWGvmI9hzNhHVwJ8q8aCSlNRyT0aUmMT5H1/zH50NVYgbntE\n\tC//vqaskfeKXKRrv8RaPNKHERGd0AROsOsNHuowPK/2Zi+jwAozVwzPEmV6FjQs9xW\n\tLLFAaSPEBGxFquthsktuRxzeY27SXeNbO8ADd6e8wo8mo2RZHk6IizNtslr+3supQ+\n\t4kjfGRbf4uUfZNaToPszOOhpalv2pUYdlEyRHc0AZwbuHZwCNQuHz/ZlM1IrJnq3oI\n\tJQ+ZpqlElHDWX8aRmuILxNEVk2ymZi88GWTLBlYgqEt4/WbCOkqQHfNg2tK7dkJQba\n\td+xVx+d7iKPEQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666170915;\n\tbh=kwncpZUEB81ikK3TL7XbYlPEeF/icQ1TrDLgkpt/f7E=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=uCTH5GIeXY7YwAMaiKmt/gdYf4Q3AvLFAsNp3dOMsgaATpN9stE75Y/PUC6WYH6ki\n\taByE/svezzlpJMSR166gk19PukObpHxqImDaM/c9HqKtsuCEFIJq4AiJ3i3xL1H1bK\n\tu77RwD43eaCIQDa5417cE3kTcxpKN169QFeNCug0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uCTH5GIe\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221019085946.djzbneswo44zotg2@uno.localdomain>","References":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>\n\t<20221019085946.djzbneswo44zotg2@uno.localdomain>","To":"Jacopo Mondi , Jacopo Mondi via libcamera-devel\n\t, \n\tPaul Elder ","Date":"Wed, 19 Oct 2022 10:15:12 +0100","Message-ID":"<166617091299.2560709.932874122358043735@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3] 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 ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25457,"web_url":"https://patchwork.libcamera.org/comment/25457/","msgid":"<20221019093713.cwojew3pzl25ylrn@uno.localdomain>","date":"2022-10-19T09:37:13","subject":"Re: [libcamera-devel] [PATCH v3] 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 Kieran\n\nOn Wed, Oct 19, 2022 at 10:15:12AM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi via libcamera-devel (2022-10-19 09:59:46)\n> > Hi Paul\n> >\n> > On Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:\n> > > Add support for manual gain and exposure in the rkisp1 IPA.\n> > >\n> >\n> > I wish we could base this on the new AEGC controls\n>\n> Likewise.\n>\n> >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v3:\n> > > - Report the exposure time and analogue gain in metadata\n> > >\n> > > Changes in v2:\n> > > - set the min and max values of ExposureTime and AnalogueGain properly\n> > > - to that end, plumb the sensorControls through the pipeline handler's\n> > > loadIPA() and the IPA's init()\n> > > ---\n> > > include/libcamera/ipa/rkisp1.mojom | 2 +-\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 | 21 +++++++++\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--\n> > > 6 files changed, 95 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > index eaf3955e..b526450d 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 04062a36..09ec6ac4 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, 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> > > @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> > > @@ -373,6 +379,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> > > + 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> I've wondered if a helpful pattern on these might be:\n>\n> \tbool agcEnable = controls.get(controls::AeEnable)\n> \t\t\t\t .value_or(agc.autoEnabled);\n> \tif (agcEnable != agc.autoEnabled) {\n> \t\tagc.autoEnabled = agcEnable;\n> \t\tLOG(...)\n> \t}\n>\n> But it's not specifically better than what you have.\n>\n>\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>\n> But this looks good, and can't use the .value_or() anyway.\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> > This seems correct to me!\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 9ad5c32f..ebceeef4 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> > > IPAFrameContext &frameContext,\n> > > const rkisp1_stat_buffer *stats) override;\n> > > + void queueRequest(IPAContext &context,\n> > > + const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + const ControlList &controls) override;\n> > >\n> > > private:\n> > > void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index c85d8abe..035d67e2 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 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 3f5c1a58..33692e5d 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -49,6 +49,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> > > @@ -183,6 +185,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> >\n> > We have a list of mandatory controls in camera_sensor.cpp, among which\n> > we have CID_EXPOSURE but not CID_ANALOGUE_GAIN.\n> >\n> > I wonder if we shouldn't make it mandatory too.\n> >\n> > Apart from that, I wonder if the IPA can assume the pipeline handler\n> > has already validated the sensor driver by using CameraSensor or it\n> > should re-check here.\n> >\n> > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> > >\n> > > return 0;\n> > > @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)\n> > >\n> > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > {\n> > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > ControlList ctrls(controls::controls);\n> > >\n> > > if (aeState)\n> > > ctrls.set(controls::AeLocked, aeState == 2);\n>\n> Not your patch - but I don't like seeing aeState==2 ... What is 2 ?\n>\n>\n\nThis has been bothering me as well, and I thought I had a patch in my\ntree somewhere to drop the whole prepareMetadata() function as (before\nthis patch) it was just a stub.\n\nI think as part of this patch the aeState argument can now be dropped.\n\nAlso consider it is always called as:\n\n\tunsigned int aeState = 0;\n\n\tfor (auto const &algo : algorithms())\n\t\talgo->process(context_, frame, frameContext, stats);\n\n\tsetControls(frame);\n\n\tprepareMetadata(frame, aeState);\n\nSo the argument is always effectively zero. It feels like a leftover\nfrom some initial IPA skeleton.\n\n> > >\n> > > + ctrls.set(controls::ExposureTime, frameContext.agc.exposure);\n> > > + ctrls.set(controls::AnalogueGain, frameContext.agc.gain);\n> > > +\n> > > metadataReady.emit(frame, ctrls);\n> > > }\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 455ee2a0..4f8e467a 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -91,7 +91,7 @@ public:\n> > > }\n> > >\n> > > PipelineHandlerRkISP1 *pipe();\n> > > - int loadIPA(unsigned int hwRevision);\n> > > + int loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);\n> > >\n> > > Stream mainPathStream_;\n> > > Stream selfPathStream_;\n> > > @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> > > return static_cast(Camera::Private::pipe());\n> > > }\n> > >\n> > > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > +int RkISP1CameraData::loadIPA(unsigned int hwRevision,\n> > > + const ControlInfoMap &sensorControls)\n> >\n> > Do you need to pass the controls list in or can you retrieve it from\n> > the RkISP1CameraData::sensor_ class member ?\n> >\n> > > {\n> > > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > > if (!ipa_)\n> > > @@ -348,7 +349,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > }\n> > >\n> > > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > > - &controlInfo_);\n> > > + sensorControls, &controlInfo_);\n> > > if (ret < 0) {\n> > > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > > return ret;\n> > > @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > > &DelayedControls::applyControls);\n> > >\n> > > - ret = data->loadIPA(media_->hwRevision());\n> > > + ret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());\n> > > if (ret)\n> > > return ret;\n> > >\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 24220C0DA4\n\tfor ;\n\tWed, 19 Oct 2022 09:37:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 680F862E41;\n\tWed, 19 Oct 2022 11:37:18 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D71BF62E37\n\tfor ;\n\tWed, 19 Oct 2022 11:37:16 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id CF50F6000B;\n\tWed, 19 Oct 2022 09:37:15 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666172238;\n\tbh=qV7khiDTLP+TBLpioDFvbnFnF0LgpYI8UDA3rRlSIVM=;\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=1RrKQFqAJGyYQuQIE6TDgdXaclucecAHNVBKgm+fZchxqQBnemBGarpj4cDjF2VB0\n\tL1kCE1JALP7qdWYGuelXE7k2VA8see3RUh92KFD3PvG1AP6UonAdhTZR0J1DHyP6zv\n\tNyySSOiwMWSmjowbQW5SEFEeS5Ff6ud1UCiOTyJkb5kAbeASvqpk5euVkUVPDCehyT\n\tgrD6Jcv1zSjNCsQVpgV43H1N7D2kV3Lj/A+LtSFgx2Ii+MgqUyeyc0gMZFb/YQmfQL\n\ttx6t+yAluS7EfAkEGnZyINLzMC9vLCjgXRIhKaOmTXBnAPAop6UxillqaD4fDO940t\n\t0t9f0wbGZRS0w==","Date":"Wed, 19 Oct 2022 11:37:13 +0200","To":"Kieran Bingham ","Message-ID":"<20221019093713.cwojew3pzl25ylrn@uno.localdomain>","References":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>\n\t<20221019085946.djzbneswo44zotg2@uno.localdomain>\n\t<166617091299.2560709.932874122358043735@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166617091299.2560709.932874122358043735@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3] 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":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25461,"web_url":"https://patchwork.libcamera.org/comment/25461/","msgid":"<166617256494.2560709.10074743941317017767@Monstersaurus>","date":"2022-10-19T09:42:44","subject":"Re: [libcamera-devel] [PATCH v3] 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 Jacopo Mondi (2022-10-19 10:37:13)\n> Hi Kieran\n> \n> On Wed, Oct 19, 2022 at 10:15:12AM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2022-10-19 09:59:46)\n> > > Hi Paul\n> > >\n> > > On Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:\n> > > > Add support for manual gain and exposure in the rkisp1 IPA.\n> > > >\n> > >\n> > > I wish we could base this on the new AEGC controls\n> >\n> > Likewise.\n> >\n> > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > Changes in v3:\n> > > > - Report the exposure time and analogue gain in metadata\n> > > >\n> > > > Changes in v2:\n> > > > - set the min and max values of ExposureTime and AnalogueGain properly\n> > > > - to that end, plumb the sensorControls through the pipeline handler's\n> > > > loadIPA() and the IPA's init()\n> > > > ---\n> > > > include/libcamera/ipa/rkisp1.mojom | 2 +-\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 | 21 +++++++++\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--\n> > > > 6 files changed, 95 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > index eaf3955e..b526450d 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 04062a36..09ec6ac4 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, 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> > > > @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> > > > @@ -373,6 +379,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> > > > + 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> > I've wondered if a helpful pattern on these might be:\n> >\n> > bool agcEnable = controls.get(controls::AeEnable)\n> > .value_or(agc.autoEnabled);\n> > if (agcEnable != agc.autoEnabled) {\n> > agc.autoEnabled = agcEnable;\n> > LOG(...)\n> > }\n> >\n> > But it's not specifically better than what you have.\n> >\n> >\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> >\n> > But this looks good, and can't use the .value_or() anyway.\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> > > This seems correct to me!\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 9ad5c32f..ebceeef4 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> > > > IPAFrameContext &frameContext,\n> > > > const rkisp1_stat_buffer *stats) override;\n> > > > + void queueRequest(IPAContext &context,\n> > > > + const uint32_t frame,\n> > > > + IPAFrameContext &frameContext,\n> > > > + const ControlList &controls) override;\n> > > >\n> > > > private:\n> > > > void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index c85d8abe..035d67e2 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 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 3f5c1a58..33692e5d 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -49,6 +49,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> > > > @@ -183,6 +185,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> > >\n> > > We have a list of mandatory controls in camera_sensor.cpp, among which\n> > > we have CID_EXPOSURE but not CID_ANALOGUE_GAIN.\n> > >\n> > > I wonder if we shouldn't make it mandatory too.\n> > >\n> > > Apart from that, I wonder if the IPA can assume the pipeline handler\n> > > has already validated the sensor driver by using CameraSensor or it\n> > > should re-check here.\n> > >\n> > > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> > > >\n> > > > return 0;\n> > > > @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > >\n> > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > {\n> > > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > ControlList ctrls(controls::controls);\n> > > >\n> > > > if (aeState)\n> > > > ctrls.set(controls::AeLocked, aeState == 2);\n> >\n> > Not your patch - but I don't like seeing aeState==2 ... What is 2 ?\n> >\n> >\n> \n> This has been bothering me as well, and I thought I had a patch in my\n> tree somewhere to drop the whole prepareMetadata() function as (before\n> this patch) it was just a stub.\n> \n> I think as part of this patch the aeState argument can now be dropped.\n> \n> Also consider it is always called as:\n> \n> unsigned int aeState = 0;\n> \n> for (auto const &algo : algorithms())\n> algo->process(context_, frame, frameContext, stats);\n> \n> setControls(frame);\n> \n> prepareMetadata(frame, aeState);\n> \n> So the argument is always effectively zero. It feels like a leftover\n> from some initial IPA skeleton.\n\nOk - lets get rid of it. Who's going to post the patch. Let me know if\nyou want me to do it.\n--\nKieran\n\n\n> \n> > > >\n> > > > + ctrls.set(controls::ExposureTime, frameContext.agc.exposure);\n> > > > + ctrls.set(controls::AnalogueGain, frameContext.agc.gain);\n> > > > +\n> > > > metadataReady.emit(frame, ctrls);\n> > > > }\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 455ee2a0..4f8e467a 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -91,7 +91,7 @@ public:\n> > > > }\n> > > >\n> > > > PipelineHandlerRkISP1 *pipe();\n> > > > - int loadIPA(unsigned int hwRevision);\n> > > > + int loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);\n> > > >\n> > > > Stream mainPathStream_;\n> > > > Stream selfPathStream_;\n> > > > @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> > > > return static_cast(Camera::Private::pipe());\n> > > > }\n> > > >\n> > > > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > +int RkISP1CameraData::loadIPA(unsigned int hwRevision,\n> > > > + const ControlInfoMap &sensorControls)\n> > >\n> > > Do you need to pass the controls list in or can you retrieve it from\n> > > the RkISP1CameraData::sensor_ class member ?\n> > >\n> > > > {\n> > > > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > > > if (!ipa_)\n> > > > @@ -348,7 +349,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > }\n> > > >\n> > > > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > > > - &controlInfo_);\n> > > > + sensorControls, &controlInfo_);\n> > > > if (ret < 0) {\n> > > > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > > > return ret;\n> > > > @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > > > &DelayedControls::applyControls);\n> > > >\n> > > > - ret = data->loadIPA(media_->hwRevision());\n> > > > + ret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());\n> > > > if (ret)\n> > > > return ret;\n> > > >\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 4CC94BD16B\n\tfor ;\n\tWed, 19 Oct 2022 09:42:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE7F562E49;\n\tWed, 19 Oct 2022 11:42:49 +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 23AD062E37\n\tfor ;\n\tWed, 19 Oct 2022 11:42:48 +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 7AF805A4;\n\tWed, 19 Oct 2022 11:42:47 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666172569;\n\tbh=gqdODNO8FwwXzy1qWiEFLSxK4swj1IWLH/KH2JrD6pk=;\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=USbMe9aJr3Nt0+9u17zdgyyzCOD17MHln8hkT+CJABsvXO8Gu27C9gN6dshvZWjWL\n\t3g2iDYhrF+5BDl2NkO474mrhQsRC30UVmlS0zEvAAuZT24cmW4yGoQxVOqJQ6xrMWG\n\tsBJRb6AwWFWJubMZZD8jgd9iW5086PnaFjupKeZ3Ymmjj6aiTVvZ+vgpskBRPQuxEY\n\t6cEB5DhYIv5iDv11VqxuLVs50zKjHu+ZNgGxmLH04YcCE0oTVsEmXgxmAHp7Ay+Hrm\n\tql+ZAnyiWySc31vs6ppUc1MafQVy/eljVgk8Ta94NAzcwgd/PA7ec3OWsNiyaKlx/z\n\tX9opI/isQ0pbQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666172567;\n\tbh=gqdODNO8FwwXzy1qWiEFLSxK4swj1IWLH/KH2JrD6pk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Y7bo8xJIn/Dy8fZRkW7q8vCbnwNxvPDs8L9lY3yREy9yzuEGjtQZGbzEQP7McOkGK\n\tEMw05IIX39na57/KuECz5CPLLeaRccHHFxmrCP8Ap4RZdcw/TjRrcmaQlmYmIKQqQs\n\t0DnRaXb94aoJOc5qwpy2jnxrpI5u8A3IvqigEVt0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Y7bo8xJI\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221019093713.cwojew3pzl25ylrn@uno.localdomain>","References":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>\n\t<20221019085946.djzbneswo44zotg2@uno.localdomain>\n\t<166617091299.2560709.932874122358043735@Monstersaurus>\n\t<20221019093713.cwojew3pzl25ylrn@uno.localdomain>","To":"Jacopo Mondi ","Date":"Wed, 19 Oct 2022 10:42:44 +0100","Message-ID":"<166617256494.2560709.10074743941317017767@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3] 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 ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25462,"web_url":"https://patchwork.libcamera.org/comment/25462/","msgid":"","date":"2022-10-19T09:42:30","subject":"Re: [libcamera-devel] [PATCH v3] 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":"On Wed, Oct 19, 2022 at 11:37:13AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> On Wed, Oct 19, 2022 at 10:15:12AM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2022-10-19 09:59:46)\n> > > On Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:\n> > > > Add support for manual gain and exposure in the rkisp1 IPA.\n> > > >\n> > >\n> > > I wish we could base this on the new AEGC controls\n> >\n> > Likewise.\n> >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > Changes in v3:\n> > > > - Report the exposure time and analogue gain in metadata\n> > > >\n> > > > Changes in v2:\n> > > > - set the min and max values of ExposureTime and AnalogueGain properly\n> > > > - to that end, plumb the sensorControls through the pipeline handler's\n> > > > loadIPA() and the IPA's init()\n> > > > ---\n> > > > include/libcamera/ipa/rkisp1.mojom | 2 +-\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 | 21 +++++++++\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--\n> > > > 6 files changed, 95 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > index eaf3955e..b526450d 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 04062a36..09ec6ac4 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, 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> > > > @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> > > > @@ -373,6 +379,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> > > > + 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> > I've wondered if a helpful pattern on these might be:\n> >\n> > \tbool agcEnable = controls.get(controls::AeEnable)\n> > \t\t\t\t .value_or(agc.autoEnabled);\n> > \tif (agcEnable != agc.autoEnabled) {\n> > \t\tagc.autoEnabled = agcEnable;\n> > \t\tLOG(...)\n> > \t}\n> >\n> > But it's not specifically better than what you have.\n> >\n> >\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> >\n> > But this looks good, and can't use the .value_or() anyway.\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> > > This seems correct to me!\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 9ad5c32f..ebceeef4 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> > > > IPAFrameContext &frameContext,\n> > > > const rkisp1_stat_buffer *stats) override;\n> > > > + void queueRequest(IPAContext &context,\n> > > > + const uint32_t frame,\n> > > > + IPAFrameContext &frameContext,\n> > > > + const ControlList &controls) override;\n> > > >\n> > > > private:\n> > > > void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index c85d8abe..035d67e2 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 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 3f5c1a58..33692e5d 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -49,6 +49,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> > > > @@ -183,6 +185,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> > >\n> > > We have a list of mandatory controls in camera_sensor.cpp, among which\n> > > we have CID_EXPOSURE but not CID_ANALOGUE_GAIN.\n> > >\n> > > I wonder if we shouldn't make it mandatory too.\n> > >\n> > > Apart from that, I wonder if the IPA can assume the pipeline handler\n> > > has already validated the sensor driver by using CameraSensor or it\n> > > should re-check here.\n> > >\n> > > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> > > >\n> > > > return 0;\n> > > > @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > >\n> > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > {\n> > > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > ControlList ctrls(controls::controls);\n> > > >\n> > > > if (aeState)\n> > > > ctrls.set(controls::AeLocked, aeState == 2);\n> >\n> > Not your patch - but I don't like seeing aeState==2 ... What is 2 ?\n> \n> This has been bothering me as well, and I thought I had a patch in my\n> tree somewhere to drop the whole prepareMetadata() function as (before\n> this patch) it was just a stub.\n> \n> I think as part of this patch the aeState argument can now be dropped.\n> \n> Also consider it is always called as:\n> \n> \tunsigned int aeState = 0;\n> \n> \tfor (auto const &algo : algorithms())\n> \t\talgo->process(context_, frame, frameContext, stats);\n> \n> \tsetControls(frame);\n> \n> \tprepareMetadata(frame, aeState);\n> \n> So the argument is always effectively zero. It feels like a leftover\n> from some initial IPA skeleton.\n\nI have a patch to drop it, I'll post it shortly (as part of a series).\n\n> > > >\n> > > > + ctrls.set(controls::ExposureTime, frameContext.agc.exposure);\n> > > > + ctrls.set(controls::AnalogueGain, frameContext.agc.gain);\n> > > > +\n> > > > metadataReady.emit(frame, ctrls);\n> > > > }\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 455ee2a0..4f8e467a 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -91,7 +91,7 @@ public:\n> > > > }\n> > > >\n> > > > PipelineHandlerRkISP1 *pipe();\n> > > > - int loadIPA(unsigned int hwRevision);\n> > > > + int loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);\n> > > >\n> > > > Stream mainPathStream_;\n> > > > Stream selfPathStream_;\n> > > > @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> > > > return static_cast(Camera::Private::pipe());\n> > > > }\n> > > >\n> > > > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > +int RkISP1CameraData::loadIPA(unsigned int hwRevision,\n> > > > + const ControlInfoMap &sensorControls)\n> > >\n> > > Do you need to pass the controls list in or can you retrieve it from\n> > > the RkISP1CameraData::sensor_ class member ?\n> > >\n> > > > {\n> > > > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > > > if (!ipa_)\n> > > > @@ -348,7 +349,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > }\n> > > >\n> > > > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > > > - &controlInfo_);\n> > > > + sensorControls, &controlInfo_);\n> > > > if (ret < 0) {\n> > > > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > > > return ret;\n> > > > @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > > > &DelayedControls::applyControls);\n> > > >\n> > > > - ret = data->loadIPA(media_->hwRevision());\n> > > > + ret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());\n> > > > if (ret)\n> > > > return ret;\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 6D348BD16B\n\tfor ;\n\tWed, 19 Oct 2022 09:42:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2790862E4A;\n\tWed, 19 Oct 2022 11:42: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 65ACB62E37\n\tfor ;\n\tWed, 19 Oct 2022 11:42:55 +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 D9AD5903;\n\tWed, 19 Oct 2022 11:42:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666172577;\n\tbh=SJ+iVl0CJ6y8vg1h6rdBgyQbkBmRoW1yH5L0y+vetDM=;\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=yBfxeRV7qIVOsLlDKh5AKNvRZ7EJoAcrLgGWxIf86OmhK8k4DlGDewRUhsLeDHuuO\n\teApyBf0Wngp9YpbmTikMiZ82xPjo5i7wN971mKSRsN9a3AYMoZpXIiH9PpioK4LAel\n\tGFZgziV98YljKURwkS5pvqlRRNCJytJev7kQnHpW7fHLGwk1VZNMQI1bXoRJJm5S5+\n\tjl3YK86OYbhPRDG73ZNDwG9SkD/hq74ZPiT6nFaWUZ4Zaf46MAgzTjTUyjc2kiZdDW\n\tnY4znB83C56+RtKBabmXKl2jVQnDod2A7+HhJG0P/Sy6EJXDpz1jpsAqEw6prKWTSz\n\tpHvZPtMES9dzg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666172575;\n\tbh=SJ+iVl0CJ6y8vg1h6rdBgyQbkBmRoW1yH5L0y+vetDM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lHfu1afr7l3Fm1R50V++tRMxG4cREvYot82dQSA33PUPagQgxygaBPeLK8tRZ6+mz\n\th2oCBj/PbHlUFfJn8bIn0MpByND1+6B6HrnIQp8ILNI9uPyENvCP9DrDtvZuZ6w6bP\n\t0keUMcqyz+9w2n2SF+SQLGNafZrq6++1EHyWM3qo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lHfu1afr\"; dkim-atps=neutral","Date":"Wed, 19 Oct 2022 12:42:30 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>\n\t<20221019085946.djzbneswo44zotg2@uno.localdomain>\n\t<166617091299.2560709.932874122358043735@Monstersaurus>\n\t<20221019093713.cwojew3pzl25ylrn@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221019093713.cwojew3pzl25ylrn@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] 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":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25484,"web_url":"https://patchwork.libcamera.org/comment/25484/","msgid":"","date":"2022-10-20T01:04:09","subject":"Re: [libcamera-devel] [PATCH v3] 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 Wed, Oct 19, 2022 at 10:42:44AM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Jacopo Mondi (2022-10-19 10:37:13)\n> > On Wed, Oct 19, 2022 at 10:15:12AM +0100, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi via libcamera-devel (2022-10-19 09:59:46)\n> > > > On Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:\n> > > > > Add support for manual gain and exposure in the rkisp1 IPA.\n> > > >\n> > > > I wish we could base this on the new AEGC controls\n> > >\n> > > Likewise.\n\nI'll get to this. Really sorry for the delay.\n\n> > > > > Signed-off-by: Paul Elder \n> > > > >\n> > > > > ---\n> > > > > Changes in v3:\n> > > > > - Report the exposure time and analogue gain in metadata\n> > > > >\n> > > > > Changes in v2:\n> > > > > - set the min and max values of ExposureTime and AnalogueGain properly\n> > > > > - to that end, plumb the sensorControls through the pipeline handler's\n> > > > > loadIPA() and the IPA's init()\n\nCould I ask you to rebase on top of \"[PATCH v2 0/3] ipa: Fill metadata\nin individual algorithms\" ? It shouldn't hurt too much, the only larger\nconflict is in IPARkISP1::prepareMetadata(), and you can just drop your\nchanges to that function as it's now gone :-) There should be no need to\nmove that code anywhere else, it should be handled correctly in agc.cpp\nalready.\n\n> > > > > ---\n> > > > > include/libcamera/ipa/rkisp1.mojom | 2 +-\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 | 21 +++++++++\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--\n> > > > > 6 files changed, 95 insertions(+), 13 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > > index eaf3955e..b526450d 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 04062a36..09ec6ac4 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\nSome line wrapping could be nice.\n\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, 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> > > > > @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> > > > > @@ -373,6 +379,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> > > > > + 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> > > I've wondered if a helpful pattern on these might be:\n> > >\n> > > bool agcEnable = controls.get(controls::AeEnable)\n> > > .value_or(agc.autoEnabled);\n> > > if (agcEnable != agc.autoEnabled) {\n> > > agc.autoEnabled = agcEnable;\n> > > LOG(...)\n> > > }\n> > >\n> > > But it's not specifically better than what you have.\n> > >\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> > >\n> > > But this looks good, and can't use the .value_or() anyway.\n\nI was going to reply to the previous comment to tell that I was tempted,\nand then that I realized we couldn't use the same pattern everywhere.\nSeems I'm just following your train of thoughts :-)\n\n> > > > > + LOG(RkISP1Agc, Debug)\n> > > > > + << \"Set exposure to: \" << agc.manual.exposure;\n\ns/://\n\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\nDitto.\n\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> > > > This seems correct to me!\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 9ad5c32f..ebceeef4 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> > > > > IPAFrameContext &frameContext,\n> > > > > const rkisp1_stat_buffer *stats) override;\n> > > > > + void queueRequest(IPAContext &context,\n> > > > > + const uint32_t frame,\n> > > > > + IPAFrameContext &frameContext,\n> > > > > + const ControlList &controls) override;\n> > > > >\n> > > > > private:\n> > > > > void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > index c85d8abe..035d67e2 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 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 3f5c1a58..33692e5d 100644\n> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > @@ -49,6 +49,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> > > > > @@ -183,6 +185,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\nLet's add the default value.\n\nAnd this isn't right, ExposureTime is expressed in µs, while the V4L2\ncontrols is expressed as a number of lines. Look at the IPU3 IPA module\nto see how to fix it.\n\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\nHere too, default value, and units.\n\n> > > > > + }\n> > > > > +\n> > > >\n> > > > We have a list of mandatory controls in camera_sensor.cpp, among which\n> > > > we have CID_EXPOSURE but not CID_ANALOGUE_GAIN.\n> > > >\n> > > > I wonder if we shouldn't make it mandatory too.\n> > > >\n> > > > Apart from that, I wonder if the IPA can assume the pipeline handler\n> > > > has already validated the sensor driver by using CameraSensor or it\n> > > > should re-check here.\n\nWe also perform the same validation in IPARkISP1::configure() and return\nan error if it fails. At the very least, I would align the two, there's\nno point in handling the lack of those controls gracefully in init() if\nwe fail configure().\n\nIf we want to be on the safe side, I would move the check from\nconfigure() to init(), as getting a valid ControlInfoMap in init() but\nnot in configure() seems like a very pathological case to me. We could\ngo one step further and consider that the pipeline handler has already\nperformed the required validation.\n\nWe also have IPAIPU3::validateSensorControls(), which is called in\nconfigure(), and we access the sensor controls in IPAIPU3::init(),\nwithout any check. Sounds like there's room for some cleanups to align\nthe two IPA modules. Let's reach a consensus on this, and it can then be\nhandled on top, as aligning the two requires passed the sensor control\ninfo map to init(), which is introduced in this patch for the RkISP1 IPA\nmodule.\n\nIn the future, I could possibly imagine needing to support sensors that\nhave no controllable analog gain, although that sounds a bit\nfar-fetched. If that happens we'll have to update the AGC algorithm\nanyway, so I don't think we need to care about this for now.\n\n> > > > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> > > > >\n> > > > > return 0;\n> > > > > @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > > >\n> > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > > {\n> > > > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > ControlList ctrls(controls::controls);\n> > > > >\n> > > > > if (aeState)\n> > > > > ctrls.set(controls::AeLocked, aeState == 2);\n> > >\n> > > Not your patch - but I don't like seeing aeState==2 ... What is 2 ?\n> > \n> > This has been bothering me as well, and I thought I had a patch in my\n> > tree somewhere to drop the whole prepareMetadata() function as (before\n> > this patch) it was just a stub.\n> > \n> > I think as part of this patch the aeState argument can now be dropped.\n> > \n> > Also consider it is always called as:\n> > \n> > unsigned int aeState = 0;\n> > \n> > for (auto const &algo : algorithms())\n> > algo->process(context_, frame, frameContext, stats);\n> > \n> > setControls(frame);\n> > \n> > prepareMetadata(frame, aeState);\n> > \n> > So the argument is always effectively zero. It feels like a leftover\n> > from some initial IPA skeleton.\n> \n> Ok - lets get rid of it. Who's going to post the patch. Let me know if\n> you want me to do it.\n\nv2 is on the list, just missing a few reviews :-)\n\n> > > > >\n> > > > > + ctrls.set(controls::ExposureTime, frameContext.agc.exposure);\n> > > > > + ctrls.set(controls::AnalogueGain, frameContext.agc.gain);\n> > > > > +\n> > > > > metadataReady.emit(frame, ctrls);\n> > > > > }\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > index 455ee2a0..4f8e467a 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > @@ -91,7 +91,7 @@ public:\n> > > > > }\n> > > > >\n> > > > > PipelineHandlerRkISP1 *pipe();\n> > > > > - int loadIPA(unsigned int hwRevision);\n> > > > > + int loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);\n> > > > >\n> > > > > Stream mainPathStream_;\n> > > > > Stream selfPathStream_;\n> > > > > @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> > > > > return static_cast(Camera::Private::pipe());\n> > > > > }\n> > > > >\n> > > > > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > > +int RkISP1CameraData::loadIPA(unsigned int hwRevision,\n> > > > > + const ControlInfoMap &sensorControls)\n> > > >\n> > > > Do you need to pass the controls list in or can you retrieve it from\n> > > > the RkISP1CameraData::sensor_ class member ?\n\nAgreed, let's use sensor_.\n\n> > > > > {\n> > > > > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > > > > if (!ipa_)\n> > > > > @@ -348,7 +349,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > > }\n> > > > >\n> > > > > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > > > > - &controlInfo_);\n> > > > > + sensorControls, &controlInfo_);\n> > > > > if (ret < 0) {\n> > > > > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > > > > return ret;\n> > > > > @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > > isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > > > > &DelayedControls::applyControls);\n> > > > >\n> > > > > - ret = data->loadIPA(media_->hwRevision());\n> > > > > + ret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());\n> > > > > if (ret)\n> > > > > return ret;\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 6E369C0DA4\n\tfor ;\n\tThu, 20 Oct 2022 01:04:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E346462E7B;\n\tThu, 20 Oct 2022 03:04:35 +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 D0E5E604DA\n\tfor ;\n\tThu, 20 Oct 2022 03:04:33 +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 0B7C8570;\n\tThu, 20 Oct 2022 03:04:32 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666227875;\n\tbh=JG+mLuCkG3NZcB0QYJL7Zej1fStmbOr20ee5gHc2oX0=;\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=JmfIyHHLQ/Cvh1P5JYSF4ZBQWBD9RM/CFSgPGcmwuQS0W5vvOF771s0LZvmsBuQ25\n\tSFHdjQ3YeHqD+gJNTVnAYMiRs9PydpkrFMogo1VS2e4/7B6j2NxaM5d4kYyz2nj069\n\tPgZCg9U5LecuctORfE1ejpqcvaB+KHXIQWv41cuuUMiHiM+yVI7wLzEaj4CoGq76oy\n\tOH0Du9itOS3mE51gjC3CvMGq1KMNNAi7xS6TsVssUacIQasQZMUxyBitoNItjfde8s\n\tEk0FFn5fQQjoUur6NwxCaNYqoyPDBXeEw4ACUsZSLjRnKE//THJLXJHXWMDVbqDuCY\n\t8qA7BpStYF/MA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666227873;\n\tbh=JG+mLuCkG3NZcB0QYJL7Zej1fStmbOr20ee5gHc2oX0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Dh81GWQB7LGpZdKdeO6poTVb/s1oYLZlZ03FKBTEjJIjV4yfThBNfcVe8tM1hZA4t\n\tkQIJARBybBCWz1AqnOTT99oAxcA4srYGBj+SQ6X4pVSdxyQcSK8aeELHR0vP9Rltpc\n\tNXkhYuOlXis9aFhluXdsI3/gNABB49yT3Qm+gPE8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Dh81GWQB\"; dkim-atps=neutral","Date":"Thu, 20 Oct 2022 04:04:09 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>\n\t<20221019085946.djzbneswo44zotg2@uno.localdomain>\n\t<166617091299.2560709.932874122358043735@Monstersaurus>\n\t<20221019093713.cwojew3pzl25ylrn@uno.localdomain>\n\t<166617256494.2560709.10074743941317017767@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<166617256494.2560709.10074743941317017767@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3] 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":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25498,"web_url":"https://patchwork.libcamera.org/comment/25498/","msgid":"<20221020080005.hfw3tnt242b7p57q@uno.localdomain>","date":"2022-10-20T08:00:05","subject":"Re: [libcamera-devel] [PATCH v3] 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 Thu, Oct 20, 2022 at 04:04:09AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n>\n> Thank you for the patch.\n>\n> On Wed, Oct 19, 2022 at 10:42:44AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Jacopo Mondi (2022-10-19 10:37:13)\n> > > On Wed, Oct 19, 2022 at 10:15:12AM +0100, Kieran Bingham wrote:\n> > > > Quoting Jacopo Mondi via libcamera-devel (2022-10-19 09:59:46)\n> > > > > On Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:\n> > > > > > Add support for manual gain and exposure in the rkisp1 IPA.\n> > > > >\n> > > > > I wish we could base this on the new AEGC controls\n> > > >\n> > > > Likewise.\n>\n> I'll get to this. Really sorry for the delay.\n>\n> > > > > > Signed-off-by: Paul Elder \n> > > > > >\n> > > > > > ---\n> > > > > > Changes in v3:\n> > > > > > - Report the exposure time and analogue gain in metadata\n> > > > > >\n> > > > > > Changes in v2:\n> > > > > > - set the min and max values of ExposureTime and AnalogueGain properly\n> > > > > > - to that end, plumb the sensorControls through the pipeline handler's\n> > > > > > loadIPA() and the IPA's init()\n>\n> Could I ask you to rebase on top of \"[PATCH v2 0/3] ipa: Fill metadata\n> in individual algorithms\" ? It shouldn't hurt too much, the only larger\n> conflict is in IPARkISP1::prepareMetadata(), and you can just drop your\n> changes to that function as it's now gone :-) There should be no need to\n> move that code anywhere else, it should be handled correctly in agc.cpp\n> already.\n>\n> > > > > > ---\n> > > > > > include/libcamera/ipa/rkisp1.mojom | 2 +-\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 | 21 +++++++++\n> > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--\n> > > > > > 6 files changed, 95 insertions(+), 13 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > > > index eaf3955e..b526450d 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 04062a36..09ec6ac4 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>\n> Some line wrapping could be nice.\n>\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, 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> > > > > > @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> > > > > > @@ -373,6 +379,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> > > > > > + 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> > > > I've wondered if a helpful pattern on these might be:\n> > > >\n> > > > bool agcEnable = controls.get(controls::AeEnable)\n> > > > .value_or(agc.autoEnabled);\n> > > > if (agcEnable != agc.autoEnabled) {\n> > > > agc.autoEnabled = agcEnable;\n> > > > LOG(...)\n> > > > }\n> > > >\n> > > > But it's not specifically better than what you have.\n> > > >\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> > > >\n> > > > But this looks good, and can't use the .value_or() anyway.\n>\n> I was going to reply to the previous comment to tell that I was tempted,\n> and then that I realized we couldn't use the same pattern everywhere.\n> Seems I'm just following your train of thoughts :-)\n>\n> > > > > > + LOG(RkISP1Agc, Debug)\n> > > > > > + << \"Set exposure to: \" << agc.manual.exposure;\n>\n> s/://\n>\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> Ditto.\n>\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> > > > > This seems correct to me!\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 9ad5c32f..ebceeef4 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> > > > > > IPAFrameContext &frameContext,\n> > > > > > const rkisp1_stat_buffer *stats) override;\n> > > > > > + void queueRequest(IPAContext &context,\n> > > > > > + const uint32_t frame,\n> > > > > > + IPAFrameContext &frameContext,\n> > > > > > + const ControlList &controls) override;\n> > > > > >\n> > > > > > private:\n> > > > > > void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > index c85d8abe..035d67e2 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 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 3f5c1a58..33692e5d 100644\n> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > @@ -49,6 +49,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> > > > > > @@ -183,6 +185,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> Let's add the default value.\n>\n> And this isn't right, ExposureTime is expressed in µs, while the V4L2\n> controls is expressed as a number of lines. Look at the IPU3 IPA module\n> to see how to fix it.\n>\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> Here too, default value, and units.\n>\n> > > > > > + }\n> > > > > > +\n> > > > >\n> > > > > We have a list of mandatory controls in camera_sensor.cpp, among which\n> > > > > we have CID_EXPOSURE but not CID_ANALOGUE_GAIN.\n> > > > >\n> > > > > I wonder if we shouldn't make it mandatory too.\n> > > > >\n> > > > > Apart from that, I wonder if the IPA can assume the pipeline handler\n> > > > > has already validated the sensor driver by using CameraSensor or it\n> > > > > should re-check here.\n>\n> We also perform the same validation in IPARkISP1::configure() and return\n> an error if it fails. At the very least, I would align the two, there's\n> no point in handling the lack of those controls gracefully in init() if\n> we fail configure().\n>\n> If we want to be on the safe side, I would move the check from\n> configure() to init(), as getting a valid ControlInfoMap in init() but\n> not in configure() seems like a very pathological case to me. We could\n> go one step further and consider that the pipeline handler has already\n> performed the required validation.\n>\n> We also have IPAIPU3::validateSensorControls(), which is called in\n> configure(), and we access the sensor controls in IPAIPU3::init(),\n> without any check. Sounds like there's room for some cleanups to align\n\nAh indeed, we check in configure() but in init() we assume the\ncontrols are there :/\n\n> the two IPA modules. Let's reach a consensus on this, and it can then be\n> handled on top, as aligning the two requires passed the sensor control\n> info map to init(), which is introduced in this patch for the RkISP1 IPA\n> module.\n\nThis is more of a general question about how much we can assume about\nthe PH/IPA module coupling... A possible concern is that someone can\nuse one of the two and replace the other with some custom component,\nand all the assumptions that the PH has validated the sensor on behalf\nof the IPA crumbles down.. BUT if anyone is taking\nthe direction of replacing pieces of a \"mainline\" platform for\nwhatever reason, I guess it's fair to say it's not an issue\nmainline development should be concerned with.\n\nFor platforms whose support is in the master branch I guess we can\nassume on the IPA that all controls have been correctly validated by\nthe PH, and the PH knows what controls the IPA needs.\n\nMy only concern here is that if we move the IPA requirements in the\nCameraSensor validation (here's another assumption that PH will use\nCameraSensor, but the same reasoning as before goes) we will rise the\nbar a little more for sensor drivers. It's not different than today,\nas the same validation that is today performed in the IPA will be\nmoved to CameraSensor, but I wonder if this will be problematic as\nCameraSensor can be used in platforms without an IPA and that might\nnot have the same requirements as the ones we have today.\n\nTo make an example, we already require today in CameraSensor to have a\ncontrollable exposure time, but for some platforms (Simple, in example)\nit might not be strictly required and, as far as I can tell, the\nexposure is not even controlled there. Adding one more requirement\ncould limit or make it more complicated to use YUV sensors with the\nSimple pipeline handler is a sort of plug&play fashion.\n\n>\n> In the future, I could possibly imagine needing to support sensors that\n> have no controllable analog gain, although that sounds a bit\n> far-fetched. If that happens we'll have to update the AGC algorithm\n> anyway, so I don't think we need to care about this for now.\n>\n> > > > > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> > > > > >\n> > > > > > return 0;\n> > > > > > @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > > > >\n> > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > > > {\n> > > > > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > > ControlList ctrls(controls::controls);\n> > > > > >\n> > > > > > if (aeState)\n> > > > > > ctrls.set(controls::AeLocked, aeState == 2);\n> > > >\n> > > > Not your patch - but I don't like seeing aeState==2 ... What is 2 ?\n> > >\n> > > This has been bothering me as well, and I thought I had a patch in my\n> > > tree somewhere to drop the whole prepareMetadata() function as (before\n> > > this patch) it was just a stub.\n> > >\n> > > I think as part of this patch the aeState argument can now be dropped.\n> > >\n> > > Also consider it is always called as:\n> > >\n> > > unsigned int aeState = 0;\n> > >\n> > > for (auto const &algo : algorithms())\n> > > algo->process(context_, frame, frameContext, stats);\n> > >\n> > > setControls(frame);\n> > >\n> > > prepareMetadata(frame, aeState);\n> > >\n> > > So the argument is always effectively zero. It feels like a leftover\n> > > from some initial IPA skeleton.\n> >\n> > Ok - lets get rid of it. Who's going to post the patch. Let me know if\n> > you want me to do it.\n>\n> v2 is on the list, just missing a few reviews :-)\n>\n> > > > > >\n> > > > > > + ctrls.set(controls::ExposureTime, frameContext.agc.exposure);\n> > > > > > + ctrls.set(controls::AnalogueGain, frameContext.agc.gain);\n> > > > > > +\n> > > > > > metadataReady.emit(frame, ctrls);\n> > > > > > }\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > index 455ee2a0..4f8e467a 100644\n> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > @@ -91,7 +91,7 @@ public:\n> > > > > > }\n> > > > > >\n> > > > > > PipelineHandlerRkISP1 *pipe();\n> > > > > > - int loadIPA(unsigned int hwRevision);\n> > > > > > + int loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);\n> > > > > >\n> > > > > > Stream mainPathStream_;\n> > > > > > Stream selfPathStream_;\n> > > > > > @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> > > > > > return static_cast(Camera::Private::pipe());\n> > > > > > }\n> > > > > >\n> > > > > > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > > > +int RkISP1CameraData::loadIPA(unsigned int hwRevision,\n> > > > > > + const ControlInfoMap &sensorControls)\n> > > > >\n> > > > > Do you need to pass the controls list in or can you retrieve it from\n> > > > > the RkISP1CameraData::sensor_ class member ?\n>\n> Agreed, let's use sensor_.\n>\n> > > > > > {\n> > > > > > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > > > > > if (!ipa_)\n> > > > > > @@ -348,7 +349,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > > > }\n> > > > > >\n> > > > > > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > > > > > - &controlInfo_);\n> > > > > > + sensorControls, &controlInfo_);\n> > > > > > if (ret < 0) {\n> > > > > > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > > > > > return ret;\n> > > > > > @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > > > isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > > > > > &DelayedControls::applyControls);\n> > > > > >\n> > > > > > - ret = data->loadIPA(media_->hwRevision());\n> > > > > > + ret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());\n> > > > > > if (ret)\n> > > > > > return ret;\n> > > > > >\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 80ABBBD16B\n\tfor ;\n\tThu, 20 Oct 2022 08:00:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDFFA62E99;\n\tThu, 20 Oct 2022 10:00:18 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::221])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2DBA262DFF\n\tfor ;\n\tThu, 20 Oct 2022 10:00:18 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id DF0AA240029;\n\tThu, 20 Oct 2022 08:00:06 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666252818;\n\tbh=cgGlk2VQQa9306wjnep1y5fzR+653S+AcmLDwQsEVwA=;\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=c/G1hED6/RRO9jysK3l84jG2eB6T1hVdLFji+eT2TABE22xdRo+FFw7qNxh87zPjG\n\t7cCL2YomiuRrCi5phmHzFowaSnqSOaYYa/NMgJityDMaFRltpucYx3U93pHvaC0Oxf\n\t+A3/iTcjQ7R2szu1Mc11S72bWKzhDXTRDiaEInQRrVFTFP5rZVd29cKK3vZgtQO/0T\n\tjUneNC0pMUUgAkyxzSDhSA1oB7ONQjhcqhiVCBCBFf+L9oJ80AgyXhsoW5X7I51kPd\n\tfWL+rIo4Kn6MiK8hu3nKoYLcUSn2Yzy398hALNLI75OyfNSZlC1ffJvlROUMoLjLud\n\tQtjnVh1PcD6rg==","Date":"Thu, 20 Oct 2022 10:00:05 +0200","To":"Laurent Pinchart ","Message-ID":"<20221020080005.hfw3tnt242b7p57q@uno.localdomain>","References":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>\n\t<20221019085946.djzbneswo44zotg2@uno.localdomain>\n\t<166617091299.2560709.932874122358043735@Monstersaurus>\n\t<20221019093713.cwojew3pzl25ylrn@uno.localdomain>\n\t<166617256494.2560709.10074743941317017767@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3] 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":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25502,"web_url":"https://patchwork.libcamera.org/comment/25502/","msgid":"","date":"2022-10-20T09:02:08","subject":"Re: [libcamera-devel] [PATCH v3] 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 Thu, Oct 20, 2022 at 10:00:05AM +0200, Jacopo Mondi wrote:\n> On Thu, Oct 20, 2022 at 04:04:09AM +0300, Laurent Pinchart wrote:\n> > On Wed, Oct 19, 2022 at 10:42:44AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > Quoting Jacopo Mondi (2022-10-19 10:37:13)\n> > > > On Wed, Oct 19, 2022 at 10:15:12AM +0100, Kieran Bingham wrote:\n> > > > > Quoting Jacopo Mondi via libcamera-devel (2022-10-19 09:59:46)\n> > > > > > On Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:\n> > > > > > > Add support for manual gain and exposure in the rkisp1 IPA.\n> > > > > >\n> > > > > > I wish we could base this on the new AEGC controls\n> > > > >\n> > > > > Likewise.\n> >\n> > I'll get to this. Really sorry for the delay.\n> >\n> > > > > > > Signed-off-by: Paul Elder \n> > > > > > >\n> > > > > > > ---\n> > > > > > > Changes in v3:\n> > > > > > > - Report the exposure time and analogue gain in metadata\n> > > > > > >\n> > > > > > > Changes in v2:\n> > > > > > > - set the min and max values of ExposureTime and AnalogueGain properly\n> > > > > > > - to that end, plumb the sensorControls through the pipeline handler's\n> > > > > > > loadIPA() and the IPA's init()\n> >\n> > Could I ask you to rebase on top of \"[PATCH v2 0/3] ipa: Fill metadata\n> > in individual algorithms\" ? It shouldn't hurt too much, the only larger\n> > conflict is in IPARkISP1::prepareMetadata(), and you can just drop your\n> > changes to that function as it's now gone :-) There should be no need to\n> > move that code anywhere else, it should be handled correctly in agc.cpp\n> > already.\n> >\n> > > > > > > ---\n> > > > > > > include/libcamera/ipa/rkisp1.mojom | 2 +-\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 | 21 +++++++++\n> > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--\n> > > > > > > 6 files changed, 95 insertions(+), 13 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > > > > > index eaf3955e..b526450d 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 04062a36..09ec6ac4 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> >\n> > Some line wrapping could be nice.\n> >\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, 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> > > > > > > @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> > > > > > > @@ -373,6 +379,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> > > > > > > + 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> > > > > I've wondered if a helpful pattern on these might be:\n> > > > >\n> > > > > bool agcEnable = controls.get(controls::AeEnable)\n> > > > > .value_or(agc.autoEnabled);\n> > > > > if (agcEnable != agc.autoEnabled) {\n> > > > > agc.autoEnabled = agcEnable;\n> > > > > LOG(...)\n> > > > > }\n> > > > >\n> > > > > But it's not specifically better than what you have.\n> > > > >\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> > > > >\n> > > > > But this looks good, and can't use the .value_or() anyway.\n> >\n> > I was going to reply to the previous comment to tell that I was tempted,\n> > and then that I realized we couldn't use the same pattern everywhere.\n> > Seems I'm just following your train of thoughts :-)\n> >\n> > > > > > > + LOG(RkISP1Agc, Debug)\n> > > > > > > + << \"Set exposure to: \" << agc.manual.exposure;\n> >\n> > s/://\n> >\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> > Ditto.\n> >\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> > > > > > This seems correct to me!\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 9ad5c32f..ebceeef4 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> > > > > > > IPAFrameContext &frameContext,\n> > > > > > > const rkisp1_stat_buffer *stats) override;\n> > > > > > > + void queueRequest(IPAContext &context,\n> > > > > > > + const uint32_t frame,\n> > > > > > > + IPAFrameContext &frameContext,\n> > > > > > > + const ControlList &controls) override;\n> > > > > > >\n> > > > > > > private:\n> > > > > > > void computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > > index c85d8abe..035d67e2 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 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 3f5c1a58..33692e5d 100644\n> > > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > @@ -49,6 +49,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> > > > > > > @@ -183,6 +185,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> > Let's add the default value.\n> >\n> > And this isn't right, ExposureTime is expressed in µs, while the V4L2\n> > controls is expressed as a number of lines. Look at the IPU3 IPA module\n> > to see how to fix it.\n> >\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> > Here too, default value, and units.\n> >\n> > > > > > > + }\n> > > > > > > +\n> > > > > >\n> > > > > > We have a list of mandatory controls in camera_sensor.cpp, among which\n> > > > > > we have CID_EXPOSURE but not CID_ANALOGUE_GAIN.\n> > > > > >\n> > > > > > I wonder if we shouldn't make it mandatory too.\n> > > > > >\n> > > > > > Apart from that, I wonder if the IPA can assume the pipeline handler\n> > > > > > has already validated the sensor driver by using CameraSensor or it\n> > > > > > should re-check here.\n> >\n> > We also perform the same validation in IPARkISP1::configure() and return\n> > an error if it fails. At the very least, I would align the two, there's\n> > no point in handling the lack of those controls gracefully in init() if\n> > we fail configure().\n> >\n> > If we want to be on the safe side, I would move the check from\n> > configure() to init(), as getting a valid ControlInfoMap in init() but\n> > not in configure() seems like a very pathological case to me. We could\n> > go one step further and consider that the pipeline handler has already\n> > performed the required validation.\n> >\n> > We also have IPAIPU3::validateSensorControls(), which is called in\n> > configure(), and we access the sensor controls in IPAIPU3::init(),\n> > without any check. Sounds like there's room for some cleanups to align\n> \n> Ah indeed, we check in configure() but in init() we assume the\n> controls are there :/\n> \n> > the two IPA modules. Let's reach a consensus on this, and it can then be\n> > handled on top, as aligning the two requires passed the sensor control\n> > info map to init(), which is introduced in this patch for the RkISP1 IPA\n> > module.\n> \n> This is more of a general question about how much we can assume about\n> the PH/IPA module coupling... A possible concern is that someone can\n> use one of the two and replace the other with some custom component,\n> and all the assumptions that the PH has validated the sensor on behalf\n> of the IPA crumbles down.. BUT if anyone is taking\n> the direction of replacing pieces of a \"mainline\" platform for\n> whatever reason, I guess it's fair to say it's not an issue\n> mainline development should be concerned with.\n\nIPA modules and pipeline handlers must be designed together, as shown by\nthe fact that they use a platform-specific interface (per-platform\n.mojom files). It's perfectly valid in my opinion to make assumptions\npart of that design. They should ideally be documented, and become part\nof the ABI.\n\nThe particular assumption we're going to make here may not be something\nwe want in an ABI though. I can imagine people wanting to use a YUV\nsensor with the RkISP1, bypassing the ISP completely. In that case, even\nthough exposure time and analog gain will likely be exposed by the\nsensor, other controls that would be considered mandatory for a raw\nsensor may not. We can always change this later as long as we don't have\nexternal API modules.\n\n> For platforms whose support is in the master branch I guess we can\n> assume on the IPA that all controls have been correctly validated by\n> the PH, and the PH knows what controls the IPA needs.\n\nI agree. A bit of defensive programming against our own mistakes may not\nbe a bad idea, the same way we have assertions here and there, we could\nalso validate controls in init(), but we don't need to check everywhere\nall the time as if the inputs to the IPA module were completely\nuntrusted.\n\n> My only concern here is that if we move the IPA requirements in the\n> CameraSensor validation (here's another assumption that PH will use\n> CameraSensor, but the same reasoning as before goes) we will rise the\n> bar a little more for sensor drivers. It's not different than today,\n> as the same validation that is today performed in the IPA will be\n> moved to CameraSensor, but I wonder if this will be problematic as\n> CameraSensor can be used in platforms without an IPA and that might\n> not have the same requirements as the ones we have today.\n\nI wouldn't phrase the IPA protocol contract as \"the pipeline handler\nuses CameraSensor\", but as \"the pipeline handler guarantees that this\nparticular set of controls is available\". How the pipeline handler\nimplements that, whether by delegating the checks to CameraSensor, or\nimplementing them manually, isn't something that the IPA module should\nbe concerned with.\n\n> To make an example, we already require today in CameraSensor to have a\n> controllable exposure time, but for some platforms (Simple, in example)\n> it might not be strictly required and, as far as I can tell, the\n> exposure is not even controlled there. Adding one more requirement\n> could limit or make it more complicated to use YUV sensors with the\n> Simple pipeline handler is a sort of plug&play fashion.\n\nYou're right, I certainly foresee the need to support different types of\nsensors, with different feature sets. It will likely not affect exposure\ntime and analog gain, but will affect other features in the future.\nThere are multiple ways to implement the requirement on the pipeline\nhandler side. I'm not too concerned at this point about what option will\nbe picked in the long run. Maybe we'll still have those checks in the\nCameraSensor class but condition them on the sensor type for instance.\nMaybe the pipeline handler will explicitly tell the CameraSensor what\ncontrols (or classes of controls) it requires. Maybe we'll do something\nelse, but in any case, how this is checked on the pipeline handler side\nisn't something the IPA module needs to know.\n\n> > In the future, I could possibly imagine needing to support sensors that\n> > have no controllable analog gain, although that sounds a bit\n> > far-fetched. If that happens we'll have to update the AGC algorithm\n> > anyway, so I don't think we need to care about this for now.\n> >\n> > > > > > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> > > > > > >\n> > > > > > > return 0;\n> > > > > > > @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > > > > >\n> > > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> > > > > > > {\n> > > > > > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > > > ControlList ctrls(controls::controls);\n> > > > > > >\n> > > > > > > if (aeState)\n> > > > > > > ctrls.set(controls::AeLocked, aeState == 2);\n> > > > >\n> > > > > Not your patch - but I don't like seeing aeState==2 ... What is 2 ?\n> > > >\n> > > > This has been bothering me as well, and I thought I had a patch in my\n> > > > tree somewhere to drop the whole prepareMetadata() function as (before\n> > > > this patch) it was just a stub.\n> > > >\n> > > > I think as part of this patch the aeState argument can now be dropped.\n> > > >\n> > > > Also consider it is always called as:\n> > > >\n> > > > unsigned int aeState = 0;\n> > > >\n> > > > for (auto const &algo : algorithms())\n> > > > algo->process(context_, frame, frameContext, stats);\n> > > >\n> > > > setControls(frame);\n> > > >\n> > > > prepareMetadata(frame, aeState);\n> > > >\n> > > > So the argument is always effectively zero. It feels like a leftover\n> > > > from some initial IPA skeleton.\n> > >\n> > > Ok - lets get rid of it. Who's going to post the patch. Let me know if\n> > > you want me to do it.\n> >\n> > v2 is on the list, just missing a few reviews :-)\n> >\n> > > > > > >\n> > > > > > > + ctrls.set(controls::ExposureTime, frameContext.agc.exposure);\n> > > > > > > + ctrls.set(controls::AnalogueGain, frameContext.agc.gain);\n> > > > > > > +\n> > > > > > > metadataReady.emit(frame, ctrls);\n> > > > > > > }\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > > index 455ee2a0..4f8e467a 100644\n> > > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > > @@ -91,7 +91,7 @@ public:\n> > > > > > > }\n> > > > > > >\n> > > > > > > PipelineHandlerRkISP1 *pipe();\n> > > > > > > - int loadIPA(unsigned int hwRevision);\n> > > > > > > + int loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);\n> > > > > > >\n> > > > > > > Stream mainPathStream_;\n> > > > > > > Stream selfPathStream_;\n> > > > > > > @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> > > > > > > return static_cast(Camera::Private::pipe());\n> > > > > > > }\n> > > > > > >\n> > > > > > > -int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > > > > +int RkISP1CameraData::loadIPA(unsigned int hwRevision,\n> > > > > > > + const ControlInfoMap &sensorControls)\n> > > > > >\n> > > > > > Do you need to pass the controls list in or can you retrieve it from\n> > > > > > the RkISP1CameraData::sensor_ class member ?\n> >\n> > Agreed, let's use sensor_.\n> >\n> > > > > > > {\n> > > > > > > ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > > > > > > if (!ipa_)\n> > > > > > > @@ -348,7 +349,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > > > > > }\n> > > > > > >\n> > > > > > > int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,\n> > > > > > > - &controlInfo_);\n> > > > > > > + sensorControls, &controlInfo_);\n> > > > > > > if (ret < 0) {\n> > > > > > > LOG(RkISP1, Error) << \"IPA initialization failure\";\n> > > > > > > return ret;\n> > > > > > > @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > > > > isp_->frameStart.connect(data->delayedCtrls_.get(),\n> > > > > > > &DelayedControls::applyControls);\n> > > > > > >\n> > > > > > > - ret = data->loadIPA(media_->hwRevision());\n> > > > > > > + ret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());\n> > > > > > > if (ret)\n> > > > > > > return ret;\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 49F95C0DA4\n\tfor ;\n\tThu, 20 Oct 2022 09:02:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C67CA62EA0;\n\tThu, 20 Oct 2022 11:02:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3721662DFF\n\tfor ;\n\tThu, 20 Oct 2022 11:02:34 +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 5FD5F570;\n\tThu, 20 Oct 2022 11:02:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666256555;\n\tbh=F2cPcwlt7oyCO2h6FR3yHC9VZwvWKZTKoxXgxcq/w4M=;\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=3AeN58ToNPiTsGCMpBRXDX9cc5QZTafSmrCHhW2NJzGo4SguMUBsasPERtQZobbCy\n\tjw1Y0KdWKXr/v3O3HMNCRW8vrH/l1KoBiFtMwYof/81mVAxEPpZe0IL8aj9ircXhM5\n\tI8Qk5SuzXpGw6t00+/yACmsvUDQ2aiHhjbTolEyMVpuju9IBrOYBMk8kkT/OtRsBSb\n\tw11lnN1sLY2YVxfIG/Yv0BvClP2xn7+GesIu/88SX8bVYiKQ+R8u8G9r1LPF5DOaB+\n\tfNkM3qGUQsy6g2xG4v72M3iYa54QZ6z0s2NwTI2PDiV/0Vi8/w0Psmx+bDWhQIsTzH\n\tNuGGExnR+IgGg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666256553;\n\tbh=F2cPcwlt7oyCO2h6FR3yHC9VZwvWKZTKoxXgxcq/w4M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DL/hguMDcJ2XFJezwvSf5gLBnpEaBKejKW9ZmHRX3WP5SpHGNknX28kT9CmSwjkOw\n\t+o1di3qKxqtiIFQydmT41Jzzxg+Hm80Qfvt3rwuurEsvNpT0HNPV9px1QL02/NNEOo\n\tlnwiylphcdHjnBRTv2pEX0+9hFybyUpU/26LVGYc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DL/hguMD\"; dkim-atps=neutral","Date":"Thu, 20 Oct 2022 12:02:08 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>\n\t<20221019085946.djzbneswo44zotg2@uno.localdomain>\n\t<166617091299.2560709.932874122358043735@Monstersaurus>\n\t<20221019093713.cwojew3pzl25ylrn@uno.localdomain>\n\t<166617256494.2560709.10074743941317017767@Monstersaurus>\n\t\n\t<20221020080005.hfw3tnt242b7p57q@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221020080005.hfw3tnt242b7p57q@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] 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":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25523,"web_url":"https://patchwork.libcamera.org/comment/25523/","msgid":"","date":"2022-10-21T22:35:33","subject":"Re: [libcamera-devel] [PATCH v3] 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\nAnother comment.\n\nOn Tue, Oct 18, 2022 at 02:49:13PM +0900, Paul Elder via libcamera-devel wrote:\n> Add support for manual gain and exposure in the rkisp1 IPA.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v3:\n> - Report the exposure time and analogue gain in metadata\n> \n> Changes in v2:\n> - set the min and max values of ExposureTime and AnalogueGain properly\n> - to that end, plumb the sensorControls through the pipeline handler's\n> loadIPA() and the IPA's init()\n> ---\n> include/libcamera/ipa/rkisp1.mojom | 2 +-\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 | 21 +++++++++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++--\n> 6 files changed, 95 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index eaf3955e..b526450d 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 04062a36..09ec6ac4 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> \t/* Configure the default exposure and gain. */\n> -\tcontext.activeState.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);\n> -\tcontext.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;\n> +\tcontext.activeState.agc.automatic.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);\n> +\tcontext.activeState.agc.automatic.exposure = 10ms / 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> @@ -221,8 +225,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> @@ -339,8 +343,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> @@ -373,6 +379,47 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \tparams->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> +\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\nThis needs to take into account the boundaries for the exposure time and\ngain.\n\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> 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 9ad5c32f..ebceeef4 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -32,6 +32,10 @@ public:\n> \tvoid process(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t const rkisp1_stat_buffer *stats) 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> \n> private:\n> \tvoid computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index c85d8abe..035d67e2 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> \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> @@ -92,6 +100,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 3f5c1a58..33692e5d 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -49,6 +49,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> @@ -183,6 +185,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> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> \n> \treturn 0;\n> @@ -368,11 +385,15 @@ void IPARkISP1::setControls(unsigned int frame)\n> \n> void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)\n> {\n> +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \tControlList ctrls(controls::controls);\n> \n> \tif (aeState)\n> \t\tctrls.set(controls::AeLocked, aeState == 2);\n> \n> +\tctrls.set(controls::ExposureTime, frameContext.agc.exposure);\n> +\tctrls.set(controls::AnalogueGain, frameContext.agc.gain);\n> +\n> \tmetadataReady.emit(frame, ctrls);\n> }\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 455ee2a0..4f8e467a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -91,7 +91,7 @@ public:\n> \t}\n> \n> \tPipelineHandlerRkISP1 *pipe();\n> -\tint loadIPA(unsigned int hwRevision);\n> +\tint loadIPA(unsigned int hwRevision, const ControlInfoMap &sensorControls);\n> \n> \tStream mainPathStream_;\n> \tStream selfPathStream_;\n> @@ -319,7 +319,8 @@ PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> \treturn static_cast(Camera::Private::pipe());\n> }\n> \n> -int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> +int RkISP1CameraData::loadIPA(unsigned int hwRevision,\n> +\t\t\t const ControlInfoMap &sensorControls)\n> {\n> \tipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> \tif (!ipa_)\n> @@ -348,7 +349,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 sensorControls, &controlInfo_);\n> \tif (ret < 0) {\n> \t\tLOG(RkISP1, Error) << \"IPA initialization failure\";\n> \t\treturn ret;\n> @@ -1023,7 +1024,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \tisp_->frameStart.connect(data->delayedCtrls_.get(),\n> \t\t\t\t &DelayedControls::applyControls);\n> \n> -\tret = data->loadIPA(media_->hwRevision());\n> +\tret = data->loadIPA(media_->hwRevision(), data->sensor_->controls());\n> \tif (ret)\n> \t\treturn ret;\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 E0495BD16B\n\tfor ;\n\tFri, 21 Oct 2022 22:36:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7AB4D62EC2;\n\tSat, 22 Oct 2022 00:36:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9FF2E62E75\n\tfor ;\n\tSat, 22 Oct 2022 00:35:58 +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 E44F8471;\n\tSat, 22 Oct 2022 00:35:57 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666391760;\n\tbh=G7eOl//XKH5XEs+ViHLICYTjaIhabuHNq/Sls1Q9gE8=;\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=tfhicJ3IN1A29G0x6vbVso18DOuA0uZeemwxV/Sn6AWgD199FRcAuwc1NzZpK3rZy\n\tmchi42v2RsW8ivZHNpNAX9yAqq8rWCtxti37i9Qcfe0usD9KN5bGvcDeFlPVIyhbMB\n\tx32dRd0SE6bOgD71gwQf8v+ViOfsuWtV0TqgXFGqGzOB+F9aiIJXswuobt/cggxh8i\n\tfL9glazvMjqdZfV1piWdh12AZBc6vEwyOv6M/vrEwVX2YrwnE+f3+4wUamiGLlc3Lq\n\termrfcwc4ATr4G42hOzVpRKBUcDCTpliWpctMaGEMr/MjISE7jDfW/3J8d5DetpGL7\n\tHu25FI1OtnpuA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666391758;\n\tbh=G7eOl//XKH5XEs+ViHLICYTjaIhabuHNq/Sls1Q9gE8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iWORKNayzmzd2oo3WuBIk8xETRZNQy3LlSlr9EnSuOGIdAEPI/dqP0u1v54AWv2qi\n\t3sgejB+3h7bcf45O50ZzmRNJFrx9I0zdGYeW4crq8mbx7hhMQFTSUkqOAZut3auv3F\n\t8DePoA71uF+tckDKFVWSmfTTorACezLGXVtQn9hY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"iWORKNay\"; dkim-atps=neutral","Date":"Sat, 22 Oct 2022 01:35:33 +0300","To":"Paul Elder ","Message-ID":"","References":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221018054913.2325021-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3] 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\" "}}]