[{"id":18754,"web_url":"https://patchwork.libcamera.org/comment/18754/","msgid":"<58a16b6a-4fa3-64d2-3a92-2a8391be01fd@ideasonboard.com>","date":"2021-08-13T08:48:37","subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 12/08/2021 17:52, Jean-Michel Hautbois wrote:\n> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a\n> EventFillParams. When this is called, the algorithms should fill every\n> field in the parameter structure they need to update.\n> \n> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let\n\nI don't think we need to demonstrate it, we 'implement' it ;-)\n\n> the Grid algorithm update the grid field.\n> This leads to removing this same update from the Awb algorithm.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/algorithm.h | 2 ++\n> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++\n> src/ipa/ipu3/algorithms/grid.h | 1 +\n> src/ipa/ipu3/ipu3.cpp | 3 +++\n> src/ipa/ipu3/ipu3_awb.cpp | 1 -\n> 5 files changed, 12 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> index c1b37276..b571f55a 100644\n> --- a/src/ipa/ipu3/algorithms/algorithm.h\n> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n> @@ -26,6 +26,8 @@ public:\n> \t{\n> \t\treturn 0;\n> \t}\n> +\n> +\tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {}\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp\n> index 3578f41b..e42a760d 100644\n> --- a/src/ipa/ipu3/algorithms/grid.cpp\n> +++ b/src/ipa/ipu3/algorithms/grid.cpp\n> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> \treturn 0;\n> }\n> \n> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms)\n> +{\n> +\t/* Update the IPU3 parameters with new calculated grid */\n\nI don't think it's a new calculated grid, as the grid is only calculated\nduring the configure phase.\n\nI would say:\n\n Update the IPU3 parameters with the configured grid\n\n\nReviewed-by: Kieran Bingham \n\n> +\tparams.acc_param.awb.config.grid = context.configuration.grid.bdsGrid;\n> +}\n> +\n> } /* namespace ipa::ipu3::algorithms */\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h\n> index b4a51b42..89399bd2 100644\n> --- a/src/ipa/ipu3/algorithms/grid.h\n> +++ b/src/ipa/ipu3/algorithms/grid.h\n> @@ -19,6 +19,7 @@ public:\n> \t~Grid() = default;\n> \n> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> +\tvoid prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;\n> };\n> \n> } /* namespace ipa::ipu3::algorithms */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index ef7fec86..394a7a45 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> \n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> +\tfor (auto const &algo : algorithms_)\n> +\t\talgo->prepare(context_, params_);\n> +\n> \tif (agcAlgo_->updateControls())\n> \t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> \n> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> index 4ee5ee6f..911760b3 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> \tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> \tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> \tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> -\tparams.acc_param.awb.config.grid = bdsGrid;\n> \tawbGrid_ = bdsGrid;\n> \n> \tparams.use.acc_bnr = 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 EBD0FC3240\n\tfor ;\n\tFri, 13 Aug 2021 08:48:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6001C6888E;\n\tFri, 13 Aug 2021 10:48:42 +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 3A36960263\n\tfor ;\n\tFri, 13 Aug 2021 10:48:41 +0200 (CEST)","from [192.168.0.20]\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 AC8D9EE;\n\tFri, 13 Aug 2021 10:48:40 +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=\"l1DCAbNx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628844520;\n\tbh=qFjWN3EFLfQ679pswX3Xcv8GvN3SD5JonMUvZX0Jq9M=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=l1DCAbNxvK305eKrzBg4j0YaUbrR1snOkzCwmkeVMrWWoTSZ1NWfs2PutXshlaolp\n\tvOVGD0j13rcNVuGvozE+RI5aoqUmAZoBUT/dJgFsttfPneT0BhsHIvm024CUNsX1MV\n\t62FAmP/AGtMGxXDnIUw9hc3mramd4UhTtmPhMFm0=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<58a16b6a-4fa3-64d2-3a92-2a8391be01fd@ideasonboard.com>","Date":"Fri, 13 Aug 2021 09:48:37 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18805,"web_url":"https://patchwork.libcamera.org/comment/18805/","msgid":"","date":"2021-08-15T18:02:35","subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote:\n> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a\n> EventFillParams. When this is called, the algorithms should fill every\n> field in the parameter structure they need to update.\n> \n> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let\n> the Grid algorithm update the grid field.\n> This leads to removing this same update from the Awb algorithm.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/algorithm.h | 2 ++\n> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++\n> src/ipa/ipu3/algorithms/grid.h | 1 +\n> src/ipa/ipu3/ipu3.cpp | 3 +++\n> src/ipa/ipu3/ipu3_awb.cpp | 1 -\n> 5 files changed, 12 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> index c1b37276..b571f55a 100644\n> --- a/src/ipa/ipu3/algorithms/algorithm.h\n> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n> @@ -26,6 +26,8 @@ public:\n> \t{\n> \t\treturn 0;\n> \t}\n> +\n> +\tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {}\n\nLine wrap.\n\nCould you add a patch after 02/10 that adds all functions (configure(),\nprepare() and process()) to the Algorithm class, and call them in the\nIPAIPU3 class as appropriate ? That patch should include the\ndocumentation of the functions, to explain how they're used. After that\nthe rest of the series can begin moving the algorithms to the new class.\n\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp\n> index 3578f41b..e42a760d 100644\n> --- a/src/ipa/ipu3/algorithms/grid.cpp\n> +++ b/src/ipa/ipu3/algorithms/grid.cpp\n> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> \treturn 0;\n> }\n> \n> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms)\n> +{\n> +\t/* Update the IPU3 parameters with new calculated grid */\n> +\tparams.acc_param.awb.config.grid = context.configuration.grid.bdsGrid;\n\nAnd this is where the spaghetti dish begins :-( It should be the\nresponsibility of the AWB algorithm to set params.acc_param.awb.\n\n> +}\n> +\n> } /* namespace ipa::ipu3::algorithms */\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h\n> index b4a51b42..89399bd2 100644\n> --- a/src/ipa/ipu3/algorithms/grid.h\n> +++ b/src/ipa/ipu3/algorithms/grid.h\n> @@ -19,6 +19,7 @@ public:\n> \t~Grid() = default;\n> \n> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> +\tvoid prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;\n> };\n> \n> } /* namespace ipa::ipu3::algorithms */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index ef7fec86..394a7a45 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> \n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> +\tfor (auto const &algo : algorithms_)\n> +\t\talgo->prepare(context_, params_);\n> +\n> \tif (agcAlgo_->updateControls())\n> \t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> \n> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> index 4ee5ee6f..911760b3 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> \tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> \tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> \tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> -\tparams.acc_param.awb.config.grid = bdsGrid;\n> \tawbGrid_ = bdsGrid;\n> \n> \tparams.use.acc_bnr = 1;","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 9B783BD87D\n\tfor ;\n\tSun, 15 Aug 2021 18:02:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE9B568892;\n\tSun, 15 Aug 2021 20:02:41 +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 85B9068889\n\tfor ;\n\tSun, 15 Aug 2021 20:02:40 +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 F1E1D2C5;\n\tSun, 15 Aug 2021 20:02:39 +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=\"WyLvc/0j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629050560;\n\tbh=emZFDsL8KDmdDAo6On2wbVLPjlhHM8nPLLm8cmTcfmo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WyLvc/0jJGZnk9eSrHj+ree3dl85E5e8UfdH8UkIQSRK+kM096lRY1WmKfDM8n6k0\n\ttgViK8rFjhUrpJIUeosOia9oeqrlUODc5hIYWCHSGninYCrTj5hzBzGGx5EFYeVbkd\n\tq5fYB0/uHHoT4Drm7LTVsJubgKpOGcFV1jy2RAlg=","Date":"Sun, 15 Aug 2021 21:02:35 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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":18820,"web_url":"https://patchwork.libcamera.org/comment/18820/","msgid":"<63951ba9-9231-5b82-cba5-48fb88cea833@ideasonboard.com>","date":"2021-08-16T08:01:14","subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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 15/08/2021 20:02, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote:\n>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a\n>> EventFillParams. When this is called, the algorithms should fill every\n>> field in the parameter structure they need to update.\n>>\n>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let\n>> the Grid algorithm update the grid field.\n>> This leads to removing this same update from the Awb algorithm.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/algorithms/algorithm.h | 2 ++\n>> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++\n>> src/ipa/ipu3/algorithms/grid.h | 1 +\n>> src/ipa/ipu3/ipu3.cpp | 3 +++\n>> src/ipa/ipu3/ipu3_awb.cpp | 1 -\n>> 5 files changed, 12 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n>> index c1b37276..b571f55a 100644\n>> --- a/src/ipa/ipu3/algorithms/algorithm.h\n>> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n>> @@ -26,6 +26,8 @@ public:\n>> \t{\n>> \t\treturn 0;\n>> \t}\n>> +\n>> +\tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {}\n> \n> Line wrap.\n> \n> Could you add a patch after 02/10 that adds all functions (configure(),\n> prepare() and process()) to the Algorithm class, and call them in the\n> IPAIPU3 class as appropriate ? That patch should include the\n> documentation of the functions, to explain how they're used. After that\n> the rest of the series can begin moving the algorithms to the new class.\n> \n\nI did that in the first place, and it was a bit of a heavy patch. So I\ndecided to split it to introduce each of configure/prepare/process calls\nstep by step...\n\n>> };\n>> \n>> } /* namespace ipa::ipu3 */\n>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp\n>> index 3578f41b..e42a760d 100644\n>> --- a/src/ipa/ipu3/algorithms/grid.cpp\n>> +++ b/src/ipa/ipu3/algorithms/grid.cpp\n>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>> \treturn 0;\n>> }\n>> \n>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms)\n>> +{\n>> +\t/* Update the IPU3 parameters with new calculated grid */\n>> +\tparams.acc_param.awb.config.grid = context.configuration.grid.bdsGrid;\n> \n> And this is where the spaghetti dish begins :-( It should be the\n> responsibility of the AWB algorithm to set params.acc_param.awb.\n> \n\nYes, I moved it at least 5 times while writing the patches as I could\nnot decide where it should be :-p.\n\n>> +}\n>> +\n>> } /* namespace ipa::ipu3::algorithms */\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h\n>> index b4a51b42..89399bd2 100644\n>> --- a/src/ipa/ipu3/algorithms/grid.h\n>> +++ b/src/ipa/ipu3/algorithms/grid.h\n>> @@ -19,6 +19,7 @@ public:\n>> \t~Grid() = default;\n>> \n>> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>> +\tvoid prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;\n>> };\n>> \n>> } /* namespace ipa::ipu3::algorithms */\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index ef7fec86..394a7a45 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>> \n>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>> {\n>> +\tfor (auto const &algo : algorithms_)\n>> +\t\talgo->prepare(context_, params_);\n>> +\n>> \tif (agcAlgo_->updateControls())\n>> \t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n>> \n>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n>> index 4ee5ee6f..911760b3 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n>> \tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n>> \tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n>> \tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n>> -\tparams.acc_param.awb.config.grid = bdsGrid;\n>> \tawbGrid_ = bdsGrid;\n>> \n>> \tparams.use.acc_bnr = 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 E1EA7BD87D\n\tfor ;\n\tMon, 16 Aug 2021 08:01:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43F3A68891;\n\tMon, 16 Aug 2021 10:01: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 266A860260\n\tfor ;\n\tMon, 16 Aug 2021 10:01:17 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:8b4d:65fb:4074:a212])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B49E28F;\n\tMon, 16 Aug 2021 10:01:16 +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=\"E37XabLk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629100876;\n\tbh=FVBbIVLhgbz0h4RBpVCZQqKUy6ZxypJQ1K5DcLFuMXM=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=E37XabLkcwFKp6z12rcIapleNKOJd9KBZ7HH1NAz+fBPgQ9g+xmDaPNMXyufXcI/h\n\tepfDKkVncf0xcCna0iqRgsF1K3v0gxNpvO5cY2b7MIVtRPE3OmoCm5wI84J/zjt8Nt\n\tHAesc6PAhAVR3HtcMT89v9sQ2FbHn7fhgFXxS5G8=","To":"Laurent Pinchart ","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<63951ba9-9231-5b82-cba5-48fb88cea833@ideasonboard.com>","Date":"Mon, 16 Aug 2021 10:01:14 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.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 v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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":18824,"web_url":"https://patchwork.libcamera.org/comment/18824/","msgid":"","date":"2021-08-16T09:41:47","subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote:\n> On 15/08/2021 20:02, Laurent Pinchart wrote:\n> > On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote:\n> >> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a\n> >> EventFillParams. When this is called, the algorithms should fill every\n> >> field in the parameter structure they need to update.\n> >>\n> >> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let\n> >> the Grid algorithm update the grid field.\n> >> This leads to removing this same update from the Awb algorithm.\n> >>\n> >> Signed-off-by: Jean-Michel Hautbois \n> >> ---\n> >> src/ipa/ipu3/algorithms/algorithm.h | 2 ++\n> >> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++\n> >> src/ipa/ipu3/algorithms/grid.h | 1 +\n> >> src/ipa/ipu3/ipu3.cpp | 3 +++\n> >> src/ipa/ipu3/ipu3_awb.cpp | 1 -\n> >> 5 files changed, 12 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> >> index c1b37276..b571f55a 100644\n> >> --- a/src/ipa/ipu3/algorithms/algorithm.h\n> >> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n> >> @@ -26,6 +26,8 @@ public:\n> >> \t{\n> >> \t\treturn 0;\n> >> \t}\n> >> +\n> >> +\tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {}\n> > \n> > Line wrap.\n> > \n> > Could you add a patch after 02/10 that adds all functions (configure(),\n> > prepare() and process()) to the Algorithm class, and call them in the\n> > IPAIPU3 class as appropriate ? That patch should include the\n> > documentation of the functions, to explain how they're used. After that\n> > the rest of the series can begin moving the algorithms to the new class.\n> \n> I did that in the first place, and it was a bit of a heavy patch. So I\n> decided to split it to introduce each of configure/prepare/process calls\n> step by step...\n\nThe issue with this approach is that it's harder to have an overview of\nthe proposed API. Is it *that* heavy to define a class with three\nabstract functions and their documentation ?\n\n> >> };\n> >> \n> >> } /* namespace ipa::ipu3 */\n> >> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp\n> >> index 3578f41b..e42a760d 100644\n> >> --- a/src/ipa/ipu3/algorithms/grid.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/grid.cpp\n> >> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> >> \treturn 0;\n> >> }\n> >> \n> >> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms)\n> >> +{\n> >> +\t/* Update the IPU3 parameters with new calculated grid */\n> >> +\tparams.acc_param.awb.config.grid = context.configuration.grid.bdsGrid;\n> > \n> > And this is where the spaghetti dish begins :-( It should be the\n> > responsibility of the AWB algorithm to set params.acc_param.awb.\n> \n> Yes, I moved it at least 5 times while writing the patches as I could\n> not decide where it should be :-p.\n\nAs commented in patch 03/10, I don't think \"grid\" should be an\nalgorithm.\n\n> >> +}\n> >> +\n> >> } /* namespace ipa::ipu3::algorithms */\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h\n> >> index b4a51b42..89399bd2 100644\n> >> --- a/src/ipa/ipu3/algorithms/grid.h\n> >> +++ b/src/ipa/ipu3/algorithms/grid.h\n> >> @@ -19,6 +19,7 @@ public:\n> >> \t~Grid() = default;\n> >> \n> >> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> >> +\tvoid prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;\n> >> };\n> >> \n> >> } /* namespace ipa::ipu3::algorithms */\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index ef7fec86..394a7a45 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> >> \n> >> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >> {\n> >> +\tfor (auto const &algo : algorithms_)\n> >> +\t\talgo->prepare(context_, params_);\n> >> +\n> >> \tif (agcAlgo_->updateControls())\n> >> \t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> >> \n> >> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> >> index 4ee5ee6f..911760b3 100644\n> >> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> >> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> >> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> >> \tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> >> \tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> >> \tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> >> -\tparams.acc_param.awb.config.grid = bdsGrid;\n> >> \tawbGrid_ = bdsGrid;\n> >> \n> >> \tparams.use.acc_bnr = 1;","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 40A8BBD87D\n\tfor ;\n\tMon, 16 Aug 2021 09:41:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B028468893;\n\tMon, 16 Aug 2021 11:41:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00B5A60260\n\tfor ;\n\tMon, 16 Aug 2021 11:41:53 +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 791CF3E5;\n\tMon, 16 Aug 2021 11:41:53 +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=\"YprSc58u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629106913;\n\tbh=vZG3X6etiKOR7POICxwJXABihymllP9GeOMrp/YNcV0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YprSc58u0mXmGziXSei8H3WJPk+nHDKmDs7hCw21u6CSEc8IbSkqmRLql+xty04LU\n\tQPNc6vkCE9191PB00VN1mx7dMZ/btpXzCMYCnQkGZ5krCrdtL0LMyqdOS7rhbWE8qe\n\tT0XgkDcepjROne3XPZdURJ1VsxTBFXeb6BO6lPFY=","Date":"Mon, 16 Aug 2021 12:41:47 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<63951ba9-9231-5b82-cba5-48fb88cea833@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<63951ba9-9231-5b82-cba5-48fb88cea833@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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":18825,"web_url":"https://patchwork.libcamera.org/comment/18825/","msgid":"<4aebf67c-f2cc-922d-6e64-32ae3482bc6b@ideasonboard.com>","date":"2021-08-16T09:43:54","subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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 16/08/2021 11:41, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote:\n>> On 15/08/2021 20:02, Laurent Pinchart wrote:\n>>> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote:\n>>>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a\n>>>> EventFillParams. When this is called, the algorithms should fill every\n>>>> field in the parameter structure they need to update.\n>>>>\n>>>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let\n>>>> the Grid algorithm update the grid field.\n>>>> This leads to removing this same update from the Awb algorithm.\n>>>>\n>>>> Signed-off-by: Jean-Michel Hautbois \n>>>> ---\n>>>> src/ipa/ipu3/algorithms/algorithm.h | 2 ++\n>>>> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++\n>>>> src/ipa/ipu3/algorithms/grid.h | 1 +\n>>>> src/ipa/ipu3/ipu3.cpp | 3 +++\n>>>> src/ipa/ipu3/ipu3_awb.cpp | 1 -\n>>>> 5 files changed, 12 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n>>>> index c1b37276..b571f55a 100644\n>>>> --- a/src/ipa/ipu3/algorithms/algorithm.h\n>>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n>>>> @@ -26,6 +26,8 @@ public:\n>>>> \t{\n>>>> \t\treturn 0;\n>>>> \t}\n>>>> +\n>>>> +\tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {}\n>>>\n>>> Line wrap.\n>>>\n>>> Could you add a patch after 02/10 that adds all functions (configure(),\n>>> prepare() and process()) to the Algorithm class, and call them in the\n>>> IPAIPU3 class as appropriate ? That patch should include the\n>>> documentation of the functions, to explain how they're used. After that\n>>> the rest of the series can begin moving the algorithms to the new class.\n>>\n>> I did that in the first place, and it was a bit of a heavy patch. So I\n>> decided to split it to introduce each of configure/prepare/process calls\n>> step by step...\n> \n> The issue with this approach is that it's harder to have an overview of\n> the proposed API. Is it *that* heavy to define a class with three\n> abstract functions and their documentation ?\n> \n\nNo, but as we already have a process() call in AGC it must be rewritten\naccordingly.\n\n>>>> };\n>>>> \n>>>> } /* namespace ipa::ipu3 */\n>>>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp\n>>>> index 3578f41b..e42a760d 100644\n>>>> --- a/src/ipa/ipu3/algorithms/grid.cpp\n>>>> +++ b/src/ipa/ipu3/algorithms/grid.cpp\n>>>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms)\n>>>> +{\n>>>> +\t/* Update the IPU3 parameters with new calculated grid */\n>>>> +\tparams.acc_param.awb.config.grid = context.configuration.grid.bdsGrid;\n>>>\n>>> And this is where the spaghetti dish begins :-( It should be the\n>>> responsibility of the AWB algorithm to set params.acc_param.awb.\n>>\n>> Yes, I moved it at least 5 times while writing the patches as I could\n>> not decide where it should be :-p.\n> \n> As commented in patch 03/10, I don't think \"grid\" should be an\n> algorithm.\n> \n>>>> +}\n>>>> +\n>>>> } /* namespace ipa::ipu3::algorithms */\n>>>> \n>>>> } /* namespace libcamera */\n>>>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h\n>>>> index b4a51b42..89399bd2 100644\n>>>> --- a/src/ipa/ipu3/algorithms/grid.h\n>>>> +++ b/src/ipa/ipu3/algorithms/grid.h\n>>>> @@ -19,6 +19,7 @@ public:\n>>>> \t~Grid() = default;\n>>>> \n>>>> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>>>> +\tvoid prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;\n>>>> };\n>>>> \n>>>> } /* namespace ipa::ipu3::algorithms */\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index ef7fec86..394a7a45 100644\n>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>>>> \n>>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>>>> {\n>>>> +\tfor (auto const &algo : algorithms_)\n>>>> +\t\talgo->prepare(context_, params_);\n>>>> +\n>>>> \tif (agcAlgo_->updateControls())\n>>>> \t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n>>>> \n>>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n>>>> index 4ee5ee6f..911760b3 100644\n>>>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>>>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n>>>> \tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n>>>> \tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n>>>> \tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n>>>> -\tparams.acc_param.awb.config.grid = bdsGrid;\n>>>> \tawbGrid_ = bdsGrid;\n>>>> \n>>>> \tparams.use.acc_bnr = 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 07855BD87C\n\tfor ;\n\tMon, 16 Aug 2021 09:43:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72CB168891;\n\tMon, 16 Aug 2021 11:43:58 +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 36A3860260\n\tfor ;\n\tMon, 16 Aug 2021 11:43:57 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:8b4d:65fb:4074:a212])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC52F3E5;\n\tMon, 16 Aug 2021 11:43:56 +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=\"EoWmoOyz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629107036;\n\tbh=kD6osB2ak61u01tQVzklc3lLIUxu42clF39oHfzsKGA=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=EoWmoOyzPA+zKchiA5vm0czSaHnCHbM8GtJoxeHexjOszcwf6USOwVf5vSl3U/cC6\n\tq1ByrLZ6GLH2ho2Wxx2N5q1Xh7mDw/3wjvq5f55gR+Gy6q6JS0oiEGUVxYa5QntnZQ\n\tZH8uGY0WddWnQjcx4y8L5oGCp2X+QXBZ1KB8MZTM=","To":"Laurent Pinchart ","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<63951ba9-9231-5b82-cba5-48fb88cea833@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<4aebf67c-f2cc-922d-6e64-32ae3482bc6b@ideasonboard.com>","Date":"Mon, 16 Aug 2021 11:43:54 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.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 v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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":18826,"web_url":"https://patchwork.libcamera.org/comment/18826/","msgid":"","date":"2021-08-16T09:49:33","subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Mon, Aug 16, 2021 at 11:43:54AM +0200, Jean-Michel Hautbois wrote:\n> On 16/08/2021 11:41, Laurent Pinchart wrote:\n> > On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote:\n> >> On 15/08/2021 20:02, Laurent Pinchart wrote:\n> >>> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote:\n> >>>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a\n> >>>> EventFillParams. When this is called, the algorithms should fill every\n> >>>> field in the parameter structure they need to update.\n> >>>>\n> >>>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let\n> >>>> the Grid algorithm update the grid field.\n> >>>> This leads to removing this same update from the Awb algorithm.\n> >>>>\n> >>>> Signed-off-by: Jean-Michel Hautbois \n> >>>> ---\n> >>>> src/ipa/ipu3/algorithms/algorithm.h | 2 ++\n> >>>> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++\n> >>>> src/ipa/ipu3/algorithms/grid.h | 1 +\n> >>>> src/ipa/ipu3/ipu3.cpp | 3 +++\n> >>>> src/ipa/ipu3/ipu3_awb.cpp | 1 -\n> >>>> 5 files changed, 12 insertions(+), 1 deletion(-)\n> >>>>\n> >>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> >>>> index c1b37276..b571f55a 100644\n> >>>> --- a/src/ipa/ipu3/algorithms/algorithm.h\n> >>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n> >>>> @@ -26,6 +26,8 @@ public:\n> >>>> \t{\n> >>>> \t\treturn 0;\n> >>>> \t}\n> >>>> +\n> >>>> +\tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {}\n> >>>\n> >>> Line wrap.\n> >>>\n> >>> Could you add a patch after 02/10 that adds all functions (configure(),\n> >>> prepare() and process()) to the Algorithm class, and call them in the\n> >>> IPAIPU3 class as appropriate ? That patch should include the\n> >>> documentation of the functions, to explain how they're used. After that\n> >>> the rest of the series can begin moving the algorithms to the new class.\n> >>\n> >> I did that in the first place, and it was a bit of a heavy patch. So I\n> >> decided to split it to introduce each of configure/prepare/process calls\n> >> step by step...\n> > \n> > The issue with this approach is that it's harder to have an overview of\n> > the proposed API. Is it *that* heavy to define a class with three\n> > abstract functions and their documentation ?\n> \n> No, but as we already have a process() call in AGC it must be rewritten\n> accordingly.\n\nThe existing process() function takes different parameters, so the\ncompiler shouldn't complain. Worst case it could be renamed in a small\npatch at the beginning of the series to move it out of the way. I agree\nthat adapting the algorithm is best done in a patch separate from the\nintroduction of the new Algorithm structure.\n\n> >>>> };\n> >>>> \n> >>>> } /* namespace ipa::ipu3 */\n> >>>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp\n> >>>> index 3578f41b..e42a760d 100644\n> >>>> --- a/src/ipa/ipu3/algorithms/grid.cpp\n> >>>> +++ b/src/ipa/ipu3/algorithms/grid.cpp\n> >>>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> >>>> \treturn 0;\n> >>>> }\n> >>>> \n> >>>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms)\n> >>>> +{\n> >>>> +\t/* Update the IPU3 parameters with new calculated grid */\n> >>>> +\tparams.acc_param.awb.config.grid = context.configuration.grid.bdsGrid;\n> >>>\n> >>> And this is where the spaghetti dish begins :-( It should be the\n> >>> responsibility of the AWB algorithm to set params.acc_param.awb.\n> >>\n> >> Yes, I moved it at least 5 times while writing the patches as I could\n> >> not decide where it should be :-p.\n> > \n> > As commented in patch 03/10, I don't think \"grid\" should be an\n> > algorithm.\n> > \n> >>>> +}\n> >>>> +\n> >>>> } /* namespace ipa::ipu3::algorithms */\n> >>>> \n> >>>> } /* namespace libcamera */\n> >>>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h\n> >>>> index b4a51b42..89399bd2 100644\n> >>>> --- a/src/ipa/ipu3/algorithms/grid.h\n> >>>> +++ b/src/ipa/ipu3/algorithms/grid.h\n> >>>> @@ -19,6 +19,7 @@ public:\n> >>>> \t~Grid() = default;\n> >>>> \n> >>>> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> >>>> +\tvoid prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;\n> >>>> };\n> >>>> \n> >>>> } /* namespace ipa::ipu3::algorithms */\n> >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>>> index ef7fec86..394a7a45 100644\n> >>>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> >>>> \n> >>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >>>> {\n> >>>> +\tfor (auto const &algo : algorithms_)\n> >>>> +\t\talgo->prepare(context_, params_);\n> >>>> +\n> >>>> \tif (agcAlgo_->updateControls())\n> >>>> \t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> >>>> \n> >>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> >>>> index 4ee5ee6f..911760b3 100644\n> >>>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> >>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> >>>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> >>>> \tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> >>>> \tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> >>>> \tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> >>>> -\tparams.acc_param.awb.config.grid = bdsGrid;\n> >>>> \tawbGrid_ = bdsGrid;\n> >>>> \n> >>>> \tparams.use.acc_bnr = 1;","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 E894FBD87D\n\tfor ;\n\tMon, 16 Aug 2021 09:49:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5AFF068890;\n\tMon, 16 Aug 2021 11:49:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0557360260\n\tfor ;\n\tMon, 16 Aug 2021 11:49:39 +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 7DBC63E5;\n\tMon, 16 Aug 2021 11:49:38 +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=\"IPGqJhQK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629107378;\n\tbh=UovLHqm3jdPzyeIpNep7VYSjTXvOgAZeGbwnECrmz9g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IPGqJhQKVIaVMmtjbyTO90pxyw1NNOs3e1cq6jhDlAgGOIOCq2BinXN3LIXLTjZhm\n\t8bD11O5Hlfe1HUP8LjeTUVxSoz6EwdDWf+nVDeAGNadZH3sON+W3FVAbVPNqnExwTy\n\tIo3rH1pCdmPJiCAVTClAz9JTwPWkNUsBVl9CCKZ4=","Date":"Mon, 16 Aug 2021 12:49:33 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<63951ba9-9231-5b82-cba5-48fb88cea833@ideasonboard.com>\n\t\n\t<4aebf67c-f2cc-922d-6e64-32ae3482bc6b@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<4aebf67c-f2cc-922d-6e64-32ae3482bc6b@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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":18895,"web_url":"https://patchwork.libcamera.org/comment/18895/","msgid":"<44894d11-e27a-927e-2aef-4d9f40ee0b93@ideasonboard.com>","date":"2021-08-18T06:47:28","subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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 16/08/2021 11:49, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> On Mon, Aug 16, 2021 at 11:43:54AM +0200, Jean-Michel Hautbois wrote:\n>> On 16/08/2021 11:41, Laurent Pinchart wrote:\n>>> On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote:\n>>>> On 15/08/2021 20:02, Laurent Pinchart wrote:\n>>>>> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote:\n>>>>>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a\n>>>>>> EventFillParams. When this is called, the algorithms should fill every\n>>>>>> field in the parameter structure they need to update.\n>>>>>>\n>>>>>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let\n>>>>>> the Grid algorithm update the grid field.\n>>>>>> This leads to removing this same update from the Awb algorithm.\n>>>>>>\n>>>>>> Signed-off-by: Jean-Michel Hautbois \n>>>>>> ---\n>>>>>> src/ipa/ipu3/algorithms/algorithm.h | 2 ++\n>>>>>> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++\n>>>>>> src/ipa/ipu3/algorithms/grid.h | 1 +\n>>>>>> src/ipa/ipu3/ipu3.cpp | 3 +++\n>>>>>> src/ipa/ipu3/ipu3_awb.cpp | 1 -\n>>>>>> 5 files changed, 12 insertions(+), 1 deletion(-)\n>>>>>>\n>>>>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n>>>>>> index c1b37276..b571f55a 100644\n>>>>>> --- a/src/ipa/ipu3/algorithms/algorithm.h\n>>>>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n>>>>>> @@ -26,6 +26,8 @@ public:\n>>>>>> \t{\n>>>>>> \t\treturn 0;\n>>>>>> \t}\n>>>>>> +\n>>>>>> +\tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {}\n>>>>>\n>>>>> Line wrap.\n>>>>>\n>>>>> Could you add a patch after 02/10 that adds all functions (configure(),\n>>>>> prepare() and process()) to the Algorithm class, and call them in the\n>>>>> IPAIPU3 class as appropriate ? That patch should include the\n>>>>> documentation of the functions, to explain how they're used. After that\n>>>>> the rest of the series can begin moving the algorithms to the new class.\n>>>>\n>>>> I did that in the first place, and it was a bit of a heavy patch. So I\n>>>> decided to split it to introduce each of configure/prepare/process calls\n>>>> step by step...\n>>>\n>>> The issue with this approach is that it's harder to have an overview of\n>>> the proposed API. Is it *that* heavy to define a class with three\n>>> abstract functions and their documentation ?\n>>\n>> No, but as we already have a process() call in AGC it must be rewritten\n>> accordingly.\n> \n> The existing process() function takes different parameters, so the\n> compiler shouldn't complain. Worst case it could be renamed in a small\n> patch at the beginning of the series to move it out of the way. I agree\n> that adapting the algorithm is best done in a patch separate from the\n> introduction of the new Algorithm structure.\n> \n\nThe compiler does not agree with you :-).\n\n../src/ipa/ipu3/ipu3_agc.h:33:7: error:\n'libcamera::ipa::ipu3::IPU3Agc::process' hides overloaded virtual\nfunction [-Werror,-Woverloaded-virtual]\n void process(const ipu3_uapi_stats_3a *stats, uint32_t\n&exposure, double &gain);\n ^\n../src/ipa/ipu3/algorithms/algorithm.h:26:15: note: hidden overloaded\nvirtual function 'libcamera::ipa::ipu3::Algorithm::process' declared\nhere: different number of parameters (2 vs 3)\n virtual void process(IPAContext &context, ipu3_uapi_stats_3a\n&stats);\n\n>>>>>> };\n>>>>>> \n>>>>>> } /* namespace ipa::ipu3 */\n>>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp\n>>>>>> index 3578f41b..e42a760d 100644\n>>>>>> --- a/src/ipa/ipu3/algorithms/grid.cpp\n>>>>>> +++ b/src/ipa/ipu3/algorithms/grid.cpp\n>>>>>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>>>>>> \treturn 0;\n>>>>>> }\n>>>>>> \n>>>>>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms)\n>>>>>> +{\n>>>>>> +\t/* Update the IPU3 parameters with new calculated grid */\n>>>>>> +\tparams.acc_param.awb.config.grid = context.configuration.grid.bdsGrid;\n>>>>>\n>>>>> And this is where the spaghetti dish begins :-( It should be the\n>>>>> responsibility of the AWB algorithm to set params.acc_param.awb.\n>>>>\n>>>> Yes, I moved it at least 5 times while writing the patches as I could\n>>>> not decide where it should be :-p.\n>>>\n>>> As commented in patch 03/10, I don't think \"grid\" should be an\n>>> algorithm.\n>>>\n>>>>>> +}\n>>>>>> +\n>>>>>> } /* namespace ipa::ipu3::algorithms */\n>>>>>> \n>>>>>> } /* namespace libcamera */\n>>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h\n>>>>>> index b4a51b42..89399bd2 100644\n>>>>>> --- a/src/ipa/ipu3/algorithms/grid.h\n>>>>>> +++ b/src/ipa/ipu3/algorithms/grid.h\n>>>>>> @@ -19,6 +19,7 @@ public:\n>>>>>> \t~Grid() = default;\n>>>>>> \n>>>>>> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n>>>>>> +\tvoid prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;\n>>>>>> };\n>>>>>> \n>>>>>> } /* namespace ipa::ipu3::algorithms */\n>>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>>>> index ef7fec86..394a7a45 100644\n>>>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>>>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>>>>>> \n>>>>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>>>>>> {\n>>>>>> +\tfor (auto const &algo : algorithms_)\n>>>>>> +\t\talgo->prepare(context_, params_);\n>>>>>> +\n>>>>>> \tif (agcAlgo_->updateControls())\n>>>>>> \t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n>>>>>> \n>>>>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n>>>>>> index 4ee5ee6f..911760b3 100644\n>>>>>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>>>>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>>>>>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n>>>>>> \tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n>>>>>> \tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n>>>>>> \tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n>>>>>> -\tparams.acc_param.awb.config.grid = bdsGrid;\n>>>>>> \tawbGrid_ = bdsGrid;\n>>>>>> \n>>>>>> \tparams.use.acc_bnr = 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 05B05BD87D\n\tfor ;\n\tWed, 18 Aug 2021 06:47:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A736688A3;\n\tWed, 18 Aug 2021 08:47:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26B7D60505\n\tfor ;\n\tWed, 18 Aug 2021 08:47:31 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:946e:2bbb:370e:41e4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B9C3D466;\n\tWed, 18 Aug 2021 08:47:30 +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=\"f8yPY/Iq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629269250;\n\tbh=EV10LNBTqzMTGYo92mp2IrEVNYvCNRyqAfSmw3MeP1U=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=f8yPY/Iq5vUGD6UC62/b8+PniTVs5vCG9CS/W8A5EtdCcu+A7o+o/v/zm3oe1sfv2\n\t42Xo5gUObaxm5zx4gf9gBNGPU8iNLRHj8c7o4w4UDC/YE3J3KqZDd5EwhOqeC/6XSz\n\t7U5I9ZJuhTaLf9c5BwPVdOliS9PdKXr+Lt7+4cAA=","To":"Laurent Pinchart ","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<63951ba9-9231-5b82-cba5-48fb88cea833@ideasonboard.com>\n\t\n\t<4aebf67c-f2cc-922d-6e64-32ae3482bc6b@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<44894d11-e27a-927e-2aef-4d9f40ee0b93@ideasonboard.com>","Date":"Wed, 18 Aug 2021 08:47:28 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.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 v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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":18896,"web_url":"https://patchwork.libcamera.org/comment/18896/","msgid":"","date":"2021-08-18T08:30:22","subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Wed, Aug 18, 2021 at 08:47:28AM +0200, Jean-Michel Hautbois wrote:\n> On 16/08/2021 11:49, Laurent Pinchart wrote:\n> > On Mon, Aug 16, 2021 at 11:43:54AM +0200, Jean-Michel Hautbois wrote:\n> >> On 16/08/2021 11:41, Laurent Pinchart wrote:\n> >>> On Mon, Aug 16, 2021 at 10:01:14AM +0200, Jean-Michel Hautbois wrote:\n> >>>> On 15/08/2021 20:02, Laurent Pinchart wrote:\n> >>>>> On Thu, Aug 12, 2021 at 06:52:37PM +0200, Jean-Michel Hautbois wrote:\n> >>>>>> When a new ipu3_uapi_params buffer is ready, IPAIPU3 receives a\n> >>>>>> EventFillParams. When this is called, the algorithms should fill every\n> >>>>>> field in the parameter structure they need to update.\n> >>>>>>\n> >>>>>> Demonstrate it by adding a loop on the prepare() call in IPAIPU3 and let\n> >>>>>> the Grid algorithm update the grid field.\n> >>>>>> This leads to removing this same update from the Awb algorithm.\n> >>>>>>\n> >>>>>> Signed-off-by: Jean-Michel Hautbois \n> >>>>>> ---\n> >>>>>> src/ipa/ipu3/algorithms/algorithm.h | 2 ++\n> >>>>>> src/ipa/ipu3/algorithms/grid.cpp | 6 ++++++\n> >>>>>> src/ipa/ipu3/algorithms/grid.h | 1 +\n> >>>>>> src/ipa/ipu3/ipu3.cpp | 3 +++\n> >>>>>> src/ipa/ipu3/ipu3_awb.cpp | 1 -\n> >>>>>> 5 files changed, 12 insertions(+), 1 deletion(-)\n> >>>>>>\n> >>>>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> >>>>>> index c1b37276..b571f55a 100644\n> >>>>>> --- a/src/ipa/ipu3/algorithms/algorithm.h\n> >>>>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n> >>>>>> @@ -26,6 +26,8 @@ public:\n> >>>>>> \t{\n> >>>>>> \t\treturn 0;\n> >>>>>> \t}\n> >>>>>> +\n> >>>>>> +\tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params ¶ms) {}\n> >>>>>\n> >>>>> Line wrap.\n> >>>>>\n> >>>>> Could you add a patch after 02/10 that adds all functions (configure(),\n> >>>>> prepare() and process()) to the Algorithm class, and call them in the\n> >>>>> IPAIPU3 class as appropriate ? That patch should include the\n> >>>>> documentation of the functions, to explain how they're used. After that\n> >>>>> the rest of the series can begin moving the algorithms to the new class.\n> >>>>\n> >>>> I did that in the first place, and it was a bit of a heavy patch. So I\n> >>>> decided to split it to introduce each of configure/prepare/process calls\n> >>>> step by step...\n> >>>\n> >>> The issue with this approach is that it's harder to have an overview of\n> >>> the proposed API. Is it *that* heavy to define a class with three\n> >>> abstract functions and their documentation ?\n> >>\n> >> No, but as we already have a process() call in AGC it must be rewritten\n> >> accordingly.\n> > \n> > The existing process() function takes different parameters, so the\n> > compiler shouldn't complain. Worst case it could be renamed in a small\n> > patch at the beginning of the series to move it out of the way. I agree\n> > that adapting the algorithm is best done in a patch separate from the\n> > introduction of the new Algorithm structure.\n> \n> The compiler does not agree with you :-).\n> \n> ../src/ipa/ipu3/ipu3_agc.h:33:7: error:\n> 'libcamera::ipa::ipu3::IPU3Agc::process' hides overloaded virtual\n> function [-Werror,-Woverloaded-virtual]\n\nIt's a warning that we treat as an error, as all warnings. You could\ndisable the warning temporarily and then restore it as part of the\nseries to keep patches from getting large.\n\n> void process(const ipu3_uapi_stats_3a *stats, uint32_t\n> &exposure, double &gain);\n> ^\n> ../src/ipa/ipu3/algorithms/algorithm.h:26:15: note: hidden overloaded\n> virtual function 'libcamera::ipa::ipu3::Algorithm::process' declared\n> here: different number of parameters (2 vs 3)\n> virtual void process(IPAContext &context, ipu3_uapi_stats_3a\n> &stats);\n> \n> >>>>>> };\n> >>>>>> \n> >>>>>> } /* namespace ipa::ipu3 */\n> >>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp\n> >>>>>> index 3578f41b..e42a760d 100644\n> >>>>>> --- a/src/ipa/ipu3/algorithms/grid.cpp\n> >>>>>> +++ b/src/ipa/ipu3/algorithms/grid.cpp\n> >>>>>> @@ -78,6 +78,12 @@ int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> >>>>>> \treturn 0;\n> >>>>>> }\n> >>>>>> \n> >>>>>> +void Grid::prepare(IPAContext &context, ipu3_uapi_params ¶ms)\n> >>>>>> +{\n> >>>>>> +\t/* Update the IPU3 parameters with new calculated grid */\n> >>>>>> +\tparams.acc_param.awb.config.grid = context.configuration.grid.bdsGrid;\n> >>>>>\n> >>>>> And this is where the spaghetti dish begins :-( It should be the\n> >>>>> responsibility of the AWB algorithm to set params.acc_param.awb.\n> >>>>\n> >>>> Yes, I moved it at least 5 times while writing the patches as I could\n> >>>> not decide where it should be :-p.\n> >>>\n> >>> As commented in patch 03/10, I don't think \"grid\" should be an\n> >>> algorithm.\n> >>>\n> >>>>>> +}\n> >>>>>> +\n> >>>>>> } /* namespace ipa::ipu3::algorithms */\n> >>>>>> \n> >>>>>> } /* namespace libcamera */\n> >>>>>> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h\n> >>>>>> index b4a51b42..89399bd2 100644\n> >>>>>> --- a/src/ipa/ipu3/algorithms/grid.h\n> >>>>>> +++ b/src/ipa/ipu3/algorithms/grid.h\n> >>>>>> @@ -19,6 +19,7 @@ public:\n> >>>>>> \t~Grid() = default;\n> >>>>>> \n> >>>>>> \tint configure(IPAContext &context, const IPAConfigInfo &configInfo) override;\n> >>>>>> +\tvoid prepare(IPAContext &context, ipu3_uapi_params ¶ms) override;\n> >>>>>> };\n> >>>>>> \n> >>>>>> } /* namespace ipa::ipu3::algorithms */\n> >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>>>>> index ef7fec86..394a7a45 100644\n> >>>>>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>>>>> @@ -305,6 +305,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> >>>>>> \n> >>>>>> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >>>>>> {\n> >>>>>> +\tfor (auto const &algo : algorithms_)\n> >>>>>> +\t\talgo->prepare(context_, params_);\n> >>>>>> +\n> >>>>>> \tif (agcAlgo_->updateControls())\n> >>>>>> \t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> >>>>>> \n> >>>>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> >>>>>> index 4ee5ee6f..911760b3 100644\n> >>>>>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> >>>>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> >>>>>> @@ -159,7 +159,6 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> >>>>>> \tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> >>>>>> \tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> >>>>>> \tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> >>>>>> -\tparams.acc_param.awb.config.grid = bdsGrid;\n> >>>>>> \tawbGrid_ = bdsGrid;\n> >>>>>> \n> >>>>>> \tparams.use.acc_bnr = 1;","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 12F6FBD87C\n\tfor ;\n\tWed, 18 Aug 2021 08:30:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E81068895;\n\tWed, 18 Aug 2021 10:30:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 467BF6025E\n\tfor ;\n\tWed, 18 Aug 2021 10:30:30 +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 AAB99466;\n\tWed, 18 Aug 2021 10:30:29 +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=\"oXy3jEzu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629275429;\n\tbh=m3VVgIL8J7nLcAcktS2xMYANs3A0miHDEUdfPWp6I9A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oXy3jEzuYgkHs3JMO3LkQ9hytVAYnHFWSmUZWXJ2JtsoDKh1PiexQLWWCK8pgdAKG\n\tvVgXdKuqJQIXpnlS/9k1Pj5/BmH61vWQOFqLR6AYUiG0pGUI6rSSPY4KhWHpm1DOhQ\n\t4A0OeI5nkkWdQxmER313E7AF8ZeOMytHwpP7dmYk=","Date":"Wed, 18 Aug 2021 11:30:22 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-5-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<63951ba9-9231-5b82-cba5-48fb88cea833@ideasonboard.com>\n\t\n\t<4aebf67c-f2cc-922d-6e64-32ae3482bc6b@ideasonboard.com>\n\t\n\t<44894d11-e27a-927e-2aef-4d9f40ee0b93@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<44894d11-e27a-927e-2aef-4d9f40ee0b93@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 04/10] ipa: ipu3: Introduce a\n\tprepare() call to Algorithm","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\" "}}]