[{"id":37390,"web_url":"https://patchwork.libcamera.org/comment/37390/","msgid":"","date":"2025-12-15T16:21:43","subject":"Re: [RFC PATCH v1] ipa: rkisp1: Allow algorithms to update\n\t`ControlInfoMap`","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 Mon, Dec 15, 2025 at 12:53:48PM +0100, Barnabás Pőcze wrote:\n> In `IPARkISP1::init()`, the `ControlInfoMap` that will be returned\n> to the pipeline handler is updated after creating the algorithms;\n> naturally, since otherwise no algorithm could register its own controls.\n>\n> However, in `IPARkISP1::configure()`, the update is done before the\n> algorithms are configured. This is not ideal because this prevents\n> algorithms from correctly updating e.g. the `ControlInfo`s of their\n> controls.\n>\n> While no algorithm is affected today, during development, it is useful\n> for various debug related controls, whose ranges depend on the exact\n> configuration.\n>\n> However, `updateControls()` cannot simply be moved to be after the loop\n> because it updates the limits of `FrameDurationLimits`, which `Agc::configure()`\n> needs to access. Moving it after the loop would lead to stale data\n> being used.\n\nWhile I think the patch has merits of its own, this last issue is a\nconsequence of the fact we are abusing the context ctrlMap to\ncommunicate the control limits from IPA to the algorithms.\n\nWhat do you think of\nhttps://patchwork.libcamera.org/patch/25055/ ?\n\n>\n> So move the updating of `ipaControls` out of `updateControls()`.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/ipa/rkisp1/rkisp1.cpp | 29 +++++++++++++++++------------\n> 1 file changed, 17 insertions(+), 12 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 61d3d1f6f..6c9ff306d 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -76,7 +76,7 @@ protected:\n> private:\n> \tvoid updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\t\t const ControlInfoMap &sensorControls,\n> -\t\t\t ControlInfoMap *ipaControls);\n> +\t\t\t ControlInfoMap::Map &ctrlMap);\n> \tvoid setControls(unsigned int frame);\n>\n> \tstd::map buffers_;\n> @@ -202,12 +202,17 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n> \t\treturn -EINVAL;\n> \t}\n>\n> +\tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> +\n> +\t/* Initialize controls. */\n> +\tupdateControls(sensorInfo, sensorControls, ctrlMap);\n> +\n> \tint ret = createAlgorithms(context_, (*data)[\"algorithms\"]);\n> \tif (ret)\n> \t\treturn ret;\n>\n> -\t/* Initialize controls. */\n> -\tupdateControls(sensorInfo, sensorControls, ipaControls);\n> +\tctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());\n> +\t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>\n> \treturn 0;\n> }\n> @@ -254,9 +259,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> \tcontext_.configuration.sensor.size = info.outputSize;\n> \tcontext_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;\n>\n> -\t/* Update the camera controls using the new sensor settings. */\n> -\tupdateControls(info, sensorControls_, ipaControls);\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 exposure time and analogue gain. As it depends\n> @@ -280,6 +282,11 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> \t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> \t\t});\n>\n> +\tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> +\n> +\t/* Update the camera controls using the new sensor settings. */\n> +\tupdateControls(info, sensorControls_, ctrlMap);\n> +\n\nI wonder if we shouldn't actually register all controls in\nalgorithms..\n\n> \tfor (auto const &a : algorithms()) {\n> \t\tAlgorithm *algo = static_cast(a.get());\n>\n> @@ -293,6 +300,9 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n> \t\t\treturn ret;\n> \t}\n>\n> +\tctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());\n> +\t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> +\n> \treturn 0;\n> }\n>\n> @@ -388,10 +398,8 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n>\n> void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\t\t const ControlInfoMap &sensorControls,\n> -\t\t\t ControlInfoMap *ipaControls)\n> +\t\t\t ControlInfoMap::Map &ctrlMap)\n\nnit: why use a reference if the argument is modifiable ?\n\nI think the patch is however good, and as far as I can see doesn't\nimpact the AGC frame duration limits rework I have in progress:\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> {\n> -\tControlInfoMap::Map ctrlMap = rkisp1Controls;\n> -\n> \t/*\n> \t * Compute exposure time limits from the V4L2_CID_EXPOSURE control\n> \t * limits and the line duration.\n> @@ -441,9 +449,6 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \tcontext_.ctrlMap[&controls::FrameDurationLimits] =\n> \t\tControlInfo(frameDurations[0], frameDurations[1],\n> \t\t\t ControlValue(Span{ { frameDurations[2], frameDurations[2] } }));\n> -\n> -\tctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());\n> -\t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> }\n>\n> void IPARkISP1::setControls(unsigned int frame)\n> --\n> 2.52.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 A6A5FBD7D8\n\tfor ;\n\tMon, 15 Dec 2025 16:21:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6311D619FC;\n\tMon, 15 Dec 2025 17:21:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7E11A619D8\n\tfor ;\n\tMon, 15 Dec 2025 17:21:48 +0100 (CET)","from ideasonboard.com (mob-5-90-49-39.net.vodafone.it [5.90.49.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ABB19465;\n\tMon, 15 Dec 2025 17:21:42 +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=\"pQOzXueJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765815703;\n\tbh=5/qYEZj8XcZ3lZPo47aRnqKh+zQr+pH0Xs1iMccXBeg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pQOzXueJdjv/DiL+pQydj4XIUo+JwGsVuLi3Wo/si+GzCTq6oeJxZtr0QxJsePrut\n\toKljFvAjxiNZZtVc0HmKwqBhlUWYToEtiHMcX0H3Q/MrFOXuggiilzuWDyGma9yXLK\n\t7aRfigcKE7e9A1EljvOeoS0T3CexqZA8NT+e6FP4=","Date":"Mon, 15 Dec 2025 17:21:43 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, \n\tStefan Klug ","Subject":"Re: [RFC PATCH v1] ipa: rkisp1: Allow algorithms to update\n\t`ControlInfoMap`","Message-ID":"","References":"<20251215115348.626656-1-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":"<20251215115348.626656-1-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":37419,"web_url":"https://patchwork.libcamera.org/comment/37419/","msgid":"","date":"2025-12-17T08:40:27","subject":"Re: [RFC PATCH v1] ipa: rkisp1: Allow algorithms to update\n\t`ControlInfoMap`","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. 12. 15. 17:21 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Mon, Dec 15, 2025 at 12:53:48PM +0100, Barnabás Pőcze wrote:\n>> In `IPARkISP1::init()`, the `ControlInfoMap` that will be returned\n>> to the pipeline handler is updated after creating the algorithms;\n>> naturally, since otherwise no algorithm could register its own controls.\n>>\n>> However, in `IPARkISP1::configure()`, the update is done before the\n>> algorithms are configured. This is not ideal because this prevents\n>> algorithms from correctly updating e.g. the `ControlInfo`s of their\n>> controls.\n>>\n>> While no algorithm is affected today, during development, it is useful\n>> for various debug related controls, whose ranges depend on the exact\n>> configuration.\n>>\n>> However, `updateControls()` cannot simply be moved to be after the loop\n>> because it updates the limits of `FrameDurationLimits`, which `Agc::configure()`\n>> needs to access. Moving it after the loop would lead to stale data\n>> being used.\n> \n> While I think the patch has merits of its own, this last issue is a\n> consequence of the fact we are abusing the context ctrlMap to\n> communicate the control limits from IPA to the algorithms.\n> \n> What do you think of\n> https://patchwork.libcamera.org/patch/25055/ ?\n\nI believe it would indeed eliminate the problem outlined in the commit message.\n\n\n> \n>>\n>> So move the updating of `ipaControls` out of `updateControls()`.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/ipa/rkisp1/rkisp1.cpp | 29 +++++++++++++++++------------\n>> 1 file changed, 17 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 61d3d1f6f..6c9ff306d 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -76,7 +76,7 @@ protected:\n>> private:\n>> \tvoid updateControls(const IPACameraSensorInfo &sensorInfo,\n>> \t\t\t const ControlInfoMap &sensorControls,\n>> -\t\t\t ControlInfoMap *ipaControls);\n>> +\t\t\t ControlInfoMap::Map &ctrlMap);\n>> \tvoid setControls(unsigned int frame);\n>>\n>> \tstd::map buffers_;\n>> @@ -202,12 +202,17 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,\n>> \t\treturn -EINVAL;\n>> \t}\n>>\n>> +\tControlInfoMap::Map ctrlMap = rkisp1Controls;\n>> +\n>> +\t/* Initialize controls. */\n>> +\tupdateControls(sensorInfo, sensorControls, ctrlMap);\n>> +\n>> \tint ret = createAlgorithms(context_, (*data)[\"algorithms\"]);\n>> \tif (ret)\n>> \t\treturn ret;\n>>\n>> -\t/* Initialize controls. */\n>> -\tupdateControls(sensorInfo, sensorControls, ipaControls);\n>> +\tctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());\n>> +\t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>>\n>> \treturn 0;\n>> }\n>> @@ -254,9 +259,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n>> \tcontext_.configuration.sensor.size = info.outputSize;\n>> \tcontext_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;\n>>\n>> -\t/* Update the camera controls using the new sensor settings. */\n>> -\tupdateControls(info, sensorControls_, ipaControls);\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 exposure time and analogue gain. As it depends\n>> @@ -280,6 +282,11 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n>> \t\t\treturn format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n>> \t\t});\n>>\n>> +\tControlInfoMap::Map ctrlMap = rkisp1Controls;\n>> +\n>> +\t/* Update the camera controls using the new sensor settings. */\n>> +\tupdateControls(info, sensorControls_, ctrlMap);\n>> +\n> \n> I wonder if we shouldn't actually register all controls in\n> algorithms..\n\nI think that would make sense. At least where a control is fully \"managed\" by a single algorithm.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>> \tfor (auto const &a : algorithms()) {\n>> \t\tAlgorithm *algo = static_cast(a.get());\n>>\n>> @@ -293,6 +300,9 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,\n>> \t\t\treturn ret;\n>> \t}\n>>\n>> +\tctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());\n>> +\t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>> +\n>> \treturn 0;\n>> }\n>>\n>> @@ -388,10 +398,8 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n>>\n>> void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n>> \t\t\t const ControlInfoMap &sensorControls,\n>> -\t\t\t ControlInfoMap *ipaControls)\n>> +\t\t\t ControlInfoMap::Map &ctrlMap)\n> \n> nit: why use a reference if the argument is modifiable ?\n> \n> I think the patch is however good, and as far as I can see doesn't\n> impact the AGC frame duration limits rework I have in progress:\n> \n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n>> {\n>> -\tControlInfoMap::Map ctrlMap = rkisp1Controls;\n>> -\n>> \t/*\n>> \t * Compute exposure time limits from the V4L2_CID_EXPOSURE control\n>> \t * limits and the line duration.\n>> @@ -441,9 +449,6 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n>> \tcontext_.ctrlMap[&controls::FrameDurationLimits] =\n>> \t\tControlInfo(frameDurations[0], frameDurations[1],\n>> \t\t\t ControlValue(Span{ { frameDurations[2], frameDurations[2] } }));\n>> -\n>> -\tctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());\n>> -\t*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>> }\n>>\n>> void IPARkISP1::setControls(unsigned int frame)\n>> --\n>> 2.52.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 04C68BD7D8\n\tfor ;\n\tWed, 17 Dec 2025 08:40:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A87E561A2E;\n\tWed, 17 Dec 2025 09:40:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D228609DE\n\tfor ;\n\tWed, 17 Dec 2025 09:40:30 +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 2DB9A324;\n\tWed, 17 Dec 2025 09:40:24 +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=\"T4rQ6qN+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765960824;\n\tbh=+zZNx4vmmuF9Ug01uHhJxv9aKiRqokQ9hm+Hje6s8Uc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=T4rQ6qN+L5UgkLtontGPCHNFDk2vyib1bFdZdLYSkwUzpgQNy7j0QSHAKabKhwI49\n\tc1pOshT/B63bvupDB/hAMUB3jnrEkgS3JmbAROd//mmlPNzL7Raw96z1jGvhzP6Czv\n\tVeSPiLMIRCxyDhlb4b5ppTib+nf0mD4sHA9qqCKM=","Message-ID":"","Date":"Wed, 17 Dec 2025 09:40:27 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1] ipa: rkisp1: Allow algorithms to update\n\t`ControlInfoMap`","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ","References":"<20251215115348.626656-1-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\" "}}]