[{"id":29813,"web_url":"https://patchwork.libcamera.org/comment/29813/","msgid":"<171803012812.2248009.16116703110912470753@ping.linuxembedded.co.uk>","date":"2024-06-10T14:35:28","subject":"Re: [PATCH v5 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-06-07 09:03:30)\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> Reviewed-by: Stefan Klug \n> \n> ---\n> No change in v5\n> \n> No change in v4\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 e9f1f2095..c6d03c3b2 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> return -EINVAL;\n> }\n> \n> + std::vector availableMeteringModes;\n> for (const auto &[key, value] : yamlMeteringModes.asDict()) {\n> if (controls::AeMeteringModeNameValueMap.find(key) ==\n> controls::AeMeteringModeNameValueMap.end()) {\n> @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> << \"Matched metering matrix mode \"\n> << key << \", version \" << version;\n> \n> - meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> + int32_t control = controls::AeMeteringModeNameValueMap.at(key);\n> + meteringModes_[control] = weights;\n> + availableMeteringModes.push_back(control);\n> }\n> }\n> \n> - if (meteringModes_.empty()) {\n> + if (availableMeteringModes.empty()) {\n> int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at(\"MeteringMatrix\");\n> std::vector weights(context.hw->numHistogramWeights, 1);\n> \n> meteringModes_[meteringModeId] = weights;\n> + availableMeteringModes.push_back(meteringModeId);\n> }\n> \n> + Algorithm::controls_[&controls::AeMeteringMode] =\n> + ControlInfo(availableMeteringModes);\n> +\n> return 0;\n> }\n> \n> @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> \n> context.ctrlMap.merge(controls());\n> \n> + Algorithm::controls_.merge(ControlInfoMap::Map(controls()));\n> +\n> return 0;\n> }\n> \n> @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \n> context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> + context.activeState.agc.meteringMode = meteringModes_.begin()->first;\n> \n> /*\n> * Define the measurement window for AGC as a centered rectangle\n> @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;\n> context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;\n> \n> - /* \\todo Run this again when FrameDurationLimits is passed in */\n> setLimits(context.configuration.sensor.minShutterSpeed,\n> context.configuration.sensor.maxShutterSpeed,\n> context.configuration.sensor.minAnalogueGain,\n> @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,\n> frameContext.agc.exposure = agc.manual.exposure;\n> frameContext.agc.gain = agc.manual.gain;\n> }\n> +\n> + const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> + if (meteringMode)\n> + agc.meteringMode = *meteringMode;\n> + frameContext.agc.meteringMode = agc.meteringMode;\n> +\n> + const auto &exposureMode = controls.get(controls::AeExposureMode);\n> + if (exposureMode)\n> + agc.exposureMode = *exposureMode;\n> + frameContext.agc.exposureMode = agc.exposureMode;\n> +\n> + const auto &constraintMode = controls.get(controls::AeConstraintMode);\n> + if (constraintMode)\n> + agc.constraintMode = *constraintMode;\n> + frameContext.agc.constraintMode = agc.constraintMode;\n> +\n> + const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> + if (frameDurationLimits)\n> + agc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());\n> + frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;\n> }\n> \n> /**\n> @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> params->meas.hst_config.hist_weight,\n> context.hw->numHistogramWeights\n> };\n> - /* \\todo Get this from control */\n> - std::vector &modeWeights = meteringModes_.at(controls::MeteringMatrix);\n> + std::vector &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);\n\nI'm worried about any potential race being introduced here if requests\nare queued late (after the frame is already set to be processed).\n\nI know that scenario is not unique to this patch - but I can 'see' it\nbeing something that would be impacted here because of the reference\nabove potentially being uninitialised?\n\nI wonder if we need to catch that during the top level and if we are\nabout to call prepare on a frameContext for a sequence that has not had\nqueueRequest called on it - we should probably copy the previous\nframeContext or such.\n\nBut that's probably something to handle during the PFC reworks not this\npatch specifically as the issue will apply to all algorithms, not just\nthis one.\n\n\n\n\n> std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());\n> \n> std::stringstream str;\n> @@ -289,6 +319,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> * frameContext.sensor.exposure;\n> metadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> metadata.set(controls::ExposureTime, exposureTime.get());\n> + metadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> \n> /* \\todo Use VBlank value calculated from each frame exposure. */\n> uint32_t vTotal = context.configuration.sensor.size.height\n> @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> utils::Duration frameDuration = context.configuration.sensor.lineDuration\n> * vTotal;\n> metadata.set(controls::FrameDuration, frameDuration.get());\n> +\n> + metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> + metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> + metadata.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> [](uint32_t x) { return x >> 4; });\n> expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> \n> + utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> + frameContext.agc.maxShutterSpeed);\n> + if (!maxShutterSpeed)\n> + maxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,\n> + frameContext.agc.maxShutterSpeed);\n> + setLimits(context.configuration.sensor.minShutterSpeed,\n> + maxShutterSpeed,\n> + context.configuration.sensor.minAnalogueGain,\n> + context.configuration.sensor.maxAnalogueGain);\n> +\n> /*\n> * The Agc algorithm needs to know the effective exposure value that was\n> * 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 715cfcd82..93a4b548c 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> bool disabled_;\n> bool supportsRaw_;\n> +\n> + ControlInfoMap::Map controls_;\n\n\n\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 bd02a7a24..1cad512e1 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> } automatic;\n> \n> bool autoEnabled;\n> - uint32_t constraintMode;\n> - uint32_t exposureMode;\n> + int32_t constraintMode;\n> + int32_t exposureMode;\n> + int32_t meteringMode;\n> + utils::Duration maxShutterSpeed;\n> } agc;\n> \n> struct {\n> @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {\n> uint32_t exposure;\n> double gain;\n> bool autoEnabled;\n> + int32_t exposureMode;\n> + int32_t constraintMode;\n> + int32_t meteringMode;\n> + utils::Duration maxShutterSpeed;\n> } agc;\n> \n> struct {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 6687c91e7..12b3aedd2 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n\nAll the changes to rkisp1 here should really be in a standalone commit.\n\nI'm surprised we don't already have this handled, but it's core to\nrkisp1 IPA - not the AGC algorithm.\n\nAnd ideally that patch should be in a series that also drops\nrkisp1Controls and moves those to their respective algo implementations!\n\nErr ... wait - I think it /is/ already being handled by the core...\n\n\n> @@ -80,6 +80,7 @@ private:\n> std::map mappedBuffers_;\n> \n> ControlInfoMap sensorControls_;\n> + ControlInfoMap::Map algoControls_;\n> \n> /* Interface to the Camera Helper */\n> std::unique_ptr camHelper_;\n> @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> if (ret)\n> return ret;\n> \n> + for (auto const &a : algorithms()) {\n> + Algorithm *algo = static_cast(a.get());\n> +\n> + /* \\todo Avoid merging duplicate controls */\n\nI think that's already the case. algoControls_ can't merge duplicates,\nit can either overwrite or drop the incoming ones (based on a\nparameter).\n\nIn this case, we /shouldn't/ have duplicates - so I'd keep this as is\nwith the default MergePolicy which I believe will shout out a warning if\nsomeone tries to merge a control that's already added.\n\nIn other words, I think this todo comment can be dropped now.\n\n\nSo in fact - I think this should all be dropped here as there is already\na way to register additional controls by adding them to the IPA Context\nctlrMap.\n\nSee Stefan's recent \"ipa: rkisp1: Add GammaOutCorrection algorithm\" as\nan example.\n\nOr did I miss something different about what this is doing here?\n\n\n\n> + if (!algo->controls_.empty())\n> + algoControls_.merge(ControlInfoMap::Map(algo->controls_));\n> + }\n> +\n> /* Initialize controls. */\n> updateControls(sensorInfo, sensorControls, ipaControls);\n> \n> @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> ControlInfoMap *ipaControls)\n> {\n> ControlInfoMap::Map ctrlMap = rkisp1Controls;\n> + ctrlMap.merge(algoControls_);\n> \n> /*\n> * 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 E199BC31E9\n\tfor ;\n\tMon, 10 Jun 2024 14:35:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8A2465466;\n\tMon, 10 Jun 2024 16:35:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17652634D5\n\tfor ;\n\tMon, 10 Jun 2024 16:35:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D899A397;\n\tMon, 10 Jun 2024 16:35:18 +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=\"ZKoYlRiC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718030118;\n\tbh=scwzsSMfD5RhcFUddy7iF6IAAfUPtPrYG9c14wTHhts=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZKoYlRiCVh/EJXo3Dr0Y4k3o7Ndm9QP0r2TcAFvl7GrTZHfaw29wlH/sk5cVeq46Y\n\tp/3JZQ6VgUjGFo9VXnk53LjVxqjiupUoRynbo8Ge8ERpSn9aPIm8VjL1dsrM2xQm6p\n\t0g+u4g7CHW+8PN3i8hbTyLJJG4Dgp7d/KqBsdgwI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240607080330.2667994-4-paul.elder@ideasonboard.com>","References":"<20240607080330.2667994-1-paul.elder@ideasonboard.com>\n\t<20240607080330.2667994-4-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v5 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","From":"Kieran Bingham ","Cc":"Paul Elder ,\n\tStefan Klug ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 10 Jun 2024 15:35:28 +0100","Message-ID":"<171803012812.2248009.16116703110912470753@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":29835,"web_url":"https://patchwork.libcamera.org/comment/29835/","msgid":"<20240611142144.GA26595@pendragon.ideasonboard.com>","date":"2024-06-11T14:21:44","subject":"Re: [PATCH v5 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 10, 2024 at 03:35:28PM +0100, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-06-07 09:03:30)\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> > Reviewed-by: Stefan Klug \n> > \n> > ---\n> > No change in v5\n> > \n> > No change in v4\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 e9f1f2095..c6d03c3b2 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> > return -EINVAL;\n> > }\n> > \n> > + std::vector availableMeteringModes;\n> > for (const auto &[key, value] : yamlMeteringModes.asDict()) {\n> > if (controls::AeMeteringModeNameValueMap.find(key) ==\n> > controls::AeMeteringModeNameValueMap.end()) {\n> > @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> > << \"Matched metering matrix mode \"\n> > << key << \", version \" << version;\n> > \n> > - meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> > + int32_t control = controls::AeMeteringModeNameValueMap.at(key);\n> > + meteringModes_[control] = weights;\n> > + availableMeteringModes.push_back(control);\n> > }\n> > }\n> > \n> > - if (meteringModes_.empty()) {\n> > + if (availableMeteringModes.empty()) {\n> > int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at(\"MeteringMatrix\");\n> > std::vector weights(context.hw->numHistogramWeights, 1);\n> > \n> > meteringModes_[meteringModeId] = weights;\n> > + availableMeteringModes.push_back(meteringModeId);\n> > }\n> > \n> > + Algorithm::controls_[&controls::AeMeteringMode] =\n> > + ControlInfo(availableMeteringModes);\n\nI think you can avoid the availableMeteringModes variable by using\n\n\tAlgorithm::controls_[&controls::AeMeteringMode] =\n\t\tControlInfo(utils::map_keys(meteringModes_));\n\n> > +\n> > return 0;\n> > }\n> > \n> > @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> > \n> > context.ctrlMap.merge(controls());\n> > \n> > + Algorithm::controls_.merge(ControlInfoMap::Map(controls()));\n> > +\n> > return 0;\n> > }\n> > \n> > @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \n> > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> > + context.activeState.agc.meteringMode = meteringModes_.begin()->first;\n> > \n> > /*\n> > * Define the measurement window for AGC as a centered rectangle\n> > @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;\n> > context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;\n> > \n> > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > setLimits(context.configuration.sensor.minShutterSpeed,\n> > context.configuration.sensor.maxShutterSpeed,\n> > context.configuration.sensor.minAnalogueGain,\n> > @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,\n> > frameContext.agc.exposure = agc.manual.exposure;\n> > frameContext.agc.gain = agc.manual.gain;\n> > }\n> > +\n> > + const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > + if (meteringMode)\n> > + agc.meteringMode = *meteringMode;\n> > + frameContext.agc.meteringMode = agc.meteringMode;\n> > +\n> > + const auto &exposureMode = controls.get(controls::AeExposureMode);\n> > + if (exposureMode)\n> > + agc.exposureMode = *exposureMode;\n> > + frameContext.agc.exposureMode = agc.exposureMode;\n> > +\n> > + const auto &constraintMode = controls.get(controls::AeConstraintMode);\n> > + if (constraintMode)\n> > + agc.constraintMode = *constraintMode;\n> > + frameContext.agc.constraintMode = agc.constraintMode;\n> > +\n> > + const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > + if (frameDurationLimits)\n> > + agc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());\n> > + frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;\n> > }\n> > \n> > /**\n> > @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > params->meas.hst_config.hist_weight,\n> > context.hw->numHistogramWeights\n> > };\n> > - /* \\todo Get this from control */\n> > - std::vector &modeWeights = meteringModes_.at(controls::MeteringMatrix);\n> > + std::vector &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);\n> \n> I'm worried about any potential race being introduced here if requests\n> are queued late (after the frame is already set to be processed).\n> \n> I know that scenario is not unique to this patch - but I can 'see' it\n> being something that would be impacted here because of the reference\n> above potentially being uninitialised?\n> \n> I wonder if we need to catch that during the top level and if we are\n> about to call prepare on a frameContext for a sequence that has not had\n> queueRequest called on it - we should probably copy the previous\n> frameContext or such.\n> \n> But that's probably something to handle during the PFC reworks not this\n> patch specifically as the issue will apply to all algorithms, not just\n> this one.\n\nYes, there's an issue here, and no, Paul doesn't have to address it now\n(Paul says \"yay thanks!\" during our live review session :-)). It will be\naddressed as part of the PFC work.\n\n> > std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());\n> > \n> > std::stringstream str;\n> > @@ -289,6 +319,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > * frameContext.sensor.exposure;\n> > metadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > metadata.set(controls::ExposureTime, exposureTime.get());\n> > + metadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> > \n> > /* \\todo Use VBlank value calculated from each frame exposure. */\n> > uint32_t vTotal = context.configuration.sensor.size.height\n> > @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > utils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > * vTotal;\n> > metadata.set(controls::FrameDuration, frameDuration.get());\n> > +\n> > + metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> > + metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> > + metadata.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> > [](uint32_t x) { return x >> 4; });\n> > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > \n> > + utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> > + frameContext.agc.maxShutterSpeed);\n> > + if (!maxShutterSpeed)\n> > + maxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,\n> > + frameContext.agc.maxShutterSpeed);\n\nThis looks a bit weird. Could we instead initialize\nactiveState.agc.maxShutterSpeed somewhere (in configure() maybe ?) to a\nsensible default value ? I think it should be initialized to whatever\ndefault value the IPA module reports for the FrameDurationLimits.\n\n> > + setLimits(context.configuration.sensor.minShutterSpeed,\n> > + maxShutterSpeed,\n> > + context.configuration.sensor.minAnalogueGain,\n> > + context.configuration.sensor.maxAnalogueGain);\n> > +\n> > /*\n> > * The Agc algorithm needs to know the effective exposure value that was\n> > * 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 715cfcd82..93a4b548c 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> > bool disabled_;\n> > bool supportsRaw_;\n> > +\n> > + ControlInfoMap::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 bd02a7a24..1cad512e1 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> > } automatic;\n> > \n> > bool autoEnabled;\n> > - uint32_t constraintMode;\n> > - uint32_t exposureMode;\n> > + int32_t constraintMode;\n> > + int32_t exposureMode;\n> > + int32_t meteringMode;\n\nI don't think those could take negative values. They're essentially\nenums, which we store as int32_t internally. It would be nice if the\ncompiler could verify that we're using them correctly. We seem to have,\nfor instance, enum AeConstraintModeEnum in the control generated\nheaders. Should we use that ?\n\n> > + utils::Duration maxShutterSpeed;\n> > } agc;\n> > \n> > struct {\n> > @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {\n> > uint32_t exposure;\n> > double gain;\n> > bool autoEnabled;\n> > + int32_t exposureMode;\n> > + int32_t constraintMode;\n> > + int32_t meteringMode;\n> > + utils::Duration maxShutterSpeed;\n> > } agc;\n> > \n> > struct {\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 6687c91e7..12b3aedd2 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> \n> All the changes to rkisp1 here should really be in a standalone commit.\n> \n> I'm surprised we don't already have this handled, but it's core to\n> rkisp1 IPA - not the AGC algorithm.\n> \n> And ideally that patch should be in a series that also drops\n> rkisp1Controls and moves those to their respective algo implementations!\n> \n> Err ... wait - I think it /is/ already being handled by the core...\n> \n> > @@ -80,6 +80,7 @@ private:\n> > std::map mappedBuffers_;\n> > \n> > ControlInfoMap sensorControls_;\n> > + ControlInfoMap::Map algoControls_;\n> > \n> > /* Interface to the Camera Helper */\n> > std::unique_ptr camHelper_;\n> > @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > if (ret)\n> > return ret;\n> > \n> > + for (auto const &a : algorithms()) {\n> > + Algorithm *algo = static_cast(a.get());\n> > +\n> > + /* \\todo Avoid merging duplicate controls */\n> \n> I think that's already the case. algoControls_ can't merge duplicates,\n> it can either overwrite or drop the incoming ones (based on a\n> parameter).\n> \n> In this case, we /shouldn't/ have duplicates - so I'd keep this as is\n> with the default MergePolicy which I believe will shout out a warning if\n> someone tries to merge a control that's already added.\n> \n> In other words, I think this todo comment can be dropped now.\n> \n> \n> So in fact - I think this should all be dropped here as there is already\n> a way to register additional controls by adding them to the IPA Context\n> ctlrMap.\n> \n> See Stefan's recent \"ipa: rkisp1: Add GammaOutCorrection algorithm\" as\n> an example.\n> \n> Or did I miss something different about what this is doing here?\n> \n> > + if (!algo->controls_.empty())\n> > + algoControls_.merge(ControlInfoMap::Map(algo->controls_));\n> > + }\n> > +\n> > /* Initialize controls. */\n> > updateControls(sensorInfo, sensorControls, ipaControls);\n> > \n> > @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > ControlInfoMap *ipaControls)\n> > {\n> > ControlInfoMap::Map ctrlMap = rkisp1Controls;\n> > + ctrlMap.merge(algoControls_);\n> > \n> > /*\n> > * 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 E88CFBD87C\n\tfor ;\n\tTue, 11 Jun 2024 14:22:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E615065455;\n\tTue, 11 Jun 2024 16:22:05 +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 9257D61A26\n\tfor ;\n\tTue, 11 Jun 2024 16:22:04 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B4BF29A;\n\tTue, 11 Jun 2024 16:21:51 +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=\"H0YYwcT8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718115711;\n\tbh=lSG/LJnjNfWpPRuC++s4DeCNMaeE5ml9weqtv/n/9+8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H0YYwcT8Iagj7wuGLY61DCrIUV9+P91KAsZ3yHVKpzrbZVtOdIJu4dV4STcLNOY67\n\tF/4OTSPaHdwoXM2DSyX0bSMMZDHCXriKJSI55d3bhY1KLmK+3CAp+enYBXv6VRw+ew\n\tvfn46FckQaMw2UYRNTKpYZgnSTkgv3cHMWJpubv4=","Date":"Tue, 11 Jun 2024 17:21:44 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH v5 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","Message-ID":"<20240611142144.GA26595@pendragon.ideasonboard.com>","References":"<20240607080330.2667994-1-paul.elder@ideasonboard.com>\n\t<20240607080330.2667994-4-paul.elder@ideasonboard.com>\n\t<171803012812.2248009.16116703110912470753@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171803012812.2248009.16116703110912470753@ping.linuxembedded.co.uk>","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":29843,"web_url":"https://patchwork.libcamera.org/comment/29843/","msgid":"<20240611202458.GA8956@pendragon.ideasonboard.com>","date":"2024-06-11T20:24:58","subject":"Re: [PATCH v5 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jun 11, 2024 at 05:21:44PM +0300, Laurent Pinchart wrote:\n> On Mon, Jun 10, 2024 at 03:35:28PM +0100, Kieran Bingham wrote:\n> > Quoting Paul Elder (2024-06-07 09:03:30)\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> > > Reviewed-by: Stefan Klug \n> > > \n> > > ---\n> > > No change in v5\n> > > \n> > > No change in v4\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 e9f1f2095..c6d03c3b2 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> > > return -EINVAL;\n> > > }\n> > > \n> > > + std::vector availableMeteringModes;\n> > > for (const auto &[key, value] : yamlMeteringModes.asDict()) {\n> > > if (controls::AeMeteringModeNameValueMap.find(key) ==\n> > > controls::AeMeteringModeNameValueMap.end()) {\n> > > @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,\n> > > << \"Matched metering matrix mode \"\n> > > << key << \", version \" << version;\n> > > \n> > > - meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;\n> > > + int32_t control = controls::AeMeteringModeNameValueMap.at(key);\n> > > + meteringModes_[control] = weights;\n> > > + availableMeteringModes.push_back(control);\n> > > }\n> > > }\n> > > \n> > > - if (meteringModes_.empty()) {\n> > > + if (availableMeteringModes.empty()) {\n> > > int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at(\"MeteringMatrix\");\n> > > std::vector weights(context.hw->numHistogramWeights, 1);\n> > > \n> > > meteringModes_[meteringModeId] = weights;\n> > > + availableMeteringModes.push_back(meteringModeId);\n> > > }\n> > > \n> > > + Algorithm::controls_[&controls::AeMeteringMode] =\n> > > + ControlInfo(availableMeteringModes);\n> \n> I think you can avoid the availableMeteringModes variable by using\n> \n> \tAlgorithm::controls_[&controls::AeMeteringMode] =\n> \t\tControlInfo(utils::map_keys(meteringModes_));\n> \n> > > +\n> > > return 0;\n> > > }\n> > > \n> > > @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> > > \n> > > context.ctrlMap.merge(controls());\n> > > \n> > > + Algorithm::controls_.merge(ControlInfoMap::Map(controls()));\n> > > +\n> > > return 0;\n> > > }\n> > > \n> > > @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > \n> > > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> > > + context.activeState.agc.meteringMode = meteringModes_.begin()->first;\n> > > \n> > > /*\n> > > * Define the measurement window for AGC as a centered rectangle\n> > > @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;\n> > > context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;\n> > > \n> > > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > > setLimits(context.configuration.sensor.minShutterSpeed,\n> > > context.configuration.sensor.maxShutterSpeed,\n> > > context.configuration.sensor.minAnalogueGain,\n> > > @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,\n> > > frameContext.agc.exposure = agc.manual.exposure;\n> > > frameContext.agc.gain = agc.manual.gain;\n> > > }\n> > > +\n> > > + const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > > + if (meteringMode)\n> > > + agc.meteringMode = *meteringMode;\n> > > + frameContext.agc.meteringMode = agc.meteringMode;\n> > > +\n> > > + const auto &exposureMode = controls.get(controls::AeExposureMode);\n> > > + if (exposureMode)\n> > > + agc.exposureMode = *exposureMode;\n> > > + frameContext.agc.exposureMode = agc.exposureMode;\n> > > +\n> > > + const auto &constraintMode = controls.get(controls::AeConstraintMode);\n> > > + if (constraintMode)\n> > > + agc.constraintMode = *constraintMode;\n> > > + frameContext.agc.constraintMode = agc.constraintMode;\n> > > +\n> > > + const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > > + if (frameDurationLimits)\n> > > + agc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());\n> > > + frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;\n> > > }\n> > > \n> > > /**\n> > > @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > params->meas.hst_config.hist_weight,\n> > > context.hw->numHistogramWeights\n> > > };\n> > > - /* \\todo Get this from control */\n> > > - std::vector &modeWeights = meteringModes_.at(controls::MeteringMatrix);\n> > > + std::vector &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);\n> > \n> > I'm worried about any potential race being introduced here if requests\n> > are queued late (after the frame is already set to be processed).\n> > \n> > I know that scenario is not unique to this patch - but I can 'see' it\n> > being something that would be impacted here because of the reference\n> > above potentially being uninitialised?\n> > \n> > I wonder if we need to catch that during the top level and if we are\n> > about to call prepare on a frameContext for a sequence that has not had\n> > queueRequest called on it - we should probably copy the previous\n> > frameContext or such.\n> > \n> > But that's probably something to handle during the PFC reworks not this\n> > patch specifically as the issue will apply to all algorithms, not just\n> > this one.\n> \n> Yes, there's an issue here, and no, Paul doesn't have to address it now\n> (Paul says \"yay thanks!\" during our live review session :-)). It will be\n> addressed as part of the PFC work.\n> \n> > > std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());\n> > > \n> > > std::stringstream str;\n> > > @@ -289,6 +319,7 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > * frameContext.sensor.exposure;\n> > > metadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > > metadata.set(controls::ExposureTime, exposureTime.get());\n> > > + metadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> > > \n> > > /* \\todo Use VBlank value calculated from each frame exposure. */\n> > > uint32_t vTotal = context.configuration.sensor.size.height\n> > > @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > > utils::Duration frameDuration = context.configuration.sensor.lineDuration\n> > > * vTotal;\n> > > metadata.set(controls::FrameDuration, frameDuration.get());\n> > > +\n> > > + metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);\n> > > + metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);\n> > > + metadata.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> > > [](uint32_t x) { return x >> 4; });\n> > > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > > \n> > > + utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> > > + frameContext.agc.maxShutterSpeed);\n> > > + if (!maxShutterSpeed)\n> > > + maxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,\n> > > + frameContext.agc.maxShutterSpeed);\n> \n> This looks a bit weird. Could we instead initialize\n> activeState.agc.maxShutterSpeed somewhere (in configure() maybe ?) to a\n> sensible default value ? I think it should be initialized to whatever\n> default value the IPA module reports for the FrameDurationLimits.\n\nAnother comment on FrameDurationLimits. At the moment, the control is\ninserted in the control info map in IPARkISP1::updateControls(). I think\nit should be moved to the AGC class, as well as the\ncontrols::ExposureTime and controls::AnalogueGain controls. This means\nthat the Algorithm::configure() function will need to update the control\ninfo map. I haven't checked if that would be straightforward or would\nrequire more refactoring.\n\nAs this is a good chunk of additional rework it doesn't belong to this\npatch, and isn't a blocker to merge this series. A \\todo comment would\nbe here too, and a patch on top of this series would be splendid :-)\n\n> > > + setLimits(context.configuration.sensor.minShutterSpeed,\n> > > + maxShutterSpeed,\n> > > + context.configuration.sensor.minAnalogueGain,\n> > > + context.configuration.sensor.maxAnalogueGain);\n> > > +\n> > > /*\n> > > * The Agc algorithm needs to know the effective exposure value that was\n> > > * 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 715cfcd82..93a4b548c 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> > > bool disabled_;\n> > > bool supportsRaw_;\n> > > +\n> > > + ControlInfoMap::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 bd02a7a24..1cad512e1 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> > > } automatic;\n> > > \n> > > bool autoEnabled;\n> > > - uint32_t constraintMode;\n> > > - uint32_t exposureMode;\n> > > + int32_t constraintMode;\n> > > + int32_t exposureMode;\n> > > + int32_t meteringMode;\n> \n> I don't think those could take negative values. They're essentially\n> enums, which we store as int32_t internally. It would be nice if the\n> compiler could verify that we're using them correctly. We seem to have,\n> for instance, enum AeConstraintModeEnum in the control generated\n> headers. Should we use that ?\n> \n> > > + utils::Duration maxShutterSpeed;\n> > > } agc;\n> > > \n> > > struct {\n> > > @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {\n> > > uint32_t exposure;\n> > > double gain;\n> > > bool autoEnabled;\n> > > + int32_t exposureMode;\n> > > + int32_t constraintMode;\n> > > + int32_t meteringMode;\n> > > + utils::Duration maxShutterSpeed;\n> > > } agc;\n> > > \n> > > struct {\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 6687c91e7..12b3aedd2 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > \n> > All the changes to rkisp1 here should really be in a standalone commit.\n> > \n> > I'm surprised we don't already have this handled, but it's core to\n> > rkisp1 IPA - not the AGC algorithm.\n> > \n> > And ideally that patch should be in a series that also drops\n> > rkisp1Controls and moves those to their respective algo implementations!\n> > \n> > Err ... wait - I think it /is/ already being handled by the core...\n> > \n> > > @@ -80,6 +80,7 @@ private:\n> > > std::map mappedBuffers_;\n> > > \n> > > ControlInfoMap sensorControls_;\n> > > + ControlInfoMap::Map algoControls_;\n> > > \n> > > /* Interface to the Camera Helper */\n> > > std::unique_ptr camHelper_;\n> > > @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> > > if (ret)\n> > > return ret;\n> > > \n> > > + for (auto const &a : algorithms()) {\n> > > + Algorithm *algo = static_cast(a.get());\n> > > +\n> > > + /* \\todo Avoid merging duplicate controls */\n> > \n> > I think that's already the case. algoControls_ can't merge duplicates,\n> > it can either overwrite or drop the incoming ones (based on a\n> > parameter).\n> > \n> > In this case, we /shouldn't/ have duplicates - so I'd keep this as is\n> > with the default MergePolicy which I believe will shout out a warning if\n> > someone tries to merge a control that's already added.\n> > \n> > In other words, I think this todo comment can be dropped now.\n> > \n> > \n> > So in fact - I think this should all be dropped here as there is already\n> > a way to register additional controls by adding them to the IPA Context\n> > ctlrMap.\n> > \n> > See Stefan's recent \"ipa: rkisp1: Add GammaOutCorrection algorithm\" as\n> > an example.\n> > \n> > Or did I miss something different about what this is doing here?\n> > \n> > > + if (!algo->controls_.empty())\n> > > + algoControls_.merge(ControlInfoMap::Map(algo->controls_));\n> > > + }\n> > > +\n> > > /* Initialize controls. */\n> > > updateControls(sensorInfo, sensorControls, ipaControls);\n> > > \n> > > @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > > ControlInfoMap *ipaControls)\n> > > {\n> > > ControlInfoMap::Map ctrlMap = rkisp1Controls;\n> > > + ctrlMap.merge(algoControls_);\n> > > \n> > > /*\n> > > * 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 E8FCABD87C\n\tfor ;\n\tTue, 11 Jun 2024 20:25:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2148F65463;\n\tTue, 11 Jun 2024 22:25:20 +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 D945C65446\n\tfor ;\n\tTue, 11 Jun 2024 22:25:18 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 89BAF29A;\n\tTue, 11 Jun 2024 22:25:05 +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=\"jgo9r+Rn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718137505;\n\tbh=IVyaETHO+i5k1qH3v9lBpTRxwJ9PWtiP53Nfz7vvcv4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jgo9r+Rn69uTGZA5m5nuSR5j0ahOaT2IA0v3VIfiefHWttJ3f4j9Usitz57Kp/n6u\n\tkFuIhdRcPhdNYMbqDvDpnbZBsLNc9nTZqamFraTKZgzewvk1l2K6wAhPZ7TKp48s88\n\tVoItrScaAJA1v0lZR8vgM56tNjIKa6FWWTG6uIOI=","Date":"Tue, 11 Jun 2024 23:24:58 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH v5 3/3] ipa: rkisp1: agc: Plumb mode-selection and frame\n\tduration controls","Message-ID":"<20240611202458.GA8956@pendragon.ideasonboard.com>","References":"<20240607080330.2667994-1-paul.elder@ideasonboard.com>\n\t<20240607080330.2667994-4-paul.elder@ideasonboard.com>\n\t<171803012812.2248009.16116703110912470753@ping.linuxembedded.co.uk>\n\t<20240611142144.GA26595@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240611142144.GA26595@pendragon.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\" "}}]