[{"id":25010,"web_url":"https://patchwork.libcamera.org/comment/25010/","msgid":"<166368108528.3912877.2379777433335617224@Monstersaurus>","date":"2022-09-20T13:38:05","subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)\n> From: Umang Jain \n> \n> Introduce a common implementation in libipa to represent the queue of\n> frame contexts.\n> \n> Signed-off-by: Umang Jain \n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> ---\n> Changes since v3:\n> \n> - Split the IPU3 changes to a separate patch\n> - Use composition instead of inheritance\n> - Use vector instead of array for backend storage\n> - Make the queue size dynamic\n> - Rename initialise() to init()\n> ---\n> src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/fc_queue.h | 108 +++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 227 insertions(+)\n> create mode 100644 src/ipa/libipa/fc_queue.cpp\n> create mode 100644 src/ipa/libipa/fc_queue.h\n> \n> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> new file mode 100644\n> index 000000000000..b81d497e255a\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.cpp\n> @@ -0,0 +1,117 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * fc_queue.cpp - IPA Frame context queue\n> + */\n> +\n> +#include \"fc_queue.h\"\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(FCQueue)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\file fc_queue.h\n> + * \\brief Queue to access per-frame Context\n> + */\n> +\n> +/**\n> + * \\class FCQueue\n> + * \\brief A support class for queueing FrameContext instances through the IPA\n> + * \\tparam FrameContext The IPA specific FrameContext derived class type\n\n; or , after FrameContext?\n\n> + *\n> + * The frame context queue provides a simple circular buffer implementation to\n> + * store IPA specific context for each frame through its lifetime within the\n> + * IPA.\n> + *\n> + * FrameContext instances are expected to be referenced by a monotonically\n> + * increasing sequence count corresponding to a frame sequence number.\n> + *\n> + * A FrameContext is initialised for a given frame when the corresponding\n> + * Request is first queued into the IPA. From that point on the FrameContext can\n> + * be obtained by the IPA and its algorithms by referencing it from the frame\n> + * sequence number.\n> + *\n> + * A frame sequence number from the image source must correspond to the request\n> + * sequence number for this implementation to be supported in an IPA. It is\n> + * expected that the same sequence number will be used to reference the context\n> + * of the frame from the point of queueing the request, specifying controls for\n> + * a given frame, and processing of any ISP related controls and statistics for\n> + * the same corresponding image.\n> + *\n> + * IPA specific frame context implementations shall inherit from the\n> + * IPAFrameContext base class to support the minimum required features for a\n> + * FrameContext.\n> + *\n> + * FrameContexts are overwritten when they are recycled and re-initialised by\n> + * the first access made on them by either init(frame) or get(frame). It is\n> + * required that the number of available slots in the frame context queue is\n> + * larger or equal to the maximum number of in-flight requests a pipeline can\n> + * handle to avoid overwriting frame context instances that still have to be\n> + * processed.\n> + *\n> + * In the case an application does not queue requests to the camera fast\n> + * enough, frames can be produced and processed by the IPA without a\n> + * corresponding Request being queued. In this case the IPA algorithm\n> + * will try to access the FrameContext with a call to get() before it\n> + * had been initialized at queueRequest() time. In this case, there\n> + * is no way the controls associated with the late Request could be\n> + * applied in time, as the frame as already been processed by the IPA.\n> + *\n> + * \\todo Set an error flag for per-frame control errors.\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::FCQueue(unsigned int size)\n> + * \\brief Construct a frame context queue of a specified size\n> + * \\param[in] size The number of contexts in the queue\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::clear()\n> + * \\brief Clear the context queue\n> + *\n> + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> + * IPA modules are expected to clear the frame context queue at the beginning of\n> + * a new streaming session, in IPAModule::start().\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::init(uint32_t frame)\n\nI saw you've questioned the name here.\n\nI don't mind if this is init(), alloc() new() ?\n\nWe don't (yet?) have an explicit free/delete/release of the FCQueue, as\nthey stay in memory until something overwrites them like an LRU....\n\n\n> + * \\brief Initialize and return the Frame Context at slot specified by \\a frame\n> + * \\param[in] frame The frame context sequence number\n> + *\n> + * The first call to obtain a FrameContext from the FCQueue should be handled\n> + * through this call. The FrameContext will be initialised for the frame and\n> + * returned to the caller if it was not already initialised.\n> + *\n> + * If the FrameContext was already initialized for this sequence, a warning will\n> + * be reported and the previously initialized FrameContext is returned.\n> + *\n> + * Frame contexts are expected to be initialised when a Request is first passed\n> + * to the IPA module in IPAModule::queueRequest().\n> + *\n> + * \\return A reference to the FrameContext for sequence \\a frame\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::get()\n> + * \\brief Obtain the Frame Context at slot specified by \\a frame\n> + * \\param[in] frame The frame context sequence number\n> + *\n> + * Obtains an existing FrameContext from the queue and returns it to the caller.\n> + *\n> + * If the FrameContext is not correctly initialised for the \\a frame, it will be\n> + * initialised.\n> + *\n> + * \\return A reference to the FrameContext for sequence \\a frame\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> new file mode 100644\n> index 000000000000..0f3af0f3a189\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.h\n> @@ -0,0 +1,108 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * fc_queue.h - IPA Frame context queue\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(FCQueue)\n> +\n> +namespace ipa {\n> +\n> +template\n> +class FCQueue\n> +{\n> +public:\n> + FCQueue(unsigned int size)\n> + : contexts_(size)\n> + {\n> + }\n> +\n> + void clear()\n> + {\n> + std::fill(contexts_.begin(), contexts_.end(), FrameContext{});\n> + }\n> +\n> + FrameContext &init(const uint32_t frame)\n> + {\n> + FrameContext &frameContext = contexts_[frame % contexts_.size()];\n> +\n> + /*\n> + * Do not re-initialise if a get() call has already fetched this\n> + * frame context to preseve the context.\n> + *\n> + * \\todo If the the sequence number of the context to initialise\n> + * is smaller than the sequence number of the queue slot to use,\n> + * it means that we had a serious request underrun and more\n> + * frames than the queue size has been produced since the last\n> + * time the application has queued a request. Does this deserve\n> + * an error condition ?\n> + */\n> + if (frame != 0 && frame <= frameContext.frame)\n> + LOG(FCQueue, Warning)\n> + << \"Frame \" << frame << \" already initialised\";\n> + else\n> + init(frameContext, frame);\n> +\n> + return frameContext;\n> + }\n> +\n> + FrameContext &get(uint32_t frame)\n> + {\n> + FrameContext &frameContext = contexts_[frame % contexts_.size()];\n> +\n> + /*\n> + * If the IPA algorithms try to access a frame context slot which\n> + * has been already overwritten by a newer context, it means the\n> + * frame context queue has overflowed and the desired context\n> + * has been forever lost. The pipeline handler shall avoid\n> + * queueing more requests to the IPA than the frame context\n> + * queue size.\n> + */\n> + if (frame < frameContext.frame)\n> + LOG(FCQueue, Fatal) << \"Frame Context for \" << frame\n> + << \" has been overwritten by \"\n> + << frameContext.frame;\n> +\n> + if (frame == frameContext.frame)\n> + return frameContext;\n> +\n> + /*\n> + * The frame context has been retrieved before it was\n> + * initialised through the initialise() call. This indicates an\n> + * algorithm attempted to access a Frame context before it was\n> + * queued to the IPA. Controls applied for this request may be\n\ndouble space between 'IPA. Controls'\n\n> + * left unhandled.\n> + *\n> + * \\todo Set an error flag for per-frame control errors.\n> + */\n> + LOG(FCQueue, Warning)\n> + << \"Obtained an uninitialised FrameContext for \" << frame;\n> +\n> + init(frameContext, frame);\n> +\n> + return frameContext;\n> + }\n> +\n> +private:\n> + void init(FrameContext &frameContext, const uint32_t frame)\n> + {\n> + frameContext = {};\n> + frameContext.frame = frame;\n\nAha this is better.\n\n\nReviewed-by: Kieran Bingham \n\n> + }\n> +\n> + std::vector contexts_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index fb894bc614af..016b8e0ec9be 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -3,6 +3,7 @@\n> libipa_headers = files([\n> 'algorithm.h',\n> 'camera_sensor_helper.h',\n> + 'fc_queue.h',\n> 'histogram.h',\n> 'module.h',\n> ])\n> @@ -10,6 +11,7 @@ libipa_headers = files([\n> libipa_sources = files([\n> 'algorithm.cpp',\n> 'camera_sensor_helper.cpp',\n> + 'fc_queue.cpp',\n> 'histogram.cpp',\n> 'module.cpp',\n> ])\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 EA5D4C3272\n\tfor ;\n\tTue, 20 Sep 2022 13:38:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1704C621AC;\n\tTue, 20 Sep 2022 15:38:10 +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 28EED6218B\n\tfor ;\n\tTue, 20 Sep 2022 15:38:08 +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 A8CB96DB;\n\tTue, 20 Sep 2022 15:38:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663681090;\n\tbh=UKX0rKtr2N+8unqPp0waoqyP+VGSPEhv+Ig0V8L9Hic=;\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:\n\tFrom;\n\tb=VzVM+oFQ5WMjrA8ROo5p6cPxeHd2E8TGH9RJfVCjvutHFOxXdHFLU/3Qqn18WZAye\n\tkDp2S8jAFN6MU02ZMof6jdVk47GSIWJO8VaHbx35u0VhtI0fzMQE0DJENmECio7c8B\n\td7+YTyfFypdUzfmRu00v5Li5kwBkE0Uh2YlPORXZAkrKWS509kEBhS1gXGGmwZyDxP\n\tF3Y0gGr5d/9iL6W8vHpCwaMAgirazDSh62roUMIyOSOxcYWyQIbdg9CeVOsHRg1LY4\n\tF9LhlPJ0+lTJtP77RDqmj7Ko7SX8WzqTHCTEqcQCexPcwQr2t2A5G2nNEs4fkD4p96\n\tkm8wKRHAZIEuQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663681087;\n\tbh=UKX0rKtr2N+8unqPp0waoqyP+VGSPEhv+Ig0V8L9Hic=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=fhRLK3lp7kMpiWYABtDu/R9QLvmogYI1KvBPc0qGrWjkWXTBR0AXm9NMakYVI09hJ\n\tXcO0A4N/smILc3V/N1m+4RWMYhCTioDCyYDGJ63xtQWkCX4YvQAGznueau8K7LOKaY\n\ttLZvgVAf/wURNvyRnLoJTkn4P9n06QhlMMzYUlCg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fhRLK3lp\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220908014200.28728-5-laurent.pinchart@ideasonboard.com>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-5-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 20 Sep 2022 14:38:05 +0100","Message-ID":"<166368108528.3912877.2379777433335617224@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25026,"web_url":"https://patchwork.libcamera.org/comment/25026/","msgid":"","date":"2022-09-20T19:11:10","subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Sep 20, 2022 at 02:38:05PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)\n> > From: Umang Jain \n> > \n> > Introduce a common implementation in libipa to represent the queue of\n> > frame contexts.\n> > \n> > Signed-off-by: Umang Jain \n> > Signed-off-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > Changes since v3:\n> > \n> > - Split the IPU3 changes to a separate patch\n> > - Use composition instead of inheritance\n> > - Use vector instead of array for backend storage\n> > - Make the queue size dynamic\n> > - Rename initialise() to init()\n> > ---\n> > src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/fc_queue.h | 108 +++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 3 files changed, 227 insertions(+)\n> > create mode 100644 src/ipa/libipa/fc_queue.cpp\n> > create mode 100644 src/ipa/libipa/fc_queue.h\n> > \n> > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> > new file mode 100644\n> > index 000000000000..b81d497e255a\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.cpp\n> > @@ -0,0 +1,117 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Google Inc.\n> > + *\n> > + * fc_queue.cpp - IPA Frame context queue\n> > + */\n> > +\n> > +#include \"fc_queue.h\"\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(FCQueue)\n> > +\n> > +namespace ipa {\n> > +\n> > +/**\n> > + * \\file fc_queue.h\n> > + * \\brief Queue to access per-frame Context\n> > + */\n> > +\n> > +/**\n> > + * \\class FCQueue\n> > + * \\brief A support class for queueing FrameContext instances through the IPA\n> > + * \\tparam FrameContext The IPA specific FrameContext derived class type\n\nI'll s/IPA specific/IPA-specific/\n\n> ; or , after FrameContext?\n\nWhich one, the first or second ? The first FrameContext is the template\nparameter name and thus shouldn't be followed by anything, and the\nsecond one doesn't seem to require a comma.\n\n> > + *\n> > + * The frame context queue provides a simple circular buffer implementation to\n> > + * store IPA specific context for each frame through its lifetime within the\n> > + * IPA.\n> > + *\n> > + * FrameContext instances are expected to be referenced by a monotonically\n> > + * increasing sequence count corresponding to a frame sequence number.\n> > + *\n> > + * A FrameContext is initialised for a given frame when the corresponding\n> > + * Request is first queued into the IPA. From that point on the FrameContext can\n> > + * be obtained by the IPA and its algorithms by referencing it from the frame\n> > + * sequence number.\n> > + *\n> > + * A frame sequence number from the image source must correspond to the request\n> > + * sequence number for this implementation to be supported in an IPA. It is\n> > + * expected that the same sequence number will be used to reference the context\n> > + * of the frame from the point of queueing the request, specifying controls for\n> > + * a given frame, and processing of any ISP related controls and statistics for\n> > + * the same corresponding image.\n> > + *\n> > + * IPA specific frame context implementations shall inherit from the\n> > + * IPAFrameContext base class to support the minimum required features for a\n> > + * FrameContext.\n> > + *\n> > + * FrameContexts are overwritten when they are recycled and re-initialised by\n> > + * the first access made on them by either init(frame) or get(frame). It is\n> > + * required that the number of available slots in the frame context queue is\n> > + * larger or equal to the maximum number of in-flight requests a pipeline can\n> > + * handle to avoid overwriting frame context instances that still have to be\n> > + * processed.\n> > + *\n> > + * In the case an application does not queue requests to the camera fast\n> > + * enough, frames can be produced and processed by the IPA without a\n> > + * corresponding Request being queued. In this case the IPA algorithm\n> > + * will try to access the FrameContext with a call to get() before it\n> > + * had been initialized at queueRequest() time. In this case, there\n> > + * is no way the controls associated with the late Request could be\n> > + * applied in time, as the frame as already been processed by the IPA.\n> > + *\n> > + * \\todo Set an error flag for per-frame control errors.\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::FCQueue(unsigned int size)\n> > + * \\brief Construct a frame context queue of a specified size\n> > + * \\param[in] size The number of contexts in the queue\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::clear()\n> > + * \\brief Clear the context queue\n> > + *\n> > + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> > + * IPA modules are expected to clear the frame context queue at the beginning of\n> > + * a new streaming session, in IPAModule::start().\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::init(uint32_t frame)\n> \n> I saw you've questioned the name here.\n> \n> I don't mind if this is init(), alloc() new() ?\n> \n> We don't (yet?) have an explicit free/delete/release of the FCQueue, as\n> they stay in memory until something overwrites them like an LRU....\n\nWe shouldn't allocate memory dynamically, so there will be no memory\nallocation as such, but the concept of allocation a frame context from\nthe queue still applies. I indeed prefer alloc() over init(). Better\nnames are also possible. I'll see what I can do in the next version.\n\n> > + * \\brief Initialize and return the Frame Context at slot specified by \\a frame\n> > + * \\param[in] frame The frame context sequence number\n> > + *\n> > + * The first call to obtain a FrameContext from the FCQueue should be handled\n> > + * through this call. The FrameContext will be initialised for the frame and\n> > + * returned to the caller if it was not already initialised.\n> > + *\n> > + * If the FrameContext was already initialized for this sequence, a warning will\n> > + * be reported and the previously initialized FrameContext is returned.\n> > + *\n> > + * Frame contexts are expected to be initialised when a Request is first passed\n> > + * to the IPA module in IPAModule::queueRequest().\n> > + *\n> > + * \\return A reference to the FrameContext for sequence \\a frame\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::get()\n> > + * \\brief Obtain the Frame Context at slot specified by \\a frame\n> > + * \\param[in] frame The frame context sequence number\n> > + *\n> > + * Obtains an existing FrameContext from the queue and returns it to the caller.\n> > + *\n> > + * If the FrameContext is not correctly initialised for the \\a frame, it will be\n> > + * initialised.\n> > + *\n> > + * \\return A reference to the FrameContext for sequence \\a frame\n> > + */\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > new file mode 100644\n> > index 000000000000..0f3af0f3a189\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.h\n> > @@ -0,0 +1,108 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Google Inc.\n> > + *\n> > + * fc_queue.h - IPA Frame context queue\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(FCQueue)\n> > +\n> > +namespace ipa {\n> > +\n> > +template\n> > +class FCQueue\n> > +{\n> > +public:\n> > + FCQueue(unsigned int size)\n> > + : contexts_(size)\n> > + {\n> > + }\n> > +\n> > + void clear()\n> > + {\n> > + std::fill(contexts_.begin(), contexts_.end(), FrameContext{});\n> > + }\n> > +\n> > + FrameContext &init(const uint32_t frame)\n> > + {\n> > + FrameContext &frameContext = contexts_[frame % contexts_.size()];\n> > +\n> > + /*\n> > + * Do not re-initialise if a get() call has already fetched this\n> > + * frame context to preseve the context.\n> > + *\n> > + * \\todo If the the sequence number of the context to initialise\n> > + * is smaller than the sequence number of the queue slot to use,\n> > + * it means that we had a serious request underrun and more\n> > + * frames than the queue size has been produced since the last\n> > + * time the application has queued a request. Does this deserve\n> > + * an error condition ?\n> > + */\n> > + if (frame != 0 && frame <= frameContext.frame)\n> > + LOG(FCQueue, Warning)\n> > + << \"Frame \" << frame << \" already initialised\";\n> > + else\n> > + init(frameContext, frame);\n> > +\n> > + return frameContext;\n> > + }\n> > +\n> > + FrameContext &get(uint32_t frame)\n> > + {\n> > + FrameContext &frameContext = contexts_[frame % contexts_.size()];\n> > +\n> > + /*\n> > + * If the IPA algorithms try to access a frame context slot which\n> > + * has been already overwritten by a newer context, it means the\n> > + * frame context queue has overflowed and the desired context\n> > + * has been forever lost. The pipeline handler shall avoid\n> > + * queueing more requests to the IPA than the frame context\n> > + * queue size.\n> > + */\n> > + if (frame < frameContext.frame)\n> > + LOG(FCQueue, Fatal) << \"Frame Context for \" << frame\n> > + << \" has been overwritten by \"\n> > + << frameContext.frame;\n> > +\n> > + if (frame == frameContext.frame)\n> > + return frameContext;\n> > +\n> > + /*\n> > + * The frame context has been retrieved before it was\n> > + * initialised through the initialise() call. This indicates an\n> > + * algorithm attempted to access a Frame context before it was\n> > + * queued to the IPA. Controls applied for this request may be\n> \n> double space between 'IPA. Controls'\n> \n> > + * left unhandled.\n> > + *\n> > + * \\todo Set an error flag for per-frame control errors.\n> > + */\n> > + LOG(FCQueue, Warning)\n> > + << \"Obtained an uninitialised FrameContext for \" << frame;\n> > +\n> > + init(frameContext, frame);\n> > +\n> > + return frameContext;\n> > + }\n> > +\n> > +private:\n> > + void init(FrameContext &frameContext, const uint32_t frame)\n> > + {\n> > + frameContext = {};\n> > + frameContext.frame = frame;\n> \n> Aha this is better.\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> > + }\n> > +\n> > + std::vector contexts_;\n> > +};\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index fb894bc614af..016b8e0ec9be 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -3,6 +3,7 @@\n> > libipa_headers = files([\n> > 'algorithm.h',\n> > 'camera_sensor_helper.h',\n> > + 'fc_queue.h',\n> > 'histogram.h',\n> > 'module.h',\n> > ])\n> > @@ -10,6 +11,7 @@ libipa_headers = files([\n> > libipa_sources = files([\n> > 'algorithm.cpp',\n> > 'camera_sensor_helper.cpp',\n> > + 'fc_queue.cpp',\n> > 'histogram.cpp',\n> > 'module.cpp',\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 2A3CAC0DA4\n\tfor ;\n\tTue, 20 Sep 2022 19:11:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 756FB621C6;\n\tTue, 20 Sep 2022 21:11:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 896666218B\n\tfor ;\n\tTue, 20 Sep 2022 21:11:25 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E108C6BE;\n\tTue, 20 Sep 2022 21:11:24 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663701086;\n\tbh=6Gu3PfCdU47PuTFL6XGNIcODfkF8XIxeGbjuJHlH6FM=;\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=aGUQwc4vkCUyNgmfF4TmX0UAx3Jf0DpLvybr3B4ap0jkaM5r5W/sacNit4fwWsd/e\n\taCY6CW+FGqWTDVTxC7JLJoyJu02CdjmiWEXfCfIc2cUHkI5RNn1Xj/vZsG3eiJdEkI\n\t60S/cW2FyrbzWPg/p2cBU9hVFYq/sJYyjjQtftjBQ7Ap8DMLB5fNyEzVOgpZg2zFPv\n\tNnl3lEiU9jXgpGYb8OUVY2xe5G6vTZoY92S25CAKeFnvDjPhjtng1vtMx7I9ZDdBSA\n\tfkflEOKu0K4p7SY2IdB2DynO7OjtSTBHyGVb04uF43GKTU4fr3GECzG3OBsj0D/CKz\n\thCxdoJGV+x9rQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663701085;\n\tbh=6Gu3PfCdU47PuTFL6XGNIcODfkF8XIxeGbjuJHlH6FM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i4ryLE0sO9KGZjF4tjaohXIAQMnwdxdwLfGIj3EU6tqV9Fg4i+pRVdMZZZTQ58/HQ\n\tOcyFtZu4bmLcIVt0OeF+Cy94wKvvSXp8gqyRfSbkcUX2e2lPfCwHWE04J0nQVn066k\n\tGQO0w3VFtpulwxStMYV1ACA+yyo0LstDT9FKT7u8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"i4ryLE0s\"; dkim-atps=neutral","Date":"Tue, 20 Sep 2022 22:11:10 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-5-laurent.pinchart@ideasonboard.com>\n\t<166368108528.3912877.2379777433335617224@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166368108528.3912877.2379777433335617224@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25031,"web_url":"https://patchwork.libcamera.org/comment/25031/","msgid":"<166371273767.18961.16349181405264608360@Monstersaurus>","date":"2022-09-20T22:25:37","subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-09-20 20:11:10)\n> Hi Kieran,\n> \n> On Tue, Sep 20, 2022 at 02:38:05PM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)\n> > > From: Umang Jain \n> > > \n> > > Introduce a common implementation in libipa to represent the queue of\n> > > frame contexts.\n> > > \n> > > Signed-off-by: Umang Jain \n> > > Signed-off-by: Kieran Bingham \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > Changes since v3:\n> > > \n> > > - Split the IPU3 changes to a separate patch\n> > > - Use composition instead of inheritance\n> > > - Use vector instead of array for backend storage\n> > > - Make the queue size dynamic\n> > > - Rename initialise() to init()\n> > > ---\n> > > src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++\n> > > src/ipa/libipa/fc_queue.h | 108 +++++++++++++++++++++++++++++++++\n> > > src/ipa/libipa/meson.build | 2 +\n> > > 3 files changed, 227 insertions(+)\n> > > create mode 100644 src/ipa/libipa/fc_queue.cpp\n> > > create mode 100644 src/ipa/libipa/fc_queue.h\n> > > \n> > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> > > new file mode 100644\n> > > index 000000000000..b81d497e255a\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/fc_queue.cpp\n> > > @@ -0,0 +1,117 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Google Inc.\n> > > + *\n> > > + * fc_queue.cpp - IPA Frame context queue\n> > > + */\n> > > +\n> > > +#include \"fc_queue.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(FCQueue)\n> > > +\n> > > +namespace ipa {\n> > > +\n> > > +/**\n> > > + * \\file fc_queue.h\n> > > + * \\brief Queue to access per-frame Context\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\class FCQueue\n> > > + * \\brief A support class for queueing FrameContext instances through the IPA\n> > > + * \\tparam FrameContext The IPA specific FrameContext derived class type\n> \n> I'll s/IPA specific/IPA-specific/\n\nSure\n\n> \n> > ; or , after FrameContext?\n> \n> Which one, the first or second ? The first FrameContext is the template\n> parameter name and thus shouldn't be followed by anything, and the\n> second one doesn't seem to require a comma.\n\nThe first. It reads like:\n\nA vehicle for transporting Maize samples through the processing factory\n\\tparam Factory The place where maize is processed.\n\nYou've gone from a type to a capital \"The\" which is describing the\nprevious item, without a pause.\n\nI have no idea how to 'name' that - but it triggers the big red alarm\nfor \"This doesn't look right in native English\".\n\n> > > + * The frame context queue provides a simple circular buffer implementation to\n> > > + * store IPA specific context for each frame through its lifetime within the\n> > > + * IPA.\n> > > + *\n> > > + * FrameContext instances are expected to be referenced by a monotonically\n> > > + * increasing sequence count corresponding to a frame sequence number.\n> > > + *\n> > > + * A FrameContext is initialised for a given frame when the corresponding\n> > > + * Request is first queued into the IPA. From that point on the FrameContext can\n> > > + * be obtained by the IPA and its algorithms by referencing it from the frame\n> > > + * sequence number.\n> > > + *\n> > > + * A frame sequence number from the image source must correspond to the request\n> > > + * sequence number for this implementation to be supported in an IPA. It is\n> > > + * expected that the same sequence number will be used to reference the context\n> > > + * of the frame from the point of queueing the request, specifying controls for\n> > > + * a given frame, and processing of any ISP related controls and statistics for\n> > > + * the same corresponding image.\n> > > + *\n> > > + * IPA specific frame context implementations shall inherit from the\n> > > + * IPAFrameContext base class to support the minimum required features for a\n> > > + * FrameContext.\n> > > + *\n> > > + * FrameContexts are overwritten when they are recycled and re-initialised by\n> > > + * the first access made on them by either init(frame) or get(frame). It is\n> > > + * required that the number of available slots in the frame context queue is\n> > > + * larger or equal to the maximum number of in-flight requests a pipeline can\n> > > + * handle to avoid overwriting frame context instances that still have to be\n> > > + * processed.\n> > > + *\n> > > + * In the case an application does not queue requests to the camera fast\n> > > + * enough, frames can be produced and processed by the IPA without a\n> > > + * corresponding Request being queued. In this case the IPA algorithm\n> > > + * will try to access the FrameContext with a call to get() before it\n> > > + * had been initialized at queueRequest() time. In this case, there\n> > > + * is no way the controls associated with the late Request could be\n> > > + * applied in time, as the frame as already been processed by the IPA.\n> > > + *\n> > > + * \\todo Set an error flag for per-frame control errors.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn FCQueue::FCQueue(unsigned int size)\n> > > + * \\brief Construct a frame context queue of a specified size\n> > > + * \\param[in] size The number of contexts in the queue\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn FCQueue::clear()\n> > > + * \\brief Clear the context queue\n> > > + *\n> > > + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> > > + * IPA modules are expected to clear the frame context queue at the beginning of\n> > > + * a new streaming session, in IPAModule::start().\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn FCQueue::init(uint32_t frame)\n> > \n> > I saw you've questioned the name here.\n> > \n> > I don't mind if this is init(), alloc() new() ?\n> > \n> > We don't (yet?) have an explicit free/delete/release of the FCQueue, as\n> > they stay in memory until something overwrites them like an LRU....\n> \n> We shouldn't allocate memory dynamically, so there will be no memory\n> allocation as such, but the concept of allocation a frame context from\n> the queue still applies. I indeed prefer alloc() over init(). Better\n> names are also possible. I'll see what I can do in the next version.\n> \n> > > + * \\brief Initialize and return the Frame Context at slot specified by \\a frame\n> > > + * \\param[in] frame The frame context sequence number\n> > > + *\n> > > + * The first call to obtain a FrameContext from the FCQueue should be handled\n> > > + * through this call. The FrameContext will be initialised for the frame and\n> > > + * returned to the caller if it was not already initialised.\n> > > + *\n> > > + * If the FrameContext was already initialized for this sequence, a warning will\n> > > + * be reported and the previously initialized FrameContext is returned.\n> > > + *\n> > > + * Frame contexts are expected to be initialised when a Request is first passed\n> > > + * to the IPA module in IPAModule::queueRequest().\n> > > + *\n> > > + * \\return A reference to the FrameContext for sequence \\a frame\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn FCQueue::get()\n> > > + * \\brief Obtain the Frame Context at slot specified by \\a frame\n> > > + * \\param[in] frame The frame context sequence number\n> > > + *\n> > > + * Obtains an existing FrameContext from the queue and returns it to the caller.\n> > > + *\n> > > + * If the FrameContext is not correctly initialised for the \\a frame, it will be\n> > > + * initialised.\n> > > + *\n> > > + * \\return A reference to the FrameContext for sequence \\a frame\n> > > + */\n> > > +\n> > > +} /* namespace ipa */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > > new file mode 100644\n> > > index 000000000000..0f3af0f3a189\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/fc_queue.h\n> > > @@ -0,0 +1,108 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Google Inc.\n> > > + *\n> > > + * fc_queue.h - IPA Frame context queue\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(FCQueue)\n> > > +\n> > > +namespace ipa {\n> > > +\n> > > +template\n> > > +class FCQueue\n> > > +{\n> > > +public:\n> > > + FCQueue(unsigned int size)\n> > > + : contexts_(size)\n> > > + {\n> > > + }\n> > > +\n> > > + void clear()\n> > > + {\n> > > + std::fill(contexts_.begin(), contexts_.end(), FrameContext{});\n> > > + }\n> > > +\n> > > + FrameContext &init(const uint32_t frame)\n> > > + {\n> > > + FrameContext &frameContext = contexts_[frame % contexts_.size()];\n> > > +\n> > > + /*\n> > > + * Do not re-initialise if a get() call has already fetched this\n> > > + * frame context to preseve the context.\n> > > + *\n> > > + * \\todo If the the sequence number of the context to initialise\n> > > + * is smaller than the sequence number of the queue slot to use,\n> > > + * it means that we had a serious request underrun and more\n> > > + * frames than the queue size has been produced since the last\n> > > + * time the application has queued a request. Does this deserve\n> > > + * an error condition ?\n> > > + */\n> > > + if (frame != 0 && frame <= frameContext.frame)\n> > > + LOG(FCQueue, Warning)\n> > > + << \"Frame \" << frame << \" already initialised\";\n> > > + else\n> > > + init(frameContext, frame);\n> > > +\n> > > + return frameContext;\n> > > + }\n> > > +\n> > > + FrameContext &get(uint32_t frame)\n> > > + {\n> > > + FrameContext &frameContext = contexts_[frame % contexts_.size()];\n> > > +\n> > > + /*\n> > > + * If the IPA algorithms try to access a frame context slot which\n> > > + * has been already overwritten by a newer context, it means the\n> > > + * frame context queue has overflowed and the desired context\n> > > + * has been forever lost. The pipeline handler shall avoid\n> > > + * queueing more requests to the IPA than the frame context\n> > > + * queue size.\n> > > + */\n> > > + if (frame < frameContext.frame)\n> > > + LOG(FCQueue, Fatal) << \"Frame Context for \" << frame\n> > > + << \" has been overwritten by \"\n> > > + << frameContext.frame;\n> > > +\n> > > + if (frame == frameContext.frame)\n> > > + return frameContext;\n> > > +\n> > > + /*\n> > > + * The frame context has been retrieved before it was\n> > > + * initialised through the initialise() call. This indicates an\n> > > + * algorithm attempted to access a Frame context before it was\n> > > + * queued to the IPA. Controls applied for this request may be\n> > \n> > double space between 'IPA. Controls'\n> > \n> > > + * left unhandled.\n> > > + *\n> > > + * \\todo Set an error flag for per-frame control errors.\n> > > + */\n> > > + LOG(FCQueue, Warning)\n> > > + << \"Obtained an uninitialised FrameContext for \" << frame;\n> > > +\n> > > + init(frameContext, frame);\n> > > +\n> > > + return frameContext;\n> > > + }\n> > > +\n> > > +private:\n> > > + void init(FrameContext &frameContext, const uint32_t frame)\n> > > + {\n> > > + frameContext = {};\n> > > + frameContext.frame = frame;\n> > \n> > Aha this is better.\n> > \n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > + }\n> > > +\n> > > + std::vector contexts_;\n> > > +};\n> > > +\n> > > +} /* namespace ipa */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > index fb894bc614af..016b8e0ec9be 100644\n> > > --- a/src/ipa/libipa/meson.build\n> > > +++ b/src/ipa/libipa/meson.build\n> > > @@ -3,6 +3,7 @@\n> > > libipa_headers = files([\n> > > 'algorithm.h',\n> > > 'camera_sensor_helper.h',\n> > > + 'fc_queue.h',\n> > > 'histogram.h',\n> > > 'module.h',\n> > > ])\n> > > @@ -10,6 +11,7 @@ libipa_headers = files([\n> > > libipa_sources = files([\n> > > 'algorithm.cpp',\n> > > 'camera_sensor_helper.cpp',\n> > > + 'fc_queue.cpp',\n> > > 'histogram.cpp',\n> > > 'module.cpp',\n> > > ])\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 A565EC0DA4\n\tfor ;\n\tTue, 20 Sep 2022 22:25:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C38C8621CB;\n\tWed, 21 Sep 2022 00:25:42 +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 20CAE61F85\n\tfor ;\n\tWed, 21 Sep 2022 00:25:41 +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 7ED196BB;\n\tWed, 21 Sep 2022 00:25:40 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663712742;\n\tbh=us9hP4dVNisiETFtIwVSPvoeLtZIDlZ0A9+lNRuHrjg=;\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=of6OFpyPPmVAb0Vq5jALt6JqV7L3YYZ/P7SWWXFFzcvAo9f77k3SdwsNTY0uslwOl\n\tdImvB+7x9AYjoZ0gsOuD6ODvCHKOJwIZ/KR9mOVAqIj2m888dWT2j8Z9jl36+4OHws\n\tQtUjukkttUc6h5lJQexT6U5E3CQjjyYI+jJlaNTZvmIyNm/973tcQQSJe0U6e4MLMk\n\tNdGsNxXUD8r07M0TSZFI8vqkf2OLwAniZTHxPSk7YH7nfErsL8PpL7dgQJIZrtJOrl\n\t/fV/LHy/8SNtrg/AOp/Wvr6Et/siPK/LaDAwFb2UEb51DnKWwC2evDBYqTk+yJHyuU\n\tWtXsciecObxPQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663712740;\n\tbh=us9hP4dVNisiETFtIwVSPvoeLtZIDlZ0A9+lNRuHrjg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=vI8DzDQUpun0u2OOwjDNjyUnsnMIAp5sz69Y6CdTAqKNa8VVOAsd/difMrpCtMgel\n\tUqnj9p1Suiy+LgsCe8TeihFV4Q1YfwK/Do7nJ5l1ZsrJhbyGybEWTWOeyLUlPISDT4\n\twJ6QuIbCGgRJ2szwYk7Pv/JPcE22LlNGE0NHtsv4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vI8DzDQU\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-5-laurent.pinchart@ideasonboard.com>\n\t<166368108528.3912877.2379777433335617224@Monstersaurus>\n\t","To":"Laurent Pinchart ","Date":"Tue, 20 Sep 2022 23:25:37 +0100","Message-ID":"<166371273767.18961.16349181405264608360@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","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":25032,"web_url":"https://patchwork.libcamera.org/comment/25032/","msgid":"","date":"2022-09-20T22:29:56","subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Sep 20, 2022 at 11:25:37PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2022-09-20 20:11:10)\n> > On Tue, Sep 20, 2022 at 02:38:05PM +0100, Kieran Bingham wrote:\n> > > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)\n> > > > From: Umang Jain \n> > > > \n> > > > Introduce a common implementation in libipa to represent the queue of\n> > > > frame contexts.\n> > > > \n> > > > Signed-off-by: Umang Jain \n> > > > Signed-off-by: Kieran Bingham \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > Changes since v3:\n> > > > \n> > > > - Split the IPU3 changes to a separate patch\n> > > > - Use composition instead of inheritance\n> > > > - Use vector instead of array for backend storage\n> > > > - Make the queue size dynamic\n> > > > - Rename initialise() to init()\n> > > > ---\n> > > > src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++\n> > > > src/ipa/libipa/fc_queue.h | 108 +++++++++++++++++++++++++++++++++\n> > > > src/ipa/libipa/meson.build | 2 +\n> > > > 3 files changed, 227 insertions(+)\n> > > > create mode 100644 src/ipa/libipa/fc_queue.cpp\n> > > > create mode 100644 src/ipa/libipa/fc_queue.h\n> > > > \n> > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..b81d497e255a\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/fc_queue.cpp\n> > > > @@ -0,0 +1,117 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Google Inc.\n> > > > + *\n> > > > + * fc_queue.cpp - IPA Frame context queue\n> > > > + */\n> > > > +\n> > > > +#include \"fc_queue.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(FCQueue)\n> > > > +\n> > > > +namespace ipa {\n> > > > +\n> > > > +/**\n> > > > + * \\file fc_queue.h\n> > > > + * \\brief Queue to access per-frame Context\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\class FCQueue\n> > > > + * \\brief A support class for queueing FrameContext instances through the IPA\n> > > > + * \\tparam FrameContext The IPA specific FrameContext derived class type\n> > \n> > I'll s/IPA specific/IPA-specific/\n> \n> Sure\n> \n> > > ; or , after FrameContext?\n> > \n> > Which one, the first or second ? The first FrameContext is the template\n> > parameter name and thus shouldn't be followed by anything, and the\n> > second one doesn't seem to require a comma.\n> \n> The first. It reads like:\n> \n> A vehicle for transporting Maize samples through the processing factory\n> \\tparam Factory The place where maize is processed.\n> \n> You've gone from a type to a capital \"The\" which is describing the\n> previous item, without a pause.\n> \n> I have no idea how to 'name' that - but it triggers the big red alarm\n> for \"This doesn't look right in native English\".\n\n\\tparam is like \\param, but for template parameters, not function\nparameters. The \\brief and \\tparam lines are distinct, the second isn't\na continuation of the first.\n\n> > > > + * The frame context queue provides a simple circular buffer implementation to\n> > > > + * store IPA specific context for each frame through its lifetime within the\n> > > > + * IPA.\n> > > > + *\n> > > > + * FrameContext instances are expected to be referenced by a monotonically\n> > > > + * increasing sequence count corresponding to a frame sequence number.\n> > > > + *\n> > > > + * A FrameContext is initialised for a given frame when the corresponding\n> > > > + * Request is first queued into the IPA. From that point on the FrameContext can\n> > > > + * be obtained by the IPA and its algorithms by referencing it from the frame\n> > > > + * sequence number.\n> > > > + *\n> > > > + * A frame sequence number from the image source must correspond to the request\n> > > > + * sequence number for this implementation to be supported in an IPA. It is\n> > > > + * expected that the same sequence number will be used to reference the context\n> > > > + * of the frame from the point of queueing the request, specifying controls for\n> > > > + * a given frame, and processing of any ISP related controls and statistics for\n> > > > + * the same corresponding image.\n> > > > + *\n> > > > + * IPA specific frame context implementations shall inherit from the\n> > > > + * IPAFrameContext base class to support the minimum required features for a\n> > > > + * FrameContext.\n> > > > + *\n> > > > + * FrameContexts are overwritten when they are recycled and re-initialised by\n> > > > + * the first access made on them by either init(frame) or get(frame). It is\n> > > > + * required that the number of available slots in the frame context queue is\n> > > > + * larger or equal to the maximum number of in-flight requests a pipeline can\n> > > > + * handle to avoid overwriting frame context instances that still have to be\n> > > > + * processed.\n> > > > + *\n> > > > + * In the case an application does not queue requests to the camera fast\n> > > > + * enough, frames can be produced and processed by the IPA without a\n> > > > + * corresponding Request being queued. In this case the IPA algorithm\n> > > > + * will try to access the FrameContext with a call to get() before it\n> > > > + * had been initialized at queueRequest() time. In this case, there\n> > > > + * is no way the controls associated with the late Request could be\n> > > > + * applied in time, as the frame as already been processed by the IPA.\n> > > > + *\n> > > > + * \\todo Set an error flag for per-frame control errors.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn FCQueue::FCQueue(unsigned int size)\n> > > > + * \\brief Construct a frame context queue of a specified size\n> > > > + * \\param[in] size The number of contexts in the queue\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn FCQueue::clear()\n> > > > + * \\brief Clear the context queue\n> > > > + *\n> > > > + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> > > > + * IPA modules are expected to clear the frame context queue at the beginning of\n> > > > + * a new streaming session, in IPAModule::start().\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn FCQueue::init(uint32_t frame)\n> > > \n> > > I saw you've questioned the name here.\n> > > \n> > > I don't mind if this is init(), alloc() new() ?\n> > > \n> > > We don't (yet?) have an explicit free/delete/release of the FCQueue, as\n> > > they stay in memory until something overwrites them like an LRU....\n> > \n> > We shouldn't allocate memory dynamically, so there will be no memory\n> > allocation as such, but the concept of allocation a frame context from\n> > the queue still applies. I indeed prefer alloc() over init(). Better\n> > names are also possible. I'll see what I can do in the next version.\n> > \n> > > > + * \\brief Initialize and return the Frame Context at slot specified by \\a frame\n> > > > + * \\param[in] frame The frame context sequence number\n> > > > + *\n> > > > + * The first call to obtain a FrameContext from the FCQueue should be handled\n> > > > + * through this call. The FrameContext will be initialised for the frame and\n> > > > + * returned to the caller if it was not already initialised.\n> > > > + *\n> > > > + * If the FrameContext was already initialized for this sequence, a warning will\n> > > > + * be reported and the previously initialized FrameContext is returned.\n> > > > + *\n> > > > + * Frame contexts are expected to be initialised when a Request is first passed\n> > > > + * to the IPA module in IPAModule::queueRequest().\n> > > > + *\n> > > > + * \\return A reference to the FrameContext for sequence \\a frame\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn FCQueue::get()\n> > > > + * \\brief Obtain the Frame Context at slot specified by \\a frame\n> > > > + * \\param[in] frame The frame context sequence number\n> > > > + *\n> > > > + * Obtains an existing FrameContext from the queue and returns it to the caller.\n> > > > + *\n> > > > + * If the FrameContext is not correctly initialised for the \\a frame, it will be\n> > > > + * initialised.\n> > > > + *\n> > > > + * \\return A reference to the FrameContext for sequence \\a frame\n> > > > + */\n> > > > +\n> > > > +} /* namespace ipa */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > > > new file mode 100644\n> > > > index 000000000000..0f3af0f3a189\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/fc_queue.h\n> > > > @@ -0,0 +1,108 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Google Inc.\n> > > > + *\n> > > > + * fc_queue.h - IPA Frame context queue\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(FCQueue)\n> > > > +\n> > > > +namespace ipa {\n> > > > +\n> > > > +template\n> > > > +class FCQueue\n> > > > +{\n> > > > +public:\n> > > > + FCQueue(unsigned int size)\n> > > > + : contexts_(size)\n> > > > + {\n> > > > + }\n> > > > +\n> > > > + void clear()\n> > > > + {\n> > > > + std::fill(contexts_.begin(), contexts_.end(), FrameContext{});\n> > > > + }\n> > > > +\n> > > > + FrameContext &init(const uint32_t frame)\n> > > > + {\n> > > > + FrameContext &frameContext = contexts_[frame % contexts_.size()];\n> > > > +\n> > > > + /*\n> > > > + * Do not re-initialise if a get() call has already fetched this\n> > > > + * frame context to preseve the context.\n> > > > + *\n> > > > + * \\todo If the the sequence number of the context to initialise\n> > > > + * is smaller than the sequence number of the queue slot to use,\n> > > > + * it means that we had a serious request underrun and more\n> > > > + * frames than the queue size has been produced since the last\n> > > > + * time the application has queued a request. Does this deserve\n> > > > + * an error condition ?\n> > > > + */\n> > > > + if (frame != 0 && frame <= frameContext.frame)\n> > > > + LOG(FCQueue, Warning)\n> > > > + << \"Frame \" << frame << \" already initialised\";\n> > > > + else\n> > > > + init(frameContext, frame);\n> > > > +\n> > > > + return frameContext;\n> > > > + }\n> > > > +\n> > > > + FrameContext &get(uint32_t frame)\n> > > > + {\n> > > > + FrameContext &frameContext = contexts_[frame % contexts_.size()];\n> > > > +\n> > > > + /*\n> > > > + * If the IPA algorithms try to access a frame context slot which\n> > > > + * has been already overwritten by a newer context, it means the\n> > > > + * frame context queue has overflowed and the desired context\n> > > > + * has been forever lost. The pipeline handler shall avoid\n> > > > + * queueing more requests to the IPA than the frame context\n> > > > + * queue size.\n> > > > + */\n> > > > + if (frame < frameContext.frame)\n> > > > + LOG(FCQueue, Fatal) << \"Frame Context for \" << frame\n> > > > + << \" has been overwritten by \"\n> > > > + << frameContext.frame;\n> > > > +\n> > > > + if (frame == frameContext.frame)\n> > > > + return frameContext;\n> > > > +\n> > > > + /*\n> > > > + * The frame context has been retrieved before it was\n> > > > + * initialised through the initialise() call. This indicates an\n> > > > + * algorithm attempted to access a Frame context before it was\n> > > > + * queued to the IPA. Controls applied for this request may be\n> > > \n> > > double space between 'IPA. Controls'\n> > > \n> > > > + * left unhandled.\n> > > > + *\n> > > > + * \\todo Set an error flag for per-frame control errors.\n> > > > + */\n> > > > + LOG(FCQueue, Warning)\n> > > > + << \"Obtained an uninitialised FrameContext for \" << frame;\n> > > > +\n> > > > + init(frameContext, frame);\n> > > > +\n> > > > + return frameContext;\n> > > > + }\n> > > > +\n> > > > +private:\n> > > > + void init(FrameContext &frameContext, const uint32_t frame)\n> > > > + {\n> > > > + frameContext = {};\n> > > > + frameContext.frame = frame;\n> > > \n> > > Aha this is better.\n> > > \n> > > \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > > + }\n> > > > +\n> > > > + std::vector contexts_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > > index fb894bc614af..016b8e0ec9be 100644\n> > > > --- a/src/ipa/libipa/meson.build\n> > > > +++ b/src/ipa/libipa/meson.build\n> > > > @@ -3,6 +3,7 @@\n> > > > libipa_headers = files([\n> > > > 'algorithm.h',\n> > > > 'camera_sensor_helper.h',\n> > > > + 'fc_queue.h',\n> > > > 'histogram.h',\n> > > > 'module.h',\n> > > > ])\n> > > > @@ -10,6 +11,7 @@ libipa_headers = files([\n> > > > libipa_sources = files([\n> > > > 'algorithm.cpp',\n> > > > 'camera_sensor_helper.cpp',\n> > > > + 'fc_queue.cpp',\n> > > > 'histogram.cpp',\n> > > > 'module.cpp',\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 CD276C3272\n\tfor ;\n\tTue, 20 Sep 2022 22:30:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F392C621CC;\n\tWed, 21 Sep 2022 00:30:13 +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 EDFDF61F85\n\tfor ;\n\tWed, 21 Sep 2022 00:30:11 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 402CD6BB;\n\tWed, 21 Sep 2022 00:30:11 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663713014;\n\tbh=C9fL68A6X3/BzQO3bgVWP5FtS2fuyKoTADYPZrs5Sok=;\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=kDmCLjRTzWBPydnAZddmrIEJxIkFQ8gYnsU97k/9zEVZES2B8AK5Yg4S3CqBT8LK5\n\tQcKj193zmUkLKDUH5ABtXkKlRiHVRp6Wqi7GqYaKCPxdxR20cyRn20gtAb18uLbiXb\n\tIIH4a74XPclRr8vBc9YLpVxSTB+lQTGWFHL7dLRj5HpJbxPcIvQF+jr/H0BBhjnKq3\n\tGU5AO/hNokhIVNd+VWfrfdT2OGifWf71yUeeFnuZQozAPMlGpfX3qsPsh+6CzBCnM8\n\t33Rxwatxl3gdIuio1OoftLfeoQwmQ8l+/cjFFTZ9Pl4C5099w7Txiqw07QatgLjMFR\n\tal/UDjkKausNg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663713011;\n\tbh=C9fL68A6X3/BzQO3bgVWP5FtS2fuyKoTADYPZrs5Sok=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FcfoGvCme22gURrKX6q8fkVp2TYW+xH3kxBh+MvYaKg56+9x+38164GUNarwihOpu\n\tfz9GNJsKMcyr1vSD4cUb6dJy7RT1dufVBLFRdZPdaGSqpAHRvhsgVvmY4jibquIDha\n\tWPTSKKJNpUxaCDl8kmXpCNAchm/pTVdLp5wJrezQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FcfoGvCm\"; dkim-atps=neutral","Date":"Wed, 21 Sep 2022 01:29:56 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-5-laurent.pinchart@ideasonboard.com>\n\t<166368108528.3912877.2379777433335617224@Monstersaurus>\n\t\n\t<166371273767.18961.16349181405264608360@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166371273767.18961.16349181405264608360@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25046,"web_url":"https://patchwork.libcamera.org/comment/25046/","msgid":"<166371801871.18961.9351820634394073865@Monstersaurus>","date":"2022-09-20T23:53:38","subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-09-20 23:29:56)\n> On Tue, Sep 20, 2022 at 11:25:37PM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2022-09-20 20:11:10)\n> > > On Tue, Sep 20, 2022 at 02:38:05PM +0100, Kieran Bingham wrote:\n> > > > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)\n> > > > > From: Umang Jain \n> > > > > \n> > > > > Introduce a common implementation in libipa to represent the queue of\n> > > > > frame contexts.\n> > > > > \n> > > > > Signed-off-by: Umang Jain \n> > > > > Signed-off-by: Kieran Bingham \n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > Changes since v3:\n> > > > > \n> > > > > - Split the IPU3 changes to a separate patch\n> > > > > - Use composition instead of inheritance\n> > > > > - Use vector instead of array for backend storage\n> > > > > - Make the queue size dynamic\n> > > > > - Rename initialise() to init()\n> > > > > ---\n> > > > > src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++\n> > > > > src/ipa/libipa/fc_queue.h | 108 +++++++++++++++++++++++++++++++++\n> > > > > src/ipa/libipa/meson.build | 2 +\n> > > > > 3 files changed, 227 insertions(+)\n> > > > > create mode 100644 src/ipa/libipa/fc_queue.cpp\n> > > > > create mode 100644 src/ipa/libipa/fc_queue.h\n> > > > > \n> > > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> > > > > new file mode 100644\n> > > > > index 000000000000..b81d497e255a\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/libipa/fc_queue.cpp\n> > > > > @@ -0,0 +1,117 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2022, Google Inc.\n> > > > > + *\n> > > > > + * fc_queue.cpp - IPA Frame context queue\n> > > > > + */\n> > > > > +\n> > > > > +#include \"fc_queue.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +LOG_DEFINE_CATEGORY(FCQueue)\n> > > > > +\n> > > > > +namespace ipa {\n> > > > > +\n> > > > > +/**\n> > > > > + * \\file fc_queue.h\n> > > > > + * \\brief Queue to access per-frame Context\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\class FCQueue\n> > > > > + * \\brief A support class for queueing FrameContext instances through the IPA\n> > > > > + * \\tparam FrameContext The IPA specific FrameContext derived class type\n> > > \n> > > I'll s/IPA specific/IPA-specific/\n> > \n> > Sure\n> > \n> > > > ; or , after FrameContext?\n> > > \n> > > Which one, the first or second ? The first FrameContext is the template\n> > > parameter name and thus shouldn't be followed by anything, and the\n> > > second one doesn't seem to require a comma.\n> > \n> > The first. It reads like:\n> > \n> > A vehicle for transporting Maize samples through the processing factory\n> > \\tparam Factory The place where maize is processed.\n> > \n> > You've gone from a type to a capital \"The\" which is describing the\n> > previous item, without a pause.\n> > \n> > I have no idea how to 'name' that - but it triggers the big red alarm\n> > for \"This doesn't look right in native English\".\n> \n> \\tparam is like \\param, but for template parameters, not function\n> parameters. The \\brief and \\tparam lines are distinct, the second isn't\n> a continuation of the first.\n\nArgh - yes - I was reading the line like a continuation.\n\nI'll go back to sleep ;-) But now I'll put up more of a fight for\nindentation on continued lines ...\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 22A15C3272\n\tfor ;\n\tTue, 20 Sep 2022 23:53:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 753CE61F7D;\n\tWed, 21 Sep 2022 01:53:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2882C61F7D\n\tfor ;\n\tWed, 21 Sep 2022 01:53:42 +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 92FCB415;\n\tWed, 21 Sep 2022 01:53:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663718023;\n\tbh=ymTE8IUXSd6wMiS2nE0fp+T8ZFdh+G4zpGhDXuLiSjw=;\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=ygOkwXDXVEHw/FtO82fyHB6asxO3oRh/J2aobr2zJk9cKKKBVLUCUZ6RvdqBtNO5x\n\tD9ZHb1LyJSt5s39ch0DGYvT2shvz7dDOeyAtbESrQzpJzQlnnu/XQe4pfjYYHciZ20\n\tgre82xEZX7aAy8jg4eFutG4Va/9N/pwHPfusdt504XP4Xucn4djbYDN9/UQAtzc9qo\n\tNJHAWinIdOIF9UxuePRRXI3SD2W0VoZOlmcAZIIbnwN/FQsnTgEeQplnq2h66+8ptD\n\tKC7Lt2Jv7QI66fV1n6d+hNS3kfx4zAfhspIGJQqpy9TkkLTxFByfkcdgVIpDsbZ9fV\n\tVCfiUr6Rpx98A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663718021;\n\tbh=ymTE8IUXSd6wMiS2nE0fp+T8ZFdh+G4zpGhDXuLiSjw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=h3FsIpNfWcA0hJvTTIxQZ21trskvX3/Zpnp1ONw0ZFcC26i++hlNOrxOzjgvgBBcA\n\tdZ4bKaQtb1+8RtFshq/A+Ac3xXUajqsqglDPheN1Av1yXDK+GVJdffQiMRcysJVbrB\n\tObjUWd7hIvlN6KL/Xd8cBGJaBzZnRcMOJtC3UdKk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"h3FsIpNf\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-5-laurent.pinchart@ideasonboard.com>\n\t<166368108528.3912877.2379777433335617224@Monstersaurus>\n\t\n\t<166371273767.18961.16349181405264608360@Monstersaurus>\n\t","To":"Laurent Pinchart ","Date":"Wed, 21 Sep 2022 00:53:38 +0100","Message-ID":"<166371801871.18961.9351820634394073865@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","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":25051,"web_url":"https://patchwork.libcamera.org/comment/25051/","msgid":"<20220921174912.hf4v2vjdghmqliln@uno.localdomain>","date":"2022-09-21T17:49:12","subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Thu, Sep 08, 2022 at 04:41:32AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> From: Umang Jain \n>\n> Introduce a common implementation in libipa to represent the queue of\n> frame contexts.\n>\n> Signed-off-by: Umang Jain \n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> ---\n> Changes since v3:\n>\n> - Split the IPU3 changes to a separate patch\n> - Use composition instead of inheritance\n\n\n\n> - Use vector instead of array for backend storage\n\nI guess this is because now the FCQ size has dynamic size ?\n\n> - Make the queue size dynamic\n> - Rename initialise() to init()\n> ---\n> src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/fc_queue.h | 108 +++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 227 insertions(+)\n> create mode 100644 src/ipa/libipa/fc_queue.cpp\n> create mode 100644 src/ipa/libipa/fc_queue.h\n>\n> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> new file mode 100644\n> index 000000000000..b81d497e255a\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.cpp\n> @@ -0,0 +1,117 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * fc_queue.cpp - IPA Frame context queue\n> + */\n> +\n> +#include \"fc_queue.h\"\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(FCQueue)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\file fc_queue.h\n> + * \\brief Queue to access per-frame Context\n\ncontexts maybe, or does Context name a type\n\n> + */\n> +\n> +/**\n> + * \\class FCQueue\n> + * \\brief A support class for queueing FrameContext instances through the IPA\n> + * \\tparam FrameContext The IPA specific FrameContext derived class type\n> + *\n> + * The frame context queue provides a simple circular buffer implementation to\n> + * store IPA specific context for each frame through its lifetime within the\n> + * IPA.\n> + *\n> + * FrameContext instances are expected to be referenced by a monotonically\n> + * increasing sequence count corresponding to a frame sequence number.\n> + *\n> + * A FrameContext is initialised for a given frame when the corresponding\n> + * Request is first queued into the IPA. From that point on the FrameContext can\n> + * be obtained by the IPA and its algorithms by referencing it from the frame\n> + * sequence number.\n> + *\n> + * A frame sequence number from the image source must correspond to the request\n\n\"A frame sequence number\" or \"The frame sequence number\" ?\n\n> + * sequence number for this implementation to be supported in an IPA. It is\n> + * expected that the same sequence number will be used to reference the context\n> + * of the frame from the point of queueing the request, specifying controls for\n> + * a given frame, and processing of any ISP related controls and statistics for\n> + * the same corresponding image.\n> + *\n> + * IPA specific frame context implementations shall inherit from the\n> + * IPAFrameContext base class to support the minimum required features for a\n> + * FrameContext.\n> + *\n> + * FrameContexts are overwritten when they are recycled and re-initialised by\n> + * the first access made on them by either init(frame) or get(frame). It is\n> + * required that the number of available slots in the frame context queue is\n> + * larger or equal to the maximum number of in-flight requests a pipeline can\n> + * handle to avoid overwriting frame context instances that still have to be\n> + * processed.\n> + *\n> + * In the case an application does not queue requests to the camera fast\n> + * enough, frames can be produced and processed by the IPA without a\n> + * corresponding Request being queued. In this case the IPA algorithm\n> + * will try to access the FrameContext with a call to get() before it\n> + * had been initialized at queueRequest() time. In this case, there\n> + * is no way the controls associated with the late Request could be\n> + * applied in time, as the frame as already been processed by the IPA.\n\ns/as already/has already/\n\n> + *\n> + * \\todo Set an error flag for per-frame control errors.\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::FCQueue(unsigned int size)\n> + * \\brief Construct a frame context queue of a specified size\n> + * \\param[in] size The number of contexts in the queue\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::clear()\n> + * \\brief Clear the context queue\n> + *\n> + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> + * IPA modules are expected to clear the frame context queue at the beginning of\n> + * a new streaming session, in IPAModule::start().\n\nI recall we had a discussion and if I'm not mistaken clearing the FCQ at\nstart() time would create issues with Requests queued to the camera\nbefore Camera::start() was called.\n\niirc we settled on stop() as the \"right\" place to clear the FCQ\nbetween streaming sessions (assuming it gets initialized in a clean\nstate ofc).\n\nHowever the doc here matches the v3 version.\n\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::init(uint32_t frame)\n> + * \\brief Initialize and return the Frame Context at slot specified by \\a frame\n> + * \\param[in] frame The frame context sequence number\n> + *\n> + * The first call to obtain a FrameContext from the FCQueue should be handled\n> + * through this call. The FrameContext will be initialised for the frame and\n> + * returned to the caller if it was not already initialised.\n\nThis sounds like \"it is returned to the caller if it was not already\ninitialised.\"\n\nWhat about \"The FrameContext will be initialised, if not initialised\nalready, and returned to the caller.\"\n\n> + *\n> + * If the FrameContext was already initialized for this sequence, a warning will\n> + * be reported and the previously initialized FrameContext is returned.\n> + *\n> + * Frame contexts are expected to be initialised when a Request is first passed\n> + * to the IPA module in IPAModule::queueRequest().\n> + *\n> + * \\return A reference to the FrameContext for sequence \\a frame\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::get()\n> + * \\brief Obtain the Frame Context at slot specified by \\a frame\n> + * \\param[in] frame The frame context sequence number\n> + *\n> + * Obtains an existing FrameContext from the queue and returns it to the caller.\n> + *\n> + * If the FrameContext is not correctly initialised for the \\a frame, it will be\n> + * initialised.\n> + *\n> + * \\return A reference to the FrameContext for sequence \\a frame\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> new file mode 100644\n> index 000000000000..0f3af0f3a189\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.h\n> @@ -0,0 +1,108 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * fc_queue.h - IPA Frame context queue\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(FCQueue)\n> +\n> +namespace ipa {\n> +\n> +template\n> +class FCQueue\n> +{\n> +public:\n> +\tFCQueue(unsigned int size)\n> +\t\t: contexts_(size)\n\nI'm not sure I get why the FCQ size should be dynamic. If I recall\ncorrectly Umang had this on v2 ? The size of the FCQ is a camera\nproperty which corresponds to the max number of requests in flight\na camera can handle and its fixed at compile time ?\n\nUsing a libcamera::property also guarantee that the two sizes matches.\nIs it too demanding for IPA implementations ?\n\n> +\t{\n> +\t}\n> +\n> +\tvoid clear()\n> +\t{\n> +\t\tstd::fill(contexts_.begin(), contexts_.end(), FrameContext{});\n> +\t}\n> +\n> +\tFrameContext &init(const uint32_t frame)\n> +\t{\n> +\t\tFrameContext &frameContext = contexts_[frame % contexts_.size()];\n> +\n> +\t\t/*\n> +\t\t * Do not re-initialise if a get() call has already fetched this\n> +\t\t * frame context to preseve the context.\n> +\t\t *\n> +\t\t * \\todo If the the sequence number of the context to initialise\n> +\t\t * is smaller than the sequence number of the queue slot to use,\n> +\t\t * it means that we had a serious request underrun and more\n> +\t\t * frames than the queue size has been produced since the last\n> +\t\t * time the application has queued a request. Does this deserve\n> +\t\t * an error condition ?\n> +\t\t */\n> +\t\tif (frame != 0 && frame <= frameContext.frame)\n> +\t\t\tLOG(FCQueue, Warning)\n> +\t\t\t\t<< \"Frame \" << frame << \" already initialised\";\n> +\t\telse\n> +\t\t\tinit(frameContext, frame);\n> +\n> +\t\treturn frameContext;\n> +\t}\n> +\n> +\tFrameContext &get(uint32_t frame)\n> +\t{\n> +\t\tFrameContext &frameContext = contexts_[frame % contexts_.size()];\n> +\n> +\t\t/*\n> +\t\t * If the IPA algorithms try to access a frame context slot which\n> +\t\t * has been already overwritten by a newer context, it means the\n> +\t\t * frame context queue has overflowed and the desired context\n> +\t\t * has been forever lost. The pipeline handler shall avoid\n> +\t\t * queueing more requests to the IPA than the frame context\n> +\t\t * queue size.\n> +\t\t */\n> +\t\tif (frame < frameContext.frame)\n> +\t\t\tLOG(FCQueue, Fatal) << \"Frame Context for \" << frame\n> +\t\t\t\t\t << \" has been overwritten by \"\n> +\t\t\t\t\t << frameContext.frame;\n> +\n> +\t\tif (frame == frameContext.frame)\n> +\t\t\treturn frameContext;\n> +\n> +\t\t/*\n> +\t\t * The frame context has been retrieved before it was\n> +\t\t * initialised through the initialise() call. This indicates an\n> +\t\t * algorithm attempted to access a Frame context before it was\n> +\t\t * queued to the IPA. Controls applied for this request may be\n> +\t\t * left unhandled.\n> +\t\t *\n> +\t\t * \\todo Set an error flag for per-frame control errors.\n> +\t\t */\n> +\t\tLOG(FCQueue, Warning)\n> +\t\t\t<< \"Obtained an uninitialised FrameContext for \" << frame;\n> +\n> +\t\tinit(frameContext, frame);\n> +\n> +\t\treturn frameContext;\n> +\t}\n> +\n> +private:\n> +\tvoid init(FrameContext &frameContext, const uint32_t frame)\n> +\t{\n> +\t\tframeContext = {};\n> +\t\tframeContext.frame = frame;\n> +\t}\n> +\n> +\tstd::vector contexts_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index fb894bc614af..016b8e0ec9be 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -3,6 +3,7 @@\n> libipa_headers = files([\n> 'algorithm.h',\n> 'camera_sensor_helper.h',\n> + 'fc_queue.h',\n> 'histogram.h',\n> 'module.h',\n> ])\n> @@ -10,6 +11,7 @@ libipa_headers = files([\n> libipa_sources = files([\n> 'algorithm.cpp',\n> 'camera_sensor_helper.cpp',\n> + 'fc_queue.cpp',\n> 'histogram.cpp',\n> 'module.cpp',\n> ])\n> --\n> Regards,\n>\n> Laurent Pinchart\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 282C0C0DA4\n\tfor ;\n\tWed, 21 Sep 2022 17:49:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0648621E8;\n\tWed, 21 Sep 2022 19:49:16 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00FAA600AA\n\tfor ;\n\tWed, 21 Sep 2022 19:49:14 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 58A481C0002;\n\tWed, 21 Sep 2022 17:49:14 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663782556;\n\tbh=aDd23f7vWHaRdmnTTuseSpjU7PM7aBug7npHUHqfc+Y=;\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=vCSSUZcTQcddCwOnjFhANmlVC1WO+X4k+IWj/fhEejC0SOCgKnq0fzPPzchgDHHzb\n\tmSgNoDiL5nw8rBs4wPiMn2e6skKQmfVKnZ/qoWL9wzTT/enA8q/OcrDBQAxsxMvq1o\n\tHn57mKe++Z3mvQnCF9MDw/WiuueAa2N+SKPSFpzGs/y2gqnt6bGyV1NG7kBfIShb+M\n\tIQ51Pw9Hy9og9HzyGpSF69EZYSAVBqumcymmo+IWBQpFD+2WphQ6ML8T9PAfywgeZF\n\tdnMJ+9jWwQ18Y2hLxXHGidwaq/UHJvIFpau+n3PiEaR+OoNakatEFOIbcFeo8Q0Gql\n\tsZHkiYLoG/P3g==","Date":"Wed, 21 Sep 2022 19:49:12 +0200","To":"Laurent Pinchart ","Message-ID":"<20220921174912.hf4v2vjdghmqliln@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220908014200.28728-5-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","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":25086,"web_url":"https://patchwork.libcamera.org/comment/25086/","msgid":"","date":"2022-09-22T19:22:16","subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Sep 21, 2022 at 07:49:12PM +0200, Jacopo Mondi wrote:\n> On Thu, Sep 08, 2022 at 04:41:32AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > From: Umang Jain \n> >\n> > Introduce a common implementation in libipa to represent the queue of\n> > frame contexts.\n> >\n> > Signed-off-by: Umang Jain \n> > Signed-off-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > Changes since v3:\n> >\n> > - Split the IPU3 changes to a separate patch\n> > - Use composition instead of inheritance\n> \n> \n> \n> > - Use vector instead of array for backend storage\n> \n> I guess this is because now the FCQ size has dynamic size ?\n\nThat's right.\n\n> > - Make the queue size dynamic\n> > - Rename initialise() to init()\n> > ---\n> > src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/fc_queue.h | 108 +++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 3 files changed, 227 insertions(+)\n> > create mode 100644 src/ipa/libipa/fc_queue.cpp\n> > create mode 100644 src/ipa/libipa/fc_queue.h\n> >\n> > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp\n> > new file mode 100644\n> > index 000000000000..b81d497e255a\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.cpp\n> > @@ -0,0 +1,117 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Google Inc.\n> > + *\n> > + * fc_queue.cpp - IPA Frame context queue\n> > + */\n> > +\n> > +#include \"fc_queue.h\"\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(FCQueue)\n> > +\n> > +namespace ipa {\n> > +\n> > +/**\n> > + * \\file fc_queue.h\n> > + * \\brief Queue to access per-frame Context\n> \n> contexts maybe, or does Context name a type\n\nMaybe Context was a type at some point, or maybe I considered turning it\ninto a type, but it's not now, so I'll use \"contexts\". I'll also replace\n\"Frame Context\" with \"FrameContext\" below.\n\n> > + */\n> > +\n> > +/**\n> > + * \\class FCQueue\n> > + * \\brief A support class for queueing FrameContext instances through the IPA\n> > + * \\tparam FrameContext The IPA specific FrameContext derived class type\n> > + *\n> > + * The frame context queue provides a simple circular buffer implementation to\n> > + * store IPA specific context for each frame through its lifetime within the\n> > + * IPA.\n> > + *\n> > + * FrameContext instances are expected to be referenced by a monotonically\n> > + * increasing sequence count corresponding to a frame sequence number.\n> > + *\n> > + * A FrameContext is initialised for a given frame when the corresponding\n> > + * Request is first queued into the IPA. From that point on the FrameContext can\n> > + * be obtained by the IPA and its algorithms by referencing it from the frame\n> > + * sequence number.\n> > + *\n> > + * A frame sequence number from the image source must correspond to the request\n> \n> \"A frame sequence number\" or \"The frame sequence number\" ?\n\nAck.\n\n> > + * sequence number for this implementation to be supported in an IPA. It is\n> > + * expected that the same sequence number will be used to reference the context\n> > + * of the frame from the point of queueing the request, specifying controls for\n> > + * a given frame, and processing of any ISP related controls and statistics for\n> > + * the same corresponding image.\n> > + *\n> > + * IPA specific frame context implementations shall inherit from the\n> > + * IPAFrameContext base class to support the minimum required features for a\n> > + * FrameContext.\n> > + *\n> > + * FrameContexts are overwritten when they are recycled and re-initialised by\n> > + * the first access made on them by either init(frame) or get(frame). It is\n> > + * required that the number of available slots in the frame context queue is\n> > + * larger or equal to the maximum number of in-flight requests a pipeline can\n> > + * handle to avoid overwriting frame context instances that still have to be\n> > + * processed.\n> > + *\n> > + * In the case an application does not queue requests to the camera fast\n> > + * enough, frames can be produced and processed by the IPA without a\n> > + * corresponding Request being queued. In this case the IPA algorithm\n> > + * will try to access the FrameContext with a call to get() before it\n> > + * had been initialized at queueRequest() time. In this case, there\n> > + * is no way the controls associated with the late Request could be\n> > + * applied in time, as the frame as already been processed by the IPA.\n> \n> s/as already/has already/\n> \n> > + *\n> > + * \\todo Set an error flag for per-frame control errors.\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::FCQueue(unsigned int size)\n> > + * \\brief Construct a frame context queue of a specified size\n> > + * \\param[in] size The number of contexts in the queue\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::clear()\n> > + * \\brief Clear the context queue\n> > + *\n> > + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> > + * IPA modules are expected to clear the frame context queue at the beginning of\n> > + * a new streaming session, in IPAModule::start().\n> \n> I recall we had a discussion and if I'm not mistaken clearing the FCQ at\n> start() time would create issues with Requests queued to the camera\n> before Camera::start() was called.\n> \n> iirc we settled on stop() as the \"right\" place to clear the FCQ\n> between streaming sessions (assuming it gets initialized in a clean\n> state ofc).\n> \n> However the doc here matches the v3 version.\n\nThat's a good point. I'll see if I can fix it.\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::init(uint32_t frame)\n> > + * \\brief Initialize and return the Frame Context at slot specified by \\a frame\n> > + * \\param[in] frame The frame context sequence number\n> > + *\n> > + * The first call to obtain a FrameContext from the FCQueue should be handled\n> > + * through this call. The FrameContext will be initialised for the frame and\n> > + * returned to the caller if it was not already initialised.\n> \n> This sounds like \"it is returned to the caller if it was not already\n> initialised.\"\n> \n> What about \"The FrameContext will be initialised, if not initialised\n> already, and returned to the caller.\"\n\nAck.\n\n> > + *\n> > + * If the FrameContext was already initialized for this sequence, a warning will\n> > + * be reported and the previously initialized FrameContext is returned.\n> > + *\n> > + * Frame contexts are expected to be initialised when a Request is first passed\n> > + * to the IPA module in IPAModule::queueRequest().\n> > + *\n> > + * \\return A reference to the FrameContext for sequence \\a frame\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::get()\n> > + * \\brief Obtain the Frame Context at slot specified by \\a frame\n> > + * \\param[in] frame The frame context sequence number\n> > + *\n> > + * Obtains an existing FrameContext from the queue and returns it to the caller.\n> > + *\n> > + * If the FrameContext is not correctly initialised for the \\a frame, it will be\n> > + * initialised.\n> > + *\n> > + * \\return A reference to the FrameContext for sequence \\a frame\n> > + */\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h\n> > new file mode 100644\n> > index 000000000000..0f3af0f3a189\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.h\n> > @@ -0,0 +1,108 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Google Inc.\n> > + *\n> > + * fc_queue.h - IPA Frame context queue\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(FCQueue)\n> > +\n> > +namespace ipa {\n> > +\n> > +template\n> > +class FCQueue\n> > +{\n> > +public:\n> > +\tFCQueue(unsigned int size)\n> > +\t\t: contexts_(size)\n> \n> I'm not sure I get why the FCQ size should be dynamic. If I recall\n> correctly Umang had this on v2 ? The size of the FCQ is a camera\n> property which corresponds to the max number of requests in flight\n> a camera can handle and its fixed at compile time ?\n> \n> Using a libcamera::property also guarantee that the two sizes matches.\n> Is it too demanding for IPA implementations ?\n\nGood point. I wanted to allow different IPA modules to use different\ncontext queue sizes, and didn't consider making this a template\nparameter. I'll check what it entails.\n\n> > +\t{\n> > +\t}\n> > +\n> > +\tvoid clear()\n> > +\t{\n> > +\t\tstd::fill(contexts_.begin(), contexts_.end(), FrameContext{});\n> > +\t}\n> > +\n> > +\tFrameContext &init(const uint32_t frame)\n> > +\t{\n> > +\t\tFrameContext &frameContext = contexts_[frame % contexts_.size()];\n> > +\n> > +\t\t/*\n> > +\t\t * Do not re-initialise if a get() call has already fetched this\n> > +\t\t * frame context to preseve the context.\n> > +\t\t *\n> > +\t\t * \\todo If the the sequence number of the context to initialise\n> > +\t\t * is smaller than the sequence number of the queue slot to use,\n> > +\t\t * it means that we had a serious request underrun and more\n> > +\t\t * frames than the queue size has been produced since the last\n> > +\t\t * time the application has queued a request. Does this deserve\n> > +\t\t * an error condition ?\n> > +\t\t */\n> > +\t\tif (frame != 0 && frame <= frameContext.frame)\n> > +\t\t\tLOG(FCQueue, Warning)\n> > +\t\t\t\t<< \"Frame \" << frame << \" already initialised\";\n> > +\t\telse\n> > +\t\t\tinit(frameContext, frame);\n> > +\n> > +\t\treturn frameContext;\n> > +\t}\n> > +\n> > +\tFrameContext &get(uint32_t frame)\n> > +\t{\n> > +\t\tFrameContext &frameContext = contexts_[frame % contexts_.size()];\n> > +\n> > +\t\t/*\n> > +\t\t * If the IPA algorithms try to access a frame context slot which\n> > +\t\t * has been already overwritten by a newer context, it means the\n> > +\t\t * frame context queue has overflowed and the desired context\n> > +\t\t * has been forever lost. The pipeline handler shall avoid\n> > +\t\t * queueing more requests to the IPA than the frame context\n> > +\t\t * queue size.\n> > +\t\t */\n> > +\t\tif (frame < frameContext.frame)\n> > +\t\t\tLOG(FCQueue, Fatal) << \"Frame Context for \" << frame\n> > +\t\t\t\t\t << \" has been overwritten by \"\n> > +\t\t\t\t\t << frameContext.frame;\n> > +\n> > +\t\tif (frame == frameContext.frame)\n> > +\t\t\treturn frameContext;\n> > +\n> > +\t\t/*\n> > +\t\t * The frame context has been retrieved before it was\n> > +\t\t * initialised through the initialise() call. This indicates an\n> > +\t\t * algorithm attempted to access a Frame context before it was\n> > +\t\t * queued to the IPA. Controls applied for this request may be\n> > +\t\t * left unhandled.\n> > +\t\t *\n> > +\t\t * \\todo Set an error flag for per-frame control errors.\n> > +\t\t */\n> > +\t\tLOG(FCQueue, Warning)\n> > +\t\t\t<< \"Obtained an uninitialised FrameContext for \" << frame;\n> > +\n> > +\t\tinit(frameContext, frame);\n> > +\n> > +\t\treturn frameContext;\n> > +\t}\n> > +\n> > +private:\n> > +\tvoid init(FrameContext &frameContext, const uint32_t frame)\n> > +\t{\n> > +\t\tframeContext = {};\n> > +\t\tframeContext.frame = frame;\n> > +\t}\n> > +\n> > +\tstd::vector contexts_;\n> > +};\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index fb894bc614af..016b8e0ec9be 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -3,6 +3,7 @@\n> > libipa_headers = files([\n> > 'algorithm.h',\n> > 'camera_sensor_helper.h',\n> > + 'fc_queue.h',\n> > 'histogram.h',\n> > 'module.h',\n> > ])\n> > @@ -10,6 +11,7 @@ libipa_headers = files([\n> > libipa_sources = files([\n> > 'algorithm.cpp',\n> > 'camera_sensor_helper.cpp',\n> > + 'fc_queue.cpp',\n> > 'histogram.cpp',\n> > 'module.cpp',\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 63BAEBD16B\n\tfor ;\n\tThu, 22 Sep 2022 19:22:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8B166220C;\n\tThu, 22 Sep 2022 21:22:33 +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 021266219A\n\tfor ;\n\tThu, 22 Sep 2022 21:22:31 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A6EA6BE;\n\tThu, 22 Sep 2022 21:22:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663874553;\n\tbh=zGi/dMEXQDsUhMRczUukBh2jy/eVGaH58ADqYvfMOeU=;\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=DYjCut9hymwcAJFnRFLtTxz3nKdJahj/6zqTsM5+lbNvZCRp/JGwzktLRO6qLF1I2\n\toE4q6RM2Mu+2fT45sKYx3lXDwGgD+f5Ccn23sdCmGxj4p3J6EKr5CgcZadpi6i3Vrh\n\tbWfTBPy4fLFLFo5cpDkMhTCjgdWp0Gde6JM++P/81r4sh0k4557CYv6gqaPVnrmOCs\n\tRw+l+Ife/0SQ5ODn5WVMU13KpZYbO1FAcP8FOX2gHmivoYNUXjPt9GzTG/CRot72TK\n\tO76A218gz4Oc+CSE/CeyMlSuXUMpsOIHvzxwiNCCS1+Zuu5IC10sbHE3FD2jWA64ND\n\tmUxjqOPHUyk/w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663874551;\n\tbh=zGi/dMEXQDsUhMRczUukBh2jy/eVGaH58ADqYvfMOeU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SwHARVyJ00CqLWRKp9JRSycB18qyjhI1lYzd5h4oHDwihnrId3f4PdslVvFyEWRuA\n\ti9BLsuboYRdJLE+2q+f3B/62Aup9GyKtS/kjSfv0XWJpNq4TGWYsdHzzDGU5I4JShc\n\tIuHxa2eOJc9zNBsLvY9OtwwOs/nZaVT2mKPliito="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SwHARVyJ\"; dkim-atps=neutral","Date":"Thu, 22 Sep 2022 22:22:16 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-5-laurent.pinchart@ideasonboard.com>\n\t<20220921174912.hf4v2vjdghmqliln@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220921174912.hf4v2vjdghmqliln@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce\n\tFrameContextQueue","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]