[{"id":31934,"web_url":"https://patchwork.libcamera.org/comment/31934/","msgid":"<38d084ef-0696-4f4e-826b-d21d4c92e108@ideasonboard.com>","date":"2024-10-28T10:08:12","subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 16/10/2024 18:03, Jacopo Mondi wrote:\n> Some IPA modules, like the RkISP1 one, call FCQueue::get(0) at\n> IPA::start() time, before any frame context has been allocated with\n> FCQueue::alloc() called at queueRequest() time.\n>\n> The FCQueue implementation aims to detect when a FrameContext is get()\n> before it is alloc()-ated, Warns about it, and initializes the\n> FrameContext before returning it.\n>\n> In case of frame#0, a get() preceding an alloc() call is not detected\n> as the \"frame == frameContext.frame\" test returns success, as\n> FrameContexts are zeroed by default.\n>\n> As a result, the first returned FrameContext is not initialized.\n>\n> Explicitly test for frame#0 to make sure the FrameContext is initialized\n> if get(0) is called before alloc(0). To avoid re-initializing a frame\n> context, in case alloc() has been called correctly before get(),\n> introduce an \"initialised\" state variable that tracks the FrameContext\n> initialisation state.\n>\n> Signed-off-by: Jacopo Mondi \nReviewed-by: Daniel Scally \n> ---\n> src/ipa/libipa/fc_queue.h | 21 ++++++++++++++++++++-\n> 1 file changed, 20 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> index b1e8bc1485d4..bfcce5a81356 100644\n> --- a/src/ipa/libipa/fc_queue.h\n> +++ b/src/ipa/libipa/fc_queue.h\n> @@ -26,11 +26,13 @@ protected:\n> \tvirtual void init(const uint32_t frameNum)\n> \t{\n> \t\tframe = frameNum;\n> +\t\tinitialised = true;\n> \t}\n> \n> private:\n> \ttemplate friend class FCQueue;\n> \tuint32_t frame;\n> +\tbool initialised = false;\n> };\n> \n> template\n> @@ -44,8 +46,10 @@ public:\n> \n> \tvoid clear()\n> \t{\n> -\t\tfor (FrameContext &ctx : contexts_)\n> +\t\tfor (FrameContext &ctx : contexts_) {\n> +\t\t\tctx.initialised = false;\n> \t\t\tctx.frame = 0;\n> +\t\t}\n> \t}\n> \n> \tFrameContext &alloc(const uint32_t frame)\n> @@ -89,6 +93,21 @@ public:\n> \t\t\t\t\t << \" has been overwritten by \"\n> \t\t\t\t\t << frameContext.frame;\n> \n> +\t\tif (frame == 0 && !frameContext.initialised) {\n> +\t\t\t/*\n> +\t\t\t * If the IPA calls get() at start() time it will get an\n> +\t\t\t * un-intialized FrameContext as the below \"frame ==\n> +\t\t\t * frameContext.frame\" check will return success because\n> +\t\t\t * FrameContexts are zeroed at creation time.\n> +\t\t\t *\n> +\t\t\t * Make sure the FrameContext gets initialised if get()\n> +\t\t\t * is called before alloc() by the IPA for frame#0.\n> +\t\t\t */\n> +\t\t\tframeContext.init(frame);\n> +\n> +\t\t\treturn frameContext;\n> +\t\t}\n> +\n> \t\tif (frame == frameContext.frame)\n> \t\t\treturn frameContext;\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 A25F3C3220\n\tfor ;\n\tMon, 28 Oct 2024 10:08:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4AFC36539F;\n\tMon, 28 Oct 2024 11:08:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F2E9618C0\n\tfor ;\n\tMon, 28 Oct 2024 11:08:15 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1C87346\n\tfor ;\n\tMon, 28 Oct 2024 11:08:13 +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=\"J/QU+ACD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730110093;\n\tbh=w8El1CjTdrmJN5NYqvCqc7YJnxcw2TTVWY5kEn9Kzkc=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=J/QU+ACDfaon6+USxPqB4dXngGdLV58A2ZTPOlQFyFDHg5u6HI+iqh7tfWDQWkB5J\n\tPijr9J8aTskZPCNiI5zwnnqQXVlyBrZqsQemUf2OMu0eNYomGQlB1IgPESj3YB0wpR\n\tnrhEMyX4FTEHh4c65J6Gxdx6Yu2ScXx68ow5c3kI=","Message-ID":"<38d084ef-0696-4f4e-826b-d21d4c92e108@ideasonboard.com>","Date":"Mon, 28 Oct 2024 10:08:12 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","To":"libcamera-devel@lists.libcamera.org","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-3-jacopo.mondi@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20241016170348.715993-3-jacopo.mondi@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":31935,"web_url":"https://patchwork.libcamera.org/comment/31935/","msgid":"","date":"2024-10-28T10:16:23","subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch. \n\nOn Wed, Oct 16, 2024 at 07:03:43PM +0200, Jacopo Mondi wrote:\n> Some IPA modules, like the RkISP1 one, call FCQueue::get(0) at\n> IPA::start() time, before any frame context has been allocated with\n> FCQueue::alloc() called at queueRequest() time.\n> \n> The FCQueue implementation aims to detect when a FrameContext is get()\n> before it is alloc()-ated, Warns about it, and initializes the\n> FrameContext before returning it.\n> \n> In case of frame#0, a get() preceding an alloc() call is not detected\n> as the \"frame == frameContext.frame\" test returns success, as\n> FrameContexts are zeroed by default.\n> \n> As a result, the first returned FrameContext is not initialized.\n> \n> Explicitly test for frame#0 to make sure the FrameContext is initialized\n> if get(0) is called before alloc(0). To avoid re-initializing a frame\n> context, in case alloc() has been called correctly before get(),\n> introduce an \"initialised\" state variable that tracks the FrameContext\n> initialisation state.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/libipa/fc_queue.h | 21 ++++++++++++++++++++-\n> 1 file changed, 20 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> index b1e8bc1485d4..bfcce5a81356 100644\n> --- a/src/ipa/libipa/fc_queue.h\n> +++ b/src/ipa/libipa/fc_queue.h\n> @@ -26,11 +26,13 @@ protected:\n> \tvirtual void init(const uint32_t frameNum)\n> \t{\n> \t\tframe = frameNum;\n> +\t\tinitialised = true;\n\nIf I got it right, this only applies to the first initialization and not\nwhen the frame context gets reused for another frame.\n\nI believe we need to implement start controls anyways. I had a prototype\nfor that in:\n\nc67de53b882e (\"pipeline: rkisp1: Apply initial controls\")\n\nIf I'm not mistaken we could do the the explicit alloc of the context\nfor frame 0 there.\n\n\n> \t}\n> \n> private:\n> \ttemplate friend class FCQueue;\n> \tuint32_t frame;\n> +\tbool initialised = false;\n> };\n> \n> template\n> @@ -44,8 +46,10 @@ public:\n> \n> \tvoid clear()\n> \t{\n> -\t\tfor (FrameContext &ctx : contexts_)\n> +\t\tfor (FrameContext &ctx : contexts_) {\n> +\t\t\tctx.initialised = false;\n> \t\t\tctx.frame = 0;\n> +\t\t}\n> \t}\n> \n> \tFrameContext &alloc(const uint32_t frame)\n> @@ -89,6 +93,21 @@ public:\n> \t\t\t\t\t << \" has been overwritten by \"\n> \t\t\t\t\t << frameContext.frame;\n> \n> +\t\tif (frame == 0 && !frameContext.initialised) {\n> +\t\t\t/*\n> +\t\t\t * If the IPA calls get() at start() time it will get an\n> +\t\t\t * un-intialized FrameContext as the below \"frame ==\n> +\t\t\t * frameContext.frame\" check will return success because\n> +\t\t\t * FrameContexts are zeroed at creation time.\n> +\t\t\t *\n> +\t\t\t * Make sure the FrameContext gets initialised if get()\n> +\t\t\t * is called before alloc() by the IPA for frame#0.\n> +\t\t\t */\n> +\t\t\tframeContext.init(frame);\n\nWouldn't it be more consistent if we warn in the get(0) case as for the\nother cases and ensure alloc get's called for frame 0?\n\nCheers,\nStefan\n\n> +\n> +\t\t\treturn frameContext;\n> +\t\t}\n> +\n> \t\tif (frame == frameContext.frame)\n> \t\t\treturn frameContext;\n> \n> -- \n> 2.47.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 30AD3BD78E\n\tfor ;\n\tMon, 28 Oct 2024 10:16:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD2056539F;\n\tMon, 28 Oct 2024 11:16:27 +0100 (CET)","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 A1EA2618C0\n\tfor ;\n\tMon, 28 Oct 2024 11:16:26 +0100 (CET)","from ideasonboard.com (unknown [188.228.25.77])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E2C20346;\n\tMon, 28 Oct 2024 11:16: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=\"OHAlBzV8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730110585;\n\tbh=oPHGX8dw5/vNRKsgNQnY11TcGRsgRc2UYPDP4UWCGaQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OHAlBzV8wFLUxGrOlRu+P9VJdtjechsMRRtTbB954j6RgZyA61FwLV6y9bQE/9xa7\n\ta6xsu5MKUEEBRzm1R0CyP2IRQ657XFmEU4XREkkNZ6gWMk7dcMxbNO7NtrI89B1mqK\n\tkc4XsTj7zkvLMBvX64BCfZyqh3B8rCXkFyHcwUZs=","Date":"Mon, 28 Oct 2024 11:16:23 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","Message-ID":"","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-3-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241016170348.715993-3-jacopo.mondi@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":31938,"web_url":"https://patchwork.libcamera.org/comment/31938/","msgid":"","date":"2024-10-28T10:39:13","subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, Oct 28, 2024 at 11:16:23AM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Oct 16, 2024 at 07:03:43PM +0200, Jacopo Mondi wrote:\n> > Some IPA modules, like the RkISP1 one, call FCQueue::get(0) at\n> > IPA::start() time, before any frame context has been allocated with\n> > FCQueue::alloc() called at queueRequest() time.\n> >\n> > The FCQueue implementation aims to detect when a FrameContext is get()\n> > before it is alloc()-ated, Warns about it, and initializes the\n> > FrameContext before returning it.\n> >\n> > In case of frame#0, a get() preceding an alloc() call is not detected\n> > as the \"frame == frameContext.frame\" test returns success, as\n> > FrameContexts are zeroed by default.\n> >\n> > As a result, the first returned FrameContext is not initialized.\n> >\n> > Explicitly test for frame#0 to make sure the FrameContext is initialized\n> > if get(0) is called before alloc(0). To avoid re-initializing a frame\n> > context, in case alloc() has been called correctly before get(),\n> > introduce an \"initialised\" state variable that tracks the FrameContext\n> > initialisation state.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/libipa/fc_queue.h | 21 ++++++++++++++++++++-\n> > 1 file changed, 20 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > index b1e8bc1485d4..bfcce5a81356 100644\n> > --- a/src/ipa/libipa/fc_queue.h\n> > +++ b/src/ipa/libipa/fc_queue.h\n> > @@ -26,11 +26,13 @@ protected:\n> > \tvirtual void init(const uint32_t frameNum)\n> > \t{\n> > \t\tframe = frameNum;\n> > +\t\tinitialised = true;\n>\n> If I got it right, this only applies to the first initialization and not\n> when the frame context gets reused for another frame.\n>\n> I believe we need to implement start controls anyways. I had a prototype\n> for that in:\n>\n> c67de53b882e (\"pipeline: rkisp1: Apply initial controls\")\n>\n> If I'm not mistaken we could do the the explicit alloc of the context\n> for frame 0 there.\n>\n>\n> > \t}\n> >\n> > private:\n> > \ttemplate friend class FCQueue;\n> > \tuint32_t frame;\n> > +\tbool initialised = false;\n> > };\n> >\n> > template\n> > @@ -44,8 +46,10 @@ public:\n> >\n> > \tvoid clear()\n> > \t{\n> > -\t\tfor (FrameContext &ctx : contexts_)\n> > +\t\tfor (FrameContext &ctx : contexts_) {\n> > +\t\t\tctx.initialised = false;\n> > \t\t\tctx.frame = 0;\n> > +\t\t}\n> > \t}\n> >\n> > \tFrameContext &alloc(const uint32_t frame)\n> > @@ -89,6 +93,21 @@ public:\n> > \t\t\t\t\t << \" has been overwritten by \"\n> > \t\t\t\t\t << frameContext.frame;\n> >\n> > +\t\tif (frame == 0 && !frameContext.initialised) {\n> > +\t\t\t/*\n> > +\t\t\t * If the IPA calls get() at start() time it will get an\n> > +\t\t\t * un-intialized FrameContext as the below \"frame ==\n> > +\t\t\t * frameContext.frame\" check will return success because\n> > +\t\t\t * FrameContexts are zeroed at creation time.\n> > +\t\t\t *\n> > +\t\t\t * Make sure the FrameContext gets initialised if get()\n> > +\t\t\t * is called before alloc() by the IPA for frame#0.\n> > +\t\t\t */\n> > +\t\t\tframeContext.init(frame);\n>\n> Wouldn't it be more consistent if we warn in the get(0) case as for the\n> other cases and ensure alloc get's called for frame 0?\n>\n\nThe only entry point (in the current implementation) for alloc() is\nqueueRequest. So if we set controls at start() when no request is\nqueued we will always get(0) before alloc(0).\n\nI think this should change going forward once we rework the IPA to\nwork based on when a frame/stats are available instead of being\nclocked by the user submitted requests, but for now I think this is\nwhat we have. Or did you mean something completely different I have\nmissed ?\n\nThanks\n j\n\n> Cheers,\n> Stefan\n>\n> > +\n> > +\t\t\treturn frameContext;\n> > +\t\t}\n> > +\n> > \t\tif (frame == frameContext.frame)\n> > \t\t\treturn frameContext;\n> >\n> > --\n> > 2.47.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 1FAEBBD78E\n\tfor ;\n\tMon, 28 Oct 2024 10:39:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB6476539F;\n\tMon, 28 Oct 2024 11:39:18 +0100 (CET)","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 7551660360\n\tfor ;\n\tMon, 28 Oct 2024 11:39:17 +0100 (CET)","from ideasonboard.com (mob-5-90-59-111.net.vodafone.it\n\t[5.90.59.111])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71F66346;\n\tMon, 28 Oct 2024 11:39:15 +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=\"CJ7Akpa3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730111955;\n\tbh=3+rSkgVSss4lJfzXiBfIx7KgE/M7WB+HlFOR2HLLCJQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CJ7Akpa3bykLkqKQDmDF/OWp44RDZYg6Ud1NMZixMc5ZzOO+zKbMaMT6agoA+CESA\n\twdCdg3d9e+bXfNHc0FGl7u7D0rw4OmtNCHOIgBpWKH2OK8qTMGiKls37/UVStPqTtp\n\tFuEjM6jPryWXv/MzjCJrwm+7ZrYxdE1NK+AH6/5w=","Date":"Mon, 28 Oct 2024 11:39:13 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","Message-ID":"","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-3-jacopo.mondi@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":31941,"web_url":"https://patchwork.libcamera.org/comment/31941/","msgid":"","date":"2024-10-28T15:40:15","subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Oct 28, 2024 at 11:39:13AM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Mon, Oct 28, 2024 at 11:16:23AM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Oct 16, 2024 at 07:03:43PM +0200, Jacopo Mondi wrote:\n> > > Some IPA modules, like the RkISP1 one, call FCQueue::get(0) at\n> > > IPA::start() time, before any frame context has been allocated with\n> > > FCQueue::alloc() called at queueRequest() time.\n> > >\n> > > The FCQueue implementation aims to detect when a FrameContext is get()\n> > > before it is alloc()-ated, Warns about it, and initializes the\n> > > FrameContext before returning it.\n> > >\n> > > In case of frame#0, a get() preceding an alloc() call is not detected\n> > > as the \"frame == frameContext.frame\" test returns success, as\n> > > FrameContexts are zeroed by default.\n> > >\n> > > As a result, the first returned FrameContext is not initialized.\n> > >\n> > > Explicitly test for frame#0 to make sure the FrameContext is initialized\n> > > if get(0) is called before alloc(0). To avoid re-initializing a frame\n> > > context, in case alloc() has been called correctly before get(),\n> > > introduce an \"initialised\" state variable that tracks the FrameContext\n> > > initialisation state.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/ipa/libipa/fc_queue.h | 21 ++++++++++++++++++++-\n> > > 1 file changed, 20 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > > index b1e8bc1485d4..bfcce5a81356 100644\n> > > --- a/src/ipa/libipa/fc_queue.h\n> > > +++ b/src/ipa/libipa/fc_queue.h\n> > > @@ -26,11 +26,13 @@ protected:\n> > > \tvirtual void init(const uint32_t frameNum)\n> > > \t{\n> > > \t\tframe = frameNum;\n> > > +\t\tinitialised = true;\n> >\n> > If I got it right, this only applies to the first initialization and not\n> > when the frame context gets reused for another frame.\n> >\n> > I believe we need to implement start controls anyways. I had a prototype\n> > for that in:\n> >\n> > c67de53b882e (\"pipeline: rkisp1: Apply initial controls\")\n> >\n> > If I'm not mistaken we could do the the explicit alloc of the context\n> > for frame 0 there.\n> >\n> >\n> > > \t}\n> > >\n> > > private:\n> > > \ttemplate friend class FCQueue;\n> > > \tuint32_t frame;\n> > > +\tbool initialised = false;\n> > > };\n> > >\n> > > template\n> > > @@ -44,8 +46,10 @@ public:\n> > >\n> > > \tvoid clear()\n> > > \t{\n> > > -\t\tfor (FrameContext &ctx : contexts_)\n> > > +\t\tfor (FrameContext &ctx : contexts_) {\n> > > +\t\t\tctx.initialised = false;\n> > > \t\t\tctx.frame = 0;\n> > > +\t\t}\n> > > \t}\n> > >\n> > > \tFrameContext &alloc(const uint32_t frame)\n> > > @@ -89,6 +93,21 @@ public:\n> > > \t\t\t\t\t << \" has been overwritten by \"\n> > > \t\t\t\t\t << frameContext.frame;\n> > >\n> > > +\t\tif (frame == 0 && !frameContext.initialised) {\n> > > +\t\t\t/*\n> > > +\t\t\t * If the IPA calls get() at start() time it will get an\n> > > +\t\t\t * un-intialized FrameContext as the below \"frame ==\n> > > +\t\t\t * frameContext.frame\" check will return success because\n> > > +\t\t\t * FrameContexts are zeroed at creation time.\n> > > +\t\t\t *\n> > > +\t\t\t * Make sure the FrameContext gets initialised if get()\n> > > +\t\t\t * is called before alloc() by the IPA for frame#0.\n> > > +\t\t\t */\n> > > +\t\t\tframeContext.init(frame);\n> >\n> > Wouldn't it be more consistent if we warn in the get(0) case as for the\n> > other cases and ensure alloc get's called for frame 0?\n> >\n> \n> The only entry point (in the current implementation) for alloc() is\n> queueRequest. So if we set controls at start() when no request is\n> queued we will always get(0) before alloc(0).\n> \n> I think this should change going forward once we rework the IPA to\n> work based on when a frame/stats are available instead of being\n> clocked by the user submitted requests, but for now I think this is\n> what we have. Or did you mean something completely different I have\n> missed ?\n\nMy thought was to call alloc(0) in start() before setControls(0) if no\nrequest was queued. So that the edge case is handled outside of the\nFCQueue and the FCQueue can consistently warn for any context that\nwasn't alloced before a get(). \n\nOr is there another reason to silence the warning in FCQueue? As imho\ngetting a frame without prior alloc should warn in any case.\n\nCheers,\nStefan\n\n> \n> Thanks\n> j\n> \n> > Cheers,\n> > Stefan\n> >\n> > > +\n> > > +\t\t\treturn frameContext;\n> > > +\t\t}\n> > > +\n> > > \t\tif (frame == frameContext.frame)\n> > > \t\t\treturn frameContext;\n> > >\n> > > --\n> > > 2.47.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 8ECE8C3220\n\tfor ;\n\tMon, 28 Oct 2024 15:40:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 237CE6539E;\n\tMon, 28 Oct 2024 16:40:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 344BF60367\n\tfor ;\n\tMon, 28 Oct 2024 16:40:19 +0100 (CET)","from ideasonboard.com (tmo-120-11.customers.d1-online.com\n\t[80.187.120.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3BF843;\n\tMon, 28 Oct 2024 16:40:16 +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=\"bNrIFwVC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730130017;\n\tbh=vXdBc+yYWIjYZW451Z31TUBeDfYBXuQNNbr3E7iGbQY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bNrIFwVCRXY2hjutJ2ACGgy/nmjcoaJiRlXQ7GT0rOdrn7yB1ePeaDHJBQvJGVwiY\n\tRxOgJutk9fUzjG5gsk8jGFMe+GGQj4WCbYyEDq9SfhbJSU2WAwoA/y5wdTvUOKoIeb\n\tq25aveBL9ikE/FgVhIlt2lepVYFgTkKiIdNwnILQ=","Date":"Mon, 28 Oct 2024 16:40:15 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","Message-ID":"","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-3-jacopo.mondi@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":31942,"web_url":"https://patchwork.libcamera.org/comment/31942/","msgid":"","date":"2024-10-28T16:24:48","subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, Oct 28, 2024 at 04:40:15PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> On Mon, Oct 28, 2024 at 11:39:13AM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Mon, Oct 28, 2024 at 11:16:23AM +0100, Stefan Klug wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Wed, Oct 16, 2024 at 07:03:43PM +0200, Jacopo Mondi wrote:\n> > > > Some IPA modules, like the RkISP1 one, call FCQueue::get(0) at\n> > > > IPA::start() time, before any frame context has been allocated with\n> > > > FCQueue::alloc() called at queueRequest() time.\n> > > >\n> > > > The FCQueue implementation aims to detect when a FrameContext is get()\n> > > > before it is alloc()-ated, Warns about it, and initializes the\n> > > > FrameContext before returning it.\n> > > >\n> > > > In case of frame#0, a get() preceding an alloc() call is not detected\n> > > > as the \"frame == frameContext.frame\" test returns success, as\n> > > > FrameContexts are zeroed by default.\n> > > >\n> > > > As a result, the first returned FrameContext is not initialized.\n> > > >\n> > > > Explicitly test for frame#0 to make sure the FrameContext is initialized\n> > > > if get(0) is called before alloc(0). To avoid re-initializing a frame\n> > > > context, in case alloc() has been called correctly before get(),\n> > > > introduce an \"initialised\" state variable that tracks the FrameContext\n> > > > initialisation state.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/ipa/libipa/fc_queue.h | 21 ++++++++++++++++++++-\n> > > > 1 file changed, 20 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > > > index b1e8bc1485d4..bfcce5a81356 100644\n> > > > --- a/src/ipa/libipa/fc_queue.h\n> > > > +++ b/src/ipa/libipa/fc_queue.h\n> > > > @@ -26,11 +26,13 @@ protected:\n> > > > \tvirtual void init(const uint32_t frameNum)\n> > > > \t{\n> > > > \t\tframe = frameNum;\n> > > > +\t\tinitialised = true;\n> > >\n> > > If I got it right, this only applies to the first initialization and not\n> > > when the frame context gets reused for another frame.\n> > >\n> > > I believe we need to implement start controls anyways. I had a prototype\n> > > for that in:\n> > >\n> > > c67de53b882e (\"pipeline: rkisp1: Apply initial controls\")\n> > >\n> > > If I'm not mistaken we could do the the explicit alloc of the context\n> > > for frame 0 there.\n> > >\n> > >\n> > > > \t}\n> > > >\n> > > > private:\n> > > > \ttemplate friend class FCQueue;\n> > > > \tuint32_t frame;\n> > > > +\tbool initialised = false;\n> > > > };\n> > > >\n> > > > template\n> > > > @@ -44,8 +46,10 @@ public:\n> > > >\n> > > > \tvoid clear()\n> > > > \t{\n> > > > -\t\tfor (FrameContext &ctx : contexts_)\n> > > > +\t\tfor (FrameContext &ctx : contexts_) {\n> > > > +\t\t\tctx.initialised = false;\n> > > > \t\t\tctx.frame = 0;\n> > > > +\t\t}\n> > > > \t}\n> > > >\n> > > > \tFrameContext &alloc(const uint32_t frame)\n> > > > @@ -89,6 +93,21 @@ public:\n> > > > \t\t\t\t\t << \" has been overwritten by \"\n> > > > \t\t\t\t\t << frameContext.frame;\n> > > >\n> > > > +\t\tif (frame == 0 && !frameContext.initialised) {\n> > > > +\t\t\t/*\n> > > > +\t\t\t * If the IPA calls get() at start() time it will get an\n> > > > +\t\t\t * un-intialized FrameContext as the below \"frame ==\n> > > > +\t\t\t * frameContext.frame\" check will return success because\n> > > > +\t\t\t * FrameContexts are zeroed at creation time.\n> > > > +\t\t\t *\n> > > > +\t\t\t * Make sure the FrameContext gets initialised if get()\n> > > > +\t\t\t * is called before alloc() by the IPA for frame#0.\n> > > > +\t\t\t */\n> > > > +\t\t\tframeContext.init(frame);\n> > >\n> > > Wouldn't it be more consistent if we warn in the get(0) case as for the\n> > > other cases and ensure alloc get's called for frame 0?\n> > >\n> >\n> > The only entry point (in the current implementation) for alloc() is\n> > queueRequest. So if we set controls at start() when no request is\n> > queued we will always get(0) before alloc(0).\n> >\n> > I think this should change going forward once we rework the IPA to\n> > work based on when a frame/stats are available instead of being\n> > clocked by the user submitted requests, but for now I think this is\n> > what we have. Or did you mean something completely different I have\n> > missed ?\n>\n> My thought was to call alloc(0) in start() before setControls(0) if no\n> request was queued. So that the edge case is handled outside of the\n\nThat's a possibility. I'm not sure if we have decided or not we're\ngoing to allow Request pre-queing or not (it's a rather invasive API\nchange) but in that case each single IPA should go and check if a\nRequests have been queued before start.\n\n> FCQueue and the FCQueue can consistently warn for any context that\n> wasn't alloced before a get().\n>\n> Or is there another reason to silence the warning in FCQueue? As imho\n> getting a frame without prior alloc should warn in any case.\n>\n\nWeeeeeeel, moving forward we're going to see Request underrun more\noften, I don't think we should consider that a WARN, but I'm sure\nwe're going to discuss it.\n\n\n> Cheers,\n> Stefan\n>\n> >\n> > Thanks\n> > j\n> >\n> > > Cheers,\n> > > Stefan\n> > >\n> > > > +\n> > > > +\t\t\treturn frameContext;\n> > > > +\t\t}\n> > > > +\n> > > > \t\tif (frame == frameContext.frame)\n> > > > \t\t\treturn frameContext;\n> > > >\n> > > > --\n> > > > 2.47.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 E188EBD78E\n\tfor ;\n\tMon, 28 Oct 2024 16:24:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D99B26539E;\n\tMon, 28 Oct 2024 17:24:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 24CA760367\n\tfor ;\n\tMon, 28 Oct 2024 17:24:53 +0100 (CET)","from ideasonboard.com (mob-5-90-59-111.net.vodafone.it\n\t[5.90.59.111])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8EC11641;\n\tMon, 28 Oct 2024 17:24:50 +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=\"TUNPIdrs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730132690;\n\tbh=wR6hL0R4WnxLe+EG3tkW58uX07fe5DVIZGMmXdNKefc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TUNPIdrsW48Lpm084vXh+/I4JbG3S2PbjHGRlX3F+QC9TD/5U5l+guHJDWDTqNlOm\n\tDvhDcLEtLsotpQYk5Nv16UVYLNOwi9IMkyoeF0tj8iPKeueqznwsNACV9r6y8xHslH\n\tXRqM+ZzUkeoaB49nKAK19N/4UscRybsnRNg74mnQ=","Date":"Mon, 28 Oct 2024 17:24:48 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","Message-ID":"","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-3-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":31946,"web_url":"https://patchwork.libcamera.org/comment/31946/","msgid":"","date":"2024-10-28T17:19:25","subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Oct 28, 2024 at 05:24:48PM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Mon, Oct 28, 2024 at 04:40:15PM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, Oct 28, 2024 at 11:39:13AM +0100, Jacopo Mondi wrote:\n> > > Hi Stefan\n> > >\n> > > On Mon, Oct 28, 2024 at 11:16:23AM +0100, Stefan Klug wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > On Wed, Oct 16, 2024 at 07:03:43PM +0200, Jacopo Mondi wrote:\n> > > > > Some IPA modules, like the RkISP1 one, call FCQueue::get(0) at\n> > > > > IPA::start() time, before any frame context has been allocated with\n> > > > > FCQueue::alloc() called at queueRequest() time.\n> > > > >\n> > > > > The FCQueue implementation aims to detect when a FrameContext is get()\n> > > > > before it is alloc()-ated, Warns about it, and initializes the\n> > > > > FrameContext before returning it.\n> > > > >\n> > > > > In case of frame#0, a get() preceding an alloc() call is not detected\n> > > > > as the \"frame == frameContext.frame\" test returns success, as\n> > > > > FrameContexts are zeroed by default.\n> > > > >\n> > > > > As a result, the first returned FrameContext is not initialized.\n> > > > >\n> > > > > Explicitly test for frame#0 to make sure the FrameContext is initialized\n> > > > > if get(0) is called before alloc(0). To avoid re-initializing a frame\n> > > > > context, in case alloc() has been called correctly before get(),\n> > > > > introduce an \"initialised\" state variable that tracks the FrameContext\n> > > > > initialisation state.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/ipa/libipa/fc_queue.h | 21 ++++++++++++++++++++-\n> > > > > 1 file changed, 20 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > > > > index b1e8bc1485d4..bfcce5a81356 100644\n> > > > > --- a/src/ipa/libipa/fc_queue.h\n> > > > > +++ b/src/ipa/libipa/fc_queue.h\n> > > > > @@ -26,11 +26,13 @@ protected:\n> > > > > \tvirtual void init(const uint32_t frameNum)\n> > > > > \t{\n> > > > > \t\tframe = frameNum;\n> > > > > +\t\tinitialised = true;\n> > > >\n> > > > If I got it right, this only applies to the first initialization and not\n> > > > when the frame context gets reused for another frame.\n> > > >\n> > > > I believe we need to implement start controls anyways. I had a prototype\n> > > > for that in:\n> > > >\n> > > > c67de53b882e (\"pipeline: rkisp1: Apply initial controls\")\n> > > >\n> > > > If I'm not mistaken we could do the the explicit alloc of the context\n> > > > for frame 0 there.\n> > > >\n> > > >\n> > > > > \t}\n> > > > >\n> > > > > private:\n> > > > > \ttemplate friend class FCQueue;\n> > > > > \tuint32_t frame;\n> > > > > +\tbool initialised = false;\n> > > > > };\n> > > > >\n> > > > > template\n> > > > > @@ -44,8 +46,10 @@ public:\n> > > > >\n> > > > > \tvoid clear()\n> > > > > \t{\n> > > > > -\t\tfor (FrameContext &ctx : contexts_)\n> > > > > +\t\tfor (FrameContext &ctx : contexts_) {\n> > > > > +\t\t\tctx.initialised = false;\n> > > > > \t\t\tctx.frame = 0;\n> > > > > +\t\t}\n> > > > > \t}\n> > > > >\n> > > > > \tFrameContext &alloc(const uint32_t frame)\n> > > > > @@ -89,6 +93,21 @@ public:\n> > > > > \t\t\t\t\t << \" has been overwritten by \"\n> > > > > \t\t\t\t\t << frameContext.frame;\n> > > > >\n> > > > > +\t\tif (frame == 0 && !frameContext.initialised) {\n> > > > > +\t\t\t/*\n> > > > > +\t\t\t * If the IPA calls get() at start() time it will get an\n> > > > > +\t\t\t * un-intialized FrameContext as the below \"frame ==\n> > > > > +\t\t\t * frameContext.frame\" check will return success because\n> > > > > +\t\t\t * FrameContexts are zeroed at creation time.\n> > > > > +\t\t\t *\n> > > > > +\t\t\t * Make sure the FrameContext gets initialised if get()\n> > > > > +\t\t\t * is called before alloc() by the IPA for frame#0.\n> > > > > +\t\t\t */\n> > > > > +\t\t\tframeContext.init(frame);\n> > > >\n> > > > Wouldn't it be more consistent if we warn in the get(0) case as for the\n> > > > other cases and ensure alloc get's called for frame 0?\n> > > >\n> > >\n> > > The only entry point (in the current implementation) for alloc() is\n> > > queueRequest. So if we set controls at start() when no request is\n> > > queued we will always get(0) before alloc(0).\n> > >\n> > > I think this should change going forward once we rework the IPA to\n> > > work based on when a frame/stats are available instead of being\n> > > clocked by the user submitted requests, but for now I think this is\n> > > what we have. Or did you mean something completely different I have\n> > > missed ?\n> >\n> > My thought was to call alloc(0) in start() before setControls(0) if no\n> > request was queued. So that the edge case is handled outside of the\n> \n> That's a possibility. I'm not sure if we have decided or not we're\n> going to allow Request pre-queing or not (it's a rather invasive API\n> change) but in that case each single IPA should go and check if a\n> Requests have been queued before start.\n\nOh, I forgot that queueRequest is only allowed after start() sorry.\nThat is something we maybe should discuss again. In my head it's always\nqueueRequests() -> start(). Now I understood that this is not a special\ncase but happens on every start and that also explains why you don't\nwant to warn. Sorry that it took so long on my side.\n\nI still don't like the asymmetric handling of get(0) but don't see an\neasy way around. Maybe we should completely remove the alloc() function\nand always do initialization on demand. But I didn't think that through.\n\nAs it stands now:\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> \n> > FCQueue and the FCQueue can consistently warn for any context that\n> > wasn't alloced before a get().\n> >\n> > Or is there another reason to silence the warning in FCQueue? As imho\n> > getting a frame without prior alloc should warn in any case.\n> >\n> \n> Weeeeeeel, moving forward we're going to see Request underrun more\n> often, I don't think we should consider that a WARN, but I'm sure\n> we're going to discuss it.\n> \n> \n> > Cheers,\n> > Stefan\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > Cheers,\n> > > > Stefan\n> > > >\n> > > > > +\n> > > > > +\t\t\treturn frameContext;\n> > > > > +\t\t}\n> > > > > +\n> > > > > \t\tif (frame == frameContext.frame)\n> > > > > \t\t\treturn frameContext;\n> > > > >\n> > > > > --\n> > > > > 2.47.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 AB904BD78E\n\tfor ;\n\tMon, 28 Oct 2024 17:19:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0D086539F;\n\tMon, 28 Oct 2024 18:19:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5AAA65388\n\tfor ;\n\tMon, 28 Oct 2024 18:19:33 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a01:599:708:840f:814a:6b8b:c97e:5da6])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33944641;\n\tMon, 28 Oct 2024 18:19:29 +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=\"UYniB85q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730135969;\n\tbh=aqJIl18bkGLNBTWe6vtCMuxSS5mlZPK7TuHBcpgwO7Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UYniB85qhaSJdPi1XD1V9dqP7tI7VEO15JwOgPVDODBf57wdKBCM1AVxTxYjuoykk\n\tTXkzsoazrW0RvnkQI5jHWXbU3ecEiH8pyGkmhnAZ+GnzD4/V270pRgr09hxYUb1NL3\n\tNgFBwmIh5P9KbQl4qFlrSKj5ReX3qjhKGyDwoccY=","Date":"Mon, 28 Oct 2024 18:19:25 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH 2/4] libipa: FCQueue: Make sure FrameContext#0 is\n\tinitialized","Message-ID":"","References":"<20241016170348.715993-1-jacopo.mondi@ideasonboard.com>\n\t<20241016170348.715993-3-jacopo.mondi@ideasonboard.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]