[{"id":37418,"web_url":"https://patchwork.libcamera.org/comment/37418/","msgid":"","date":"2025-12-17T08:39:16","subject":"Re: [PATCH v3 03/19] ipa: rkisp1: Store FrameDurationLimits as\n\tsensor config","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 11. 14. 15:16 keltezéssel, Jacopo Mondi írta:\n> The RkISP1 IPA context stores a control list ctrlMap which is used by the\n> AGC algorithm to initialize controls that are then added to the list of the\n> ones registered by the IPA module.\n> \n> The FrameDurationLimits control is a bit of an outlier, as it's the only\n> control that is registered by the IPA in the context ctrlMap and is then\n> used by the AGC algorithm.\n> \n> As the purpose of the context ctrlMap is to be populated by algorithms for\n> the IPA, do not abuse it by registering a control there in the main IPA\n> module and only store the control limits in the active context.\n> \n> Removing FrameDurationLimits from the ctrlMap requires to store it as\n> a sensor configuration parameter in the IPA context and use it for\n> clamping the FrameDurationLimits control and to initialize the AGC\n> active state.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 28 ++++++++++++------------\n> src/ipa/rkisp1/ipa_context.h | 2 ++\n> src/ipa/rkisp1/rkisp1.cpp | 46 ++++++++++++++++++++++++---------------\n> 3 files changed, 44 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..aa1a90daf3ca7d0041c56000c12fc4d1ab5700eb 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -190,10 +190,10 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> [...]\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index f85a130d9c23dba7987f388e395239e4b141d776..5fe727bd0b508617d993d226ae785056a3771ce0 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -62,6 +62,8 @@ struct IPASessionConfiguration {\n> [...]\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index fa22bfc349043002345d275b11a60ac983e329d7..f25e477f0fb77241bd1ccddb7778205e58bdc8a9 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -227,6 +227,7 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> \t\t\t const std::map &streamConfig,\n> \t\t\t ControlInfoMap *ipaControls)\n> {\n> +\tconst IPACameraSensorInfo &info = ipaConfig.sensorInfo;\n> \tsensorControls_ = ipaConfig.sensorControls;\n> \n> \tconst auto itExp = sensorControls_.find(V4L2_CID_EXPOSURE);\n> @@ -237,6 +238,12 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> \tint32_t minGain = itGain->second.min().get();\n> \tint32_t maxGain = itGain->second.max().get();\n> \n> +\tconst auto itVBlank = sensorControls_.find(V4L2_CID_VBLANK);\n> +\tstd::array frameHeights{\n> +\t\titVBlank->second.min().get() + info.outputSize.height,\n> +\t\titVBlank->second.max().get() + info.outputSize.height,\n> +\t};\n> +\n> \tLOG(IPARkISP1, Debug)\n> \t\t<< \"Exposure: [\" << minExposure << \", \" << maxExposure\n> \t\t<< \"], gain: [\" << minGain << \", \" << maxGain << \"]\";\n> @@ -248,11 +255,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> \n> \tcontext_.configuration.paramFormat = ipaConfig.paramFormat;\n> \n> -\tconst IPACameraSensorInfo &info = ipaConfig.sensorInfo;\n> -\tconst ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second;\n> -\tcontext_.configuration.sensor.defVBlank = vBlank.def().get();\n> +\tutils::Duration lineDuration = info.minLineLength * 1.0s / info.pixelRate;\n> +\tcontext_.configuration.sensor.defVBlank = itVBlank->second.def().get();\n> \tcontext_.configuration.sensor.size = info.outputSize;\n> -\tcontext_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;\n> +\tcontext_.configuration.sensor.lineDuration = lineDuration;\n> \n> \t/* Update the camera controls using the new sensor settings. */\n> \tupdateControls(info, sensorControls_, ipaControls);\n> @@ -261,17 +267,13 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> \t * When the AGC computes the new exposure values for a frame, it needs\n> \t * to know the limits for exposure time and analogue gain. As it depends\n> \t * on the sensor, update it with the controls.\n> -\t *\n> -\t * \\todo take VBLANK into account for maximum exposure time\n\nI believe it would be nice to mention this change in the commit message.\n\n\n> \t */\n> -\tcontext_.configuration.sensor.minExposureTime =\n> -\t\tminExposure * context_.configuration.sensor.lineDuration;\n> -\tcontext_.configuration.sensor.maxExposureTime =\n> -\t\tmaxExposure * context_.configuration.sensor.lineDuration;\n> -\tcontext_.configuration.sensor.minAnalogueGain =\n> -\t\tcontext_.camHelper->gain(minGain);\n> -\tcontext_.configuration.sensor.maxAnalogueGain =\n> -\t\tcontext_.camHelper->gain(maxGain);\n> +\tcontext_.configuration.sensor.minExposureTime = minExposure * lineDuration;\n> +\tcontext_.configuration.sensor.maxExposureTime = maxExposure * lineDuration;\n> +\tcontext_.configuration.sensor.minFrameDuration = frameHeights[0] * lineDuration;\n> +\tcontext_.configuration.sensor.maxFrameDuration = frameHeights[1] * lineDuration;\n> +\tcontext_.configuration.sensor.minAnalogueGain = context_.camHelper->gain(minGain);\n> +\tcontext_.configuration.sensor.maxAnalogueGain = context_.camHelper->gain(maxGain);\n> \n> \tcontext_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(),\n> \t\t[](auto &cfg) -> bool {\n> @@ -436,12 +438,20 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\tuint64_t frameSize = lineLength * frameHeights[i];\n> \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> \t}\n> -\n> -\t/* \\todo Move this (and other agc-related controls) to agc */\n> -\tcontext_.ctrlMap[&controls::FrameDurationLimits] =\n> +\tctrlMap[&controls::FrameDurationLimits] =\n> \t\tControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]);\n\nShouldn't this be moved into `Agc::init()` or such for the above \"todo\" to be considered done?\n\n\n> \n> -\tctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());\n> +\t/*\n> +\t * Store the min/max frame duration in the active context to initialize\n> +\t * the AGC algorithm.\n> +\t *\n> +\t * \\todo Move this (and other agc-related controls) to agc\n> +\t */\n> +\tcontext_.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurations[0]);\n> +\tcontext_.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurations[1]);\n> +\n> +\tctrlMap.merge(context_.ctrlMap);\n\nThis will remove items fron `context_.ctrlMap`. Is that the expectation?\n\n\n> +\n> \t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\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 E872EBD7D8\n\tfor ;\n\tWed, 17 Dec 2025 08:39:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0ECC1609DE;\n\tWed, 17 Dec 2025 09:39:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4ED4B609DE\n\tfor ;\n\tWed, 17 Dec 2025 09:39:21 +0100 (CET)","from [192.168.33.22] (185.221.143.114.nat.pool.zt.hu\n\t[185.221.143.114])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A555E324;\n\tWed, 17 Dec 2025 09:39:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mudanpfQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765960754;\n\tbh=CWemN8ZVDE5COziCedVyOSdJPaFKTFNIj6qhI9yz6bE=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=mudanpfQAWEFiJN6Ix/YZbyJyfy/L+vGZCYhTYycXyRY3H0Va1D1kI8xGCRSpNSjy\n\t7pENfR7SzO/IaagkBhmR9Pa6Jc9hKWuA9KC40lqDPrnAAUOF3L3XDqigxB73Noqwjq\n\tknmeZwFrAERGdWn4hiyVqoxG7uvr3ghInuwTL3Lc=","Message-ID":"","Date":"Wed, 17 Dec 2025 09:39:16 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 03/19] ipa: rkisp1: Store FrameDurationLimits as\n\tsensor config","To":"Jacopo Mondi , =?utf-8?q?Niklas_S=C3=B6?=\n\t=?utf-8?q?derlund?= , Robert Mader\n\t, libcamera-devel@lists.libcamera.org","References":"<20251114-exposure-limits-v3-0-b7c07feba026@ideasonboard.com>\n\t<20251114-exposure-limits-v3-3-b7c07feba026@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251114-exposure-limits-v3-3-b7c07feba026@ideasonboard.com>","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\" "}}]