[{"id":34925,"web_url":"https://patchwork.libcamera.org/comment/34925/","msgid":"","date":"2025-07-18T08:55:29","subject":"Re: [PATCH] ipa: rpi: Fix static initialisation order bug in the\n\tController","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. 07. 18. 10:23 keltezéssel, Naushir Patuck írta:\n> There is a possible static initialisation issue with accessing the\n> HardwareConfigMap static object through Controller::getHardwareConfig().\n> Fix this by providing a static function &hardwareConfigMap() to access\n> the object.\n> \n> Though not proven, this is possibly the cause of a very infrequent\n> lockup in https://github.com/raspberrypi/rpicam-apps/issues/799.\n\nI really believe non-`constinit` (C++20) globals should removed altogether...\nA simple array with a for loop would be sufficient in many (most?) cases. And\nwhat cannot be made `constinit` should be moved into a getter function like this.\n\n\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/rpi/controller/controller.cpp | 119 ++++++++++++++------------\n> 1 file changed, 64 insertions(+), 55 deletions(-)\n> \n> diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp\n> index 651fff632400..72c0592c9585 100644\n> --- a/src/ipa/rpi/controller/controller.cpp\n> +++ b/src/ipa/rpi/controller/controller.cpp\n> @@ -21,61 +21,70 @@ using namespace std::literals::chrono_literals;\n> \n> LOG_DEFINE_CATEGORY(RPiController)\n> \n> -static const std::map HardwareConfigMap = {\n> -\t{\n> -\t\t\"bcm2835\",\n> +namespace {\n> +\n> +const std::map &hardwareConfigMap() {\n\nconst std::map &hardwareConfigMap()\n{\n\n\nRegards,\nBarnabás Pőcze\n\n> +\n> +\tstatic const std::map map = {\n> \t\t{\n> -\t\t\t/*\n> -\t\t\t * There are only ever 15 AGC regions computed by the firmware\n> -\t\t\t * due to zoning, but the HW defines AGC_REGIONS == 16!\n> -\t\t\t */\n> -\t\t\t.agcRegions = { 15 , 1 },\n> -\t\t\t.agcZoneWeights = { 15 , 1 },\n> -\t\t\t.awbRegions = { 16, 12 },\n> -\t\t\t.cacRegions = { 0, 0 },\n> -\t\t\t.focusRegions = { 4, 3 },\n> -\t\t\t.numHistogramBins = 128,\n> -\t\t\t.numGammaPoints = 33,\n> -\t\t\t.pipelineWidth = 13,\n> -\t\t\t.statsInline = false,\n> -\t\t\t.minPixelProcessingTime = 0s,\n> -\t\t\t.dataBufferStrided = true,\n> -\t\t}\n> -\t},\n> -\t{\n> -\t\t\"pisp\",\n> +\t\t\t\"bcm2835\",\n> +\t\t\t{\n> +\t\t\t\t/*\n> +\t\t\t\t* There are only ever 15 AGC regions computed by the firmware\n> +\t\t\t\t* due to zoning, but the HW defines AGC_REGIONS == 16!\n> +\t\t\t\t*/\n> +\t\t\t\t.agcRegions = { 15 , 1 },\n> +\t\t\t\t.agcZoneWeights = { 15 , 1 },\n> +\t\t\t\t.awbRegions = { 16, 12 },\n> +\t\t\t\t.cacRegions = { 0, 0 },\n> +\t\t\t\t.focusRegions = { 4, 3 },\n> +\t\t\t\t.numHistogramBins = 128,\n> +\t\t\t\t.numGammaPoints = 33,\n> +\t\t\t\t.pipelineWidth = 13,\n> +\t\t\t\t.statsInline = false,\n> +\t\t\t\t.minPixelProcessingTime = 0s,\n> +\t\t\t\t.dataBufferStrided = true,\n> +\t\t\t}\n> +\t\t},\n> \t\t{\n> -\t\t\t.agcRegions = { 0, 0 },\n> -\t\t\t.agcZoneWeights = { 15, 15 },\n> -\t\t\t.awbRegions = { 32, 32 },\n> -\t\t\t.cacRegions = { 8, 8 },\n> -\t\t\t.focusRegions = { 8, 8 },\n> -\t\t\t.numHistogramBins = 1024,\n> -\t\t\t.numGammaPoints = 64,\n> -\t\t\t.pipelineWidth = 16,\n> -\t\t\t.statsInline = true,\n> -\n> -\t\t\t/*\n> -\t\t\t * The constraint below is on the rate of pixels going\n> -\t\t\t * from CSI2 peripheral to ISP-FE (400Mpix/s, plus tiny\n> -\t\t\t * overheads per scanline, for which 380Mpix/s is a\n> -\t\t\t * conservative bound).\n> -\t\t\t *\n> -\t\t\t * There is a 64kbit data FIFO before the bottleneck,\n> -\t\t\t * which means that in all reasonable cases the\n> -\t\t\t * constraint applies at a timescale >= 1 scanline, so\n> -\t\t\t * adding horizontal blanking can prevent loss.\n> -\t\t\t *\n> -\t\t\t * If the backlog were to grow beyond 64kbit during a\n> -\t\t\t * single scanline, there could still be loss. This\n> -\t\t\t * could happen using 4 lanes at 1.5Gbps at 10bpp with\n> -\t\t\t * frames wider than ~16,000 pixels.\n> -\t\t\t */\n> -\t\t\t.minPixelProcessingTime = 1.0us / 380,\n> -\t\t\t.dataBufferStrided = false,\n> -\t\t}\n> -\t},\n> -};\n> +\t\t\t\"pisp\",\n> +\t\t\t{\n> +\t\t\t\t.agcRegions = { 0, 0 },\n> +\t\t\t\t.agcZoneWeights = { 15, 15 },\n> +\t\t\t\t.awbRegions = { 32, 32 },\n> +\t\t\t\t.cacRegions = { 8, 8 },\n> +\t\t\t\t.focusRegions = { 8, 8 },\n> +\t\t\t\t.numHistogramBins = 1024,\n> +\t\t\t\t.numGammaPoints = 64,\n> +\t\t\t\t.pipelineWidth = 16,\n> +\t\t\t\t.statsInline = true,\n> +\n> +\t\t\t\t/*\n> +\t\t\t\t* The constraint below is on the rate of pixels going\n> +\t\t\t\t* from CSI2 peripheral to ISP-FE (400Mpix/s, plus tiny\n> +\t\t\t\t* overheads per scanline, for which 380Mpix/s is a\n> +\t\t\t\t* conservative bound).\n> +\t\t\t\t*\n> +\t\t\t\t* There is a 64kbit data FIFO before the bottleneck,\n> +\t\t\t\t* which means that in all reasonable cases the\n> +\t\t\t\t* constraint applies at a timescale >= 1 scanline, so\n> +\t\t\t\t* adding horizontal blanking can prevent loss.\n> +\t\t\t\t*\n> +\t\t\t\t* If the backlog were to grow beyond 64kbit during a\n> +\t\t\t\t* single scanline, there could still be loss. This\n> +\t\t\t\t* could happen using 4 lanes at 1.5Gbps at 10bpp with\n> +\t\t\t\t* frames wider than ~16,000 pixels.\n> +\t\t\t\t*/\n> +\t\t\t\t.minPixelProcessingTime = 1.0us / 380,\n> +\t\t\t\t.dataBufferStrided = false,\n> +\t\t\t}\n> +\t\t},\n> +\t};\n> +\n> +\treturn map;\n> +}\n> +\n> +} /* namespace */\n> \n> Controller::Controller()\n> \t: switchModeCalled_(false)\n> @@ -211,12 +220,12 @@ const std::string &Controller::getTarget() const\n> \n> const Controller::HardwareConfig &Controller::getHardwareConfig() const\n> {\n> -\tauto cfg = HardwareConfigMap.find(getTarget());\n> +\tauto cfg = hardwareConfigMap().find(getTarget());\n> \n> \t/*\n> \t * This really should not happen, the IPA ought to validate the target\n> \t * on initialisation.\n> \t */\n> -\tASSERT(cfg != HardwareConfigMap.end());\n> +\tASSERT(cfg != hardwareConfigMap().end());\n> \treturn cfg->second;\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 B4322BE175\n\tfor ;\n\tFri, 18 Jul 2025 08:55:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7575068F93;\n\tFri, 18 Jul 2025 10:55:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E7C36150A\n\tfor ;\n\tFri, 18 Jul 2025 10:55:33 +0200 (CEST)","from [192.168.33.15] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3738959F1;\n\tFri, 18 Jul 2025 10:54:58 +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=\"BWJVMyTf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752828898;\n\tbh=2lFam7Y9uCh8Cr+b2eZdk1RVYPzg3EksMaN3u6xnNmg=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=BWJVMyTfa+GqtnTVz36UdRVNRsYsnsgoPvsDv6h0ZEl1Yut5I7ihjBYdlIlPsEStc\n\tL+VFBEpLHKd7F3vo9hsnwJLwqhz4GjvDYUFgg5GtddT+gqEDlRd964nv8guYLcAHg+\n\tuRoyNkfL+kGxsM0WMzbzwNWkOheCwCdkdW3PjbhM=","Message-ID":"","Date":"Fri, 18 Jul 2025 10:55:29 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] ipa: rpi: Fix static initialisation order bug in the\n\tController","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250718082402.1207460-1-naush@raspberrypi.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250718082402.1207460-1-naush@raspberrypi.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\" "}}]