[{"id":26792,"web_url":"https://patchwork.libcamera.org/comment/26792/","msgid":"<168003417701.3713142.9150841262659282216@Monstersaurus>","date":"2023-03-28T20:09:37","subject":"Re: [libcamera-devel] [PATCH v2 02/10] ipa: raspberrypi: Add\n\thardware configuration to the controller","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2023-03-27 13:20:22)\n> Add a new Controller::HardwareConfig structure that captures the\n> hardware statistics grid/histogram sizes and pipeline widths. This\n> ensures there is a single centralised places for these parameters.\n> \n> Add a getHardwareConfig() helper function to retrieve these values for a\n> given hardware target.\n> \n> Update the statistics populating routine in the IPA to use the values\n> from this structure instead of the hardcoded numbers.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n\nReviewed-by: Kieran Bingham \n\n> ---\n> src/ipa/raspberrypi/controller/algorithm.h | 4 +++\n> src/ipa/raspberrypi/controller/controller.cpp | 31 +++++++++++++++++++\n> src/ipa/raspberrypi/controller/controller.h | 11 +++++++\n> src/ipa/raspberrypi/raspberrypi.cpp | 21 +++++--------\n> 4 files changed, 54 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/algorithm.h b/src/ipa/raspberrypi/controller/algorithm.h\n> index 7c22fbe4945c..4aa814ebbebd 100644\n> --- a/src/ipa/raspberrypi/controller/algorithm.h\n> +++ b/src/ipa/raspberrypi/controller/algorithm.h\n> @@ -45,6 +45,10 @@ public:\n> {\n> return controller_->getTarget();\n> }\n> + const Controller::HardwareConfig &getHardwareConfig() const\n> + {\n> + return controller_->getHardwareConfig();\n> + }\n> \n> private:\n> Controller *controller_;\n> diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp\n> index a6250ee140b0..fa1721130783 100644\n> --- a/src/ipa/raspberrypi/controller/controller.cpp\n> +++ b/src/ipa/raspberrypi/controller/controller.cpp\n> @@ -20,6 +20,25 @@ using namespace libcamera;\n> \n> LOG_DEFINE_CATEGORY(RPiController)\n> \n> +static const std::map HardwareConfigMap = {\n> + {\n> + \"bcm2835\",\n> + {\n> + /*\n> + * There are only ever 15 AGC regions computed by the firmware\n> + * due to zoning, but the HW defines AGC_REGIONS == 16!\n> + */\n> + .agcRegions = { 15 , 1 },\n> + .agcZoneWeights = { 15 , 1 },\n> + .awbRegions = { 16, 12 },\n> + .focusRegions = { 4, 3 },\n> + .numHistogramBins = 128,\n> + .numGammaPoints = 33,\n> + .pipelineWidth = 13\n> + }\n> + },\n> +};\n> +\n> Controller::Controller()\n> : switchModeCalled_(false)\n> {\n> @@ -148,3 +167,15 @@ const std::string &Controller::getTarget() const\n> {\n> return target_;\n> }\n> +\n> +const Controller::HardwareConfig &Controller::getHardwareConfig() const\n> +{\n> + auto cfg = HardwareConfigMap.find(getTarget());\n> +\n> + /*\n> + * This really should not happen, the IPA ought to validate the target\n> + * on initialisation.\n> + */\n> + ASSERT(cfg != HardwareConfigMap.end());\n> + return cfg->second;\n> +}\n> diff --git a/src/ipa/raspberrypi/controller/controller.h b/src/ipa/raspberrypi/controller/controller.h\n> index 24e02903d438..c6af5cd6c7d4 100644\n> --- a/src/ipa/raspberrypi/controller/controller.h\n> +++ b/src/ipa/raspberrypi/controller/controller.h\n> @@ -37,6 +37,16 @@ typedef std::unique_ptr AlgorithmPtr;\n> class Controller\n> {\n> public:\n> + struct HardwareConfig {\n> + libcamera::Size agcRegions;\n> + libcamera::Size agcZoneWeights;\n> + libcamera::Size awbRegions;\n> + libcamera::Size focusRegions;\n> + unsigned int numHistogramBins;\n> + unsigned int numGammaPoints;\n> + unsigned int pipelineWidth;\n> + };\n> +\n> Controller();\n> ~Controller();\n> int read(char const *filename);\n> @@ -47,6 +57,7 @@ public:\n> Metadata &getGlobalMetadata();\n> Algorithm *getAlgorithm(std::string const &name) const;\n> const std::string &getTarget() const;\n> + const HardwareConfig &getHardwareConfig() const;\n> \n> protected:\n> int createAlgorithm(const std::string &name, const libcamera::YamlObject ¶ms);\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 86359538cf67..b64cb96e2dde 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -1392,20 +1392,19 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> {\n> using namespace RPiController;\n> \n> + const Controller::HardwareConfig &hw = controller_.getHardwareConfig();\n> unsigned int i;\n> StatisticsPtr statistics =\n> std::make_unique(Statistics::AgcStatsPos::PreWb, Statistics::ColourStatsPos::PostLsc);\n> \n> /* RGB histograms are not used, so do not populate them. */\n> - statistics->yHist = RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS);\n> + statistics->yHist = RPiController::Histogram(stats->hist[0].g_hist,\n> + hw.numHistogramBins);\n> \n> - /*\n> - * All region sums are based on a 13-bit pipeline bit-depth. Normalise\n> - * this to 16-bits for the AGC/AWB/ALSC algorithms.\n> - */\n> - constexpr unsigned int scale = Statistics::NormalisationFactorPow2 - 13;\n> + /* All region sums are based on a 16-bit normalised pipeline bit-depth. */\n> + unsigned int scale = Statistics::NormalisationFactorPow2 - hw.pipelineWidth;\n> \n> - statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y });\n> + statistics->awbRegions.init(hw.awbRegions);\n> for (i = 0; i < statistics->awbRegions.numRegions(); i++)\n> statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << scale,\n> stats->awb_stats[i].g_sum << scale,\n> @@ -1413,11 +1412,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> stats->awb_stats[i].counted,\n> stats->awb_stats[i].notcounted });\n> \n> - /*\n> - * There are only ever 15 regions computed by the firmware due to zoning,\n> - * but the HW defines AGC_REGIONS == 16!\n> - */\n> - statistics->agcRegions.init(15);\n> + statistics->agcRegions.init(hw.agcRegions);\n> for (i = 0; i < statistics->agcRegions.numRegions(); i++)\n> statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << scale,\n> stats->agc_stats[i].g_sum << scale,\n> @@ -1425,7 +1420,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co\n> stats->agc_stats[i].counted,\n> stats->awb_stats[i].notcounted });\n> \n> - statistics->focusRegions.init({ 4, 3 });\n> + statistics->focusRegions.init(hw.focusRegions);\n> for (i = 0; i < statistics->focusRegions.numRegions(); i++)\n> statistics->focusRegions.set(i, { stats->focus_stats[i].contrast_val[1][1] / 1000,\n> stats->focus_stats[i].contrast_val_num[1][1],\n> -- \n> 2.34.1\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 0A703C0F2A\n\tfor ;\n\tTue, 28 Mar 2023 20:09:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CCD762740;\n\tTue, 28 Mar 2023 22:09:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CED5561ECD\n\tfor ;\n\tTue, 28 Mar 2023 22:09:39 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6F538AD5;\n\tTue, 28 Mar 2023 22:09:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680034181;\n\tbh=mbf1Z2DP74dKgISkzrqWq/8JV+392CuqPpzFNMVMBQ0=;\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=SdKpEgK7LMTZDiKQxsAcQlJ1NrASyWXyY8UGN9F8DGlQvF9W0sLv/iG8gOXDnwHLL\n\tN6wobROohwV7PrOxaTJ3GPRokwquC5RKWoMCjrFTOfZLS0EiGz9c4ZO57fpCRIE/ZW\n\t4qiy5WK2BiKoaODsw2aMvLej3ttIaRhkARxYucwS6jiuKNjB+BM7eWLl8SixFb6pwM\n\tSBroEGrvCHGp+2zr7FLAh6su/+4b+Um+WIux5tzQMeBN2+3EqJDIgg2myYIsY9Pbbf\n\t+ME+QjpNb8/n1fVLlAZ6axYNOOtLfbqXN4nq6RrNgh15l3lYJ/3PHsdKLwT+XtiDkj\n\t7YGkPXKOdBhUA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680034179;\n\tbh=mbf1Z2DP74dKgISkzrqWq/8JV+392CuqPpzFNMVMBQ0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=XH5jcnuQ/UrMdzzk8nHBsmeq0P7y6SSK+jWXIjG9u5aekOk7MQN4wLUB4GybIxiNq\n\t4DMsUNyxM8sqyzU67yEye7m2r3YX+mH1o82cfOPKS7AZ2Vg0QA6EPPzJ6a7ZCSoto7\n\tfjaST7bpt5S70wrwHfbXVTZFByy5DMAVyFrcG9cs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XH5jcnuQ\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230327122030.11756-3-naush@raspberrypi.com>","References":"<20230327122030.11756-1-naush@raspberrypi.com>\n\t<20230327122030.11756-3-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 28 Mar 2023 21:09:37 +0100","Message-ID":"<168003417701.3713142.9150841262659282216@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 02/10] ipa: raspberrypi: Add\n\thardware configuration to the controller","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\" "}}]