[{"id":23032,"web_url":"https://patchwork.libcamera.org/comment/23032/","msgid":"<20220518074726.k47zfauj7e6stbng@uno.localdomain>","date":"2022-05-18T07:47:26","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Tue, May 17, 2022 at 09:18:33PM +0200, Umang Jain via libcamera-devel wrote:\n> Instead of having one frame context constantly being updated,\n> this patch aims to introduce per-frame IPAFrameContext which\n> are stored in a ring buffer. Whenever a request is queued, a new\n> IPAFrameContext is created and inserted into the ring buffer.\n>\n> The IPAFrameContext structure itself has been slightly extended\n> to store a frame id and a ControlList for incoming frame\n> controls (sent in by the application). The next step would be to\n> read and set these controls whenever the request is actually queued\n> to the hardware.\n>\n> Since now we are working in multiples of IPAFrameContext, the\n> Algorithm::process() will actually take in a IPAFrameContext pointer\n> (as opposed to a nullptr while preparing for this change).\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------\n> src/ipa/ipu3/algorithms/agc.h | 4 ++--\n> src/ipa/ipu3/ipa_context.cpp | 24 ++++++++++++++++++++++--\n> src/ipa/ipu3/ipa_context.h | 14 +++++++++++++-\n> src/ipa/ipu3/ipu3.cpp | 22 +++++++++++++---------\n> 5 files changed, 55 insertions(+), 20 deletions(-)\n>\n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 383a8deb..f16be534 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -183,14 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> * \\param[in] yGain The gain calculated based on the relative luminance target\n> * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> */\n> -void Agc::computeExposure(IPAContext &context, double yGain,\n> -\t\t\t double iqMeanGain)\n> +void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,\n> +\t\t\t double yGain, double iqMeanGain)\n> {\n> \tconst IPASessionConfiguration &configuration = context.configuration;\n> -\tIPAFrameContext &frameContext = context.frameContext;\n> \t/* Get the effective exposure and gain applied on the sensor. */\n> -\tuint32_t exposure = frameContext.sensor.exposure;\n> -\tdouble analogueGain = frameContext.sensor.gain;\n> +\tuint32_t exposure = frameContext->sensor.exposure;\n> +\tdouble analogueGain = frameContext->sensor.gain;\n>\n> \t/* Use the highest of the two gain estimates. */\n> \tdouble evGain = std::max(yGain, iqMeanGain);\n> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo\n> \t\t\tbreak;\n> \t}\n>\n> -\tcomputeExposure(context, yGain, iqMeanGain);\n> +\tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> }\n>\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 219a1a96..105ae0f2 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -35,8 +35,8 @@ private:\n> \tdouble measureBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\t\t const ipu3_uapi_grid_config &grid) const;\n> \tutils::Duration filterExposure(utils::Duration currentExposure);\n> -\tvoid computeExposure(IPAContext &context, double yGain,\n> -\t\t\t double iqMeanGain);\n> +\tvoid computeExposure(IPAContext &context, IPAFrameContext *frameContext,\n> +\t\t\t double yGain, double iqMeanGain);\n> \tdouble estimateLuminance(IPAActiveState &activeState,\n> \t\t\t\t const ipu3_uapi_grid_config &grid,\n> \t\t\t\t const ipu3_uapi_stats_3a *stats,\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 06eb2776..81b30600 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {\n> * \\var IPAContext::configuration\n> * \\brief The IPA session configuration, immutable during the session\n> *\n> - * \\var IPAContext::frameContext\n> - * \\brief The frame context for the frame being processed\n> + * \\var IPAContext::frameContexts\n> + * \\brief Ring buffer of the IPAFrameContext(s)\n> *\n> * \\var IPAContext::activeState\n> * \\brief The current state of IPA algorithms\n> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {\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 8d681131..42e11141 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -8,16 +8,22 @@\n>\n> #pragma once\n>\n> +#include \n> +\n> #include \n>\n> #include \n>\n> +#include \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> @@ -71,17 +77,23 @@ struct IPAActiveState {\n> };\n>\n> struct IPAFrameContext {\n> +\tIPAFrameContext();\n> +\tIPAFrameContext(uint32_t id, const ControlList &reqControls);\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> -\tIPAFrameContext frameContext;\n> +\tstd::array frameContexts;\n> };\n>\n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 16e5028f..4322f96b 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> \t}\n>\n> \t/* Clean context */\n> -\tcontext_ = {};\n> +\tcontext_.configuration = {};\n> \tcontext_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n>\n> \t/* Construct our Algorithms */\n> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>\n> \t/* Clean IPAActiveState at each reconfiguration. */\n> \tcontext_.activeState = {};\n> -\tcontext_.frameContext = {};\n> +\tIPAFrameContext initFrameContext;\n> +\tcontext_.frameContexts.fill(initFrameContext);\n>\n> \tif (!validateSensorControls()) {\n> \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \tconst ipu3_uapi_stats_3a *stats =\n> \t\treinterpret_cast(mem.data());\n>\n> -\tcontext_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> -\tcontext_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> +\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\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>\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_, nullptr, stats);\n> +\t\talgo->process(context_, &frameContext, stats);\n>\n> \tsetControls(frame);\n>\n> @@ -584,11 +587,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, context_.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, context_.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> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> * Parse the request to handle any IPA-managed controls that were set from the\n> * application such as manual sensor settings.\n> */\n> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,\n> -\t\t\t [[maybe_unused]] const ControlList &controls)\n> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> +\tIPAFrameContext newFrameContext(frame, controls);\n> +\tcontext_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;\n\nJust a little note, which now is not that relevant as we have a\nlimited number of controls, but this goes through at least 2 copies a\nthe 'controls' list.\n\nConstructor:\n\nIPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n\t: frame(id), frameControls(reqControls)\n\nAnd the implicitely defined copy assignment operator.\n\nI wonder if\n\tcontext_.frameContexts[frame % kMaxFrameContexts] = {frame, controls};\n\nwould spare a copy\n\nThe rest looks good to me\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n> }\n>\n> /**\n> --\n> 2.31.0\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 182FBC3256\n\tfor ;\n\tWed, 18 May 2022 07:47:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 377B365657;\n\tWed, 18 May 2022 09:47:30 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C775261FB9\n\tfor ;\n\tWed, 18 May 2022 09:47:28 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 1122CFF80C;\n\tWed, 18 May 2022 07:47:27 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652860050;\n\tbh=tt+9Afad7u/d0EYmFDrJtCurDOhry+Z3N7v4U3KmwFs=;\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=bD9N/QszflDbyXnfn9dtMg0x7B+4cNggZtbiq8Smv2iBd31RUNuxztegf9vJXvK1D\n\t18sa7/DhMd+GzRM8Owkv53q6/+L7rM1FX87ROvpA29/wWeHyzlR2RciHNmaBaP5ewh\n\tgJdNc4xqAsi1jrklpsHXGxxuWZfbGaoCc0eubO776heK9NBWASiRfR3J8qDCVnnahz\n\twV+7vgtaBSHAeum04VVwZiu7LifBBU8NzU4sq1g9yopulfDF8wyWPv4m6t3ougt3Ai\n\tLnW9Ztgy+POdU19eqgITbdQJgKKdo3C8d5A5uIhqoQ7bqdDJNp9G7zDbiVWk8/EkxM\n\tATiKQpQwu9x5w==","Date":"Wed, 18 May 2022 09:47:26 +0200","To":"Umang Jain ","Message-ID":"<20220518074726.k47zfauj7e6stbng@uno.localdomain>","References":"<20220517191833.333122-1-umang.jain@ideasonboard.com>\n\t<20220517191833.333122-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220517191833.333122-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","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":23036,"web_url":"https://patchwork.libcamera.org/comment/23036/","msgid":"","date":"2022-05-18T09:08:29","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang,\n\nOn 17/05/2022 21:18, Umang Jain via libcamera-devel wrote:\n> Instead of having one frame context constantly being updated,\n> this patch aims to introduce per-frame IPAFrameContext which\n> are stored in a ring buffer. Whenever a request is queued, a new\n> IPAFrameContext is created and inserted into the ring buffer.\n> \n> The IPAFrameContext structure itself has been slightly extended\n> to store a frame id and a ControlList for incoming frame\n> controls (sent in by the application). The next step would be to\n> read and set these controls whenever the request is actually queued\n> to the hardware.\n> \n> Since now we are working in multiples of IPAFrameContext, the\n> Algorithm::process() will actually take in a IPAFrameContext pointer\n> (as opposed to a nullptr while preparing for this change).\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------\n> src/ipa/ipu3/algorithms/agc.h | 4 ++--\n> src/ipa/ipu3/ipa_context.cpp | 24 ++++++++++++++++++++++--\n> src/ipa/ipu3/ipa_context.h | 14 +++++++++++++-\n> src/ipa/ipu3/ipu3.cpp | 22 +++++++++++++---------\n> 5 files changed, 55 insertions(+), 20 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 383a8deb..f16be534 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -183,14 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> * \\param[in] yGain The gain calculated based on the relative luminance target\n> * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> */\n> -void Agc::computeExposure(IPAContext &context, double yGain,\n> -\t\t\t double iqMeanGain)\n> +void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,\n> +\t\t\t double yGain, double iqMeanGain)\n> {\n> \tconst IPASessionConfiguration &configuration = context.configuration;\n> -\tIPAFrameContext &frameContext = context.frameContext;\n> \t/* Get the effective exposure and gain applied on the sensor. */\n> -\tuint32_t exposure = frameContext.sensor.exposure;\n> -\tdouble analogueGain = frameContext.sensor.gain;\n> +\tuint32_t exposure = frameContext->sensor.exposure;\n> +\tdouble analogueGain = frameContext->sensor.gain;\n> \n> \t/* Use the highest of the two gain estimates. */\n> \tdouble evGain = std::max(yGain, iqMeanGain);\n> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo\n> \t\t\tbreak;\n> \t}\n> \n> -\tcomputeExposure(context, yGain, iqMeanGain);\n> +\tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> }\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 219a1a96..105ae0f2 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -35,8 +35,8 @@ private:\n> \tdouble measureBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\t\t const ipu3_uapi_grid_config &grid) const;\n> \tutils::Duration filterExposure(utils::Duration currentExposure);\n> -\tvoid computeExposure(IPAContext &context, double yGain,\n> -\t\t\t double iqMeanGain);\n> +\tvoid computeExposure(IPAContext &context, IPAFrameContext *frameContext,\n> +\t\t\t double yGain, double iqMeanGain);\n> \tdouble estimateLuminance(IPAActiveState &activeState,\n> \t\t\t\t const ipu3_uapi_grid_config &grid,\n> \t\t\t\t const ipu3_uapi_stats_3a *stats,\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 06eb2776..81b30600 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {\n> * \\var IPAContext::configuration\n> * \\brief The IPA session configuration, immutable during the session\n> *\n> - * \\var IPAContext::frameContext\n> - * \\brief The frame context for the frame being processed\n> + * \\var IPAContext::frameContexts\n> + * \\brief Ring buffer of the IPAFrameContext(s)\n> *\n> * \\var IPAContext::activeState\n> * \\brief The current state of IPA algorithms\n> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {\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 8d681131..42e11141 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -8,16 +8,22 @@\n> \n> #pragma once\n> \n> +#include \n> +\n> #include \n> \n> #include \n> \n> +#include \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> @@ -71,17 +77,23 @@ struct IPAActiveState {\n> };\n> \n> struct IPAFrameContext {\n> +\tIPAFrameContext();\n> +\tIPAFrameContext(uint32_t id, const ControlList &reqControls);\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> -\tIPAFrameContext frameContext;\n> +\tstd::array frameContexts;\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 16e5028f..4322f96b 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> \t}\n> \n> \t/* Clean context */\n> -\tcontext_ = {};\n> +\tcontext_.configuration = {};\n> \tcontext_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n> \n> \t/* Construct our Algorithms */\n> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> \n> \t/* Clean IPAActiveState at each reconfiguration. */\n> \tcontext_.activeState = {};\n> -\tcontext_.frameContext = {};\n> +\tIPAFrameContext initFrameContext;\n> +\tcontext_.frameContexts.fill(initFrameContext);\n> \n> \tif (!validateSensorControls()) {\n> \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \tconst ipu3_uapi_stats_3a *stats =\n> \t\treinterpret_cast(mem.data());\n> \n> -\tcontext_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> -\tcontext_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> +\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n\nShouldn't the frame be \"double-checked\", verifying that \n(frameContext->frame == frame) before passing it to the algorithms somehow ?\n\nApart from that:\nReviewed-by: Jean-Michel Hautbois \n\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> \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_, nullptr, stats);\n> +\t\talgo->process(context_, &frameContext, stats);\n> \n> \tsetControls(frame);\n> \n> @@ -584,11 +587,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, context_.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, context_.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> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> * Parse the request to handle any IPA-managed controls that were set from the\n> * application such as manual sensor settings.\n> */\n> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,\n> -\t\t\t [[maybe_unused]] const ControlList &controls)\n> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> +\tIPAFrameContext newFrameContext(frame, controls);\n> +\tcontext_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;\n> }\n> \n> /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AD791C0F2A\n\tfor ;\n\tWed, 18 May 2022 09:08:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75AC36565C;\n\tWed, 18 May 2022 11:08:35 +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 3501065655\n\tfor ;\n\tWed, 18 May 2022 11:08:34 +0200 (CEST)","from [192.168.106.15] (unknown [37.171.4.20])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D75C48F;\n\tWed, 18 May 2022 11:08:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652864915;\n\tbh=DJ9pymm9ox78DxY6CmUci3bSs/T+3GRZKmdzO74qNCs=;\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=WhcpImhHqWp9PTcCe9ASzrS3A33njRbg6KaqH3gJKJI5IMmGfmsfBoQDw8gfTbCyD\n\tipBPleDbcwYMCmNMJJlJhuND9D7KvEB9e+/Ml5yDictDrgYj3v1ALx8Ukn0jGI8E7Q\n\tUF/kFjdjPmvWWJiH5vRZvVmnhet/AgIVbspBFGwTTyJJhBh5DTFfObJGV3jf2A6rCI\n\tFZGI0alXGW3JBBYPu0xKhtX4ONIGVT67ANaF9B+TCyYPc/pJc8aL8M/zr7QXp22DK4\n\twuLw+7rugDt4KjWlttQSdLPX+O+XO1vm4IOmc3aKV0CfjmYSnDD4VP3QGxn4eYNgwH\n\tKBQQsUXVNr8Vg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652864913;\n\tbh=DJ9pymm9ox78DxY6CmUci3bSs/T+3GRZKmdzO74qNCs=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=d4+giLqImv3C5OkQNBD7TQ6xnyhRL0OBvZayFymEnLV3lWveiT6Yj7aDuw9zAiE3V\n\t1eFA17Kric/D5s0QZUYUWB+Qd/GAmeJgthpjuKNcJOKppVmTynQXK5BdbXydBcUVSa\n\t98PoOE3/M4FHNYC83YSY7jWAnar0IriZQLye/smQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"d4+giLqI\"; dkim-atps=neutral","Message-ID":"","Date":"Wed, 18 May 2022 11:08:29 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.8.1","Content-Language":"en-US","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220517191833.333122-1-umang.jain@ideasonboard.com>\n\t<20220517191833.333122-4-umang.jain@ideasonboard.com>","In-Reply-To":"<20220517191833.333122-4-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jean-Michel Hautbois via libcamera-devel\n\t","Reply-To":"Jean-Michel Hautbois ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23038,"web_url":"https://patchwork.libcamera.org/comment/23038/","msgid":"<7ed4f7b5-a3ac-2c4b-a878-c704757249d7@ideasonboard.com>","date":"2022-05-18T09:39:11","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM,\n\nOn 5/18/22 11:08, Jean-Michel Hautbois wrote:\n> Hi Umang,\n>\n> On 17/05/2022 21:18, Umang Jain via libcamera-devel wrote:\n>> Instead of having one frame context constantly being updated,\n>> this patch aims to introduce per-frame IPAFrameContext which\n>> are stored in a ring buffer. Whenever a request is queued, a new\n>> IPAFrameContext is created and inserted into the ring buffer.\n>>\n>> The IPAFrameContext structure itself has been slightly extended\n>> to store a frame id and a ControlList for incoming frame\n>> controls (sent in by the application). The next step would be to\n>> read and set these controls whenever the request is actually queued\n>> to the hardware.\n>>\n>> Since now we are working in multiples of IPAFrameContext, the\n>> Algorithm::process() will actually take in a IPAFrameContext pointer\n>> (as opposed to a nullptr while preparing for this change).\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>>   src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------\n>>   src/ipa/ipu3/algorithms/agc.h   |  4 ++--\n>>   src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--\n>>   src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-\n>>   src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------\n>>   5 files changed, 55 insertions(+), 20 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp \n>> b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 383a8deb..f16be534 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -183,14 +183,13 @@ utils::Duration \n>> Agc::filterExposure(utils::Duration exposureValue)\n>>    * \\param[in] yGain The gain calculated based on the relative \n>> luminance target\n>>    * \\param[in] iqMeanGain The gain calculated based on the relative \n>> luminance target\n>>    */\n>> -void Agc::computeExposure(IPAContext &context, double yGain,\n>> -              double iqMeanGain)\n>> +void Agc::computeExposure(IPAContext &context, IPAFrameContext \n>> *frameContext,\n>> +              double yGain, double iqMeanGain)\n>>   {\n>>       const IPASessionConfiguration &configuration = \n>> context.configuration;\n>> -    IPAFrameContext &frameContext = context.frameContext;\n>>       /* Get the effective exposure and gain applied on the sensor. */\n>> -    uint32_t exposure = frameContext.sensor.exposure;\n>> -    double analogueGain = frameContext.sensor.gain;\n>> +    uint32_t exposure = frameContext->sensor.exposure;\n>> +    double analogueGain = frameContext->sensor.gain;\n>>         /* Use the highest of the two gain estimates. */\n>>       double evGain = std::max(yGain, iqMeanGain);\n>> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, \n>> [[maybe_unused]] IPAFrameContext *frameCo\n>>               break;\n>>       }\n>>   -    computeExposure(context, yGain, iqMeanGain);\n>> +    computeExposure(context, frameContext, yGain, iqMeanGain);\n>>       frameCount_++;\n>>   }\n>>   diff --git a/src/ipa/ipu3/algorithms/agc.h \n>> b/src/ipa/ipu3/algorithms/agc.h\n>> index 219a1a96..105ae0f2 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -35,8 +35,8 @@ private:\n>>       double measureBrightness(const ipu3_uapi_stats_3a *stats,\n>>                    const ipu3_uapi_grid_config &grid) const;\n>>       utils::Duration filterExposure(utils::Duration currentExposure);\n>> -    void computeExposure(IPAContext &context, double yGain,\n>> -                 double iqMeanGain);\n>> +    void computeExposure(IPAContext &context, IPAFrameContext \n>> *frameContext,\n>> +                 double yGain, double iqMeanGain);\n>>       double estimateLuminance(IPAActiveState &activeState,\n>>                    const ipu3_uapi_grid_config &grid,\n>>                    const ipu3_uapi_stats_3a *stats,\n>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>> index 06eb2776..81b30600 100644\n>> --- a/src/ipa/ipu3/ipa_context.cpp\n>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {\n>>    * \\var IPAContext::configuration\n>>    * \\brief The IPA session configuration, immutable during the session\n>>    *\n>> - * \\var IPAContext::frameContext\n>> - * \\brief The frame context for the frame being processed\n>> + * \\var IPAContext::frameContexts\n>> + * \\brief Ring buffer of the IPAFrameContext(s)\n>>    *\n>>    * \\var IPAContext::activeState\n>>    * \\brief The current state of IPA algorithms\n>> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {\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 \n>> &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 8d681131..42e11141 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -8,16 +8,22 @@\n>>     #pragma once\n>>   +#include \n>> +\n>>   #include \n>>     #include \n>>   +#include \n>>   #include \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>>       struct {\n>>           ipu3_uapi_grid_config bdsGrid;\n>> @@ -71,17 +77,23 @@ struct IPAActiveState {\n>>   };\n>>     struct IPAFrameContext {\n>> +    IPAFrameContext();\n>> +    IPAFrameContext(uint32_t id, const ControlList &reqControls);\n>> +\n>>       struct {\n>>           uint32_t exposure;\n>>           double gain;\n>>       } sensor;\n>> +\n>> +    uint32_t frame;\n>> +    ControlList frameControls;\n>>   };\n>>     struct IPAContext {\n>>       IPASessionConfiguration configuration;\n>>       IPAActiveState activeState;\n>>   -    IPAFrameContext frameContext;\n>> +    std::array frameContexts;\n>>   };\n>>     } /* namespace ipa::ipu3 */\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 16e5028f..4322f96b 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,\n>>       }\n>>         /* Clean context */\n>> -    context_ = {};\n>> +    context_.configuration = {};\n>>       context_.configuration.sensor.lineDuration = \n>> sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n>>         /* Construct our Algorithms */\n>> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo \n>> &configInfo,\n>>         /* Clean IPAActiveState at each reconfiguration. */\n>>       context_.activeState = {};\n>> -    context_.frameContext = {};\n>> +    IPAFrameContext initFrameContext;\n>> +    context_.frameContexts.fill(initFrameContext);\n>>         if (!validateSensorControls()) {\n>>           LOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n>> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t \n>> frame,\n>>       const ipu3_uapi_stats_3a *stats =\n>>           reinterpret_cast(mem.data());\n>>   -    context_.frameContext.sensor.exposure = \n>> sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> -    context_.frameContext.sensor.gain = \n>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>> +    IPAFrameContext &frameContext = context_.frameContexts[frame % \n>> kMaxFrameContexts];\n>\n> Shouldn't the frame be \"double-checked\", verifying that \n> (frameContext->frame == frame) before passing it to the algorithms \n> somehow ?\n\n\nWell, we can surely verify that but we know in advance that the \nverification is subjected to failure with known cases (for e.g. frame \ndrop) so I am not very keen putting the verification in. If you had seen \nthe v2, I had a ASSERT similar to this basically (and has some \ndiscussion on this there)\n\n>\n> Apart from that:\n> Reviewed-by: Jean-Michel Hautbois \n\n\nThanks for reviewing!\n\n>\n>> +\n>> +    frameContext.sensor.exposure = \n>> sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> +    frameContext.sensor.gain = \n>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>>         double lineDuration = \n>> context_.configuration.sensor.lineDuration.get();\n>>       int32_t vBlank = context_.configuration.sensor.defVBlank;\n>>       ControlList ctrls(controls::controls);\n>>         for (auto const &algo : algorithms_)\n>> -        algo->process(context_, nullptr, stats);\n>> +        algo->process(context_, &frameContext, stats);\n>>         setControls(frame);\n>>   @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const \n>> uint32_t frame,\n>>       int64_t frameDuration = (vBlank + \n>> sensorInfo_.outputSize.height) * lineDuration;\n>>       ctrls.set(controls::FrameDuration, frameDuration);\n>>   -    ctrls.set(controls::AnalogueGain, \n>> context_.frameContext.sensor.gain);\n>> +    ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);\n>>         ctrls.set(controls::ColourTemperature, \n>> context_.activeState.awb.temperatureK);\n>>   -    ctrls.set(controls::ExposureTime, \n>> context_.frameContext.sensor.exposure * lineDuration);\n>> +    ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * \n>> lineDuration);\n>>         /*\n>>        * \\todo The Metadata provides a path to getting extended data\n>> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t \n>> frame,\n>>    * Parse the request to handle any IPA-managed controls that were \n>> set from the\n>>    * application such as manual sensor settings.\n>>    */\n>> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,\n>> -               [[maybe_unused]] const ControlList &controls)\n>> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList \n>> &controls)\n>>   {\n>>       /* \\todo Start processing for 'frame' based on 'controls'. */\n>> +    IPAFrameContext newFrameContext(frame, controls);\n>> +    context_.frameContexts[frame % kMaxFrameContexts] = \n>> newFrameContext;\n>>   }\n>>     /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 31AB5C0F2A\n\tfor ;\n\tWed, 18 May 2022 09:39:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EBC7961FB9;\n\tWed, 18 May 2022 11:39:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7809F61FB9\n\tfor ;\n\tWed, 18 May 2022 11:39:14 +0200 (CEST)","from [192.168.1.164] (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E3FA0475;\n\tWed, 18 May 2022 11:39:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652866756;\n\tbh=4mVR2aQOtOEFIF4VJPGJDH7G888qt9V8HZ4s4h2+I3M=;\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=awj9au459JDp0ZG4Ykk9OO1ZCsjIu44Wbbj2uEtI7oz4wX/RQABR93BguQd3MC1sZ\n\tVIIzCvAXwdz0YuVTQKbJEwaZMFKC1gJY1ewWxG+PG4QQsldGzlTjDJXwpCvRPQU9EH\n\t0EJU/SG1MNdOkONuct0Vap0W+Ep7e5dSXjW2cpddmoHYuGmsRVeiMThiD1P/128ay8\n\twz7rDUhYhC2oTbld1/HxIFNjaEiGc2bNgrxmWu6cgmaCjPuB9CCaotbzmj5KUn4ZG2\n\tk0Kpb7R96dWcFeQRs7UiEKOQbft+DuaRMFFqY1Cs/Oma3Uc1cWOQEEdafzM8WSKBX+\n\tzEAnTQZ7Iidtg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652866754;\n\tbh=4mVR2aQOtOEFIF4VJPGJDH7G888qt9V8HZ4s4h2+I3M=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=jtsZa6goXDCtBr+sU+9+HHFfvr/aCeFe2CTmJLzuWvKMobGNWMcZcHajAv1m2VtND\n\tIVO6bz2C++X4CiNj67+Iez043q/GQBW5monl9Pt+fckqMjl1sPvy7jmzVLejXBkhkV\n\t737xiL4Msh6Z9Wc3FY8xNUfxzWapN4Psc1R1ahKk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jtsZa6go\"; dkim-atps=neutral","Message-ID":"<7ed4f7b5-a3ac-2c4b-a878-c704757249d7@ideasonboard.com>","Date":"Wed, 18 May 2022 11:39:11 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220517191833.333122-1-umang.jain@ideasonboard.com>\n\t<20220517191833.333122-4-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","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":23041,"web_url":"https://patchwork.libcamera.org/comment/23041/","msgid":"","date":"2022-05-18T10:34:15","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for reviewing.\n\nOn 5/18/22 09:47, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Tue, May 17, 2022 at 09:18:33PM +0200, Umang Jain via libcamera-devel wrote:\n>> Instead of having one frame context constantly being updated,\n>> this patch aims to introduce per-frame IPAFrameContext which\n>> are stored in a ring buffer. Whenever a request is queued, a new\n>> IPAFrameContext is created and inserted into the ring buffer.\n>>\n>> The IPAFrameContext structure itself has been slightly extended\n>> to store a frame id and a ControlList for incoming frame\n>> controls (sent in by the application). The next step would be to\n>> read and set these controls whenever the request is actually queued\n>> to the hardware.\n>>\n>> Since now we are working in multiples of IPAFrameContext, the\n>> Algorithm::process() will actually take in a IPAFrameContext pointer\n>> (as opposed to a nullptr while preparing for this change).\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------\n>> src/ipa/ipu3/algorithms/agc.h | 4 ++--\n>> src/ipa/ipu3/ipa_context.cpp | 24 ++++++++++++++++++++++--\n>> src/ipa/ipu3/ipa_context.h | 14 +++++++++++++-\n>> src/ipa/ipu3/ipu3.cpp | 22 +++++++++++++---------\n>> 5 files changed, 55 insertions(+), 20 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 383a8deb..f16be534 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -183,14 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n>> * \\param[in] yGain The gain calculated based on the relative luminance target\n>> * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n>> */\n>> -void Agc::computeExposure(IPAContext &context, double yGain,\n>> -\t\t\t double iqMeanGain)\n>> +void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,\n>> +\t\t\t double yGain, double iqMeanGain)\n>> {\n>> \tconst IPASessionConfiguration &configuration = context.configuration;\n>> -\tIPAFrameContext &frameContext = context.frameContext;\n>> \t/* Get the effective exposure and gain applied on the sensor. */\n>> -\tuint32_t exposure = frameContext.sensor.exposure;\n>> -\tdouble analogueGain = frameContext.sensor.gain;\n>> +\tuint32_t exposure = frameContext->sensor.exposure;\n>> +\tdouble analogueGain = frameContext->sensor.gain;\n>>\n>> \t/* Use the highest of the two gain estimates. */\n>> \tdouble evGain = std::max(yGain, iqMeanGain);\n>> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo\n>> \t\t\tbreak;\n>> \t}\n>>\n>> -\tcomputeExposure(context, yGain, iqMeanGain);\n>> +\tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n>> \tframeCount_++;\n>> }\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index 219a1a96..105ae0f2 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -35,8 +35,8 @@ private:\n>> \tdouble measureBrightness(const ipu3_uapi_stats_3a *stats,\n>> \t\t\t\t const ipu3_uapi_grid_config &grid) const;\n>> \tutils::Duration filterExposure(utils::Duration currentExposure);\n>> -\tvoid computeExposure(IPAContext &context, double yGain,\n>> -\t\t\t double iqMeanGain);\n>> +\tvoid computeExposure(IPAContext &context, IPAFrameContext *frameContext,\n>> +\t\t\t double yGain, double iqMeanGain);\n>> \tdouble estimateLuminance(IPAActiveState &activeState,\n>> \t\t\t\t const ipu3_uapi_grid_config &grid,\n>> \t\t\t\t const ipu3_uapi_stats_3a *stats,\n>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>> index 06eb2776..81b30600 100644\n>> --- a/src/ipa/ipu3/ipa_context.cpp\n>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {\n>> * \\var IPAContext::configuration\n>> * \\brief The IPA session configuration, immutable during the session\n>> *\n>> - * \\var IPAContext::frameContext\n>> - * \\brief The frame context for the frame being processed\n>> + * \\var IPAContext::frameContexts\n>> + * \\brief Ring buffer of the IPAFrameContext(s)\n>> *\n>> * \\var IPAContext::activeState\n>> * \\brief The current state of IPA algorithms\n>> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {\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 8d681131..42e11141 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -8,16 +8,22 @@\n>>\n>> #pragma once\n>>\n>> +#include \n>> +\n>> #include \n>>\n>> #include \n>>\n>> +#include \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>> @@ -71,17 +77,23 @@ struct IPAActiveState {\n>> };\n>>\n>> struct IPAFrameContext {\n>> +\tIPAFrameContext();\n>> +\tIPAFrameContext(uint32_t id, const ControlList &reqControls);\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>> -\tIPAFrameContext frameContext;\n>> +\tstd::array frameContexts;\n>> };\n>>\n>> } /* namespace ipa::ipu3 */\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 16e5028f..4322f96b 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,\n>> \t}\n>>\n>> \t/* Clean context */\n>> -\tcontext_ = {};\n>> +\tcontext_.configuration = {};\n>> \tcontext_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n>>\n>> \t/* Construct our Algorithms */\n>> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>>\n>> \t/* Clean IPAActiveState at each reconfiguration. */\n>> \tcontext_.activeState = {};\n>> -\tcontext_.frameContext = {};\n>> +\tIPAFrameContext initFrameContext;\n>> +\tcontext_.frameContexts.fill(initFrameContext);\n>>\n>> \tif (!validateSensorControls()) {\n>> \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n>> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n>> \tconst ipu3_uapi_stats_3a *stats =\n>> \t\treinterpret_cast(mem.data());\n>>\n>> -\tcontext_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> -\tcontext_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>> +\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\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>>\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_, nullptr, stats);\n>> +\t\talgo->process(context_, &frameContext, stats);\n>>\n>> \tsetControls(frame);\n>>\n>> @@ -584,11 +587,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, context_.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, context_.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>> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n>> * Parse the request to handle any IPA-managed controls that were set from the\n>> * application such as manual sensor settings.\n>> */\n>> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,\n>> -\t\t\t [[maybe_unused]] const ControlList &controls)\n>> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n>> {\n>> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n>> +\tIPAFrameContext newFrameContext(frame, controls);\n>> +\tcontext_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;\n> Just a little note, which now is not that relevant as we have a\n> limited number of controls, but this goes through at least 2 copies a\n> the 'controls' list.\n>\n> Constructor:\n>\n> IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> \t: frame(id), frameControls(reqControls)\n>\n> And the implicitely defined copy assignment operator.\n>\n> I wonder if\n> \tcontext_.frameContexts[frame % kMaxFrameContexts] = {frame, controls};\n>\n> would spare a copy\n\n\nI am also wondering. I think your version is still doing two copies as \nper my understanding.\n\nOne would be through the constructor as you said above.\n\nThe other one would be while assigning `context_.frameContexts[frame % \nkMaxFrameContexts]` as operator= would do a member-wise copy, does my \ninference make sense?\n\nBut nevertheless,\nYour version is a more clear to read hence I would keep this anyway.\n\n>\n> The rest looks good to me\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n>\n>> }\n>>\n>> /**\n>> --\n>> 2.31.0\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 5936FC3256\n\tfor ;\n\tWed, 18 May 2022 10:34:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE41365659;\n\tWed, 18 May 2022 12:34:19 +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 94B2C65656\n\tfor ;\n\tWed, 18 May 2022 12:34:18 +0200 (CEST)","from [192.168.1.164] (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A68C9F7;\n\tWed, 18 May 2022 12:34:17 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652870059;\n\tbh=KnNaOz+GViDXDSCstrILy3Cki+gnUHH0QQM0zEhxzy0=;\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=cIsl3EZ+/NpjRvM0Mi1d68xi5qKPpjx5tAMDoyn5WhXiirIh2TICPgSBL7ydiP14/\n\t6lrwNT5ZTDmb8l6qcdHlilrfFe4vWsE9llth1PFteGzfNvcjZOmu6/MnDE6RdBGv8D\n\ta1OSz76Msu2Aw19cOgwb3X4k6vPDZSSaCAIKNZintj298xjaASjok9jwInRuhujIfI\n\tMk6Egn9FMxrUlJu8+B9kxBE5l7cNtEUN60lTK5cRzg/Dry3k3G1mnvWxCAKD2l0v1d\n\t0aT7jROQsOw9xFSYEHpj9DsR0OA3Iuc69qLROzEU3oCwh9rA9hVReELCBhG1AWyVab\n\tTqsUjCDXtESzw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652870058;\n\tbh=KnNaOz+GViDXDSCstrILy3Cki+gnUHH0QQM0zEhxzy0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=rWm0cm9tAPo/SakqSEKOAhS82NBO/XH9BTw027lni4/MCbFW1K57A543wfjAf73RB\n\ts4KFaXJ+DurwxQSN/5eAvmpHeLTPkMoxUWO2OdksE3/UMrgLkLe+bsGcStPF/XdQw1\n\t4YumZ2UYybV77Y8KUDY5MK6bL2U+vmxMPoM7caE8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rWm0cm9t\"; dkim-atps=neutral","Message-ID":"","Date":"Wed, 18 May 2022 12:34:15 +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":"<20220517191833.333122-1-umang.jain@ideasonboard.com>\n\t<20220517191833.333122-4-umang.jain@ideasonboard.com>\n\t<20220518074726.k47zfauj7e6stbng@uno.localdomain>","In-Reply-To":"<20220518074726.k47zfauj7e6stbng@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","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":23043,"web_url":"https://patchwork.libcamera.org/comment/23043/","msgid":"<165287064942.368702.14864609516246569168@Monstersaurus>","date":"2022-05-18T10:44:09","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain via libcamera-devel (2022-05-18 10:39:11)\n> Hi JM,\n> \n> On 5/18/22 11:08, Jean-Michel Hautbois wrote:\n> > Hi Umang,\n> >\n> > On 17/05/2022 21:18, Umang Jain via libcamera-devel wrote:\n> >> Instead of having one frame context constantly being updated,\n> >> this patch aims to introduce per-frame IPAFrameContext which\n> >> are stored in a ring buffer. Whenever a request is queued, a new\n> >> IPAFrameContext is created and inserted into the ring buffer.\n> >>\n> >> The IPAFrameContext structure itself has been slightly extended\n> >> to store a frame id and a ControlList for incoming frame\n> >> controls (sent in by the application). The next step would be to\n> >> read and set these controls whenever the request is actually queued\n> >> to the hardware.\n> >>\n> >> Since now we are working in multiples of IPAFrameContext, the\n> >> Algorithm::process() will actually take in a IPAFrameContext pointer\n> >> (as opposed to a nullptr while preparing for this change).\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >>   src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------\n> >>   src/ipa/ipu3/algorithms/agc.h   |  4 ++--\n> >>   src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--\n> >>   src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-\n> >>   src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------\n> >>   5 files changed, 55 insertions(+), 20 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp \n> >> b/src/ipa/ipu3/algorithms/agc.cpp\n> >> index 383a8deb..f16be534 100644\n> >> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> >> @@ -183,14 +183,13 @@ utils::Duration \n> >> Agc::filterExposure(utils::Duration exposureValue)\n> >>    * \\param[in] yGain The gain calculated based on the relative \n> >> luminance target\n> >>    * \\param[in] iqMeanGain The gain calculated based on the relative \n> >> luminance target\n> >>    */\n> >> -void Agc::computeExposure(IPAContext &context, double yGain,\n> >> -              double iqMeanGain)\n> >> +void Agc::computeExposure(IPAContext &context, IPAFrameContext \n> >> *frameContext,\n> >> +              double yGain, double iqMeanGain)\n> >>   {\n> >>       const IPASessionConfiguration &configuration = \n> >> context.configuration;\n> >> -    IPAFrameContext &frameContext = context.frameContext;\n> >>       /* Get the effective exposure and gain applied on the sensor. */\n> >> -    uint32_t exposure = frameContext.sensor.exposure;\n> >> -    double analogueGain = frameContext.sensor.gain;\n> >> +    uint32_t exposure = frameContext->sensor.exposure;\n> >> +    double analogueGain = frameContext->sensor.gain;\n> >>         /* Use the highest of the two gain estimates. */\n> >>       double evGain = std::max(yGain, iqMeanGain);\n> >> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, \n> >> [[maybe_unused]] IPAFrameContext *frameCo\n> >>               break;\n> >>       }\n> >>   -    computeExposure(context, yGain, iqMeanGain);\n> >> +    computeExposure(context, frameContext, yGain, iqMeanGain);\n> >>       frameCount_++;\n> >>   }\n> >>   diff --git a/src/ipa/ipu3/algorithms/agc.h \n> >> b/src/ipa/ipu3/algorithms/agc.h\n> >> index 219a1a96..105ae0f2 100644\n> >> --- a/src/ipa/ipu3/algorithms/agc.h\n> >> +++ b/src/ipa/ipu3/algorithms/agc.h\n> >> @@ -35,8 +35,8 @@ private:\n> >>       double measureBrightness(const ipu3_uapi_stats_3a *stats,\n> >>                    const ipu3_uapi_grid_config &grid) const;\n> >>       utils::Duration filterExposure(utils::Duration currentExposure);\n> >> -    void computeExposure(IPAContext &context, double yGain,\n> >> -                 double iqMeanGain);\n> >> +    void computeExposure(IPAContext &context, IPAFrameContext \n> >> *frameContext,\n> >> +                 double yGain, double iqMeanGain);\n> >>       double estimateLuminance(IPAActiveState &activeState,\n> >>                    const ipu3_uapi_grid_config &grid,\n> >>                    const ipu3_uapi_stats_3a *stats,\n> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> >> index 06eb2776..81b30600 100644\n> >> --- a/src/ipa/ipu3/ipa_context.cpp\n> >> +++ b/src/ipa/ipu3/ipa_context.cpp\n> >> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {\n> >>    * \\var IPAContext::configuration\n> >>    * \\brief The IPA session configuration, immutable during the session\n> >>    *\n> >> - * \\var IPAContext::frameContext\n> >> - * \\brief The frame context for the frame being processed\n> >> + * \\var IPAContext::frameContexts\n> >> + * \\brief Ring buffer of the IPAFrameContext(s)\n> >>    *\n> >>    * \\var IPAContext::activeState\n> >>    * \\brief The current state of IPA algorithms\n> >> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {\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 \n> >> &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 8d681131..42e11141 100644\n> >> --- a/src/ipa/ipu3/ipa_context.h\n> >> +++ b/src/ipa/ipu3/ipa_context.h\n> >> @@ -8,16 +8,22 @@\n> >>     #pragma once\n> >>   +#include \n> >> +\n> >>   #include \n> >>     #include \n> >>   +#include \n> >>   #include \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> >>       struct {\n> >>           ipu3_uapi_grid_config bdsGrid;\n> >> @@ -71,17 +77,23 @@ struct IPAActiveState {\n> >>   };\n> >>     struct IPAFrameContext {\n> >> +    IPAFrameContext();\n> >> +    IPAFrameContext(uint32_t id, const ControlList &reqControls);\n> >> +\n> >>       struct {\n> >>           uint32_t exposure;\n> >>           double gain;\n> >>       } sensor;\n> >> +\n> >> +    uint32_t frame;\n> >> +    ControlList frameControls;\n> >>   };\n> >>     struct IPAContext {\n> >>       IPASessionConfiguration configuration;\n> >>       IPAActiveState activeState;\n> >>   -    IPAFrameContext frameContext;\n> >> +    std::array frameContexts;\n> >>   };\n> >>     } /* namespace ipa::ipu3 */\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index 16e5028f..4322f96b 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> >>       }\n> >>         /* Clean context */\n> >> -    context_ = {};\n> >> +    context_.configuration = {};\n> >>       context_.configuration.sensor.lineDuration = \n> >> sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n> >>         /* Construct our Algorithms */\n> >> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo \n> >> &configInfo,\n> >>         /* Clean IPAActiveState at each reconfiguration. */\n> >>       context_.activeState = {};\n> >> -    context_.frameContext = {};\n> >> +    IPAFrameContext initFrameContext;\n> >> +    context_.frameContexts.fill(initFrameContext);\n> >>         if (!validateSensorControls()) {\n> >>           LOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> >> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t \n> >> frame,\n> >>       const ipu3_uapi_stats_3a *stats =\n> >>           reinterpret_cast(mem.data());\n> >>   -    context_.frameContext.sensor.exposure = \n> >> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >> -    context_.frameContext.sensor.gain = \n> >> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> >> +    IPAFrameContext &frameContext = context_.frameContexts[frame % \n> >> kMaxFrameContexts];\n> >\n> > Shouldn't the frame be \"double-checked\", verifying that \n> > (frameContext->frame == frame) before passing it to the algorithms \n> > somehow ?\n> \n> \n> Well, we can surely verify that but we know in advance that the \n> verification is subjected to failure with known cases (for e.g. frame \n> drop) so I am not very keen putting the verification in. If you had seen \n> the v2, I had a ASSERT similar to this basically (and has some \n> discussion on this there)\n\nI do think some sort of check is warranted though, because the CIO2\nmight be free-running with no requests coming in - so I can envisage we\ncould easily end up overflowing the ring buffer in a (bad) scenario - so\nwe should at least report that.\n\nI would expect that in that scenario the CIO2 will run but the IMGU\nmight not - so we wouldn't get any statistics for that frame.\n\nIt's something to think about later anyway ... but a check/print would\nbe beneficial at least. For now it could still operate anyway, but with\na warning print.\n\n\nReviewed-by: Kieran Bingham \n\n> \n> >\n> > Apart from that:\n> > Reviewed-by: Jean-Michel Hautbois \n> \n> \n> Thanks for reviewing!\n> \n> >\n> >> +\n> >> +    frameContext.sensor.exposure = \n> >> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >> +    frameContext.sensor.gain = \n> >> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> >>         double lineDuration = \n> >> context_.configuration.sensor.lineDuration.get();\n> >>       int32_t vBlank = context_.configuration.sensor.defVBlank;\n> >>       ControlList ctrls(controls::controls);\n> >>         for (auto const &algo : algorithms_)\n> >> -        algo->process(context_, nullptr, stats);\n> >> +        algo->process(context_, &frameContext, stats);\n> >>         setControls(frame);\n> >>   @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const \n> >> uint32_t frame,\n> >>       int64_t frameDuration = (vBlank + \n> >> sensorInfo_.outputSize.height) * lineDuration;\n> >>       ctrls.set(controls::FrameDuration, frameDuration);\n> >>   -    ctrls.set(controls::AnalogueGain, \n> >> context_.frameContext.sensor.gain);\n> >> +    ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);\n> >>         ctrls.set(controls::ColourTemperature, \n> >> context_.activeState.awb.temperatureK);\n> >>   -    ctrls.set(controls::ExposureTime, \n> >> context_.frameContext.sensor.exposure * lineDuration);\n> >> +    ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * \n> >> lineDuration);\n> >>         /*\n> >>        * \\todo The Metadata provides a path to getting extended data\n> >> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t \n> >> frame,\n> >>    * Parse the request to handle any IPA-managed controls that were \n> >> set from the\n> >>    * application such as manual sensor settings.\n> >>    */\n> >> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,\n> >> -               [[maybe_unused]] const ControlList &controls)\n> >> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList \n> >> &controls)\n> >>   {\n> >>       /* \\todo Start processing for 'frame' based on 'controls'. */\n> >> +    IPAFrameContext newFrameContext(frame, controls);\n> >> +    context_.frameContexts[frame % kMaxFrameContexts] = \n> >> newFrameContext;\n> >>   }\n> >>     /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D76A0C3256\n\tfor ;\n\tWed, 18 May 2022 10:44:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0CB765659;\n\tWed, 18 May 2022 12:44:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D62A65656\n\tfor ;\n\tWed, 18 May 2022 12:44:12 +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 769B19C4;\n\tWed, 18 May 2022 12:44:11 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652870653;\n\tbh=glR0GTiqcraft9LkKoEkMjtsQj/cifCEgsqHoin+f3Y=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=KwJymtPOlnrWGC6UUAD1upaqpQbg8VOIbMR7eTHBTyLES2571rgMnVa8J6X/U1Gsi\n\tdlbZbVcnby6KoBQFQFNVo9LWMqMIJegl522MGKMWaI13AeQ1FXqjCPhHiZF/uyS8xh\n\tFBPiErZvd3jCTuFqynhvbp9SRHv/Hx1weipLQ/ywXQsbEgmCesvtkyrGJi6hD3ZOxo\n\t6dH5OGlf1qi1R3nu1ZXw2t8ML1nhRYS6tvhp1tIaaOjACe95ZNkTLb3hug1eOJEOQK\n\te/METM8LZ9Gb5GbyzdFw7TkQWA5wQ8IMES3iqtIA3oRoiWB0NN8vop+iK3uwm1XLhn\n\txbn8iIMVdYfhw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652870651;\n\tbh=glR0GTiqcraft9LkKoEkMjtsQj/cifCEgsqHoin+f3Y=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=LGcIuMGxRSyvLxLvHjOELpnq3iHljUNrh0g2ekGKu5NrC+E10nvUHX51s1ZmAG/jv\n\tAVL7nFXzdS9YMTJxhe2Bfw87xy+aKlxEQE5nafoB1Fy6Cx14xAYj4Cet7N2mCj7XMX\n\tPZxv4xkWDKQwOTHTUuDL6Ix7vlODQwRhvt+ZcReU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LGcIuMGx\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<7ed4f7b5-a3ac-2c4b-a878-c704757249d7@ideasonboard.com>","References":"<20220517191833.333122-1-umang.jain@ideasonboard.com>\n\t<20220517191833.333122-4-umang.jain@ideasonboard.com>\n\t\n\t<7ed4f7b5-a3ac-2c4b-a878-c704757249d7@ideasonboard.com>","To":"Jean-Michel Hautbois ,\n\tUmang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 18 May 2022 11:44:09 +0100","Message-ID":"<165287064942.368702.14864609516246569168@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23046,"web_url":"https://patchwork.libcamera.org/comment/23046/","msgid":"<20220518105905.3uaufctl7zgoygya@uno.localdomain>","date":"2022-05-18T10:59:05","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Wed, May 18, 2022 at 12:34:15PM +0200, Umang Jain wrote:\n> Hi Jacopo,\n>\n> Thanks for reviewing.\n>\n> On 5/18/22 09:47, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Tue, May 17, 2022 at 09:18:33PM +0200, Umang Jain via libcamera-devel wrote:\n> > > Instead of having one frame context constantly being updated,\n> > > this patch aims to introduce per-frame IPAFrameContext which\n> > > are stored in a ring buffer. Whenever a request is queued, a new\n> > > IPAFrameContext is created and inserted into the ring buffer.\n> > >\n> > > The IPAFrameContext structure itself has been slightly extended\n> > > to store a frame id and a ControlList for incoming frame\n> > > controls (sent in by the application). The next step would be to\n> > > read and set these controls whenever the request is actually queued\n> > > to the hardware.\n> > >\n> > > Since now we are working in multiples of IPAFrameContext, the\n> > > Algorithm::process() will actually take in a IPAFrameContext pointer\n> > > (as opposed to a nullptr while preparing for this change).\n> > >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------\n> > > src/ipa/ipu3/algorithms/agc.h | 4 ++--\n> > > src/ipa/ipu3/ipa_context.cpp | 24 ++++++++++++++++++++++--\n> > > src/ipa/ipu3/ipa_context.h | 14 +++++++++++++-\n> > > src/ipa/ipu3/ipu3.cpp | 22 +++++++++++++---------\n> > > 5 files changed, 55 insertions(+), 20 deletions(-)\n> > >\n> > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > > index 383a8deb..f16be534 100644\n> > > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > > @@ -183,14 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> > > * \\param[in] yGain The gain calculated based on the relative luminance target\n> > > * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> > > */\n> > > -void Agc::computeExposure(IPAContext &context, double yGain,\n> > > -\t\t\t double iqMeanGain)\n> > > +void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,\n> > > +\t\t\t double yGain, double iqMeanGain)\n> > > {\n> > > \tconst IPASessionConfiguration &configuration = context.configuration;\n> > > -\tIPAFrameContext &frameContext = context.frameContext;\n> > > \t/* Get the effective exposure and gain applied on the sensor. */\n> > > -\tuint32_t exposure = frameContext.sensor.exposure;\n> > > -\tdouble analogueGain = frameContext.sensor.gain;\n> > > +\tuint32_t exposure = frameContext->sensor.exposure;\n> > > +\tdouble analogueGain = frameContext->sensor.gain;\n> > >\n> > > \t/* Use the highest of the two gain estimates. */\n> > > \tdouble evGain = std::max(yGain, iqMeanGain);\n> > > @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo\n> > > \t\t\tbreak;\n> > > \t}\n> > >\n> > > -\tcomputeExposure(context, yGain, iqMeanGain);\n> > > +\tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> > > \tframeCount_++;\n> > > }\n> > >\n> > > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> > > index 219a1a96..105ae0f2 100644\n> > > --- a/src/ipa/ipu3/algorithms/agc.h\n> > > +++ b/src/ipa/ipu3/algorithms/agc.h\n> > > @@ -35,8 +35,8 @@ private:\n> > > \tdouble measureBrightness(const ipu3_uapi_stats_3a *stats,\n> > > \t\t\t\t const ipu3_uapi_grid_config &grid) const;\n> > > \tutils::Duration filterExposure(utils::Duration currentExposure);\n> > > -\tvoid computeExposure(IPAContext &context, double yGain,\n> > > -\t\t\t double iqMeanGain);\n> > > +\tvoid computeExposure(IPAContext &context, IPAFrameContext *frameContext,\n> > > +\t\t\t double yGain, double iqMeanGain);\n> > > \tdouble estimateLuminance(IPAActiveState &activeState,\n> > > \t\t\t\t const ipu3_uapi_grid_config &grid,\n> > > \t\t\t\t const ipu3_uapi_stats_3a *stats,\n> > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > > index 06eb2776..81b30600 100644\n> > > --- a/src/ipa/ipu3/ipa_context.cpp\n> > > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > > @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {\n> > > * \\var IPAContext::configuration\n> > > * \\brief The IPA session configuration, immutable during the session\n> > > *\n> > > - * \\var IPAContext::frameContext\n> > > - * \\brief The frame context for the frame being processed\n> > > + * \\var IPAContext::frameContexts\n> > > + * \\brief Ring buffer of the IPAFrameContext(s)\n> > > *\n> > > * \\var IPAContext::activeState\n> > > * \\brief The current state of IPA algorithms\n> > > @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {\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 8d681131..42e11141 100644\n> > > --- a/src/ipa/ipu3/ipa_context.h\n> > > +++ b/src/ipa/ipu3/ipa_context.h\n> > > @@ -8,16 +8,22 @@\n> > >\n> > > #pragma once\n> > >\n> > > +#include \n> > > +\n> > > #include \n> > >\n> > > #include \n> > >\n> > > +#include \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> > > @@ -71,17 +77,23 @@ struct IPAActiveState {\n> > > };\n> > >\n> > > struct IPAFrameContext {\n> > > +\tIPAFrameContext();\n> > > +\tIPAFrameContext(uint32_t id, const ControlList &reqControls);\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> > > -\tIPAFrameContext frameContext;\n> > > +\tstd::array frameContexts;\n> > > };\n> > >\n> > > } /* namespace ipa::ipu3 */\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 16e5028f..4322f96b 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> > > \t}\n> > >\n> > > \t/* Clean context */\n> > > -\tcontext_ = {};\n> > > +\tcontext_.configuration = {};\n> > > \tcontext_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n> > >\n> > > \t/* Construct our Algorithms */\n> > > @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> > >\n> > > \t/* Clean IPAActiveState at each reconfiguration. */\n> > > \tcontext_.activeState = {};\n> > > -\tcontext_.frameContext = {};\n> > > +\tIPAFrameContext initFrameContext;\n> > > +\tcontext_.frameContexts.fill(initFrameContext);\n> > >\n> > > \tif (!validateSensorControls()) {\n> > > \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> > > @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > > \tconst ipu3_uapi_stats_3a *stats =\n> > > \t\treinterpret_cast(mem.data());\n> > >\n> > > -\tcontext_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > -\tcontext_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > > +\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\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> > >\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_, nullptr, stats);\n> > > +\t\talgo->process(context_, &frameContext, stats);\n> > >\n> > > \tsetControls(frame);\n> > >\n> > > @@ -584,11 +587,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, context_.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, context_.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> > > @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > > * Parse the request to handle any IPA-managed controls that were set from the\n> > > * application such as manual sensor settings.\n> > > */\n> > > -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,\n> > > -\t\t\t [[maybe_unused]] const ControlList &controls)\n> > > +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > > {\n> > > \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> > > +\tIPAFrameContext newFrameContext(frame, controls);\n> > > +\tcontext_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;\n> > Just a little note, which now is not that relevant as we have a\n> > limited number of controls, but this goes through at least 2 copies a\n> > the 'controls' list.\n> >\n> > Constructor:\n> >\n> > IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)\n> > \t: frame(id), frameControls(reqControls)\n> >\n> > And the implicitely defined copy assignment operator.\n> >\n> > I wonder if\n> > \tcontext_.frameContexts[frame % kMaxFrameContexts] = {frame, controls};\n> >\n> > would spare a copy\n>\n>\n> I am also wondering. I think your version is still doing two copies as per\n> my understanding.\n>\n> One would be through the constructor as you said above.\n\nAh, {frame, controls} would construct an instance anyway, you're\nright.\n\nI assume that's a recurring problem, how do you re-initialize elements in\na container without going through their copy operators (which require\nan instance of the same type to be passed in as parameter) ?\n\n>\n> The other one would be while assigning `context_.frameContexts[frame %\n> kMaxFrameContexts]` as operator= would do a member-wise copy, does my\n> inference make sense?\n>\n> But nevertheless,\n> Your version is a more clear to read hence I would keep this anyway.\n>\n> >\n> > The rest looks good to me\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> >\n> > > }\n> > >\n> > > /**\n> > > --\n> > > 2.31.0\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 A440EC3256\n\tfor ;\n\tWed, 18 May 2022 10:59:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA3FB6565B;\n\tWed, 18 May 2022 12:59:09 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4E10565656\n\tfor ;\n\tWed, 18 May 2022 12:59:08 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 7B479240002;\n\tWed, 18 May 2022 10:59:07 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652871549;\n\tbh=KG7Mz4AWnzqDphbAfekn24dQ4iYLL/uDETpW//evfOY=;\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=bSUyX4GENOeZqAcaJ1pOb6t7+UpBNqIbOtVeadkWrUVyg8So+mpDur61bIn/C8wyn\n\to2HkqPCacoEc7D0oZO/BllvvQDsqhkzjsd/xlJDH/HeN/oaviNJfqhpyfrR7zVYdbv\n\tDGpXYgVs2ys2XqXR+rtame1CsaQBc1muhyaWDvPUqXoUk17fUjlzP/iB5jrVoy0yqY\n\t5958yXZr9UKWVFaFKCc6STBtvFxgSubOADHnlZxyCDifIMh8raKxS1P0yzpo2hPljK\n\t/0OB0lg94/srXMf5dcnC973WRUJNZvJMkswyGLmSSeYGiGP02sIv/FUGsvisYmULOf\n\tlWf5s1Gf3LJ5g==","Date":"Wed, 18 May 2022 12:59:05 +0200","To":"Umang Jain ","Message-ID":"<20220518105905.3uaufctl7zgoygya@uno.localdomain>","References":"<20220517191833.333122-1-umang.jain@ideasonboard.com>\n\t<20220517191833.333122-4-umang.jain@ideasonboard.com>\n\t<20220518074726.k47zfauj7e6stbng@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 v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","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":23049,"web_url":"https://patchwork.libcamera.org/comment/23049/","msgid":"<539403a5-8ce4-4b94-bc9f-ac6bbd8801d5@ideasonboard.com>","date":"2022-05-18T11:32:55","subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 5/18/22 12:44, Kieran Bingham wrote:\n> Quoting Umang Jain via libcamera-devel (2022-05-18 10:39:11)\n>> Hi JM,\n>>\n>> On 5/18/22 11:08, Jean-Michel Hautbois wrote:\n>>> Hi Umang,\n>>>\n>>> On 17/05/2022 21:18, Umang Jain via libcamera-devel wrote:\n>>>> Instead of having one frame context constantly being updated,\n>>>> this patch aims to introduce per-frame IPAFrameContext which\n>>>> are stored in a ring buffer. Whenever a request is queued, a new\n>>>> IPAFrameContext is created and inserted into the ring buffer.\n>>>>\n>>>> The IPAFrameContext structure itself has been slightly extended\n>>>> to store a frame id and a ControlList for incoming frame\n>>>> controls (sent in by the application). The next step would be to\n>>>> read and set these controls whenever the request is actually queued\n>>>> to the hardware.\n>>>>\n>>>> Since now we are working in multiples of IPAFrameContext, the\n>>>> Algorithm::process() will actually take in a IPAFrameContext pointer\n>>>> (as opposed to a nullptr while preparing for this change).\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>>   src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------\n>>>>   src/ipa/ipu3/algorithms/agc.h   |  4 ++--\n>>>>   src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--\n>>>>   src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-\n>>>>   src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------\n>>>>   5 files changed, 55 insertions(+), 20 deletions(-)\n>>>>\n>>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp\n>>>> b/src/ipa/ipu3/algorithms/agc.cpp\n>>>> index 383a8deb..f16be534 100644\n>>>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>>>> @@ -183,14 +183,13 @@ utils::Duration\n>>>> Agc::filterExposure(utils::Duration exposureValue)\n>>>>    * \\param[in] yGain The gain calculated based on the relative\n>>>> luminance target\n>>>>    * \\param[in] iqMeanGain The gain calculated based on the relative\n>>>> luminance target\n>>>>    */\n>>>> -void Agc::computeExposure(IPAContext &context, double yGain,\n>>>> -              double iqMeanGain)\n>>>> +void Agc::computeExposure(IPAContext &context, IPAFrameContext\n>>>> *frameContext,\n>>>> +              double yGain, double iqMeanGain)\n>>>>   {\n>>>>       const IPASessionConfiguration &configuration =\n>>>> context.configuration;\n>>>> -    IPAFrameContext &frameContext = context.frameContext;\n>>>>       /* Get the effective exposure and gain applied on the sensor. */\n>>>> -    uint32_t exposure = frameContext.sensor.exposure;\n>>>> -    double analogueGain = frameContext.sensor.gain;\n>>>> +    uint32_t exposure = frameContext->sensor.exposure;\n>>>> +    double analogueGain = frameContext->sensor.gain;\n>>>>         /* Use the highest of the two gain estimates. */\n>>>>       double evGain = std::max(yGain, iqMeanGain);\n>>>> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context,\n>>>> [[maybe_unused]] IPAFrameContext *frameCo\n>>>>               break;\n>>>>       }\n>>>>   -    computeExposure(context, yGain, iqMeanGain);\n>>>> +    computeExposure(context, frameContext, yGain, iqMeanGain);\n>>>>       frameCount_++;\n>>>>   }\n>>>>   diff --git a/src/ipa/ipu3/algorithms/agc.h\n>>>> b/src/ipa/ipu3/algorithms/agc.h\n>>>> index 219a1a96..105ae0f2 100644\n>>>> --- a/src/ipa/ipu3/algorithms/agc.h\n>>>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>>>> @@ -35,8 +35,8 @@ private:\n>>>>       double measureBrightness(const ipu3_uapi_stats_3a *stats,\n>>>>                    const ipu3_uapi_grid_config &grid) const;\n>>>>       utils::Duration filterExposure(utils::Duration currentExposure);\n>>>> -    void computeExposure(IPAContext &context, double yGain,\n>>>> -                 double iqMeanGain);\n>>>> +    void computeExposure(IPAContext &context, IPAFrameContext\n>>>> *frameContext,\n>>>> +                 double yGain, double iqMeanGain);\n>>>>       double estimateLuminance(IPAActiveState &activeState,\n>>>>                    const ipu3_uapi_grid_config &grid,\n>>>>                    const ipu3_uapi_stats_3a *stats,\n>>>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>>>> index 06eb2776..81b30600 100644\n>>>> --- a/src/ipa/ipu3/ipa_context.cpp\n>>>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>>>> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {\n>>>>    * \\var IPAContext::configuration\n>>>>    * \\brief The IPA session configuration, immutable during the session\n>>>>    *\n>>>> - * \\var IPAContext::frameContext\n>>>> - * \\brief The frame context for the frame being processed\n>>>> + * \\var IPAContext::frameContexts\n>>>> + * \\brief Ring buffer of the IPAFrameContext(s)\n>>>>    *\n>>>>    * \\var IPAContext::activeState\n>>>>    * \\brief The current state of IPA algorithms\n>>>> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {\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\n>>>> &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 8d681131..42e11141 100644\n>>>> --- a/src/ipa/ipu3/ipa_context.h\n>>>> +++ b/src/ipa/ipu3/ipa_context.h\n>>>> @@ -8,16 +8,22 @@\n>>>>     #pragma once\n>>>>   +#include \n>>>> +\n>>>>   #include \n>>>>     #include \n>>>>   +#include \n>>>>   #include \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>>>>       struct {\n>>>>           ipu3_uapi_grid_config bdsGrid;\n>>>> @@ -71,17 +77,23 @@ struct IPAActiveState {\n>>>>   };\n>>>>     struct IPAFrameContext {\n>>>> +    IPAFrameContext();\n>>>> +    IPAFrameContext(uint32_t id, const ControlList &reqControls);\n>>>> +\n>>>>       struct {\n>>>>           uint32_t exposure;\n>>>>           double gain;\n>>>>       } sensor;\n>>>> +\n>>>> +    uint32_t frame;\n>>>> +    ControlList frameControls;\n>>>>   };\n>>>>     struct IPAContext {\n>>>>       IPASessionConfiguration configuration;\n>>>>       IPAActiveState activeState;\n>>>>   -    IPAFrameContext frameContext;\n>>>> +    std::array frameContexts;\n>>>>   };\n>>>>     } /* namespace ipa::ipu3 */\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index 16e5028f..4322f96b 100644\n>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,\n>>>>       }\n>>>>         /* Clean context */\n>>>> -    context_ = {};\n>>>> +    context_.configuration = {};\n>>>>       context_.configuration.sensor.lineDuration =\n>>>> sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n>>>>         /* Construct our Algorithms */\n>>>> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo\n>>>> &configInfo,\n>>>>         /* Clean IPAActiveState at each reconfiguration. */\n>>>>       context_.activeState = {};\n>>>> -    context_.frameContext = {};\n>>>> +    IPAFrameContext initFrameContext;\n>>>> +    context_.frameContexts.fill(initFrameContext);\n>>>>         if (!validateSensorControls()) {\n>>>>           LOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n>>>> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t\n>>>> frame,\n>>>>       const ipu3_uapi_stats_3a *stats =\n>>>>           reinterpret_cast(mem.data());\n>>>>   -    context_.frameContext.sensor.exposure =\n>>>> sensorControls.get(V4L2_CID_EXPOSURE).get();\n>>>> -    context_.frameContext.sensor.gain =\n>>>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>>>> +    IPAFrameContext &frameContext = context_.frameContexts[frame %\n>>>> kMaxFrameContexts];\n>>> Shouldn't the frame be \"double-checked\", verifying that\n>>> (frameContext->frame == frame) before passing it to the algorithms\n>>> somehow ?\n>>\n>> Well, we can surely verify that but we know in advance that the\n>> verification is subjected to failure with known cases (for e.g. frame\n>> drop) so I am not very keen putting the verification in. If you had seen\n>> the v2, I had a ASSERT similar to this basically (and has some\n>> discussion on this there)\n> I do think some sort of check is warranted though, because the CIO2\n> might be free-running with no requests coming in - so I can envisage we\n> could easily end up overflowing the ring buffer in a (bad) scenario - so\n> we should at least report that.\n>\n> I would expect that in that scenario the CIO2 will run but the IMGU\n> might not - so we wouldn't get any statistics for that frame.\n>\n> It's something to think about later anyway ... but a check/print would\n> be beneficial at least. For now it could still operate anyway, but with\n> a warning print.\n\n\nFair enough, I have introduced a warning:\n\n+       if (frameContext.frame != frame)\n+               LOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not \nmatch its frame context\";\n\n>\n> Reviewed-by: Kieran Bingham \n>\n>>> Apart from that:\n>>> Reviewed-by: Jean-Michel Hautbois \n>>\n>> Thanks for reviewing!\n>>\n>>>> +\n>>>> +    frameContext.sensor.exposure =\n>>>> sensorControls.get(V4L2_CID_EXPOSURE).get();\n>>>> +    frameContext.sensor.gain =\n>>>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>>>>         double lineDuration =\n>>>> context_.configuration.sensor.lineDuration.get();\n>>>>       int32_t vBlank = context_.configuration.sensor.defVBlank;\n>>>>       ControlList ctrls(controls::controls);\n>>>>         for (auto const &algo : algorithms_)\n>>>> -        algo->process(context_, nullptr, stats);\n>>>> +        algo->process(context_, &frameContext, stats);\n>>>>         setControls(frame);\n>>>>   @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const\n>>>> uint32_t frame,\n>>>>       int64_t frameDuration = (vBlank +\n>>>> sensorInfo_.outputSize.height) * lineDuration;\n>>>>       ctrls.set(controls::FrameDuration, frameDuration);\n>>>>   -    ctrls.set(controls::AnalogueGain,\n>>>> context_.frameContext.sensor.gain);\n>>>> +    ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);\n>>>>         ctrls.set(controls::ColourTemperature,\n>>>> context_.activeState.awb.temperatureK);\n>>>>   -    ctrls.set(controls::ExposureTime,\n>>>> context_.frameContext.sensor.exposure * lineDuration);\n>>>> +    ctrls.set(controls::ExposureTime, frameContext.sensor.exposure *\n>>>> lineDuration);\n>>>>         /*\n>>>>        * \\todo The Metadata provides a path to getting extended data\n>>>> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t\n>>>> frame,\n>>>>    * Parse the request to handle any IPA-managed controls that were\n>>>> set from the\n>>>>    * application such as manual sensor settings.\n>>>>    */\n>>>> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,\n>>>> -               [[maybe_unused]] const ControlList &controls)\n>>>> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList\n>>>> &controls)\n>>>>   {\n>>>>       /* \\todo Start processing for 'frame' based on 'controls'. */\n>>>> +    IPAFrameContext newFrameContext(frame, controls);\n>>>> +    context_.frameContexts[frame % kMaxFrameContexts] =\n>>>> newFrameContext;\n>>>>   }\n>>>>     /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DFAD6C0F2A\n\tfor ;\n\tWed, 18 May 2022 11:33:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 24DBC6565B;\n\tWed, 18 May 2022 13:33:01 +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 452F465656\n\tfor ;\n\tWed, 18 May 2022 13:32:59 +0200 (CEST)","from [192.168.1.164] (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7CA7EE50;\n\tWed, 18 May 2022 13:32:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652873581;\n\tbh=FzCE5YrMG9cJwT9EUq5Yl2JgVXjoBQYHchLKO6yadEE=;\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=X1MeZxKvyGw5z4erKJgZgwg2mEE+4xXHUQmvZgIqJYWMtzAn4gUTBrvaRz3ybZ3Yx\n\tg3JaktOZR/dIGRlmAACNtQ6sa5JaNnZwcHr2xQG0xVDqOGvy4Dm2QExgj9uFOoFZRZ\n\t3jiE/Xl70WNyxunS4qvFLujM7ctt/XQ6tcjK42PyVZpLzdTNEamsBpFKNCl4g4HWdI\n\tVYmSHJE94sBWLFxjKNyOIRAJ2vMTGOnOyCxNULnmVitmlcHAb6eYIugYg/6alZpoyu\n\tY8wM1/kpTTOtZupEkbG250gp2NilxZX7q/YsVi7ykVOQg7bu1/mvTZQ3W8YTJa9jDW\n\toUBilbT9jTd+w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652873578;\n\tbh=FzCE5YrMG9cJwT9EUq5Yl2JgVXjoBQYHchLKO6yadEE=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=BAWH44cApHmPOWkZmvXZlPvyHW9qW7AZweaMVd0xOz9ouCFu1i9rTSsrZQ5lolfLq\n\tZ2Ns03i6V14bP9UFZBGw14GrRc4ZG/oi4JWK4cfRmu49GO18CgyrCl6zgoTsE7x6sM\n\tAQxVSN5/lRgJzgd+dbIHCVCpfm9hR0ORluBvJ70Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BAWH44cA\"; dkim-atps=neutral","Message-ID":"<539403a5-8ce4-4b94-bc9f-ac6bbd8801d5@ideasonboard.com>","Date":"Wed, 18 May 2022 13:32:55 +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":"Kieran Bingham ,\n\tJean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220517191833.333122-1-umang.jain@ideasonboard.com>\n\t<20220517191833.333122-4-umang.jain@ideasonboard.com>\n\t\n\t<7ed4f7b5-a3ac-2c4b-a878-c704757249d7@ideasonboard.com>\n\t<165287064942.368702.14864609516246569168@Monstersaurus>","In-Reply-To":"<165287064942.368702.14864609516246569168@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","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\" "}}]