[{"id":23355,"web_url":"https://patchwork.libcamera.org/comment/23355/","msgid":"<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>","date":"2022-06-08T13:56:10","subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::get()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Fri, Jun 03, 2022 at 03:22:57PM +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>\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 | 95 +++++++++++++++++++++++++++++++++---\n> src/ipa/ipu3/ipa_context.h | 9 ++++\n> src/ipa/ipu3/ipu3.cpp | 4 +-\n> 3 files changed, 100 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 9f95a61c..2438d68d 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -222,13 +222,33 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\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.\n> + * to be processed by the IPA at a given point. An 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> + * \\var FCQueue::isFull_\n> + * \\brief Flag set when the FCQueue is full\n> */\n>\n> /**\n> * \\brief FCQueue constructor\n> */\n> FCQueue::FCQueue()\n> +\t: head_(nullptr), tail_(nullptr), isFull_(false)\n> {\n> \tclear();\n> }\n> @@ -238,21 +258,75 @@ FCQueue::FCQueue()\n> * \\param[in] frame Frame number for which the IPAFrameContext needs to\n> * retrieved\n> *\n> - * \\return Pointer to the IPAFrameContext\n> + * This function returns the IPAFrameContext for the desired frame. If the\n> + * frame context does not exist in the queue, the next available slot in the\n> + * queue is returned. It is the responsibility of the caller to fill the correct\n> + * IPAFrameContext parameters of newly returned IPAFrameContext.\n> + *\n> + * \\return Pointer to the IPAFrameContext or nullptr if frame context couldn't\n> + * be created\n> */\n> IPAFrameContext *FCQueue::get(uint32_t frame)\n> {\n> -\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> +\tif (frame <= tail_->frame) {\n\nHow does this work ? The first get() you call after construction has tail_\n== nullptr. How come this doesn't segfault ? Is it because there's a\ncall to clear() that initializes the pointers before usage ?\nShouldn't the constructor take care of creating a usable object\nwithout requiring clear() to be called ?\n\nAlso, are we sure using the frame number in tail_ and head_ is the best\nway to ensure that we actually track where we are in the queue ?\n\nI have a few worries:\n\n1) are frame numbers always starting from 0 ?\n\n2) frame numbers are monotonically increasing, but can have gaps.\n When you create a new frame context you increase by one to get the\n next slot, but when you get an existing frame you compute the index\n by doing frame % kMaxFrameContexts\n\n IPAFrameContext *FCQueue::get(uint32_t frame) {\n\n if (frame <= tail_->frame)\n /* GET */\n IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n else\n /* PUSH */\n tail_ = &this->at((tail_->frame + 1) % kMaxFrameContexts);\n \t\ttail_->frame = frame;\n\n Isn't there a risk you get the wrong frame ?\n\n (also being this a LIFO, isn't the head the most recent and the\n tail the oldest entry ? you seem to advance tail when you push a\n new frame)\n\n2 ) tail_ gets initialized with an empty IPAFrameContext whose constructor\n is\n IPAFrameContext::IPAFrameContext() = default;\n\n hence its frame id is not intialized, or are POD types default\n initialized in C++ ?\n\nAnyway, isn't it simpler to use plain counters for head and tail instead\nof pointers to the contained objects ? (This would maybe make\ncomplicated to generalize the libcamera::RingBuffer implementation\nmaybe), but head_ and tail_ mainly serve for two purpose:\n- underflow detection (trying to complete a frame not yet queued)\n- overflow detection (trying to queue a frame overwrites a\n not-yet-consumed one)\n\nCan't we have head and tail simply follow the latest frame number that\nhave been queued and the lastest frame number that has been consumed\nrespectively ?\n\nCollision detection will simply be\n- undeflow: tail + 1 == head\n- overflow (queue frame): head - tail == array size\n\nThe actual position on the array can always be computed as (frame %\nkMaxFrameContexts)\n\nThis doesn't however work well with gaps, as one frame gap means we're\nactually not using one slot, so a little space is wasted. Ofc as the\nnumber of gaps approaches the number of available slots, the risk of\noverflow increases. But gaps should be an exceptional events and with\nlarge enough buffers this shouldn't be a problem ?\n\nAlso I wonder if a push/get interface wouldn't be simpler, with new\nreuests being pushed() and frames consumed with get(), but that's more\nan implementation detail maybe..\n\nIPA components cannot have tests right ? It would be nice to have a\nunit test for this component to make sure it behaves as intended\ninstead of having to debug it \"live\"\n\nsorry, a lot of questions actually..\n\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> +\n> +\t\treturn &frameContext;\n> +\t} else {\n> +\t\tif (isFull_) {\n> +\t\t\tLOG(IPAIPU3, Warning)\n> +\t\t\t\t<< \"Cannot create frame context for frame \" << frame\n> +\t\t\t\t<< \" since FCQueue is full\";\n> +\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\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> +\t\t/* Check if the queue is full to avoid over-queueing later */\n> +\t\tif (tail_->frame - head_->frame >= kMaxFrameContexts - 1)\n> +\t\t\tisFull_ = true;\n>\n> -\tif (frame != frameContext.frame) {\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> +\tif (isFull_)\n> +\t\tisFull_ = false;\n> }\n>\n> +/**\n> + * \\fn FCQueue::isFull()\n> + * \\brief Checks whether the frame context queue is full\n> + */\n> +\n> /**\n> * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n> */\n> @@ -260,6 +334,13 @@ void FCQueue::clear()\n> {\n> \tIPAFrameContext initFrameContext;\n> \tthis->fill(initFrameContext);\n> +\n> +\tisFull_ = false;\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 56d281f6..475855da 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -93,9 +93,18 @@ 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> +\tbool isFull() { return isFull_; }\n> +\n> +private:\n> +\tIPAFrameContext *head_;\n> +\tIPAFrameContext *tail_;\n> +\n> +\tbool isFull_;\n> };\n>\n> struct IPAContext {\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 0843d882..1d6ee515 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> +\t*context_.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 123E9BD160\n\tfor ;\n\tWed, 8 Jun 2022 13:56:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B10965637;\n\tWed, 8 Jun 2022 15:56:15 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E5C4A65632\n\tfor ;\n\tWed, 8 Jun 2022 15:56:13 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id A0A95100013;\n\tWed, 8 Jun 2022 13:56:12 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654696575;\n\tbh=Ee3GZbVIeYPaUfTpbNMlPTOhQtyV9DZJyZLOoh+Raas=;\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=ndOu07sz72GVwaY+WNEFVTkQJ617NwU0h45RBZrFhGDC3dAfRu06gPLKXd/tjyL4J\n\todalTtgZtizDcz6TmFcB6hlIj39YW+mLIPTWlllTmQffs6qn+JAWTzqqxw+g9jbkqE\n\td9GYW8R/IkACzasBEe4NmDd0gU0FXx8nhLudCBFUdkMF/ZYWjScSkk5k9mFxW/MdwE\n\tWyogTvh3tGMOKOe2C11xFsu0Nqe9XQldNvkzknlQ3bFAocL0qmt7eKMnMR7D3i6X1f\n\tZ/oAylszZTqKjpgJ2d74mEE2VqXJDTdZrtHJHYLM6ftySAMzewAQCzI9hQT69lpNW2\n\tdv5AYU6m1RqZQ==","Date":"Wed, 8 Jun 2022 15:56:10 +0200","To":"Umang Jain ","Message-ID":"<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220603132259.188845-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23364,"web_url":"https://patchwork.libcamera.org/comment/23364/","msgid":"<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>","date":"2022-06-08T23:09:28","subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::get()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for the review.\n\nOn 6/8/22 15:56, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Fri, Jun 03, 2022 at 03:22:57PM +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>>\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 | 95 +++++++++++++++++++++++++++++++++---\n>> src/ipa/ipu3/ipa_context.h | 9 ++++\n>> src/ipa/ipu3/ipu3.cpp | 4 +-\n>> 3 files changed, 100 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>> index 9f95a61c..2438d68d 100644\n>> --- a/src/ipa/ipu3/ipa_context.cpp\n>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>> @@ -222,13 +222,33 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\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.\n>> + * to be processed by the IPA at a given point. An 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>> + * \\var FCQueue::isFull_\n>> + * \\brief Flag set when the FCQueue is full\n>> */\n>>\n>> /**\n>> * \\brief FCQueue constructor\n>> */\n>> FCQueue::FCQueue()\n>> +\t: head_(nullptr), tail_(nullptr), isFull_(false)\n>> {\n>> \tclear();\n>> }\n>> @@ -238,21 +258,75 @@ FCQueue::FCQueue()\n>> * \\param[in] frame Frame number for which the IPAFrameContext needs to\n>> * retrieved\n>> *\n>> - * \\return Pointer to the IPAFrameContext\n>> + * This function returns the IPAFrameContext for the desired frame. If the\n>> + * frame context does not exist in the queue, the next available slot in the\n>> + * queue is returned. It is the responsibility of the caller to fill the correct\n>> + * IPAFrameContext parameters of newly returned IPAFrameContext.\n>> + *\n>> + * \\return Pointer to the IPAFrameContext or nullptr if frame context couldn't\n>> + * be created\n>> */\n>> IPAFrameContext *FCQueue::get(uint32_t frame)\n>> {\n>> -\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>> +\tif (frame <= tail_->frame) {\n> How does this work ? The first get() you call after construction has tail_\n> == nullptr. How come this doesn't segfault ? Is it because there's a\n> call to clear() that initializes the pointers before usage ?\n> Shouldn't the constructor take care of creating a usable object\n> without requiring clear() to be called ?\n\n\nYes, bad naming. I think reset() would be more appropriate? The \nconstructor is\nresponsible yes (hence it calls clear() in the first place) so it was \nthe matter of\ncode-deduplication. We can discuss the naming conventions but tail_ \nshouldn't\nbe a nullptr before any .get() calls. So I do want proper initialization \nplus, a\nclear() or reset() to clear the ring buffer and resetting the tail_, \nhead_ etc.\n\n>\n> Also, are we sure using the frame number in tail_ and head_ is the best\n> way to ensure that we actually track where we are in the queue ?\n>\n> I have a few worries:\n>\n> 1) are frame numbers always starting from 0 ?\n\n\nIt should be, no?\n\n>\n> 2) frame numbers are monotonically increasing, but can have gaps.\n> When you create a new frame context you increase by one to get the\n> next slot, but when you get an existing frame you compute the index\n> by doing frame % kMaxFrameContexts\n>\n> IPAFrameContext *FCQueue::get(uint32_t frame) {\n>\n> if (frame <= tail_->frame)\n> /* GET */\n> IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> else\n> /* PUSH */\n> tail_ = &this->at((tail_->frame + 1) % kMaxFrameContexts);\n> \t\ttail_->frame = frame;\n>\n> Isn't there a risk you get the wrong frame ?\n\n\nYes, I've realized I have made an error here after posting to the list.\nI put the IPAFrameContext on the next slot immediate to tail_ , but that's\nnot correct. It's only correct if we ensure there are not any gaps \n(majority of the\ndevelopment has been done by assuming that there will not be gaps for now).\n\nGaps / Frame drops is yet another beast to handle. I guess I am keeping \nit separate\nfrom the \"per-frame controls\" planning for now. I discussed the same \nwith Kieran\nthe other day - that the frame-drop handling and per-frame controls need \nto kept\nseparate for progress. Otherwise both half-baked designs, trying to \nsatisfying/handle\neach other arbitrarily  might create chicken-and-egg problems.\n\n>\n> (also being this a LIFO, isn't the head the most recent and the\n> tail the oldest entry ? you seem to advance tail when you push a\n> new frame)\n\n\nNo, head_ represents the oldest entry and tail_ represents the latest \nIPAFrameContext\nthat got created (refer documentation in this patch).\n\n>\n> 2 ) tail_ gets initialized with an empty IPAFrameContext whose constructor\n> is\n> IPAFrameContext::IPAFrameContext() = default;\n>\n> hence its frame id is not intialized, or are POD types default\n> initialized in C++ ?\n\n\nWe manually give it a value of '0' in clear(). It should work out from \nthere.\nThe .get() calls will over-write the frame number while creating the \nIPAFrameContext(s),\nso I don't think it will be a problem as such..\n\n>\n> Anyway, isn't it simpler to use plain counters for head and tail instead\n> of pointers to the contained objects ? (This would maybe make\n> complicated to generalize the libcamera::RingBuffer implementation\n> maybe), but head_ and tail_ mainly serve for two purpose:\n> - underflow detection (trying to complete a frame not yet queued)\n> - overflow detection (trying to queue a frame overwrites a\n> not-yet-consumed one)\n\n\nWell, the main point for having these pointers is to know if a \nIPAFrameContext exists in the first\nplace or not. The underflow/overflow checks comes later (... and we need \nhave to a\nadditional head_ for that, otherwise simply a tail_ would have been \nsufficient for the former)\n\nI agree with you that plain counters would be sufficient (and it doesn't \nreally make a difference\nto me, either you store pointers OR frame numbers of oldest's and \nlatest's). It is just storing\na unique handles somehow.\n\n>\n> Can't we have head and tail simply follow the latest frame number that\n> have been queued and the lastest frame number that has been consumed\n> respectively ?\n>\n> Collision detection will simply be\n> - undeflow: tail + 1 == head\n\n\nrather should be head + 1 == tail (according to how tail and head are \nused in this patch)\n\n> - overflow (queue frame): head - tail == array size\n>\n> The actual position on the array can always be computed as (frame %\n> kMaxFrameContexts)\n>\n> This doesn't however work well with gaps, as one frame gap means we're\n> actually not using one slot, so a little space is wasted. Ofc as the\n> number of gaps approaches the number of available slots, the risk of\n> overflow increases. But gaps should be an exceptional events and with\n> large enough buffers this shouldn't be a problem ?\n\n\nTo be honest, I don't think storing IPAFrameContext pointers vs frame \nnumbers\nshould be a major concern. I say this because intrinsically everything \nrevolves\naround the frame number in both cases.\n\nGoing forward (and it's only my educated guess), storing head and tail frame\nnumbers will get limiting a bit. I need to see how Kieran's work on \nmerging/retaining\ncontrols is going on. The idea is controls from latest IPAFrameContext \ngets retained\ninto next-created IPAFrameContext(s) and so on... If you think about it, \nthe tail_->controls\nwill get copied into the next IPAFrameContext while being created. In \ncases like this,\nhaving IPAFrameContexts pointers are useful in my opinion.\n\n>\n> Also I wonder if a push/get interface wouldn't be simpler, with new\n> reuests being pushed() and frames consumed with get(), but that's more\n> an implementation detail maybe..\n\n\nI do wondered that as well, but in my opinion having a push() + get() \ninterface also means,\neach algorithm has to do various checks on their part. For e.g.\n\nAlgorithm1:\n\n     .get(X)  <--- Failed\n     .push(XFrameContext)\n     .get(X) <---- Success and carry on\n\nAlgorithm2:\n\n     .get(X) <-- Success because Algorithm1 created XFrameContext\n     // but you still need to write failure path here containing \n.push(XFrameContext)\n\n.. and so on.\n\nAlso, various Algorithm might need to create different frame's \nIPAFrameContext in a single run,\ndepending on their latencies.\n\nSo I think we are unnecessarily outsourcing the responsibility of \n\"pushing\" the IPAFrameContext\nto the algorithms. All this can be encapsulated in the .get(), no? I \nthought we all agreed on\nthe .get() design, so hence this direction.\n\n>\n> IPA components cannot have tests right ? It would be nice to have a\n> unit test for this component to make sure it behaves as intended\n> instead of having to debug it \"live\"\n\n\nI see that you have mentioned a desire to make a generic \nlibcamera::RingBuffer\nclass implementation. While I do agree on the point of tests, but I am \nnot looking\nat this angle just yet. It feels to me a bit early to do so because \nthings can be very\nspecific w.r.t IPAs. Even if we do it \"generically\" right now, only IPU3 \nwould be the\nonly user I think. There are other similar parts that we want to provide \na generic\nimplementation to all the IPAs for e.g. IPAContext, IPAFrameContexts and \nso on,\nso probably once we have a good usage across multiple IPAs, we would be \nin a\nbetter position to write a generic ring buffer implementation then and adapt\nthe IPAs on top of it?\n\n>\n> sorry, a lot of questions actually..\n\n\nNo issues ;-)\n\n>\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>> +\n>> +\t\treturn &frameContext;\n>> +\t} else {\n>> +\t\tif (isFull_) {\n>> +\t\t\tLOG(IPAIPU3, Warning)\n>> +\t\t\t\t<< \"Cannot create frame context for frame \" << frame\n>> +\t\t\t\t<< \" since FCQueue is full\";\n>> +\n>> +\t\t\treturn nullptr;\n>> +\t\t}\n>> +\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>> +\t\t/* Check if the queue is full to avoid over-queueing later */\n>> +\t\tif (tail_->frame - head_->frame >= kMaxFrameContexts - 1)\n>> +\t\t\tisFull_ = true;\n>>\n>> -\tif (frame != frameContext.frame) {\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>> +\tif (isFull_)\n>> +\t\tisFull_ = false;\n>> }\n>>\n>> +/**\n>> + * \\fn FCQueue::isFull()\n>> + * \\brief Checks whether the frame context queue is full\n>> + */\n>> +\n>> /**\n>> * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n>> */\n>> @@ -260,6 +334,13 @@ void FCQueue::clear()\n>> {\n>> \tIPAFrameContext initFrameContext;\n>> \tthis->fill(initFrameContext);\n>> +\n>> +\tisFull_ = false;\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 56d281f6..475855da 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -93,9 +93,18 @@ 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>> +\tbool isFull() { return isFull_; }\n>> +\n>> +private:\n>> +\tIPAFrameContext *head_;\n>> +\tIPAFrameContext *tail_;\n>> +\n>> +\tbool isFull_;\n>> };\n>>\n>> struct IPAContext {\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 0843d882..1d6ee515 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>> +\t*context_.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 7EE71BD160\n\tfor ;\n\tWed, 8 Jun 2022 23:09:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DD4665635;\n\tThu, 9 Jun 2022 01:09:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C0D1633A5\n\tfor ;\n\tThu, 9 Jun 2022 01:09:32 +0200 (CEST)","from [10.1.0.226] (227.red-83-48-39.staticip.rima-tde.net\n\t[83.48.39.227])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9793A2E4;\n\tThu, 9 Jun 2022 01:09:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654729773;\n\tbh=AlJHlLc+7QsIgc468c3f/G4ti7wOibZN0G4h5fjLeFE=;\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=LIxAdOg51+uMWzY8bR2y1rscyC1WddKnO3aIWhEpSxmWzyrjeFFSs1I4wlXbh0GAV\n\tdc1UYSTpMTLLKajWaVItNxX6i7P5nboznyqFPpfiEKtJzf7ZbRJlXiDumkQgrxpzjB\n\tO57M0YL0SfWY94PRekoGM+fRArk9a+9ZMBYGlfBuRi0cntrg9bNX0l6oLUDZ86iJ8R\n\tXMrRrbuUK3t9rzR6KXespsjiJv4Spic2qx84O/GTQg3QbdwW25sT4PhnvUkfoCG7Io\n\tTHKDj5Fo5aM5mwXMOgbmZE2xhtBZzw2FZtiTGNtalMATItgGi1aTidU0T9LeG8uSIX\n\tlKRAJiaxpsAVw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654729771;\n\tbh=AlJHlLc+7QsIgc468c3f/G4ti7wOibZN0G4h5fjLeFE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Lp8eVAHqoJNGKmMQkouMzTqlgkgIYF6wir/DZtAz6mboXtLjK6tw4mcfuFfFHOn1w\n\tNy7Xf4HUktseu8Z5+lnnlEbQuaghz/dRz6EsBbRj8MMwY1koeBxKkYsEy4vWpdBSU2\n\tSZq2x6zVqTwm+977nms/bVLLJVnb++siVgbHDUYA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Lp8eVAHq\"; dkim-atps=neutral","Message-ID":"<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>","Date":"Thu, 9 Jun 2022 01:09:28 +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":"Jacopo Mondi ","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-3-umang.jain@ideasonboard.com>\n\t<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>","In-Reply-To":"<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::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":23372,"web_url":"https://patchwork.libcamera.org/comment/23372/","msgid":"<20220609162054.enj4kswtjtomm5po@uno.localdomain>","date":"2022-06-09T16:20:54","subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::get()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Thu, Jun 09, 2022 at 01:09:28AM +0200, Umang Jain wrote:\n> Hi Jacopo,\n>\n> Thanks for the review.\n>\n> On 6/8/22 15:56, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Fri, Jun 03, 2022 at 03:22:57PM +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> > >\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 | 95 +++++++++++++++++++++++++++++++++---\n> > > src/ipa/ipu3/ipa_context.h | 9 ++++\n> > > src/ipa/ipu3/ipu3.cpp | 4 +-\n> > > 3 files changed, 100 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > > index 9f95a61c..2438d68d 100644\n> > > --- a/src/ipa/ipu3/ipa_context.cpp\n> > > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > > @@ -222,13 +222,33 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\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.\n> > > + * to be processed by the IPA at a given point. An 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> > > + * \\var FCQueue::isFull_\n> > > + * \\brief Flag set when the FCQueue is full\n> > > */\n> > >\n> > > /**\n> > > * \\brief FCQueue constructor\n> > > */\n> > > FCQueue::FCQueue()\n> > > +\t: head_(nullptr), tail_(nullptr), isFull_(false)\n> > > {\n> > > \tclear();\n> > > }\n> > > @@ -238,21 +258,75 @@ FCQueue::FCQueue()\n> > > * \\param[in] frame Frame number for which the IPAFrameContext needs to\n> > > * retrieved\n> > > *\n> > > - * \\return Pointer to the IPAFrameContext\n> > > + * This function returns the IPAFrameContext for the desired frame. If the\n> > > + * frame context does not exist in the queue, the next available slot in the\n> > > + * queue is returned. It is the responsibility of the caller to fill the correct\n> > > + * IPAFrameContext parameters of newly returned IPAFrameContext.\n> > > + *\n> > > + * \\return Pointer to the IPAFrameContext or nullptr if frame context couldn't\n> > > + * be created\n> > > */\n> > > IPAFrameContext *FCQueue::get(uint32_t frame)\n> > > {\n> > > -\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > +\tif (frame <= tail_->frame) {\n> > How does this work ? The first get() you call after construction has tail_\n> > == nullptr. How come this doesn't segfault ? Is it because there's a\n> > call to clear() that initializes the pointers before usage ?\n> > Shouldn't the constructor take care of creating a usable object\n> > without requiring clear() to be called ?\n>\n>\n> Yes, bad naming. I think reset() would be more appropriate? The constructor\n> is\n> responsible yes (hence it calls clear() in the first place) so it was the\n\nI missed the call to clear() in the constructor and I thought this\nworked because of the clear() call in IPA::configure(). Sorry, this is\nok I guess.\n\n> matter of\n> code-deduplication. We can discuss the naming conventions but tail_\n> shouldn't\n> be a nullptr before any .get() calls. So I do want proper initialization\n> plus, a\n> clear() or reset() to clear the ring buffer and resetting the tail_, head_\n> etc.\n>\n> >\n> > Also, are we sure using the frame number in tail_ and head_ is the best\n> > way to ensure that we actually track where we are in the queue ?\n> >\n> > I have a few worries:\n> >\n> > 1) are frame numbers always starting from 0 ?\n>\n>\n> It should be, no?\n>\n\nI would say that it depends on how the kernel driver counts the frame\nnumbers. If the MIPI CSI-2 frame number is used it will count from 1\nand increment per-virtual channel. If the driver maintains an internal\ncounter it should be reset at every start_stream/stop_stream session,\nbut I don't this there are guarantees if not looking at the driver and\nmaking sure it does the right thing ?\n\nHowever I now also recall Kieran had a patch on his FrameSequence\nseries to restart counting from 0 the frame sequence number in\nlibcamera, do we assume they will always start from 0 because of this?\n\n> >\n> > 2) frame numbers are monotonically increasing, but can have gaps.\n> > When you create a new frame context you increase by one to get the\n> > next slot, but when you get an existing frame you compute the index\n> > by doing frame % kMaxFrameContexts\n> >\n> > IPAFrameContext *FCQueue::get(uint32_t frame) {\n> >\n> > if (frame <= tail_->frame)\n> > /* GET */\n> > IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > else\n> > /* PUSH */\n> > tail_ = &this->at((tail_->frame + 1) % kMaxFrameContexts);\n> > \t\ttail_->frame = frame;\n> >\n> > Isn't there a risk you get the wrong frame ?\n>\n>\n> Yes, I've realized I have made an error here after posting to the list.\n> I put the IPAFrameContext on the next slot immediate to tail_ , but that's\n> not correct. It's only correct if we ensure there are not any gaps (majority\n> of the\n> development has been done by assuming that there will not be gaps for now).\n>\n> Gaps / Frame drops is yet another beast to handle. I guess I am keeping it\n> separate\n> from the \"per-frame controls\" planning for now. I discussed the same with\n> Kieran\n> the other day - that the frame-drop handling and per-frame controls need to\n> kept\n> separate for progress. Otherwise both half-baked designs, trying to\n> satisfying/handle\n> each other arbitrarily  might create chicken-and-egg problems.\n\nFrame drops had been biting us hard enough already. I think we should\ntake them into account from the beginning in order to avoid having to\nre-think how to handle them later..\n\nThat's why I think we need to define how mapping the frame number to\nthe intended slot and do so uniquely in both \"get\" and \"push\". The\ncurrent \"frame % kMaxFrameContexts\" is enough (it will only cause\nproblems if the number of dropped frames in a kMaxFrameContexts period\nis larger than the queue depth I think).\n>\n> >\n> > (also being this a LIFO, isn't the head the most recent and the\n\nSorry, I clearly meant FIFO\n\n> > tail the oldest entry ? you seem to advance tail when you push a\n> > new frame)\n>\n>\n> No, head_ represents the oldest entry and tail_ represents the latest\n> IPAFrameContext\n> that got created (refer documentation in this patch).\n>\n\nMine was mostly a comment on the general concept, not as much as the\ndocumentation of what you've done :) My thought was that in a stack\n(LIFO) you pop the head and in a queue (FIFO) you pop the tail (and\nconsequentlly adavance the head when you push a new context).\n\nBasically this:\nhttps://softwareengineering.stackexchange.com/questions/144477/on-a-queue-which-end-is-the-head\n\nHowever STL seems to use \"front\" and \"back\" for queues [2] as\nsynonymous for your head and tail, so I guess it's fine the way you\nhave it (you could use \"front\" and \"back\" to stay closer to STL)\n\n[2] https://en.cppreference.com/w/cpp/container/queue\n\n> >\n> > 2 ) tail_ gets initialized with an empty IPAFrameContext whose constructor\n> > is\n> > IPAFrameContext::IPAFrameContext() = default;\n> >\n> > hence its frame id is not intialized, or are POD types default\n> > initialized in C++ ?\n>\n>\n> We manually give it a value of '0' in clear(). It should work out from\n> there.\n\nRight. Again careful that if frame numbers are numbered using the\nCSI-2 frame number, it will count from 1.\n\n> The .get() calls will over-write the frame number while creating the\n> IPAFrameContext(s),\n> so I don't think it will be a problem as such..\n>\n> >\n> > Anyway, isn't it simpler to use plain counters for head and tail instead\n> > of pointers to the contained objects ? (This would maybe make\n> > complicated to generalize the libcamera::RingBuffer implementation\n> > maybe), but head_ and tail_ mainly serve for two purpose:\n> > - underflow detection (trying to complete a frame not yet queued)\n> > - overflow detection (trying to queue a frame overwrites a\n> > not-yet-consumed one)\n>\n>\n> Well, the main point for having these pointers is to know if a\n> IPAFrameContext exists in the first\n> place or not. The underflow/overflow checks comes later (... and we need\n\nWell, you have this to find out if a frame context is the \"right\" one\n\n IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n\tif (frame != frameContext.frame)\n\t\tLOG(IPAIPU3, Warning)\n\t\t\t<< \"Got wrong frame context for frame \" << frame;\n\nBut the distinction between a \"push\" and a \"get\" is just\n\n\tif (frame <= tail_->frame)\n\nwhich could be very well realized with integers.\n\nAnyway, just a suggestion to have the implementation possibly simpler\n\n\n> have to a\n> additional head_ for that, otherwise simply a tail_ would have been\n> sufficient for the former)\n>\n> I agree with you that plain counters would be sufficient (and it doesn't\n> really make a difference\n> to me, either you store pointers OR frame numbers of oldest's and latest's).\n> It is just storing\n> a unique handles somehow.\n>\n> >\n> > Can't we have head and tail simply follow the latest frame number that\n> > have been queued and the lastest frame number that has been consumed\n> > respectively ?\n> >\n> > Collision detection will simply be\n> > - undeflow: tail + 1 == head\n>\n>\n> rather should be head + 1 == tail (according to how tail and head are used\n> in this patch)\n\nYeah, I was using the notion I had in my head. Must be biased by the\ndog metaphore in the link above :)\n\n>\n> > - overflow (queue frame): head - tail == array size\n> >\n> > The actual position on the array can always be computed as (frame %\n> > kMaxFrameContexts)\n> >\n> > This doesn't however work well with gaps, as one frame gap means we're\n> > actually not using one slot, so a little space is wasted. Ofc as the\n> > number of gaps approaches the number of available slots, the risk of\n> > overflow increases. But gaps should be an exceptional events and with\n> > large enough buffers this shouldn't be a problem ?\n>\n>\n> To be honest, I don't think storing IPAFrameContext pointers vs frame\n> numbers\n> should be a major concern. I say this because intrinsically everything\n> revolves\n> around the frame number in both cases.\n>\n> Going forward (and it's only my educated guess), storing head and tail frame\n> numbers will get limiting a bit. I need to see how Kieran's work on\n> merging/retaining\n> controls is going on. The idea is controls from latest IPAFrameContext gets\n> retained\n> into next-created IPAFrameContext(s) and so on... If you think about it, the\n> tail_->controls\n> will get copied into the next IPAFrameContext while being created. In cases\n> like this,\n> having IPAFrameContexts pointers are useful in my opinion.\n>\n\nI don't think that's an issue as head and tail will simply allow you\nget the context and from there you can do whatever you want.\nSimilarly, from the frame context you can get the frame number, so\nyes, whatever you prefer for now\n\n> >\n> > Also I wonder if a push/get interface wouldn't be simpler, with new\n> > reuests being pushed() and frames consumed with get(), but that's more\n> > an implementation detail maybe..\n>\n>\n> I do wondered that as well, but in my opinion having a push() + get()\n> interface also means,\n> each algorithm has to do various checks on their part. For e.g.\n>\n> Algorithm1:\n>\n>     .get(X)  <--- Failed\n>     .push(XFrameContext)\n>     .get(X) <---- Success and carry on\n>\n> Algorithm2:\n>\n>     .get(X) <-- Success because Algorithm1 created XFrameContext\n>     // but you still need to write failure path here containing\n> .push(XFrameContext)\n>\n> .. and so on.\n>\n> Also, various Algorithm might need to create different frame's\n> IPAFrameContext in a single run,\n> depending on their latencies.\n>\n> So I think we are unnecessarily outsourcing the responsibility of \"pushing\"\n> the IPAFrameContext\n> to the algorithms. All this can be encapsulated in the .get(), no? I thought\n> we all agreed on\n> the .get() design, so hence this direction.\n>\n\nIn case of requests underrun the algorithms would try to access a\nnon-existing frame context, hence their first \"get()\" will have to\ncreate one. Indeed having a get that creates entries blurries the line\nbetween a push and a get enough to justify using a single function.\n\n> >\n> > IPA components cannot have tests right ? It would be nice to have a\n> > unit test for this component to make sure it behaves as intended\n> > instead of having to debug it \"live\"\n>\n>\n> I see that you have mentioned a desire to make a generic\n> libcamera::RingBuffer\n> class implementation. While I do agree on the point of tests, but I am not\n> looking\n> at this angle just yet. It feels to me a bit early to do so because things\n> can be very\n> specific w.r.t IPAs. Even if we do it \"generically\" right now, only IPU3\n> would be the\n> only user I think. There are other similar parts that we want to provide a\n> generic\n> implementation to all the IPAs for e.g. IPAContext, IPAFrameContexts and so\n> on,\n> so probably once we have a good usage across multiple IPAs, we would be in a\n> better position to write a generic ring buffer implementation then and adapt\n> the IPAs on top of it?\n>\n\nAs I said I was thinking about other components outside of the IPAs\nthat could benefit from this as well. But I agree getting to have a\nfirst user is the first step to later eventually generalize.\n\n> >\n> > sorry, a lot of questions actually..\n>\n>\n> No issues ;-)\n>\n\nSo it seems to me that the only remaining concerns are actually:\n\n1) Map uniquely the frame number to the slot index\n\n2) Clarify if it can be assumed frames always start from 0. This is\n desirable as in the current model where frames start being\n produced when enough buffers have been queued, the FCQ will be\n filled with requests numbered from 0 on, while the first frame\n could potentially have an arbitrary value. Is that handled by\n Kieran's series from december ? (sorry, I lost track of all the\n moving parts).\n\nThanks\n j\n\n> >\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> > > +\n> > > +\t\treturn &frameContext;\n> > > +\t} else {\n> > > +\t\tif (isFull_) {\n> > > +\t\t\tLOG(IPAIPU3, Warning)\n> > > +\t\t\t\t<< \"Cannot create frame context for frame \" << frame\n> > > +\t\t\t\t<< \" since FCQueue is full\";\n> > > +\n> > > +\t\t\treturn nullptr;\n> > > +\t\t}\n> > > +\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> > > +\t\t/* Check if the queue is full to avoid over-queueing later */\n> > > +\t\tif (tail_->frame - head_->frame >= kMaxFrameContexts - 1)\n> > > +\t\t\tisFull_ = true;\n> > >\n> > > -\tif (frame != frameContext.frame) {\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> > > +\tif (isFull_)\n> > > +\t\tisFull_ = false;\n> > > }\n> > >\n> > > +/**\n> > > + * \\fn FCQueue::isFull()\n> > > + * \\brief Checks whether the frame context queue is full\n> > > + */\n> > > +\n> > > /**\n> > > * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n> > > */\n> > > @@ -260,6 +334,13 @@ void FCQueue::clear()\n> > > {\n> > > \tIPAFrameContext initFrameContext;\n> > > \tthis->fill(initFrameContext);\n> > > +\n> > > +\tisFull_ = false;\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 56d281f6..475855da 100644\n> > > --- a/src/ipa/ipu3/ipa_context.h\n> > > +++ b/src/ipa/ipu3/ipa_context.h\n> > > @@ -93,9 +93,18 @@ 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> > > +\tbool isFull() { return isFull_; }\n> > > +\n> > > +private:\n> > > +\tIPAFrameContext *head_;\n> > > +\tIPAFrameContext *tail_;\n> > > +\n> > > +\tbool isFull_;\n> > > };\n> > >\n> > > struct IPAContext {\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 0843d882..1d6ee515 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> > > +\t*context_.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 0517ABD160\n\tfor ;\n\tThu, 9 Jun 2022 16:20:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4936A65637;\n\tThu, 9 Jun 2022 18:20:58 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 274116559A\n\tfor ;\n\tThu, 9 Jun 2022 18:20:57 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 9ABF52000B;\n\tThu, 9 Jun 2022 16:20:56 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654791658;\n\tbh=dqp5EXv6kgZEIJuGMbfHNPnjPpk6Xg1tNmQeifzmCyo=;\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=RBm6tvyOh38Q4UfGqONeUbtJk5hRWSZrCniQe2VsMYXu/wMQPpSgADNjGiZnOx7Ey\n\tqfUHxVsH8hQ7bXHdFhwf9/lvlJBZQMze99b9WcmU15u60zI1Jj64kWe+R36E8Amj55\n\t+lZKrlngypYxEP65IEOesrAT4700PMp8AumpTgyphKlx/7vxZbxF0c0jw2YqLXq3nV\n\t3YJqBWpuIVqRygl38GiK+xybPjUuh1plgEL1ZZqR3UYvALHSxI4khf7gq/ZReadPgC\n\tjbgKMvxxqYcebQctJIfvHJ67EW7ptT8MXrPUz+QNZN+Ub6VlwkhXNhh7Wf3qUjs2cp\n\t9hNXFEMDsEV2Q==","Date":"Thu, 9 Jun 2022 18:20:54 +0200","To":"Umang Jain ","Message-ID":"<20220609162054.enj4kswtjtomm5po@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-3-umang.jain@ideasonboard.com>\n\t<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>\n\t<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23383,"web_url":"https://patchwork.libcamera.org/comment/23383/","msgid":"","date":"2022-06-10T09:46:20","subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::get()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 6/9/22 18:20, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Thu, Jun 09, 2022 at 01:09:28AM +0200, Umang Jain wrote:\n>> Hi Jacopo,\n>>\n>> Thanks for the review.\n>>\n>> On 6/8/22 15:56, Jacopo Mondi wrote:\n>>> Hi Umang,\n>>>\n>>> On Fri, Jun 03, 2022 at 03:22:57PM +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>>>>\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 | 95 +++++++++++++++++++++++++++++++++---\n>>>> src/ipa/ipu3/ipa_context.h | 9 ++++\n>>>> src/ipa/ipu3/ipu3.cpp | 4 +-\n>>>> 3 files changed, 100 insertions(+), 8 deletions(-)\n>>>>\n>>>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>>>> index 9f95a61c..2438d68d 100644\n>>>> --- a/src/ipa/ipu3/ipa_context.cpp\n>>>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>>>> @@ -222,13 +222,33 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\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.\n>>>> + * to be processed by the IPA at a given point. An 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>>>> + * \\var FCQueue::isFull_\n>>>> + * \\brief Flag set when the FCQueue is full\n>>>> */\n>>>>\n>>>> /**\n>>>> * \\brief FCQueue constructor\n>>>> */\n>>>> FCQueue::FCQueue()\n>>>> +\t: head_(nullptr), tail_(nullptr), isFull_(false)\n>>>> {\n>>>> \tclear();\n>>>> }\n>>>> @@ -238,21 +258,75 @@ FCQueue::FCQueue()\n>>>> * \\param[in] frame Frame number for which the IPAFrameContext needs to\n>>>> * retrieved\n>>>> *\n>>>> - * \\return Pointer to the IPAFrameContext\n>>>> + * This function returns the IPAFrameContext for the desired frame. If the\n>>>> + * frame context does not exist in the queue, the next available slot in the\n>>>> + * queue is returned. It is the responsibility of the caller to fill the correct\n>>>> + * IPAFrameContext parameters of newly returned IPAFrameContext.\n>>>> + *\n>>>> + * \\return Pointer to the IPAFrameContext or nullptr if frame context couldn't\n>>>> + * be created\n>>>> */\n>>>> IPAFrameContext *FCQueue::get(uint32_t frame)\n>>>> {\n>>>> -\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>>>> +\tif (frame <= tail_->frame) {\n>>> How does this work ? The first get() you call after construction has tail_\n>>> == nullptr. How come this doesn't segfault ? Is it because there's a\n>>> call to clear() that initializes the pointers before usage ?\n>>> Shouldn't the constructor take care of creating a usable object\n>>> without requiring clear() to be called ?\n>>\n>> Yes, bad naming. I think reset() would be more appropriate? The constructor\n>> is\n>> responsible yes (hence it calls clear() in the first place) so it was the\n> I missed the call to clear() in the constructor and I thought this\n> worked because of the clear() call in IPA::configure(). Sorry, this is\n> ok I guess.\n\n\nAck\n\n>\n>> matter of\n>> code-deduplication. We can discuss the naming conventions but tail_\n>> shouldn't\n>> be a nullptr before any .get() calls. So I do want proper initialization\n>> plus, a\n>> clear() or reset() to clear the ring buffer and resetting the tail_, head_\n>> etc.\n>>\n>>> Also, are we sure using the frame number in tail_ and head_ is the best\n>>> way to ensure that we actually track where we are in the queue ?\n>>>\n>>> I have a few worries:\n>>>\n>>> 1) are frame numbers always starting from 0 ?\n>>\n>> It should be, no?\n>>\n> I would say that it depends on how the kernel driver counts the frame\n> numbers. If the MIPI CSI-2 frame number is used it will count from 1\n> and increment per-virtual channel. If the driver maintains an internal\n> counter it should be reset at every start_stream/stop_stream session,\n> but I don't this there are guarantees if not looking at the driver and\n> making sure it does the right thing ?\n>\n> However I now also recall Kieran had a patch on his FrameSequence\n> series to restart counting from 0 the frame sequence number in\n> libcamera, do we assume they will always start from 0 because of this?\n>\n>>> 2) frame numbers are monotonically increasing, but can have gaps.\n>>> When you create a new frame context you increase by one to get the\n>>> next slot, but when you get an existing frame you compute the index\n>>> by doing frame % kMaxFrameContexts\n>>>\n>>> IPAFrameContext *FCQueue::get(uint32_t frame) {\n>>>\n>>> if (frame <= tail_->frame)\n>>> /* GET */\n>>> IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>>> else\n>>> /* PUSH */\n>>> tail_ = &this->at((tail_->frame + 1) % kMaxFrameContexts);\n>>> \t\ttail_->frame = frame;\n>>>\n>>> Isn't there a risk you get the wrong frame ?\n>>\n>> Yes, I've realized I have made an error here after posting to the list.\n>> I put the IPAFrameContext on the next slot immediate to tail_ , but that's\n>> not correct. It's only correct if we ensure there are not any gaps (majority\n>> of the\n>> development has been done by assuming that there will not be gaps for now).\n>>\n>> Gaps / Frame drops is yet another beast to handle. I guess I am keeping it\n>> separate\n>> from the \"per-frame controls\" planning for now. I discussed the same with\n>> Kieran\n>> the other day - that the frame-drop handling and per-frame controls need to\n>> kept\n>> separate for progress. Otherwise both half-baked designs, trying to\n>> satisfying/handle\n>> each other arbitrarily  might create chicken-and-egg problems.\n> Frame drops had been biting us hard enough already. I think we should\n> take them into account from the beginning in order to avoid having to\n> re-think how to handle them later..\n>\n> That's why I think we need to define how mapping the frame number to\n> the intended slot and do so uniquely in both \"get\" and \"push\". The\n> current \"frame % kMaxFrameContexts\" is enough (it will only cause\n> problems if the number of dropped frames in a kMaxFrameContexts period\n> is larger than the queue depth I think).\n>>> (also being this a LIFO, isn't the head the most recent and the\n> Sorry, I clearly meant FIFO\n>\n>>> tail the oldest entry ? you seem to advance tail when you push a\n>>> new frame)\n>>\n>> No, head_ represents the oldest entry and tail_ represents the latest\n>> IPAFrameContext\n>> that got created (refer documentation in this patch).\n>>\n> Mine was mostly a comment on the general concept, not as much as the\n> documentation of what you've done :) My thought was that in a stack\n> (LIFO) you pop the head and in a queue (FIFO) you pop the tail (and\n> consequentlly adavance the head when you push a new context).\n>\n> Basically this:\n> https://softwareengineering.stackexchange.com/questions/144477/on-a-queue-which-end-is-the-head\n>\n> However STL seems to use \"front\" and \"back\" for queues [2] as\n> synonymous for your head and tail, so I guess it's fine the way you\n> have it (you could use \"front\" and \"back\" to stay closer to STL)\n>\n> [2] https://en.cppreference.com/w/cpp/container/queue\n>\n>>> 2 ) tail_ gets initialized with an empty IPAFrameContext whose constructor\n>>> is\n>>> IPAFrameContext::IPAFrameContext() = default;\n>>>\n>>> hence its frame id is not intialized, or are POD types default\n>>> initialized in C++ ?\n>>\n>> We manually give it a value of '0' in clear(). It should work out from\n>> there.\n> Right. Again careful that if frame numbers are numbered using the\n> CSI-2 frame number, it will count from 1.\n>\n>> The .get() calls will over-write the frame number while creating the\n>> IPAFrameContext(s),\n>> so I don't think it will be a problem as such..\n>>\n>>> Anyway, isn't it simpler to use plain counters for head and tail instead\n>>> of pointers to the contained objects ? (This would maybe make\n>>> complicated to generalize the libcamera::RingBuffer implementation\n>>> maybe), but head_ and tail_ mainly serve for two purpose:\n>>> - underflow detection (trying to complete a frame not yet queued)\n>>> - overflow detection (trying to queue a frame overwrites a\n>>> not-yet-consumed one)\n>>\n>> Well, the main point for having these pointers is to know if a\n>> IPAFrameContext exists in the first\n>> place or not. The underflow/overflow checks comes later (... and we need\n> Well, you have this to find out if a frame context is the \"right\" one\n>\n> IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> \tif (frame != frameContext.frame)\n> \t\tLOG(IPAIPU3, Warning)\n> \t\t\t<< \"Got wrong frame context for frame \" << frame;\n>\n> But the distinction between a \"push\" and a \"get\" is just\n>\n> \tif (frame <= tail_->frame)\n>\n> which could be very well realized with integers.\n>\n> Anyway, just a suggestion to have the implementation possibly simpler\n>\n>\n>> have to a\n>> additional head_ for that, otherwise simply a tail_ would have been\n>> sufficient for the former)\n>>\n>> I agree with you that plain counters would be sufficient (and it doesn't\n>> really make a difference\n>> to me, either you store pointers OR frame numbers of oldest's and latest's).\n>> It is just storing\n>> a unique handles somehow.\n>>\n>>> Can't we have head and tail simply follow the latest frame number that\n>>> have been queued and the lastest frame number that has been consumed\n>>> respectively ?\n>>>\n>>> Collision detection will simply be\n>>> - undeflow: tail + 1 == head\n>>\n>> rather should be head + 1 == tail (according to how tail and head are used\n>> in this patch)\n> Yeah, I was using the notion I had in my head. Must be biased by the\n> dog metaphore in the link above :)\n\n\nA dog metaphor :D\n\n>\n>>> - overflow (queue frame): head - tail == array size\n>>>\n>>> The actual position on the array can always be computed as (frame %\n>>> kMaxFrameContexts)\n>>>\n>>> This doesn't however work well with gaps, as one frame gap means we're\n>>> actually not using one slot, so a little space is wasted. Ofc as the\n>>> number of gaps approaches the number of available slots, the risk of\n>>> overflow increases. But gaps should be an exceptional events and with\n>>> large enough buffers this shouldn't be a problem ?\n>>\n>> To be honest, I don't think storing IPAFrameContext pointers vs frame\n>> numbers\n>> should be a major concern. I say this because intrinsically everything\n>> revolves\n>> around the frame number in both cases.\n>>\n>> Going forward (and it's only my educated guess), storing head and tail frame\n>> numbers will get limiting a bit. I need to see how Kieran's work on\n>> merging/retaining\n>> controls is going on. The idea is controls from latest IPAFrameContext gets\n>> retained\n>> into next-created IPAFrameContext(s) and so on... If you think about it, the\n>> tail_->controls\n>> will get copied into the next IPAFrameContext while being created. In cases\n>> like this,\n>> having IPAFrameContexts pointers are useful in my opinion.\n>>\n> I don't think that's an issue as head and tail will simply allow you\n> get the context and from there you can do whatever you want.\n> Similarly, from the frame context you can get the frame number, so\n> yes, whatever you prefer for now\n>\n>>> Also I wonder if a push/get interface wouldn't be simpler, with new\n>>> reuests being pushed() and frames consumed with get(), but that's more\n>>> an implementation detail maybe..\n>>\n>> I do wondered that as well, but in my opinion having a push() + get()\n>> interface also means,\n>> each algorithm has to do various checks on their part. For e.g.\n>>\n>> Algorithm1:\n>>\n>>     .get(X)  <--- Failed\n>>     .push(XFrameContext)\n>>     .get(X) <---- Success and carry on\n>>\n>> Algorithm2:\n>>\n>>     .get(X) <-- Success because Algorithm1 created XFrameContext\n>>     // but you still need to write failure path here containing\n>> .push(XFrameContext)\n>>\n>> .. and so on.\n>>\n>> Also, various Algorithm might need to create different frame's\n>> IPAFrameContext in a single run,\n>> depending on their latencies.\n>>\n>> So I think we are unnecessarily outsourcing the responsibility of \"pushing\"\n>> the IPAFrameContext\n>> to the algorithms. All this can be encapsulated in the .get(), no? I thought\n>> we all agreed on\n>> the .get() design, so hence this direction.\n>>\n> In case of requests underrun the algorithms would try to access a\n> non-existing frame context, hence their first \"get()\" will have to\n> create one. Indeed having a get that creates entries blurries the line\n\n\nYes, it's expected behavior  for algorithms to access a non-existent \nframe context. It will get created and the algorithm will populate its \nvalue into the frame-context even before the frame is queued up.\n\nYes, in underrun, we will need to notify the application about it as \nsoon as possible (we have some discussion in PFC Error v2) so let it get \ncleared up. It would be interesting to see how we recover up - from the \nunderruns...\n\n> between a push and a get enough to justify using a single function.\n>\n>>> IPA components cannot have tests right ? It would be nice to have a\n>>> unit test for this component to make sure it behaves as intended\n>>> instead of having to debug it \"live\"\n>>\n>> I see that you have mentioned a desire to make a generic\n>> libcamera::RingBuffer\n>> class implementation. While I do agree on the point of tests, but I am not\n>> looking\n>> at this angle just yet. It feels to me a bit early to do so because things\n>> can be very\n>> specific w.r.t IPAs. Even if we do it \"generically\" right now, only IPU3\n>> would be the\n>> only user I think. There are other similar parts that we want to provide a\n>> generic\n>> implementation to all the IPAs for e.g. IPAContext, IPAFrameContexts and so\n>> on,\n>> so probably once we have a good usage across multiple IPAs, we would be in a\n>> better position to write a generic ring buffer implementation then and adapt\n>> the IPAs on top of it?\n>>\n> As I said I was thinking about other components outside of the IPAs\n> that could benefit from this as well. But I agree getting to have a\n> first user is the first step to later eventually generalize.\n\n\nAck\n\n>\n>>> sorry, a lot of questions actually..\n>>\n>> No issues ;-)\n>>\n> So it seems to me that the only remaining concerns are actually:\n>\n> 1) Map uniquely the frame number to the slot index\n>\n> 2) Clarify if it can be assumed frames always start from 0. This is\n\n\nSo I have been thinking on this, and it seems it should be okay even if \nframe doesn't start from '0'.\n\nFor e.g. let's assume the FCQueue size is 16 and first frame sequence \ncomes to be 11 (or any number really).\n\nWhat shall happen?\n\nIn IPA::queueRequest(), frame context #11 not found - so a new one is \ncreated and tail(or the latest frame counter) is assigned to that.\n\nSo the first 10 slots will remained un-initialised (as a result of \nclear()) until the next 16 (FCQueue.size()) frames are queued up and \nthen everything circulates accordingly from FCqueue point-of-view then.\n\nNow the question is what if something tries to access those \nun-initialised 10 frame contexts. The probable contenders to that would \nbe algorithms_ in IPAIPU3. The Algorithm::prepare() doesn't have a frame \nnumber - so probably it's independent in its own way ? (I am not sure, \nhave asked Jean-M. for that) and Algorithm::process() works off a frame \nnumber (in 4/4), so we are in control which frame context is accessed by \nthe algorithms there. So I think it should be fine to have a transient \nsituation of un-initialised frame contexts. If we really want to prevent \naccess, I guess it shouldn't be that hard to detect this situation and \nblock access to these frame-contexts.\n\nAs long as the sequence number comes in incremental order of '1' - it \nshould be fine even if the sequence does _not_ start from '0'.\n\nIf it comes in incremental order  > 1, then surely we'll have gaps as \ndiscussed previously, but that's for later / 'frame drop handling' I think.\n\n> desirable as in the current model where frames start being\n> produced when enough buffers have been queued, the FCQ will be\n> filled with requests numbered from 0 on, while the first frame\n> could potentially have an arbitrary value. Is that handled by\n> Kieran's series from december ? (sorry, I lost track of all the\n> moving parts).\n>\n> Thanks\n> j\n>\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>>>> +\n>>>> +\t\treturn &frameContext;\n>>>> +\t} else {\n>>>> +\t\tif (isFull_) {\n>>>> +\t\t\tLOG(IPAIPU3, Warning)\n>>>> +\t\t\t\t<< \"Cannot create frame context for frame \" << frame\n>>>> +\t\t\t\t<< \" since FCQueue is full\";\n>>>> +\n>>>> +\t\t\treturn nullptr;\n>>>> +\t\t}\n>>>> +\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>>>> +\t\t/* Check if the queue is full to avoid over-queueing later */\n>>>> +\t\tif (tail_->frame - head_->frame >= kMaxFrameContexts - 1)\n>>>> +\t\t\tisFull_ = true;\n>>>>\n>>>> -\tif (frame != frameContext.frame) {\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>>>> +\tif (isFull_)\n>>>> +\t\tisFull_ = false;\n>>>> }\n>>>>\n>>>> +/**\n>>>> + * \\fn FCQueue::isFull()\n>>>> + * \\brief Checks whether the frame context queue is full\n>>>> + */\n>>>> +\n>>>> /**\n>>>> * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n>>>> */\n>>>> @@ -260,6 +334,13 @@ void FCQueue::clear()\n>>>> {\n>>>> \tIPAFrameContext initFrameContext;\n>>>> \tthis->fill(initFrameContext);\n>>>> +\n>>>> +\tisFull_ = false;\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 56d281f6..475855da 100644\n>>>> --- a/src/ipa/ipu3/ipa_context.h\n>>>> +++ b/src/ipa/ipu3/ipa_context.h\n>>>> @@ -93,9 +93,18 @@ 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>>>> +\tbool isFull() { return isFull_; }\n>>>> +\n>>>> +private:\n>>>> +\tIPAFrameContext *head_;\n>>>> +\tIPAFrameContext *tail_;\n>>>> +\n>>>> +\tbool isFull_;\n>>>> };\n>>>>\n>>>> struct IPAContext {\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index 0843d882..1d6ee515 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>>>> +\t*context_.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 B9B3EBD161\n\tfor ;\n\tFri, 10 Jun 2022 09:46:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0E3B65635;\n\tFri, 10 Jun 2022 11:46:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10118600F4\n\tfor ;\n\tFri, 10 Jun 2022 11:46:24 +0200 (CEST)","from [192.168.101.240] (217.red-95-124-192.dynamicip.rima-tde.net\n\t[95.124.192.217])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6D3986CF;\n\tFri, 10 Jun 2022 11:46:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654854385;\n\tbh=Tvyaff25h5+nR1jGnx10KfUi/qDvyhwJVGqHRclkttM=;\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=AWZ0SlyCGb45+veZAjtFeXYoQgOeJBXZbUT8LQx/bw9RKx+SV6bBNnSpXsET9aehM\n\t35s1RClXoHKjgPrFGFjYT/C6mpsCrY+p5Nh2sYfH/93KNPsitlGFUVe3Fy8zRE15tz\n\tRmJQlPNzHLCGmjVyEmtrvHJxWQohOlR0zqKA0LG2fo7Bf8+Zl9J270v6I5yprRZOLJ\n\tcdvp16Rlegt3J6ANlMKAhNSG8nS3FoKUuvswk3MvS9egKvAJYSFc8vBjax0UZk+b9j\n\tsNIFgO5U5MBkQNiF+P3jZwdARtFw6fTmCnM3IGHl55H2UVchmrkwk98s0HVsz6BDAt\n\tVQfQlJDWHF9Wg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654854383;\n\tbh=Tvyaff25h5+nR1jGnx10KfUi/qDvyhwJVGqHRclkttM=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=JZeUVrF4gaEc51acPoVzN6sTG0KqbUqKdQp3mfOFDGSKE5xJAJfGHezY97T4fZG8y\n\tqyg+wP5mMG7J4W3ctfigy7gc0QHgQlxTvPKOAAwSPVLrdyGWZpOqViBNeQoDgVzU+s\n\tdyed1ZEogG7VT1gnsSuZUk7k9Nkzd4OT9AqCah1Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JZeUVrF4\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 10 Jun 2022 11:46:20 +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":"Jacopo Mondi ","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-3-umang.jain@ideasonboard.com>\n\t<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>\n\t<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>\n\t<20220609162054.enj4kswtjtomm5po@uno.localdomain>","In-Reply-To":"<20220609162054.enj4kswtjtomm5po@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::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":23385,"web_url":"https://patchwork.libcamera.org/comment/23385/","msgid":"<20220610103201.4ranhfbns7d46gpf@uno.localdomain>","date":"2022-06-10T10:32:01","subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::get()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Fri, Jun 10, 2022 at 11:46:20AM +0200, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 6/9/22 18:20, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Thu, Jun 09, 2022 at 01:09:28AM +0200, Umang Jain wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for the review.\n> > >\n> > > On 6/8/22 15:56, Jacopo Mondi wrote:\n> > > > Hi Umang,\n> > > >\n> > > > On Fri, Jun 03, 2022 at 03:22:57PM +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> > > > >\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 | 95 +++++++++++++++++++++++++++++++++---\n> > > > > src/ipa/ipu3/ipa_context.h | 9 ++++\n> > > > > src/ipa/ipu3/ipu3.cpp | 4 +-\n> > > > > 3 files changed, 100 insertions(+), 8 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > > > > index 9f95a61c..2438d68d 100644\n> > > > > --- a/src/ipa/ipu3/ipa_context.cpp\n> > > > > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > > > > @@ -222,13 +222,33 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\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.\n> > > > > + * to be processed by the IPA at a given point. An 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> > > > > + * \\var FCQueue::isFull_\n> > > > > + * \\brief Flag set when the FCQueue is full\n> > > > > */\n> > > > >\n> > > > > /**\n> > > > > * \\brief FCQueue constructor\n> > > > > */\n> > > > > FCQueue::FCQueue()\n> > > > > +\t: head_(nullptr), tail_(nullptr), isFull_(false)\n> > > > > {\n> > > > > \tclear();\n> > > > > }\n> > > > > @@ -238,21 +258,75 @@ FCQueue::FCQueue()\n> > > > > * \\param[in] frame Frame number for which the IPAFrameContext needs to\n> > > > > * retrieved\n> > > > > *\n> > > > > - * \\return Pointer to the IPAFrameContext\n> > > > > + * This function returns the IPAFrameContext for the desired frame. If the\n> > > > > + * frame context does not exist in the queue, the next available slot in the\n> > > > > + * queue is returned. It is the responsibility of the caller to fill the correct\n> > > > > + * IPAFrameContext parameters of newly returned IPAFrameContext.\n> > > > > + *\n> > > > > + * \\return Pointer to the IPAFrameContext or nullptr if frame context couldn't\n> > > > > + * be created\n> > > > > */\n> > > > > IPAFrameContext *FCQueue::get(uint32_t frame)\n> > > > > {\n> > > > > -\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > > > +\tif (frame <= tail_->frame) {\n> > > > How does this work ? The first get() you call after construction has tail_\n> > > > == nullptr. How come this doesn't segfault ? Is it because there's a\n> > > > call to clear() that initializes the pointers before usage ?\n> > > > Shouldn't the constructor take care of creating a usable object\n> > > > without requiring clear() to be called ?\n> > >\n> > > Yes, bad naming. I think reset() would be more appropriate? The constructor\n> > > is\n> > > responsible yes (hence it calls clear() in the first place) so it was the\n> > I missed the call to clear() in the constructor and I thought this\n> > worked because of the clear() call in IPA::configure(). Sorry, this is\n> > ok I guess.\n>\n>\n> Ack\n>\n> >\n> > > matter of\n> > > code-deduplication. We can discuss the naming conventions but tail_\n> > > shouldn't\n> > > be a nullptr before any .get() calls. So I do want proper initialization\n> > > plus, a\n> > > clear() or reset() to clear the ring buffer and resetting the tail_, head_\n> > > etc.\n> > >\n> > > > Also, are we sure using the frame number in tail_ and head_ is the best\n> > > > way to ensure that we actually track where we are in the queue ?\n> > > >\n> > > > I have a few worries:\n> > > >\n> > > > 1) are frame numbers always starting from 0 ?\n> > >\n> > > It should be, no?\n> > >\n> > I would say that it depends on how the kernel driver counts the frame\n> > numbers. If the MIPI CSI-2 frame number is used it will count from 1\n> > and increment per-virtual channel. If the driver maintains an internal\n> > counter it should be reset at every start_stream/stop_stream session,\n> > but I don't this there are guarantees if not looking at the driver and\n> > making sure it does the right thing ?\n> >\n> > However I now also recall Kieran had a patch on his FrameSequence\n> > series to restart counting from 0 the frame sequence number in\n> > libcamera, do we assume they will always start from 0 because of this?\n> >\n> > > > 2) frame numbers are monotonically increasing, but can have gaps.\n> > > > When you create a new frame context you increase by one to get the\n> > > > next slot, but when you get an existing frame you compute the index\n> > > > by doing frame % kMaxFrameContexts\n> > > >\n> > > > IPAFrameContext *FCQueue::get(uint32_t frame) {\n> > > >\n> > > > if (frame <= tail_->frame)\n> > > > /* GET */\n> > > > IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > > else\n> > > > /* PUSH */\n> > > > tail_ = &this->at((tail_->frame + 1) % kMaxFrameContexts);\n> > > > \t\ttail_->frame = frame;\n> > > >\n> > > > Isn't there a risk you get the wrong frame ?\n> > >\n> > > Yes, I've realized I have made an error here after posting to the list.\n> > > I put the IPAFrameContext on the next slot immediate to tail_ , but that's\n> > > not correct. It's only correct if we ensure there are not any gaps (majority\n> > > of the\n> > > development has been done by assuming that there will not be gaps for now).\n> > >\n> > > Gaps / Frame drops is yet another beast to handle. I guess I am keeping it\n> > > separate\n> > > from the \"per-frame controls\" planning for now. I discussed the same with\n> > > Kieran\n> > > the other day - that the frame-drop handling and per-frame controls need to\n> > > kept\n> > > separate for progress. Otherwise both half-baked designs, trying to\n> > > satisfying/handle\n> > > each other arbitrarily  might create chicken-and-egg problems.\n> > Frame drops had been biting us hard enough already. I think we should\n> > take them into account from the beginning in order to avoid having to\n> > re-think how to handle them later..\n> >\n> > That's why I think we need to define how mapping the frame number to\n> > the intended slot and do so uniquely in both \"get\" and \"push\". The\n> > current \"frame % kMaxFrameContexts\" is enough (it will only cause\n> > problems if the number of dropped frames in a kMaxFrameContexts period\n> > is larger than the queue depth I think).\n> > > > (also being this a LIFO, isn't the head the most recent and the\n> > Sorry, I clearly meant FIFO\n> >\n> > > > tail the oldest entry ? you seem to advance tail when you push a\n> > > > new frame)\n> > >\n> > > No, head_ represents the oldest entry and tail_ represents the latest\n> > > IPAFrameContext\n> > > that got created (refer documentation in this patch).\n> > >\n> > Mine was mostly a comment on the general concept, not as much as the\n> > documentation of what you've done :) My thought was that in a stack\n> > (LIFO) you pop the head and in a queue (FIFO) you pop the tail (and\n> > consequentlly adavance the head when you push a new context).\n> >\n> > Basically this:\n> > https://softwareengineering.stackexchange.com/questions/144477/on-a-queue-which-end-is-the-head\n> >\n> > However STL seems to use \"front\" and \"back\" for queues [2] as\n> > synonymous for your head and tail, so I guess it's fine the way you\n> > have it (you could use \"front\" and \"back\" to stay closer to STL)\n> >\n> > [2] https://en.cppreference.com/w/cpp/container/queue\n> >\n> > > > 2 ) tail_ gets initialized with an empty IPAFrameContext whose constructor\n> > > > is\n> > > > IPAFrameContext::IPAFrameContext() = default;\n> > > >\n> > > > hence its frame id is not intialized, or are POD types default\n> > > > initialized in C++ ?\n> > >\n> > > We manually give it a value of '0' in clear(). It should work out from\n> > > there.\n> > Right. Again careful that if frame numbers are numbered using the\n> > CSI-2 frame number, it will count from 1.\n> >\n> > > The .get() calls will over-write the frame number while creating the\n> > > IPAFrameContext(s),\n> > > so I don't think it will be a problem as such..\n> > >\n> > > > Anyway, isn't it simpler to use plain counters for head and tail instead\n> > > > of pointers to the contained objects ? (This would maybe make\n> > > > complicated to generalize the libcamera::RingBuffer implementation\n> > > > maybe), but head_ and tail_ mainly serve for two purpose:\n> > > > - underflow detection (trying to complete a frame not yet queued)\n> > > > - overflow detection (trying to queue a frame overwrites a\n> > > > not-yet-consumed one)\n> > >\n> > > Well, the main point for having these pointers is to know if a\n> > > IPAFrameContext exists in the first\n> > > place or not. The underflow/overflow checks comes later (... and we need\n> > Well, you have this to find out if a frame context is the \"right\" one\n> >\n> > IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > \tif (frame != frameContext.frame)\n> > \t\tLOG(IPAIPU3, Warning)\n> > \t\t\t<< \"Got wrong frame context for frame \" << frame;\n> >\n> > But the distinction between a \"push\" and a \"get\" is just\n> >\n> > \tif (frame <= tail_->frame)\n> >\n> > which could be very well realized with integers.\n> >\n> > Anyway, just a suggestion to have the implementation possibly simpler\n> >\n> >\n> > > have to a\n> > > additional head_ for that, otherwise simply a tail_ would have been\n> > > sufficient for the former)\n> > >\n> > > I agree with you that plain counters would be sufficient (and it doesn't\n> > > really make a difference\n> > > to me, either you store pointers OR frame numbers of oldest's and latest's).\n> > > It is just storing\n> > > a unique handles somehow.\n> > >\n> > > > Can't we have head and tail simply follow the latest frame number that\n> > > > have been queued and the lastest frame number that has been consumed\n> > > > respectively ?\n> > > >\n> > > > Collision detection will simply be\n> > > > - undeflow: tail + 1 == head\n> > >\n> > > rather should be head + 1 == tail (according to how tail and head are used\n> > > in this patch)\n> > Yeah, I was using the notion I had in my head. Must be biased by the\n> > dog metaphore in the link above :)\n>\n>\n> A dog metaphor :D\n>\n> >\n> > > > - overflow (queue frame): head - tail == array size\n> > > >\n> > > > The actual position on the array can always be computed as (frame %\n> > > > kMaxFrameContexts)\n> > > >\n> > > > This doesn't however work well with gaps, as one frame gap means we're\n> > > > actually not using one slot, so a little space is wasted. Ofc as the\n> > > > number of gaps approaches the number of available slots, the risk of\n> > > > overflow increases. But gaps should be an exceptional events and with\n> > > > large enough buffers this shouldn't be a problem ?\n> > >\n> > > To be honest, I don't think storing IPAFrameContext pointers vs frame\n> > > numbers\n> > > should be a major concern. I say this because intrinsically everything\n> > > revolves\n> > > around the frame number in both cases.\n> > >\n> > > Going forward (and it's only my educated guess), storing head and tail frame\n> > > numbers will get limiting a bit. I need to see how Kieran's work on\n> > > merging/retaining\n> > > controls is going on. The idea is controls from latest IPAFrameContext gets\n> > > retained\n> > > into next-created IPAFrameContext(s) and so on... If you think about it, the\n> > > tail_->controls\n> > > will get copied into the next IPAFrameContext while being created. In cases\n> > > like this,\n> > > having IPAFrameContexts pointers are useful in my opinion.\n> > >\n> > I don't think that's an issue as head and tail will simply allow you\n> > get the context and from there you can do whatever you want.\n> > Similarly, from the frame context you can get the frame number, so\n> > yes, whatever you prefer for now\n> >\n> > > > Also I wonder if a push/get interface wouldn't be simpler, with new\n> > > > reuests being pushed() and frames consumed with get(), but that's more\n> > > > an implementation detail maybe..\n> > >\n> > > I do wondered that as well, but in my opinion having a push() + get()\n> > > interface also means,\n> > > each algorithm has to do various checks on their part. For e.g.\n> > >\n> > > Algorithm1:\n> > >\n> > >     .get(X)  <--- Failed\n> > >     .push(XFrameContext)\n> > >     .get(X) <---- Success and carry on\n> > >\n> > > Algorithm2:\n> > >\n> > >     .get(X) <-- Success because Algorithm1 created XFrameContext\n> > >     // but you still need to write failure path here containing\n> > > .push(XFrameContext)\n> > >\n> > > .. and so on.\n> > >\n> > > Also, various Algorithm might need to create different frame's\n> > > IPAFrameContext in a single run,\n> > > depending on their latencies.\n> > >\n> > > So I think we are unnecessarily outsourcing the responsibility of \"pushing\"\n> > > the IPAFrameContext\n> > > to the algorithms. All this can be encapsulated in the .get(), no? I thought\n> > > we all agreed on\n> > > the .get() design, so hence this direction.\n> > >\n> > In case of requests underrun the algorithms would try to access a\n> > non-existing frame context, hence their first \"get()\" will have to\n> > create one. Indeed having a get that creates entries blurries the line\n>\n>\n> Yes, it's expected behavior  for algorithms to access a non-existent frame\n> context. It will get created and the algorithm will populate its value into\n> the frame-context even before the frame is queued up.\n>\n> Yes, in underrun, we will need to notify the application about it as soon as\n> possible (we have some discussion in PFC Error v2) so let it get cleared up.\n> It would be interesting to see how we recover up - from the underruns...\n>\n> > between a push and a get enough to justify using a single function.\n> >\n> > > > IPA components cannot have tests right ? It would be nice to have a\n> > > > unit test for this component to make sure it behaves as intended\n> > > > instead of having to debug it \"live\"\n> > >\n> > > I see that you have mentioned a desire to make a generic\n> > > libcamera::RingBuffer\n> > > class implementation. While I do agree on the point of tests, but I am not\n> > > looking\n> > > at this angle just yet. It feels to me a bit early to do so because things\n> > > can be very\n> > > specific w.r.t IPAs. Even if we do it \"generically\" right now, only IPU3\n> > > would be the\n> > > only user I think. There are other similar parts that we want to provide a\n> > > generic\n> > > implementation to all the IPAs for e.g. IPAContext, IPAFrameContexts and so\n> > > on,\n> > > so probably once we have a good usage across multiple IPAs, we would be in a\n> > > better position to write a generic ring buffer implementation then and adapt\n> > > the IPAs on top of it?\n> > >\n> > As I said I was thinking about other components outside of the IPAs\n> > that could benefit from this as well. But I agree getting to have a\n> > first user is the first step to later eventually generalize.\n>\n>\n> Ack\n>\n> >\n> > > > sorry, a lot of questions actually..\n> > >\n> > > No issues ;-)\n> > >\n> > So it seems to me that the only remaining concerns are actually:\n> >\n> > 1) Map uniquely the frame number to the slot index\n> >\n> > 2) Clarify if it can be assumed frames always start from 0. This is\n>\n>\n> So I have been thinking on this, and it seems it should be okay even if\n> frame doesn't start from '0'.\n>\n> For e.g. let's assume the FCQueue size is 16 and first frame sequence comes\n> to be 11 (or any number really).\n>\n> What shall happen?\n>\n> In IPA::queueRequest(), frame context #11 not found - so a new one is\n> created and tail(or the latest frame counter) is assigned to that.\n>\n> So the first 10 slots will remained un-initialised (as a result of clear())\n> until the next 16 (FCQueue.size()) frames are queued up and then everything\n> circulates accordingly from FCqueue point-of-view then.\n>\n> Now the question is what if something tries to access those un-initialised\n> 10 frame contexts. The probable contenders to that would be algorithms_ in\n> IPAIPU3. The Algorithm::prepare() doesn't have a frame number - so probably\n> it's independent in its own way ? (I am not sure, have asked Jean-M. for\n> that) and Algorithm::process() works off a frame number (in 4/4), so we are\n> in control which frame context is accessed by the algorithms there. So I\n> think it should be fine to have a transient situation of un-initialised\n> frame contexts. If we really want to prevent access, I guess it shouldn't be\n> that hard to detect this situation and block access to these frame-contexts.\n>\n> As long as the sequence number comes in incremental order of '1' - it should\n> be fine even if the sequence does _not_ start from '0'.\n>\n\nThanks for the analysis.\n\nMy concern is however not that much on unused slots (starting from any\narbitrary number should not be an issue, this is a circular buffer\nafter all) but regarding how to match Request numbers and frame\nnumbers, as I'm under the probably false impression that we assume\nthey both start from 0 (and increment at the same pace ?)\n\nRequest has a sequence number incremented per-camera each time a new\nrequest is queued. (it is not reset by a camera start()/stop() it\nseems, but I guess this was intentional ?)\n\nThe IPU3 pipeline handler use the Request number to create IPU3Frames::Info\nsee IPU3CameraData::queuePendingRequests() where the frame info is\nfirst created for a new request\n\n IPU3Frames::Info *IPU3Frames::create(Request *request)\n {\n unsigned int id = request->sequence();\n\n info->id = id;\n\n }\n\nThe info->id (which carries the request sequence number) is used as\n'frame id' to all operations towards the IPA\n\n ipa_->queueRequest(info->id, ..)\n ipa_->fillParamsBuffer(info->id, ..)\n\tipa_->processStatsBuffer(info->id, ..)\n\nprocessStatsBuffer() in particular runs all the algorithms with the\nframe context for a \"request sequence number\" and produces the\ncontrols to be applied on the sensor providing it the request sequence\n\n void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,\n\nAs the [[maybe_unused]] suggests we don't currently use the request\nid, but going forward I assume we want pass back the pipeline the\nframe sequence id to which the computed controls apply to. Right now\nthis information is deduced using the Request sequence, hence my\nassumption that currently assume they both start from 0 and increment\nat the same pace ?\n\nI have a gut feeling we'll have the same problem when having to detect\nframe drops, if we want a model where if frame X has been skept, then\nrequest X should be returned with error.\n\nSo yes, this all might be not be a concern for the FCQ itself, which\njust wants a numeric index without caring about what it represents,\nbut I feel that we should start thinking sooner than later about how\nto associate the request id and the frame sequence uniquely in the\nPH/IPA. Maybe this was already a known point and I have just realized\nit now ? Well, it will stay written form at least :)\n\nThanks\n j\n\n> If it comes in incremental order  > 1, then surely we'll have gaps as\n> discussed previously, but that's for later / 'frame drop handling' I think.\n>\n> > desirable as in the current model where frames start being\n> > produced when enough buffers have been queued, the FCQ will be\n> > filled with requests numbered from 0 on, while the first frame\n> > could potentially have an arbitrary value. Is that handled by\n> > Kieran's series from december ? (sorry, I lost track of all the\n> > moving parts).\n> >\n> > Thanks\n> > j\n> >\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> > > > > +\n> > > > > +\t\treturn &frameContext;\n> > > > > +\t} else {\n> > > > > +\t\tif (isFull_) {\n> > > > > +\t\t\tLOG(IPAIPU3, Warning)\n> > > > > +\t\t\t\t<< \"Cannot create frame context for frame \" << frame\n> > > > > +\t\t\t\t<< \" since FCQueue is full\";\n> > > > > +\n> > > > > +\t\t\treturn nullptr;\n> > > > > +\t\t}\n> > > > > +\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> > > > > +\t\t/* Check if the queue is full to avoid over-queueing later */\n> > > > > +\t\tif (tail_->frame - head_->frame >= kMaxFrameContexts - 1)\n> > > > > +\t\t\tisFull_ = true;\n> > > > >\n> > > > > -\tif (frame != frameContext.frame) {\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> > > > > +\tif (isFull_)\n> > > > > +\t\tisFull_ = false;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\fn FCQueue::isFull()\n> > > > > + * \\brief Checks whether the frame context queue is full\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n> > > > > */\n> > > > > @@ -260,6 +334,13 @@ void FCQueue::clear()\n> > > > > {\n> > > > > \tIPAFrameContext initFrameContext;\n> > > > > \tthis->fill(initFrameContext);\n> > > > > +\n> > > > > +\tisFull_ = false;\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 56d281f6..475855da 100644\n> > > > > --- a/src/ipa/ipu3/ipa_context.h\n> > > > > +++ b/src/ipa/ipu3/ipa_context.h\n> > > > > @@ -93,9 +93,18 @@ 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> > > > > +\tbool isFull() { return isFull_; }\n> > > > > +\n> > > > > +private:\n> > > > > +\tIPAFrameContext *head_;\n> > > > > +\tIPAFrameContext *tail_;\n> > > > > +\n> > > > > +\tbool isFull_;\n> > > > > };\n> > > > >\n> > > > > struct IPAContext {\n> > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > > index 0843d882..1d6ee515 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> > > > > +\t*context_.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 3F79CBD161\n\tfor ;\n\tFri, 10 Jun 2022 10:32:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E99365635;\n\tFri, 10 Jun 2022 12:32:05 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::221])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0479C633A4\n\tfor ;\n\tFri, 10 Jun 2022 12:32:04 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 54D93240011;\n\tFri, 10 Jun 2022 10:32:03 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654857125;\n\tbh=v7ZhIZKYgfgj0GlYu2HvC2zcaijn+2oL9EcIwpBD90c=;\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=KpKBGEdhXiVtSpnccaJBCFPvndxKfCWxE7pejd4BCHlRiuYpJgBT2GjuXTbRhHScO\n\tY5oL3y1v6YcOBnaACoK2jBb8cP4ZeuvnrKZ3bN0JUunBdyixZHF7h1otuvT9lzmZ2F\n\tF2xHSONtdWjn5veoaPf0zuDduG7WgjajxHTygqkwivqzYdBJ+mCRNIQwECHGvFkfnD\n\tLVQ1ySj7dg45yXSymKF6JZGWOJ11NpWfO8V3VOgrN2ry0rpDV7991hwit8w13BXn5c\n\t+Vp1D3eVYUo5hiEyOjJuE/dH5PRQvGq24h9dk1BfYeDlI9toaJJBIR/IQJ4e2PAL04\n\t0yuJFzpy4bc7Q==","Date":"Fri, 10 Jun 2022 12:32:01 +0200","To":"Umang Jain ","Message-ID":"<20220610103201.4ranhfbns7d46gpf@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-3-umang.jain@ideasonboard.com>\n\t<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>\n\t<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>\n\t<20220609162054.enj4kswtjtomm5po@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23386,"web_url":"https://patchwork.libcamera.org/comment/23386/","msgid":"<165488883227.1070686.8210324805195113270@Monstersaurus>","date":"2022-06-10T19:20:32","subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::get()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nQuoting Jacopo Mondi via libcamera-devel (2022-06-09 17:20:54)\n> Hi Umang,\n> \n> On Thu, Jun 09, 2022 at 01:09:28AM +0200, Umang Jain wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for the review.\n> >\n> > On 6/8/22 15:56, Jacopo Mondi wrote:\n> > > Hi Umang,\n> > >\n> > > On Fri, Jun 03, 2022 at 03:22:57PM +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> > > >\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 | 95 +++++++++++++++++++++++++++++++++---\n> > > > src/ipa/ipu3/ipa_context.h | 9 ++++\n> > > > src/ipa/ipu3/ipu3.cpp | 4 +-\n> > > > 3 files changed, 100 insertions(+), 8 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > > > index 9f95a61c..2438d68d 100644\n> > > > --- a/src/ipa/ipu3/ipa_context.cpp\n> > > > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > > > @@ -222,13 +222,33 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\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.\n> > > > + * to be processed by the IPA at a given point. An 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> > > > + * \\var FCQueue::isFull_\n> > > > + * \\brief Flag set when the FCQueue is full\n> > > > */\n> > > >\n> > > > /**\n> > > > * \\brief FCQueue constructor\n> > > > */\n> > > > FCQueue::FCQueue()\n> > > > + : head_(nullptr), tail_(nullptr), isFull_(false)\n> > > > {\n> > > > clear();\n> > > > }\n> > > > @@ -238,21 +258,75 @@ FCQueue::FCQueue()\n> > > > * \\param[in] frame Frame number for which the IPAFrameContext needs to\n> > > > * retrieved\n> > > > *\n> > > > - * \\return Pointer to the IPAFrameContext\n> > > > + * This function returns the IPAFrameContext for the desired frame. If the\n> > > > + * frame context does not exist in the queue, the next available slot in the\n> > > > + * queue is returned. It is the responsibility of the caller to fill the correct\n> > > > + * IPAFrameContext parameters of newly returned IPAFrameContext.\n> > > > + *\n> > > > + * \\return Pointer to the IPAFrameContext or nullptr if frame context couldn't\n> > > > + * be created\n> > > > */\n> > > > IPAFrameContext *FCQueue::get(uint32_t frame)\n> > > > {\n> > > > - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > > + if (frame <= tail_->frame) {\n> > > How does this work ? The first get() you call after construction has tail_\n> > > == nullptr. How come this doesn't segfault ? Is it because there's a\n> > > call to clear() that initializes the pointers before usage ?\n> > > Shouldn't the constructor take care of creating a usable object\n> > > without requiring clear() to be called ?\n> >\n> >\n> > Yes, bad naming. I think reset() would be more appropriate? The constructor\n> > is\n> > responsible yes (hence it calls clear() in the first place) so it was the\n> \n> I missed the call to clear() in the constructor and I thought this\n> worked because of the clear() call in IPA::configure(). Sorry, this is\n> ok I guess.\n> \n> > matter of\n> > code-deduplication. We can discuss the naming conventions but tail_\n> > shouldn't\n> > be a nullptr before any .get() calls. So I do want proper initialization\n> > plus, a\n> > clear() or reset() to clear the ring buffer and resetting the tail_, head_\n> > etc.\n> >\n> > >\n> > > Also, are we sure using the frame number in tail_ and head_ is the best\n> > > way to ensure that we actually track where we are in the queue ?\n> > >\n> > > I have a few worries:\n> > >\n> > > 1) are frame numbers always starting from 0 ?\n> >\n> >\n> > It should be, no?\n> >\n> \n> I would say that it depends on how the kernel driver counts the frame\n> numbers. If the MIPI CSI-2 frame number is used it will count from 1\n> and increment per-virtual channel. If the driver maintains an internal\n> counter it should be reset at every start_stream/stop_stream session,\n> but I don't this there are guarantees if not looking at the driver and\n> making sure it does the right thing ?\n> \n> However I now also recall Kieran had a patch on his FrameSequence\n> series to restart counting from 0 the frame sequence number in\n> libcamera, do we assume they will always start from 0 because of this?\n> \n\nYes, I have a patch that will ensure frame numbers always start at zero\nfrom the V4L2VideoDevice.\n\nI think this is important to keep things consistent, regardless of what\nthe kernel does or doesn't do correctly.. So the first frame from the\nCSI2 receiver should always be zero from any call to start().\n\nWe have it in development branches, but I don't think I've posted it to\nthe list yet. I'll try to make sure I do that on Monday, but you'll\nprobably find it in the code camp development branch before that if\nyou're interested.\n\n\n\n> > > 2) frame numbers are monotonically increasing, but can have gaps.\n> > > When you create a new frame context you increase by one to get the\n> > > next slot, but when you get an existing frame you compute the index\n> > > by doing frame % kMaxFrameContexts\n> > >\n> > > IPAFrameContext *FCQueue::get(uint32_t frame) {\n> > >\n> > > if (frame <= tail_->frame)\n> > > /* GET */\n> > > IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > else\n> > > /* PUSH */\n> > > tail_ = &this->at((tail_->frame + 1) % kMaxFrameContexts);\n> > > tail_->frame = frame;\n> > >\n> > > Isn't there a risk you get the wrong frame ?\n> >\n> >\n> > Yes, I've realized I have made an error here after posting to the list.\n> > I put the IPAFrameContext on the next slot immediate to tail_ , but that's\n> > not correct. It's only correct if we ensure there are not any gaps (majority\n> > of the\n> > development has been done by assuming that there will not be gaps for now).\n> >\n> > Gaps / Frame drops is yet another beast to handle. I guess I am keeping it\n> > separate\n> > from the \"per-frame controls\" planning for now. I discussed the same with\n> > Kieran\n> > the other day - that the frame-drop handling and per-frame controls need to\n> > kept\n> > separate for progress. Otherwise both half-baked designs, trying to\n> > satisfying/handle\n> > each other arbitrarily  might create chicken-and-egg problems.\n> \n> Frame drops had been biting us hard enough already. I think we should\n> take them into account from the beginning in order to avoid having to\n> re-think how to handle them later..\n> \n> That's why I think we need to define how mapping the frame number to\n> the intended slot and do so uniquely in both \"get\" and \"push\". The\n> current \"frame % kMaxFrameContexts\" is enough (it will only cause\n> problems if the number of dropped frames in a kMaxFrameContexts period\n> is larger than the queue depth I think).\n\nYes, a missed frame should still consume it's slot. Lets not try to keep\nall slots used - that will get a little messy I think. Just indexing the\nslot based on the frame should be sufficient - if we get more than\nkMaxFrameContexts of frame drop - then we have bigger issues and things\nwill be quite wrong (in PFC terms) but if we can detect it, we should\nset any PFC error flag (and frame/request underrun flag?) and continue.\n\n> >\n> > >\n> > > (also being this a LIFO, isn't the head the most recent and the\n> \n> Sorry, I clearly meant FIFO\n> \n> > > tail the oldest entry ? you seem to advance tail when you push a\n> > > new frame)\n> >\n> >\n> > No, head_ represents the oldest entry and tail_ represents the latest\n> > IPAFrameContext\n> > that got created (refer documentation in this patch).\n> >\n> \n> Mine was mostly a comment on the general concept, not as much as the\n> documentation of what you've done :) My thought was that in a stack\n> (LIFO) you pop the head and in a queue (FIFO) you pop the tail (and\n> consequentlly adavance the head when you push a new context).\n> \n> Basically this:\n> https://softwareengineering.stackexchange.com/questions/144477/on-a-queue-which-end-is-the-head\n> \n> However STL seems to use \"front\" and \"back\" for queues [2] as\n> synonymous for your head and tail, so I guess it's fine the way you\n> have it (you could use \"front\" and \"back\" to stay closer to STL)\n> \n> [2] https://en.cppreference.com/w/cpp/container/queue\n> \n> > >\n> > > 2 ) tail_ gets initialized with an empty IPAFrameContext whose constructor\n> > > is\n> > > IPAFrameContext::IPAFrameContext() = default;\n> > >\n> > > hence its frame id is not intialized, or are POD types default\n> > > initialized in C++ ?\n> >\n> >\n> > We manually give it a value of '0' in clear(). It should work out from\n> > there.\n> \n> Right. Again careful that if frame numbers are numbered using the\n> CSI-2 frame number, it will count from 1.\n\nWhere have you got this '1' from ? Is that 'just' the IPU3 CIO2\nreceiver?\n\nIt's precisely why I created the patch to V4L2VideoDevice to make all\ndevices consistent.\n\n> > The .get() calls will over-write the frame number while creating the\n> > IPAFrameContext(s),\n> > so I don't think it will be a problem as such..\n> >\n> > >\n> > > Anyway, isn't it simpler to use plain counters for head and tail instead\n> > > of pointers to the contained objects ? (This would maybe make\n> > > complicated to generalize the libcamera::RingBuffer implementation\n> > > maybe), but head_ and tail_ mainly serve for two purpose:\n> > > - underflow detection (trying to complete a frame not yet queued)\n> > > - overflow detection (trying to queue a frame overwrites a\n> > > not-yet-consumed one)\n> >\n> >\n> > Well, the main point for having these pointers is to know if a\n> > IPAFrameContext exists in the first\n> > place or not. The underflow/overflow checks comes later (... and we need\n> \n> Well, you have this to find out if a frame context is the \"right\" one\n> \n> IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> if (frame != frameContext.frame)\n> LOG(IPAIPU3, Warning)\n> << \"Got wrong frame context for frame \" << frame;\n> \n> But the distinction between a \"push\" and a \"get\" is just\n> \n> if (frame <= tail_->frame)\n> \n> which could be very well realized with integers.\n\nA frame count in the tail and head variables could indeed be easy and\nclear enough. Probably easier all round as you don't have to care for\nwrap around.\n\n\n> \n> Anyway, just a suggestion to have the implementation possibly simpler\n> \n> \n> > have to a\n> > additional head_ for that, otherwise simply a tail_ would have been\n> > sufficient for the former)\n> >\n> > I agree with you that plain counters would be sufficient (and it doesn't\n> > really make a difference\n> > to me, either you store pointers OR frame numbers of oldest's and latest's).\n> > It is just storing\n> > a unique handles somehow.\n> >\n> > >\n> > > Can't we have head and tail simply follow the latest frame number that\n> > > have been queued and the lastest frame number that has been consumed\n> > > respectively ?\n> > >\n> > > Collision detection will simply be\n> > > - undeflow: tail + 1 == head\n> >\n> >\n> > rather should be head + 1 == tail (according to how tail and head are used\n> > in this patch)\n> \n> Yeah, I was using the notion I had in my head. Must be biased by the\n> dog metaphore in the link above :)\n> \n> >\n> > > - overflow (queue frame): head - tail == array size\n> > >\n> > > The actual position on the array can always be computed as (frame %\n> > > kMaxFrameContexts)\n> > >\n> > > This doesn't however work well with gaps, as one frame gap means we're\n> > > actually not using one slot, so a little space is wasted. Ofc as the\n> > > number of gaps approaches the number of available slots, the risk of\n> > > overflow increases. But gaps should be an exceptional events and with\n> > > large enough buffers this shouldn't be a problem ?\n> >\n> >\n> > To be honest, I don't think storing IPAFrameContext pointers vs frame\n> > numbers\n> > should be a major concern. I say this because intrinsically everything\n> > revolves\n> > around the frame number in both cases.\n> >\n> > Going forward (and it's only my educated guess), storing head and tail frame\n> > numbers will get limiting a bit. I need to see how Kieran's work on\n> > merging/retaining\n> > controls is going on. The idea is controls from latest IPAFrameContext gets\n> > retained\n> > into next-created IPAFrameContext(s) and so on... If you think about it, the\n> > tail_->controls\n> > will get copied into the next IPAFrameContext while being created. In cases\n> > like this,\n> > having IPAFrameContexts pointers are useful in my opinion.\n> >\n> \n> I don't think that's an issue as head and tail will simply allow you\n> get the context and from there you can do whatever you want.\n> Similarly, from the frame context you can get the frame number, so\n> yes, whatever you prefer for now\n> \n> > >\n> > > Also I wonder if a push/get interface wouldn't be simpler, with new\n> > > reuests being pushed() and frames consumed with get(), but that's more\n> > > an implementation detail maybe..\n> >\n> >\n> > I do wondered that as well, but in my opinion having a push() + get()\n> > interface also means,\n> > each algorithm has to do various checks on their part. For e.g.\n> >\n> > Algorithm1:\n> >\n> >     .get(X)  <--- Failed\n> >     .push(XFrameContext)\n> >     .get(X) <---- Success and carry on\n> >\n> > Algorithm2:\n> >\n> >     .get(X) <-- Success because Algorithm1 created XFrameContext\n> >     // but you still need to write failure path here containing\n> > .push(XFrameContext)\n> >\n> > .. and so on.\n> >\n> > Also, various Algorithm might need to create different frame's\n> > IPAFrameContext in a single run,\n> > depending on their latencies.\n> >\n> > So I think we are unnecessarily outsourcing the responsibility of \"pushing\"\n> > the IPAFrameContext\n> > to the algorithms. All this can be encapsulated in the .get(), no? I thought\n> > we all agreed on\n> > the .get() design, so hence this direction.\n> >\n\nYes, a '.get()' should certainly be available to algorithms to 'get' the\nFC based on an index. The question here seems to be around 'what happens\nif the algorithm 'gets' the slot - and it hadn't yet been filled in by\nthe queue request call.\n\nWell. ... first - the .get() call should hopefully know that ... and\nwill have to set the PFC error / underrun flag.\n\nBut we may have to see what data is expected to be referenced, as it may\nbe 'uninitialised'. But this is where we really need to be clear on what\nstate is in the FCQ vs in the algorithm itself. To me the FCQ is\ngoing to store 'commands' (i.e. controls). There will be some state\naround the algorithm, but I expect that there should also be private\nstate for the algorithm for auto controls too - so it should be able to\nremember any current filter positions or such ...\n\nI'm certain I'm not explaining what's in my head above clearly there -\nso dig deeper and push me where it's not clear and I'll try to reword or\ndraw something up (which will be part of the PFC design doc I'm working\non anyway).\n\nBut tl;dr; - I believe an algorithm should still be able to run if there\nis no FCQ slot filled with a request - but it should be an error state.\n\n\n\n> In case of requests underrun the algorithms would try to access a\n> non-existing frame context, hence their first \"get()\" will have to\n> create one. Indeed having a get that creates entries blurries the line\n> between a push and a get enough to justify using a single function.\n> \n> > >\n> > > IPA components cannot have tests right ? It would be nice to have a\n> > > unit test for this component to make sure it behaves as intended\n> > > instead of having to debug it \"live\"\n> >\n> >\n> > I see that you have mentioned a desire to make a generic\n> > libcamera::RingBuffer\n> > class implementation. While I do agree on the point of tests, but I am not\n> > looking\n> > at this angle just yet. It feels to me a bit early to do so because things\n> > can be very\n> > specific w.r.t IPAs. Even if we do it \"generically\" right now, only IPU3\n> > would be the\n> > only user I think. There are other similar parts that we want to provide a\n> > generic\n> > implementation to all the IPAs for e.g. IPAContext, IPAFrameContexts and so\n> > on,\n> > so probably once we have a good usage across multiple IPAs, we would be in a\n> > better position to write a generic ring buffer implementation then and adapt\n> > the IPAs on top of it?\n> >\n> \n> As I said I was thinking about other components outside of the IPAs\n> that could benefit from this as well. But I agree getting to have a\n> first user is the first step to later eventually generalize.\n> \n> > >\n> > > sorry, a lot of questions actually..\n> >\n> >\n> > No issues ;-)\n> >\n> \n> So it seems to me that the only remaining concerns are actually:\n> \n> 1) Map uniquely the frame number to the slot index\n\nYes, a frame number and a slot in the FCQ should be directly mappable.\n\n> \n> 2) Clarify if it can be assumed frames always start from 0. This is\n> desirable as in the current model where frames start being\n> produced when enough buffers have been queued, the FCQ will be\n> filled with requests numbered from 0 on, while the first frame\n> could potentially have an arbitrary value. Is that handled by\n> Kieran's series from december ? (sorry, I lost track of all the\n> moving parts).\n\nYes, I believe we work on the assumption all frames start at zero (after\nany call to start(), so a stop() / start() resets to Frame/Request 0.\n\n\n--\nKieran\n\n\n\n\n> \n> Thanks\n> j\n> \n> > >\n> > > > + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > > + if (frame != frameContext.frame)\n> > > > + LOG(IPAIPU3, Warning)\n> > > > + << \"Got wrong frame context for frame \" << frame;\n> > > > +\n> > > > + return &frameContext;\n> > > > + } else {\n> > > > + if (isFull_) {\n> > > > + LOG(IPAIPU3, Warning)\n> > > > + << \"Cannot create frame context for frame \" << frame\n> > > > + << \" since FCQueue is full\";\n> > > > +\n> > > > + return nullptr;\n> > > > + }\n> > > > +\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> > > > +\n> > > > + /* Check if the queue is full to avoid over-queueing later */\n> > > > + if (tail_->frame - head_->frame >= kMaxFrameContexts - 1)\n> > > > + isFull_ = true;\n> > > >\n> > > > - if (frame != frameContext.frame) {\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> > > >\n> > > > - return &frameContext;\n> > > > + head_ = this->get(frame + 1);\n> > > > +\n> > > > + if (isFull_)\n> > > > + isFull_ = false;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\fn FCQueue::isFull()\n> > > > + * \\brief Checks whether the frame context queue is full\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n> > > > */\n> > > > @@ -260,6 +334,13 @@ void FCQueue::clear()\n> > > > {\n> > > > IPAFrameContext initFrameContext;\n> > > > this->fill(initFrameContext);\n> > > > +\n> > > > + isFull_ = false;\n> > > > +\n> > > > + /* Intialise 0th index to frame 0 */\n> > > > + this->at(0).frame = 0;\n> > > > + tail_ = &this->at(0);\n> > > > + head_ = 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 56d281f6..475855da 100644\n> > > > --- a/src/ipa/ipu3/ipa_context.h\n> > > > +++ b/src/ipa/ipu3/ipa_context.h\n> > > > @@ -93,9 +93,18 @@ class FCQueue : public std::array\n> > > > {\n> > > > public:\n> > > > FCQueue();\n> > > > + FCQueue &operator[](const FCQueue &) = delete;\n> > > >\n> > > > void clear();\n> > > > + void completeFrame(uint32_t frame);\n> > > > IPAFrameContext *get(uint32_t frame);\n> > > > + bool isFull() { return isFull_; }\n> > > > +\n> > > > +private:\n> > > > + IPAFrameContext *head_;\n> > > > + IPAFrameContext *tail_;\n> > > > +\n> > > > + bool isFull_;\n> > > > };\n> > > >\n> > > > struct IPAContext {\n> > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > index 0843d882..1d6ee515 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> > > > */\n> > > >\n> > > > metadataReady.emit(frame, ctrls);\n> > > > +\n> > > > + context_.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> > > > /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> > > > + *context_.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 BB7E5BD160\n\tfor ;\n\tFri, 10 Jun 2022 19:20:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB06165632;\n\tFri, 10 Jun 2022 21:20: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 1E260633A4\n\tfor ;\n\tFri, 10 Jun 2022 21:20:35 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9BE8A2F3;\n\tFri, 10 Jun 2022 21:20:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654888835;\n\tbh=JMS06lo62boJ2e7OY8V0a8lXpoH/5xvMlWkroUF2QhM=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=O5vQajkk9TNuNyI0sclS44GWL4wtZqjD+Nq/OLQLtyswNiqCvRtQ415newzx0vB9g\n\tVAhEC8FqkuLiVtaqcw9lman9u9AheuD7iPkDA7Twjboixx1R/2bxW18FYIbjqivFwW\n\t1h3DR1qvgvzPVBirDIPMWNBl1mpZj4HcPbEG0EVTMtn1LlyPZgpKeWnW68tQPN8/SR\n\tbONjDm67aAiQttrWBWmJLzxXjHaErF9qXZF+i4supvIsHxPjLFyU7LVzlWZCVqvv4y\n\t5bzNl+aeXI5e3uTDbHDBCiG4fAvGrKLG3wvP81rpy29x938Eosf2n36Wa/WByWeCal\n\tTykuO8EzKBctQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654888834;\n\tbh=JMS06lo62boJ2e7OY8V0a8lXpoH/5xvMlWkroUF2QhM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=jRgTW7d7/hwwEJCcMf9gUVaLlfjjNTTPYhg98fcW0Flap4jbREppA8qvHoOI5cTYD\n\tzI/0z0/Rcrv2WOJpiiIRKetFik9Zf0LcH/Ow6yzG+JruhG2nImZtU3XqF28nwgNrA+\n\tJH0XWX/fz7yO7pTYKAsBPN7V6carayUNarEHfT2c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jRgTW7d7\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220609162054.enj4kswtjtomm5po@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-3-umang.jain@ideasonboard.com>\n\t<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>\n\t<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>\n\t<20220609162054.enj4kswtjtomm5po@uno.localdomain>","To":"Jacopo Mondi , Jacopo Mondi via libcamera-devel\n\t, \n\tUmang Jain ","Date":"Fri, 10 Jun 2022 20:20:32 +0100","Message-ID":"<165488883227.1070686.8210324805195113270@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23387,"web_url":"https://patchwork.libcamera.org/comment/23387/","msgid":"<20220611144009.meewiilill66alcf@uno.localdomain>","date":"2022-06-11T14:40:09","subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::get()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Fri, Jun 10, 2022 at 08:20:32PM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> Quoting Jacopo Mondi via libcamera-devel (2022-06-09 17:20:54)\n> > Hi Umang,\n> >\n> > On Thu, Jun 09, 2022 at 01:09:28AM +0200, Umang Jain wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for the review.\n> > >\n> > > On 6/8/22 15:56, Jacopo Mondi wrote:\n> > > > Hi Umang,\n> > > >\n> > > > On Fri, Jun 03, 2022 at 03:22:57PM +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> > > > >\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 | 95 +++++++++++++++++++++++++++++++++---\n> > > > > src/ipa/ipu3/ipa_context.h | 9 ++++\n> > > > > src/ipa/ipu3/ipu3.cpp | 4 +-\n> > > > > 3 files changed, 100 insertions(+), 8 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > > > > index 9f95a61c..2438d68d 100644\n> > > > > --- a/src/ipa/ipu3/ipa_context.cpp\n> > > > > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > > > > @@ -222,13 +222,33 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\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.\n> > > > > + * to be processed by the IPA at a given point. An 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> > > > > + * \\var FCQueue::isFull_\n> > > > > + * \\brief Flag set when the FCQueue is full\n> > > > > */\n> > > > >\n> > > > > /**\n> > > > > * \\brief FCQueue constructor\n> > > > > */\n> > > > > FCQueue::FCQueue()\n> > > > > + : head_(nullptr), tail_(nullptr), isFull_(false)\n> > > > > {\n> > > > > clear();\n> > > > > }\n> > > > > @@ -238,21 +258,75 @@ FCQueue::FCQueue()\n> > > > > * \\param[in] frame Frame number for which the IPAFrameContext needs to\n> > > > > * retrieved\n> > > > > *\n> > > > > - * \\return Pointer to the IPAFrameContext\n> > > > > + * This function returns the IPAFrameContext for the desired frame. If the\n> > > > > + * frame context does not exist in the queue, the next available slot in the\n> > > > > + * queue is returned. It is the responsibility of the caller to fill the correct\n> > > > > + * IPAFrameContext parameters of newly returned IPAFrameContext.\n> > > > > + *\n> > > > > + * \\return Pointer to the IPAFrameContext or nullptr if frame context couldn't\n> > > > > + * be created\n> > > > > */\n> > > > > IPAFrameContext *FCQueue::get(uint32_t frame)\n> > > > > {\n> > > > > - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > > > + if (frame <= tail_->frame) {\n> > > > How does this work ? The first get() you call after construction has tail_\n> > > > == nullptr. How come this doesn't segfault ? Is it because there's a\n> > > > call to clear() that initializes the pointers before usage ?\n> > > > Shouldn't the constructor take care of creating a usable object\n> > > > without requiring clear() to be called ?\n> > >\n> > >\n> > > Yes, bad naming. I think reset() would be more appropriate? The constructor\n> > > is\n> > > responsible yes (hence it calls clear() in the first place) so it was the\n> >\n> > I missed the call to clear() in the constructor and I thought this\n> > worked because of the clear() call in IPA::configure(). Sorry, this is\n> > ok I guess.\n> >\n> > > matter of\n> > > code-deduplication. We can discuss the naming conventions but tail_\n> > > shouldn't\n> > > be a nullptr before any .get() calls. So I do want proper initialization\n> > > plus, a\n> > > clear() or reset() to clear the ring buffer and resetting the tail_, head_\n> > > etc.\n> > >\n> > > >\n> > > > Also, are we sure using the frame number in tail_ and head_ is the best\n> > > > way to ensure that we actually track where we are in the queue ?\n> > > >\n> > > > I have a few worries:\n> > > >\n> > > > 1) are frame numbers always starting from 0 ?\n> > >\n> > >\n> > > It should be, no?\n> > >\n> >\n> > I would say that it depends on how the kernel driver counts the frame\n> > numbers. If the MIPI CSI-2 frame number is used it will count from 1\n> > and increment per-virtual channel. If the driver maintains an internal\n> > counter it should be reset at every start_stream/stop_stream session,\n> > but I don't this there are guarantees if not looking at the driver and\n> > making sure it does the right thing ?\n> >\n> > However I now also recall Kieran had a patch on his FrameSequence\n> > series to restart counting from 0 the frame sequence number in\n> > libcamera, do we assume they will always start from 0 because of this?\n> >\n>\n> Yes, I have a patch that will ensure frame numbers always start at zero\n> from the V4L2VideoDevice.\n>\n> I think this is important to keep things consistent, regardless of what\n> the kernel does or doesn't do correctly.. So the first frame from the\n> CSI2 receiver should always be zero from any call to start().\n>\n> We have it in development branches, but I don't think I've posted it to\n> the list yet. I'll try to make sure I do that on Monday, but you'll\n> probably find it in the code camp development branch before that if\n> you're interested.\n>\n>\n>\n> > > > 2) frame numbers are monotonically increasing, but can have gaps.\n> > > > When you create a new frame context you increase by one to get the\n> > > > next slot, but when you get an existing frame you compute the index\n> > > > by doing frame % kMaxFrameContexts\n> > > >\n> > > > IPAFrameContext *FCQueue::get(uint32_t frame) {\n> > > >\n> > > > if (frame <= tail_->frame)\n> > > > /* GET */\n> > > > IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > > else\n> > > > /* PUSH */\n> > > > tail_ = &this->at((tail_->frame + 1) % kMaxFrameContexts);\n> > > > tail_->frame = frame;\n> > > >\n> > > > Isn't there a risk you get the wrong frame ?\n> > >\n> > >\n> > > Yes, I've realized I have made an error here after posting to the list.\n> > > I put the IPAFrameContext on the next slot immediate to tail_ , but that's\n> > > not correct. It's only correct if we ensure there are not any gaps (majority\n> > > of the\n> > > development has been done by assuming that there will not be gaps for now).\n> > >\n> > > Gaps / Frame drops is yet another beast to handle. I guess I am keeping it\n> > > separate\n> > > from the \"per-frame controls\" planning for now. I discussed the same with\n> > > Kieran\n> > > the other day - that the frame-drop handling and per-frame controls need to\n> > > kept\n> > > separate for progress. Otherwise both half-baked designs, trying to\n> > > satisfying/handle\n> > > each other arbitrarily  might create chicken-and-egg problems.\n> >\n> > Frame drops had been biting us hard enough already. I think we should\n> > take them into account from the beginning in order to avoid having to\n> > re-think how to handle them later..\n> >\n> > That's why I think we need to define how mapping the frame number to\n> > the intended slot and do so uniquely in both \"get\" and \"push\". The\n> > current \"frame % kMaxFrameContexts\" is enough (it will only cause\n> > problems if the number of dropped frames in a kMaxFrameContexts period\n> > is larger than the queue depth I think).\n>\n> Yes, a missed frame should still consume it's slot. Lets not try to keep\n> all slots used - that will get a little messy I think. Just indexing the\n> slot based on the frame should be sufficient - if we get more than\n> kMaxFrameContexts of frame drop - then we have bigger issues and things\n> will be quite wrong (in PFC terms) but if we can detect it, we should\n> set any PFC error flag (and frame/request underrun flag?) and continue.\n>\n> > >\n> > > >\n> > > > (also being this a LIFO, isn't the head the most recent and the\n> >\n> > Sorry, I clearly meant FIFO\n> >\n> > > > tail the oldest entry ? you seem to advance tail when you push a\n> > > > new frame)\n> > >\n> > >\n> > > No, head_ represents the oldest entry and tail_ represents the latest\n> > > IPAFrameContext\n> > > that got created (refer documentation in this patch).\n> > >\n> >\n> > Mine was mostly a comment on the general concept, not as much as the\n> > documentation of what you've done :) My thought was that in a stack\n> > (LIFO) you pop the head and in a queue (FIFO) you pop the tail (and\n> > consequentlly adavance the head when you push a new context).\n> >\n> > Basically this:\n> > https://softwareengineering.stackexchange.com/questions/144477/on-a-queue-which-end-is-the-head\n> >\n> > However STL seems to use \"front\" and \"back\" for queues [2] as\n> > synonymous for your head and tail, so I guess it's fine the way you\n> > have it (you could use \"front\" and \"back\" to stay closer to STL)\n> >\n> > [2] https://en.cppreference.com/w/cpp/container/queue\n> >\n> > > >\n> > > > 2 ) tail_ gets initialized with an empty IPAFrameContext whose constructor\n> > > > is\n> > > > IPAFrameContext::IPAFrameContext() = default;\n> > > >\n> > > > hence its frame id is not intialized, or are POD types default\n> > > > initialized in C++ ?\n> > >\n> > >\n> > > We manually give it a value of '0' in clear(). It should work out from\n> > > there.\n> >\n> > Right. Again careful that if frame numbers are numbered using the\n> > CSI-2 frame number, it will count from 1.\n>\n> Where have you got this '1' from ? Is that 'just' the IPU3 CIO2\n\nFrom the CSI-2 specification.\n\nI admit I have an old version: \"Version 1.01.00 r0.04 2-Apr-2009\"\nSection 9.8.1 \"Frame Synchonization Packets\"\n\n------------------------------------------------------------------------------\nFor FS and FE synchronization packets the Short Packet Data Field\nshall contain a 16-bit frame number. This frame number shall be the\nsame for the FS and FE synchronization packets corresponding to a\ngiven frame.\n\nThe 16-bit frame number, when used, shall be non-zero to distinguish\nit from the use-case where frame number is inoperative and remains set\nto zero.\n\nThe behavior of the 16-bit frame number shall be as one of the\nfollowing\n•Frame number is always zero – frame number is inoperative.\n•Frame number increments by 1 for every FS packet with the same\n Virtual Channel and is periodically reset to one e.g. 1, 2, 1, 2, 1,\n 2, 1, 2 or 1, 2, 3, 4, 1, 2, 3, 4\n------------------------------------------------------------------------------\n\n> receiver?\n>\n\nAs far as I can see IPU3, RPi and RKISP1 use a counter and not the\nCSI-2 frame number\n\n> It's precisely why I created the patch to V4L2VideoDevice to make all\n> devices consistent.\n>\n> > > The .get() calls will over-write the frame number while creating the\n> > > IPAFrameContext(s),\n> > > so I don't think it will be a problem as such..\n> > >\n> > > >\n> > > > Anyway, isn't it simpler to use plain counters for head and tail instead\n> > > > of pointers to the contained objects ? (This would maybe make\n> > > > complicated to generalize the libcamera::RingBuffer implementation\n> > > > maybe), but head_ and tail_ mainly serve for two purpose:\n> > > > - underflow detection (trying to complete a frame not yet queued)\n> > > > - overflow detection (trying to queue a frame overwrites a\n> > > > not-yet-consumed one)\n> > >\n> > >\n> > > Well, the main point for having these pointers is to know if a\n> > > IPAFrameContext exists in the first\n> > > place or not. The underflow/overflow checks comes later (... and we need\n> >\n> > Well, you have this to find out if a frame context is the \"right\" one\n> >\n> > IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > if (frame != frameContext.frame)\n> > LOG(IPAIPU3, Warning)\n> > << \"Got wrong frame context for frame \" << frame;\n> >\n> > But the distinction between a \"push\" and a \"get\" is just\n> >\n> > if (frame <= tail_->frame)\n> >\n> > which could be very well realized with integers.\n>\n> A frame count in the tail and head variables could indeed be easy and\n> clear enough. Probably easier all round as you don't have to care for\n> wrap around.\n>\n>\n> >\n> > Anyway, just a suggestion to have the implementation possibly simpler\n> >\n> >\n> > > have to a\n> > > additional head_ for that, otherwise simply a tail_ would have been\n> > > sufficient for the former)\n> > >\n> > > I agree with you that plain counters would be sufficient (and it doesn't\n> > > really make a difference\n> > > to me, either you store pointers OR frame numbers of oldest's and latest's).\n> > > It is just storing\n> > > a unique handles somehow.\n> > >\n> > > >\n> > > > Can't we have head and tail simply follow the latest frame number that\n> > > > have been queued and the lastest frame number that has been consumed\n> > > > respectively ?\n> > > >\n> > > > Collision detection will simply be\n> > > > - undeflow: tail + 1 == head\n> > >\n> > >\n> > > rather should be head + 1 == tail (according to how tail and head are used\n> > > in this patch)\n> >\n> > Yeah, I was using the notion I had in my head. Must be biased by the\n> > dog metaphore in the link above :)\n> >\n> > >\n> > > > - overflow (queue frame): head - tail == array size\n> > > >\n> > > > The actual position on the array can always be computed as (frame %\n> > > > kMaxFrameContexts)\n> > > >\n> > > > This doesn't however work well with gaps, as one frame gap means we're\n> > > > actually not using one slot, so a little space is wasted. Ofc as the\n> > > > number of gaps approaches the number of available slots, the risk of\n> > > > overflow increases. But gaps should be an exceptional events and with\n> > > > large enough buffers this shouldn't be a problem ?\n> > >\n> > >\n> > > To be honest, I don't think storing IPAFrameContext pointers vs frame\n> > > numbers\n> > > should be a major concern. I say this because intrinsically everything\n> > > revolves\n> > > around the frame number in both cases.\n> > >\n> > > Going forward (and it's only my educated guess), storing head and tail frame\n> > > numbers will get limiting a bit. I need to see how Kieran's work on\n> > > merging/retaining\n> > > controls is going on. The idea is controls from latest IPAFrameContext gets\n> > > retained\n> > > into next-created IPAFrameContext(s) and so on... If you think about it, the\n> > > tail_->controls\n> > > will get copied into the next IPAFrameContext while being created. In cases\n> > > like this,\n> > > having IPAFrameContexts pointers are useful in my opinion.\n> > >\n> >\n> > I don't think that's an issue as head and tail will simply allow you\n> > get the context and from there you can do whatever you want.\n> > Similarly, from the frame context you can get the frame number, so\n> > yes, whatever you prefer for now\n> >\n> > > >\n> > > > Also I wonder if a push/get interface wouldn't be simpler, with new\n> > > > reuests being pushed() and frames consumed with get(), but that's more\n> > > > an implementation detail maybe..\n> > >\n> > >\n> > > I do wondered that as well, but in my opinion having a push() + get()\n> > > interface also means,\n> > > each algorithm has to do various checks on their part. For e.g.\n> > >\n> > > Algorithm1:\n> > >\n> > >     .get(X)  <--- Failed\n> > >     .push(XFrameContext)\n> > >     .get(X) <---- Success and carry on\n> > >\n> > > Algorithm2:\n> > >\n> > >     .get(X) <-- Success because Algorithm1 created XFrameContext\n> > >     // but you still need to write failure path here containing\n> > > .push(XFrameContext)\n> > >\n> > > .. and so on.\n> > >\n> > > Also, various Algorithm might need to create different frame's\n> > > IPAFrameContext in a single run,\n> > > depending on their latencies.\n> > >\n> > > So I think we are unnecessarily outsourcing the responsibility of \"pushing\"\n> > > the IPAFrameContext\n> > > to the algorithms. All this can be encapsulated in the .get(), no? I thought\n> > > we all agreed on\n> > > the .get() design, so hence this direction.\n> > >\n>\n> Yes, a '.get()' should certainly be available to algorithms to 'get' the\n> FC based on an index. The question here seems to be around 'what happens\n> if the algorithm 'gets' the slot - and it hadn't yet been filled in by\n> the queue request call.\n>\n> Well. ... first - the .get() call should hopefully know that ... and\n> will have to set the PFC error / underrun flag.\n>\n> But we may have to see what data is expected to be referenced, as it may\n> be 'uninitialised'. But this is where we really need to be clear on what\n> state is in the FCQ vs in the algorithm itself. To me the FCQ is\n> going to store 'commands' (i.e. controls). There will be some state\n> around the algorithm, but I expect that there should also be private\n> state for the algorithm for auto controls too - so it should be able to\n> remember any current filter positions or such ...\n>\n> I'm certain I'm not explaining what's in my head above clearly there -\n> so dig deeper and push me where it's not clear and I'll try to reword or\n> draw something up (which will be part of the PFC design doc I'm working\n> on anyway).\n>\n> But tl;dr; - I believe an algorithm should still be able to run if there\n> is no FCQ slot filled with a request - but it should be an error state.\n>\n>\n>\n> > In case of requests underrun the algorithms would try to access a\n> > non-existing frame context, hence their first \"get()\" will have to\n> > create one. Indeed having a get that creates entries blurries the line\n> > between a push and a get enough to justify using a single function.\n> >\n> > > >\n> > > > IPA components cannot have tests right ? It would be nice to have a\n> > > > unit test for this component to make sure it behaves as intended\n> > > > instead of having to debug it \"live\"\n> > >\n> > >\n> > > I see that you have mentioned a desire to make a generic\n> > > libcamera::RingBuffer\n> > > class implementation. While I do agree on the point of tests, but I am not\n> > > looking\n> > > at this angle just yet. It feels to me a bit early to do so because things\n> > > can be very\n> > > specific w.r.t IPAs. Even if we do it \"generically\" right now, only IPU3\n> > > would be the\n> > > only user I think. There are other similar parts that we want to provide a\n> > > generic\n> > > implementation to all the IPAs for e.g. IPAContext, IPAFrameContexts and so\n> > > on,\n> > > so probably once we have a good usage across multiple IPAs, we would be in a\n> > > better position to write a generic ring buffer implementation then and adapt\n> > > the IPAs on top of it?\n> > >\n> >\n> > As I said I was thinking about other components outside of the IPAs\n> > that could benefit from this as well. But I agree getting to have a\n> > first user is the first step to later eventually generalize.\n> >\n> > > >\n> > > > sorry, a lot of questions actually..\n> > >\n> > >\n> > > No issues ;-)\n> > >\n> >\n> > So it seems to me that the only remaining concerns are actually:\n> >\n> > 1) Map uniquely the frame number to the slot index\n>\n> Yes, a frame number and a slot in the FCQ should be directly mappable.\n>\n> >\n> > 2) Clarify if it can be assumed frames always start from 0. This is\n> > desirable as in the current model where frames start being\n> > produced when enough buffers have been queued, the FCQ will be\n> > filled with requests numbered from 0 on, while the first frame\n> > could potentially have an arbitrary value. Is that handled by\n> > Kieran's series from december ? (sorry, I lost track of all the\n> > moving parts).\n>\n> Yes, I believe we work on the assumption all frames start at zero (after\n> any call to start(), so a stop() / start() resets to Frame/Request 0.\n\nCould you have a look at my other reply about the fact we use request\nnumbers as frame numbers ?\n\nThanks\n j\n\n>\n>\n> --\n> Kieran\n>\n>\n>\n>\n> >\n> > Thanks\n> > j\n> >\n> > > >\n> > > > > + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > > > + if (frame != frameContext.frame)\n> > > > > + LOG(IPAIPU3, Warning)\n> > > > > + << \"Got wrong frame context for frame \" << frame;\n> > > > > +\n> > > > > + return &frameContext;\n> > > > > + } else {\n> > > > > + if (isFull_) {\n> > > > > + LOG(IPAIPU3, Warning)\n> > > > > + << \"Cannot create frame context for frame \" << frame\n> > > > > + << \" since FCQueue is full\";\n> > > > > +\n> > > > > + return nullptr;\n> > > > > + }\n> > > > > +\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> > > > > +\n> > > > > + /* Check if the queue is full to avoid over-queueing later */\n> > > > > + if (tail_->frame - head_->frame >= kMaxFrameContexts - 1)\n> > > > > + isFull_ = true;\n> > > > >\n> > > > > - if (frame != frameContext.frame) {\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> > > > >\n> > > > > - return &frameContext;\n> > > > > + head_ = this->get(frame + 1);\n> > > > > +\n> > > > > + if (isFull_)\n> > > > > + isFull_ = false;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\fn FCQueue::isFull()\n> > > > > + * \\brief Checks whether the frame context queue is full\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n> > > > > */\n> > > > > @@ -260,6 +334,13 @@ void FCQueue::clear()\n> > > > > {\n> > > > > IPAFrameContext initFrameContext;\n> > > > > this->fill(initFrameContext);\n> > > > > +\n> > > > > + isFull_ = false;\n> > > > > +\n> > > > > + /* Intialise 0th index to frame 0 */\n> > > > > + this->at(0).frame = 0;\n> > > > > + tail_ = &this->at(0);\n> > > > > + head_ = 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 56d281f6..475855da 100644\n> > > > > --- a/src/ipa/ipu3/ipa_context.h\n> > > > > +++ b/src/ipa/ipu3/ipa_context.h\n> > > > > @@ -93,9 +93,18 @@ class FCQueue : public std::array\n> > > > > {\n> > > > > public:\n> > > > > FCQueue();\n> > > > > + FCQueue &operator[](const FCQueue &) = delete;\n> > > > >\n> > > > > void clear();\n> > > > > + void completeFrame(uint32_t frame);\n> > > > > IPAFrameContext *get(uint32_t frame);\n> > > > > + bool isFull() { return isFull_; }\n> > > > > +\n> > > > > +private:\n> > > > > + IPAFrameContext *head_;\n> > > > > + IPAFrameContext *tail_;\n> > > > > +\n> > > > > + bool isFull_;\n> > > > > };\n> > > > >\n> > > > > struct IPAContext {\n> > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > > index 0843d882..1d6ee515 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> > > > > */\n> > > > >\n> > > > > metadataReady.emit(frame, ctrls);\n> > > > > +\n> > > > > + context_.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> > > > > /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > > > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> > > > > + *context_.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 A6CA8BD808\n\tfor ;\n\tSat, 11 Jun 2022 14:40:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A56C865635;\n\tSat, 11 Jun 2022 16:40:15 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99A7161FB4\n\tfor ;\n\tSat, 11 Jun 2022 16:40:13 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 94F3F200004;\n\tSat, 11 Jun 2022 14:40:11 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654958415;\n\tbh=iw1oVisJqkVa9Xzg2yhFcZZx13AkjqPAc+57vaZmukw=;\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=bavEoZzFZjlPF+5aWf3AUkZwe5jUKbg+Qbw5HMzhh4KynnZTc6/ilBcNWAO8pJZSU\n\tXcSE2pPduiBqLJORoDJoxePpPOYaf1iQEP9wlZINvrjvClTPpPqVYCTQdE3W0qTxMi\n\tVhd0X3wLUXefUYd7dg5LE4SKSFYwMn0wAT8I+YbZQEc0iJ6sIGkxaB4eZ3E4AUMY99\n\taBNsD5Yq1oIzoMXmJ9EPhANEnROW9bx3eDFL9LJhCijT+We8aeNFrcjvHRfkDRS7LY\n\txnP49ScLk1xcx9YQW6SqZkMCzd8KQiyZLsZXVObm2VhuxJvLZgtuKB/Hv07PaVJ//C\n\taIPgtwe1kxKdA==","Date":"Sat, 11 Jun 2022 16:40:09 +0200","To":"Kieran Bingham ","Message-ID":"<20220611144009.meewiilill66alcf@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-3-umang.jain@ideasonboard.com>\n\t<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>\n\t<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>\n\t<20220609162054.enj4kswtjtomm5po@uno.localdomain>\n\t<165488883227.1070686.8210324805195113270@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<165488883227.1070686.8210324805195113270@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23402,"web_url":"https://patchwork.libcamera.org/comment/23402/","msgid":"<7aac5ed3-bdcf-ae7d-016c-1641a9865bd2@ideasonboard.com>","date":"2022-06-13T14:39:59","subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::get()","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang, Jacopo, Kieran,\n\nOn 11/06/2022 16:40, Jacopo Mondi via libcamera-devel wrote:\n> Hi Kieran\n> \n> On Fri, Jun 10, 2022 at 08:20:32PM +0100, Kieran Bingham wrote:\n>> Hi Jacopo,\n>>\n>> Quoting Jacopo Mondi via libcamera-devel (2022-06-09 17:20:54)\n>>> Hi Umang,\n>>>\n>>> On Thu, Jun 09, 2022 at 01:09:28AM +0200, Umang Jain wrote:\n>>>> Hi Jacopo,\n>>>>\n>>>> Thanks for the review.\n>>>>\n>>>> On 6/8/22 15:56, Jacopo Mondi wrote:\n>>>>> Hi Umang,\n>>>>>\n>>>>> On Fri, Jun 03, 2022 at 03:22:57PM +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>>>>>>\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 | 95 +++++++++++++++++++++++++++++++++---\n>>>>>> src/ipa/ipu3/ipa_context.h | 9 ++++\n>>>>>> src/ipa/ipu3/ipu3.cpp | 4 +-\n>>>>>> 3 files changed, 100 insertions(+), 8 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>>>>>> index 9f95a61c..2438d68d 100644\n>>>>>> --- a/src/ipa/ipu3/ipa_context.cpp\n>>>>>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>>>>>> @@ -222,13 +222,33 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\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.\n>>>>>> + * to be processed by the IPA at a given point. An 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>>>>>> + * \\var FCQueue::isFull_\n>>>>>> + * \\brief Flag set when the FCQueue is full\n>>>>>> */\n>>>>>>\n>>>>>> /**\n>>>>>> * \\brief FCQueue constructor\n>>>>>> */\n>>>>>> FCQueue::FCQueue()\n>>>>>> + : head_(nullptr), tail_(nullptr), isFull_(false)\n>>>>>> {\n>>>>>> clear();\n>>>>>> }\n>>>>>> @@ -238,21 +258,75 @@ FCQueue::FCQueue()\n>>>>>> * \\param[in] frame Frame number for which the IPAFrameContext needs to\n>>>>>> * retrieved\n>>>>>> *\n>>>>>> - * \\return Pointer to the IPAFrameContext\n>>>>>> + * This function returns the IPAFrameContext for the desired frame. If the\n>>>>>> + * frame context does not exist in the queue, the next available slot in the\n>>>>>> + * queue is returned. It is the responsibility of the caller to fill the correct\n>>>>>> + * IPAFrameContext parameters of newly returned IPAFrameContext.\n>>>>>> + *\n>>>>>> + * \\return Pointer to the IPAFrameContext or nullptr if frame context couldn't\n>>>>>> + * be created\n>>>>>> */\n>>>>>> IPAFrameContext *FCQueue::get(uint32_t frame)\n>>>>>> {\n>>>>>> - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>>>>>> + if (frame <= tail_->frame) {\n>>>>> How does this work ? The first get() you call after construction has tail_\n>>>>> == nullptr. How come this doesn't segfault ? Is it because there's a\n>>>>> call to clear() that initializes the pointers before usage ?\n>>>>> Shouldn't the constructor take care of creating a usable object\n>>>>> without requiring clear() to be called ?\n>>>>\n>>>>\n>>>> Yes, bad naming. I think reset() would be more appropriate? The constructor\n>>>> is\n>>>> responsible yes (hence it calls clear() in the first place) so it was the\n>>>\n>>> I missed the call to clear() in the constructor and I thought this\n>>> worked because of the clear() call in IPA::configure(). Sorry, this is\n>>> ok I guess.\n>>>\n>>>> matter of\n>>>> code-deduplication. We can discuss the naming conventions but tail_\n>>>> shouldn't\n>>>> be a nullptr before any .get() calls. So I do want proper initialization\n>>>> plus, a\n>>>> clear() or reset() to clear the ring buffer and resetting the tail_, head_\n>>>> etc.\n>>>>\n>>>>>\n>>>>> Also, are we sure using the frame number in tail_ and head_ is the best\n>>>>> way to ensure that we actually track where we are in the queue ?\n>>>>>\n>>>>> I have a few worries:\n>>>>>\n>>>>> 1) are frame numbers always starting from 0 ?\n>>>>\n>>>>\n>>>> It should be, no?\n>>>>\n>>>\n>>> I would say that it depends on how the kernel driver counts the frame\n>>> numbers. If the MIPI CSI-2 frame number is used it will count from 1\n>>> and increment per-virtual channel. If the driver maintains an internal\n>>> counter it should be reset at every start_stream/stop_stream session,\n>>> but I don't this there are guarantees if not looking at the driver and\n>>> making sure it does the right thing ?\n>>>\n>>> However I now also recall Kieran had a patch on his FrameSequence\n>>> series to restart counting from 0 the frame sequence number in\n>>> libcamera, do we assume they will always start from 0 because of this?\n>>>\n>>\n>> Yes, I have a patch that will ensure frame numbers always start at zero\n>> from the V4L2VideoDevice.\n>>\n>> I think this is important to keep things consistent, regardless of what\n>> the kernel does or doesn't do correctly.. So the first frame from the\n>> CSI2 receiver should always be zero from any call to start().\n>>\n>> We have it in development branches, but I don't think I've posted it to\n>> the list yet. I'll try to make sure I do that on Monday, but you'll\n>> probably find it in the code camp development branch before that if\n>> you're interested.\n>>\n>>\n>>\n>>>>> 2) frame numbers are monotonically increasing, but can have gaps.\n>>>>> When you create a new frame context you increase by one to get the\n>>>>> next slot, but when you get an existing frame you compute the index\n>>>>> by doing frame % kMaxFrameContexts\n>>>>>\n>>>>> IPAFrameContext *FCQueue::get(uint32_t frame) {\n>>>>>\n>>>>> if (frame <= tail_->frame)\n>>>>> /* GET */\n>>>>> IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>>>>> else\n>>>>> /* PUSH */\n>>>>> tail_ = &this->at((tail_->frame + 1) % kMaxFrameContexts);\n>>>>> tail_->frame = frame;\n>>>>>\n>>>>> Isn't there a risk you get the wrong frame ?\n>>>>\n>>>>\n>>>> Yes, I've realized I have made an error here after posting to the list.\n>>>> I put the IPAFrameContext on the next slot immediate to tail_ , but that's\n>>>> not correct. It's only correct if we ensure there are not any gaps (majority\n>>>> of the\n>>>> development has been done by assuming that there will not be gaps for now).\n>>>>\n>>>> Gaps / Frame drops is yet another beast to handle. I guess I am keeping it\n>>>> separate\n>>>> from the \"per-frame controls\" planning for now. I discussed the same with\n>>>> Kieran\n>>>> the other day - that the frame-drop handling and per-frame controls need to\n>>>> kept\n>>>> separate for progress. Otherwise both half-baked designs, trying to\n>>>> satisfying/handle\n>>>> each other arbitrarily  might create chicken-and-egg problems.\n>>>\n>>> Frame drops had been biting us hard enough already. I think we should\n>>> take them into account from the beginning in order to avoid having to\n>>> re-think how to handle them later..\n>>>\n>>> That's why I think we need to define how mapping the frame number to\n>>> the intended slot and do so uniquely in both \"get\" and \"push\". The\n>>> current \"frame % kMaxFrameContexts\" is enough (it will only cause\n>>> problems if the number of dropped frames in a kMaxFrameContexts period\n>>> is larger than the queue depth I think).\n>>\n>> Yes, a missed frame should still consume it's slot. Lets not try to keep\n>> all slots used - that will get a little messy I think. Just indexing the\n>> slot based on the frame should be sufficient - if we get more than\n>> kMaxFrameContexts of frame drop - then we have bigger issues and things\n>> will be quite wrong (in PFC terms) but if we can detect it, we should\n>> set any PFC error flag (and frame/request underrun flag?) and continue.\n>>\n>>>>\n>>>>>\n>>>>> (also being this a LIFO, isn't the head the most recent and the\n>>>\n>>> Sorry, I clearly meant FIFO\n>>>\n>>>>> tail the oldest entry ? you seem to advance tail when you push a\n>>>>> new frame)\n>>>>\n>>>>\n>>>> No, head_ represents the oldest entry and tail_ represents the latest\n>>>> IPAFrameContext\n>>>> that got created (refer documentation in this patch).\n>>>>\n>>>\n>>> Mine was mostly a comment on the general concept, not as much as the\n>>> documentation of what you've done :) My thought was that in a stack\n>>> (LIFO) you pop the head and in a queue (FIFO) you pop the tail (and\n>>> consequentlly adavance the head when you push a new context).\n>>>\n>>> Basically this:\n>>> https://softwareengineering.stackexchange.com/questions/144477/on-a-queue-which-end-is-the-head\n>>>\n>>> However STL seems to use \"front\" and \"back\" for queues [2] as\n>>> synonymous for your head and tail, so I guess it's fine the way you\n>>> have it (you could use \"front\" and \"back\" to stay closer to STL)\n>>>\n>>> [2] https://en.cppreference.com/w/cpp/container/queue\n>>>\n>>>>>\n>>>>> 2 ) tail_ gets initialized with an empty IPAFrameContext whose constructor\n>>>>> is\n>>>>> IPAFrameContext::IPAFrameContext() = default;\n>>>>>\n>>>>> hence its frame id is not intialized, or are POD types default\n>>>>> initialized in C++ ?\n>>>>\n>>>>\n>>>> We manually give it a value of '0' in clear(). It should work out from\n>>>> there.\n>>>\n>>> Right. Again careful that if frame numbers are numbered using the\n>>> CSI-2 frame number, it will count from 1.\n>>\n>> Where have you got this '1' from ? Is that 'just' the IPU3 CIO2\n> \n> From the CSI-2 specification.\n> \n> I admit I have an old version: \"Version 1.01.00 r0.04 2-Apr-2009\"\n> Section 9.8.1 \"Frame Synchonization Packets\"\n> \n> ------------------------------------------------------------------------------\n> For FS and FE synchronization packets the Short Packet Data Field\n> shall contain a 16-bit frame number. This frame number shall be the\n> same for the FS and FE synchronization packets corresponding to a\n> given frame.\n> \n> The 16-bit frame number, when used, shall be non-zero to distinguish\n> it from the use-case where frame number is inoperative and remains set\n> to zero.\n> \n> The behavior of the 16-bit frame number shall be as one of the\n> following\n> •Frame number is always zero – frame number is inoperative.\n> •Frame number increments by 1 for every FS packet with the same\n> Virtual Channel and is periodically reset to one e.g. 1, 2, 1, 2, 1,\n> 2, 1, 2 or 1, 2, 3, 4, 1, 2, 3, 4\n> ------------------------------------------------------------------------------\n> \n>> receiver?\n>>\n> \n> As far as I can see IPU3, RPi and RKISP1 use a counter and not the\n> CSI-2 frame number\n> \n\nThe sequence number beeing forced to 0 in Kieran's patch [1] is \ncorrecting the CSI-2 issue. But I agree with Jacopo, we have another \none, because we set the frame number to the request->sequence() and when \nwe are in a stop/start it won't be reset to 0.\n\nQuoting d874b3e34173811fa89b68b4b71f22182bc5fd98:\n\"When requests are queued, they are given a sequential number to track \nthe order in which requests are queued to a camera. This number counts \nall requests given to a camera through its lifetime, and is not reset to \nzero between camera stop/start sequences.\n\nIt can be used to support debugging and identifying the flow of requests\nthrough a pipeline, but does not guarantee to represent the sequence \nnumber of any images in the stream. The sequence number is stored as an \nunsigned integer and will wrap when overflowed.\n\"\n\nThus, I think IPU3Frames is not using the correct counter...\n\n[1]: https://patchwork.libcamera.org/project/libcamera/list/?series=3173\n\n>> It's precisely why I created the patch to V4L2VideoDevice to make all\n>> devices consistent.\n>>\n>>>> The .get() calls will over-write the frame number while creating the\n>>>> IPAFrameContext(s),\n>>>> so I don't think it will be a problem as such..\n>>>>\n>>>>>\n>>>>> Anyway, isn't it simpler to use plain counters for head and tail instead\n>>>>> of pointers to the contained objects ? (This would maybe make\n>>>>> complicated to generalize the libcamera::RingBuffer implementation\n>>>>> maybe), but head_ and tail_ mainly serve for two purpose:\n>>>>> - underflow detection (trying to complete a frame not yet queued)\n>>>>> - overflow detection (trying to queue a frame overwrites a\n>>>>> not-yet-consumed one)\n>>>>\n>>>>\n>>>> Well, the main point for having these pointers is to know if a\n>>>> IPAFrameContext exists in the first\n>>>> place or not. The underflow/overflow checks comes later (... and we need\n>>>\n>>> Well, you have this to find out if a frame context is the \"right\" one\n>>>\n>>> IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>>> if (frame != frameContext.frame)\n>>> LOG(IPAIPU3, Warning)\n>>> << \"Got wrong frame context for frame \" << frame;\n>>>\n>>> But the distinction between a \"push\" and a \"get\" is just\n>>>\n>>> if (frame <= tail_->frame)\n>>>\n>>> which could be very well realized with integers.\n>>\n>> A frame count in the tail and head variables could indeed be easy and\n>> clear enough. Probably easier all round as you don't have to care for\n>> wrap around.\n>>\n>>\n>>>\n>>> Anyway, just a suggestion to have the implementation possibly simpler\n>>>\n>>>\n>>>> have to a\n>>>> additional head_ for that, otherwise simply a tail_ would have been\n>>>> sufficient for the former)\n>>>>\n>>>> I agree with you that plain counters would be sufficient (and it doesn't\n>>>> really make a difference\n>>>> to me, either you store pointers OR frame numbers of oldest's and latest's).\n>>>> It is just storing\n>>>> a unique handles somehow.\n>>>>\n>>>>>\n>>>>> Can't we have head and tail simply follow the latest frame number that\n>>>>> have been queued and the lastest frame number that has been consumed\n>>>>> respectively ?\n>>>>>\n>>>>> Collision detection will simply be\n>>>>> - undeflow: tail + 1 == head\n>>>>\n>>>>\n>>>> rather should be head + 1 == tail (according to how tail and head are used\n>>>> in this patch)\n>>>\n>>> Yeah, I was using the notion I had in my head. Must be biased by the\n>>> dog metaphore in the link above :)\n>>>\n>>>>\n>>>>> - overflow (queue frame): head - tail == array size\n>>>>>\n>>>>> The actual position on the array can always be computed as (frame %\n>>>>> kMaxFrameContexts)\n>>>>>\n>>>>> This doesn't however work well with gaps, as one frame gap means we're\n>>>>> actually not using one slot, so a little space is wasted. Ofc as the\n>>>>> number of gaps approaches the number of available slots, the risk of\n>>>>> overflow increases. But gaps should be an exceptional events and with\n>>>>> large enough buffers this shouldn't be a problem ?\n>>>>\n>>>>\n>>>> To be honest, I don't think storing IPAFrameContext pointers vs frame\n>>>> numbers\n>>>> should be a major concern. I say this because intrinsically everything\n>>>> revolves\n>>>> around the frame number in both cases.\n>>>>\n>>>> Going forward (and it's only my educated guess), storing head and tail frame\n>>>> numbers will get limiting a bit. I need to see how Kieran's work on\n>>>> merging/retaining\n>>>> controls is going on. The idea is controls from latest IPAFrameContext gets\n>>>> retained\n>>>> into next-created IPAFrameContext(s) and so on... If you think about it, the\n>>>> tail_->controls\n>>>> will get copied into the next IPAFrameContext while being created. In cases\n>>>> like this,\n>>>> having IPAFrameContexts pointers are useful in my opinion.\n>>>>\n>>>\n>>> I don't think that's an issue as head and tail will simply allow you\n>>> get the context and from there you can do whatever you want.\n>>> Similarly, from the frame context you can get the frame number, so\n>>> yes, whatever you prefer for now\n>>>\n>>>>>\n>>>>> Also I wonder if a push/get interface wouldn't be simpler, with new\n>>>>> reuests being pushed() and frames consumed with get(), but that's more\n>>>>> an implementation detail maybe..\n>>>>\n>>>>\n>>>> I do wondered that as well, but in my opinion having a push() + get()\n>>>> interface also means,\n>>>> each algorithm has to do various checks on their part. For e.g.\n>>>>\n>>>> Algorithm1:\n>>>>\n>>>>     .get(X)  <--- Failed\n>>>>     .push(XFrameContext)\n>>>>     .get(X) <---- Success and carry on\n>>>>\n>>>> Algorithm2:\n>>>>\n>>>>     .get(X) <-- Success because Algorithm1 created XFrameContext\n>>>>     // but you still need to write failure path here containing\n>>>> .push(XFrameContext)\n>>>>\n>>>> .. and so on.\n>>>>\n>>>> Also, various Algorithm might need to create different frame's\n>>>> IPAFrameContext in a single run,\n>>>> depending on their latencies.\n>>>>\n>>>> So I think we are unnecessarily outsourcing the responsibility of \"pushing\"\n>>>> the IPAFrameContext\n>>>> to the algorithms. All this can be encapsulated in the .get(), no? I thought\n>>>> we all agreed on\n>>>> the .get() design, so hence this direction.\n>>>>\n>>\n>> Yes, a '.get()' should certainly be available to algorithms to 'get' the\n>> FC based on an index. The question here seems to be around 'what happens\n>> if the algorithm 'gets' the slot - and it hadn't yet been filled in by\n>> the queue request call.\n>>\n>> Well. ... first - the .get() call should hopefully know that ... and\n>> will have to set the PFC error / underrun flag.\n>>\n>> But we may have to see what data is expected to be referenced, as it may\n>> be 'uninitialised'. But this is where we really need to be clear on what\n>> state is in the FCQ vs in the algorithm itself. To me the FCQ is\n>> going to store 'commands' (i.e. controls). There will be some state\n>> around the algorithm, but I expect that there should also be private\n>> state for the algorithm for auto controls too - so it should be able to\n>> remember any current filter positions or such ...\n>>\n>> I'm certain I'm not explaining what's in my head above clearly there -\n>> so dig deeper and push me where it's not clear and I'll try to reword or\n>> draw something up (which will be part of the PFC design doc I'm working\n>> on anyway).\n>>\n>> But tl;dr; - I believe an algorithm should still be able to run if there\n>> is no FCQ slot filled with a request - but it should be an error state.\n>>\n>>\n>>\n>>> In case of requests underrun the algorithms would try to access a\n>>> non-existing frame context, hence their first \"get()\" will have to\n>>> create one. Indeed having a get that creates entries blurries the line\n>>> between a push and a get enough to justify using a single function.\n>>>\n>>>>>\n>>>>> IPA components cannot have tests right ? It would be nice to have a\n>>>>> unit test for this component to make sure it behaves as intended\n>>>>> instead of having to debug it \"live\"\n>>>>\n>>>>\n>>>> I see that you have mentioned a desire to make a generic\n>>>> libcamera::RingBuffer\n>>>> class implementation. While I do agree on the point of tests, but I am not\n>>>> looking\n>>>> at this angle just yet. It feels to me a bit early to do so because things\n>>>> can be very\n>>>> specific w.r.t IPAs. Even if we do it \"generically\" right now, only IPU3\n>>>> would be the\n>>>> only user I think. There are other similar parts that we want to provide a\n>>>> generic\n>>>> implementation to all the IPAs for e.g. IPAContext, IPAFrameContexts and so\n>>>> on,\n>>>> so probably once we have a good usage across multiple IPAs, we would be in a\n>>>> better position to write a generic ring buffer implementation then and adapt\n>>>> the IPAs on top of it?\n>>>>\n>>>\n>>> As I said I was thinking about other components outside of the IPAs\n>>> that could benefit from this as well. But I agree getting to have a\n>>> first user is the first step to later eventually generalize.\n>>>\n>>>>>\n>>>>> sorry, a lot of questions actually..\n>>>>\n>>>>\n>>>> No issues ;-)\n>>>>\n>>>\n>>> So it seems to me that the only remaining concerns are actually:\n>>>\n>>> 1) Map uniquely the frame number to the slot index\n>>\n>> Yes, a frame number and a slot in the FCQ should be directly mappable.\n>>\n>>>\n>>> 2) Clarify if it can be assumed frames always start from 0. This is\n>>> desirable as in the current model where frames start being\n>>> produced when enough buffers have been queued, the FCQ will be\n>>> filled with requests numbered from 0 on, while the first frame\n>>> could potentially have an arbitrary value. Is that handled by\n>>> Kieran's series from december ? (sorry, I lost track of all the\n>>> moving parts).\n>>\n>> Yes, I believe we work on the assumption all frames start at zero (after\n>> any call to start(), so a stop() / start() resets to Frame/Request 0.\n> \n> Could you have a look at my other reply about the fact we use request\n> numbers as frame numbers ?\n> \n> Thanks\n> j\n> \n>>\n>>\n>> --\n>> Kieran\n>>\n>>\n>>\n>>\n>>>\n>>> Thanks\n>>> j\n>>>\n>>>>>\n>>>>>> + IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>>>>>> + if (frame != frameContext.frame)\n>>>>>> + LOG(IPAIPU3, Warning)\n>>>>>> + << \"Got wrong frame context for frame \" << frame;\n>>>>>> +\n>>>>>> + return &frameContext;\n>>>>>> + } else {\n>>>>>> + if (isFull_) {\n>>>>>> + LOG(IPAIPU3, Warning)\n>>>>>> + << \"Cannot create frame context for frame \" << frame\n>>>>>> + << \" since FCQueue is full\";\n>>>>>> +\n>>>>>> + return nullptr;\n>>>>>> + }\n>>>>>> +\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>>>>>> +\n>>>>>> + /* Check if the queue is full to avoid over-queueing later */\n>>>>>> + if (tail_->frame - head_->frame >= kMaxFrameContexts - 1)\n>>>>>> + isFull_ = true;\n>>>>>>\n>>>>>> - if (frame != frameContext.frame) {\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>>>>>>\n>>>>>> - return &frameContext;\n>>>>>> + head_ = this->get(frame + 1);\n>>>>>> +\n>>>>>> + if (isFull_)\n>>>>>> + isFull_ = false;\n>>>>>> }\n>>>>>>\n>>>>>> +/**\n>>>>>> + * \\fn FCQueue::isFull()\n>>>>>> + * \\brief Checks whether the frame context queue is full\n>>>>>> + */\n>>>>>> +\n>>>>>> /**\n>>>>>> * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n>>>>>> */\n>>>>>> @@ -260,6 +334,13 @@ void FCQueue::clear()\n>>>>>> {\n>>>>>> IPAFrameContext initFrameContext;\n>>>>>> this->fill(initFrameContext);\n>>>>>> +\n>>>>>> + isFull_ = false;\n>>>>>> +\n>>>>>> + /* Intialise 0th index to frame 0 */\n>>>>>> + this->at(0).frame = 0;\n>>>>>> + tail_ = &this->at(0);\n>>>>>> + head_ = 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 56d281f6..475855da 100644\n>>>>>> --- a/src/ipa/ipu3/ipa_context.h\n>>>>>> +++ b/src/ipa/ipu3/ipa_context.h\n>>>>>> @@ -93,9 +93,18 @@ class FCQueue : public std::array\n>>>>>> {\n>>>>>> public:\n>>>>>> FCQueue();\n>>>>>> + FCQueue &operator[](const FCQueue &) = delete;\n>>>>>>\n>>>>>> void clear();\n>>>>>> + void completeFrame(uint32_t frame);\n>>>>>> IPAFrameContext *get(uint32_t frame);\n>>>>>> + bool isFull() { return isFull_; }\n>>>>>> +\n>>>>>> +private:\n>>>>>> + IPAFrameContext *head_;\n>>>>>> + IPAFrameContext *tail_;\n>>>>>> +\n>>>>>> + bool isFull_;\n>>>>>> };\n>>>>>>\n>>>>>> struct IPAContext {\n>>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>>>> index 0843d882..1d6ee515 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>>>>>> */\n>>>>>>\n>>>>>> metadataReady.emit(frame, ctrls);\n>>>>>> +\n>>>>>> + context_.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>>>>>> /* \\todo Start processing for 'frame' based on 'controls'. */\n>>>>>> - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n>>>>>> + *context_.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 6E61ABD808\n\tfor ;\n\tMon, 13 Jun 2022 14:40:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA59765637;\n\tMon, 13 Jun 2022 16:40:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8F937600F1\n\tfor ;\n\tMon, 13 Jun 2022 16:40:02 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:a1c7:c28c:7720:9b30] (unknown\n\t[IPv6:2a01:e0a:169:7140:a1c7:c28c:7720:9b30])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0CAE0305;\n\tMon, 13 Jun 2022 16:40:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655131203;\n\tbh=3YTB89oEr+lrj616ijZmn+5icXNmx0LXamXMJISPtY0=;\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=GnW96PoPOT0HO0csulHcR4TcPAdpR/qV4mZertG+eIQOx8CoMWsFRfjEADwCPyKy4\n\tizAubJ3wpvZPCfXVvZIsvOPkq/K7mkgoCmZ6+8CKJ+JtiXXaiEnVoVk+wQxPnaRGIM\n\t0yvn+4kDwubNWB6BuIWCZ89Bocm5f+U+xPoDM6aYNb+ME9si1v1gHco3fgp6+SXvWj\n\tJRZACgRRir3fAe8Je8airpcS8g3CukaLx8h/lnDmrfE5kTfg1ZfkjTI5itwN15bcFT\n\toxwDGVLV5Uv3iZsBJodSn++3/fc4S3JDWUiTc2ilCnE3Sz7wW6HZy6DpUlOFUzIvnr\n\t6nGVb6/Z3kzRQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655131202;\n\tbh=3YTB89oEr+lrj616ijZmn+5icXNmx0LXamXMJISPtY0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=OsmNW+Nqlt+NzQwb3PBbjxf4E0gemwPmR4RYUjuWytMLtW3WB5fPNBdQbmGkN9p5R\n\twcRZGxXcYUR80xI9HzkLE8B6YomCGFXeiwsuFkfcXYjj5ibF3B06ZwhNc2Iwya6M42\n\twFx6GqvRRltuc/nQxaAiH6VtL5itr/YbM49rqgGU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"OsmNW+Nq\"; dkim-atps=neutral","Message-ID":"<7aac5ed3-bdcf-ae7d-016c-1641a9865bd2@ideasonboard.com>","Date":"Mon, 13 Jun 2022 16:39:59 +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":"Jacopo Mondi ,\n\tKieran Bingham ","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-3-umang.jain@ideasonboard.com>\n\t<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>\n\t<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>\n\t<20220609162054.enj4kswtjtomm5po@uno.localdomain>\n\t<165488883227.1070686.8210324805195113270@Monstersaurus>\n\t<20220611144009.meewiilill66alcf@uno.localdomain>","In-Reply-To":"<20220611144009.meewiilill66alcf@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::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 ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23403,"web_url":"https://patchwork.libcamera.org/comment/23403/","msgid":"<165513417583.1793421.9359783274840537446@Monstersaurus>","date":"2022-06-13T15:29:35","subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::get()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2022-06-13 15:39:59)\n> Hi Umang, Jacopo, Kieran,\n> \n\n\n\n> >>>>\n> >>>> We manually give it a value of '0' in clear(). It should work out from\n> >>>> there.\n> >>>\n> >>> Right. Again careful that if frame numbers are numbered using the\n> >>> CSI-2 frame number, it will count from 1.\n> >>\n> >> Where have you got this '1' from ? Is that 'just' the IPU3 CIO2\n> > \n> > From the CSI-2 specification.\n> > \n> > I admit I have an old version: \"Version 1.01.00 r0.04 2-Apr-2009\"\n> > Section 9.8.1 \"Frame Synchonization Packets\"\n> > \n> > ------------------------------------------------------------------------------\n> > For FS and FE synchronization packets the Short Packet Data Field\n> > shall contain a 16-bit frame number. This frame number shall be the\n> > same for the FS and FE synchronization packets corresponding to a\n> > given frame.\n> > \n> > The 16-bit frame number, when used, shall be non-zero to distinguish\n> > it from the use-case where frame number is inoperative and remains set\n> > to zero.\n> > \n> > The behavior of the 16-bit frame number shall be as one of the\n> > following\n> > •Frame number is always zero – frame number is inoperative.\n> > •Frame number increments by 1 for every FS packet with the same\n> > Virtual Channel and is periodically reset to one e.g. 1, 2, 1, 2, 1,\n> > 2, 1, 2 or 1, 2, 3, 4, 1, 2, 3, 4\n> > ------------------------------------------------------------------------------\n> > \n> >> receiver?\n> >>\n> > \n> > As far as I can see IPU3, RPi and RKISP1 use a counter and not the\n> > CSI-2 frame number\n> > \n> \n> The sequence number beeing forced to 0 in Kieran's patch [1] is \n> correcting the CSI-2 issue. But I agree with Jacopo, we have another \n> one, because we set the frame number to the request->sequence() and when \n> we are in a stop/start it won't be reset to 0.\n> \n> Quoting d874b3e34173811fa89b68b4b71f22182bc5fd98:\n> \"When requests are queued, they are given a sequential number to track \n> the order in which requests are queued to a camera. This number counts \n> all requests given to a camera through its lifetime, and is not reset to \n> zero between camera stop/start sequences.\n> \n> It can be used to support debugging and identifying the flow of requests\n> through a pipeline, but does not guarantee to represent the sequence \n> number of any images in the stream. The sequence number is stored as an \n> unsigned integer and will wrap when overflowed.\n> \"\n\nGood spot. I wonder if we'll need to make that reset to zero, or ensure\nthat the sequence counts resume correctly from a streamOn/streamOff.\n\nI suspect we'll just make the request numbers restart, as they are only\nused internally anyway I think ...\n\n\n> Thus, I think IPU3Frames is not using the correct counter...\n> \n> [1]: https://patchwork.libcamera.org/project/libcamera/list/?series=3173\n> \n> >> It's precisely why I created the patch to V4L2VideoDevice to make all\n> >> devices consistent.\n\n--\nKieran","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 6C5CBBD161\n\tfor ;\n\tMon, 13 Jun 2022 15:29:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9706C65635;\n\tMon, 13 Jun 2022 17:29:40 +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 C261C600F1\n\tfor ;\n\tMon, 13 Jun 2022 17:29:38 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 30E7D305;\n\tMon, 13 Jun 2022 17:29:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655134180;\n\tbh=d3UEfCweerO7elAsodIPhaRQOLG+qLNS5ILihEwT8mk=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=lb78H6e8DK06sxfCXpdwoQuyvwDN30O60cJllEJPZlLeEVwmgxdfurzjC+Y9HIV+5\n\tlUy9jYw9fAb0/dC/9QFn01IYIE0Mnx2TjhehW7PxVc4WlDsvCje6Q1lIB8N5gNFnDb\n\txwWWf0OaiaEEkIT3sX3MoJVCkWy8cB96fSz/5FIeYvRLxK/pgJmc+VBF9gVNedYwMH\n\tApaAyGmH8R0W2Q/O+80rn5yzn3HPNy45ODJ2NAhuHE5/nmsuomfnvRSG4eGvyTIjZl\n\troy5RVA6Dr2m9CIhBAwmC2qCzP9u3RzDzFqe3SwuF9XxDirRai9Ku1ZdjkvYCmUPMO\n\t4TeCiJy99e83w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655134178;\n\tbh=d3UEfCweerO7elAsodIPhaRQOLG+qLNS5ILihEwT8mk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VyNbjSGGlcLtCxZEPsKNKAYzBYKwXRdDHVPesBrNW+8E198SC8voPuOrizt7u9ibe\n\tOOw3WjXnBH3gQeemqxRtX6Usp+KId2258FN+Y70Q+ZSwNcaNmJ/gdKJzSB8RRYrEDj\n\t/197kKAsFUJvA6RD7GTSBV9pubgpWT8KOH/QKJWw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VyNbjSGG\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<7aac5ed3-bdcf-ae7d-016c-1641a9865bd2@ideasonboard.com>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-3-umang.jain@ideasonboard.com>\n\t<20220608135610.qkzxx2pp7rg732bg@uno.localdomain>\n\t<0bb5dcbd-db0e-6f49-9da1-1405d9ec1a69@ideasonboard.com>\n\t<20220609162054.enj4kswtjtomm5po@uno.localdomain>\n\t<165488883227.1070686.8210324805195113270@Monstersaurus>\n\t<20220611144009.meewiilill66alcf@uno.localdomain>\n\t<7aac5ed3-bdcf-ae7d-016c-1641a9865bd2@ideasonboard.com>","To":"Jacopo Mondi ,\n\tJean-Michel Hautbois ","Date":"Mon, 13 Jun 2022 16:29:35 +0100","Message-ID":"<165513417583.1793421.9359783274840537446@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 2/4] ipa: ipu3: ipa_context: Extend\n\tFCQueue::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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]