[{"id":20041,"web_url":"https://patchwork.libcamera.org/comment/20041/","msgid":"","date":"2021-10-05T01:14:19","subject":"Re: [libcamera-devel] [PATCH v2 09/12] ipa: ipu3: awb: Use the line\n\tstride for the stats","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 Thu, Sep 30, 2021 at 11:37:12AM +0200, Jean-Michel Hautbois wrote:\n> The statistics buffer 'ipu3_uapi_awb_raw_buffer' stores the ImgU\n> calculation results in a buffer aligned horizontally to a multiple of 4\n> cells. The AWB loop should take care of it to add the proper offset\n> between lines and avoid any staircase effect.\n> \n> It is now no more required to pass the grid configuration context to the\n\nKieran commented on the wording in v1.\n\n> private functions called from process() which simplifies the code flow.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Reviewed-by: Kieran Bingham \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 13 ++++++-------\n> src/ipa/ipu3/algorithms/awb.h | 7 +++----\n> src/ipa/ipu3/ipa_context.h | 1 +\n> src/ipa/ipu3/ipu3.cpp | 3 +++\n> 4 files changed, 13 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index c1ff9450..fcd1469f 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -172,6 +172,7 @@ int Awb::configure(IPAContext &context,\n> \t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> {\n> \tconst ipu3_uapi_grid_config &grid = context.configuration.grid.bdsGrid;\n> +\tstride_ = context.configuration.grid.stride;\n> \n> \tcellsPerZoneX_ = std::round(grid.width / static_cast(kAwbStatsSizeX));\n> \tcellsPerZoneY_ = std::round(grid.height / static_cast(kAwbStatsSizeY));\n\nThis is wrong, it will cause the algorithm to go over the grid if we end\nup rounding up. It's not introduced in this patch though, so it can be\nfixed on top.\n\n> @@ -234,8 +235,7 @@ void Awb::generateZones(std::vector &zones)\n> }\n> \n> /* Translate the IPU3 statistics into the default statistics zone array */\n> -void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> -\t\t\t const ipu3_uapi_grid_config &grid)\n> +void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> {\n> \t/*\n> \t * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is\n> @@ -243,7 +243,7 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> \t */\n> \tfor (unsigned int cellY = 0; cellY < kAwbStatsSizeY * cellsPerZoneY_; cellY++) {\n> \t\tfor (unsigned int cellX = 0; cellX < kAwbStatsSizeX * cellsPerZoneX_; cellX++) {\n> -\t\t\tuint32_t cellPosition = (cellY * grid.width + cellX)\n> +\t\t\tuint32_t cellPosition = (cellY * stride_ + cellX)\n> \t\t\t\t\t * sizeof(Ipu3AwbCell);\n> \t\t\tuint32_t zoneX = cellX / cellsPerZoneX_;\n> \t\t\tuint32_t zoneY = cellY / cellsPerZoneY_;\n> @@ -318,13 +318,12 @@ void Awb::awbGreyWorld()\n> \tasyncResults_.blueGain = blueGain;\n> }\n> \n> -void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,\n> -\t\t\t const ipu3_uapi_grid_config &grid)\n> +void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> {\n> \tASSERT(stats->stats_3a_status.awb_en);\n> \tzones_.clear();\n> \tclearAwbStats();\n> -\tgenerateAwbStats(stats, grid);\n> +\tgenerateAwbStats(stats);\n> \tgenerateZones(zones_);\n> \tLOG(IPU3Awb, Debug) << \"Valid zones: \" << zones_.size();\n> \tif (zones_.size() > 10) {\n> @@ -336,7 +335,7 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,\n> \n> void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> {\n> -\tcalculateWBGains(stats, context.configuration.grid.bdsGrid);\n> +\tcalculateWBGains(stats);\n> \n> \t/*\n> \t * Gains are only recalculated if enough zones were detected.\n> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> index 681d8c2b..b3e0ad82 100644\n> --- a/src/ipa/ipu3/algorithms/awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -74,11 +74,9 @@ public:\n> \t};\n> \n> private:\n> -\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats,\n> -\t\t\t const ipu3_uapi_grid_config &grid);\n> +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> \tvoid generateZones(std::vector &zones);\n> -\tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> -\t\t\t const ipu3_uapi_grid_config &grid);\n> +\tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> \tvoid clearAwbStats();\n> \tvoid awbGreyWorld();\n> \tuint32_t estimateCCT(double red, double green, double blue);\n> @@ -87,6 +85,7 @@ private:\n> \tAccumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n> \tAwbStatus asyncResults_;\n> \n> +\tuint32_t stride_;\n> \tuint32_t cellsPerZoneX_;\n> \tuint32_t cellsPerZoneY_;\n> \tuint32_t cellsPerZoneThreshold_;\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 9d9444dc..5bab684c 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -20,6 +20,7 @@ struct IPASessionConfiguration {\n> \tstruct {\n> \t\tipu3_uapi_grid_config bdsGrid;\n> \t\tSize bdsOutputSize;\n> +\t\tuint32_t stride;\n\nYou need to document the new member.\n\n> \t} grid;\n> };\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 757a5d50..b33e8c77 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -352,6 +352,9 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \tbdsGrid.height = best.height >> bestLog2.height;\n> \tbdsGrid.block_height_log2 = bestLog2.height;\n> \n> +\t/* The grid width is aligned to the next multiple of 4 */\n\nYou're not aligning the effective with, I'd write it as\n\n\t/* The ImgU pads the lines to a multiple of 4 cells. */\n\nReviewed-by: Laurent Pinchart \n\n> +\tcontext_.configuration.grid.stride = utils::alignUp(bdsGrid.width, 4);\n> +\n> \tLOG(IPAIPU3, Debug) << \"Best grid found is: (\"\n> \t\t\t << (int)bdsGrid.width << \" << \" << (int)bdsGrid.block_width_log2 << \") x (\"\n> \t\t\t << (int)bdsGrid.height << \" << \" << (int)bdsGrid.block_height_log2 << \")\";","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 D33E9C3243\n\tfor ;\n\tTue, 5 Oct 2021 01:14:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A040691B6;\n\tTue, 5 Oct 2021 03:14:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65D2669189\n\tfor ;\n\tTue, 5 Oct 2021 03:14:26 +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 B8F3D25B;\n\tTue, 5 Oct 2021 03:14:25 +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=\"hQqRxAjk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633396465;\n\tbh=Un8WZZj0jKcFLo5rWJYEexkPBMFBCT9Wvp3KpFtCe84=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hQqRxAjkNrgPw5zllgi0b96ZeuKSSdnStRz5KJyf6SoHWzpr7vE+KHJ05CdmCpCyx\n\tbqzPrivPedbtNduplpaMr66sW7HTubm3Dw3zkcx16UAWXlG7oZFsZTDmJcrJRwy3Iy\n\tdlsEdDPDXh/Hyz9nBCDk3BP7I1KBHiykBCFmBJUU=","Date":"Tue, 5 Oct 2021 04:14:19 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210930093715.73293-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210930093715.73293-10-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210930093715.73293-10-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 09/12] ipa: ipu3: awb: Use the line\n\tstride for the stats","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\" "}}]