[{"id":23266,"web_url":"https://patchwork.libcamera.org/comment/23266/","msgid":"<20220531074141.GL2630765@pyrite.rasen.tech>","date":"2022-05-31T07:41:41","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context:\n\tExtend FCQueue::get()","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Umang,\n\nOn Fri, May 27, 2022 at 09:17:39PM +0200, Umang Jain via libcamera-devel wrote:\n> Extend the FCQueue::get() to return a IPAFrameContext for a\n> non-existent frame. The .get() should be able to figure out if a\n> existent frame context is asked for, or to create a new frame context\n> based on the next available position. To keep track of next available\n> position in the queue, use of head and tail pointer are used.\n\ns/use of//\n\ns/pointer/pointers/\n\n> \n> We specifically want to have access to the queue through the\n\ns/have/only allow/ ?\n\n> .get() function hence operator[] is deleted for FCQueue as\n> part of this patch as well.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/ipu3/ipa_context.cpp | 81 +++++++++++++++++++++++++++++++++---\n> src/ipa/ipu3/ipa_context.h | 5 +++\n> src/ipa/ipu3/ipu3.cpp | 4 +-\n> 3 files changed, 83 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index e5010e3f..dcce6b48 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -217,32 +217,96 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> * \\brief Analogue gain multiplier\n> */\n> \n> +/**\n> + * \\class FCQueue\n> + * \\brief A FIFO circular queue holding IPAFrameContext(s)\n> + *\n> + * FCQueue holds all the IPAFrameContext(s) related to frames required\n> + * to be processed by the IPA at a given point. A IPAFrameContext is created\n\ns/A/An/\n\n> + * on the first call FCQueue::get(frame) for that frame. Subsequent calls to\n> + * FCQueue::get() with the same frame number shall return the IPAFrameContext\n> + * previously created until the frame is marked as complete through\n> + * FCQueue::completeFrame(frame).\n> + */\n> +\n> +/**\n> + * \\var FCQueue::head\n> + * \\brief A pointer to the a IPAFrameContext next expected to complete\n> + */\n> +\n> +/**\n> + * \\var FCQueue::tail\n> + * \\brief A pointer to the latest IPAFrameContext created\n> + */\n> +\n> /**\n> * \\brief FCQueue constructor\n> */\n> FCQueue::FCQueue()\n> +\t: head(nullptr), tail(nullptr)\n> {\n> \tclear();\n> }\n> \n> /**\n> - * Retrieve the IPAFrameContext for the frame\n> + * \\brief Retrieve the IPAFrameContext for the frame\n> * \\param[in] frame Frame number for which the IPAFrameContext needs to\n> * retrieved\n> *\n> + * This functions returns the IPAFrameContext for the desired frame. If the\n\ns/functions/function/\n\n> + * frame context does not exists in the queue, the next available reference\n\ns/exists/exist/\n\n> + * of the position in the queue, is returned. It is the responsibility of the\n\ns/,//\n\n> + * caller to fill the correct IPAFrameContext parameters of newly returned\n> + * IPAFrameContext.\n> + *\n> * \\return Reference to the IPAFrameContext\n> */\n> IPAFrameContext &FCQueue::get(uint32_t frame)\n> {\n> -\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> +\tif (frame <= tail->frame) {\n> +\t\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> +\t\tif (frame != frameContext.frame)\n> +\t\t\tLOG(IPAIPU3, Warning)\n> +\t\t\t\t<< \"Got wrong frame context for frame \" << frame;\n> +\t\treturn frameContext;\n> +\t} else {\n> +\t\t/*\n> +\t\t * Frame context doesn't exist yet so get the next available\n> +\t\t * position in the queue.\n> +\t\t */\n> +\t\ttail = &this->at((tail->frame + 1) % kMaxFrameContexts);\n> +\t\ttail->frame = frame;\n\nIf we're overflowing the queue, don't we want to avoid overwriting tail?\n\n> \n> -\tif (frame != frameContext.frame) {\n> +\t\t/* Avoid over-queuing of frame contexts */\n> +\t\tif (tail->frame > kMaxFrameContexts &&\n> +\t\t (tail->frame - head->frame) >= kMaxFrameContexts) {\n> +\t\t\tLOG(IPAIPU3, Error) << \"FCQueue is full!\";\n> +\n> +\t\t\t/* \\todo: Determine return reference? or make it fatal? */\n\niirc we discussed that the IPA should be notified if the get() fails so\nit could handle it somehow (like saving it in another queue). So...\nwhat about returning a pointer? Or is that dangerous :/\n\n> +\t\t}\n> +\n> +\t\treturn *tail;\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Notifies the FCQueue that a frame has been completed\n> + * \\param[in] frame The completed frame number\n> + */\n> +void FCQueue::completeFrame(uint32_t frame)\n> +{\n> +\tif (head->frame != frame)\n> \t\tLOG(IPAIPU3, Warning)\n> -\t\t\t<< \"Got wrong frame context for frame\" << frame\n> -\t\t\t<< \" or frame context doesn't exist yet\";\n> +\t\t\t<< \"Frame \" << frame << \" completed out-of-sync?\";\n> +\n> +\tif (frame > tail->frame) {\n> +\t\tLOG(IPAIPU3, Error)\n> +\t\t\t<< \"Completing a frame \" << frame\n> +\t\t\t<< \" not present in the queue is disallowed\";\n> +\t\treturn;\n> \t}\n> \n> -\treturn frameContext;\n> +\thead = &this->get(frame + 1);\n\nThis ought to work fine even with frame drop right...? Because the\napplication still has to queue requests if it wants frames... so the\nskipped ones would still be completed... so the +1 increment will be\ncorrect.\n\n> }\n> \n> /**\n> @@ -252,6 +316,11 @@ void FCQueue::clear()\n> {\n> \tIPAFrameContext initFrameContext;\n> \tthis->fill(initFrameContext);\n> +\n> +\t/* Intialise 0th index to frame 0 */\n> +\tthis->at(0).frame = 0;\n> +\ttail = &this->at(0);\n> +\thead = tail;\n> }\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 61454b41..b36ec61d 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -93,9 +93,14 @@ class FCQueue : public std::array\n> {\n> public:\n> \tFCQueue();\n> +\tFCQueue &operator[](const FCQueue &) = delete;\n> \n> \tvoid clear();\n> +\tvoid completeFrame(uint32_t frame);\n> \tIPAFrameContext &get(uint32_t frame);\n> +\n> +\tIPAFrameContext *head;\n> +\tIPAFrameContext *tail;\n\nShould these have the _ suffix (because this is a class and not a\nstruct)?\n\n\nPaul\n\n> };\n> \n> struct IPAContext {\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index c48d2f62..e09c5d05 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -605,6 +605,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \t */\n> \n> \tmetadataReady.emit(frame, ctrls);\n> +\n> +\tcontext_.frameContexts.completeFrame(frame);\n> }\n> \n> /**\n> @@ -618,7 +620,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> -\tcontext_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> +\tcontext_.frameContexts.get(frame) = { frame, controls };\n> }\n> \n> /**\n> -- \n> 2.31.1\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 C2636BD160\n\tfor ;\n\tTue, 31 May 2022 07:41:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1835665635;\n\tTue, 31 May 2022 09:41:51 +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 2C4956040A\n\tfor ;\n\tTue, 31 May 2022 09:41:50 +0200 (CEST)","from pyrite.rasen.tech (softbank036240126034.bbtec.net\n\t[36.240.126.34])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 765096F0;\n\tTue, 31 May 2022 09:41:48 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653982911;\n\tbh=cldAYhAvsr9lRJL8bxnjsyZec+7veAHdqag9JkbAkco=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3AQxYnTJmZd5x9Xsv10m9h9lLtbkTaM6XqADx+B0aZt24HGk250SAylpZ8UaT5sGQ\n\tRAiWmDrlpudsDtYQU6LiQ2g5snl0e8oe6ljqAZCEt+D0/3UNuN0iIDnpEb6K6DByly\n\twaAGfZa6WeIVgeJNUpiaOw2MVugJlziiDQ3zTUej7Dzd+CEOCewtjDVY5Bp0XacY02\n\tAwd00LnyQRXrIPuTCbwBoLY7D7K2LYSkqamf1zHuppdO1G+0LHyA1C2ifwt3YgeR54\n\t09t3Av26o08J+YKVtdT5uvKC/RDmph/ahv7Z2eNCJtUyQtT6MMv10AH1dfEQEX9nhu\n\thd4DWKBQED2qw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653982909;\n\tbh=cldAYhAvsr9lRJL8bxnjsyZec+7veAHdqag9JkbAkco=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ijj0UiHKEOeka8msSSHpH+bcEVnEcCbDSSkv2yvMgjlRE6vAoH4ZBTo5o692U6IVd\n\t7r98af8aAFTjc2dCd36NLNQKklI1xY4FE+57CiM5R4ijgoirJS6+VprUaNlxtZc030\n\t5tMOQIMVCc5h6ZNAt0ctVYO576xqPcswbdc+P4Wg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ijj0UiHK\"; dkim-atps=neutral","Date":"Tue, 31 May 2022 16:41:41 +0900","To":"Umang Jain ","Message-ID":"<20220531074141.GL2630765@pyrite.rasen.tech>","References":"<20220527191740.242300-1-umang.jain@ideasonboard.com>\n\t<20220527191740.242300-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220527191740.242300-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context:\n\tExtend FCQueue::get()","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","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23268,"web_url":"https://patchwork.libcamera.org/comment/23268/","msgid":"<6bbefe7a-6fe3-e9e0-e9e4-94c3b86c311e@ideasonboard.com>","date":"2022-05-31T13:50:32","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context:\n\tExtend FCQueue::get()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the review.\n\nOn 5/31/22 09:41, paul.elder@ideasonboard.com wrote:\n> Hi Umang,\n>\n> On Fri, May 27, 2022 at 09:17:39PM +0200, Umang Jain via libcamera-devel wrote:\n>> Extend the FCQueue::get() to return a IPAFrameContext for a\n>> non-existent frame. The .get() should be able to figure out if a\n>> existent frame context is asked for, or to create a new frame context\n>> based on the next available position. To keep track of next available\n>> position in the queue, use of head and tail pointer are used.\n> s/use of//\n>\n> s/pointer/pointers/\n>\n>> We specifically want to have access to the queue through the\n> s/have/only allow/ ?\n>\n>> .get() function hence operator[] is deleted for FCQueue as\n>> part of this patch as well.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/ipa/ipu3/ipa_context.cpp | 81 +++++++++++++++++++++++++++++++++---\n>> src/ipa/ipu3/ipa_context.h | 5 +++\n>> src/ipa/ipu3/ipu3.cpp | 4 +-\n>> 3 files changed, 83 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>> index e5010e3f..dcce6b48 100644\n>> --- a/src/ipa/ipu3/ipa_context.cpp\n>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>> @@ -217,32 +217,96 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n>> * \\brief Analogue gain multiplier\n>> */\n>> \n>> +/**\n>> + * \\class FCQueue\n>> + * \\brief A FIFO circular queue holding IPAFrameContext(s)\n>> + *\n>> + * FCQueue holds all the IPAFrameContext(s) related to frames required\n>> + * to be processed by the IPA at a given point. A IPAFrameContext is created\n> s/A/An/\n>\n>> + * on the first call FCQueue::get(frame) for that frame. Subsequent calls to\n>> + * FCQueue::get() with the same frame number shall return the IPAFrameContext\n>> + * previously created until the frame is marked as complete through\n>> + * FCQueue::completeFrame(frame).\n>> + */\n>> +\n>> +/**\n>> + * \\var FCQueue::head\n>> + * \\brief A pointer to the a IPAFrameContext next expected to complete\n>> + */\n>> +\n>> +/**\n>> + * \\var FCQueue::tail\n>> + * \\brief A pointer to the latest IPAFrameContext created\n>> + */\n>> +\n>> /**\n>> * \\brief FCQueue constructor\n>> */\n>> FCQueue::FCQueue()\n>> +\t: head(nullptr), tail(nullptr)\n>> {\n>> \tclear();\n>> }\n>> \n>> /**\n>> - * Retrieve the IPAFrameContext for the frame\n>> + * \\brief Retrieve the IPAFrameContext for the frame\n>> * \\param[in] frame Frame number for which the IPAFrameContext needs to\n>> * retrieved\n>> *\n>> + * This functions returns the IPAFrameContext for the desired frame. If the\n> s/functions/function/\n>\n>> + * frame context does not exists in the queue, the next available reference\n> s/exists/exist/\n>\n>> + * of the position in the queue, is returned. It is the responsibility of the\n> s/,//\n>\n>> + * caller to fill the correct IPAFrameContext parameters of newly returned\n>> + * IPAFrameContext.\n>> + *\n>> * \\return Reference to the IPAFrameContext\n>> */\n>> IPAFrameContext &FCQueue::get(uint32_t frame)\n>> {\n>> -\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>> +\tif (frame <= tail->frame) {\n>> +\t\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>> +\t\tif (frame != frameContext.frame)\n>> +\t\t\tLOG(IPAIPU3, Warning)\n>> +\t\t\t\t<< \"Got wrong frame context for frame \" << frame;\n>> +\t\treturn frameContext;\n>> +\t} else {\n>> +\t\t/*\n>> +\t\t * Frame context doesn't exist yet so get the next available\n>> +\t\t * position in the queue.\n>> +\t\t */\n>> +\t\ttail = &this->at((tail->frame + 1) % kMaxFrameContexts);\n>> +\t\ttail->frame = frame;\n> If we're overflowing the queue, don't we want to avoid overwriting tail?\n\n\nI haven't particularly decided on how should be we handle overflowing of \nrequests. I just\" detect it\" for now, and print an error\n(as mentioned in the \\todo below) so yes, it is a matter of discussion \nand some design thoughts. I don't have a very good options right now\n\n>\n>> \n>> -\tif (frame != frameContext.frame) {\n>> +\t\t/* Avoid over-queuing of frame contexts */\n>> +\t\tif (tail->frame > kMaxFrameContexts &&\n>> +\t\t (tail->frame - head->frame) >= kMaxFrameContexts) {\n>> +\t\t\tLOG(IPAIPU3, Error) << \"FCQueue is full!\";\n>> +\n>> +\t\t\t/* \\todo: Determine return reference? or make it fatal? */\n> iirc we discussed that the IPA should be notified if the get() fails so\n> it could handle it somehow (like saving it in another queue). So...\n> what about returning a pointer? Or is that dangerous :/\n\n\nI don't recall the discussion of maintaining yet another queue in the \nIPA. Yes, we should notify if the .get() fails but what's will it get \nplumbed to?\n\nSince we return a reference, we can't return a nullptr type reference. \nWe will have to return something, or change the function signature\n(overall). Maybe if we want to notify .get() failed then how about `bool \nFcQueue::get(uint32_t frame, IPAFrameContext &outFrameContext)` ?\nJust bouncing thoughts...\n\nBut are we sure that in some cases, the IPA::queueRequest() can be \noverflown with requests?\nI need to check that probability because the IPU3 pipeline handler \nitself maintains two queue for requests I think which are:\n\n\n     std::queue pendingRequests_;\n     std::queue processingRequests_;\n\nSo everything goes through pendingRequests_ first and then to \nprocessingRequests_.\n\nI see that before the request is popped from pendingRequests_ it does \nget queued to IPA.\nSo maybe we can surely overflow in the IPA itself....\n\n>\n>> +\t\t}\n>> +\n>> +\t\treturn *tail;\n>> +\t}\n>> +}\n>> +\n>> +/**\n>> + * \\brief Notifies the FCQueue that a frame has been completed\n>> + * \\param[in] frame The completed frame number\n>> + */\n>> +void FCQueue::completeFrame(uint32_t frame)\n>> +{\n>> +\tif (head->frame != frame)\n>> \t\tLOG(IPAIPU3, Warning)\n>> -\t\t\t<< \"Got wrong frame context for frame\" << frame\n>> -\t\t\t<< \" or frame context doesn't exist yet\";\n>> +\t\t\t<< \"Frame \" << frame << \" completed out-of-sync?\";\n>> +\n>> +\tif (frame > tail->frame) {\n>> +\t\tLOG(IPAIPU3, Error)\n>> +\t\t\t<< \"Completing a frame \" << frame\n>> +\t\t\t<< \" not present in the queue is disallowed\";\n>> +\t\treturn;\n>> \t}\n>> \n>> -\treturn frameContext;\n>> +\thead = &this->get(frame + 1);\n> This ought to work fine even with frame drop right...? Because the\n> application still has to queue requests if it wants frames... so the\n> skipped ones would still be completed... so the +1 increment will be\n> correct.\n\n\nYes it should, that was more or less why we went with ring-buffer \nimplementation rather than a queue\nAnd we don't care to cleanup since its a fixed-sized ring buffer. Things \nwill get over-written as requests come in\n\n>\n>> }\n>> \n>> /**\n>> @@ -252,6 +316,11 @@ void FCQueue::clear()\n>> {\n>> \tIPAFrameContext initFrameContext;\n>> \tthis->fill(initFrameContext);\n>> +\n>> +\t/* Intialise 0th index to frame 0 */\n>> +\tthis->at(0).frame = 0;\n>> +\ttail = &this->at(0);\n>> +\thead = tail;\n>> }\n>> \n>> } /* namespace ipa::ipu3 */\n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index 61454b41..b36ec61d 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -93,9 +93,14 @@ class FCQueue : public std::array\n>> {\n>> public:\n>> \tFCQueue();\n>> +\tFCQueue &operator[](const FCQueue &) = delete;\n>> \n>> \tvoid clear();\n>> +\tvoid completeFrame(uint32_t frame);\n>> \tIPAFrameContext &get(uint32_t frame);\n>> +\n>> +\tIPAFrameContext *head;\n>> +\tIPAFrameContext *tail;\n> Should these have the _ suffix (because this is a class and not a\n> struct)?\n\n\nAh the _ suffix are for private members I think. I haven't made head and \ntail private here. I think they need to be private!\n\n>\n>\n> Paul\n>\n>> };\n>> \n>> struct IPAContext {\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index c48d2f62..e09c5d05 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -605,6 +605,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n>> \t */\n>> \n>> \tmetadataReady.emit(frame, ctrls);\n>> +\n>> +\tcontext_.frameContexts.completeFrame(frame);\n>> }\n>> \n>> /**\n>> @@ -618,7 +620,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n>> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n>> {\n>> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n>> -\tcontext_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n>> +\tcontext_.frameContexts.get(frame) = { frame, controls };\n>> }\n>> \n>> /**\n>> -- \n>> 2.31.1\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 BF514BD161\n\tfor ;\n\tTue, 31 May 2022 13:50:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C7D8365631;\n\tTue, 31 May 2022 15:50:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1BEDF6040A\n\tfor ;\n\tTue, 31 May 2022 15:50:42 +0200 (CEST)","from [192.168.1.68] (235.red-81-36-186.dynamicip.rima-tde.net\n\t[81.36.186.235])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 73843474;\n\tTue, 31 May 2022 15:50:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654005043;\n\tbh=mmBuw07sijDnt0PIqZlYplC1nVv3d5wO+GRi4TsgsxI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=GSOOlNiug/20a+JBPiQ3CvPdC0L0M8RZkR1Ez7MFmx5ikPYRWG5OJ7acSMe6hLPq2\n\tQQhL2J0nDUdwy1XOviV4J9NtKnw+iSaiWAP167/Y3k/MY2kksQXNSZ+ciIW6ECBlB0\n\tmmYZCw8WmBuN6adKoDcqlGESEXlpSBb9EdnVuseXvLVuV9K/ajFwIlXcK0rT4nNHCn\n\tVpWdJJhi2veoQMdlyUOox3ujWRO1Y6xaR7kVQ+FquOny/GsrAvQqAMszA6CXqG0UR5\n\tc1jNPPqZIW6xw0aWMK+8sp4h/9/35+/C0Gp8+qQsbDtvwYakf6nv0XSTtRfKKDdgH3\n\tH0PHMNwAqQ9cA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654005041;\n\tbh=mmBuw07sijDnt0PIqZlYplC1nVv3d5wO+GRi4TsgsxI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=CyAFOah2M2jTsFrZt06aK2gvYVRl3CtsjyOhxwIvj0JqdJG3yXXgICm5yfEBFVXGg\n\tZ6QUes4gcWDRGXOOvDFZ23eo49cxGJjZJNAXqIKuyzjd475Y7RKoyfAMGdn3E0exk+\n\tzfKPN9A5wRInhl8x6sJy3BvMDw6EZ9XDT8fqj4wY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CyAFOah2\"; dkim-atps=neutral","Message-ID":"<6bbefe7a-6fe3-e9e0-e9e4-94c3b86c311e@ideasonboard.com>","Date":"Tue, 31 May 2022 15:50:32 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"paul.elder@ideasonboard.com","References":"<20220527191740.242300-1-umang.jain@ideasonboard.com>\n\t<20220527191740.242300-3-umang.jain@ideasonboard.com>\n\t<20220531074141.GL2630765@pyrite.rasen.tech>","In-Reply-To":"<20220531074141.GL2630765@pyrite.rasen.tech>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context:\n\tExtend FCQueue::get()","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23270,"web_url":"https://patchwork.libcamera.org/comment/23270/","msgid":"<7a1290e1-1a5e-950a-3e3f-a7d3be757c8b@ideasonboard.com>","date":"2022-06-01T06:38:29","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context:\n\tExtend FCQueue::get()","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang,\n\nThanks for the patch !\n\nOn 27/05/2022 21:17, Umang Jain via libcamera-devel wrote:\n> Extend the FCQueue::get() to return a IPAFrameContext for a\n> non-existent frame. The .get() should be able to figure out if a\n> existent frame context is asked for, or to create a new frame context\n> based on the next available position. To keep track of next available\n> position in the queue, use of head and tail pointer are used.\n\nI don't know if it is the easier way for us :-). I like the idea that, \nwhen get() can't find the frame, it creates one, but as we see it later \nin the code, it also introduces a difficulty: what should we do when the \nqueue is full, and we can't create a new one ?\nI think it makes it a \"too-smart\" ring buffer, maybe ?\n\nAFAIK, the frameContext is created when we enter the process() call. A \nsimple approach would be to have a set() when we enter process(), and \nthe algorithms will only call get(). This way set() would be responsible \nof managing the size ?\n\n> \n> We specifically want to have access to the queue through the\n> .get() function hence operator[] is deleted for FCQueue as\n> part of this patch as well.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/ipu3/ipa_context.cpp | 81 +++++++++++++++++++++++++++++++++---\n> src/ipa/ipu3/ipa_context.h | 5 +++\n> src/ipa/ipu3/ipu3.cpp | 4 +-\n> 3 files changed, 83 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index e5010e3f..dcce6b48 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -217,32 +217,96 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> * \\brief Analogue gain multiplier\n> */\n> \n> +/**\n> + * \\class FCQueue\n> + * \\brief A FIFO circular queue holding IPAFrameContext(s)\n> + *\n> + * FCQueue holds all the IPAFrameContext(s) related to frames required\n> + * to be processed by the IPA at a given point. A IPAFrameContext is created\n> + * on the first call FCQueue::get(frame) for that frame. Subsequent calls to\n> + * FCQueue::get() with the same frame number shall return the IPAFrameContext\n> + * previously created until the frame is marked as complete through\n> + * FCQueue::completeFrame(frame).\n> + */\n> +\n> +/**\n> + * \\var FCQueue::head\n> + * \\brief A pointer to the a IPAFrameContext next expected to complete\n> + */\n> +\n> +/**\n> + * \\var FCQueue::tail\n> + * \\brief A pointer to the latest IPAFrameContext created\n> + */\n> +\n> /**\n> * \\brief FCQueue constructor\n> */\n> FCQueue::FCQueue()\n> +\t: head(nullptr), tail(nullptr)\n> {\n> \tclear();\n> }\n> \n> /**\n> - * Retrieve the IPAFrameContext for the frame\n> + * \\brief Retrieve the IPAFrameContext for the frame\n> * \\param[in] frame Frame number for which the IPAFrameContext needs to\n> * retrieved\n> *\n> + * This functions returns the IPAFrameContext for the desired frame. If the\n> + * frame context does not exists in the queue, the next available reference\n> + * of the position in the queue, is returned. It is the responsibility of the\n> + * caller to fill the correct IPAFrameContext parameters of newly returned\n> + * IPAFrameContext.\n> + *\n> * \\return Reference to the IPAFrameContext\n> */\n> IPAFrameContext &FCQueue::get(uint32_t frame)\n> {\n> -\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> +\tif (frame <= tail->frame) {\n> +\t\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> +\t\tif (frame != frameContext.frame)\n> +\t\t\tLOG(IPAIPU3, Warning)\n> +\t\t\t\t<< \"Got wrong frame context for frame \" << frame;\n> +\t\treturn frameContext;\n> +\t} else {\n> +\t\t/*\n> +\t\t * Frame context doesn't exist yet so get the next available\n> +\t\t * position in the queue.\n> +\t\t */\n> +\t\ttail = &this->at((tail->frame + 1) % kMaxFrameContexts);\n> +\t\ttail->frame = frame;\n> \n> -\tif (frame != frameContext.frame) {\n> +\t\t/* Avoid over-queuing of frame contexts */\n> +\t\tif (tail->frame > kMaxFrameContexts &&\n> +\t\t (tail->frame - head->frame) >= kMaxFrameContexts) {\n> +\t\t\tLOG(IPAIPU3, Error) << \"FCQueue is full!\";\n> +\n> +\t\t\t/* \\todo: Determine return reference? or make it fatal? */\n> +\t\t}\n> +\n> +\t\treturn *tail;\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Notifies the FCQueue that a frame has been completed\n> + * \\param[in] frame The completed frame number\n> + */\n> +void FCQueue::completeFrame(uint32_t frame)\n> +{\n> +\tif (head->frame != frame)\n> \t\tLOG(IPAIPU3, Warning)\n> -\t\t\t<< \"Got wrong frame context for frame\" << frame\n> -\t\t\t<< \" or frame context doesn't exist yet\";\n> +\t\t\t<< \"Frame \" << frame << \" completed out-of-sync?\";\n> +\n> +\tif (frame > tail->frame) {\n> +\t\tLOG(IPAIPU3, Error)\n> +\t\t\t<< \"Completing a frame \" << frame\n> +\t\t\t<< \" not present in the queue is disallowed\";\n> +\t\treturn;\n> \t}\n> \n> -\treturn frameContext;\n> +\thead = &this->get(frame + 1);\n> }\n> \n> /**\n> @@ -252,6 +316,11 @@ void FCQueue::clear()\n> {\n> \tIPAFrameContext initFrameContext;\n> \tthis->fill(initFrameContext);\n> +\n> +\t/* Intialise 0th index to frame 0 */\n> +\tthis->at(0).frame = 0;\n> +\ttail = &this->at(0);\n> +\thead = tail;\n> }\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 61454b41..b36ec61d 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -93,9 +93,14 @@ class FCQueue : public std::array\n> {\n> public:\n> \tFCQueue();\n> +\tFCQueue &operator[](const FCQueue &) = delete;\n> \n> \tvoid clear();\n> +\tvoid completeFrame(uint32_t frame);\n> \tIPAFrameContext &get(uint32_t frame);\n> +\n> +\tIPAFrameContext *head;\n> +\tIPAFrameContext *tail;\n\nIf you want it to be public, I think having a tail() and a head() \nfunction would be cleaner. Those pointer should be private with _ suffixes.\n\nJM\n\n> };\n> \n> struct IPAContext {\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index c48d2f62..e09c5d05 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -605,6 +605,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \t */\n> \n> \tmetadataReady.emit(frame, ctrls);\n> +\n> +\tcontext_.frameContexts.completeFrame(frame);\n> }\n> \n> /**\n> @@ -618,7 +620,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> -\tcontext_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> +\tcontext_.frameContexts.get(frame) = { frame, 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 C3A97BD161\n\tfor ;\n\tWed, 1 Jun 2022 06:38:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D64765631;\n\tWed, 1 Jun 2022 08:38:34 +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 79FDF60105\n\tfor ;\n\tWed, 1 Jun 2022 08:38:32 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:2d00:c89c:70b:c9ee] (unknown\n\t[IPv6:2a01:e0a:169:7140:2d00:c89c:70b:c9ee])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ED3736D1;\n\tWed, 1 Jun 2022 08:38:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654065514;\n\tbh=Tqfk91CtC3cti3Xx6cmIg/oHmQY2RvsqVfMDKq5PKyY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=Dj9bNZRNtKDJ0Mcc4h/0nq7SKv78Spm1wXQrjKqeauh6+Wy3NfZwnWXV/D1DKBt8j\n\tXHR5PYtzQXmkIhtm/WQjZ4L9/SUtli7TkQK18e587BPuX5d30VE9a8Y4vTT+bcjwWl\n\tefivMMyLjWX6x2wY14Vn1CjtXNmh276cWDPl5h21FAYYreIe1I5V3RuqWGjTNOoft7\n\t5tQd0rvFZLX27O8B4WcCFW9jD3fAjfm+n/ZTmueuBlPvY8iI/Ultvpn3aUnSu/Hi4O\n\trx1UhavEr0HPhE9STYW7YbIpYGwp9R2HlVp58k5h9jMQR5zGJMB/xJ/JIlR8CQQe7J\n\tpgnpSiwq082Yw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654065512;\n\tbh=Tqfk91CtC3cti3Xx6cmIg/oHmQY2RvsqVfMDKq5PKyY=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Bnz3XxsWRJdrmmNXKTBJRq7qxCfQXHkO0g00pNomRqtEzxsHUW0ZhSuTUL/f6+aMH\n\tiZ2RU/23md6uEx7s4HUaLwb6W7GzVflGjFnDGCojKgNje/jB7Ac/J9nPER3C2hOq2q\n\tNXSy0WnquGZ4Hehr8lAfVvrCNGBsBObB99ILNfI8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Bnz3XxsW\"; dkim-atps=neutral","Message-ID":"<7a1290e1-1a5e-950a-3e3f-a7d3be757c8b@ideasonboard.com>","Date":"Wed, 1 Jun 2022 08:38:29 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220527191740.242300-1-umang.jain@ideasonboard.com>\n\t<20220527191740.242300-3-umang.jain@ideasonboard.com>","In-Reply-To":"<20220527191740.242300-3-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context:\n\tExtend FCQueue::get()","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","From":"Jean-Michel Hautbois via libcamera-devel\n\t","Reply-To":"Jean-Michel Hautbois ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23271,"web_url":"https://patchwork.libcamera.org/comment/23271/","msgid":"<0cc0c9da-0b03-f14c-334c-bfbd259eee7b@ideasonboard.com>","date":"2022-06-01T07:10:19","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context:\n\tExtend FCQueue::get()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn 6/1/22 08:38, Jean-Michel Hautbois wrote:\n> Hi Umang,\n>\n> Thanks for the patch !\n>\n> On 27/05/2022 21:17, Umang Jain via libcamera-devel wrote:\n>> Extend the FCQueue::get() to return a IPAFrameContext for a\n>> non-existent frame. The .get() should be able to figure out if a\n>> existent frame context is asked for, or to create a new frame context\n>> based on the next available position. To keep track of next available\n>> position in the queue, use of head and tail pointer are used.\n>\n> I don't know if it is the easier way for us :-). I like the idea that, \n> when get() can't find the frame, it creates one, but as we see it \n> later in the code, it also introduces a difficulty: what should we do \n> when the queue is full, and we can't create a new one ?\n\n\nYes, but it's an edge case - that we need to handle. It's not like we \nshould drop the idea of ring-buffer overall.\n\n> I think it makes it a \"too-smart\" ring buffer, maybe ?\n>\n> AFAIK, the frameContext is created when we enter the process() call. A \n> simple approach would be to have a set() when we enter process(), and \n> the algorithms will only call get(). This way set() would be \n> responsible of managing the size ?\n\n\nNo, we just cannot assume this - that were it's created\n\nAny algorithm depending on it's \"look-ahead\", shall be free to create a \nframeContext and populate it.\n\n>\n>>\n>> We specifically want to have access to the queue through the\n>> .get() function hence operator[] is deleted for FCQueue as\n>> part of this patch as well.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>>   src/ipa/ipu3/ipa_context.cpp | 81 +++++++++++++++++++++++++++++++++---\n>>   src/ipa/ipu3/ipa_context.h   |  5 +++\n>>   src/ipa/ipu3/ipu3.cpp        |  4 +-\n>>   3 files changed, 83 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>> index e5010e3f..dcce6b48 100644\n>> --- a/src/ipa/ipu3/ipa_context.cpp\n>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>> @@ -217,32 +217,96 @@ IPAFrameContext::IPAFrameContext(uint32_t id, \n>> const ControlList &reqControls)\n>>    * \\brief Analogue gain multiplier\n>>    */\n>>   +/**\n>> + * \\class FCQueue\n>> + * \\brief A FIFO circular queue holding IPAFrameContext(s)\n>> + *\n>> + * FCQueue holds all the IPAFrameContext(s) related to frames required\n>> + * to be processed by the IPA at a given point. A IPAFrameContext is \n>> created\n>> + * on the first call FCQueue::get(frame) for that frame. Subsequent \n>> calls to\n>> + * FCQueue::get() with the same frame number shall return the \n>> IPAFrameContext\n>> + * previously created until the frame is marked as complete through\n>> + * FCQueue::completeFrame(frame).\n>> + */\n>> +\n>> +/**\n>> + * \\var FCQueue::head\n>> + * \\brief A pointer to the a IPAFrameContext next expected to complete\n>> + */\n>> +\n>> +/**\n>> + * \\var FCQueue::tail\n>> + * \\brief A pointer to the latest IPAFrameContext created\n>> + */\n>> +\n>>   /**\n>>    * \\brief FCQueue constructor\n>>    */\n>>   FCQueue::FCQueue()\n>> +    : head(nullptr), tail(nullptr)\n>>   {\n>>       clear();\n>>   }\n>>     /**\n>> - * Retrieve the IPAFrameContext for the frame\n>> + * \\brief Retrieve the IPAFrameContext for the frame\n>>    * \\param[in] frame Frame number for which the IPAFrameContext \n>> needs to\n>>    * retrieved\n>>    *\n>> + * This functions returns the IPAFrameContext for the desired frame. \n>> If the\n>> + * frame context does not exists in the queue, the next available \n>> reference\n>> + * of the position in the queue, is returned. It is the \n>> responsibility of the\n>> + * caller to fill the correct IPAFrameContext parameters of newly \n>> returned\n>> + * IPAFrameContext.\n>> + *\n>>    * \\return Reference to the IPAFrameContext\n>>    */\n>>   IPAFrameContext &FCQueue::get(uint32_t frame)\n>>   {\n>> -    IPAFrameContext &frameContext = this->at(frame % \n>> kMaxFrameContexts);\n>> +    if (frame <= tail->frame) {\n>> +        IPAFrameContext &frameContext = this->at(frame % \n>> kMaxFrameContexts);\n>> +        if (frame != frameContext.frame)\n>> +            LOG(IPAIPU3, Warning)\n>> +                << \"Got wrong frame context for frame \" << frame;\n>> +        return frameContext;\n>> +    } else {\n>> +        /*\n>> +         * Frame context doesn't exist yet so get the next available\n>> +         * position in the queue.\n>> +         */\n>> +        tail = &this->at((tail->frame + 1) % kMaxFrameContexts);\n>> +        tail->frame = frame;\n>>   -    if (frame != frameContext.frame) {\n>> +        /* Avoid over-queuing of frame contexts */\n>> +        if (tail->frame > kMaxFrameContexts &&\n>> +            (tail->frame - head->frame) >= kMaxFrameContexts) {\n>> +            LOG(IPAIPU3, Error) << \"FCQueue is full!\";\n>> +\n>> +            /* \\todo: Determine return reference? or make it fatal? */\n>> +        }\n>> +\n>> +        return *tail;\n>> +    }\n>> +}\n>> +\n>> +/**\n>> + * \\brief Notifies the FCQueue that a frame has been completed\n>> + * \\param[in] frame The completed frame number\n>> + */\n>> +void FCQueue::completeFrame(uint32_t frame)\n>> +{\n>> +    if (head->frame != frame)\n>>           LOG(IPAIPU3, Warning)\n>> -            << \"Got wrong frame context for frame\" << frame\n>> -            << \" or frame context doesn't exist yet\";\n>> +            << \"Frame \" << frame << \" completed out-of-sync?\";\n>> +\n>> +    if (frame > tail->frame) {\n>> +        LOG(IPAIPU3, Error)\n>> +            << \"Completing a frame \" << frame\n>> +            << \" not present in the queue is disallowed\";\n>> +        return;\n>>       }\n>>   -    return frameContext;\n>> +    head = &this->get(frame + 1);\n>>   }\n>>     /**\n>> @@ -252,6 +316,11 @@ void FCQueue::clear()\n>>   {\n>>       IPAFrameContext initFrameContext;\n>>       this->fill(initFrameContext);\n>> +\n>> +    /* Intialise 0th index to frame 0 */\n>> +    this->at(0).frame = 0;\n>> +    tail = &this->at(0);\n>> +    head = tail;\n>>   }\n>>     } /* namespace ipa::ipu3 */\n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index 61454b41..b36ec61d 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -93,9 +93,14 @@ class FCQueue : public std::array> kMaxFrameContexts>\n>>   {\n>>   public:\n>>       FCQueue();\n>> +    FCQueue &operator[](const FCQueue &) = delete;\n>>         void clear();\n>> +    void completeFrame(uint32_t frame);\n>>       IPAFrameContext &get(uint32_t frame);\n>> +\n>> +    IPAFrameContext *head;\n>> +    IPAFrameContext *tail;\n>\n> If you want it to be public, I think having a tail() and a head() \n> function would be cleaner. Those pointer should be private with _ \n> suffixes.\n\n\nDon't think these pointers and/or tail() head(), are needed to be \npublic. Will make it private.\n\nDown the line, we can have helpers like, if necessary:\n\nFCQueue::isFull()  and FCQueue::isEmpty()  which will operate on head \nand tail.\n\n\n>\n> JM\n>\n>>   };\n>>     struct IPAContext {\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index c48d2f62..e09c5d05 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -605,6 +605,8 @@ void IPAIPU3::processStatsBuffer(const uint32_t \n>> frame,\n>>        */\n>>         metadataReady.emit(frame, ctrls);\n>> +\n>> +    context_.frameContexts.completeFrame(frame);\n>>   }\n>>     /**\n>> @@ -618,7 +620,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t \n>> frame,\n>>   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList \n>> &controls)\n>>   {\n>>       /* \\todo Start processing for 'frame' based on 'controls'. */\n>> -    context_.frameContexts[frame % kMaxFrameContexts] = { frame, \n>> controls };\n>> +    context_.frameContexts.get(frame) = { frame, controls };\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 A75F1BD160\n\tfor ;\n\tWed, 1 Jun 2022 07:10:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E07AE65635;\n\tWed, 1 Jun 2022 09:10:24 +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 403E360414\n\tfor ;\n\tWed, 1 Jun 2022 09:10:23 +0200 (CEST)","from [192.168.1.68] (235.red-81-36-186.dynamicip.rima-tde.net\n\t[81.36.186.235])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AA9996D1;\n\tWed, 1 Jun 2022 09:10:22 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654067424;\n\tbh=rCUwgc94o/qShJV1rYGP6+Hzgn3LzArOHg+SVA/vw78=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=q6iaLaFWYHs07hcBWvB9kZi7Bg0OE97UMEb79VF7DGo820RDvFKH5GfYCwTK1l6iA\n\tMtX4nboc5SR70vT9myAGqtE1LyejML1wM/nV8s4ZW7QXHvP4qZbvGMQyk2Hh1vEnDv\n\t5EGVre/mOhwZJcy6MOYmqFjNK4WMEBlkLXdIpQ7RObP3ho1QdofNt6xsJwymK1n113\n\tTAH8PUMvWQD4gWv3BDA0+prtD/FOWXcAZ7NBvqyu9LhIoqD6zhwCYEcgeUMPJMbqQ2\n\tNDEn8+UhsnaeYtlYwxk6njSHQVOsKK5vPEukKNdOVM5InU3/8t63wCB/iDKySZT5wb\n\ttnNW/tkmpgqwQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654067422;\n\tbh=rCUwgc94o/qShJV1rYGP6+Hzgn3LzArOHg+SVA/vw78=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Ac0Z93BD3yqFZbHIZNU01TkwiIoCtGimyjwDsxIFQWji2Lq5tcka0DeZfCIFTRL55\n\tCpUstuqLHQA0h55SK1GGSYxeNNghftdhozD5XU7Uw7WAT9Mzd3qTvR5Th5OeBPtvWq\n\tjjweeILfsiK27n6tLhwKbIESE0q9HGBtRijjih8E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Ac0Z93BD\"; dkim-atps=neutral","Message-ID":"<0cc0c9da-0b03-f14c-334c-bfbd259eee7b@ideasonboard.com>","Date":"Wed, 1 Jun 2022 09:10:19 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220527191740.242300-1-umang.jain@ideasonboard.com>\n\t<20220527191740.242300-3-umang.jain@ideasonboard.com>\n\t<7a1290e1-1a5e-950a-3e3f-a7d3be757c8b@ideasonboard.com>","In-Reply-To":"<7a1290e1-1a5e-950a-3e3f-a7d3be757c8b@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] ipa: ipu3: ipa_context:\n\tExtend FCQueue::get()","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]