[{"id":29555,"web_url":"https://patchwork.libcamera.org/comment/29555/","msgid":"<20240517104701.vreuvc4tmulv7xgc@jasper>","date":"2024-05-17T10:47:01","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nthanks for the patch.\n\nOn Fri, May 17, 2024 at 05:08:01PM +0900, Paul Elder wrote:\n> Plumb controls for setting metering mode, exposure mode, constraint\n> mode, and frame duration limits. Also report them as available controls,\n> as well as in metadata.\n> \n> While at it, add the missing #include for tuple, as a std::tie is used.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v3:\n> - prevent maxShutterSpeed (for setLimits) from choosing 0 shutter speed\n> if one of the inputs std::min() is not set (this fixes an assertion\n> failure in the exposure mode helper)\n> \n> Changes in v2:\n> - add #include \n> - don't overwrite metering/exposure/constraint mode controls to default\n> if no control is supplied, and retain the old value instead (same for\n> frame duration limits)\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 55 ++++++++++++++++++++++++---\n> src/ipa/rkisp1/algorithms/algorithm.h | 2 +\n> src/ipa/rkisp1/ipa_context.h | 10 ++++-\n> src/ipa/rkisp1/rkisp1.cpp | 10 +++++\n> 4 files changed, 70 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 1c9872d02..ce8beae24 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -10,6 +10,8 @@\n> #include \n> #include \n> #include \n> +#include \n> +#include \n> \n> #include \n> #include \n> @@ -48,6 +50,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tstd::vector availableMeteringModes;\n> \tfor (const auto &[key, value] : yamlMeteringModes.asDict()) {\n> \t\tif (controls::AeMeteringModeNameValueMap.find(key) ==\n> \t\t controls::AeMeteringModeNameValueMap.end()) {\n> @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> \t\t\t\t<< \"Matched metering matrix mode \"\n> \t\t\t\t<< key << \", version \" << version;\n> \n> -\t\t\tmeteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> +\t\t\tint32_t control = controls::AeMeteringModeNameValueMap.at(key);\n> +\t\t\tmeteringModes_[control] = weights;\n> +\t\t\tavailableMeteringModes.push_back(control);\n> \t\t}\n> \t}\n> \n> -\tif (meteringModes_.empty()) {\n> +\tif (availableMeteringModes.empty()) {\n> \t\tint32_t meteringModeId = controls::AeMeteringModeNameValueMap.at(\"MeteringMatrix\");\n> \t\tstd::vector weights(context.hw->numHistogramWeights, 1);\n> \n> \t\tmeteringModes_[meteringModeId] = weights;\n> +\t\tavailableMeteringModes.push_back(meteringModeId);\n> \t}\n> \n> +\tAlgorithm::controls_[&controls::AeMeteringMode] =\n> +\t\tControlInfo(availableMeteringModes);\n> +\n> \treturn 0;\n> }\n> \n> @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> \n> \tcontext.ctrlMap.merge(controls());\n> \n> +\tAlgorithm::controls_.merge(ControlInfoMap::Map(controls()));\n> +\n> \treturn 0;\n> }\n> \n> @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \n> \tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n> \tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> +\tcontext.activeState.agc.meteringMode = meteringModes_.begin()->first;\n> \n> \t/*\n> \t * Define the measurement window for AGC as a centered rectangle\n> @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \tcontext.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;\n> \tcontext.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;\n> \n> -\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> \tsetLimits(context.configuration.sensor.minShutterSpeed,\n> \t\t context.configuration.sensor.maxShutterSpeed,\n> \t\t context.configuration.sensor.minAnalogueGain,\n> @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,\n> \t\tframeContext.agc.exposure = agc.manual.exposure;\n> \t\tframeContext.agc.gain = agc.manual.gain;\n> \t}\n> +\n> +\tconst auto &meteringMode = controls.get(controls::AeMeteringMode);\n> +\tif (meteringMode)\n> +\t\tagc.meteringMode = *meteringMode;\n> +\tframeContext.agc.meteringMode = agc.meteringMode;\n> +\n> +\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n> +\tif (exposureMode)\n> +\t\tagc.exposureMode = *exposureMode;\n> +\tframeContext.agc.exposureMode = agc.exposureMode;\n> +\n> +\tconst auto &constraintMode = controls.get(controls::AeConstraintMode);\n> +\tif (constraintMode)\n> +\t\tagc.constraintMode = *constraintMode;\n> +\tframeContext.agc.constraintMode = agc.constraintMode;\n> +\n> +\tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> +\tif (frameDurationLimits)\n> +\t\tagc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());\n> +\tframeContext.agc.maxShutterSpeed = agc.maxShutterSpeed;\n> }\n> \n> /**\n> @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t\tparams->meas.hst_config.hist_weight,\n> \t\tcontext.hw->numHistogramWeights\n> \t};\n> -\t/* \\todo Get this from control */\n> -\tstd::vector &modeWeights = meteringModes_.at(controls::MeteringMatrix);\n> +\tstd::vector &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);\n> \tstd::copy(modeWeights.begin(), modeWeights.end(), weights.begin());\n> \n> \tstd::stringstream str;\n> @@ -289,6 +319,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t\t * frameContext.sensor.exposure;\n> \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> +\tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> \n> \t/* \\todo Use VBlank value calculated from each frame exposure. */\n> \tuint32_t vTotal = context.configuration.sensor.size.height\n> @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> \t\t\t\t * vTotal;\n> \tmetadata.set(controls::FrameDuration, frameDuration.get());\n> +\n> +\tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> +\tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> +\tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> }\n> \n> /**\n> @@ -370,6 +405,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t [](uint32_t x) { return x >> 4; });\n> \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> \n> +\tutils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> +\t\t\t\t\t\t frameContext.agc.maxShutterSpeed);\n> +\tif (!maxShutterSpeed)\n> +\t\tmaxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,\n> +\t\t\t\t\t frameContext.agc.maxShutterSpeed);\n> +\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> +\t\t maxShutterSpeed,\n> +\t\t context.configuration.sensor.minAnalogueGain,\n> +\t\t context.configuration.sensor.maxAnalogueGain);\n> +\n> \t/*\n> \t * The Agc algorithm needs to know the effective exposure value that was\n> \t * applied to the sensor when the statistics were collected.\n> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> index 9454c9a1f..c3a002b85 100644\n> --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> @@ -25,6 +25,8 @@ public:\n> \n> \tbool disabled_;\n> \tbool supportsRaw_;\n> +\n> +\tControlInfoMap::Map controls_;\n> };\n> \n> } /* namespace ipa::rkisp1 */\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 256b75ebc..26c395704 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -68,8 +68,10 @@ struct IPAActiveState {\n> \t\t} automatic;\n> \n> \t\tbool autoEnabled;\n> -\t\tuint32_t constraintMode;\n> -\t\tuint32_t exposureMode;\n> +\t\tint32_t constraintMode;\n> +\t\tint32_t exposureMode;\n> +\t\tint32_t meteringMode;\n> +\t\tutils::Duration maxShutterSpeed;\n> \t} agc;\n> \n> \tstruct {\n> @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> \t\tbool autoEnabled;\n> +\t\tint32_t exposureMode;\n> +\t\tint32_t constraintMode;\n> +\t\tint32_t meteringMode;\n> +\t\tutils::Duration maxShutterSpeed;\n> \t} agc;\n> \n> \tstruct {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 963cdbab4..7716c11bb 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -80,6 +80,7 @@ private:\n> \tstd::map mappedBuffers_;\n> \n> \tControlInfoMap sensorControls_;\n> +\tControlInfoMap::Map algoControls_;\n> \n> \t/* Interface to the Camera Helper */\n> \tstd::unique_ptr camHelper_;\n> @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tfor (auto const &a : algorithms()) {\n> +\t\tAlgorithm *algo = static_cast(a.get());\n> +\n> +\t\t/* \\todo Avoid merging duplicate controls */\n> +\t\tif (!algo->controls_.empty())\n> +\t\t\talgoControls_.merge(ControlInfoMap::Map(algo->controls_));\n\nIt doesn't feel right to have controls_ as public member without an\naccessor function. But maybe it doesn't matter... and Laurent started it\nwith the disabled_ mamber :-) I should use this way of adding controls\nin Gamma also...\n\nThe rest looks good to me.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> +\t}\n> +\n> \t/* Initialize controls. */\n> \tupdateControls(sensorInfo, sensorControls, ipaControls);\n> \n> @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\t\t ControlInfoMap *ipaControls)\n> {\n> \tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> +\tctrlMap.merge(algoControls_);\n> \n> \t/*\n> \t * Compute exposure time limits from the V4L2_CID_EXPOSURE control\n> -- \n> 2.39.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 7459CBDE6B\n\tfor ;\n\tFri, 17 May 2024 10:47:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 701EF6347C;\n\tFri, 17 May 2024 12:47:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1891561A5A\n\tfor ;\n\tFri, 17 May 2024 12:47:05 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:d572:8aa2:3e8e:5b99])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE9BF82E;\n\tFri, 17 May 2024 12:46:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qbLRqT9i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1715942816;\n\tbh=R2W0EY/TvJFU3XS+H9rJfPszixq4k3f/zME4PZWp5Pw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qbLRqT9imueDGFGzUQduXwDlvdT2dqmruuqMdV/B29NtV3UwnUn/iWJxB4zRCRByq\n\toOgfxd6fkwgQJScqRpCN7RWw+R4aN7st87Peww82FwivRm3W8FQRk1ym1jGIsRHeZQ\n\tDYsihKIDO99T4nOqsBIMawxZKyzxId9ZSFMDUynI=","Date":"Fri, 17 May 2024 12:47:01 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","Message-ID":"<20240517104701.vreuvc4tmulv7xgc@jasper>","References":"<20240517080802.3896531-1-paul.elder@ideasonboard.com>\n\t<20240517080802.3896531-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240517080802.3896531-4-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29570,"web_url":"https://patchwork.libcamera.org/comment/29570/","msgid":"","date":"2024-05-20T12:34:56","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Paul, thanks for the patch\n\nOn 17/05/2024 09:08, Paul Elder wrote:\n> Plumb controls for setting metering mode, exposure mode, constraint\n> mode, and frame duration limits. Also report them as available controls,\n> as well as in metadata.\n>\n> While at it, add the missing #include for tuple, as a std::tie is used.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v3:\n> - prevent maxShutterSpeed (for setLimits) from choosing 0 shutter speed\n> if one of the inputs std::min() is not set (this fixes an assertion\n> failure in the exposure mode helper)\n>\n> Changes in v2:\n> - add #include \n> - don't overwrite metering/exposure/constraint mode controls to default\n> if no control is supplied, and retain the old value instead (same for\n> frame duration limits)\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 55 ++++++++++++++++++++++++---\n> src/ipa/rkisp1/algorithms/algorithm.h | 2 +\n> src/ipa/rkisp1/ipa_context.h | 10 ++++-\n> src/ipa/rkisp1/rkisp1.cpp | 10 +++++\n> 4 files changed, 70 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 1c9872d02..ce8beae24 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -10,6 +10,8 @@\n> #include \n> #include \n> #include \n> +#include \n> +#include \n> \n> #include \n> #include \n> @@ -48,6 +50,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tstd::vector availableMeteringModes;\n> \tfor (const auto &[key, value] : yamlMeteringModes.asDict()) {\n> \t\tif (controls::AeMeteringModeNameValueMap.find(key) ==\n> \t\t controls::AeMeteringModeNameValueMap.end()) {\n> @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> \t\t\t\t<< \"Matched metering matrix mode \"\n> \t\t\t\t<< key << \", version \" << version;\n> \n> -\t\t\tmeteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> +\t\t\tint32_t control = controls::AeMeteringModeNameValueMap.at(key);\n> +\t\t\tmeteringModes_[control] = weights;\n> +\t\t\tavailableMeteringModes.push_back(control);\n> \t\t}\n> \t}\n> \n> -\tif (meteringModes_.empty()) {\n> +\tif (availableMeteringModes.empty()) {\n> \t\tint32_t meteringModeId = controls::AeMeteringModeNameValueMap.at(\"MeteringMatrix\");\n> \t\tstd::vector weights(context.hw->numHistogramWeights, 1);\n> \n> \t\tmeteringModes_[meteringModeId] = weights;\n> +\t\tavailableMeteringModes.push_back(meteringModeId);\n> \t}\n> \n> +\tAlgorithm::controls_[&controls::AeMeteringMode] =\n> +\t\tControlInfo(availableMeteringModes);\n> +\n\n\nI was going to say \"shouldn't this be controls()[&controls::AeMeteringMode] instead?\", but I see \nthat the patch adds a new ControlInfoMap to the Algorithm base class. I did a similar thing in the \ncentral AGC series with a ControlInfoMap in the context that algorithms fill with control data - we \nneed to merge those methods and just do one or the other...I don't particularly mind which method we \nsettle on, the only difference I can see at the moment is that the context.ctrlMap allows the \nalgorithms to all fill the same ControlInfoMap and so avoids the needs to merge them all into \nIPARkISP1::algoControls_ with the loop in IPARkISP1::init()\n\n> \treturn 0;\n> }\n> \n> @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> \n> \tcontext.ctrlMap.merge(controls());\n> \n> +\tAlgorithm::controls_.merge(ControlInfoMap::Map(controls()));\n> +\n> \treturn 0;\n> }\n> \n> @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \n> \tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n> \tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> +\tcontext.activeState.agc.meteringMode = meteringModes_.begin()->first;\n> \n> \t/*\n> \t * Define the measurement window for AGC as a centered rectangle\n> @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \tcontext.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;\n> \tcontext.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;\n> \n> -\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> \tsetLimits(context.configuration.sensor.minShutterSpeed,\n> \t\t context.configuration.sensor.maxShutterSpeed,\n> \t\t context.configuration.sensor.minAnalogueGain,\n> @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,\n> \t\tframeContext.agc.exposure = agc.manual.exposure;\n> \t\tframeContext.agc.gain = agc.manual.gain;\n> \t}\n> +\n> +\tconst auto &meteringMode = controls.get(controls::AeMeteringMode);\n> +\tif (meteringMode)\n> +\t\tagc.meteringMode = *meteringMode;\n> +\tframeContext.agc.meteringMode = agc.meteringMode;\n> +\n> +\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n> +\tif (exposureMode)\n> +\t\tagc.exposureMode = *exposureMode;\n> +\tframeContext.agc.exposureMode = agc.exposureMode;\n> +\n> +\tconst auto &constraintMode = controls.get(controls::AeConstraintMode);\n> +\tif (constraintMode)\n> +\t\tagc.constraintMode = *constraintMode;\n> +\tframeContext.agc.constraintMode = agc.constraintMode;\n> +\n> +\tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> +\tif (frameDurationLimits)\n> +\t\tagc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());\n> +\tframeContext.agc.maxShutterSpeed = agc.maxShutterSpeed;\n> }\n> \n> /**\n> @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t\tparams->meas.hst_config.hist_weight,\n> \t\tcontext.hw->numHistogramWeights\n> \t};\n> -\t/* \\todo Get this from control */\n> -\tstd::vector &modeWeights = meteringModes_.at(controls::MeteringMatrix);\n> +\tstd::vector &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);\n> \tstd::copy(modeWeights.begin(), modeWeights.end(), weights.begin());\n> \n> \tstd::stringstream str;\n> @@ -289,6 +319,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t\t * frameContext.sensor.exposure;\n> \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> +\tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> \n> \t/* \\todo Use VBlank value calculated from each frame exposure. */\n> \tuint32_t vTotal = context.configuration.sensor.size.height\n> @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> \t\t\t\t * vTotal;\n> \tmetadata.set(controls::FrameDuration, frameDuration.get());\n> +\n> +\tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> +\tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> +\tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> }\n> \n> /**\n> @@ -370,6 +405,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t [](uint32_t x) { return x >> 4; });\n> \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> \n> +\tutils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> +\t\t\t\t\t\t frameContext.agc.maxShutterSpeed);\n> +\tif (!maxShutterSpeed)\n> +\t\tmaxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,\n> +\t\t\t\t\t frameContext.agc.maxShutterSpeed);\n> +\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> +\t\t maxShutterSpeed,\n> +\t\t context.configuration.sensor.minAnalogueGain,\n> +\t\t context.configuration.sensor.maxAnalogueGain);\n> +\n> \t/*\n> \t * The Agc algorithm needs to know the effective exposure value that was\n> \t * applied to the sensor when the statistics were collected.\n> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> index 9454c9a1f..c3a002b85 100644\n> --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> @@ -25,6 +25,8 @@ public:\n> \n> \tbool disabled_;\n> \tbool supportsRaw_;\n> +\n> +\tControlInfoMap::Map controls_;\n> };\n> \n> } /* namespace ipa::rkisp1 */\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 256b75ebc..26c395704 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -68,8 +68,10 @@ struct IPAActiveState {\n> \t\t} automatic;\n> \n> \t\tbool autoEnabled;\n> -\t\tuint32_t constraintMode;\n> -\t\tuint32_t exposureMode;\n> +\t\tint32_t constraintMode;\n> +\t\tint32_t exposureMode;\n> +\t\tint32_t meteringMode;\n> +\t\tutils::Duration maxShutterSpeed;\n> \t} agc;\n> \n> \tstruct {\n> @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> \t\tbool autoEnabled;\n> +\t\tint32_t exposureMode;\n> +\t\tint32_t constraintMode;\n> +\t\tint32_t meteringMode;\n> +\t\tutils::Duration maxShutterSpeed;\n> \t} agc;\n> \n> \tstruct {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 963cdbab4..7716c11bb 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -80,6 +80,7 @@ private:\n> \tstd::map mappedBuffers_;\n> \n> \tControlInfoMap sensorControls_;\n> +\tControlInfoMap::Map algoControls_;\n> \n> \t/* Interface to the Camera Helper */\n> \tstd::unique_ptr camHelper_;\n> @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\tfor (auto const &a : algorithms()) {\n> +\t\tAlgorithm *algo = static_cast(a.get());\n> +\n> +\t\t/* \\todo Avoid merging duplicate controls */\n> +\t\tif (!algo->controls_.empty())\n> +\t\t\talgoControls_.merge(ControlInfoMap::Map(algo->controls_));\n> +\t}\n> +\n\n\nIf we settle on having a ControlInfoMap in the Algorithm class then I think this loop should move to \nupdateControls() and just merge to ctrlMap directly, and algoControls_ can be dropped.\n\n> \t/* Initialize controls. */\n> \tupdateControls(sensorInfo, sensorControls, ipaControls);\n> \n> @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\t\t ControlInfoMap *ipaControls)\n> {\n> \tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> +\tctrlMap.merge(algoControls_);\n> \n> \t/*\n> \t * Compute exposure time limits from the V4L2_CID_EXPOSURE control","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 64856BDE6B\n\tfor ;\n\tMon, 20 May 2024 12:35:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 77D066347C;\n\tMon, 20 May 2024 14:35: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 4553461A58\n\tfor ;\n\tMon, 20 May 2024 14:34:59 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F1ABF593\n\tfor ;\n\tMon, 20 May 2024 14:34:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Gfh6R/0v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716208488;\n\tbh=+AVZR0vyxaudrCTAio16IJiMW24jslYeuIMhAM9XFbI=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Gfh6R/0vLZlu6NtcB2jT83UA+a9+yYIoigQWoc/HgL5O+Ng3n8sKVtLxrN1KxaknT\n\tS8MkKp/vaozXWqMI7wbPYma7zVV+HhtTWYv0D0y2EJ8xX2v43+UyT/K0JcB+eYtDUm\n\tFnNywxgFwWOHqQwipF39JJ/XNuFL3kI8lTNddNRI=","Message-ID":"","Date":"Mon, 20 May 2024 13:34:56 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","To":"libcamera-devel@lists.libcamera.org","References":"<20240517080802.3896531-1-paul.elder@ideasonboard.com>\n\t<20240517080802.3896531-4-paul.elder@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20240517080802.3896531-4-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29648,"web_url":"https://patchwork.libcamera.org/comment/29648/","msgid":"","date":"2024-05-29T07:44:29","subject":"Re: [PATCH v3 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, May 20, 2024 at 01:34:56PM +0100, Dan Scally wrote:\n> Hi Paul, thanks for the patch\n> \n> On 17/05/2024 09:08, Paul Elder wrote:\n> > Plumb controls for setting metering mode, exposure mode, constraint\n> > mode, and frame duration limits. Also report them as available controls,\n> > as well as in metadata.\n> > \n> > While at it, add the missing #include for tuple, as a std::tie is used.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v3:\n> > - prevent maxShutterSpeed (for setLimits) from choosing 0 shutter speed\n> > if one of the inputs std::min() is not set (this fixes an assertion\n> > failure in the exposure mode helper)\n> > \n> > Changes in v2:\n> > - add #include \n> > - don't overwrite metering/exposure/constraint mode controls to default\n> > if no control is supplied, and retain the old value instead (same for\n> > frame duration limits)\n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 55 ++++++++++++++++++++++++---\n> > src/ipa/rkisp1/algorithms/algorithm.h | 2 +\n> > src/ipa/rkisp1/ipa_context.h | 10 ++++-\n> > src/ipa/rkisp1/rkisp1.cpp | 10 +++++\n> > 4 files changed, 70 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 1c9872d02..ce8beae24 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -10,6 +10,8 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > +#include \n> > #include \n> > #include \n> > @@ -48,6 +50,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> > \t\treturn -EINVAL;\n> > \t}\n> > +\tstd::vector availableMeteringModes;\n> > \tfor (const auto &[key, value] : yamlMeteringModes.asDict()) {\n> > \t\tif (controls::AeMeteringModeNameValueMap.find(key) ==\n> > \t\t controls::AeMeteringModeNameValueMap.end()) {\n> > @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> > \t\t\t\t<< \"Matched metering matrix mode \"\n> > \t\t\t\t<< key << \", version \" << version;\n> > -\t\t\tmeteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> > +\t\t\tint32_t control = controls::AeMeteringModeNameValueMap.at(key);\n> > +\t\t\tmeteringModes_[control] = weights;\n> > +\t\t\tavailableMeteringModes.push_back(control);\n> > \t\t}\n> > \t}\n> > -\tif (meteringModes_.empty()) {\n> > +\tif (availableMeteringModes.empty()) {\n> > \t\tint32_t meteringModeId = controls::AeMeteringModeNameValueMap.at(\"MeteringMatrix\");\n> > \t\tstd::vector weights(context.hw->numHistogramWeights, 1);\n> > \t\tmeteringModes_[meteringModeId] = weights;\n> > +\t\tavailableMeteringModes.push_back(meteringModeId);\n> > \t}\n> > +\tAlgorithm::controls_[&controls::AeMeteringMode] =\n> > +\t\tControlInfo(availableMeteringModes);\n> > +\n> \n> \n> I was going to say \"shouldn't this be controls()[&controls::AeMeteringMode]\n> instead?\", but I see that the patch adds a new ControlInfoMap to the\n> Algorithm base class. I did a similar thing in the central AGC series with a\n> ControlInfoMap in the context that algorithms fill with control data - we\n> need to merge those methods and just do one or the other...I don't\n> particularly mind which method we settle on, the only difference I can see\n> at the moment is that the context.ctrlMap allows the algorithms to all fill\n> the same ControlInfoMap and so avoids the needs to merge them all into\n> IPARkISP1::algoControls_ with the loop in IPARkISP1::init()\n> \n\nYeah... I needed it for rkisp1's algorithms for reporting and merging\nthe valid controls and values, so to consolidate it with the one that\nyou added in agc I have them merged in ipa::rkisp1::algorithms::Agc::init():\n\nAlgorithm::controls_.merge(ControlInfoMap::Map(controls()));\n\n\nPaul\n\n> > \treturn 0;\n> > }\n> > @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> > \tcontext.ctrlMap.merge(controls());\n> > +\tAlgorithm::controls_.merge(ControlInfoMap::Map(controls()));\n> > +\n> > \treturn 0;\n> > }\n> > @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \tcontext.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > \tcontext.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> > +\tcontext.activeState.agc.meteringMode = meteringModes_.begin()->first;\n> > \t/*\n> > \t * Define the measurement window for AGC as a centered rectangle\n> > @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \tcontext.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;\n> > \tcontext.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;\n> > -\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> > \tsetLimits(context.configuration.sensor.minShutterSpeed,\n> > \t\t context.configuration.sensor.maxShutterSpeed,\n> > \t\t context.configuration.sensor.minAnalogueGain,\n> > @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,\n> > \t\tframeContext.agc.exposure = agc.manual.exposure;\n> > \t\tframeContext.agc.gain = agc.manual.gain;\n> > \t}\n> > +\n> > +\tconst auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > +\tif (meteringMode)\n> > +\t\tagc.meteringMode = *meteringMode;\n> > +\tframeContext.agc.meteringMode = agc.meteringMode;\n> > +\n> > +\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n> > +\tif (exposureMode)\n> > +\t\tagc.exposureMode = *exposureMode;\n> > +\tframeContext.agc.exposureMode = agc.exposureMode;\n> > +\n> > +\tconst auto &constraintMode = controls.get(controls::AeConstraintMode);\n> > +\tif (constraintMode)\n> > +\t\tagc.constraintMode = *constraintMode;\n> > +\tframeContext.agc.constraintMode = agc.constraintMode;\n> > +\n> > +\tconst auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > +\tif (frameDurationLimits)\n> > +\t\tagc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());\n> > +\tframeContext.agc.maxShutterSpeed = agc.maxShutterSpeed;\n> > }\n> > /**\n> > @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > \t\tparams->meas.hst_config.hist_weight,\n> > \t\tcontext.hw->numHistogramWeights\n> > \t};\n> > -\t/* \\todo Get this from control */\n> > -\tstd::vector &modeWeights = meteringModes_.at(controls::MeteringMatrix);\n> > +\tstd::vector &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);\n> > \tstd::copy(modeWeights.begin(), modeWeights.end(), weights.begin());\n> > \tstd::stringstream str;\n> > @@ -289,6 +319,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > \t\t\t\t * frameContext.sensor.exposure;\n> > \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> > +\tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> > \t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > \tuint32_t vTotal = context.configuration.sensor.size.height\n> > @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > \tutils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > \t\t\t\t * vTotal;\n> > \tmetadata.set(controls::FrameDuration, frameDuration.get());\n> > +\n> > +\tmetadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> > +\tmetadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> > +\tmetadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);\n> > }\n> > /**\n> > @@ -370,6 +405,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > \t\t [](uint32_t x) { return x >> 4; });\n> > \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > +\tutils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> > +\t\t\t\t\t\t frameContext.agc.maxShutterSpeed);\n> > +\tif (!maxShutterSpeed)\n> > +\t\tmaxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,\n> > +\t\t\t\t\t frameContext.agc.maxShutterSpeed);\n> > +\tsetLimits(context.configuration.sensor.minShutterSpeed,\n> > +\t\t maxShutterSpeed,\n> > +\t\t context.configuration.sensor.minAnalogueGain,\n> > +\t\t context.configuration.sensor.maxAnalogueGain);\n> > +\n> > \t/*\n> > \t * The Agc algorithm needs to know the effective exposure value that was\n> > \t * applied to the sensor when the statistics were collected.\n> > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> > index 9454c9a1f..c3a002b85 100644\n> > --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> > +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> > @@ -25,6 +25,8 @@ public:\n> > \tbool disabled_;\n> > \tbool supportsRaw_;\n> > +\n> > +\tControlInfoMap::Map controls_;\n> > };\n> > } /* namespace ipa::rkisp1 */\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 256b75ebc..26c395704 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -68,8 +68,10 @@ struct IPAActiveState {\n> > \t\t} automatic;\n> > \t\tbool autoEnabled;\n> > -\t\tuint32_t constraintMode;\n> > -\t\tuint32_t exposureMode;\n> > +\t\tint32_t constraintMode;\n> > +\t\tint32_t exposureMode;\n> > +\t\tint32_t meteringMode;\n> > +\t\tutils::Duration maxShutterSpeed;\n> > \t} agc;\n> > \tstruct {\n> > @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {\n> > \t\tuint32_t exposure;\n> > \t\tdouble gain;\n> > \t\tbool autoEnabled;\n> > +\t\tint32_t exposureMode;\n> > +\t\tint32_t constraintMode;\n> > +\t\tint32_t meteringMode;\n> > +\t\tutils::Duration maxShutterSpeed;\n> > \t} agc;\n> > \tstruct {\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 963cdbab4..7716c11bb 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -80,6 +80,7 @@ private:\n> > \tstd::map mappedBuffers_;\n> > \tControlInfoMap sensorControls_;\n> > +\tControlInfoMap::Map algoControls_;\n> > \t/* Interface to the Camera Helper */\n> > \tstd::unique_ptr camHelper_;\n> > @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > \tif (ret)\n> > \t\treturn ret;\n> > +\tfor (auto const &a : algorithms()) {\n> > +\t\tAlgorithm *algo = static_cast(a.get());\n> > +\n> > +\t\t/* \\todo Avoid merging duplicate controls */\n> > +\t\tif (!algo->controls_.empty())\n> > +\t\t\talgoControls_.merge(ControlInfoMap::Map(algo->controls_));\n> > +\t}\n> > +\n> \n> \n> If we settle on having a ControlInfoMap in the Algorithm class then I think\n> this loop should move to updateControls() and just merge to ctrlMap\n> directly, and algoControls_ can be dropped.\n> \n> > \t/* Initialize controls. */\n> > \tupdateControls(sensorInfo, sensorControls, ipaControls);\n> > @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > \t\t\t ControlInfoMap *ipaControls)\n> > {\n> > \tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> > +\tctrlMap.merge(algoControls_);\n> > \t/*\n> > \t * Compute exposure time limits from the V4L2_CID_EXPOSURE control","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 AC1C9BD87C\n\tfor ;\n\tWed, 29 May 2024 07:44:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4CFB634B2;\n\tWed, 29 May 2024 09:44:37 +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 595E161A46\n\tfor ;\n\tWed, 29 May 2024 09:44:35 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D8D5566F;\n\tWed, 29 May 2024 09:44:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"K6hAJuIT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716968671;\n\tbh=d8yWNXsZlP3i8AKcOiwZb77jg11pbeVU5lhbYdC7GaQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K6hAJuITZinmK8rQqNb65BdiJLJZt7sEJ20eQsaXmMQOPG5Iyy3UQVx13SznoLzA9\n\tpVfTD4/6fnn4NC9t3M6spV93QrJ02/vD9cyw6xz6LKSoDfnE0sShlYy/kGkokTON6G\n\tMY9YXE7dfpdrGPAwoCFtGHJus8o+bif037EXZXQg=","Date":"Wed, 29 May 2024 16:44:29 +0900","From":"Paul Elder ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","Message-ID":"","References":"<20240517080802.3896531-1-paul.elder@ideasonboard.com>\n\t<20240517080802.3896531-4-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]