[{"id":18608,"web_url":"https://patchwork.libcamera.org/comment/18608/","msgid":"","date":"2021-08-09T09:31:50","subject":"Re: [libcamera-devel] [RFC PATCH 1/5] ipa: ipu3: Introduce a\n\tContext structure","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM,\n\nOn 09/08/2021 10:20, Jean-Michel Hautbois wrote:\n> Passing parameters, statistics and all specific data each algorithm shares.\n\nWe're introducing something new, so we should describe the need first.\n\n\"\"\"\nAn increasing amount of data and information needs to be shared between\nthe components that build up to implement image processing algorithms.\n\nCreate a context structure which will allow us to work towards calling\nalgorithms in a modular way, and sharing information between the modules.\n\"\"\"\n\n\n> As the ipu3_uapi_params are part of the IPA context referenced by the\n> algorithms, move the structure into the IPAContext directly and adapt existing\n> algorithm implementations.\n> \n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipa_context.h | 30 ++++++++++++++++++++++++++++++\n> src/ipa/ipu3/ipu3.cpp | 16 +++++++++-------\n> 2 files changed, 39 insertions(+), 7 deletions(-)\n> create mode 100644 src/ipa/ipu3/ipa_context.h\n> \n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> new file mode 100644\n> index 00000000..5d717be5\n> --- /dev/null\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -0,0 +1,30 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * ipu3_ipa_context.h - IPU3 IPA Context\n> + *\n> + * Context information shared between the algorithms\n> + */\n> +#ifndef __LIBCAMERA_IPU3_IPA_CONTEXT_H__\n> +#define __LIBCAMERA_IPU3_IPA_CONTEXT_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::ipu3 {\n> +\n> +struct IPAContext {\n> +\t/* Input statistics from the previous frame */\n> +\tconst ipu3_uapi_stats_3a *stats;\n\nIt looks like we don't use this yet in this patch.\n\nGiven that we also question in patch 2/5 if this should be used in here,\nI think it should be introduced there too.\n\n\nOther than that,\n\nReviewed-by: Kieran Bingham \n\n> +\n> +\t/* Output Parameters which will be written to the hardware */\n> +\tipu3_uapi_params params;\n> +};\n> +\n> +} /* namespace ipa::ipu3 */\n> +\n> +} /* namespace libcamera*/\n> +\n> +#endif /* __LIBCAMERA_IPU3_IPA_CONTEXT_H__ */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 71698d36..a714af85 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -22,6 +22,8 @@\n> \n> #include \"libcamera/internal/framebuffer.h\"\n> \n> +#include \"ipa_context.h\"\n> +\n> #include \"ipu3_agc.h\"\n> #include \"ipu3_awb.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n> @@ -81,9 +83,8 @@ private:\n> \tstd::unique_ptr camHelper_;\n> \n> \t/* Local parameter storage */\n> -\tstruct ipu3_uapi_params params_;\n> -\n> \tstruct ipu3_uapi_grid_config bdsGrid_;\n> +\tstruct IPAContext context_;\n> };\n> \n> int IPAIPU3::init(const IPASettings &settings)\n> @@ -94,6 +95,9 @@ int IPAIPU3::init(const IPASettings &settings)\n> \t\treturn -ENODEV;\n> \t}\n> \n> +\t/* Reset all the hardware settings */\n> +\tcontext_.params = {};\n> +\n> \treturn 0;\n> }\n> \n> @@ -193,12 +197,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \n> \tdefVBlank_ = itVBlank->second.def().get();\n> \n> -\tparams_ = {};\n> -\n> \tcalculateBdsGrid(configInfo.bdsOutputSize);\n> \n> \tawbAlgo_ = std::make_unique();\n> -\tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n> +\tawbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);\n> \n> \tagcAlgo_ = std::make_unique();\n> \tagcAlgo_->initialise(bdsGrid_, sensorInfo_);\n> @@ -276,9 +278,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> \tif (agcAlgo_->updateControls())\n> -\t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> +\t\tawbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());\n> \n> -\t*params = params_;\n> +\t*params = context_.params;\n> \n> \tIPU3Action op;\n> \top.op = ActionParamFilled;\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 8E266C3240\n\tfor ;\n\tMon, 9 Aug 2021 09:31:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C33368823;\n\tMon, 9 Aug 2021 11:31:55 +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 AE5F4687EB\n\tfor ;\n\tMon, 9 Aug 2021 11:31:53 +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 39C4CEE;\n\tMon, 9 Aug 2021 11:31:53 +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=\"VhVp5+Qq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628501513;\n\tbh=YVqkpV1xREpTiN5Hc7QBwtfZo/pOq6laya5bLkLIdX0=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=VhVp5+Qqax3MWyEY7ZD/B1C8xNZXEkJwtvC6fiTwm6QMcpJQGNyC37TqelWacGrr6\n\t1QYgW+1XJJMRzkWa1sOUr3hXFiL4CbJn+cJSRX5lhZSne39Au1D3dAolzOfIhn7i6Z\n\tCdBmVLJUpAd5uXycYTsoRyPczDAECiV77hEyCCGU=","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-2-jeanmichel.hautbois@ideasonboard.com>","Message-ID":"","Date":"Mon, 9 Aug 2021 10:31:50 +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":"<20210809092007.79067-2-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 1/5] ipa: ipu3: Introduce a\n\tContext structure","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":18609,"web_url":"https://patchwork.libcamera.org/comment/18609/","msgid":"","date":"2021-08-09T09:35:31","subject":"Re: [libcamera-devel] [RFC PATCH 1/5] ipa: ipu3: Introduce a\n\tContext structure","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM,\n\nOn 8/9/21 2:50 PM, Jean-Michel Hautbois wrote:\n> Passing parameters, statistics and all specific data each algorithm shares.\nI would re-phrase this sentence to sound more complete.\n> As the ipu3_uapi_params are part of the IPA context referenced by the\n> algorithms, move the structure into the IPAContext directly and adapt existing\n> algorithm implementations.\n>\n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipa_context.h | 30 ++++++++++++++++++++++++++++++\n> src/ipa/ipu3/ipu3.cpp | 16 +++++++++-------\n> 2 files changed, 39 insertions(+), 7 deletions(-)\n> create mode 100644 src/ipa/ipu3/ipa_context.h\n>\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> new file mode 100644\n> index 00000000..5d717be5\n> --- /dev/null\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -0,0 +1,30 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * ipu3_ipa_context.h - IPU3 IPA Context\n> + *\n> + * Context information shared between the algorithms\n> + */\n> +#ifndef __LIBCAMERA_IPU3_IPA_CONTEXT_H__\n> +#define __LIBCAMERA_IPU3_IPA_CONTEXT_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::ipu3 {\n> +\n> +struct IPAContext {\n> +\t/* Input statistics from the previous frame */\n> +\tconst ipu3_uapi_stats_3a *stats;\n\nIs there used somewhere in the series? If yes, I suggest moving this to \nthe patch where this is used, as I can't see in this patch.\n\nWith that addressed,\n\nReviewed-by: Umang Jain \n\n> +\n> +\t/* Output Parameters which will be written to the hardware */\n> +\tipu3_uapi_params params;\n> +};\n> +\n> +} /* namespace ipa::ipu3 */\n> +\n> +} /* namespace libcamera*/\n> +\n> +#endif /* __LIBCAMERA_IPU3_IPA_CONTEXT_H__ */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 71698d36..a714af85 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -22,6 +22,8 @@\n> \n> #include \"libcamera/internal/framebuffer.h\"\n> \n> +#include \"ipa_context.h\"\n> +\n> #include \"ipu3_agc.h\"\n> #include \"ipu3_awb.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n> @@ -81,9 +83,8 @@ private:\n> \tstd::unique_ptr camHelper_;\n> \n> \t/* Local parameter storage */\n> -\tstruct ipu3_uapi_params params_;\n> -\n> \tstruct ipu3_uapi_grid_config bdsGrid_;\n> +\tstruct IPAContext context_;\n> };\n> \n> int IPAIPU3::init(const IPASettings &settings)\n> @@ -94,6 +95,9 @@ int IPAIPU3::init(const IPASettings &settings)\n> \t\treturn -ENODEV;\n> \t}\n> \n> +\t/* Reset all the hardware settings */\n> +\tcontext_.params = {};\n> +\n> \treturn 0;\n> }\n> \n> @@ -193,12 +197,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \n> \tdefVBlank_ = itVBlank->second.def().get();\n> \n> -\tparams_ = {};\n> -\n> \tcalculateBdsGrid(configInfo.bdsOutputSize);\n> \n> \tawbAlgo_ = std::make_unique();\n> -\tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n> +\tawbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);\n> \n> \tagcAlgo_ = std::make_unique();\n> \tagcAlgo_->initialise(bdsGrid_, sensorInfo_);\n> @@ -276,9 +278,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> \tif (agcAlgo_->updateControls())\n> -\t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> +\t\tawbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());\n> \n> -\t*params = params_;\n> +\t*params = context_.params;\n> \n> \tIPU3Action op;\n> \top.op = ActionParamFilled;","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 663C9BD87D\n\tfor ;\n\tMon, 9 Aug 2021 09:35:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C86BF6884D;\n\tMon, 9 Aug 2021 11:35:37 +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 DB590687EB\n\tfor ;\n\tMon, 9 Aug 2021 11:35:36 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.61])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF992EE;\n\tMon, 9 Aug 2021 11:35:35 +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=\"RwY+f5eZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628501736;\n\tbh=43AEOHcpnpsi+9W0JGGWzBeSFG1JZa72GJKDQfYMnzE=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=RwY+f5eZtdTsIhyzUV2NMA1WUyyFrn+VngBb0eBp4nxsBmQ2jAi6pAM7QAXG9xaDh\n\t9cwb0rKVHXa9NTZoUN8Zs88iUNqNBHmsG7sOPtxYnG2f2LIAm2Qe1xA4zPgjFJq4pd\n\tlo+4jED1mWlFUNhiudwPz8Rj8RlG6D1P8fRdDj1E=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-2-jeanmichel.hautbois@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 9 Aug 2021 15:05:31 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210809092007.79067-2-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 1/5] ipa: ipu3: Introduce a\n\tContext structure","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\" "}}]