[{"id":39881,"web_url":"https://patchwork.libcamera.org/comment/39881/","msgid":"","date":"2026-07-27T14:43:01","subject":"Re: [RFC PATCH v2 27/43] ipa: mali-c55: agc: Port to `AgcAlgorithm`","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Jul 23, 2026 at 05:43:10PM +0200, Barnabás Pőcze wrote:\n> Use the `AgcAlgorithm` class to implement the mali-c55 agc algorithm.\n>\n> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/262\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/ipa/mali-c55/algorithms/agc.cpp | 130 ++++++----------------\n> src/ipa/mali-c55/algorithms/agc.h | 4 +-\n> src/ipa/mali-c55/ipa_context.h | 35 +++---\n> src/ipa/mali-c55/mali-c55.cpp | 160 +++++-----------------------\n> 4 files changed, 75 insertions(+), 254 deletions(-)\n>\n> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> index 821f43e9fd..8af4df53e7 100644\n> --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> @@ -123,12 +123,19 @@ Agc::Agc()\n\nI think you could now drop\n\n#include \n#include \n\n>\n> int Agc::init(IPAContext &context, const ValueNode &tuningData)\n> {\n> -\tint ret = agc_.parseTuningData(tuningData);\n> +\tint ret = agc_.init(tuningData);\n> \tif (ret)\n> \t\treturn ret;\n>\n> -\tcontext.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);\n> -\tcontext.ctrlMap.merge(agc_.controls());\n> +\tret = agc_.configure(context.configuration.agc, context.activeState.agc, {\n> +\t\t.sensor = context.camHelper.get(),\n> +\t\t.sensorInfo = context.sensorInfo,\n> +\t\t.sensorControls = context.sensorControls,\n> +\t\t.ctrlMap = context.ctrlMap,\n> +\t\t.autoAllowed = true, // \\todo if not raw?\n> +\t});\n> +\tif (ret)\n> +\t\treturn ret;\n>\n> \treturn 0;\n> }\n> @@ -140,79 +147,24 @@ int Agc::configure(IPAContext &context,\n> \tif (ret)\n> \t\treturn ret;\n>\n> -\t/*\n> -\t * Defaults; we use whatever the sensor's default exposure is and the\n> -\t * minimum analogue gain. AEGC is _active_ by default.\n> -\t */\n> -\tcontext.activeState.agc.autoEnabled = true;\n> -\tcontext.activeState.agc.automatic.sensorGain = context.configuration.agc.minAnalogueGain;\n> -\tcontext.activeState.agc.automatic.exposure = context.configuration.agc.defaultExposure;\n> -\tcontext.activeState.agc.manual.sensorGain = context.configuration.agc.minAnalogueGain;\n> -\tcontext.activeState.agc.manual.exposure = context.configuration.agc.defaultExposure;\n> -\tcontext.activeState.agc.constraintMode = agc_.constraintModes().begin()->first;\n> -\tcontext.activeState.agc.exposureMode = agc_.exposureModeHelpers().begin()->first;\n> -\n> -\t/* \\todo Run this again when FrameDurationLimits is passed in */\n> -\tagc_.setLimits(context.configuration.agc.minShutterSpeed,\n> -\t\t context.configuration.agc.maxShutterSpeed,\n> -\t\t context.configuration.agc.minAnalogueGain,\n> -\t\t context.configuration.agc.maxAnalogueGain,\n> -\t\t {});\n> -\n> -\tagc_.resetFrameCount();\n> +\tret = agc_.configure(context.configuration.agc, context.activeState.agc, {\n> +\t\t.sensor = context.camHelper.get(),\n> +\t\t.sensorInfo = context.sensorInfo,\n> +\t\t.sensorControls = context.sensorControls,\n> +\t\t.ctrlMap = context.ctrlMap,\n> +\t\t.autoAllowed = true, // \\todo if not raw?\n> +\t});\n> +\tif (ret)\n> +\t\treturn ret;\n>\n> \treturn 0;\n> }\n>\n> -void Agc::queueRequest(IPAContext &context, const uint32_t frame,\n> -\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> +void Agc::queueRequest(IPAContext &context, [[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 &constraintMode = controls.get(controls::AeConstraintMode);\n> -\tagc.constraintMode = constraintMode.value_or(agc.constraintMode);\n> -\n> -\tconst auto &exposureMode = controls.get(controls::AeExposureMode);\n> -\tagc.exposureMode = exposureMode.value_or(agc.exposureMode);\n> -\n> -\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> -\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> -\t\tagc.autoEnabled = *agcEnable;\n> -\n> -\t\tLOG(MaliC55Agc, Info)\n> -\t\t\t<< (agc.autoEnabled ? \"Enabling\" : \"Disabling\")\n> -\t\t\t<< \" AGC\";\n> -\t}\n> -\n> -\t/*\n> -\t * If the automatic exposure and gain is enabled we have no further work\n> -\t * to do here...\n> -\t */\n> -\tif (agc.autoEnabled)\n> -\t\treturn;\n> -\n> -\t/*\n> -\t * ...otherwise we need to look for exposure and gain controls and use\n> -\t * those to set the activeState.\n> -\t */\n> -\tconst auto &exposure = controls.get(controls::ExposureTime);\n> -\tif (exposure) {\n> -\t\tagc.manual.exposure = *exposure * 1.0us / context.configuration.sensor.lineDuration;\n> -\n> -\t\tLOG(MaliC55Agc, Debug)\n> -\t\t\t<< \"Exposure set to \" << agc.manual.exposure\n> -\t\t\t<< \" on request sequence \" << frame;\n> -\t}\n> -\n> -\tconst auto &analogueGain = controls.get(controls::AnalogueGain);\n> -\tif (analogueGain) {\n> -\t\tagc.manual.sensorGain = *analogueGain;\n> -\n> -\t\tLOG(MaliC55Agc, Debug)\n> -\t\t\t<< \"Analogue gain set to \" << agc.manual.sensorGain\n> -\t\t\t<< \" on request sequence \" << frame;\n> -\t}\n> +\tagc_.queueRequest(context.configuration.agc, context.activeState.agc, frameContext.agc, controls);\n> }\n>\n> void Agc::fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type)\n> @@ -265,9 +217,11 @@ void Agc::fillWeightsArrayBuffer(MaliC55Params *params, const enum MaliC55Blocks\n> \tstd::fill(weights.begin(), weights.end(), 1);\n> }\n>\n> -void Agc::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,\n> -\t\t [[maybe_unused]] IPAFrameContext &frameContext, MaliC55Params *params)\n> +void Agc::prepare(IPAContext &context, const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext, MaliC55Params *params)\n> {\n> +\tagc_.prepare(context.activeState.agc, frameContext.agc);\n> +\n> \tif (frame > 0)\n> \t\treturn;\n>\n> @@ -310,9 +264,6 @@ void Agc::process(IPAContext &context,\n> \t\t const mali_c55_stats_buffer *stats,\n> \t\t [[maybe_unused]] ControlList &metadata)\n> {\n> -\tIPASessionConfiguration &configuration = context.configuration;\n> -\tIPAActiveState &activeState = context.activeState;\n> -\n> \tif (!stats) {\n> \t\tLOG(MaliC55Agc, Error) << \"No statistics buffer passed to Agc\";\n> \t\treturn;\n> @@ -323,33 +274,12 @@ void Agc::process(IPAContext &context,\n> \t\t\t\t\t\t\t statistics_.gHist.interQuantileMean(0, 1),\n> \t\t\t\t\t\t\t statistics_.bHist.interQuantileMean(0, 1) } });\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> -\t */\n> -\tuint32_t exposure = frameContext.agc.exposure;\n> -\tdouble analogueGain = frameContext.agc.sensorGain;\n> -\tutils::Duration currentShutter = exposure * configuration.sensor.lineDuration;\n> -\tutils::Duration effectiveExposureValue = currentShutter * analogueGain;\n> -\n> -\tconst auto &newEv = agc_.calculateNewEv({\n> +\tagc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{\n> \t\t.traits = AgcTraits(statistics_),\n> \t\t.yHist = statistics_.yHist,\n> -\t\t.effectiveExposureValue = effectiveExposureValue,\n> -\t\t.constraintModeIndex = activeState.agc.constraintMode,\n> -\t\t.exposureModeIndex = activeState.agc.exposureMode,\n> -\t});\n> -\n> -\tLOG(MaliC55Agc, Debug)\n> -\t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n> -\t\t<< newEv.exposureTime << \", \" << newEv.analogueGain << \" and \" << newEv.digitalGain;\n> -\n> -\tactiveState.agc.automatic.exposure = newEv.exposureTime / configuration.sensor.lineDuration;\n> -\tactiveState.agc.automatic.sensorGain = newEv.analogueGain;\n> -\n> -\tmetadata.set(controls::ExposureTime, currentShutter.get());\n> -\tmetadata.set(controls::AnalogueGain, frameContext.agc.sensorGain);\n> -\tmetadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);\n> +\t\t.exposure = frameContext.sensor.exposure,\n> +\t\t.gain = frameContext.sensor.gain,\n> +\t}}, metadata);\n\nSo nice!\n\n> }\n>\n> REGISTER_IPA_ALGORITHM(Agc, \"Agc\")\n> diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h\n> index ec7fe28c17..809a57c53a 100644\n> --- a/src/ipa/mali-c55/algorithms/agc.h\n> +++ b/src/ipa/mali-c55/algorithms/agc.h\n> @@ -11,7 +11,7 @@\n>\n> #include \"libcamera/internal/bayer_format.h\"\n>\n> -#include \"libipa/agc_mean_luminance.h\"\n> +#include \"libipa/agc.h\"\n> #include \"libipa/histogram.h\"\n>\n> #include \"algorithm.h\"\n> @@ -68,7 +68,7 @@ private:\n> \tvoid fillWeightsArrayBuffer(MaliC55Params *params, enum MaliC55Blocks type);\n>\n> \tAgcStatistics statistics_;\n> -\tAgcMeanLuminance agc_;\n> +\tAgcAlgorithm agc_;\n> };\n>\n> } /* namespace ipa::mali_c55::algorithms */\n> diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h\n> index 075d6f66ef..9dd947d7fc 100644\n> --- a/src/ipa/mali-c55/ipa_context.h\n> +++ b/src/ipa/mali-c55/ipa_context.h\n> @@ -12,6 +12,8 @@\n>\n> #include \"libcamera/internal/bayer_format.h\"\n>\n> +#include \n> +#include \n> #include \n>\n> #include \"libipa/fixedpoint.h\"\n> @@ -21,34 +23,17 @@ namespace libcamera {\n> namespace ipa::mali_c55 {\n>\n> struct IPASessionConfiguration {\n> -\tstruct {\n> -\t\tutils::Duration minShutterSpeed;\n> -\t\tutils::Duration maxShutterSpeed;\n> -\t\tuint32_t defaultExposure;\n> -\t\tdouble minAnalogueGain;\n> -\t\tdouble maxAnalogueGain;\n> +\tstruct Agc : agc::Session {\n> \t} agc;\n\nCan't this just be\n\n\tagc::Session agc;\n\n>\n> \tstruct {\n> \t\tBayerFormat::Order bayerOrder;\n> -\t\tutils::Duration lineDuration;\n> \t\tuint32_t blackLevel;\n> \t} sensor;\n> };\n>\n> struct IPAActiveState {\n> -\tstruct {\n> -\t\tstruct {\n> -\t\t\tuint32_t exposure;\n> -\t\t\tdouble sensorGain;\n> -\t\t} automatic;\n> -\t\tstruct {\n> -\t\t\tuint32_t exposure;\n> -\t\t\tdouble sensorGain;\n> -\t\t} manual;\n> -\t\tbool autoEnabled;\n> -\t\tuint32_t constraintMode;\n> -\t\tuint32_t exposureMode;\n> +\tstruct Agc : agc::ActiveState {\n> \t\tuint32_t temperatureK;\n> \t} agc;\n>\n> @@ -59,10 +44,13 @@ struct IPAActiveState {\n> };\n>\n> struct IPAFrameContext : public FrameContext {\n> +\tstruct Agc : agc::FrameContext {\n> +\t} agc;\n> +\n\nand this\n\n\tagc::FrameContext agc;\n\n> \tstruct {\n> \t\tuint32_t exposure;\n> -\t\tdouble sensorGain;\n> -\t} agc;\n> +\t\tdouble gain;\n> +\t} sensor;\n>\n> \tstruct {\n> \t\tUQ<4, 8> rGain;\n> @@ -77,10 +65,15 @@ struct IPAContext {\n> \t}\n>\n> \tIPASessionConfiguration configuration;\n> +\tIPACameraSensorInfo sensorInfo;\n\nNot a big deal, but you could apply:\nipa: mali-c55: Add sensorInfo to IPAContext\n\n(Or I could rebase on this, but if we both need the same piece, it's\nprobably easier to keep it as a standalong patch to reduce conflicts?)\n\n> \tIPAActiveState activeState;\n>\n> \tFCQueue frameContexts;\n>\n> +\tControlInfoMap sensorControls;\n> +\n> +\tstd::unique_ptr camHelper;\n> +\n> \tControlInfoMap::Map ctrlMap;\n> };\n>\n> diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp\n> index 65e5297766..c0c2c83dca 100644\n> --- a/src/ipa/mali-c55/mali-c55.cpp\n> +++ b/src/ipa/mali-c55/mali-c55.cpp\n> @@ -5,7 +5,6 @@\n> * Mali-C55 ISP image processing algorithms\n> */\n>\n> -#include \n> #include \n> #include \n> #include \n> @@ -66,21 +65,11 @@ protected:\n> \tstd::string logPrefix() const override;\n>\n> private:\n> -\tvoid updateSessionConfiguration(const IPACameraSensorInfo &info,\n> -\t\t\t\t\tconst ControlInfoMap &sensorControls,\n> -\t\t\t\t\tBayerFormat::Order bayerOrder);\n> -\tvoid updateControls(const IPACameraSensorInfo &sensorInfo,\n> -\t\t\t const ControlInfoMap &sensorControls,\n> -\t\t\t ControlInfoMap *ipaControls);\n> -\tvoid setControls();\n> +\tvoid updateControls(ControlInfoMap *ipaControls);\n> +\tvoid setControls(const IPAFrameContext &frameContext);\n>\n> \tstd::map buffers_;\n>\n> -\tControlInfoMap sensorControls_;\n> -\n> -\t/* Interface to the Camera Helper */\n> -\tstd::unique_ptr camHelper_;\n> -\n> \t/* Local parameter storage */\n> \tstruct IPAContext context_;\n> };\n> @@ -102,8 +91,8 @@ std::string IPAMaliC55::logPrefix() const\n> int IPAMaliC55::init(const IPASettings &settings, const IPAConfigInfo &ipaConfig,\n> \t\t ControlInfoMap *ipaControls)\n> {\n> -\tcamHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> -\tif (!camHelper_) {\n> +\tcontext_.camHelper = CameraSensorHelperFactoryBase::create(settings.sensorModel);\n> +\tif (!context_.camHelper) {\n> \t\tLOG(IPAMaliC55, Error)\n> \t\t\t<< \"Failed to create camera sensor helper for \"\n> \t\t\t<< settings.sensorModel;\n> @@ -129,31 +118,23 @@ int IPAMaliC55::init(const IPASettings &settings, const IPAConfigInfo &ipaConfig\n> \t\treturn -EINVAL;\n> \t}\n>\n> +\tcontext_.sensorControls = ipaConfig.sensorControls;\n> +\tcontext_.sensorInfo = ipaConfig.sensorInfo;\n> +\n> \tint ret = createAlgorithms(context_, (*data)[\"algorithms\"]);\n> \tif (ret)\n> \t\treturn ret;\n>\n> -\tupdateControls(ipaConfig.sensorInfo, ipaConfig.sensorControls, ipaControls);\n> +\tupdateControls(ipaControls);\n>\n> \treturn 0;\n> }\n>\n> -void IPAMaliC55::setControls()\n> +void IPAMaliC55::setControls(const IPAFrameContext &frameContext)\n> {\n> -\tIPAActiveState &activeState = context_.activeState;\n> -\tuint32_t exposure;\n> -\tdouble gain;\n> -\n> -\tif (activeState.agc.autoEnabled) {\n> -\t\texposure = activeState.agc.automatic.exposure;\n> -\t\tgain = activeState.agc.automatic.sensorGain;\n> -\t} else {\n> -\t\texposure = activeState.agc.manual.exposure;\n> -\t\tgain = activeState.agc.manual.sensorGain;\n> -\t}\n> +\tControlList ctrls(context_.sensorControls);\n> +\tagc::prepareControls(ctrls, context_.camHelper.get(), frameContext.agc.exposure, frameContext.agc.gain);\n>\n> -\tControlList ctrls(sensorControls_);\n> -\tagc::prepareControls(ctrls, camHelper_.get(), exposure, gain);\n> \tsetSensorControls.emit(ctrls);\n> }\n>\n> @@ -167,111 +148,19 @@ void IPAMaliC55::stop()\n> \tcontext_.frameContexts.clear();\n> }\n>\n> -void IPAMaliC55::updateSessionConfiguration(const IPACameraSensorInfo &info,\n> -\t\t\t\t\t const ControlInfoMap &sensorControls,\n> -\t\t\t\t\t BayerFormat::Order bayerOrder)\n> -{\n> -\tcontext_.configuration.sensor.bayerOrder = bayerOrder;\n> -\n> -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> -\tint32_t minExposure = v4l2Exposure.min().get();\n> -\tint32_t maxExposure = v4l2Exposure.max().get();\n> -\tint32_t defExposure = v4l2Exposure.def().get();\n> -\n> -\tconst ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> -\tint32_t minGain = v4l2Gain.min().get();\n> -\tint32_t maxGain = v4l2Gain.max().get();\n> -\n> -\t/*\n> -\t * When the AGC computes the new exposure values for a frame, it needs\n> -\t * to know the limits for shutter speed and analogue gain.\n> -\t * As it depends on the sensor, update it with the controls.\n> -\t *\n> -\t * \\todo take VBLANK into account for maximum shutter speed\n> -\t */\n> -\tcontext_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;\n> -\tcontext_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;\n> -\tcontext_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;\n> -\tcontext_.configuration.agc.defaultExposure = defExposure;\n> -\tcontext_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> -\tcontext_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> -\n> -\tif (camHelper_->blackLevel().has_value()) {\n> -\t\t/*\n> -\t\t * The black level from CameraSensorHelper is a 16-bit value.\n> -\t\t * The Mali-C55 ISP expects 20-bit settings, so we shift it to\n> -\t\t * the appropriate width\n> -\t\t */\n> -\t\tcontext_.configuration.sensor.blackLevel =\n> -\t\t\tcamHelper_->blackLevel().value() << 4;\n> -\t}\n> -}\n> -\n> -void IPAMaliC55::updateControls(const IPACameraSensorInfo &sensorInfo,\n> -\t\t\t\tconst ControlInfoMap &sensorControls,\n> -\t\t\t\tControlInfoMap *ipaControls)\n> +void IPAMaliC55::updateControls(ControlInfoMap *ipaControls)\n> {\n> \tControlInfoMap::Map ctrlMap;\n>\n> -\t/*\n> -\t * Compute the frame duration limits.\n> -\t *\n> -\t * The frame length is computed assuming a fixed line length combined\n> -\t * with the vertical frame sizes.\n> -\t */\n> -\tconst ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;\n> -\tuint32_t hblank = v4l2HBlank.def().get();\n> -\tuint32_t lineLength = sensorInfo.outputSize.width + hblank;\n> -\n> -\tconst ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;\n> -\tstd::array frameHeights{\n> -\t\tv4l2VBlank.min().get() + sensorInfo.outputSize.height,\n> -\t\tv4l2VBlank.max().get() + sensorInfo.outputSize.height,\n> -\t\tv4l2VBlank.def().get() + sensorInfo.outputSize.height,\n> -\t};\n> -\n> -\tstd::array frameDurations;\n> -\tfor (unsigned int i = 0; i < frameHeights.size(); ++i) {\n> -\t\tuint64_t frameSize = lineLength * frameHeights[i];\n> -\t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> -\t}\n> -\n> -\tctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> -\t\t\t\t\t\t\t frameDurations[1],\n> -\t\t\t\t\t\t\t Span{ { frameDurations[2], frameDurations[2] } });\n> -\n> -\t/*\n> -\t * Compute exposure time limits from the V4L2_CID_EXPOSURE control\n> -\t * limits and the line duration.\n> -\t */\n> -\tdouble lineDuration = sensorInfo.minLineLength / sensorInfo.pixelRate;\n> -\n> -\tconst ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> -\tint32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> -\tint32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> -\tint32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> -\tctrlMap[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, defExposure);\n> -\n> -\t/* Compute the analogue gain limits. */\n> -\tconst ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> -\tfloat minGain = camHelper_->gain(v4l2Gain.min().get());\n> -\tfloat maxGain = camHelper_->gain(v4l2Gain.max().get());\n> -\tfloat defGain = camHelper_->gain(v4l2Gain.def().get());\n> -\tctrlMap[&controls::AnalogueGain] = ControlInfo(minGain, maxGain, defGain);\n> -\n> -\t/*\n> -\t * Merge in any controls that we support either statically or from the\n> -\t * algorithms.\n> -\t */\n> \tctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());\n> -\n> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> }\n>\n> int IPAMaliC55::configure(const IPAConfigInfo &ipaConfig, uint8_t bayerOrder,\n> \t\t\t ControlInfoMap *ipaControls)\n> {\n> -\tsensorControls_ = ipaConfig.sensorControls;\n> +\tcontext_.sensorControls = ipaConfig.sensorControls;\n> +\tcontext_.sensorInfo = ipaConfig.sensorInfo;\n>\n> \t/* Clear the IPA context before the streaming session. */\n> \tcontext_.configuration = {};\n> @@ -280,9 +169,16 @@ int IPAMaliC55::configure(const IPAConfigInfo &ipaConfig, uint8_t bayerOrder,\n>\n> \tconst IPACameraSensorInfo &info = ipaConfig.sensorInfo;\n>\n> -\tupdateSessionConfiguration(info, ipaConfig.sensorControls,\n> -\t\t\t\t static_cast(bayerOrder));\n> -\tupdateControls(info, ipaConfig.sensorControls, ipaControls);\n> +\tcontext_.configuration.sensor.bayerOrder = static_cast(bayerOrder);\n> +\n> +\tif (auto bl = context_.camHelper->blackLevel()) {\n> +\t\t/*\n> +\t\t * The black level from CameraSensorHelper is a 16-bit value.\n> +\t\t * The Mali-C55 ISP expects 20-bit settings, so we shift it to\n> +\t\t * the appropriate width\n> +\t\t */\n> +\t\tcontext_.configuration.sensor.blackLevel = *bl << 4;\n> +\t}\n>\n> \tfor (const auto &a : algorithms()) {\n> \t\tAlgorithm *algo = static_cast(a.get());\n> @@ -292,6 +188,8 @@ int IPAMaliC55::configure(const IPAConfigInfo &ipaConfig, uint8_t bayerOrder,\n> \t\t\treturn ret;\n> \t}\n>\n> +\tupdateControls(ipaControls);\n> +\n> \treturn 0;\n> }\n>\n> @@ -351,8 +249,8 @@ void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,\n> \tstats = reinterpret_cast(\n> \t\tbuffers_.at(bufferId).planes()[0].data());\n>\n> -\tstd::tie(frameContext.agc.exposure, frameContext.agc.sensorGain)\n> -\t\t= agc::extractControls(sensorControls, camHelper_.get());\n> +\tstd::tie(frameContext.sensor.exposure, frameContext.sensor.gain)\n> +\t\t= agc::extractControls(sensorControls, context_.camHelper.get());\n>\n> \tControlList metadata(controls::controls);\n>\n> @@ -362,7 +260,7 @@ void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,\n> \t\talgo->process(context_, request, frameContext, stats, metadata);\n> \t}\n>\n> -\tsetControls();\n> +\tsetControls(frameContext);\n>\n> \tstatsProcessed.emit(request, metadata);\n\nSo nice saving so much code!\n\nReviewed-by: Jacopo Mondi \n\n> }\n> --\n> 2.55.0\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 90F7FBDE4C\n\tfor ;\n\tMon, 27 Jul 2026 14:43:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7AA3967F7B;\n\tMon, 27 Jul 2026 16:43: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 751FA67EB2\n\tfor ;\n\tMon, 27 Jul 2026 16:43:04 +0200 (CEST)","from ideasonboard.com (mob-5-90-50-102.net.vodafone.it\n\t[5.90.50.102])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6B8253A4;\n\tMon, 27 Jul 2026 16:42:00 +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=\"HRNvGN2F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785163320;\n\tbh=kqX83025AlZ87d55542cfZ7zR310RvqIUcLOYYjxnMA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HRNvGN2FzjvskSD4dTpBLSXRA8rwWZ8HYL4t1G8UxRXHka+2p3TAw88I5vYkI+fS9\n\to5bbntC/hZl1iiQElkJRJ3yqRSso+gjmKYq3GAp1ZV9pX79miickg1LdL3EhI2T9fF\n\tcu6t2DOa73PMKYMOCsvPWzxkFcKd1ILEaJHqtEmE=","Date":"Mon, 27 Jul 2026 16:43:01 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 27/43] ipa: mali-c55: agc: Port to `AgcAlgorithm`","Message-ID":"","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-28-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20260723154327.1357866-28-barnabas.pocze@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":39892,"web_url":"https://patchwork.libcamera.org/comment/39892/","msgid":"","date":"2026-07-28T10:40:38","subject":"Re: [RFC PATCH v2 27/43] ipa: mali-c55: agc: Port to `AgcAlgorithm`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2026. 07. 27. 16:43 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Thu, Jul 23, 2026 at 05:43:10PM +0200, Barnabás Pőcze wrote:\n>> Use the `AgcAlgorithm` class to implement the mali-c55 agc algorithm.\n>>\n>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/262\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/ipa/mali-c55/algorithms/agc.cpp | 130 ++++++----------------\n>> src/ipa/mali-c55/algorithms/agc.h | 4 +-\n>> src/ipa/mali-c55/ipa_context.h | 35 +++---\n>> src/ipa/mali-c55/mali-c55.cpp | 160 +++++-----------------------\n>> 4 files changed, 75 insertions(+), 254 deletions(-)\n>>\n>> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n>> index 821f43e9fd..8af4df53e7 100644\n>> --- a/src/ipa/mali-c55/algorithms/agc.cpp\n>> +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n>> @@ -123,12 +123,19 @@ Agc::Agc()\n> \n> I think you could now drop\n> \n> #include \n> #include \n\nAhh, not entirely. I have accidentally removed the `ColourTemperature`\nmetadata setting, but properties are indeed not needed.\n\n\n> \n>>\n>> int Agc::init(IPAContext &context, const ValueNode &tuningData)\n>> {\n>> -\tint ret = agc_.parseTuningData(tuningData);\n>> +\tint ret = agc_.init(tuningData);\n>> \tif (ret)\n>> \t\treturn ret;\n>>\n>> -\tcontext.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);\n>> -\tcontext.ctrlMap.merge(agc_.controls());\n>> +\tret = agc_.configure(context.configuration.agc, context.activeState.agc, {\n>> +\t\t.sensor = context.camHelper.get(),\n>> +\t\t.sensorInfo = context.sensorInfo,\n>> +\t\t.sensorControls = context.sensorControls,\n>> +\t\t.ctrlMap = context.ctrlMap,\n>> +\t\t.autoAllowed = true, // \\todo if not raw?\n>> +\t});\n>> +\tif (ret)\n>> +\t\treturn ret;\n>>\n>> \treturn 0;\n>> }\n> [...]\n>> diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h\n>> index 075d6f66ef..9dd947d7fc 100644\n>> --- a/src/ipa/mali-c55/ipa_context.h\n>> +++ b/src/ipa/mali-c55/ipa_context.h\n>> @@ -12,6 +12,8 @@\n>>\n>> #include \"libcamera/internal/bayer_format.h\"\n>>\n>> +#include \n>> +#include \n>> #include \n>>\n>> #include \"libipa/fixedpoint.h\"\n>> @@ -21,34 +23,17 @@ namespace libcamera {\n>> namespace ipa::mali_c55 {\n>>\n>> struct IPASessionConfiguration {\n>> -\tstruct {\n>> -\t\tutils::Duration minShutterSpeed;\n>> -\t\tutils::Duration maxShutterSpeed;\n>> -\t\tuint32_t defaultExposure;\n>> -\t\tdouble minAnalogueGain;\n>> -\t\tdouble maxAnalogueGain;\n>> +\tstruct Agc : agc::Session {\n>> \t} agc;\n> \n> Can't this just be\n> \n> \tagc::Session agc;\n> \n>>\n>> \tstruct {\n>> \t\tBayerFormat::Order bayerOrder;\n>> -\t\tutils::Duration lineDuration;\n>> \t\tuint32_t blackLevel;\n>> \t} sensor;\n>> };\n>>\n>> struct IPAActiveState {\n>> -\tstruct {\n>> -\t\tstruct {\n>> -\t\t\tuint32_t exposure;\n>> -\t\t\tdouble sensorGain;\n>> -\t\t} automatic;\n>> -\t\tstruct {\n>> -\t\t\tuint32_t exposure;\n>> -\t\t\tdouble sensorGain;\n>> -\t\t} manual;\n>> -\t\tbool autoEnabled;\n>> -\t\tuint32_t constraintMode;\n>> -\t\tuint32_t exposureMode;\n>> +\tstruct Agc : agc::ActiveState {\n>> \t\tuint32_t temperatureK;\n>> \t} agc;\n>>\n>> @@ -59,10 +44,13 @@ struct IPAActiveState {\n>> };\n>>\n>> struct IPAFrameContext : public FrameContext {\n>> +\tstruct Agc : agc::FrameContext {\n>> +\t} agc;\n>> +\n> \n> and this\n> \n> \tagc::FrameContext agc;\n> \n\nYes, I will change them.\n\n\n>> \tstruct {\n>> \t\tuint32_t exposure;\n>> -\t\tdouble sensorGain;\n>> -\t} agc;\n>> +\t\tdouble gain;\n>> +\t} sensor;\n>>\n>> \tstruct {\n>> \t\tUQ<4, 8> rGain;\n>> @@ -77,10 +65,15 @@ struct IPAContext {\n>> \t}\n>>\n>> \tIPASessionConfiguration configuration;\n>> +\tIPACameraSensorInfo sensorInfo;\n> \n> Not a big deal, but you could apply:\n> ipa: mali-c55: Add sensorInfo to IPAContext\n> \n> (Or I could rebase on this, but if we both need the same piece, it's\n> probably easier to keep it as a standalong patch to reduce conflicts?)\n\nI think I will wait for it to be merged, then it's a trivial rebase.\n\n\n> \n>> \tIPAActiveState activeState;\n>>\n>> \tFCQueue frameContexts;\n>>\n>> +\tControlInfoMap sensorControls;\n>> +\n>> +\tstd::unique_ptr camHelper;\n>> +\n>> \tControlInfoMap::Map ctrlMap;\n>> };\n>>\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 D9350BDE4C\n\tfor ;\n\tTue, 28 Jul 2026 10:40:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0121E67E44;\n\tTue, 28 Jul 2026 12:40:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBB3167E44\n\tfor ;\n\tTue, 28 Jul 2026 12:40:41 +0200 (CEST)","from [192.168.33.47] (185.182.215.156.nat.pool.zt.hu\n\t[185.182.215.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3AED622E;\n\tTue, 28 Jul 2026 12:39:37 +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=\"TUQk5IIk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1785235177;\n\tbh=+J/5WxSVacsHW2iDSMenlpdw3U8DW06WYIztOjQU+v0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=TUQk5IIkHl7N/1ykQ8DlL+DUvZNJwABSnwE3+bSj5SRAWxW/RhI4nQAu906hTAv4g\n\tWneKZMEPHlvc62T0YBhzwZygqLbL14unnCSEcZmz6xZT9IVABDE+FLWeRywCgcwWBG\n\tOBk459sM7yD1TUxfZo7wPNBVxo1KDgkFPbIoFZFc=","Message-ID":"","Date":"Tue, 28 Jul 2026 12:40:38 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2 27/43] ipa: mali-c55: agc: Port to `AgcAlgorithm`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20260723154327.1357866-1-barnabas.pocze@ideasonboard.com>\n\t<20260723154327.1357866-28-barnabas.pocze@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]