[{"id":25016,"web_url":"https://patchwork.libcamera.org/comment/25016/","msgid":"<166368412449.3912877.13178903374104311584@Monstersaurus>","date":"2022-09-20T14:28:44","subject":"Re: [libcamera-devel] [PATCH v4 10/32] ipa: ipu3: Use the FCQueue","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:38)\n> Replace the manual ring buffer implementation with the FCQueue class\n> from libipa.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/ipu3/ipa_context.cpp | 14 --------------\n> src/ipa/ipu3/ipa_context.h | 10 +---------\n> src/ipa/ipu3/ipu3.cpp | 32 ++++++++++++++++++++++++--------\n> 3 files changed, 25 insertions(+), 31 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 9cfca0db3a0d..6904ccbbdf8b 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -164,20 +164,6 @@ namespace libcamera::ipa::ipu3 {\n> * struct ipu3_uapi_gamma_corr_lut for further details.\n> */\n> \n> -/**\n> - * \\brief Default constructor for IPAFrameContext\n> - */\n> -IPAFrameContext::IPAFrameContext() = default;\n> -\n> -/**\n> - * \\brief Construct a IPAFrameContext instance\n> - */\n> -IPAFrameContext::IPAFrameContext(const ControlList &reqControls)\n> - : frameControls(reqControls)\n> -{\n> - sensor = {};\n> -}\n> -\n> /**\n> * \\struct IPAFrameContext\n> * \\brief IPU3-specific FrameContext\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index e8fc42769075..bfc0196e098a 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -8,8 +8,6 @@\n> \n> #pragma once\n> \n> -#include \n> -\n> #include \n> \n> #include \n> @@ -23,9 +21,6 @@ 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> struct {\n> ipu3_uapi_grid_config bdsGrid;\n> @@ -79,9 +74,6 @@ struct IPAActiveState {\n> };\n> \n> struct IPAFrameContext : public FrameContext {\n> - IPAFrameContext();\n> - IPAFrameContext(const ControlList &reqControls);\n> -\n> struct {\n> uint32_t exposure;\n> double gain;\n> @@ -94,7 +86,7 @@ struct IPAContext {\n> IPASessionConfiguration configuration;\n> IPAActiveState activeState;\n> \n> - std::array frameContexts;\n> + FCQueue frameContexts;\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index b1b23fd8f927..8158ca0883e8 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -40,6 +40,8 @@\n> #include \"algorithms/tone_mapping.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n> \n> +#include \"ipa_context.h\"\n> +\n> /* Minimum grid width, expressed as a number of cells */\n> static constexpr uint32_t kMinGridWidth = 16;\n> /* Maximum grid width, expressed as a number of cells */\n> @@ -53,6 +55,9 @@ static constexpr uint32_t kMinCellSizeLog2 = 3;\n> /* log2 of the maximum grid cell width and height, in pixels */\n> static constexpr uint32_t kMaxCellSizeLog2 = 6;\n> \n> +/* Maximum number of frame contexts to be held */\n> +static constexpr uint32_t kMaxFrameContexts = 16;\n> +\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPAIPU3)\n> @@ -135,6 +140,8 @@ namespace ipa::ipu3 {\n> class IPAIPU3 : public IPAIPU3Interface, public Module\n> {\n> public:\n> + IPAIPU3();\n> +\n> int init(const IPASettings &settings,\n> const IPACameraSensorInfo &sensorInfo,\n> const ControlInfoMap &sensorControls,\n> @@ -183,6 +190,11 @@ private:\n> struct IPAContext context_;\n> };\n> \n> +IPAIPU3::IPAIPU3()\n> + : context_({ {}, {}, { kMaxFrameContexts } })\n> +{\n> +}\n> +\n> std::string IPAIPU3::logPrefix() const\n> {\n> return \"ipu3\";\n> @@ -205,6 +217,11 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> int32_t minGain = v4l2Gain.min().get();\n> int32_t maxGain = v4l2Gain.max().get();\n> \n> + /* Clear the IPA context before the streaming session. */\n> + context_.configuration = {};\n> + context_.activeState = {};\n> + context_.frameContexts.clear();\n\nI'm scared about stop / configure / start bugs here ...\n\n( scared or scarred, or both :D )\n\n> +\n> /*\n> * When the AGC computes the new exposure values for a frame, it needs\n> * to know the limits for shutter speed and analogue gain.\n> @@ -382,6 +399,7 @@ int IPAIPU3::start()\n> */\n> void IPAIPU3::stop()\n> {\n> + context_.frameContexts.clear();\n> }\n> \n> /**\n> @@ -488,11 +506,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> \n> calculateBdsGrid(configInfo.bdsOutputSize);\n> \n> - /* Clean IPAActiveState at each reconfiguration. */\n> - context_.activeState = {};\n> - IPAFrameContext initFrameContext;\n> - context_.frameContexts.fill(initFrameContext);\n> -\n> if (!validateSensorControls()) {\n> LOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> return -EINVAL;\n> @@ -572,7 +585,7 @@ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> */\n> params->use = {};\n> \n> - IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \n> for (auto const &algo : algorithms())\n> algo->prepare(context_, frame, frameContext, params);\n> @@ -605,7 +618,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> const ipu3_uapi_stats_3a *stats =\n> reinterpret_cast(mem.data());\n> \n> - IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> \n> frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> @@ -651,7 +664,10 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> /* \\todo Start processing for 'frame' based on 'controls'. */\n> - context_.frameContexts[frame % kMaxFrameContexts] = { controls };\n> + IPAFrameContext &frameContext = context_.frameContexts.init(frame);\n> +\n> + /* \\todo Implement queueRequest to each algorithm. */\n> + frameContext.frameControls = controls;\n\nOk - all looks good enough to build upon...\n\n\nReviewed-by: Kieran Bingham \n\n> }\n> \n> /**\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DB7B4C3272\n\tfor ;\n\tTue, 20 Sep 2022 14:28:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 10ED6621B5;\n\tTue, 20 Sep 2022 16:28:49 +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 839BF6218B\n\tfor ;\n\tTue, 20 Sep 2022 16:28:47 +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 0904B6BE;\n\tTue, 20 Sep 2022 16:28:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663684129;\n\tbh=QutbgIuoEzOUHtBdzscBoumDCIvYCFVvymqV7sSDN0k=;\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=sXB1KwWIJpUEKiK0wS1v79S8eyXo8E2GL5hbyT6dINR/wPU+3kqvDOegpaMmOywfW\n\tNlBczbFFtBZnIOaTUC08Qz9iTYXNVP0Gqf8/8+0n7+ze241KeYpBu1kWDPknw1cHVg\n\t2sZoqWUBCqPbZhasxZ/6QTkyPZkN5rvwP6RouYL05K/3Is1arajfpTilNDAJtlSncU\n\tzr5TAxBa0n4rX2LVUFQ9b8gd5J7gEu8vrGLyMyW5lx7vOWG0R5aWp+1dSsoVB6+wWf\n\tGtliIYekC9zo9IvYOUyuaQPaVpTY7FANMcoUCVol5HI0MhePCDtlLwWUJKn8quEGrb\n\t2aDDRxP/rgtHQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663684127;\n\tbh=QutbgIuoEzOUHtBdzscBoumDCIvYCFVvymqV7sSDN0k=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=FWr3PlM0hyIFWCfw5aKqdST4Di/CkvPOl6g9BeZZJdjqceZT7f+4hssd3Q64h+/se\n\tp/RGngaVZlGdPUis7v5CkLHmiaUaAAUc85IzxlmYjKcdoa9hkI09RGqtzNxmCd5QT7\n\tlDObg+Rs7XsNytvJS0yyjdtl3lHRUnIX1D02M720="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FWr3PlM0\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220908014200.28728-11-laurent.pinchart@ideasonboard.com>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-11-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 20 Sep 2022 15:28:44 +0100","Message-ID":"<166368412449.3912877.13178903374104311584@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 10/32] ipa: ipu3: Use the FCQueue","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":25029,"web_url":"https://patchwork.libcamera.org/comment/25029/","msgid":"","date":"2022-09-20T19:43:33","subject":"Re: [libcamera-devel] [PATCH v4 10/32] ipa: ipu3: Use the FCQueue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Sep 20, 2022 at 03:28:44PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:38)\n> > Replace the manual ring buffer implementation with the FCQueue class\n> > from libipa.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/ipu3/ipa_context.cpp | 14 --------------\n> > src/ipa/ipu3/ipa_context.h | 10 +---------\n> > src/ipa/ipu3/ipu3.cpp | 32 ++++++++++++++++++++++++--------\n> > 3 files changed, 25 insertions(+), 31 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> > index 9cfca0db3a0d..6904ccbbdf8b 100644\n> > --- a/src/ipa/ipu3/ipa_context.cpp\n> > +++ b/src/ipa/ipu3/ipa_context.cpp\n> > @@ -164,20 +164,6 @@ namespace libcamera::ipa::ipu3 {\n> > * struct ipu3_uapi_gamma_corr_lut for further details.\n> > */\n> > \n> > -/**\n> > - * \\brief Default constructor for IPAFrameContext\n> > - */\n> > -IPAFrameContext::IPAFrameContext() = default;\n> > -\n> > -/**\n> > - * \\brief Construct a IPAFrameContext instance\n> > - */\n> > -IPAFrameContext::IPAFrameContext(const ControlList &reqControls)\n> > - : frameControls(reqControls)\n> > -{\n> > - sensor = {};\n> > -}\n> > -\n> > /**\n> > * \\struct IPAFrameContext\n> > * \\brief IPU3-specific FrameContext\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index e8fc42769075..bfc0196e098a 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -8,8 +8,6 @@\n> > \n> > #pragma once\n> > \n> > -#include \n> > -\n> > #include \n> > \n> > #include \n> > @@ -23,9 +21,6 @@ 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> > struct {\n> > ipu3_uapi_grid_config bdsGrid;\n> > @@ -79,9 +74,6 @@ struct IPAActiveState {\n> > };\n> > \n> > struct IPAFrameContext : public FrameContext {\n> > - IPAFrameContext();\n> > - IPAFrameContext(const ControlList &reqControls);\n> > -\n> > struct {\n> > uint32_t exposure;\n> > double gain;\n> > @@ -94,7 +86,7 @@ struct IPAContext {\n> > IPASessionConfiguration configuration;\n> > IPAActiveState activeState;\n> > \n> > - std::array frameContexts;\n> > + FCQueue frameContexts;\n> > };\n> > \n> > } /* namespace ipa::ipu3 */\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index b1b23fd8f927..8158ca0883e8 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -40,6 +40,8 @@\n> > #include \"algorithms/tone_mapping.h\"\n> > #include \"libipa/camera_sensor_helper.h\"\n> > \n> > +#include \"ipa_context.h\"\n> > +\n> > /* Minimum grid width, expressed as a number of cells */\n> > static constexpr uint32_t kMinGridWidth = 16;\n> > /* Maximum grid width, expressed as a number of cells */\n> > @@ -53,6 +55,9 @@ static constexpr uint32_t kMinCellSizeLog2 = 3;\n> > /* log2 of the maximum grid cell width and height, in pixels */\n> > static constexpr uint32_t kMaxCellSizeLog2 = 6;\n> > \n> > +/* Maximum number of frame contexts to be held */\n> > +static constexpr uint32_t kMaxFrameContexts = 16;\n> > +\n> > namespace libcamera {\n> > \n> > LOG_DEFINE_CATEGORY(IPAIPU3)\n> > @@ -135,6 +140,8 @@ namespace ipa::ipu3 {\n> > class IPAIPU3 : public IPAIPU3Interface, public Module\n> > {\n> > public:\n> > + IPAIPU3();\n> > +\n> > int init(const IPASettings &settings,\n> > const IPACameraSensorInfo &sensorInfo,\n> > const ControlInfoMap &sensorControls,\n> > @@ -183,6 +190,11 @@ private:\n> > struct IPAContext context_;\n> > };\n> > \n> > +IPAIPU3::IPAIPU3()\n> > + : context_({ {}, {}, { kMaxFrameContexts } })\n> > +{\n> > +}\n> > +\n> > std::string IPAIPU3::logPrefix() const\n> > {\n> > return \"ipu3\";\n> > @@ -205,6 +217,11 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> > int32_t minGain = v4l2Gain.min().get();\n> > int32_t maxGain = v4l2Gain.max().get();\n> > \n> > + /* Clear the IPA context before the streaming session. */\n> > + context_.configuration = {};\n> > + context_.activeState = {};\n> > + context_.frameContexts.clear();\n> \n> I'm scared about stop / configure / start bugs here ...\n\nThe queue is cleared on stop(), so that part should be fine. The\nconfiguration isn't meant to be modified at runtime, so it should be\nfine too. The active state may be problematic, but that's not introduced\nby this patch, it was previously cleared in IPAIPU3::configure() and\nonly moved to IPAIPU3::updateSessionConfiguration() here.\n\n> ( scared or scarred, or both :D )\n> \n> > +\n> > /*\n> > * When the AGC computes the new exposure values for a frame, it needs\n> > * to know the limits for shutter speed and analogue gain.\n> > @@ -382,6 +399,7 @@ int IPAIPU3::start()\n> > */\n> > void IPAIPU3::stop()\n> > {\n> > + context_.frameContexts.clear();\n> > }\n> > \n> > /**\n> > @@ -488,11 +506,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> > \n> > calculateBdsGrid(configInfo.bdsOutputSize);\n> > \n> > - /* Clean IPAActiveState at each reconfiguration. */\n> > - context_.activeState = {};\n> > - IPAFrameContext initFrameContext;\n> > - context_.frameContexts.fill(initFrameContext);\n> > -\n> > if (!validateSensorControls()) {\n> > LOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> > return -EINVAL;\n> > @@ -572,7 +585,7 @@ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > */\n> > params->use = {};\n> > \n> > - IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > \n> > for (auto const &algo : algorithms())\n> > algo->prepare(context_, frame, frameContext, params);\n> > @@ -605,7 +618,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > const ipu3_uapi_stats_3a *stats =\n> > reinterpret_cast(mem.data());\n> > \n> > - IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> > + IPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > \n> > frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > @@ -651,7 +664,10 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > {\n> > /* \\todo Start processing for 'frame' based on 'controls'. */\n> > - context_.frameContexts[frame % kMaxFrameContexts] = { controls };\n> > + IPAFrameContext &frameContext = context_.frameContexts.init(frame);\n> > +\n> > + /* \\todo Implement queueRequest to each algorithm. */\n> > + frameContext.frameControls = controls;\n> \n> Ok - all looks good enough to build upon...\n> \n> \n> Reviewed-by: Kieran Bingham \n> \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 229D1C0DA4\n\tfor ;\n\tTue, 20 Sep 2022 19:43:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CF26621C9;\n\tTue, 20 Sep 2022 21:43:50 +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 963ED6218B\n\tfor ;\n\tTue, 20 Sep 2022 21:43:48 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D88716BE;\n\tTue, 20 Sep 2022 21:43:47 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663703030;\n\tbh=Lvulg7pz2SWYcQY8Qbno1f2XdncKw9reZUjfNGMiVKk=;\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=ZEs6fS5PAbDFlP8jLd5xcHUnjdpEi1cV25L96PGEGl7+EKMwEW+ne2BHPisgOCZ5E\n\tAC7jNSbVvqeND053/jb+WZg88dt4hbHAbo2k2jBa80SV8ahfGe1zEamUNinxAmBkQi\n\tiweTgsR9C2KTUDoRAzxc2+lBRKj7KAagVHrr0OHzKJjn+LCEMeDjO+9AfYbPsaEROm\n\te9eVPGoleTSZKhWYD9HV4pUGx8Zqy4xlQwiAoRvMq/HIhJaMpXv9byUxnsS9orMETv\n\thYqlpQHAi9YCOCJXHoTAMBoooTYxCwDjLWcZ8Y2WMAe/mSiUCCbyS5vKylcFvw9Rlr\n\t4Vf3hk/BsOZoQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663703028;\n\tbh=Lvulg7pz2SWYcQY8Qbno1f2XdncKw9reZUjfNGMiVKk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eQiwy2oV9DQHFJL6BoGPXRNy//1qNmD0rU64rnK2rL0GWJIeWf9CJ+o5g3AO1Ms1O\n\tBrz7GfZeezZS2vSmaVvj4q12VXef9NE/QC/3aUSGBQ5tAf+Dr2lj/SyGpRe0SikRFp\n\tr5/2qa7ERnm2p8WA6dj5kBnyps0yYw55lQrLSbK8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eQiwy2oV\"; dkim-atps=neutral","Date":"Tue, 20 Sep 2022 22:43:33 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-11-laurent.pinchart@ideasonboard.com>\n\t<166368412449.3912877.13178903374104311584@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166368412449.3912877.13178903374104311584@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 10/32] ipa: ipu3: Use the FCQueue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25056,"web_url":"https://patchwork.libcamera.org/comment/25056/","msgid":"<20220921181353.dr45khowblzpjwkb@uno.localdomain>","date":"2022-09-21T18:13:53","subject":"Re: [libcamera-devel] [PATCH v4 10/32] ipa: ipu3: Use the FCQueue","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"This answers most of my previous questions...\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\nOn Thu, Sep 08, 2022 at 04:41:38AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Replace the manual ring buffer implementation with the FCQueue class\n> from libipa.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/ipu3/ipa_context.cpp | 14 --------------\n> src/ipa/ipu3/ipa_context.h | 10 +---------\n> src/ipa/ipu3/ipu3.cpp | 32 ++++++++++++++++++++++++--------\n> 3 files changed, 25 insertions(+), 31 deletions(-)\n>\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 9cfca0db3a0d..6904ccbbdf8b 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -164,20 +164,6 @@ namespace libcamera::ipa::ipu3 {\n> * struct ipu3_uapi_gamma_corr_lut for further details.\n> */\n>\n> -/**\n> - * \\brief Default constructor for IPAFrameContext\n> - */\n> -IPAFrameContext::IPAFrameContext() = default;\n> -\n> -/**\n> - * \\brief Construct a IPAFrameContext instance\n> - */\n> -IPAFrameContext::IPAFrameContext(const ControlList &reqControls)\n> -\t: frameControls(reqControls)\n> -{\n> -\tsensor = {};\n> -}\n> -\n> /**\n> * \\struct IPAFrameContext\n> * \\brief IPU3-specific FrameContext\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index e8fc42769075..bfc0196e098a 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -8,8 +8,6 @@\n>\n> #pragma once\n>\n> -#include \n> -\n> #include \n>\n> #include \n> @@ -23,9 +21,6 @@ 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> @@ -79,9 +74,6 @@ struct IPAActiveState {\n> };\n>\n> struct IPAFrameContext : public FrameContext {\n> -\tIPAFrameContext();\n> -\tIPAFrameContext(const ControlList &reqControls);\n> -\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> @@ -94,7 +86,7 @@ struct IPAContext {\n> \tIPASessionConfiguration configuration;\n> \tIPAActiveState activeState;\n>\n> -\tstd::array frameContexts;\n> +\tFCQueue frameContexts;\n> };\n>\n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index b1b23fd8f927..8158ca0883e8 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -40,6 +40,8 @@\n> #include \"algorithms/tone_mapping.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n>\n> +#include \"ipa_context.h\"\n> +\n> /* Minimum grid width, expressed as a number of cells */\n> static constexpr uint32_t kMinGridWidth = 16;\n> /* Maximum grid width, expressed as a number of cells */\n> @@ -53,6 +55,9 @@ static constexpr uint32_t kMinCellSizeLog2 = 3;\n> /* log2 of the maximum grid cell width and height, in pixels */\n> static constexpr uint32_t kMaxCellSizeLog2 = 6;\n>\n> +/* Maximum number of frame contexts to be held */\n> +static constexpr uint32_t kMaxFrameContexts = 16;\n> +\n> namespace libcamera {\n>\n> LOG_DEFINE_CATEGORY(IPAIPU3)\n> @@ -135,6 +140,8 @@ namespace ipa::ipu3 {\n> class IPAIPU3 : public IPAIPU3Interface, public Module\n> {\n> public:\n> +\tIPAIPU3();\n> +\n> \tint init(const IPASettings &settings,\n> \t\t const IPACameraSensorInfo &sensorInfo,\n> \t\t const ControlInfoMap &sensorControls,\n> @@ -183,6 +190,11 @@ private:\n> \tstruct IPAContext context_;\n> };\n>\n> +IPAIPU3::IPAIPU3()\n> +\t: context_({ {}, {}, { kMaxFrameContexts } })\n> +{\n> +}\n> +\n> std::string IPAIPU3::logPrefix() const\n> {\n> \treturn \"ipu3\";\n> @@ -205,6 +217,11 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)\n> \tint32_t minGain = v4l2Gain.min().get();\n> \tint32_t maxGain = v4l2Gain.max().get();\n>\n> +\t/* Clear the IPA context before the streaming session. */\n> +\tcontext_.configuration = {};\n> +\tcontext_.activeState = {};\n> +\tcontext_.frameContexts.clear();\n> +\n> \t/*\n> \t * When the AGC computes the new exposure values for a frame, it needs\n> \t * to know the limits for shutter speed and analogue gain.\n> @@ -382,6 +399,7 @@ int IPAIPU3::start()\n> */\n> void IPAIPU3::stop()\n> {\n> +\tcontext_.frameContexts.clear();\n> }\n>\n> /**\n> @@ -488,11 +506,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>\n> \tcalculateBdsGrid(configInfo.bdsOutputSize);\n>\n> -\t/* Clean IPAActiveState at each reconfiguration. */\n> -\tcontext_.activeState = {};\n> -\tIPAFrameContext initFrameContext;\n> -\tcontext_.frameContexts.fill(initFrameContext);\n> -\n> \tif (!validateSensorControls()) {\n> \t\tLOG(IPAIPU3, Error) << \"Sensor control validation failed.\";\n> \t\treturn -EINVAL;\n> @@ -572,7 +585,7 @@ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> \t */\n> \tparams->use = {};\n>\n> -\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>\n> \tfor (auto const &algo : algorithms())\n> \t\talgo->prepare(context_, frame, frameContext, params);\n> @@ -605,7 +618,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> \tconst ipu3_uapi_stats_3a *stats =\n> \t\treinterpret_cast(mem.data());\n>\n> -\tIPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];\n> +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>\n> \tframeContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> \tframeContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> @@ -651,7 +664,10 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> -\tcontext_.frameContexts[frame % kMaxFrameContexts] = { controls };\n> +\tIPAFrameContext &frameContext = context_.frameContexts.init(frame);\n> +\n> +\t/* \\todo Implement queueRequest to each algorithm. */\n> +\tframeContext.frameControls = controls;\n> }\n>\n> /**\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1B448C3272\n\tfor ;\n\tWed, 21 Sep 2022 18:13:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 505A1621ED;\n\tWed, 21 Sep 2022 20:13:57 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6C422600AA\n\tfor ;\n\tWed, 21 Sep 2022 20:13:55 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id DB2601C0003;\n\tWed, 21 Sep 2022 18:13:54 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663784037;\n\tbh=eg7YYDk0jXQ4INYJPsT/b8HWg0fCZmBehjCAAvBjhPc=;\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=hDEsKkzZYO7AHXJDkL6HVPbV8hspI0QwootbmkWm68BdK9NQRRt+6qTIWublHIrpF\n\t+bD08FQnKOhji0MgUmNdp7Gpe4KujA6Bi3b+OhN6T9IKyG7hoqbdD2EqesKIX9OH+j\n\t/g0o0M3xcE4UbGl7g7Oeed5o7s6aukUkSkfXg5oKlzMCqR6fB5ocJ+P/N+xZJb5KIn\n\tfIdvPMdO1RI7p43VRea3HRKBY0VavttsWGQ6G4I24yC++4Dna0Rc5hpIkTfltFr4Gw\n\td5ml183KNjCExDNm4hyuHf6Idaxnvu3VDTnJQUcq1mj6sPJvUpqGrRJx2TVxbOUegB\n\t+jYQOA0I/qKyg==","Date":"Wed, 21 Sep 2022 20:13:53 +0200","To":"Laurent Pinchart ","Message-ID":"<20220921181353.dr45khowblzpjwkb@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-11-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220908014200.28728-11-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 10/32] ipa: ipu3: Use the FCQueue","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\" "}}]