[{"id":18032,"web_url":"https://patchwork.libcamera.org/comment/18032/","msgid":"","date":"2021-07-08T13:07:20","subject":"Re: [libcamera-devel] [PATCH v1 6/7] ipa: ipu3: Call exposure and\n\tgain controls from AGC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Mon, Jun 28, 2021 at 10:22:54PM +0200, Jean-Michel Hautbois wrote:\n> IPAIPU3 does not need to directly know the exposure and gain limits.\n> Move the control min and max values to IPU3Agc as for the moment it is the one which\n> needs to use the values.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 14 +++-----------\n> src/ipa/ipu3/ipu3_agc.cpp | 28 +++++++++++++++++++++++-----\n> src/ipa/ipu3/ipu3_agc.h | 11 +++++++++--\n> 3 files changed, 35 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 9a2def64..40b626ed 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -67,11 +67,7 @@ private:\n> \t/* Camera sensor controls. */\n> \tuint32_t defVBlank_;\n> \tuint32_t exposure_;\n> -\tuint32_t minExposure_;\n> -\tuint32_t maxExposure_;\n> \tuint32_t gain_;\n> -\tuint32_t minGain_;\n> -\tuint32_t maxGain_;\n> \n> \t/* Interface to the AWB algorithm */\n> \tstd::unique_ptr awbAlgo_;\n> @@ -187,13 +183,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tminExposure_ = std::max(itExp->second.min().get(), 1);\n> -\tmaxExposure_ = itExp->second.max().get();\n> -\texposure_ = minExposure_;\n> +\texposure_ = itExp->second.def().get();\n\nThis changes the exposure_ value, it used to be set to the minimum, it's\nnow the default. Is that intentiona ? If so, it should be explained in\nthe commit message.\n> \n> -\tminGain_ = std::max(itGain->second.min().get(), 1);\n> -\tmaxGain_ = itGain->second.max().get();\n> -\tgain_ = minGain_;\n> +\tgain_ = itGain->second.def().get();\n\nSame here.\n\n> \n> \tdefVBlank_ = itVBlank->second.def().get();\n> \n> @@ -205,7 +197,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n> \n> \tagcAlgo_ = std::make_unique();\n> -\tagcAlgo_->initialise(bdsGrid_, sensorInfo_);\n> +\tagcAlgo_->initialise(bdsGrid_, configInfo);\n> \n> \treturn 0;\n> }\n> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n> index 6253ab94..042d67fa 100644\n> --- a/src/ipa/ipu3/ipu3_agc.cpp\n> +++ b/src/ipa/ipu3/ipu3_agc.cpp\n> @@ -10,10 +10,12 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> -#include \n> +#include \n> \n> -#include \n> +#include \n> +#include \n> \n> #include \"libipa/histogram.h\"\n> \n> @@ -59,12 +61,28 @@ IPU3Agc::IPU3Agc()\n> {\n> }\n> \n> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo)\n> +void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPAConfigInfo &configInfo)\n> {\n> \taeGrid_ = bdsGrid;\n> +\tctrls_ = configInfo.entityControls.at(0);\n> \n> -\tlineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;\n> -\tmaxExposureTime_ = kMaxExposure * lineDuration_;\n> +\tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> +\tif (itExp == ctrls_.end()) {\n> +\t\tLOG(IPU3Agc, Debug) << \"Can't find exposure control\";\n> +\t\treturn;\n> +\t}\n> +\tminExposure_ = itExp->second.min().get();\n> +\tmaxExposure_ = itExp->second.max().get();\n> +\tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s / configInfo.sensorInfo.pixelRate;\n> +\tmaxExposureTime_ = maxExposure_ * lineDuration_;\n> +\n> +\tconst auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> +\tif (itGain == ctrls_.end()) {\n> +\t\tLOG(IPU3Agc, Debug) << \"Can't find gain control\";\n> +\t\treturn;\n> +\t}\n> +\tminGain_ = std::max(itGain->second.min().get(), 1);\n> +\tmaxGain_ = itGain->second.max().get();\n> }\n> \n> void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)\n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index 774c8385..ce43c534 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -15,7 +15,7 @@\n> #include \n> \n> #include \n> -\n> +#include \n> #include \n> \n> #include \"libipa/algorithm.h\"\n> @@ -35,8 +35,8 @@ public:\n> \tIPU3Agc();\n> \t~IPU3Agc() = default;\n> \n> -\tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);\n> \tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);\n> +\tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPAConfigInfo &configInfo);\n> \tbool converged() { return converged_; }\n> \tbool updateControls() { return updateControls_; }\n> \t/* \\todo Use a metadata exchange between IPAs */\n> @@ -48,6 +48,13 @@ private:\n> \tvoid lockExposureGain(uint32_t &exposure, double &gain);\n> \n> \tstruct ipu3_uapi_grid_config aeGrid_;\n> +\tControlInfoMap ctrls_;\n\nI'm not too fond of storing another copy of this, especially given that\nit's not used.\n\n> +\n> +\tuint32_t minExposure_;\n> +\tuint32_t maxExposure_;\n> +\n> +\tuint32_t minGain_;\n> +\tuint32_t maxGain_;\n> \n> \tuint64_t frameCount_;\n> \tuint64_t lastFrame_;","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 44EEEC3224\n\tfor ;\n\tThu, 8 Jul 2021 13:08:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A13A96851F;\n\tThu, 8 Jul 2021 15:08:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0477668506\n\tfor ;\n\tThu, 8 Jul 2021 15:08:05 +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 8AC7AE7;\n\tThu, 8 Jul 2021 15:08:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EfNy4LGf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625749684;\n\tbh=PKD5SHewCC6pz/tsDkn9wmdAQpsJfJw3OdJunNLM/fk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EfNy4LGfDuazLOVlk0VTn42XfhJ8OmbMzeXcunFlygkG034K+4BSHgdpqzvQAc+w+\n\tpuqh5MhdTXLjhEZDUUvYKXQRPUX0Xh2NyYm+0u/u7BO3x9sQLr+OMQkFy3b2tQVlG6\n\tmw9i2KAX8ZVmQxLLyc35lnZB2rRM706o2juDrxpc=","Date":"Thu, 8 Jul 2021 16:07:20 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210628202255.138874-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210628202255.138874-7-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210628202255.138874-7-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 6/7] ipa: ipu3: Call exposure and\n\tgain controls from AGC","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]