[{"id":25132,"web_url":"https://patchwork.libcamera.org/comment/25132/","msgid":"<20220927091952.ifrg6673t6374okn@uno.localdomain>","date":"2022-09-27T09:19:52","subject":"Re: [libcamera-devel] [PATCH v5 20/33] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Tue, Sep 27, 2022 at 05:36:29AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Rework the algorithm's usage of the active state to store the value of\n> controls for the last queued request in the queueRequest() function, and\n> store a copy of the values in the corresponding frame context.\n>\n> The frame context is used in the prepare() function to populate the ISP\n> parameters with values corresponding to the right frame.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n> ---\n> Changes since v4:\n>\n> - Add todo comments related to sensor controls\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 27 +++++++++++++------\n> src/ipa/rkisp1/algorithms/agc.h | 3 ++-\n> src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++-----------\n> src/ipa/rkisp1/ipa_context.h | 14 ++++++----\n> src/ipa/rkisp1/rkisp1.cpp | 14 +++++++---\n> 5 files changed, 68 insertions(+), 33 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 87fc5d1ffec7..4540bde66db4 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> /**\n> * \\brief Estimate the new exposure and gain values\n> * \\param[inout] context The shared IPA Context\n> + * \\param[in] frameContext The FrameContext for this frame\n> * \\param[in] yGain The gain calculated on the current brightness level\n> * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> */\n> -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n> +void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,\n> +\t\t\t double yGain, double iqMeanGain)\n> {\n> \tIPASessionConfiguration &configuration = context.configuration;\n> \tIPAActiveState &activeState = context.activeState;\n>\n> \t/* Get the effective exposure and gain applied on the sensor. */\n> -\tuint32_t exposure = activeState.sensor.exposure;\n> -\tdouble analogueGain = activeState.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> @@ -286,9 +288,16 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> * new exposure and gain for the scene.\n> */\n> void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> -\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> -\t\t const rkisp1_stat_buffer *stats)\n> +\t\t IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats)\n> {\n> +\t/*\n> +\t * \\todo Verify that the exposure and gain applied by the sensor for\n> +\t * this frame match what has been requested. This isn't a hard\n> +\t * requirement for stability of the AGC (the guarantee we need in\n> +\t * automatic mode is a perfect match between the frame and the values\n> +\t * we receive), but is important in manual mode.\n> +\t */\n> +\n> \tconst rkisp1_cif_isp_stat *params = &stats->params;\n> \tASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);\n>\n> @@ -320,7 +329,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t\tbreak;\n> \t}\n>\n> -\tcomputeExposure(context, yGain, iqMeanGain);\n> +\tcomputeExposure(context, frameContext, yGain, iqMeanGain);\n> \tframeCount_++;\n> }\n>\n> @@ -328,9 +337,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> -\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> -\t\t rkisp1_params_cfg *params)\n> +\t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> {\n> +\tframeContext.agc.exposure = context.activeState.agc.exposure;\n> +\tframeContext.agc.gain = context.activeState.agc.gain;\n> +\n> \tif (frame > 0)\n> \t\treturn;\n>\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index f115ba2ed85c..9ad5c32fd6f6 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -34,7 +34,8 @@ public:\n> \t\t const rkisp1_stat_buffer *stats) override;\n>\n> private:\n> -\tvoid computeExposure(IPAContext &Context, double yGain, double iqMeanGain);\n> +\tvoid computeExposure(IPAContext &Context, IPAFrameContext &frameContext,\n> +\t\t\t double yGain, double iqMeanGain);\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n> \tdouble estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> \tdouble measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 78a785f5f982..c7d5b1b6ec5a 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPAActiveState::agc\n> * \\brief State for the Automatic Gain Control algorithm\n> *\n> - * The exposure and gain determined are expected to be applied to the sensor\n> - * at the earliest opportunity.\n> + * The exposure and gain are the latest values computed by the AGC algorithm.\n> *\n> * \\var IPAActiveState::agc.exposure\n> * \\brief Exposure time expressed as a number of lines\n> *\n> * \\var IPAActiveState::agc.gain\n> * \\brief Analogue gain multiplier\n> - *\n> - * The gain should be adapted to the sensor specific gain code before applying.\n> */\n>\n> /**\n> @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 {\n> * \\brief Indicates if ISP parameters need to be updated\n> */\n>\n> -/**\n> - * \\var IPAActiveState::sensor\n> - * \\brief Effective sensor values\n> - *\n> - * \\var IPAActiveState::sensor.exposure\n> - * \\brief Exposure time expressed as a number of lines\n> - *\n> - * \\var IPAActiveState::sensor.gain\n> - * \\brief Analogue gain multiplier\n> - */\n> -\n> /**\n> * \\struct IPAFrameContext\n> * \\brief Per-frame context for algorithms\n> @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 {\n> * \\todo Populate the frame context for all algorithms\n> */\n>\n> +/**\n> + * \\var IPAFrameContext::agc\n> + * \\brief Automatic Gain Control parameters for this frame\n> + *\n> + * The exposure and gain are provided by the AGC algorithm, and are to be\n> + * applied to the sensor in order to take effect for this frame.\n> + *\n> + * \\var IPAFrameContext::agc.exposure\n> + * \\brief Exposure time expressed as a number of lines\n> + *\n> + * \\var IPAFrameContext::agc.gain\n> + * \\brief Analogue gain multiplier\n> + *\n> + * The gain should be adapted to the sensor specific gain code before applying.\n> + */\n> +\n> +/**\n> + * \\var IPAFrameContext::sensor\n> + * \\brief Sensor configuration that used been used for this frame\n> + *\n> + * \\var IPAFrameContext::sensor.exposure\n> + * \\brief Exposure time expressed as a number of lines\n> + *\n> + * \\var IPAFrameContext::sensor.gain\n> + * \\brief Analogue gain multiplier\n> + */\n> +\n> /**\n> * \\struct IPAContext\n> * \\brief Global IPA context data shared between all algorithms\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index c7041ea2a214..a4d134e700b5 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -83,14 +83,18 @@ struct IPAActiveState {\n> \t\tuint8_t sharpness;\n> \t\tbool updateParams;\n> \t} filter;\n> -\n> -\tstruct {\n> -\t\tuint32_t exposure;\n> -\t\tdouble gain;\n> -\t} sensor;\n> };\n>\n> struct IPAFrameContext : public FrameContext {\n> +\tstruct {\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t} agc;\n> +\n> +\tstruct {\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t} sensor;\n> };\n>\n> static constexpr uint32_t kMaxFrameContexts = 16;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 5409c2d5219e..6297916cf86d 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -332,9 +332,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> \t\treinterpret_cast(\n> \t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n>\n> -\tcontext_.activeState.sensor.exposure =\n> +\tframeContext.sensor.exposure =\n> \t\tsensorControls.get(V4L2_CID_EXPOSURE).get();\n> -\tcontext_.activeState.sensor.gain =\n> +\tframeContext.sensor.gain =\n> \t\tcamHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>\n> \tunsigned int aeState = 0;\n> @@ -349,8 +349,14 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n>\n> void IPARkISP1::setControls(unsigned int frame)\n> {\n> -\tuint32_t exposure = context_.activeState.agc.exposure;\n> -\tuint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);\n> +\t/*\n> +\t * \\todo The frame number is most likely wrong here, we need to take\n> +\t * internal sensor delays and other timing parameters into account.\n> +\t */\n> +\n> +\tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> +\tuint32_t exposure = frameContext.agc.exposure;\n> +\tuint32_t gain = camHelper_->gainCode(frameContext.agc.gain);\n>\n> \tControlList ctrls(ctrls_);\n> \tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\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 A5FECBD16B\n\tfor ;\n\tTue, 27 Sep 2022 09:19:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CF0E6226E;\n\tTue, 27 Sep 2022 11:19:57 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::221])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83D9C61F7B\n\tfor ;\n\tTue, 27 Sep 2022 11:19:55 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id BDF9A24000E;\n\tTue, 27 Sep 2022 09:19:54 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664270397;\n\tbh=KlkHcGHnHRPyCaSkyBZR+2u0s8rU/ikwsQI/rj7oa70=;\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=ewbdna2fGnx1YxDZOWhT+3c1sMMNLd6mqVeaUuE6CZiiFUFUIZWJTILF5TxTDpNZm\n\ttJn5jxaQfktedKGrs/CH666WP4jmu795EKqMgclnFV0s1NP41b3FglK7G0OcwdDPPW\n\tGOteQ1EPEkZTs3QDwv2ixuUz3QXqEmPeXAgzNLdUIdoJN5vpgnRRgpYqR94E9fpXGT\n\tWS1gTjpLkpMdibQZKnNAEYZI3NarFBPMPyz8wPFJM8+dCs2LhUuZKbCFFNnqwrqbjT\n\t/Wh0Q43E1EQZdEhExLBHm82cgn4v5wsdwCy28VHUBovld6MB+mRP624DRlYx9Zxb8M\n\tDNi6Ot8Ejd7AQ==","Date":"Tue, 27 Sep 2022 11:19:52 +0200","To":"Laurent Pinchart ","Message-ID":"<20220927091952.ifrg6673t6374okn@uno.localdomain>","References":"<20220927023642.12341-1-laurent.pinchart@ideasonboard.com>\n\t<20220927023642.12341-21-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220927023642.12341-21-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 20/33] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","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\" "}}]