[{"id":24064,"web_url":"https://patchwork.libcamera.org/comment/24064/","msgid":"","date":"2022-07-22T20:51:48","subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 7/21/22 17:43, Kieran Bingham wrote:\n> From: Umang Jain \n>\n> Provide a direct FCQueue abstraction to facilitate creating a queue of\n> Frame Context structures.\n\n\nAh we are back again to have FCQueue separately (in class files) :-)\n\n>\n> The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue\n\n\nYou can just say that the constructors for IPAFrameContext are dropped.\n\n\n> added to the IPU3, but the Algorithms are not yet moved from the single\n\nnit: s/IPU3/IPAIPU3/\n\nFurthermore, a queue is already present in the form of:\n\n         std::array frameContexts;\n\nIt is just that the queue is transformed to FCQueue. Since this is a \nRFC, I won't dwell much on the commit message, it can get accurately \ndescribed in future interations.\n\n> FrameContext exposed by the IPA context to use the correct one from the\n> queue until the Algorithm interfaces are fully updated.\n>\n> Signed-off-by: Umang Jain \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/ipu3/ipa_context.cpp | 17 -------\n> src/ipa/ipu3/ipa_context.h | 15 ++----\n> src/ipa/ipu3/ipu3.cpp | 13 +++--\n> src/ipa/libipa/fc_queue.cpp | 96 ++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/fc_queue.h | 93 ++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 6 files changed, 204 insertions(+), 32 deletions(-)\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/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 13cdb835ef7f..dd139cb4c868 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {\n> * struct ipu3_uapi_gamma_corr_lut for further details.\n> */\n> \n> -/**\n> - * \\brief Default constructor for IPAFrameContext\n> - */\n> -IPAFrameContext::IPAFrameContext() = default;\n> -\n> -/**\n> - * \\brief Construct a IPAFrameContext instance\n> - */\n> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> -\t: frame(id), frameControls(reqControls)\n> -{\n> -\tsensor = {};\n> -}\n> -\n> /**\n> * \\var IPAFrameContext::frame\n> * \\brief The frame number\n> *\n> - * \\var IPAFrameContext::frameControls\n> - * \\brief Controls sent in by the application while queuing the request\n> - *\n> * \\var IPAFrameContext::sensor\n> * \\brief Effective sensor values that were applied for the frame\n> *\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 42e11141d3a1..193fc44a821a 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -8,8 +8,6 @@\n> \n> #pragma once\n> \n> -#include \n> -\n> #include \n> \n> #include \n> @@ -17,13 +15,12 @@\n> #include \n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> namespace ipa::ipu3 {\n> \n> -/* Maximum number of frame contexts to be held */\n> -static constexpr uint32_t kMaxFrameContexts = 16;\n> -\n> struct IPASessionConfiguration {\n> \tstruct {\n> \t\tipu3_uapi_grid_config bdsGrid;\n> @@ -77,23 +74,19 @@ struct IPAActiveState {\n> };\n> \n> struct IPAFrameContext {\n> -\tIPAFrameContext();\n> -\tIPAFrameContext(uint32_t id, const ControlList &reqControls);\n> +\tuint32_t frame;\n> \n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> \t} sensor;\n> -\n> -\tuint32_t frame;\n> -\tControlList frameControls;\n\n\nOkay, we dropped the ControlList here, because every algo will get the \nControlList in the new interface\n\n> };\n> \n> struct IPAContext {\n> \tIPASessionConfiguration configuration;\n> \tIPAActiveState activeState;\n> \n> -\tstd::array frameContexts;\n> +\tFCQueue frameContexts;\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 2f6bb672f7bb..55e82cd404f4 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -38,6 +38,8 @@\n> #include \"algorithms/tone_mapping.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n> \n> +#include \"ipa_context.h\"\n> +\n> /* Minimum grid width, expressed as a number of cells */\n> static constexpr uint32_t kMinGridWidth = 16;\n> /* Maximum grid width, expressed as a number of cells */\n> @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> \n> \t/* Clean IPAActiveState at each reconfiguration. */\n> \tcontext_.activeState = {};\n> -\tIPAFrameContext initFrameContext;\n> -\tcontext_.frameContexts.fill(initFrameContext);\n> +\tcontext_.frameContexts.clear();\ncontext_.frameContexts.clear();\n\nShould also be done in IPAIPU3::stop().\n\n> \n> \tif (!validateSensorControls()) {\n> \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> @@ -569,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \tconst ipu3_uapi_stats_3a *stats =\n> \t\treinterpret_cast(mem.data());\n> \n> -\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \n> \tif (frameContext.frame != frame)\n> \t\tLOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n> @@ -618,7 +619,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> -\tcontext_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> +\tIPAFrameContext &frameContext = context_.frameContexts.initialise(frame);\n> +\n> +\t/* \\todo Implement queueRequest to each algorithm. */\n> +\t(void)frameContext;\n> +\t(void)controls;\n> }\n> \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..ecb47f287350\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.cpp\n> @@ -0,0 +1,96 @@\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 Frame Context instances through the IPA\n\n\ns/queueing/queuing/\n\n> + * \\tparam FrameContext The IPA specific Frame Context type for this queue\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> + * FrameContexts are expected to be referenced by a monotonically increasing\n> + * sequence count referring to a Frame sequence.\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\n\ns/queueing/queuing/\n\n> + * a given frame, and processing of any ISP related controls and statistics for\n> + * the same corresponding image.\n> + *\n> + * IPA specific FrameContexts should inherit from the IPAFrameContext to support\n> + * the minimum required features for a FrameContext, including the frame number\n> + * and error flags that relate to the frame.\n> + *\n> + * FrameContexts are overwritten when they are recycled and re-initialised by\n> + * the first access made on them by either initialise(frame) or get(frame). If a\n> + * FrameContext is first accessed through get(frame) before calling initialise()\n> + * a PFCError is automatically raised on the FrameContext to be transferred to\n> + * the Request when it completes.\n\n\nI get the point here, but I feel the documentation on get()/PFCError \netc. is a bit jerky.\n\nOnce we have some context setup for FCQ, goals, error situations etc. \nmaybe it will fit into the flow.\n\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::clear()\n> + * \\brief Reinitialise all data on the queue\n> + *\n> + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::initialise(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> + * If the FrameContext was already initialized for this sequence, a warning will\n> + * be reported and the previously initialized FrameContext is returned.\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 intiialised for the \\a frame, it will be\ns/intiialised/initialised/\n> + * initialised and a PFCError will be flagged on the context to be transferred\n> + * to the Request when it completes.\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..82ce36811926\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.h\n> @@ -0,0 +1,93 @@\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> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(FCQueue)\n> +\n> +namespace ipa {\n> +\n> +/*\n> + * Maximum number of frame contexts to be held onto\n> + *\n> + * \\todo Should be larger than ISP delay + sensor delay\n> + */\n> +static constexpr uint32_t kMaxFrameContexts = 16;\n> +\n> +template\n\nNice, templates wohoo!\n\n> +class FCQueue : public std::array\n\n\nPrivate inheritance will be required. We don't want to expose std::array \nAPIs to get used accidentally.\n\n> +{\n> +private:\n> +\tvoid initialise(FrameContext &frameContext, const uint32_t frame)\n> +\t{\n> +\t\tframeContext = {};\n> +\t\tframeContext.frame = frame;\n> +\t}\n> +\n> +public:\n> +\tvoid clear()\n> +\t{\n> +\t\tthis->fill({});\n> +\t}\n> +\n> +\tFrameContext &initialise(const uint32_t frame)\n> +\t{\n> +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\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 error flags already raised.\n> +\t\t */\n> +\t\tif (frame == frameContext.frame && frame != 0) {\n> +\t\t\tLOG(FCQueue, Warning)\n> +\t\t\t\t<< \"Frame \" << frame << \" already initialised\";\n> +\t\t} else {\n> +\t\t\tinitialise(frameContext, frame);\n> +\t\t}\n> +\n> +\t\treturn frameContext;\n> +\t}\n> +\n> +\tFrameContext &get(uint32_t frame)\n> +\t{\n> +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\n> +\n> +\t\tif (frame != frameContext.frame) {\n> +\t\t\tLOG(FCQueue, Warning)\n> +\t\t\t\t<< \"Obtained an uninitialised FrameContext for \"\n> +\t\t\t\t<< frame;\n> +\n> +\t\t\tinitialise(frameContext, frame);\n> +\n> +\t\t\t/*\n> +\t\t\t * The frame context has been retrieved before it was\n> +\t\t\t * initialised through the initialise() call. This\n> +\t\t\t * indicates an algorithm attempted to access a Frame\n> +\t\t\t * context before it was queued to the IPA.\n> +\t\t\t *\n> +\t\t\t * Controls applied for this request may be left\n> +\t\t\t * unhandled.\n> +\t\t\t */\n> +\t\t\tframeContext.error |= Request::PFCError;\n> +\t\t}\n> +\n> +\t\treturn frameContext;\n> +\t}\n> +};\n\n\nFor now, it looks more or less as per stated in the design.\n\nThank you for the work!\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 F0877BD1F1\n\tfor ;\n\tFri, 22 Jul 2022 20:51:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C154601B8;\n\tFri, 22 Jul 2022 22:51:56 +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 81EAC601B8\n\tfor ;\n\tFri, 22 Jul 2022 22:51:54 +0200 (CEST)","from [IPV6:2401:4900:1f3f:a2d:c6dd:7a7c:3d3c:dbb9] (unknown\n\t[IPv6:2401:4900:1f3f:a2d:c6dd:7a7c:3d3c:dbb9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5415D6D5;\n\tFri, 22 Jul 2022 22:51:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658523116;\n\tbh=ZD61ljtta8y2aV2WpcIHg5D/RH4kzfs1LJhBSCkHFes=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=mC55OwZzJWNiCpT88xdqOVfC9H8Isyp7Tb2JjFxQg0SE77o00VNfja+KjDQ2ctIC8\n\tXeMjr594j0nsV5BOWZPKixUDgr2nb/HcwxgHYlr/LEB+1ys6BSca3zKvBUzZ4uP2fM\n\tSsFBeshxmHuzcllLsd42df3GVSjhpHHOzJistJKtSHgIodnApsxV5ZCOF25IPAP+rI\n\tO1whj/9+2k5ubXVnnu0FQzkhj1aiTeoiAVksu7mndXUO6zPn8WvCcx8bgC1NQk1eHK\n\tKI8N7DIkKqVX9foLrDZfBQ5nXXfG+YyK7weFBFkLu3M5ikpr4Jvm3uVRUJ3pJ7E1Cf\n\tE48D3WLsFP3xQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658523114;\n\tbh=ZD61ljtta8y2aV2WpcIHg5D/RH4kzfs1LJhBSCkHFes=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=I16H7jZL8IEKloPlbQf2d3PKe55Rpf4U2hEWeXXkLUNI1b8PEyi8IaZQ7MuS5gLFB\n\tsp1tR0wdb+U6N1Aa5Z17xqa+wyCnQ7KFYwm0MhEosuDagOJipfRPe1k7MyKWIyGQkA\n\t9tiYUJHiqXFt+fJqA/DaDS5SON+x+9SPfuRdltgg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"I16H7jZL\"; dkim-atps=neutral","Message-ID":"","Date":"Sat, 23 Jul 2022 02:21:48 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20220721121310.1286862-1-kieran.bingham@ideasonboard.com>\n\t<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>","In-Reply-To":"<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24091,"web_url":"https://patchwork.libcamera.org/comment/24091/","msgid":"<20220725082917.hnl72dn22ybbzhxm@uno.localdomain>","date":"2022-07-25T08:29:17","subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n one comment on the class implementation before doing a full review\n\nOn Thu, Jul 21, 2022 at 01:13:02PM +0100, Kieran Bingham via libcamera-devel wrote:\n> From: Umang Jain \n>\n> Provide a direct FCQueue abstraction to facilitate creating a queue of\n> Frame Context structures.\n>\n> The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue\n> added to the IPU3, but the Algorithms are not yet moved from the single\n> FrameContext exposed by the IPA context to use the correct one from the\n> queue until the Algorithm interfaces are fully updated.\n>\n> Signed-off-by: Umang Jain \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/ipu3/ipa_context.cpp | 17 -------\n> src/ipa/ipu3/ipa_context.h | 15 ++----\n> src/ipa/ipu3/ipu3.cpp | 13 +++--\n> src/ipa/libipa/fc_queue.cpp | 96 ++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/fc_queue.h | 93 ++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 6 files changed, 204 insertions(+), 32 deletions(-)\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/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 13cdb835ef7f..dd139cb4c868 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {\n> * struct ipu3_uapi_gamma_corr_lut for further details.\n> */\n>\n> -/**\n> - * \\brief Default constructor for IPAFrameContext\n> - */\n> -IPAFrameContext::IPAFrameContext() = default;\n> -\n> -/**\n> - * \\brief Construct a IPAFrameContext instance\n> - */\n> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> -\t: frame(id), frameControls(reqControls)\n> -{\n> -\tsensor = {};\n> -}\n> -\n> /**\n> * \\var IPAFrameContext::frame\n> * \\brief The frame number\n> *\n> - * \\var IPAFrameContext::frameControls\n> - * \\brief Controls sent in by the application while queuing the request\n> - *\n> * \\var IPAFrameContext::sensor\n> * \\brief Effective sensor values that were applied for the frame\n> *\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 42e11141d3a1..193fc44a821a 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -8,8 +8,6 @@\n>\n> #pragma once\n>\n> -#include \n> -\n> #include \n>\n> #include \n> @@ -17,13 +15,12 @@\n> #include \n> #include \n>\n> +#include \n> +\n> namespace libcamera {\n>\n> namespace ipa::ipu3 {\n>\n> -/* Maximum number of frame contexts to be held */\n> -static constexpr uint32_t kMaxFrameContexts = 16;\n> -\n> struct IPASessionConfiguration {\n> \tstruct {\n> \t\tipu3_uapi_grid_config bdsGrid;\n> @@ -77,23 +74,19 @@ struct IPAActiveState {\n> };\n>\n> struct IPAFrameContext {\n> -\tIPAFrameContext();\n> -\tIPAFrameContext(uint32_t id, const ControlList &reqControls);\n> +\tuint32_t frame;\n>\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> \t} sensor;\n> -\n> -\tuint32_t frame;\n> -\tControlList frameControls;\n> };\n>\n> struct IPAContext {\n> \tIPASessionConfiguration configuration;\n> \tIPAActiveState activeState;\n>\n> -\tstd::array frameContexts;\n> +\tFCQueue frameContexts;\n> };\n>\n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 2f6bb672f7bb..55e82cd404f4 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -38,6 +38,8 @@\n> #include \"algorithms/tone_mapping.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n>\n> +#include \"ipa_context.h\"\n> +\n> /* Minimum grid width, expressed as a number of cells */\n> static constexpr uint32_t kMinGridWidth = 16;\n> /* Maximum grid width, expressed as a number of cells */\n> @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>\n> \t/* Clean IPAActiveState at each reconfiguration. */\n> \tcontext_.activeState = {};\n> -\tIPAFrameContext initFrameContext;\n> -\tcontext_.frameContexts.fill(initFrameContext);\n> +\tcontext_.frameContexts.clear();\n>\n> \tif (!validateSensorControls()) {\n> \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> @@ -569,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \tconst ipu3_uapi_stats_3a *stats =\n> \t\treinterpret_cast(mem.data());\n>\n> -\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>\n> \tif (frameContext.frame != frame)\n> \t\tLOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n> @@ -618,7 +619,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> -\tcontext_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> +\tIPAFrameContext &frameContext = context_.frameContexts.initialise(frame);\n> +\n> +\t/* \\todo Implement queueRequest to each algorithm. */\n> +\t(void)frameContext;\n> +\t(void)controls;\n> }\n>\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..ecb47f287350\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.cpp\n> @@ -0,0 +1,96 @@\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 Frame Context instances through the IPA\n> + * \\tparam FrameContext The IPA specific Frame Context type for this queue\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> + * FrameContexts are expected to be referenced by a monotonically increasing\n> + * sequence count referring to a Frame sequence.\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 FrameContexts should inherit from the IPAFrameContext to support\n> + * the minimum required features for a FrameContext, including the frame number\n> + * and error flags that relate to the frame.\n> + *\n> + * FrameContexts are overwritten when they are recycled and re-initialised by\n> + * the first access made on them by either initialise(frame) or get(frame). If a\n> + * FrameContext is first accessed through get(frame) before calling initialise()\n> + * a PFCError is automatically raised on the FrameContext to be transferred to\n> + * the Request when it completes.\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::clear()\n> + * \\brief Reinitialise all data on the queue\n> + *\n> + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::initialise(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> + * If the FrameContext was already initialized for this sequence, a warning will\n> + * be reported and the previously initialized FrameContext is returned.\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 intiialised for the \\a frame, it will be\n> + * initialised and a PFCError will be flagged on the context to be transferred\n> + * to the Request when it completes.\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..82ce36811926\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.h\n> @@ -0,0 +1,93 @@\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> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(FCQueue)\n> +\n> +namespace ipa {\n> +\n> +/*\n> + * Maximum number of frame contexts to be held onto\n> + *\n> + * \\todo Should be larger than ISP delay + sensor delay\n> + */\n> +static constexpr uint32_t kMaxFrameContexts = 16;\n> +\n> +template\n\nI presume the idea of using a template is to allow IPAs to define\ntheir own frame context and use this class as a container.\n\nHowever the class assumes that the FrameContext template argument has\na well defined interface, specifically it requires a 'uint32_t frame'\nfield.\n\nThere is no guarantee at all that the class used to instantiate the\ntemplate has such field with the here proposed design, am I wrong ?\n\nShouldn't we create a base class in libipa such as:\n\nclass IPAFrameContext\n{\npublic:\n uint32_t frame = 0;\n};\n\nAnd let IPA implementation subclass it ?\n\nCan we force it to be a pure virtual class by making the constructor\nvirtual ? There might be some more clever ways to make this a\nnon-instantiable interface...\n\nHow to force the FCQueue to nicely accept a class derived from\nIPAFrameContext might also require some thinking, but I presume we\nmight want enforce that ?\n\n> +class FCQueue : public std::array\n> +{\n> +private:\n> +\tvoid initialise(FrameContext &frameContext, const uint32_t frame)\n> +\t{\n> +\t\tframeContext = {};\n> +\t\tframeContext.frame = frame;\n> +\t}\n> +\n> +public:\n> +\tvoid clear()\n> +\t{\n> +\t\tthis->fill({});\n> +\t}\n> +\n> +\tFrameContext &initialise(const uint32_t frame)\n> +\t{\n> +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\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 error flags already raised.\n> +\t\t */\n> +\t\tif (frame == frameContext.frame && frame != 0) {\n> +\t\t\tLOG(FCQueue, Warning)\n> +\t\t\t\t<< \"Frame \" << frame << \" already initialised\";\n> +\t\t} else {\n> +\t\t\tinitialise(frameContext, frame);\n> +\t\t}\n> +\n> +\t\treturn frameContext;\n> +\t}\n> +\n> +\tFrameContext &get(uint32_t frame)\n> +\t{\n> +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\n> +\n> +\t\tif (frame != frameContext.frame) {\n> +\t\t\tLOG(FCQueue, Warning)\n> +\t\t\t\t<< \"Obtained an uninitialised FrameContext for \"\n> +\t\t\t\t<< frame;\n> +\n> +\t\t\tinitialise(frameContext, frame);\n> +\n> +\t\t\t/*\n> +\t\t\t * The frame context has been retrieved before it was\n> +\t\t\t * initialised through the initialise() call. This\n> +\t\t\t * indicates an algorithm attempted to access a Frame\n> +\t\t\t * context before it was queued to the IPA.\n> +\t\t\t *\n> +\t\t\t * Controls applied for this request may be left\n> +\t\t\t * unhandled.\n> +\t\t\t */\n> +\t\t\tframeContext.error |= Request::PFCError;\n> +\t\t}\n> +\n> +\t\treturn frameContext;\n> +\t}\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> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E7BC6C3275\n\tfor ;\n\tMon, 25 Jul 2022 08:29:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7178B6330E;\n\tMon, 25 Jul 2022 10:29:21 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2182663309\n\tfor ;\n\tMon, 25 Jul 2022 10:29:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 8DD7120009;\n\tMon, 25 Jul 2022 08:29:19 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658737761;\n\tbh=19Ts7pEx/4b7sylQv4X7pyv1cvlp9fudtgQeLyXxDqI=;\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=kgAaClLlEh97K85GMoJbADPlopfebfFtG30cg14Fd0bSKhnKc1E2QOVgYKJkJrP2p\n\te10e/CfCfEpp864CQf7b1dxS0QVAN71rXUXdmcH1bZeSoPWnAywzF8Z7CnS5SjSO2t\n\tlVSrCyUoi/QrT913lye/J21vUM+FwlN3nn21jsFbT1Yhg0kLqoDQRetkAQQjmwkeWh\n\tIbS8C/UlWoAm0vYlYjpeIxb+DEufO+L4OeAb8wiAnuyiHLQUAf5VABbIcqTMSZsUEl\n\tltjx6XA9qf4P/FB/Z+86ex0W/+sKUQdtMdEbHhYI7Of423KF9+k2/QOk1LQs616f8N\n\tKptZ5AEaAneVQ==","Date":"Mon, 25 Jul 2022 10:29:17 +0200","To":"Kieran Bingham ","Message-ID":"<20220725082917.hnl72dn22ybbzhxm@uno.localdomain>","References":"<20220721121310.1286862-1-kieran.bingham@ideasonboard.com>\n\t<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24092,"web_url":"https://patchwork.libcamera.org/comment/24092/","msgid":"<20220725100052.qfud2omqmjeskylj@uno.localdomain>","date":"2022-07-25T10:00:52","subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"One additional note\n\nOn Mon, Jul 25, 2022 at 10:29:17AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Hi Kieran,\n> one comment on the class implementation before doing a full review\n>\n> On Thu, Jul 21, 2022 at 01:13:02PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > From: Umang Jain \n> >\n> > Provide a direct FCQueue abstraction to facilitate creating a queue of\n> > Frame Context structures.\n> >\n> > The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue\n> > added to the IPU3, but the Algorithms are not yet moved from the single\n> > FrameContext exposed by the IPA context to use the correct one from the\n> > queue until the Algorithm interfaces are fully updated.\n> >\n> > Signed-off-by: Umang Jain \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/ipu3/ipa_context.cpp | 17 -------\n> > src/ipa/ipu3/ipa_context.h | 15 ++----\n> > src/ipa/ipu3/ipu3.cpp | 13 +++--\n> > src/ipa/libipa/fc_queue.cpp | 96 ++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/fc_queue.h | 93 ++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 6 files changed, 204 insertions(+), 32 deletions(-)\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/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > index 13cdb835ef7f..dd139cb4c868 100644\n> > --- a/src/ipa/ipu3/ipa_context.cpp\n> > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {\n> > * struct ipu3_uapi_gamma_corr_lut for further details.\n> > */\n> >\n> > -/**\n> > - * \\brief Default constructor for IPAFrameContext\n> > - */\n> > -IPAFrameContext::IPAFrameContext() = default;\n> > -\n> > -/**\n> > - * \\brief Construct a IPAFrameContext instance\n> > - */\n> > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> > -\t: frame(id), frameControls(reqControls)\n> > -{\n> > -\tsensor = {};\n> > -}\n> > -\n> > /**\n> > * \\var IPAFrameContext::frame\n> > * \\brief The frame number\n> > *\n> > - * \\var IPAFrameContext::frameControls\n> > - * \\brief Controls sent in by the application while queuing the request\n> > - *\n> > * \\var IPAFrameContext::sensor\n> > * \\brief Effective sensor values that were applied for the frame\n> > *\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index 42e11141d3a1..193fc44a821a 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -8,8 +8,6 @@\n> >\n> > #pragma once\n> >\n> > -#include \n> > -\n> > #include \n> >\n> > #include \n> > @@ -17,13 +15,12 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > namespace libcamera {\n> >\n> > namespace ipa::ipu3 {\n> >\n> > -/* Maximum number of frame contexts to be held */\n> > -static constexpr uint32_t kMaxFrameContexts = 16;\n> > -\n> > struct IPASessionConfiguration {\n> > \tstruct {\n> > \t\tipu3_uapi_grid_config bdsGrid;\n> > @@ -77,23 +74,19 @@ struct IPAActiveState {\n> > };\n> >\n> > struct IPAFrameContext {\n> > -\tIPAFrameContext();\n> > -\tIPAFrameContext(uint32_t id, const ControlList &reqControls);\n> > +\tuint32_t frame;\n> >\n> > \tstruct {\n> > \t\tuint32_t exposure;\n> > \t\tdouble gain;\n> > \t} sensor;\n> > -\n> > -\tuint32_t frame;\n> > -\tControlList frameControls;\n> > };\n> >\n> > struct IPAContext {\n> > \tIPASessionConfiguration configuration;\n> > \tIPAActiveState activeState;\n> >\n> > -\tstd::array frameContexts;\n> > +\tFCQueue frameContexts;\n> > };\n> >\n> > } /* namespace ipa::ipu3 */\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 2f6bb672f7bb..55e82cd404f4 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -38,6 +38,8 @@\n> > #include \"algorithms/tone_mapping.h\"\n> > #include \"libipa/camera_sensor_helper.h\"\n> >\n> > +#include \"ipa_context.h\"\n> > +\n> > /* Minimum grid width, expressed as a number of cells */\n> > static constexpr uint32_t kMinGridWidth = 16;\n> > /* Maximum grid width, expressed as a number of cells */\n> > @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> >\n> > \t/* Clean IPAActiveState at each reconfiguration. */\n> > \tcontext_.activeState = {};\n> > -\tIPAFrameContext initFrameContext;\n> > -\tcontext_.frameContexts.fill(initFrameContext);\n> > +\tcontext_.frameContexts.clear();\n> >\n> > \tif (!validateSensorControls()) {\n> > \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> > @@ -569,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > \tconst ipu3_uapi_stats_3a *stats =\n> > \t\treinterpret_cast(mem.data());\n> >\n> > -\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> > +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> >\n> > \tif (frameContext.frame != frame)\n> > \t\tLOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n> > @@ -618,7 +619,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > {\n> > \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> > -\tcontext_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> > +\tIPAFrameContext &frameContext = context_.frameContexts.initialise(frame);\n> > +\n> > +\t/* \\todo Implement queueRequest to each algorithm. */\n> > +\t(void)frameContext;\n> > +\t(void)controls;\n> > }\n> >\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..ecb47f287350\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.cpp\n> > @@ -0,0 +1,96 @@\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 Frame Context instances through the IPA\n> > + * \\tparam FrameContext The IPA specific Frame Context type for this queue\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> > + * FrameContexts are expected to be referenced by a monotonically increasing\n> > + * sequence count referring to a Frame sequence.\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 FrameContexts should inherit from the IPAFrameContext to support\n> > + * the minimum required features for a FrameContext, including the frame number\n> > + * and error flags that relate to the frame.\n> > + *\n> > + * FrameContexts are overwritten when they are recycled and re-initialised by\n> > + * the first access made on them by either initialise(frame) or get(frame). If a\n> > + * FrameContext is first accessed through get(frame) before calling initialise()\n> > + * a PFCError is automatically raised on the FrameContext to be transferred to\n> > + * the Request when it completes.\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::clear()\n> > + * \\brief Reinitialise all data on the queue\n> > + *\n> > + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::initialise(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> > + * If the FrameContext was already initialized for this sequence, a warning will\n> > + * be reported and the previously initialized FrameContext is returned.\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 intiialised for the \\a frame, it will be\n> > + * initialised and a PFCError will be flagged on the context to be transferred\n> > + * to the Request when it completes.\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..82ce36811926\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.h\n> > @@ -0,0 +1,93 @@\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> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(FCQueue)\n> > +\n> > +namespace ipa {\n> > +\n> > +/*\n> > + * Maximum number of frame contexts to be held onto\n> > + *\n> > + * \\todo Should be larger than ISP delay + sensor delay\n> > + */\n> > +static constexpr uint32_t kMaxFrameContexts = 16;\n> > +\n> > +template\n>\n> I presume the idea of using a template is to allow IPAs to define\n> their own frame context and use this class as a container.\n>\n> However the class assumes that the FrameContext template argument has\n> a well defined interface, specifically it requires a 'uint32_t frame'\n> field.\n>\n> There is no guarantee at all that the class used to instantiate the\n> template has such field with the here proposed design, am I wrong ?\n>\n> Shouldn't we create a base class in libipa such as:\n>\n> class IPAFrameContext\n> {\n> public:\n> uint32_t frame = 0;\n> };\n>\n> And let IPA implementation subclass it ?\n\nThis happens later :)\n>\n> Can we force it to be a pure virtual class by making the constructor\n> virtual ? There might be some more clever ways to make this a\n> non-instantiable interface...\n>\n> How to force the FCQueue to nicely accept a class derived from\n> IPAFrameContext might also require some thinking, but I presume we\n> might want enforce that ?\n>\n\nPlease also note that is a non-problem as to my surprise the templates\nargument substitution compile-time checks if the provided type matches\nthe requested interface. The C++ templating system does duck typing\nand if the provided FrameContext template argument does not inherit\nfrom IPAFrameContext it breaks with:\n\n../src/ipa/ipu3/ipu3.cpp:577:26: error: ‘struct libcamera::ipa::ipu3::IPU3FrameContext’ has no member named ‘frame’\n 577 | if (frameContext.frame != frame)\n | ^~~~~\n\nHowever we can exploit std::type_traits to more explicitly check for the\nFrameContext template argument to be a derived class of\nIPAFrameContext with:\n\ntemplate\nclass FCQueue : public std::array\n{\nprivate:\n\tusing valueType =\n\t\tstd::enable_if_t,\n\t\t\t\t FrameContext>;\n\nHowever the error message, in case FrameContext does not inherit from\nIPAFrameContext, is not much more helpful I'm afraid\n\n/usr/include/c++/12.1.0/type_traits: In substitution of ‘template using enable_if_t = typename std::enable_if::type [with bool _Cond = false; _Tp = libcamera::ipa::ipu3::IPU3FrameContext]’:\n../src/ipa/libipa/fc_queue.h:41:8: required from ‘class libcamera::ipa::FCQueue’\n../src/ipa/ipu3/ipa_context.h:88:28: required from here\n/usr/include/c++/12.1.0/type_traits:2614:11: error: no type named ‘type’ in ‘struct std::enable_if’\n 2614 | using enable_if_t = typename enable_if<_Cond, _Tp>::type;\n\nBut could help pointing to the source file where a comment might\nexplain the required inheritance.\n\nAnyway, as it breaks at compile time, this is a minor nit indeed\n\n\n> > +class FCQueue : public std::array\n> > +{\n> > +private:\n> > +\tvoid initialise(FrameContext &frameContext, const uint32_t frame)\n> > +\t{\n> > +\t\tframeContext = {};\n> > +\t\tframeContext.frame = frame;\n> > +\t}\n> > +\n> > +public:\n> > +\tvoid clear()\n> > +\t{\n> > +\t\tthis->fill({});\n> > +\t}\n> > +\n> > +\tFrameContext &initialise(const uint32_t frame)\n> > +\t{\n> > +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\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 error flags already raised.\n> > +\t\t */\n> > +\t\tif (frame == frameContext.frame && frame != 0) {\n> > +\t\t\tLOG(FCQueue, Warning)\n> > +\t\t\t\t<< \"Frame \" << frame << \" already initialised\";\n> > +\t\t} else {\n> > +\t\t\tinitialise(frameContext, frame);\n> > +\t\t}\n> > +\n> > +\t\treturn frameContext;\n> > +\t}\n> > +\n> > +\tFrameContext &get(uint32_t frame)\n> > +\t{\n> > +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\n> > +\n> > +\t\tif (frame != frameContext.frame) {\n> > +\t\t\tLOG(FCQueue, Warning)\n> > +\t\t\t\t<< \"Obtained an uninitialised FrameContext for \"\n> > +\t\t\t\t<< frame;\n> > +\n> > +\t\t\tinitialise(frameContext, frame);\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * The frame context has been retrieved before it was\n> > +\t\t\t * initialised through the initialise() call. This\n> > +\t\t\t * indicates an algorithm attempted to access a Frame\n> > +\t\t\t * context before it was queued to the IPA.\n> > +\t\t\t *\n> > +\t\t\t * Controls applied for this request may be left\n> > +\t\t\t * unhandled.\n> > +\t\t\t */\n> > +\t\t\tframeContext.error |= Request::PFCError;\n> > +\t\t}\n> > +\n> > +\t\treturn frameContext;\n> > +\t}\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> > 2.34.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 604DABE173\n\tfor ;\n\tMon, 25 Jul 2022 10:00:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0927B63312;\n\tMon, 25 Jul 2022 12:00:57 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::228])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B28F56330A\n\tfor ;\n\tMon, 25 Jul 2022 12:00:54 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4011C1BF210;\n\tMon, 25 Jul 2022 10:00:53 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658743257;\n\tbh=sxuC9ZSmoTjuUxEawl7/j2i8tu/XmtwRtgEvsivL2Bs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=u9Jyx3SbDBShn2oTIbqbyDxhqQ1auvdMYfDBdzM8h3kC5sEqifddQ8qKTwQNRCEec\n\tpj97TmohoKn9NeKlBIAYFMFaN3LVTc1K+qxv3T/tfa1zUlzZY1VEH6IU77Z22jAOGI\n\t3e7gvxxo68bYLa015Bz/+Il7VAFmMXCuKFlnYhtgJh3wp+kqh4wyevJ6/mgrIPxBSC\n\tjLLRT/686ieDkxwhdFiaf4kBznovHbaDf6DrR8NS/PRBkttXqplgswGZRx6CZE4iis\n\toQpGBrDXSAJSyf3I9fWLr6VJvCF5GTpGgnvFhzrih9MjvYyhz8z+khYnUBWhwidgEf\n\tDMfCyNy/pZXIQ==","Date":"Mon, 25 Jul 2022 12:00:52 +0200","To":"Kieran Bingham ,\n\tlibcamera devel ","Message-ID":"<20220725100052.qfud2omqmjeskylj@uno.localdomain>","References":"<20220721121310.1286862-1-kieran.bingham@ideasonboard.com>\n\t<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>\n\t<20220725082917.hnl72dn22ybbzhxm@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20220725082917.hnl72dn22ybbzhxm@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24106,"web_url":"https://patchwork.libcamera.org/comment/24106/","msgid":"<20220725152051.asydfrne3gjiqpin@uno.localdomain>","date":"2022-07-25T15:20:51","subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Thu, Jul 21, 2022 at 01:13:02PM +0100, Kieran Bingham via libcamera-devel wrote:\n> From: Umang Jain \n>\n> Provide a direct FCQueue abstraction to facilitate creating a queue of\n> Frame Context structures.\n>\n> The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue\n> added to the IPU3, but the Algorithms are not yet moved from the single\n> FrameContext exposed by the IPA context to use the correct one from the\n> queue until the Algorithm interfaces are fully updated.\n>\n> Signed-off-by: Umang Jain \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/ipu3/ipa_context.cpp | 17 -------\n> src/ipa/ipu3/ipa_context.h | 15 ++----\n> src/ipa/ipu3/ipu3.cpp | 13 +++--\n> src/ipa/libipa/fc_queue.cpp | 96 ++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/fc_queue.h | 93 ++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 6 files changed, 204 insertions(+), 32 deletions(-)\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/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 13cdb835ef7f..dd139cb4c868 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {\n> * struct ipu3_uapi_gamma_corr_lut for further details.\n> */\n>\n> -/**\n> - * \\brief Default constructor for IPAFrameContext\n> - */\n> -IPAFrameContext::IPAFrameContext() = default;\n> -\n> -/**\n> - * \\brief Construct a IPAFrameContext instance\n> - */\n> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> -\t: frame(id), frameControls(reqControls)\n> -{\n> -\tsensor = {};\n> -}\n> -\n> /**\n> * \\var IPAFrameContext::frame\n> * \\brief The frame number\n> *\n> - * \\var IPAFrameContext::frameControls\n> - * \\brief Controls sent in by the application while queuing the request\n> - *\n> * \\var IPAFrameContext::sensor\n> * \\brief Effective sensor values that were applied for the frame\n> *\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 42e11141d3a1..193fc44a821a 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -8,8 +8,6 @@\n>\n> #pragma once\n>\n> -#include \n> -\n> #include \n>\n> #include \n> @@ -17,13 +15,12 @@\n> #include \n> #include \n>\n> +#include \n> +\n> namespace libcamera {\n>\n> namespace ipa::ipu3 {\n>\n> -/* Maximum number of frame contexts to be held */\n> -static constexpr uint32_t kMaxFrameContexts = 16;\n\nThe size of the FCQueue should probably come from the IPA\nimplementation, as different platforms will have a different queue\ndepth to handle.\n\nIt can be made a second template argument.\n\n> -\n> struct IPASessionConfiguration {\n> \tstruct {\n> \t\tipu3_uapi_grid_config bdsGrid;\n> @@ -77,23 +74,19 @@ struct IPAActiveState {\n> };\n>\n> struct IPAFrameContext {\n> -\tIPAFrameContext();\n> -\tIPAFrameContext(uint32_t id, const ControlList &reqControls);\n> +\tuint32_t frame;\n>\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> \t} sensor;\n> -\n> -\tuint32_t frame;\n> -\tControlList frameControls;\n> };\n>\n> struct IPAContext {\n> \tIPASessionConfiguration configuration;\n> \tIPAActiveState activeState;\n>\n> -\tstd::array frameContexts;\n> +\tFCQueue frameContexts;\n> };\n>\n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 2f6bb672f7bb..55e82cd404f4 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -38,6 +38,8 @@\n> #include \"algorithms/tone_mapping.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n>\n> +#include \"ipa_context.h\"\n> +\n> /* Minimum grid width, expressed as a number of cells */\n> static constexpr uint32_t kMinGridWidth = 16;\n> /* Maximum grid width, expressed as a number of cells */\n> @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>\n> \t/* Clean IPAActiveState at each reconfiguration. */\n> \tcontext_.activeState = {};\n> -\tIPAFrameContext initFrameContext;\n> -\tcontext_.frameContexts.fill(initFrameContext);\n> +\tcontext_.frameContexts.clear();\n>\n> \tif (!validateSensorControls()) {\n> \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> @@ -569,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \tconst ipu3_uapi_stats_3a *stats =\n> \t\treinterpret_cast(mem.data());\n>\n> -\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>\n> \tif (frameContext.frame != frame)\n> \t\tLOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n> @@ -618,7 +619,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> -\tcontext_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> +\tIPAFrameContext &frameContext = context_.frameContexts.initialise(frame);\n> +\n> +\t/* \\todo Implement queueRequest to each algorithm. */\n> +\t(void)frameContext;\n> +\t(void)controls;\n\nI'm sure this will be expanded later but it could be simplified as\n\n void IPAIPU3::queueRequest(const uint32_t frame, [[maybe_unused]] const ControlList &controls)\n {\n \t/* \\todo Start processing for 'frame' based on 'controls'. */\n\tcontext_.frameContexts.initialise(frame);\n }\n\n> }\n>\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..ecb47f287350\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.cpp\n> @@ -0,0 +1,96 @@\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\nIs this needed here or can be moved to the .h ?\n\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 Frame Context instances through the IPA\n\nframe context or FrameContext, here and in the other parts of the\ndocumentation ?\n\n> + * \\tparam FrameContext The IPA specific Frame Context type for this queue\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> + * FrameContexts are expected to be referenced by a monotonically increasing\n> + * sequence count referring to a Frame sequence.\n \"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 FrameContexts should inherit from the IPAFrameContext to support\n\nIPA specific frame context implementations shall inherit from the\nIPAFrameContext base class to support..\n\n> + * the minimum required features for a FrameContext, including the frame number\n> + * and error flags that relate to the frame.\n> + *\n> + * FrameContexts are overwritten when they are recycled and re-initialised by\n> + * the first access made on them by either initialise(frame) or get(frame). If a\n\nThat's what a circular array does.\n\nI would rather express the fact that the number of available contexts\nis required to be larger than the platform pipeline depth (with a\n\\todo to reference the property once we have it defined). After\npipelineDepth frames have been processed the FrameContext is recycled\nand forever lost.\n\n> + * FrameContext is first accessed through get(frame) before calling initialise()\n> + * a PFCError is automatically raised on the FrameContext to be transferred to\n> + * the Request when it completes.\n\nThis would happen because we have a request underrun, right ?\n\nI would state that. Like:\n\nIn the case an application does not queue requests to the Camera fast\nenough, frames can be produced and processed by the IPA without a\ncorresponding Request being queued. In this case the IPA algorithm\nwill try to access the FrameContext with a call to get() before it\nhad been initialized at queueRequest() time. In this case there\nis now way the controls associated with the late Request could be\napplied in time, as the frame as already been processed by the IPA,\nthe PFCError flag is automatically raised on the FrameContext.\n\nHow does it sound ?\n\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::clear()\n> + * \\brief Reinitialise all data on the queue\n\nOr \"Clear the queue content\" ?\n\n> + *\n> + * Reset the queue and ensure all FrameContext slots are re-initialised.\n\nI would add that the Queue has to be re-initialized at every streaming\nsession start, likely in IPA::configure()\n\n> + */\n> +\n> +/**\n> + * \\fn FCQueue::initialise(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> + * If the FrameContext was already initialized for this sequence, a warning will\n> + * be reported and the previously initialized FrameContext is returned.\n> + *\n\nShould we say it is expected to be called at queueRequest() time ?\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 intiialised for the \\a frame, it will be\n\ns/intiialised/initialised/\n\n> + * initialised and a PFCError will be flagged on the context to be transferred\n> + * to the Request when it completes.\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..82ce36811926\n> --- /dev/null\n> +++ b/src/ipa/libipa/fc_queue.h\n> @@ -0,0 +1,93 @@\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\nUnecessary empty line\n\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n\nNot needed it seems\n\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(FCQueue)\n> +\n> +namespace ipa {\n> +\n> +/*\n> + * Maximum number of frame contexts to be held onto\n> + *\n> + * \\todo Should be larger than ISP delay + sensor delay\n> + */\n> +static constexpr uint32_t kMaxFrameContexts = 16;\n> +\n> +template\n> +class FCQueue : public std::array\n> +{\n> +private:\n> +\tvoid initialise(FrameContext &frameContext, const uint32_t frame)\n> +\t{\n> +\t\tframeContext = {};\n> +\t\tframeContext.frame = frame;\n> +\t}\n> +\n> +public:\n> +\tvoid clear()\n> +\t{\n> +\t\tthis->fill({});\n> +\t}\n> +\n> +\tFrameContext &initialise(const uint32_t frame)\n> +\t{\n> +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\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 error flags already raised.\n> +\t\t */\n> +\t\tif (frame == frameContext.frame && frame != 0) {\n\nThis should be <= as it needs to catch cases where the frame context\nhas been possibile overwritten by algorithms calling get() for a\nnumber of times larger than the pipeline depth.\n\n> +\t\t\tLOG(FCQueue, Warning)\n> +\t\t\t\t<< \"Frame \" << frame << \" already initialised\";\n> +\t\t} else {\n> +\t\t\tinitialise(frameContext, frame);\n> +\t\t}\n\nNo brances needed it seems\n\n> +\n> +\t\treturn frameContext;\n> +\t}\n> +\n> +\tFrameContext &get(uint32_t frame)\n> +\t{\n> +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\n> +\n> +\t\tif (frame != frameContext.frame) {\n\nOr if (frame == frameContext.frame)\n return frameContext;\n\nTo save one indentation level. Up to you.\n\n> +\t\t\tLOG(FCQueue, Warning)\n> +\t\t\t\t<< \"Obtained an uninitialised FrameContext for \"\n> +\t\t\t\t<< frame;\n> +\n> +\t\t\tinitialise(frameContext, frame);\n> +\n> +\t\t\t/*\n> +\t\t\t * The frame context has been retrieved before it was\n> +\t\t\t * initialised through the initialise() call. This\n> +\t\t\t * indicates an algorithm attempted to access a Frame\n> +\t\t\t * context before it was queued to the IPA.\n> +\t\t\t *\n> +\t\t\t * Controls applied for this request may be left\n> +\t\t\t * unhandled.\n> +\t\t\t */\n> +\t\t\tframeContext.error |= Request::PFCError;\n> +\t\t}\n> +\n> +\t\treturn frameContext;\n> +\t}\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> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7A871BE173\n\tfor ;\n\tMon, 25 Jul 2022 15:20:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D714463312;\n\tMon, 25 Jul 2022 17:20:54 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B53236330A\n\tfor ;\n\tMon, 25 Jul 2022 17:20:53 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 322D62000F;\n\tMon, 25 Jul 2022 15:20:52 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658762454;\n\tbh=Op72TFE9iYhLhmvxtB+rn1t230PRLQrmZ1GhQbB9JZ8=;\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=2T0s+UTYwcWDu1gr97ao0JlDNW1/SJSmwCDzekthuwWt2Ci/Ma5YVx94N56wed4vg\n\t7jqWG5CVzZLE7Tw6CWII01d88s0FdCs/TTQHSYKjLrZbj/d66PdV8LonWvdDlRoO/0\n\toSxmWnauqEVtbBMnui8gfWbFl7P7dlRPVqO1rpdzXK+qW/t2sLwPJerTJT2AB3Fe0s\n\tII601vkPlaYO4skYwSRU8wa8MFoINayrRHbGHMcPk1J6aH4vrFD2MUZ3cETmFAu4Ar\n\ts8aKxLV2r77KuAR1ZoXpTao3Iv/ILdRaWMYFSMBxspUt+Yq0Au7qbUBXZtF3YqHqQ9\n\t5JUPGNb5ZyR0A==","Date":"Mon, 25 Jul 2022 17:20:51 +0200","To":"Kieran Bingham ","Message-ID":"<20220725152051.asydfrne3gjiqpin@uno.localdomain>","References":"<20220721121310.1286862-1-kieran.bingham@ideasonboard.com>\n\t<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24141,"web_url":"https://patchwork.libcamera.org/comment/24141/","msgid":"<165882960310.3981176.9524645649595131859@Monstersaurus>","date":"2022-07-26T10:00:03","subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2022-07-25 16:20:51)\n> Hi Kieran\n> \n> On Thu, Jul 21, 2022 at 01:13:02PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > From: Umang Jain \n> >\n> > Provide a direct FCQueue abstraction to facilitate creating a queue of\n> > Frame Context structures.\n> >\n> > The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue\n> > added to the IPU3, but the Algorithms are not yet moved from the single\n> > FrameContext exposed by the IPA context to use the correct one from the\n> > queue until the Algorithm interfaces are fully updated.\n> >\n> > Signed-off-by: Umang Jain \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/ipu3/ipa_context.cpp | 17 -------\n> > src/ipa/ipu3/ipa_context.h | 15 ++----\n> > src/ipa/ipu3/ipu3.cpp | 13 +++--\n> > src/ipa/libipa/fc_queue.cpp | 96 ++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/fc_queue.h | 93 ++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 6 files changed, 204 insertions(+), 32 deletions(-)\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/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > index 13cdb835ef7f..dd139cb4c868 100644\n> > --- a/src/ipa/ipu3/ipa_context.cpp\n> > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {\n> > * struct ipu3_uapi_gamma_corr_lut for further details.\n> > */\n> >\n> > -/**\n> > - * \\brief Default constructor for IPAFrameContext\n> > - */\n> > -IPAFrameContext::IPAFrameContext() = default;\n> > -\n> > -/**\n> > - * \\brief Construct a IPAFrameContext instance\n> > - */\n> > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> > - : frame(id), frameControls(reqControls)\n> > -{\n> > - sensor = {};\n> > -}\n> > -\n> > /**\n> > * \\var IPAFrameContext::frame\n> > * \\brief The frame number\n> > *\n> > - * \\var IPAFrameContext::frameControls\n> > - * \\brief Controls sent in by the application while queuing the request\n> > - *\n> > * \\var IPAFrameContext::sensor\n> > * \\brief Effective sensor values that were applied for the frame\n> > *\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index 42e11141d3a1..193fc44a821a 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -8,8 +8,6 @@\n> >\n> > #pragma once\n> >\n> > -#include \n> > -\n> > #include \n> >\n> > #include \n> > @@ -17,13 +15,12 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > namespace libcamera {\n> >\n> > namespace ipa::ipu3 {\n> >\n> > -/* Maximum number of frame contexts to be held */\n> > -static constexpr uint32_t kMaxFrameContexts = 16;\n> \n> The size of the FCQueue should probably come from the IPA\n> implementation, as different platforms will have a different queue\n> depth to handle.\n> \n> It can be made a second template argument.\n\nYes, this is absoultely a platform specific value.\n\nI simply haven't made it adjustable yet.\n\nI believe it could also be handled on top, as '16' is bigger than both\nexpected platforms currently ;-)\n\nI'd rather handle that as a separate patch as well - as whatever size it\nis contstructed as needs to be conveyed to the pipeline handler too.\n\nEither the PH should state what size the IPA should make, or otherwise\nthey should both be hardcoded to the same size.\n\nBut that's enough discussion that I think it deserves it's own\npatch/implementation.\n\n\n> > -\n> > struct IPASessionConfiguration {\n> > struct {\n> > ipu3_uapi_grid_config bdsGrid;\n> > @@ -77,23 +74,19 @@ struct IPAActiveState {\n> > };\n> >\n> > struct IPAFrameContext {\n> > - IPAFrameContext();\n> > - IPAFrameContext(uint32_t id, const ControlList &reqControls);\n> > + uint32_t frame;\n> >\n> > struct {\n> > uint32_t exposure;\n> > double gain;\n> > } sensor;\n> > -\n> > - uint32_t frame;\n> > - ControlList frameControls;\n> > };\n> >\n> > struct IPAContext {\n> > IPASessionConfiguration configuration;\n> > IPAActiveState activeState;\n> >\n> > - std::array frameContexts;\n> > + FCQueue frameContexts;\n> > };\n> >\n> > } /* namespace ipa::ipu3 */\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 2f6bb672f7bb..55e82cd404f4 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -38,6 +38,8 @@\n> > #include \"algorithms/tone_mapping.h\"\n> > #include \"libipa/camera_sensor_helper.h\"\n> >\n> > +#include \"ipa_context.h\"\n> > +\n> > /* Minimum grid width, expressed as a number of cells */\n> > static constexpr uint32_t kMinGridWidth = 16;\n> > /* Maximum grid width, expressed as a number of cells */\n> > @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> >\n> > /* Clean IPAActiveState at each reconfiguration. */\n> > context_.activeState = {};\n> > - IPAFrameContext initFrameContext;\n> > - context_.frameContexts.fill(initFrameContext);\n> > + context_.frameContexts.clear();\n> >\n> > if (!validateSensorControls()) {\n> > LOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> > @@ -569,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > const ipu3_uapi_stats_3a *stats =\n> > reinterpret_cast(mem.data());\n> >\n> > - IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> >\n> > if (frameContext.frame != frame)\n> > LOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n> > @@ -618,7 +619,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > {\n> > /* \\todo Start processing for 'frame' based on 'controls'. */\n> > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> > + IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);\n> > +\n> > + /* \\todo Implement queueRequest to each algorithm. */\n> > + (void)frameContext;\n> > + (void)controls;\n> \n> I'm sure this will be expanded later but it could be simplified as\n> \n> void IPAIPU3::queueRequest(const uint32_t frame, [[maybe_unused]] const ControlList &controls)\n> {\n> /* \\todo Start processing for 'frame' based on 'controls'. */\n> context_.frameContexts.initialise(frame);\n\nYes, but I don't want to make add the maybe_unused's just to remove\nthem.\n\nAnd The frame context will be used by the last patch in this series.\n\nIf you insist, we can simplify here, but it's just churn eitherway, on\nthe way to get to the last patch.\n\n\n> }\n> \n> > }\n> >\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..ecb47f287350\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.cpp\n> > @@ -0,0 +1,96 @@\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> Is this needed here or can be moved to the .h ?\n\nThis is the definition, that has to be in a .cpp doesn't it ?\n\nIt's also 'private' to the fc_queue. I wouldn't expect other components\nto use this log category.\n\n> \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 Frame Context instances through the IPA\n> \n> frame context or FrameContext, here and in the other parts of the\n> documentation ?\n\nI prefer capitalizing the component names, so that it's clear that we're\ntalking about a 'libcamera noun'.\n\nIn the instance above, I could remove the space..\n (s/Frame Context/FrameContext)\n\n> \n> > + * \\tparam FrameContext The IPA specific Frame Context type for this queue\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> > + * FrameContexts are expected to be referenced by a monotonically increasing\n> > + * sequence count referring to a Frame sequence.\n> \"Frame sequence number\" ?\n\nAck.\n\n\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 FrameContexts should inherit from the IPAFrameContext to support\n> \n> IPA specific frame context implementations shall inherit from the\n> IPAFrameContext base class to support..\n> \n> > + * the minimum required features for a FrameContext, including the frame number\n> > + * and error flags that relate to the frame.\n> > + *\n> > + * FrameContexts are overwritten when they are recycled and re-initialised by\n> > + * the first access made on them by either initialise(frame) or get(frame). If a\n> \n> That's what a circular array does.\n> \n> I would rather express the fact that the number of available contexts\n> is required to be larger than the platform pipeline depth (with a\n> \\todo to reference the property once we have it defined). After\n> pipelineDepth frames have been processed the FrameContext is recycled\n> and forever lost.\n\nThe important part to me here is to describe the behaviour of overflow\nindeed.\n\n> > + * FrameContext is first accessed through get(frame) before calling initialise()\n> > + * a PFCError is automatically raised on the FrameContext to be transferred to\n> > + * the Request when it completes.\n> \n> This would happen because we have a request underrun, right ?\n> \n> I would state that. Like:\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 now 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> the PFCError flag is automatically raised on the FrameContext.\n> \n> How does it sound ?\n\nI'll try to respin it all into an updated v2.\n\n\n> \n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::clear()\n> > + * \\brief Reinitialise all data on the queue\n> \n> Or \"Clear the queue content\" ?\n\nSure.\n\n> \n> > + *\n> > + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> \n> I would add that the Queue has to be re-initialized at every streaming\n> session start, likely in IPA::configure()\n\nYes, but actually IPA::start() is the real key point to ensure the queue\nis clear. configure() should be irrelevant in fact. \n\nExcept I fear start() may race with queueReqeust(), so I'll have to\nverify this. In which case, configure() and stop() would be an\nalternative.\n\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::initialise(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> > + * If the FrameContext was already initialized for this sequence, a warning will\n> > + * be reported and the previously initialized FrameContext is returned.\n> > + *\n> \n> Should we say it is expected to be called at queueRequest() time ?\n\nYes, probably.\n\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 intiialised for the \\a frame, it will be\n> \n> s/intiialised/initialised/\n> \n> > + * initialised and a PFCError will be flagged on the context to be transferred\n> > + * to the Request when it completes.\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..82ce36811926\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.h\n> > @@ -0,0 +1,93 @@\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> Unecessary empty line\n> \n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> \n> Not needed it seems\n> \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(FCQueue)\n> > +\n> > +namespace ipa {\n> > +\n> > +/*\n> > + * Maximum number of frame contexts to be held onto\n> > + *\n> > + * \\todo Should be larger than ISP delay + sensor delay\n> > + */\n> > +static constexpr uint32_t kMaxFrameContexts = 16;\n> > +\n> > +template\n> > +class FCQueue : public std::array\n> > +{\n> > +private:\n> > + void initialise(FrameContext &frameContext, const uint32_t frame)\n> > + {\n> > + frameContext = {};\n> > + frameContext.frame = frame;\n> > + }\n> > +\n> > +public:\n> > + void clear()\n> > + {\n> > + this->fill({});\n> > + }\n> > +\n> > + FrameContext &initialise(const uint32_t frame)\n> > + {\n> > + FrameContext &frameContext = this->at(frame & kMaxFrameContexts);\n> > +\n> > + /*\n> > + * Do not re-initialise if a get() call has already fetched this\n> > + * frame context to preseve the error flags already raised.\n> > + */\n> > + if (frame == frameContext.frame && frame != 0) {\n> \n> This should be <= as it needs to catch cases where the frame context\n> has been possibile overwritten by algorithms calling get() for a\n> number of times larger than the pipeline depth.\n\nYes!\n\nI was working on the theory that the PFCError flag would already be\nraised by the get() call, but if we overflow even the internal storage\nhere, I wonder if we should also explicitly set the PFCError here again\nas well.\n\n\n\n> \n> > + LOG(FCQueue, Warning)\n> > + << \"Frame \" << frame << \" already initialised\";\n> > + } else {\n> > + initialise(frameContext, frame);\n> > + }\n> \n> No brances needed it seems\n> \n> > +\n> > + return frameContext;\n> > + }\n> > +\n> > + FrameContext &get(uint32_t frame)\n> > + {\n> > + FrameContext &frameContext = this->at(frame & kMaxFrameContexts);\n> > +\n> > + if (frame != frameContext.frame) {\n> \n> Or if (frame == frameContext.frame)\n> return frameContext;\n> \n> To save one indentation level. Up to you.\n\nOh yes, definitely ;-)\n\n\n\n> \n> > + LOG(FCQueue, Warning)\n> > + << \"Obtained an uninitialised FrameContext for \"\n> > + << frame;\n> > +\n> > + initialise(frameContext, frame);\n> > +\n> > + /*\n> > + * The frame context has been retrieved before it was\n> > + * initialised through the initialise() call. This\n> > + * indicates an algorithm attempted to access a Frame\n> > + * context before it was queued to the IPA.\n> > + *\n> > + * Controls applied for this request may be left\n> > + * unhandled.\n> > + */\n> > + frameContext.error |= Request::PFCError;\n> > + }\n> > +\n> > + return frameContext;\n> > + }\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> > 2.34.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 73698C3275\n\tfor ;\n\tTue, 26 Jul 2022 10:00:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F79D63314;\n\tTue, 26 Jul 2022 12:00:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CEDEB60487\n\tfor ;\n\tTue, 26 Jul 2022 12:00:06 +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 6558A735;\n\tTue, 26 Jul 2022 12:00:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658829608;\n\tbh=pj3QszN0V9YOG6+SCghCjEOeQkh/59hVb5yawRmWr2A=;\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=eSDdYXHbzYKOZhq9U0hUlg0z5ge40ACxJmow/w/2nr10qcUCsdYinV5smhZIKo3LW\n\tFshxvUucPmpdyrDgAaGl1MnxfEmYFFIsmwdb37cHt5SWjaRmAtF0qetykrrONg3yzR\n\txutxh9TjNbV6CR/cmlt9rmiK2meQTmmPiCqL8odZb/otRvLMLagos7558hPN9U1OTQ\n\tAiwWyXFkfzb9nCUD43AQ0i/fI9q500ra4lYx6DCRSF0NTl40dnHTYE6Mt/ADz8xYNr\n\tN1fWDQWqVKQwUjA4+QL+BtvPpSlL/Pa+nwmlBxlHEwTVm/qV6dutR93BkiGdha4oKP\n\tHr/nJMqyhIYDw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658829606;\n\tbh=pj3QszN0V9YOG6+SCghCjEOeQkh/59hVb5yawRmWr2A=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=peU3MapMJIRTJYLABiW78YwgCo8rSodvJ4aRoBYJqphgIFY/0dFHiPdcuMVNRROxz\n\t6SpVo//TNiX3RzMyWSRgxJw1Pu7JYaLNY4m1zQoVNrW/OEGxOewq7aZ3QvoeDWzrqP\n\tOyYhqKtqVb5RZQgTslFFTR+O8wAVnoRw6KpZ2ZJQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"peU3MapM\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220725152051.asydfrne3gjiqpin@uno.localdomain>","References":"<20220721121310.1286862-1-kieran.bingham@ideasonboard.com>\n\t<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>\n\t<20220725152051.asydfrne3gjiqpin@uno.localdomain>","To":"Jacopo Mondi ","Date":"Tue, 26 Jul 2022 11:00:03 +0100","Message-ID":"<165882960310.3981176.9524645649595131859@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24144,"web_url":"https://patchwork.libcamera.org/comment/24144/","msgid":"<165883253296.3981176.14388077510418585560@Monstersaurus>","date":"2022-07-26T10:48:52","subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2022-07-26 11:00:03)\n> Quoting Jacopo Mondi (2022-07-25 16:20:51)\n> > Hi Kieran\n> > \n> > On Thu, Jul 21, 2022 at 01:13:02PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > From: Umang Jain \n> > >\n> > > Provide a direct FCQueue abstraction to facilitate creating a queue of\n> > > Frame Context structures.\n> > >\n> > > The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue\n> > > added to the IPU3, but the Algorithms are not yet moved from the single\n> > > FrameContext exposed by the IPA context to use the correct one from the\n> > > queue until the Algorithm interfaces are fully updated.\n> > >\n> > > Signed-off-by: Umang Jain \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > src/ipa/ipu3/ipa_context.cpp | 17 -------\n> > > src/ipa/ipu3/ipa_context.h | 15 ++----\n> > > src/ipa/ipu3/ipu3.cpp | 13 +++--\n> > > src/ipa/libipa/fc_queue.cpp | 96 ++++++++++++++++++++++++++++++++++++\n> > > src/ipa/libipa/fc_queue.h | 93 ++++++++++++++++++++++++++++++++++\n> > > src/ipa/libipa/meson.build | 2 +\n> > > 6 files changed, 204 insertions(+), 32 deletions(-)\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/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > > index 13cdb835ef7f..dd139cb4c868 100644\n> > > --- a/src/ipa/ipu3/ipa_context.cpp\n> > > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > > @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {\n> > > * struct ipu3_uapi_gamma_corr_lut for further details.\n> > > */\n> > >\n> > > -/**\n> > > - * \\brief Default constructor for IPAFrameContext\n> > > - */\n> > > -IPAFrameContext::IPAFrameContext() = default;\n> > > -\n> > > -/**\n> > > - * \\brief Construct a IPAFrameContext instance\n> > > - */\n> > > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> > > - : frame(id), frameControls(reqControls)\n> > > -{\n> > > - sensor = {};\n> > > -}\n> > > -\n> > > /**\n> > > * \\var IPAFrameContext::frame\n> > > * \\brief The frame number\n> > > *\n> > > - * \\var IPAFrameContext::frameControls\n> > > - * \\brief Controls sent in by the application while queuing the request\n> > > - *\n> > > * \\var IPAFrameContext::sensor\n> > > * \\brief Effective sensor values that were applied for the frame\n> > > *\n> > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > > index 42e11141d3a1..193fc44a821a 100644\n> > > --- a/src/ipa/ipu3/ipa_context.h\n> > > +++ b/src/ipa/ipu3/ipa_context.h\n> > > @@ -8,8 +8,6 @@\n> > >\n> > > #pragma once\n> > >\n> > > -#include \n> > > -\n> > > #include \n> > >\n> > > #include \n> > > @@ -17,13 +15,12 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > +\n> > > namespace libcamera {\n> > >\n> > > namespace ipa::ipu3 {\n> > >\n> > > -/* Maximum number of frame contexts to be held */\n> > > -static constexpr uint32_t kMaxFrameContexts = 16;\n> > \n> > The size of the FCQueue should probably come from the IPA\n> > implementation, as different platforms will have a different queue\n> > depth to handle.\n> > \n> > It can be made a second template argument.\n> \n> Yes, this is absoultely a platform specific value.\n> \n> I simply haven't made it adjustable yet.\n> \n> I believe it could also be handled on top, as '16' is bigger than both\n> expected platforms currently ;-)\n> \n> I'd rather handle that as a separate patch as well - as whatever size it\n> is contstructed as needs to be conveyed to the pipeline handler too.\n> \n> Either the PH should state what size the IPA should make, or otherwise\n> they should both be hardcoded to the same size.\n> \n> But that's enough discussion that I think it deserves it's own\n> patch/implementation.\n> \n> \n> > > -\n> > > struct IPASessionConfiguration {\n> > > struct {\n> > > ipu3_uapi_grid_config bdsGrid;\n> > > @@ -77,23 +74,19 @@ struct IPAActiveState {\n> > > };\n> > >\n> > > struct IPAFrameContext {\n> > > - IPAFrameContext();\n> > > - IPAFrameContext(uint32_t id, const ControlList &reqControls);\n> > > + uint32_t frame;\n> > >\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > } sensor;\n> > > -\n> > > - uint32_t frame;\n> > > - ControlList frameControls;\n> > > };\n> > >\n> > > struct IPAContext {\n> > > IPASessionConfiguration configuration;\n> > > IPAActiveState activeState;\n> > >\n> > > - std::array frameContexts;\n> > > + FCQueue frameContexts;\n> > > };\n> > >\n> > > } /* namespace ipa::ipu3 */\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 2f6bb672f7bb..55e82cd404f4 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -38,6 +38,8 @@\n> > > #include \"algorithms/tone_mapping.h\"\n> > > #include \"libipa/camera_sensor_helper.h\"\n> > >\n> > > +#include \"ipa_context.h\"\n> > > +\n> > > /* Minimum grid width, expressed as a number of cells */\n> > > static constexpr uint32_t kMinGridWidth = 16;\n> > > /* Maximum grid width, expressed as a number of cells */\n> > > @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> > >\n> > > /* Clean IPAActiveState at each reconfiguration. */\n> > > context_.activeState = {};\n> > > - IPAFrameContext initFrameContext;\n> > > - context_.frameContexts.fill(initFrameContext);\n> > > + context_.frameContexts.clear();\n> > >\n> > > if (!validateSensorControls()) {\n> > > LOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> > > @@ -569,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > > const ipu3_uapi_stats_3a *stats =\n> > > reinterpret_cast(mem.data());\n> > >\n> > > - IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> > > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > >\n> > > if (frameContext.frame != frame)\n> > > LOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n> > > @@ -618,7 +619,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > > {\n> > > /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> > > + IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);\n> > > +\n> > > + /* \\todo Implement queueRequest to each algorithm. */\n> > > + (void)frameContext;\n> > > + (void)controls;\n> > \n> > I'm sure this will be expanded later but it could be simplified as\n> > \n> > void IPAIPU3::queueRequest(const uint32_t frame, [[maybe_unused]] const ControlList &controls)\n> > {\n> > /* \\todo Start processing for 'frame' based on 'controls'. */\n> > context_.frameContexts.initialise(frame);\n> \n> Yes, but I don't want to make add the maybe_unused's just to remove\n> them.\n> \n> And The frame context will be used by the last patch in this series.\n> \n> If you insist, we can simplify here, but it's just churn eitherway, on\n> the way to get to the last patch.\n> \n> \n> > }\n> > \n> > > }\n> > >\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..ecb47f287350\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/fc_queue.cpp\n> > > @@ -0,0 +1,96 @@\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> > Is this needed here or can be moved to the .h ?\n> \n> This is the definition, that has to be in a .cpp doesn't it ?\n> \n> It's also 'private' to the fc_queue. I wouldn't expect other components\n> to use this log category.\n> \n> > \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 Frame Context instances through the IPA\n> > \n> > frame context or FrameContext, here and in the other parts of the\n> > documentation ?\n> \n> I prefer capitalizing the component names, so that it's clear that we're\n> talking about a 'libcamera noun'.\n> \n> In the instance above, I could remove the space..\n> (s/Frame Context/FrameContext)\n> \n> > \n> > > + * \\tparam FrameContext The IPA specific Frame Context type for this queue\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> > > + * FrameContexts are expected to be referenced by a monotonically increasing\n> > > + * sequence count referring to a Frame sequence.\n> > \"Frame sequence number\" ?\n> \n> Ack.\n> \n> \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 FrameContexts should inherit from the IPAFrameContext to support\n> > \n> > IPA specific frame context implementations shall inherit from the\n> > IPAFrameContext base class to support..\n> > \n> > > + * the minimum required features for a FrameContext, including the frame number\n> > > + * and error flags that relate to the frame.\n> > > + *\n> > > + * FrameContexts are overwritten when they are recycled and re-initialised by\n> > > + * the first access made on them by either initialise(frame) or get(frame). If a\n> > \n> > That's what a circular array does.\n> > \n> > I would rather express the fact that the number of available contexts\n> > is required to be larger than the platform pipeline depth (with a\n> > \\todo to reference the property once we have it defined). After\n> > pipelineDepth frames have been processed the FrameContext is recycled\n> > and forever lost.\n> \n> The important part to me here is to describe the behaviour of overflow\n> indeed.\n> \n> > > + * FrameContext is first accessed through get(frame) before calling initialise()\n> > > + * a PFCError is automatically raised on the FrameContext to be transferred to\n> > > + * the Request when it completes.\n> > \n> > This would happen because we have a request underrun, right ?\n> > \n> > I would state that. Like:\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 now 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> > the PFCError flag is automatically raised on the FrameContext.\n> > \n> > How does it sound ?\n> \n> I'll try to respin it all into an updated v2.\n> \n> \n> > \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn FCQueue::clear()\n> > > + * \\brief Reinitialise all data on the queue\n> > \n> > Or \"Clear the queue content\" ?\n> \n> Sure.\n> \n> > \n> > > + *\n> > > + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> > \n> > I would add that the Queue has to be re-initialized at every streaming\n> > session start, likely in IPA::configure()\n> \n> Yes, but actually IPA::start() is the real key point to ensure the queue\n> is clear. configure() should be irrelevant in fact. \n> \n> Except I fear start() may race with queueReqeust(), so I'll have to\n> verify this. In which case, configure() and stop() would be an\n> alternative.\n> \n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn FCQueue::initialise(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> > > + * If the FrameContext was already initialized for this sequence, a warning will\n> > > + * be reported and the previously initialized FrameContext is returned.\n> > > + *\n> > \n> > Should we say it is expected to be called at queueRequest() time ?\n> \n> Yes, probably.\n> \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 intiialised for the \\a frame, it will be\n> > \n> > s/intiialised/initialised/\n> > \n> > > + * initialised and a PFCError will be flagged on the context to be transferred\n> > > + * to the Request when it completes.\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..82ce36811926\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/fc_queue.h\n> > > @@ -0,0 +1,93 @@\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> > Unecessary empty line\n> > \n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > \n> > Not needed it seems\n> > \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(FCQueue)\n> > > +\n> > > +namespace ipa {\n> > > +\n> > > +/*\n> > > + * Maximum number of frame contexts to be held onto\n> > > + *\n> > > + * \\todo Should be larger than ISP delay + sensor delay\n> > > + */\n> > > +static constexpr uint32_t kMaxFrameContexts = 16;\n> > > +\n> > > +template\n> > > +class FCQueue : public std::array\n> > > +{\n> > > +private:\n> > > + void initialise(FrameContext &frameContext, const uint32_t frame)\n> > > + {\n> > > + frameContext = {};\n> > > + frameContext.frame = frame;\n> > > + }\n> > > +\n> > > +public:\n> > > + void clear()\n> > > + {\n> > > + this->fill({});\n> > > + }\n> > > +\n> > > + FrameContext &initialise(const uint32_t frame)\n> > > + {\n> > > + FrameContext &frameContext = this->at(frame & kMaxFrameContexts);\n> > > +\n> > > + /*\n> > > + * Do not re-initialise if a get() call has already fetched this\n> > > + * frame context to preseve the error flags already raised.\n> > > + */\n> > > + if (frame == frameContext.frame && frame != 0) {\n> > \n> > This should be <= as it needs to catch cases where the frame context\n> > has been possibile overwritten by algorithms calling get() for a\n> > number of times larger than the pipeline depth.\n> \n> Yes!\n> \n> I was working on the theory that the PFCError flag would already be\n> raised by the get() call, but if we overflow even the internal storage\n> here, I wonder if we should also explicitly set the PFCError here again\n> as well.\n> \n> \n> \n> > \n> > > + LOG(FCQueue, Warning)\n> > > + << \"Frame \" << frame << \" already initialised\";\n> > > + } else {\n> > > + initialise(frameContext, frame);\n> > > + }\n> > \n> > No brances needed it seems\n> > \n> > > +\n> > > + return frameContext;\n> > > + }\n> > > +\n> > > + FrameContext &get(uint32_t frame)\n> > > + {\n> > > + FrameContext &frameContext = this->at(frame & kMaxFrameContexts);\n> > > +\n> > > + if (frame != frameContext.frame) {\n> > \n> > Or if (frame == frameContext.frame)\n> > return frameContext;\n> > \n> > To save one indentation level. Up to you.\n> \n> Oh yes, definitely ;-)\n> \n> \n\n\n\nAn assertion here might be useful to ensure that get() isn't trying to\ninitialise over a 'newer' frame.\n\n\tASSERT(frameContext.frame < frame);\n\nIf frameContext.frame > frame - then it means something is trying to\naccess the FCQ while the incoming request queue is overflowing the queue\nwhich I believe we will, but have not yet prevented from happening on \nthe Pipeline Handler side.\n\n\n> \n> > \n> > > + LOG(FCQueue, Warning)\n> > > + << \"Obtained an uninitialised FrameContext for \"\n> > > + << frame;\n> > > +\n> > > + initialise(frameContext, frame);\n> > > +\n> > > + /*\n> > > + * The frame context has been retrieved before it was\n> > > + * initialised through the initialise() call. This\n> > > + * indicates an algorithm attempted to access a Frame\n> > > + * context before it was queued to the IPA.\n> > > + *\n> > > + * Controls applied for this request may be left\n> > > + * unhandled.\n> > > + */\n> > > + frameContext.error |= Request::PFCError;\n> > > + }\n> > > +\n> > > + return frameContext;\n> > > + }\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> > > 2.34.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3494ABE173\n\tfor ;\n\tTue, 26 Jul 2022 10:48:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BB566330E;\n\tTue, 26 Jul 2022 12:48:57 +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 8CD3760487\n\tfor ;\n\tTue, 26 Jul 2022 12:48:55 +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 1D5CB735;\n\tTue, 26 Jul 2022 12:48:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658832537;\n\tbh=8aLA6S7YzFkbfMPTj7GAvDlCrla4hn2Li6g+e6s5h/U=;\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=AW2iOZRad0oUBttSFeqEKf9O9ZyUXavhizingnp+yksNGGE+dtGbATwgmGJY4a8Gm\n\tX3v5aGWOXQJ9qo2LxFZaMUwFuZIKZ6D563uF3sf4TDpCr5i68moG91/+pPxL1gywS1\n\tMfLoLVJiB9O2L6ALB6Q/5q+d+qoGDa0EEhwVmD9eyFtSxZFDcl9Pji+ylWwvuPBOb/\n\tZcD27fB8qB/SgmFd6iXoNNr4zxDv910JzAsx8K8LVokEJIqww9aHYPfXgNB5AnKGDS\n\tJfN5StzPbeizQWXVLNfoZ8RRF8L1bKNVEwbq+RjwC3e4SwIbswxXlcpcDeEIASTMSb\n\tGTCL94y+AqYOg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658832535;\n\tbh=8aLA6S7YzFkbfMPTj7GAvDlCrla4hn2Li6g+e6s5h/U=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hJf0OFqWpgNNchc6iBTL++4rO3x/qCjh1ekimVCv1xxio8H6N8CcAxetRwNGbJw7t\n\tYoFzgv757tw5OM3mOkNtzjod+NCq9JV1zND+zVamt2OvtZ9g5DjxkBogIUjYKU+Vz6\n\t2+xT2m2/dXe53Gk4Pe2kkEzRcMHuvakeHeXAHHww="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"hJf0OFqW\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<165882960310.3981176.9524645649595131859@Monstersaurus>","References":"<20220721121310.1286862-1-kieran.bingham@ideasonboard.com>\n\t<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>\n\t<20220725152051.asydfrne3gjiqpin@uno.localdomain>\n\t<165882960310.3981176.9524645649595131859@Monstersaurus>","To":"Jacopo Mondi ","Date":"Tue, 26 Jul 2022 11:48:52 +0100","Message-ID":"<165883253296.3981176.14388077510418585560@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24368,"web_url":"https://patchwork.libcamera.org/comment/24368/","msgid":"<20220804125437.tpiytk5j4rpucnta@uno.localdomain>","date":"2022-08-04T12:54:37","subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n I'm applying comments on top of Kierans' RFC and I have two\nquestions here\n\nOn Sat, Jul 23, 2022 at 02:21:48AM +0530, Umang Jain via libcamera-devel wrote:\n> Hi Kieran,\n>\n> On 7/21/22 17:43, Kieran Bingham wrote:\n> > From: Umang Jain \n> >\n> > Provide a direct FCQueue abstraction to facilitate creating a queue of\n> > Frame Context structures.\n>\n>\n> Ah we are back again to have FCQueue separately (in class files) :-)\n>\n> >\n> > The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue\n>\n>\n> You can just say that the constructors for IPAFrameContext are dropped.\n>\n>\n> > added to the IPU3, but the Algorithms are not yet moved from the single\n>\n> nit: s/IPU3/IPAIPU3/\n>\n> Furthermore, a queue is already present in the form of:\n>\n>         std::array frameContexts;\n>\n> It is just that the queue is transformed to FCQueue. Since this is a RFC, I\n> won't dwell much on the commit message, it can get accurately described in\n> future interations.\n>\n> > FrameContext exposed by the IPA context to use the correct one from the\n> > queue until the Algorithm interfaces are fully updated.\n> >\n> > Signed-off-by: Umang Jain \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/ipu3/ipa_context.cpp | 17 -------\n> > src/ipa/ipu3/ipa_context.h | 15 ++----\n> > src/ipa/ipu3/ipu3.cpp | 13 +++--\n> > src/ipa/libipa/fc_queue.cpp | 96 ++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/fc_queue.h | 93 ++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 6 files changed, 204 insertions(+), 32 deletions(-)\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/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > index 13cdb835ef7f..dd139cb4c868 100644\n> > --- a/src/ipa/ipu3/ipa_context.cpp\n> > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {\n> > * struct ipu3_uapi_gamma_corr_lut for further details.\n> > */\n> > -/**\n> > - * \\brief Default constructor for IPAFrameContext\n> > - */\n> > -IPAFrameContext::IPAFrameContext() = default;\n> > -\n> > -/**\n> > - * \\brief Construct a IPAFrameContext instance\n> > - */\n> > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> > -\t: frame(id), frameControls(reqControls)\n> > -{\n> > -\tsensor = {};\n> > -}\n> > -\n> > /**\n> > * \\var IPAFrameContext::frame\n> > * \\brief The frame number\n> > *\n> > - * \\var IPAFrameContext::frameControls\n> > - * \\brief Controls sent in by the application while queuing the request\n> > - *\n> > * \\var IPAFrameContext::sensor\n> > * \\brief Effective sensor values that were applied for the frame\n> > *\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index 42e11141d3a1..193fc44a821a 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -8,8 +8,6 @@\n> > #pragma once\n> > -#include \n> > -\n> > #include \n> > #include \n> > @@ -17,13 +15,12 @@\n> > #include \n> > #include \n> > +#include \n> > +\n> > namespace libcamera {\n> > namespace ipa::ipu3 {\n> > -/* Maximum number of frame contexts to be held */\n> > -static constexpr uint32_t kMaxFrameContexts = 16;\n> > -\n> > struct IPASessionConfiguration {\n> > \tstruct {\n> > \t\tipu3_uapi_grid_config bdsGrid;\n> > @@ -77,23 +74,19 @@ struct IPAActiveState {\n> > };\n> > struct IPAFrameContext {\n> > -\tIPAFrameContext();\n> > -\tIPAFrameContext(uint32_t id, const ControlList &reqControls);\n> > +\tuint32_t frame;\n> > \tstruct {\n> > \t\tuint32_t exposure;\n> > \t\tdouble gain;\n> > \t} sensor;\n> > -\n> > -\tuint32_t frame;\n> > -\tControlList frameControls;\n>\n>\n> Okay, we dropped the ControlList here, because every algo will get the\n> ControlList in the new interface\n>\n> > };\n> > struct IPAContext {\n> > \tIPASessionConfiguration configuration;\n> > \tIPAActiveState activeState;\n> > -\tstd::array frameContexts;\n> > +\tFCQueue frameContexts;\n> > };\n> > } /* namespace ipa::ipu3 */\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 2f6bb672f7bb..55e82cd404f4 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -38,6 +38,8 @@\n> > #include \"algorithms/tone_mapping.h\"\n> > #include \"libipa/camera_sensor_helper.h\"\n> > +#include \"ipa_context.h\"\n> > +\n> > /* Minimum grid width, expressed as a number of cells */\n> > static constexpr uint32_t kMinGridWidth = 16;\n> > /* Maximum grid width, expressed as a number of cells */\n> > @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> > \t/* Clean IPAActiveState at each reconfiguration. */\n> > \tcontext_.activeState = {};\n> > -\tIPAFrameContext initFrameContext;\n> > -\tcontext_.frameContexts.fill(initFrameContext);\n> > +\tcontext_.frameContexts.clear();\n> context_.frameContexts.clear();\n>\n> Should also be done in IPAIPU3::stop().\n>\n\nI understand IPA::configure() is not enough, as we can have star/stop\nsequences without a reconfiguration in between.\n\nBut why stop() and not start()\n\n> > \tif (!validateSensorControls()) {\n> > \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> > @@ -569,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > \tconst ipu3_uapi_stats_3a *stats =\n> > \t\treinterpret_cast(mem.data());\n> > -\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> > +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > \tif (frameContext.frame != frame)\n> > \t\tLOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n> > @@ -618,7 +619,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > {\n> > \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> > -\tcontext_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\n> > +\tIPAFrameContext &frameContext = context_.frameContexts.initialise(frame);\n> > +\n> > +\t/* \\todo Implement queueRequest to each algorithm. */\n> > +\t(void)frameContext;\n> > +\t(void)controls;\n> > }\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..ecb47f287350\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.cpp\n> > @@ -0,0 +1,96 @@\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 Frame Context instances through the IPA\n>\n>\n> s/queueing/queuing/\n>\n\nBoth spelling seems to be correct, and we have both in the codebase.\nI'll keep what Kieran had initially.\n\nThanks\n j\n\n> > + * \\tparam FrameContext The IPA specific Frame Context type for this queue\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> > + * FrameContexts are expected to be referenced by a monotonically increasing\n> > + * sequence count referring to a Frame sequence.\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>\n>\n> s/queueing/queuing/\n>\n> > + * a given frame, and processing of any ISP related controls and statistics for\n> > + * the same corresponding image.\n> > + *\n> > + * IPA specific FrameContexts should inherit from the IPAFrameContext to support\n> > + * the minimum required features for a FrameContext, including the frame number\n> > + * and error flags that relate to the frame.\n> > + *\n> > + * FrameContexts are overwritten when they are recycled and re-initialised by\n> > + * the first access made on them by either initialise(frame) or get(frame). If a\n> > + * FrameContext is first accessed through get(frame) before calling initialise()\n> > + * a PFCError is automatically raised on the FrameContext to be transferred to\n> > + * the Request when it completes.\n>\n>\n> I get the point here, but I feel the documentation on get()/PFCError etc. is\n> a bit jerky.\n>\n> Once we have some context setup for FCQ, goals, error situations etc. maybe\n> it will fit into the flow.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::clear()\n> > + * \\brief Reinitialise all data on the queue\n> > + *\n> > + * Reset the queue and ensure all FrameContext slots are re-initialised.\n> > + */\n> > +\n> > +/**\n> > + * \\fn FCQueue::initialise(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> > + * If the FrameContext was already initialized for this sequence, a warning will\n> > + * be reported and the previously initialized FrameContext is returned.\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 intiialised for the \\a frame, it will be\n> s/intiialised/initialised/\n> > + * initialised and a PFCError will be flagged on the context to be transferred\n> > + * to the Request when it completes.\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..82ce36811926\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/fc_queue.h\n> > @@ -0,0 +1,93 @@\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> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(FCQueue)\n> > +\n> > +namespace ipa {\n> > +\n> > +/*\n> > + * Maximum number of frame contexts to be held onto\n> > + *\n> > + * \\todo Should be larger than ISP delay + sensor delay\n> > + */\n> > +static constexpr uint32_t kMaxFrameContexts = 16;\n> > +\n> > +template\n>\n> Nice, templates wohoo!\n>\n> > +class FCQueue : public std::array\n>\n>\n> Private inheritance will be required. We don't want to expose std::array\n> APIs to get used accidentally.\n>\n> > +{\n> > +private:\n> > +\tvoid initialise(FrameContext &frameContext, const uint32_t frame)\n> > +\t{\n> > +\t\tframeContext = {};\n> > +\t\tframeContext.frame = frame;\n> > +\t}\n> > +\n> > +public:\n> > +\tvoid clear()\n> > +\t{\n> > +\t\tthis->fill({});\n> > +\t}\n> > +\n> > +\tFrameContext &initialise(const uint32_t frame)\n> > +\t{\n> > +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\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 error flags already raised.\n> > +\t\t */\n> > +\t\tif (frame == frameContext.frame && frame != 0) {\n> > +\t\t\tLOG(FCQueue, Warning)\n> > +\t\t\t\t<< \"Frame \" << frame << \" already initialised\";\n> > +\t\t} else {\n> > +\t\t\tinitialise(frameContext, frame);\n> > +\t\t}\n> > +\n> > +\t\treturn frameContext;\n> > +\t}\n> > +\n> > +\tFrameContext &get(uint32_t frame)\n> > +\t{\n> > +\t\tFrameContext &frameContext = this->at(frame & kMaxFrameContexts);\n> > +\n> > +\t\tif (frame != frameContext.frame) {\n> > +\t\t\tLOG(FCQueue, Warning)\n> > +\t\t\t\t<< \"Obtained an uninitialised FrameContext for \"\n> > +\t\t\t\t<< frame;\n> > +\n> > +\t\t\tinitialise(frameContext, frame);\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * The frame context has been retrieved before it was\n> > +\t\t\t * initialised through the initialise() call. This\n> > +\t\t\t * indicates an algorithm attempted to access a Frame\n> > +\t\t\t * context before it was queued to the IPA.\n> > +\t\t\t *\n> > +\t\t\t * Controls applied for this request may be left\n> > +\t\t\t * unhandled.\n> > +\t\t\t */\n> > +\t\t\tframeContext.error |= Request::PFCError;\n> > +\t\t}\n> > +\n> > +\t\treturn frameContext;\n> > +\t}\n> > +};\n>\n>\n> For now, it looks more or less as per stated in the design.\n>\n> Thank you for the work!\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 22380BE173\n\tfor ;\n\tThu, 4 Aug 2022 12:54:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 904B363311;\n\tThu, 4 Aug 2022 14:54:40 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D72F26330D\n\tfor ;\n\tThu, 4 Aug 2022 14:54:39 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 3C60720000A;\n\tThu, 4 Aug 2022 12:54:38 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659617680;\n\tbh=gDKVn+0Uussjmfxn9r8YWnEEqOCkkFjIRQkbQL3VK10=;\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=mHje+BZ7LOIkRKMKmvPxmqo0q424bcnGK0TXQCNF7sNQ80RPVFE5aZXYrnNYBiyKb\n\tb8wJpUQZica3QlblkkbconRomgTorwWPUgKa8MolHU+dxPlFACTq91tvSoVyVOzQUE\n\tZMbHXo5eo1+8KqgBrFy6xZlWAKaprq8XBg54trSMnzI1Rp/orC7wXq11h/P+g13qMm\n\tprZQdLYkP+GRdeNjXBs+iiP3HMoB5CAvsMk0ohW+TpO4rAmfuT/vEAy2gGxnfjXtQy\n\tmdHlhI7uEkc/552747gFkVWZw1OzsjgKDldKcMHYbGtTIWY1YdYp+Au42d0hjfGkol\n\t09iMaV+GDgqtA==","Date":"Thu, 4 Aug 2022 14:54:37 +0200","To":"Umang Jain ","Message-ID":"<20220804125437.tpiytk5j4rpucnta@uno.localdomain>","References":"<20220721121310.1286862-1-kieran.bingham@ideasonboard.com>\n\t<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24375,"web_url":"https://patchwork.libcamera.org/comment/24375/","msgid":"<20220804152349.bwgn2o3m6zhgems3@uno.localdomain>","date":"2022-08-04T15:23:49","subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Thu, Aug 04, 2022 at 02:54:37PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Hi Umang,\n> I'm applying comments on top of Kierans' RFC and I have two\n> questions here\n> > > index 2f6bb672f7bb..55e82cd404f4 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -38,6 +38,8 @@\n> > > #include \"algorithms/tone_mapping.h\"\n> > > #include \"libipa/camera_sensor_helper.h\"\n> > > +#include \"ipa_context.h\"\n> > > +\n> > > /* Minimum grid width, expressed as a number of cells */\n> > > static constexpr uint32_t kMinGridWidth = 16;\n> > > /* Maximum grid width, expressed as a number of cells */\n> > > @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> > > \t/* Clean IPAActiveState at each reconfiguration. */\n> > > \tcontext_.activeState = {};\n> > > -\tIPAFrameContext initFrameContext;\n> > > -\tcontext_.frameContexts.fill(initFrameContext);\n> > > +\tcontext_.frameContexts.clear();\n> > context_.frameContexts.clear();\n> >\n> > Should also be done in IPAIPU3::stop().\n> >\n>\n> I understand IPA::configure() is not enough, as we can have star/stop\n> sequences without a reconfiguration in between.\n>\n> But why stop() and not start()\n>\n\nreplying to myself after having re-read Kieran's reply to the comment:\nstart() can conflict with queueRequest().","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 285E2BE173\n\tfor ;\n\tThu, 4 Aug 2022 15:23:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 776426332B;\n\tThu, 4 Aug 2022 17:23:53 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::222])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D865D6330D\n\tfor ;\n\tThu, 4 Aug 2022 17:23:51 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 6B0274000B;\n\tThu, 4 Aug 2022 15:23:51 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659626633;\n\tbh=8+G3TUkeaKxkp7fBnQtapUjDW06OVn2vljLFPmzCww8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=rJCjUErtgPl0pC4FSPaUXSguIAfu9EdF3trKJyTJxdVooO7IfUACF2Jcuxwbn1kc7\n\t7oeBKnOCogfH9sOXgrhVRqq1Sxh05HUBc7nOT0R9CIiUkA3cUAlg1XxcCxviMBxOz6\n\tGGBnhdG57+bKgWXc5Fg74+KlYkdN6oybF0jNaDGgYU/FRF6JuuVd+v6BAoMJnpOxUX\n\tARv1sC+7yqF3AADw8JZV0TjdG4gjecdHLJJmTSmrZ5yPUY5PLp0nLagbvYmDCPmNR/\n\t2ei67IaqpHUaw3XxfOTMA686+mDnnAADdMboBK5wxuHXNNByqHAH8MQqiQce1a9nZx\n\trYZWYQ8yABzXQ==","Date":"Thu, 4 Aug 2022 17:23:49 +0200","To":"Umang Jain ,\n\tlibcamera devel ","Message-ID":"<20220804152349.bwgn2o3m6zhgems3@uno.localdomain>","References":"<20220721121310.1286862-1-kieran.bingham@ideasonboard.com>\n\t<20220721121310.1286862-5-kieran.bingham@ideasonboard.com>\n\t\n\t<20220804125437.tpiytk5j4rpucnta@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220804125437.tpiytk5j4rpucnta@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame\n\tContext queue to libipa","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]