[{"id":23066,"web_url":"https://patchwork.libcamera.org/comment/23066/","msgid":"","date":"2022-05-18T15:50:28","subject":"Re: [libcamera-devel] [PATCH v4 3/3] ipa: ipu3: Put\n\tIPAFrameContext(s) in a ring buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Wed, May 18, 2022 at 03:10:30PM +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> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Jean-Michel Hautbois \n> Reviewed-by: Kieran Bingham \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------\n> src/ipa/ipu3/algorithms/agc.h | 4 ++--\n> src/ipa/ipu3/ipa_context.cpp | 26 ++++++++++++++++++++++----\n> src/ipa/ipu3/ipa_context.h | 14 +++++++++++++-\n> src/ipa/ipu3/ipu3.cpp | 24 +++++++++++++++---------\n> 5 files changed, 57 insertions(+), 22 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 383c2e37..13cdb835 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -58,13 +58,11 @@ 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> - *\n> - * \\todo The frame context needs to be turned into real per-frame data storage.\n> */\n> \n> /**\n> @@ -183,6 +181,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\nframeId may be more explicit. Or maybe even just id ?\n\n> + * \\brief The frame number\n> + *\n> + * \\var IPAFrameContext::frameControls\n> + * \\brief Controls sent in by the application while queuing the request\n\nSame here, I'd call this controls. Everything in the frame context is\nrelated to a frame.\n\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\nI'd be tempted to pass the configuration and active state to algorithms\ninstead of the IPAContext, to avoid exposing frameContexts. That will\nmake the algorithm functions take quite a few arguments though, it may\nnot be very nice.\n\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 16e5028f..2f6bb672 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,20 @@ 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> +\tif (frameContext.frame != frame)\n> +\t\tLOG(IPAIPU3, Warning) << \"Frame \" << frame << \" does not match its frame context\";\n\nHmmm... I think the ring buffer needs to be extracted to a separate\nclass. We should avoid the need for spurious initialization, and offer\nsafety against overflows.\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 +590,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \tint64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;\n> \tctrls.set(controls::FrameDuration, frameDuration);\n> \n> -\tctrls.set(controls::AnalogueGain, 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 +615,10 @@ 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> +\tcontext_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };\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 67127C3256\n\tfor ;\n\tWed, 18 May 2022 15:50:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98CFB65659;\n\tWed, 18 May 2022 17:50:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74B3B65656\n\tfor ;\n\tWed, 18 May 2022 17:50:37 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [45.131.31.124])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71004E50;\n\tWed, 18 May 2022 17:50:36 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652889038;\n\tbh=Xoz8PmWKk7D2bw9FUBrUcKznSLHBoQRQB8k3dF7dPvY=;\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=YQaCxRBsLewektgqlv9lCMEjtq38NGcJslCKGxxq7VqMehizz5JycJPaRC3V3poQw\n\tvBerroQ/FCApy1O6eopjOD7ejDrtw7saTH1FpH9XlH2rZAJuWmRMwZGVL4M0KZbKeM\n\tRyqSOO95Vpu2AwK4EkH4l8MeZz23CTv8LuTN+QC+RLPjCchdY/jzPZg/gdkQXKnEEC\n\t84ZB7Yp76zfp0Y8ldYjbfDRHMDIiFi5GdMDHrK5d2Kl5cRop6eKyFDKbGdUG1UqEED\n\tZb5H8ZDnIhOKOjVtaQqC9X885pR1o7FWGkpdZUcZ7Smj+HkbtmU3W0l/W4zSV4dHxV\n\t55jZerkr6D9bQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652889037;\n\tbh=Xoz8PmWKk7D2bw9FUBrUcKznSLHBoQRQB8k3dF7dPvY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NOS/FVLI22WrC37RtS6h6+sJqFjJtzFOM8NWgGr1V6OuIkTBw2Eq9xdwL+qvkX8sg\n\tXpEyUrL2SkvrhjtbkK7yv+FH7V8ghzyZMvoX68dIH7zBaaIss22Iq7XXPN2gKVMKqr\n\tjytHzETAWqJCCFH+W8wfL0P7C1uT2K5HzGMMIHTo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NOS/FVLI\"; dkim-atps=neutral","Date":"Wed, 18 May 2022 18:50:28 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220518131030.421225-1-umang.jain@ideasonboard.com>\n\t<20220518131030.421225-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220518131030.421225-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]