[{"id":23352,"web_url":"https://patchwork.libcamera.org/comment/23352/","msgid":"<20220608120418.wpnrvgntmcwblawq@uno.localdomain>","date":"2022-06-08T12:04:18","subject":"Re: [libcamera-devel] [PATCH v1 1/4] ipa: ipu3: Separate out frame\n\tcontext queue as a distinct class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Fri, Jun 03, 2022 at 03:22:56PM +0200, Umang Jain via libcamera-devel wrote:\n> There are cases where we need more checks and balance to be carried out\n> by the frame context queue class. For that, separate it out as a\n> distinct class on which we can build upon.\n>\n> For now, a minimialistic implementation is provided with .get(frame)\n> helper which returns a IPAFrameContext pointer for the required frame.\n> Going ahead more such helpers can be provided to access/modify the\n> frame context queue.\n>\n> Signed-off-by: Umang Jain \n> Reviewed-by: Paul Elder \n> Reviewed-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipa_context.cpp | 57 ++++++++++++++++++++++++++++++++++--\n> src/ipa/ipu3/ipa_context.h | 11 ++++++-\n> src/ipa/ipu3/ipu3.cpp | 18 ++++++------\n> 3 files changed, 74 insertions(+), 12 deletions(-)\n>\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 13cdb835..9f95a61c 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -7,12 +7,18 @@\n>\n> #include \"ipa_context.h\"\n>\n> +#include \n> +\n> /**\n> * \\file ipa_context.h\n> * \\brief Context and state information shared between the algorithms\n> */\n>\n> -namespace libcamera::ipa::ipu3 {\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPAIPU3)\n> +\n> +namespace ipa::ipu3 {\n>\n> /**\n> * \\struct IPASessionConfiguration\n> @@ -211,4 +217,51 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> * \\brief Analogue gain multiplier\n> */\n>\n> -} /* namespace libcamera::ipa::ipu3 */\n> +/**\n> + * \\class FCQueue\n> + * \\brief A FIFO circular queue holding IPAFrameContext(s)\n> + *\n> + * FCQueue holds all the IPAFrameContext(s) related to frames required\n> + * to be processed by the IPA at a given point.\n> + */\n> +\n> +/**\n> + * \\brief FCQueue constructor\n> + */\n> +FCQueue::FCQueue()\n> +{\n> +\tclear();\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the IPAFrameContext for the frame\n> + * \\param[in] frame Frame number for which the IPAFrameContext needs to\n> + * retrieved\n> + *\n> + * \\return Pointer to the IPAFrameContext\n> + */\n> +IPAFrameContext *FCQueue::get(uint32_t frame)\n> +{\n> +\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> +\n> +\tif (frame != frameContext.frame) {\n> +\t\tLOG(IPAIPU3, Warning)\n> +\t\t\t<< \"Got wrong frame context for frame\" << frame\n> +\t\t\t<< \" or frame context doesn't exist yet\";\n> +\t}\n> +\n> +\treturn &frameContext;\n> +}\n> +\n> +/**\n> + * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n> + */\n> +void FCQueue::clear()\n> +{\n> +\tIPAFrameContext initFrameContext;\n> +\tthis->fill(initFrameContext);\n> +}\n> +\n> +} /* namespace ipa::ipu3 */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 42e11141..56d281f6 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -89,11 +89,20 @@ struct IPAFrameContext {\n> \tControlList frameControls;\n> };\n>\n> +class FCQueue : public std::array\n\nThis will expose the full std::array<> interface.\nI wonder if this should not rather inherit std::array<> as private and\nselectively expose only certain functions. That's what ControlInfoMap\ndoes, as an example.\n\nThen the next question is if we want to have a circular buffer class\nin libcamera and make\n\nclass FCQueue : public libcamera::CircualBuffer\n{\n\n}\n\n> +{\n> +public:\n> +\tFCQueue();\n> +\n> +\tvoid clear();\n> +\tIPAFrameContext *get(uint32_t frame);\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 2f6bb672..0843d882 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -456,8 +456,8 @@ 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\nUnecessary empty line\n\n>\n> \tif (!validateSensorControls()) {\n> \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> @@ -569,20 +569,20 @@ 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> +\tif (frameContext->frame != frame)\n> \t\tLOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n>\n> -\tframeContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> -\tframeContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> +\tframeContext->sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> +\tframeContext->sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>\n> \tdouble lineDuration = context_.configuration.sensor.lineDuration.get();\n> \tint32_t vBlank = context_.configuration.sensor.defVBlank;\n> \tControlList ctrls(controls::controls);\n>\n> \tfor (auto const &algo : algorithms_)\n> -\t\talgo->process(context_, &frameContext, stats);\n> +\t\talgo->process(context_, frameContext, stats);\n>\n> \tsetControls(frame);\n>\n> @@ -590,11 +590,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \tint64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;\n> \tctrls.set(controls::FrameDuration, frameDuration);\n>\n> -\tctrls.set(controls::AnalogueGain, frameContext.sensor.gain);\n> +\tctrls.set(controls::AnalogueGain, frameContext->sensor.gain);\n>\n> \tctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);\n>\n> -\tctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);\n> +\tctrls.set(controls::ExposureTime, frameContext->sensor.exposure * lineDuration);\n>\n> \t/*\n> \t * \\todo The Metadata provides a path to getting extended data\n> --\n> 2.31.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6D4F6BD160\n\tfor ;\n\tWed, 8 Jun 2022 12:04:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D74465635;\n\tWed, 8 Jun 2022 14:04:22 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE23665632\n\tfor ;\n\tWed, 8 Jun 2022 14:04:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 2C1261BF21B;\n\tWed, 8 Jun 2022 12:04:19 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654689862;\n\tbh=JjNKfGOWMSZUYFD6qSdxmAkrN1B3PMPckNofvpTnhmk=;\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=tWFez28AuD3HclFsJZlnz2VNZobaXonO+ODHAmo+OxGH7Q4TfvMHTQAcGrIEhasmw\n\tQHaEh34zr3fgZYrIaBp/t8+4jy0+6X3IJ2YItcJO3R9MR+GaiEaP5+VAZwAkzkUgNU\n\t8Qw2+UuaUuDfTJdKiJOkxGLGvKwgIb1z2R0SWuCX+YvZEHYNWm/Yi12ufG7Uzc/fpn\n\t1AbW0vo1RezDXiXjz2rrXluYiymdDhFDriB0BSMzg/IVSJ5cdadwYze4erCsSv3ODi\n\tk11FV3lZZ2fc1FgS37BYnTZRFPKcuZTUEGDvXYE8++a1ZJzaTKTSIQoPb0vhD0cs5/\n\tly9QxvVDGPQlg==","Date":"Wed, 8 Jun 2022 14:04:18 +0200","To":"Umang Jain ","Message-ID":"<20220608120418.wpnrvgntmcwblawq@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220603132259.188845-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 1/4] ipa: ipu3: Separate out frame\n\tcontext queue as a distinct class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23365,"web_url":"https://patchwork.libcamera.org/comment/23365/","msgid":"","date":"2022-06-09T09:13:01","subject":"Re: [libcamera-devel] [PATCH v1 1/4] ipa: ipu3: Separate out frame\n\tcontext queue as a distinct class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 6/8/22 14:04, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Fri, Jun 03, 2022 at 03:22:56PM +0200, Umang Jain via libcamera-devel wrote:\n>> There are cases where we need more checks and balance to be carried out\n>> by the frame context queue class. For that, separate it out as a\n>> distinct class on which we can build upon.\n>>\n>> For now, a minimialistic implementation is provided with .get(frame)\n>> helper which returns a IPAFrameContext pointer for the required frame.\n>> Going ahead more such helpers can be provided to access/modify the\n>> frame context queue.\n>>\n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Paul Elder \n>> Reviewed-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/ipa_context.cpp | 57 ++++++++++++++++++++++++++++++++++--\n>> src/ipa/ipu3/ipa_context.h | 11 ++++++-\n>> src/ipa/ipu3/ipu3.cpp | 18 ++++++------\n>> 3 files changed, 74 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>> index 13cdb835..9f95a61c 100644\n>> --- a/src/ipa/ipu3/ipa_context.cpp\n>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>> @@ -7,12 +7,18 @@\n>>\n>> #include \"ipa_context.h\"\n>>\n>> +#include \n>> +\n>> /**\n>> * \\file ipa_context.h\n>> * \\brief Context and state information shared between the algorithms\n>> */\n>>\n>> -namespace libcamera::ipa::ipu3 {\n>> +namespace libcamera {\n>> +\n>> +LOG_DECLARE_CATEGORY(IPAIPU3)\n>> +\n>> +namespace ipa::ipu3 {\n>>\n>> /**\n>> * \\struct IPASessionConfiguration\n>> @@ -211,4 +217,51 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n>> * \\brief Analogue gain multiplier\n>> */\n>>\n>> -} /* namespace libcamera::ipa::ipu3 */\n>> +/**\n>> + * \\class FCQueue\n>> + * \\brief A FIFO circular queue holding IPAFrameContext(s)\n>> + *\n>> + * FCQueue holds all the IPAFrameContext(s) related to frames required\n>> + * to be processed by the IPA at a given point.\n>> + */\n>> +\n>> +/**\n>> + * \\brief FCQueue constructor\n>> + */\n>> +FCQueue::FCQueue()\n>> +{\n>> +\tclear();\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the IPAFrameContext for the frame\n>> + * \\param[in] frame Frame number for which the IPAFrameContext needs to\n>> + * retrieved\n>> + *\n>> + * \\return Pointer to the IPAFrameContext\n>> + */\n>> +IPAFrameContext *FCQueue::get(uint32_t frame)\n>> +{\n>> +\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n>> +\n>> +\tif (frame != frameContext.frame) {\n>> +\t\tLOG(IPAIPU3, Warning)\n>> +\t\t\t<< \"Got wrong frame context for frame\" << frame\n>> +\t\t\t<< \" or frame context doesn't exist yet\";\n>> +\t}\n>> +\n>> +\treturn &frameContext;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n>> + */\n>> +void FCQueue::clear()\n>> +{\n>> +\tIPAFrameContext initFrameContext;\n>> +\tthis->fill(initFrameContext);\n>> +}\n>> +\n>> +} /* namespace ipa::ipu3 */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index 42e11141..56d281f6 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -89,11 +89,20 @@ struct IPAFrameContext {\n>> \tControlList frameControls;\n>> };\n>>\n>> +class FCQueue : public std::array\n> This will expose the full std::array<> interface.\n> I wonder if this should not rather inherit std::array<> as private and\n> selectively expose only certain functions. That's what ControlInfoMap\n> does, as an example.\n\n\nAh correct, I don't want to expose the entire std::array<> interface.\n\nIf you have seen I disable the [] operator in 2/3 but still .at() is \nexposed.\nSo better to inherit it as private. I'll checkout ControlInfoMap.\n\n>\n> Then the next question is if we want to have a circular buffer class\n> in libcamera and make\n>\n> class FCQueue : public libcamera::CircualBuffer\n> {\n>\n> }\n\n\nFeels a bit early, as I have mentioned before. The design discussion is \nstill\nunderway it seems (thanks to your reviews), which I assumed was solidified\nalready (my mistake probably)\n\nNow if it seems they aren't, putting something generic in libcamera core for\nthis purpose - doesn't seem to be a good idea to me because we will probably\nend up customizing the \"generic\" CircularBuffer to suit the needs of IPA \n- which\nwill be living in libcamera-core. I probably resist doing it *for now*, \nuntil we have\na clear idea of the design needs (and putting a design where other IPAs \ncan use it\nhopefully).\n\nI get the temptation to do it generically, but I suppose the priority at \nthe moment\nis get build a support layer for 'per-frame' controls. Get it working on \none platform,\nthen pull out the parts to make it generic - without regressing PFC. \nDoes this sound\ngood?\n\n>\n>> +{\n>> +public:\n>> +\tFCQueue();\n>> +\n>> +\tvoid clear();\n>> +\tIPAFrameContext *get(uint32_t frame);\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 2f6bb672..0843d882 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -456,8 +456,8 @@ 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> Unecessary empty line\n>\n>> \tif (!validateSensorControls()) {\n>> \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n>> @@ -569,20 +569,20 @@ 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>> +\tif (frameContext->frame != frame)\n>> \t\tLOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n>>\n>> -\tframeContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> -\tframeContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>> +\tframeContext->sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> +\tframeContext->sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>>\n>> \tdouble lineDuration = context_.configuration.sensor.lineDuration.get();\n>> \tint32_t vBlank = context_.configuration.sensor.defVBlank;\n>> \tControlList ctrls(controls::controls);\n>>\n>> \tfor (auto const &algo : algorithms_)\n>> -\t\talgo->process(context_, &frameContext, stats);\n>> +\t\talgo->process(context_, frameContext, stats);\n>>\n>> \tsetControls(frame);\n>>\n>> @@ -590,11 +590,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n>> \tint64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;\n>> \tctrls.set(controls::FrameDuration, frameDuration);\n>>\n>> -\tctrls.set(controls::AnalogueGain, frameContext.sensor.gain);\n>> +\tctrls.set(controls::AnalogueGain, frameContext->sensor.gain);\n>>\n>> \tctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);\n>>\n>> -\tctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);\n>> +\tctrls.set(controls::ExposureTime, frameContext->sensor.exposure * lineDuration);\n>>\n>> \t/*\n>> \t * \\todo The Metadata provides a path to getting extended data\n>> --\n>> 2.31.1\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 58B06BD160\n\tfor ;\n\tThu, 9 Jun 2022 09:13:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 881FC65635;\n\tThu, 9 Jun 2022 11:13:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 32E3A61FB4\n\tfor ;\n\tThu, 9 Jun 2022 11:13:05 +0200 (CEST)","from [192.168.101.240] (143.red-176-83-106.dynamicip.rima-tde.net\n\t[176.83.106.143])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AAC042A5;\n\tThu, 9 Jun 2022 11:13:04 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654765987;\n\tbh=BYNvhbVfmSHem9S4lj9dQ930YrPyKOuQ3t3D4XCGhFw=;\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=M7TbkHzLrANnDogBi6HH26Y2tP4vrOuix5ivb092TwWsiAA+HGuS7L2ub3Fj/AnvK\n\tiwVJepM4lU5witm12pMlsQ7s2ls2Bn0mEj4ztul6gOSfLlAH5tdn6Jn7mg3qkBBwGg\n\tgPrLnZkmCwFtMIG4XaMtxO/v1h462Y078uRPsuEjXXt+nbAYhjBkJDRpj7sZ+1x7hp\n\tdysrLekYzwoX0l3ncdTf4wNWV+AqEaD8Sd/Ffo+63qs0XAk53qFb/IAFkyf7ZsL0jl\n\tKt60hFVeubO18crWTqzPQGWAzY25E8/xXUCzvQeuFl1HIA/pqetGvOKaufBWuXY5yg\n\t+/zcUOne8gfWg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654765984;\n\tbh=BYNvhbVfmSHem9S4lj9dQ930YrPyKOuQ3t3D4XCGhFw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ecIFHYJBiKemvCX4Zpxxls3cy65nK1A/llI5FoK6bws7EHSlnU+6vM10MRFTJY67v\n\tzDRsBfmBlq2eTEEsqwxDDAaa6TqlrQXTh2hFePxnMsa7zpO1qfPGjSa6DD0qMN2Uvz\n\t3PDVlZ9PYfhGhAOzXzXv4nrXDrpWRz21A8cjYyCI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ecIFHYJB\"; dkim-atps=neutral","Message-ID":"","Date":"Thu, 9 Jun 2022 11:13:01 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-2-umang.jain@ideasonboard.com>\n\t<20220608120418.wpnrvgntmcwblawq@uno.localdomain>","In-Reply-To":"<20220608120418.wpnrvgntmcwblawq@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v1 1/4] ipa: ipu3: Separate out frame\n\tcontext queue as a distinct class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23369,"web_url":"https://patchwork.libcamera.org/comment/23369/","msgid":"<20220609152328.dyyejalebcuv26ak@uno.localdomain>","date":"2022-06-09T15:23:28","subject":"Re: [libcamera-devel] [PATCH v1 1/4] ipa: ipu3: Separate out frame\n\tcontext queue as a distinct class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Thu, Jun 09, 2022 at 11:13:01AM +0200, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 6/8/22 14:04, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Fri, Jun 03, 2022 at 03:22:56PM +0200, Umang Jain via libcamera-devel wrote:\n> > > There are cases where we need more checks and balance to be carried out\n> > > by the frame context queue class. For that, separate it out as a\n> > > distinct class on which we can build upon.\n> > >\n> > > For now, a minimialistic implementation is provided with .get(frame)\n> > > helper which returns a IPAFrameContext pointer for the required frame.\n> > > Going ahead more such helpers can be provided to access/modify the\n> > > frame context queue.\n> > >\n> > > Signed-off-by: Umang Jain \n> > > Reviewed-by: Paul Elder \n> > > Reviewed-by: Jean-Michel Hautbois \n> > > ---\n> > > src/ipa/ipu3/ipa_context.cpp | 57 ++++++++++++++++++++++++++++++++++--\n> > > src/ipa/ipu3/ipa_context.h | 11 ++++++-\n> > > src/ipa/ipu3/ipu3.cpp | 18 ++++++------\n> > > 3 files changed, 74 insertions(+), 12 deletions(-)\n> > >\n> > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > > index 13cdb835..9f95a61c 100644\n> > > --- a/src/ipa/ipu3/ipa_context.cpp\n> > > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > > @@ -7,12 +7,18 @@\n> > >\n> > > #include \"ipa_context.h\"\n> > >\n> > > +#include \n> > > +\n> > > /**\n> > > * \\file ipa_context.h\n> > > * \\brief Context and state information shared between the algorithms\n> > > */\n> > >\n> > > -namespace libcamera::ipa::ipu3 {\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(IPAIPU3)\n> > > +\n> > > +namespace ipa::ipu3 {\n> > >\n> > > /**\n> > > * \\struct IPASessionConfiguration\n> > > @@ -211,4 +217,51 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> > > * \\brief Analogue gain multiplier\n> > > */\n> > >\n> > > -} /* namespace libcamera::ipa::ipu3 */\n> > > +/**\n> > > + * \\class FCQueue\n> > > + * \\brief A FIFO circular queue holding IPAFrameContext(s)\n> > > + *\n> > > + * FCQueue holds all the IPAFrameContext(s) related to frames required\n> > > + * to be processed by the IPA at a given point.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief FCQueue constructor\n> > > + */\n> > > +FCQueue::FCQueue()\n> > > +{\n> > > +\tclear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Retrieve the IPAFrameContext for the frame\n> > > + * \\param[in] frame Frame number for which the IPAFrameContext needs to\n> > > + * retrieved\n> > > + *\n> > > + * \\return Pointer to the IPAFrameContext\n> > > + */\n> > > +IPAFrameContext *FCQueue::get(uint32_t frame)\n> > > +{\n> > > +\tIPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);\n> > > +\n> > > +\tif (frame != frameContext.frame) {\n> > > +\t\tLOG(IPAIPU3, Warning)\n> > > +\t\t\t<< \"Got wrong frame context for frame\" << frame\n> > > +\t\t\t<< \" or frame context doesn't exist yet\";\n> > > +\t}\n> > > +\n> > > +\treturn &frameContext;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Clear the FCQueue by resetting all the entries in the ring-buffer\n> > > + */\n> > > +void FCQueue::clear()\n> > > +{\n> > > +\tIPAFrameContext initFrameContext;\n> > > +\tthis->fill(initFrameContext);\n> > > +}\n> > > +\n> > > +} /* namespace ipa::ipu3 */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > > index 42e11141..56d281f6 100644\n> > > --- a/src/ipa/ipu3/ipa_context.h\n> > > +++ b/src/ipa/ipu3/ipa_context.h\n> > > @@ -89,11 +89,20 @@ struct IPAFrameContext {\n> > > \tControlList frameControls;\n> > > };\n> > >\n> > > +class FCQueue : public std::array\n> > This will expose the full std::array<> interface.\n> > I wonder if this should not rather inherit std::array<> as private and\n> > selectively expose only certain functions. That's what ControlInfoMap\n> > does, as an example.\n>\n>\n> Ah correct, I don't want to expose the entire std::array<> interface.\n>\n> If you have seen I disable the [] operator in 2/3 but still .at() is\n> exposed.\n> So better to inherit it as private. I'll checkout ControlInfoMap.\n>\n> >\n> > Then the next question is if we want to have a circular buffer class\n> > in libcamera and make\n> >\n> > class FCQueue : public libcamera::CircualBuffer\n> > {\n> >\n> > }\n>\n>\n> Feels a bit early, as I have mentioned before. The design discussion is\n> still\n> underway it seems (thanks to your reviews), which I assumed was solidified\n> already (my mistake probably)\n\nSorry, I didn't want to question already agreed design decision, but\nlooks like 2/4 needs some rework anyway, and there might be ways to\nmake the implementation more robust\n\n>\n> Now if it seems they aren't, putting something generic in libcamera core for\n> this purpose - doesn't seem to be a good idea to me because we will probably\n> end up customizing the \"generic\" CircularBuffer to suit the needs of IPA -\n> which\n> will be living in libcamera-core. I probably resist doing it *for now*,\n> until we have\n> a clear idea of the design needs (and putting a design where other IPAs can\n> use it\n> hopefully).\n\nI was thinking mostly about DelayedControls that currently implements\na circular array too, but I agree more users might help solidify the\ndesign\n\n>\n> I get the temptation to do it generically, but I suppose the priority at the\n> moment\n> is get build a support layer for 'per-frame' controls. Get it working on one\n> platform,\n> then pull out the parts to make it generic - without regressing PFC. Does\n> this sound\n> good?\n\nYes, let's clarify 2/4 and get this one done!\n\nThanks\n j\n\n>\n> >\n> > > +{\n> > > +public:\n> > > +\tFCQueue();\n> > > +\n> > > +\tvoid clear();\n> > > +\tIPAFrameContext *get(uint32_t frame);\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 2f6bb672..0843d882 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -456,8 +456,8 @@ 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> > Unecessary empty line\n> >\n> > > \tif (!validateSensorControls()) {\n> > > \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> > > @@ -569,20 +569,20 @@ 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> > > +\tif (frameContext->frame != frame)\n> > > \t\tLOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n> > >\n> > > -\tframeContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > -\tframeContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > > +\tframeContext->sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > +\tframeContext->sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > >\n> > > \tdouble lineDuration = context_.configuration.sensor.lineDuration.get();\n> > > \tint32_t vBlank = context_.configuration.sensor.defVBlank;\n> > > \tControlList ctrls(controls::controls);\n> > >\n> > > \tfor (auto const &algo : algorithms_)\n> > > -\t\talgo->process(context_, &frameContext, stats);\n> > > +\t\talgo->process(context_, frameContext, stats);\n> > >\n> > > \tsetControls(frame);\n> > >\n> > > @@ -590,11 +590,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > > \tint64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;\n> > > \tctrls.set(controls::FrameDuration, frameDuration);\n> > >\n> > > -\tctrls.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > > +\tctrls.set(controls::AnalogueGain, frameContext->sensor.gain);\n> > >\n> > > \tctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);\n> > >\n> > > -\tctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);\n> > > +\tctrls.set(controls::ExposureTime, frameContext->sensor.exposure * lineDuration);\n> > >\n> > > \t/*\n> > > \t * \\todo The Metadata provides a path to getting extended data\n> > > --\n> > > 2.31.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 44AFCBD161\n\tfor ;\n\tThu, 9 Jun 2022 15:23:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F86D65631;\n\tThu, 9 Jun 2022 17:23:32 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7112A6559A\n\tfor ;\n\tThu, 9 Jun 2022 17:23:30 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id DF5131C0008;\n\tThu, 9 Jun 2022 15:23:29 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654788212;\n\tbh=ic1AdyqseM6FuwKu5iaNXIb5d3ZBBI0za9TGzveFonU=;\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=ig40qkUJn21V3KVFY4PWiexaOSwp8QL97kVxQh0LVAyU/tsMJylRMtQmTevSWAD2v\n\tiXR9ohIIc62EKftydQ0yE7BNWGVx+EBbL6fHGHUMPq4HhWfyo3n9oZ5xBBf+np14b5\n\tCzJD4iXot4KoYxmYV4fKhux1St27tW74xU/La7zLvoi2VdO57VVjkOuR4NGFK9IqwA\n\tpmNJEziaMIcHZqcPJ7uhA8+vx0I1Q0m2CTP0t1whzYZ8+ZjhyR53cFKWl6DO6lbJ44\n\tofthHTWQUxMOj3X9vmk8kgIvKsxVgtB6PPnL63oeAzgLDVr2obOJW0/sK6u89qvVrr\n\tMBeRVQ4j5Xo9w==","Date":"Thu, 9 Jun 2022 17:23:28 +0200","To":"Umang Jain ","Message-ID":"<20220609152328.dyyejalebcuv26ak@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-2-umang.jain@ideasonboard.com>\n\t<20220608120418.wpnrvgntmcwblawq@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 1/4] ipa: ipu3: Separate out frame\n\tcontext queue as a distinct class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]