[{"id":19936,"web_url":"https://patchwork.libcamera.org/comment/19936/","msgid":"<20210928160242.7vkawzir72oug7no@ideasonboard.com>","date":"2021-09-28T16:02:42","subject":"Re: [libcamera-devel] [PATCH 09/12] ipa: ipu3: awb: Use the line\n\tstride for the stats","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On Thu, Sep 23, 2021 at 10:16:22AM +0200, Jean-Michel Hautbois wrote:\n> The statistics buffer 'ipu3_uapi_awb_raw_buffer' stores the ImgU\n> calculation results in a buffer aligned to a multiple of 4. The AWB loop\n> should take care of it to add the proper offset between lines and avoid\n> any staircase effect.\n> \n> It is now no more required to pass the grid configuration context to the\n\n\"It is no longer required to ...\"\n\nThat's a nice perk indeed.\n\n> private functions called from process() which simplifies the code flow.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 16 ++++++++--------\n> src/ipa/ipu3/algorithms/awb.h | 7 +++----\n> 2 files changed, 11 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index 8a926691..a5391653 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -172,8 +172,10 @@ 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> +\t/* The grid is aligned to the next multiple of 4 */\n> +\tstride_ = (grid.width + 3) / 4 * 4;\n\nFeels like something that could have been handled by an alignedUp macro.\nWe have that in geometry.h but for Size ...\n\nSo perhaps we can just open code it (unless anyone knows of a\nstd:: macro that does this?)\n\n\nReviewed-by: Kieran Bingham \n\n> \n> -\tcellsPerZoneX_ = round(grid.width / static_cast(kAwbStatsSizeX));\n> +\tcellsPerZoneX_ = round(stride_ / static_cast(kAwbStatsSizeX));\n> \tcellsPerZoneY_ = round(grid.height / static_cast(kAwbStatsSizeY));\n> \n> \t/*\n> @@ -234,8 +236,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 +244,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 +319,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 +336,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> -- \n> 2.30.2\n> \n\n--\nKieran","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 16A16C3243\n\tfor ;\n\tTue, 28 Sep 2021 16:02:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 469EE69191;\n\tTue, 28 Sep 2021 18:02:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9469669188\n\tfor ;\n\tTue, 28 Sep 2021 18:02:44 +0200 (CEST)","from 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 2B4D43F1;\n\tTue, 28 Sep 2021 18:02:44 +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=\"GYU5peOM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632844964;\n\tbh=I26XYpJUptB41r3/3bFmJTiWBFbshNcO2eD6myG/es4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GYU5peOMSU98p8RFWUZhTnXu63ToYX5oF8BiNKQXqSpKwaL3bKUJdYyjP25jtRQeP\n\tgbFGDtVLYVlJ6NQgyVvl1HhOvzXnX4S5eg096DTwNcR/BFwkqYFlBkN4cLGauA5I9y\n\tY34MPHscjjLsK3fn5Kpxour54i7MjAT0HgbMR7Go=","Date":"Tue, 28 Sep 2021 17:02:42 +0100","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ","Message-ID":"<20210928160242.7vkawzir72oug7no@ideasonboard.com>","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-10-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210923081625.60276-10-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":19996,"web_url":"https://patchwork.libcamera.org/comment/19996/","msgid":"","date":"2021-09-29T12:44:07","subject":"Re: [libcamera-devel] [PATCH 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":"Hello,\n\nOn Tue, Sep 28, 2021 at 05:02:42PM +0100, Kieran Bingham wrote:\n> On Thu, Sep 23, 2021 at 10:16:22AM +0200, Jean-Michel Hautbois wrote:\n> > The statistics buffer 'ipu3_uapi_awb_raw_buffer' stores the ImgU\n> > calculation results in a buffer aligned to a multiple of 4. The AWB loop\n\ns/aligned to a multiple of 4/aligned horizontally to a multiple of 4 cells/\n\n> > should take care of it to add the proper offset between lines and avoid\n> > any staircase effect.\n> > \n> > It is now no more required to pass the grid configuration context to the\n> \n> \"It is no longer required to ...\"\n> \n> That's a nice perk indeed.\n> \n> > private functions called from process() which simplifies the code flow.\n> > \n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/ipu3/algorithms/awb.cpp | 16 ++++++++--------\n> > src/ipa/ipu3/algorithms/awb.h | 7 +++----\n> > 2 files changed, 11 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> > index 8a926691..a5391653 100644\n> > --- a/src/ipa/ipu3/algorithms/awb.cpp\n> > +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> > @@ -172,8 +172,10 @@ 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> > +\t/* The grid is aligned to the next multiple of 4 */\n\ns/grid/grid width/\n\n> > +\tstride_ = (grid.width + 3) / 4 * 4;\n> \n> Feels like something that could have been handled by an alignedUp macro.\n> We have that in geometry.h but for Size ...\n> \n> So perhaps we can just open code it (unless anyone knows of a\n> std:: macro that does this?)\n\nutils::alignUp()\n\n> Reviewed-by: Kieran Bingham \n> \n> > -\tcellsPerZoneX_ = round(grid.width / static_cast(kAwbStatsSizeX));\n> > +\tcellsPerZoneX_ = round(stride_ / static_cast(kAwbStatsSizeX));\n\nThis isn't right, you will end up using the padding cells in your\ncalculations.\n\n> > \tcellsPerZoneY_ = round(grid.height / static_cast(kAwbStatsSizeY));\n> > \n> > \t/*\n> > @@ -234,8 +236,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 +244,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 +319,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 +336,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_;","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 63F27BDC71\n\tfor ;\n\tWed, 29 Sep 2021 12:44:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89730691AA;\n\tWed, 29 Sep 2021 14:44:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BBE6469185\n\tfor ;\n\tWed, 29 Sep 2021 14:44:09 +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 1BF153F0;\n\tWed, 29 Sep 2021 14:44:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lPkAw17x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632919449;\n\tbh=2MH3inKR2JMXcoZ+4iGR2vt99eeriyPZb5JFcXMMFBE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lPkAw17xnYKdeLXozwRdE9/Wj3KpJxnWgA7YfSpYzkBKsCZqRIjtUDc1hx8VLMskV\n\tY65gLAqw4m3+qigIwJk4BBaqaiAo/HCb5hG2/vGn1ztsv2nykCxZJC3TorUL6eubZq\n\tSXX5GNamthrwg14n49p55celP0K0Lm3+teQq8rPw=","Date":"Wed, 29 Sep 2021 15:44:07 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-10-jeanmichel.hautbois@ideasonboard.com>\n\t<20210928160242.7vkawzir72oug7no@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210928160242.7vkawzir72oug7no@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":19997,"web_url":"https://patchwork.libcamera.org/comment/19997/","msgid":"<16e6fc83-58c9-a9ee-ce07-78de6089fec7@ideasonboard.com>","date":"2021-09-29T12:47:15","subject":"Re: [libcamera-devel] [PATCH 09/12] ipa: ipu3: awb: Use the line\n\tstride for the stats","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 29/09/2021 14:44, Laurent Pinchart wrote:\n> Hello,\n> \n> On Tue, Sep 28, 2021 at 05:02:42PM +0100, Kieran Bingham wrote:\n>> On Thu, Sep 23, 2021 at 10:16:22AM +0200, Jean-Michel Hautbois wrote:\n>>> The statistics buffer 'ipu3_uapi_awb_raw_buffer' stores the ImgU\n>>> calculation results in a buffer aligned to a multiple of 4. The AWB loop\n> \n> s/aligned to a multiple of 4/aligned horizontally to a multiple of 4 cells/\n> \n>>> should take care of it to add the proper offset between lines and avoid\n>>> any staircase effect.\n>>>\n>>> It is now no more required to pass the grid configuration context to the\n>>\n>> \"It is no longer required to ...\"\n>>\n>> That's a nice perk indeed.\n>>\n>>> private functions called from process() which simplifies the code flow.\n>>>\n>>> Signed-off-by: Jean-Michel Hautbois \n>>> ---\n>>> src/ipa/ipu3/algorithms/awb.cpp | 16 ++++++++--------\n>>> src/ipa/ipu3/algorithms/awb.h | 7 +++----\n>>> 2 files changed, 11 insertions(+), 12 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n>>> index 8a926691..a5391653 100644\n>>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n>>> @@ -172,8 +172,10 @@ 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>>> +\t/* The grid is aligned to the next multiple of 4 */\n> \n> s/grid/grid width/\n> \n>>> +\tstride_ = (grid.width + 3) / 4 * 4;\n>>\n>> Feels like something that could have been handled by an alignedUp macro.\n>> We have that in geometry.h but for Size ...\n>>\n>> So perhaps we can just open code it (unless anyone knows of a\n>> std:: macro that does this?)\n> \n> utils::alignUp()\n> \n>> Reviewed-by: Kieran Bingham \n>>\n>>> -\tcellsPerZoneX_ = round(grid.width / static_cast(kAwbStatsSizeX));\n>>> +\tcellsPerZoneX_ = round(stride_ / static_cast(kAwbStatsSizeX));\n> \n> This isn't right, you will end up using the padding cells in your\n> calculations.\n> \n\nThe padding cell is all black, so it will not be taken into account in\nAwb::generateZones() as the green value will be lower than\nkMinGreenLevelInZone.\n\n>>> \tcellsPerZoneY_ = round(grid.height / static_cast(kAwbStatsSizeY));\n>>> \n>>> \t/*\n>>> @@ -234,8 +236,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 +244,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 +319,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 +336,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>","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 5CBC0C3243\n\tfor ;\n\tWed, 29 Sep 2021 12:47:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94317691AA;\n\tWed, 29 Sep 2021 14:47:18 +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 B170869185\n\tfor ;\n\tWed, 29 Sep 2021 14:47:17 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:b4ce:851b:eca:f3fe])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3BF273F0;\n\tWed, 29 Sep 2021 14:47:17 +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=\"Jp/o4QNI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632919637;\n\tbh=2V2FRr0wt7VOrySXFv9FzQAm41WZtGpe6BEjtPLMDX0=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Jp/o4QNIPUKYjZStd5PwAXTnxeCWcmvazQaXChS6yzK4UyS1/vv7TDI+cvCCThPtz\n\t7Fhk1095dV4RDJ1c8Ztf74HMsdCWD1H6f/+3XmlOCrx7+rBW17N5O0Di/tkE55LjNO\n\tfSW1FaLcKdxiyjcL0y7J4JYxW8JN0KlCfrPNbZ9w=","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-10-jeanmichel.hautbois@ideasonboard.com>\n\t<20210928160242.7vkawzir72oug7no@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<16e6fc83-58c9-a9ee-ce07-78de6089fec7@ideasonboard.com>","Date":"Wed, 29 Sep 2021 14:47:15 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":19999,"web_url":"https://patchwork.libcamera.org/comment/19999/","msgid":"","date":"2021-09-29T12:52:10","subject":"Re: [libcamera-devel] [PATCH 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":"On Wed, Sep 29, 2021 at 02:47:15PM +0200, Jean-Michel Hautbois wrote:\n> On 29/09/2021 14:44, Laurent Pinchart wrote:\n> > On Tue, Sep 28, 2021 at 05:02:42PM +0100, Kieran Bingham wrote:\n> >> On Thu, Sep 23, 2021 at 10:16:22AM +0200, Jean-Michel Hautbois wrote:\n> >>> The statistics buffer 'ipu3_uapi_awb_raw_buffer' stores the ImgU\n> >>> calculation results in a buffer aligned to a multiple of 4. The AWB loop\n> > \n> > s/aligned to a multiple of 4/aligned horizontally to a multiple of 4 cells/\n> > \n> >>> should take care of it to add the proper offset between lines and avoid\n> >>> any staircase effect.\n> >>>\n> >>> It is now no more required to pass the grid configuration context to the\n> >>\n> >> \"It is no longer required to ...\"\n> >>\n> >> That's a nice perk indeed.\n> >>\n> >>> private functions called from process() which simplifies the code flow.\n> >>>\n> >>> Signed-off-by: Jean-Michel Hautbois \n> >>> ---\n> >>> src/ipa/ipu3/algorithms/awb.cpp | 16 ++++++++--------\n> >>> src/ipa/ipu3/algorithms/awb.h | 7 +++----\n> >>> 2 files changed, 11 insertions(+), 12 deletions(-)\n> >>>\n> >>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> >>> index 8a926691..a5391653 100644\n> >>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> >>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> >>> @@ -172,8 +172,10 @@ 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> >>> +\t/* The grid is aligned to the next multiple of 4 */\n> > \n> > s/grid/grid width/\n> > \n> >>> +\tstride_ = (grid.width + 3) / 4 * 4;\n> >>\n> >> Feels like something that could have been handled by an alignedUp macro.\n> >> We have that in geometry.h but for Size ...\n> >>\n> >> So perhaps we can just open code it (unless anyone knows of a\n> >> std:: macro that does this?)\n> > \n> > utils::alignUp()\n> > \n> >> Reviewed-by: Kieran Bingham \n> >>\n> >>> -\tcellsPerZoneX_ = round(grid.width / static_cast(kAwbStatsSizeX));\n> >>> +\tcellsPerZoneX_ = round(stride_ / static_cast(kAwbStatsSizeX));\n> > \n> > This isn't right, you will end up using the padding cells in your\n> > calculations.\n> \n> The padding cell is all black, so it will not be taken into account in\n> Awb::generateZones() as the green value will be lower than\n> kMinGreenLevelInZone.\n\nThat's not a documented guarantee, I don't think we should rely on this.\n\n> >>> \tcellsPerZoneY_ = round(grid.height / static_cast(kAwbStatsSizeY));\n> >>> \n> >>> \t/*\n> >>> @@ -234,8 +236,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 +244,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 +319,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 +336,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_;","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 972FFBDC71\n\tfor ;\n\tWed, 29 Sep 2021 12:52:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67849691AC;\n\tWed, 29 Sep 2021 14:52:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D8C069185\n\tfor ;\n\tWed, 29 Sep 2021 14:52:12 +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 0B94F3F0;\n\tWed, 29 Sep 2021 14:52:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lVqQ6KpC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632919932;\n\tbh=MOOfeeS/NV129f48+3yYJea1x8dk+LaMsqCP8N95yU8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lVqQ6KpCIzSt/77/qKf0chSCKYYmpy2uT+ibxZ88MOp6jJ/jnSNDnvb4FngvMdn2P\n\tUjw+vlRVnr8CQ3YqCldBA1a/XRzJKsI0Ign/uGKRERVkUJb+XtBmPRKNVsRusIqeL4\n\tVSzrvP+3WBNLk7lKfhI67eaqZ0YsYFy9RyHj0sG0=","Date":"Wed, 29 Sep 2021 15:52:10 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-10-jeanmichel.hautbois@ideasonboard.com>\n\t<20210928160242.7vkawzir72oug7no@ideasonboard.com>\n\t\n\t<16e6fc83-58c9-a9ee-ce07-78de6089fec7@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<16e6fc83-58c9-a9ee-ce07-78de6089fec7@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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\" "}}]