[{"id":18610,"web_url":"https://patchwork.libcamera.org/comment/18610/","msgid":"","date":"2021-08-09T09:45:32","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] ipa: ipu3: Introduce modular\n\talgorithms","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> Provide a modular IPU3 specific algorithm, and frame context\n\n\"s/and frame context structure/and make use of the IPAContext structure/\"\n\n> structure so that algorithms can be built for the IPU3 in a\n> component based structure.\n>\n\n\n\"\"\"\nThe existing AWB and AGC algorithm's are moved to explicitly reference\nthe libipa ipa::Algorithm namespaced class and are converted separately.\n\"\"\"\n\n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/algorithm.h | 30 ++++++++++++++++++\n> src/ipa/ipu3/algorithms/meson.build | 4 +++\n> src/ipa/ipu3/ipu3.cpp | 47 +++++++++++++++++++++++++++++\n> src/ipa/ipu3/ipu3_agc.h | 3 +-\n> src/ipa/ipu3/ipu3_awb.h | 3 +-\n> src/ipa/ipu3/meson.build | 4 +++\n> 6 files changed, 89 insertions(+), 2 deletions(-)\n> create mode 100644 src/ipa/ipu3/algorithms/algorithm.h\n> create mode 100644 src/ipa/ipu3/algorithms/meson.build\n> \n> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> new file mode 100644\n> index 00000000..bac82b7c\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/algorithm.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> + * algorithm.h - IPU3 control algorithm interface\n> + */\n> +#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__\n> +#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__\n> +\n> +#include \n\nI'm pretty sure I added this line, but it should be in \"\" not <>\n\nBut we might need to 'fix' meson to provide an equivalent to -iquote to\nthe top of the IPU3 IPA source tree (as separate patch). (As otherwise,\nI think I wo0uld have used \"\")\n\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::ipu3 {\n> +\n> +class Algorithm\n> +{\n> +public:\n> +\tvirtual ~Algorithm() {}\n> +\n> +\tvirtual int initialise([[maybe_unused]] IPAContext &context) { return 0; }\n> +\tvirtual int configure([[maybe_unused]] IPAContext &context) { return 0; }\n> +\tvirtual void process([[maybe_unused]] IPAContext &context) {}\n> +};\n> +\n> +} /* namespace ipa::ipu3 */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */\n> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n> new file mode 100644\n> index 00000000..67148333\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -0,0 +1,4 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +ipu3_ipa_algorithms = files([\n> +])\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index a714af85..55c4da72 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -22,6 +22,7 @@\n> \n> #include \"libcamera/internal/framebuffer.h\"\n> \n> +#include \"algorithms/algorithm.h\"\n> #include \"ipa_context.h\"\n> \n> #include \"ipu3_agc.h\"\n> @@ -53,6 +54,10 @@ public:\n> private:\n> \tvoid processControls(unsigned int frame, const ControlList &controls);\n> \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n> +\n> +\tint initialiseAlgorithms();\n> +\tint configureAlgorithms();\n> +\tvoid processAlgorithms(const ipu3_uapi_stats_3a *stats);\n\nnew line here to keep the 'algorithm iteration' functions in their own\nblock.\n\n\n> \tvoid parseStatistics(unsigned int frame,\n> \t\t\t int64_t frameTimestamp,\n> \t\t\t const ipu3_uapi_stats_3a *stats);\n> @@ -82,6 +87,9 @@ private:\n> \t/* Interface to the Camera Helper */\n> \tstd::unique_ptr camHelper_;\n> \n> +\t/* Maintain the algorithms used by the IPA */\n> +\tstd::list> algorithms_;\n> +\n> \t/* Local parameter storage */\n> \tstruct ipu3_uapi_grid_config bdsGrid_;\n> \tstruct IPAContext context_;\n> @@ -98,6 +106,8 @@ int IPAIPU3::init(const IPASettings &settings)\n> \t/* Reset all the hardware settings */\n> \tcontext_.params = {};\n> \n> +\tinitialiseAlgorithms();\n> +\n> \treturn 0;\n> }\n> \n> @@ -199,6 +209,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \n> \tcalculateBdsGrid(configInfo.bdsOutputSize);\n> \n> +\tconfigureAlgorithms();\n> +\n> \tawbAlgo_ = std::make_unique();\n> \tawbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);\n> \n> @@ -288,12 +300,47 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> \tqueueFrameAction.emit(frame, op);\n> }\n> \n> +int IPAIPU3::initialiseAlgorithms()\n> +{\n> +\tfor (auto const &algo : algorithms_) {\n> +\t\tint ret = algo->initialise(context_);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPAIPU3::configureAlgorithms()\n> +{\n> +\tfor (auto const &algo : algorithms_) {\n> +\t\tint ret = algo->configure(context_);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void IPAIPU3::processAlgorithms(const ipu3_uapi_stats_3a *stats)\n> +{\n> +\t/* It might be better to pass the stats in as a parameter to process() ? */\n\nWe either need to make a decision on this or prefix with \"\\todo\"\n\n> +\tcontext_.stats = stats;\n> +\n> +\tfor (auto const &algo : algorithms_) {\n> +\t\talgo->process(context_);\n> +\t}\n> +}\n> +\n> void IPAIPU3::parseStatistics(unsigned int frame,\n> \t\t\t [[maybe_unused]] int64_t frameTimestamp,\n> \t\t\t [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> {\n> \tControlList ctrls(controls::controls);\n> \n> +\t/* Run the process for each algorithm on the stats */\n> +\tprocessAlgorithms(stats);\n> +\n> \tdouble gain = camHelper_->gain(gain_);\n> \tagcAlgo_->process(stats, exposure_, gain);\n> \tgain_ = camHelper_->gainCode(gain);\n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index 3deca3ae..e3e1d28b 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -16,6 +16,7 @@\n> \n> #include \n> \n> +#include \"algorithms/algorithm.h\"\n> #include \"libipa/algorithm.h\"\n> \n> namespace libcamera {\n> @@ -26,7 +27,7 @@ namespace ipa::ipu3 {\n> \n> using utils::Duration;\n> \n> -class IPU3Agc : public Algorithm\n> +class IPU3Agc : public ipa::Algorithm\n> {\n> public:\n> \tIPU3Agc();\n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> index 122cf68c..284e3844 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -13,6 +13,7 @@\n> \n> #include \n> \n> +#include \"algorithms/algorithm.h\"\n> #include \"libipa/algorithm.h\"\n> \n> namespace libcamera {\n> @@ -23,7 +24,7 @@ namespace ipa::ipu3 {\n> static constexpr uint32_t kAwbStatsSizeX = 16;\n> static constexpr uint32_t kAwbStatsSizeY = 12;\n> \n> -class IPU3Awb : public Algorithm\n> +class IPU3Awb : public ipa::Algorithm\n> {\n> public:\n> \tIPU3Awb();\n> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build\n> index b6364190..fcb27d68 100644\n> --- a/src/ipa/ipu3/meson.build\n> +++ b/src/ipa/ipu3/meson.build\n> @@ -1,5 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> +subdir('algorithms')\n> +\n> ipa_name = 'ipa_ipu3'\n> \n> ipu3_ipa_sources = files([\n> @@ -8,6 +10,8 @@ ipu3_ipa_sources = files([\n> 'ipu3_awb.cpp',\n> ])\n> \n> +ipu3_ipa_sources += ipu3_ipa_algorithms\n> +\n> mod = shared_module(ipa_name,\n> [ipu3_ipa_sources, libcamera_generated_ipa_headers],\n> name_prefix : '',\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 7EBBEC3240\n\tfor ;\n\tMon, 9 Aug 2021 09:45:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F182568864;\n\tMon, 9 Aug 2021 11:45:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7CAB0687EB\n\tfor ;\n\tMon, 9 Aug 2021 11:45:35 +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 041F5EE;\n\tMon, 9 Aug 2021 11:45:34 +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=\"acTyOpA7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628502335;\n\tbh=41XXkT0PqRz/Uegp+JwllnXr46qIotf5gTOV49sHCw4=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=acTyOpA7QWRyzgQjV3wUtcCvvRRDU5RPXZrmtDD4NGbH+HkMaVqHfW5NJUFQNCOMh\n\tyKqMqDaqMh6eJE1rtApK9Z5DbsdydgTAVAxnXS0gyN0RSx/OInPS1Jj72RQdoJRKTS\n\tkWQygOMVbFNzL98pJgA8NREUJfyC32ObWeyvf9TU=","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-3-jeanmichel.hautbois@ideasonboard.com>","Message-ID":"","Date":"Mon, 9 Aug 2021 10:45:32 +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-3-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 2/5] ipa: ipu3: Introduce modular\n\talgorithms","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":18611,"web_url":"https://patchwork.libcamera.org/comment/18611/","msgid":"<2c1bf99e-678a-e853-1d47-7e2b50a773cc@ideasonboard.com>","date":"2021-08-09T09:47:44","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] ipa: ipu3: Introduce modular\n\talgorithms","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM,\n\nThanks for the patch.\n\nOn 8/9/21 2:50 PM, Jean-Michel Hautbois wrote:\n> Provide a modular IPU3 specific algorithm, and frame context\n> structure so that algorithms can be built for the IPU3 in a\n> component based structure.\n>\n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/algorithm.h | 30 ++++++++++++++++++\n> src/ipa/ipu3/algorithms/meson.build | 4 +++\n> src/ipa/ipu3/ipu3.cpp | 47 +++++++++++++++++++++++++++++\n> src/ipa/ipu3/ipu3_agc.h | 3 +-\n> src/ipa/ipu3/ipu3_awb.h | 3 +-\n> src/ipa/ipu3/meson.build | 4 +++\n> 6 files changed, 89 insertions(+), 2 deletions(-)\n> create mode 100644 src/ipa/ipu3/algorithms/algorithm.h\n> create mode 100644 src/ipa/ipu3/algorithms/meson.build\n>\n> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> new file mode 100644\n> index 00000000..bac82b7c\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/algorithm.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> + * algorithm.h - IPU3 control algorithm interface\n> + */\n> +#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__\n> +#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::ipu3 {\n> +\n> +class Algorithm\n> +{\n> +public:\n> +\tvirtual ~Algorithm() {}\n> +\n> +\tvirtual int initialise([[maybe_unused]] IPAContext &context) { return 0; }\n> +\tvirtual int configure([[maybe_unused]] IPAContext &context) { return 0; }\n> +\tvirtual void process([[maybe_unused]] IPAContext &context) {}\nUsually I have seen in libcamera, [[maybe_unused]] is part of function \ndefinition or declaration. There are some exceptions of course, so I \nwon't worry about this too much\n> +};\n> +\n> +} /* namespace ipa::ipu3 */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */\n> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n> new file mode 100644\n> index 00000000..67148333\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -0,0 +1,4 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +ipu3_ipa_algorithms = files([\n> +])\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index a714af85..55c4da72 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -22,6 +22,7 @@\n> \n> #include \"libcamera/internal/framebuffer.h\"\n> \n> +#include \"algorithms/algorithm.h\"\n> #include \"ipa_context.h\"\n> \n> #include \"ipu3_agc.h\"\n> @@ -53,6 +54,10 @@ public:\n> private:\n> \tvoid processControls(unsigned int frame, const ControlList &controls);\n> \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n> +\n> +\tint initialiseAlgorithms();\n> +\tint configureAlgorithms();\n> +\tvoid processAlgorithms(const ipu3_uapi_stats_3a *stats);\n> \tvoid parseStatistics(unsigned int frame,\n> \t\t\t int64_t frameTimestamp,\n> \t\t\t const ipu3_uapi_stats_3a *stats);\n> @@ -82,6 +87,9 @@ private:\n> \t/* Interface to the Camera Helper */\n> \tstd::unique_ptr camHelper_;\n> \n> +\t/* Maintain the algorithms used by the IPA */\n> +\tstd::list> algorithms_;\n> +\n> \t/* Local parameter storage */\n> \tstruct ipu3_uapi_grid_config bdsGrid_;\n> \tstruct IPAContext context_;\n> @@ -98,6 +106,8 @@ int IPAIPU3::init(const IPASettings &settings)\n> \t/* Reset all the hardware settings */\n> \tcontext_.params = {};\n> \n> +\tinitialiseAlgorithms();\n> +\n> \treturn 0;\n> }\n> \n> @@ -199,6 +209,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \n> \tcalculateBdsGrid(configInfo.bdsOutputSize);\n> \n> +\tconfigureAlgorithms();\n> +\n> \tawbAlgo_ = std::make_unique();\n> \tawbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);\n> \n> @@ -288,12 +300,47 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> \tqueueFrameAction.emit(frame, op);\n> }\n> \n> +int IPAIPU3::initialiseAlgorithms()\n> +{\n> +\tfor (auto const &algo : algorithms_) {\n> +\t\tint ret = algo->initialise(context_);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPAIPU3::configureAlgorithms()\n> +{\n> +\tfor (auto const &algo : algorithms_) {\n> +\t\tint ret = algo->configure(context_);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void IPAIPU3::processAlgorithms(const ipu3_uapi_stats_3a *stats)\n> +{\n> +\t/* It might be better to pass the stats in as a parameter to process() ? */\nI think so yes,\n> +\tcontext_.stats = stats;\n> +\n> +\tfor (auto const &algo : algorithms_) {\n> +\t\talgo->process(context_);\n\nI would expect context_ to remain unchanged during a single frame is \nprocessed. I would expect the context_ to change *between* two frames. \nSo I guess, passing in stats explicitly and not populating inside \ncontext_, makes sense to me.\n\n  Reviewed-by: Umang Jain \n\n> +\t}\n> +}\n> +\n> void IPAIPU3::parseStatistics(unsigned int frame,\n> \t\t\t [[maybe_unused]] int64_t frameTimestamp,\n> \t\t\t [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> {\n> \tControlList ctrls(controls::controls);\n> \n> +\t/* Run the process for each algorithm on the stats */\n> +\tprocessAlgorithms(stats);\n> +\n> \tdouble gain = camHelper_->gain(gain_);\n> \tagcAlgo_->process(stats, exposure_, gain);\n> \tgain_ = camHelper_->gainCode(gain);\n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index 3deca3ae..e3e1d28b 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -16,6 +16,7 @@\n> \n> #include \n> \n> +#include \"algorithms/algorithm.h\"\n> #include \"libipa/algorithm.h\"\n> \n> namespace libcamera {\n> @@ -26,7 +27,7 @@ namespace ipa::ipu3 {\n> \n> using utils::Duration;\n> \n> -class IPU3Agc : public Algorithm\n> +class IPU3Agc : public ipa::Algorithm\n> {\n> public:\n> \tIPU3Agc();\n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> index 122cf68c..284e3844 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -13,6 +13,7 @@\n> \n> #include \n> \n> +#include \"algorithms/algorithm.h\"\n> #include \"libipa/algorithm.h\"\n> \n> namespace libcamera {\n> @@ -23,7 +24,7 @@ namespace ipa::ipu3 {\n> static constexpr uint32_t kAwbStatsSizeX = 16;\n> static constexpr uint32_t kAwbStatsSizeY = 12;\n> \n> -class IPU3Awb : public Algorithm\n> +class IPU3Awb : public ipa::Algorithm\n> {\n> public:\n> \tIPU3Awb();\n> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build\n> index b6364190..fcb27d68 100644\n> --- a/src/ipa/ipu3/meson.build\n> +++ b/src/ipa/ipu3/meson.build\n> @@ -1,5 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> +subdir('algorithms')\n> +\n> ipa_name = 'ipa_ipu3'\n> \n> ipu3_ipa_sources = files([\n> @@ -8,6 +10,8 @@ ipu3_ipa_sources = files([\n> 'ipu3_awb.cpp',\n> ])\n> \n> +ipu3_ipa_sources += ipu3_ipa_algorithms\n> +\n> mod = shared_module(ipa_name,\n> [ipu3_ipa_sources, libcamera_generated_ipa_headers],\n> name_prefix : '',","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 51218C3240\n\tfor ;\n\tMon, 9 Aug 2021 09:47:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9A676884F;\n\tMon, 9 Aug 2021 11:47:49 +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 E92E968823\n\tfor ;\n\tMon, 9 Aug 2021 11:47:48 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.61])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02E1DEE;\n\tMon, 9 Aug 2021 11:47:47 +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=\"AQuL0PJx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628502468;\n\tbh=01D3PcHfGEWCibEISLphdfxbOk//q3R06JGUp/z/T3E=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=AQuL0PJxrP5cKAkwjf2eneVypRA6BjTJqDQTN+Q8WxaKtqlBh5B082ec037JAMvoz\n\t2xdOC4xFGIPN9i15BTORrZWjYecf+DNIn/wVzr1KAHCxMZ+eeTuiCaLKSkXK51u/b2\n\t5w1yL4JT3ldkstmJgedspJrBh+SUlX1oFBiNUK0o=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-3-jeanmichel.hautbois@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<2c1bf99e-678a-e853-1d47-7e2b50a773cc@ideasonboard.com>","Date":"Mon, 9 Aug 2021 15:17:44 +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-3-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] ipa: ipu3: Introduce modular\n\talgorithms","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":18657,"web_url":"https://patchwork.libcamera.org/comment/18657/","msgid":"","date":"2021-08-10T07:24:08","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] ipa: ipu3: Introduce modular\n\talgorithms","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang,\n\nOn 09/08/2021 11:47, Umang Jain wrote:\n> Hi JM,\n> \n> Thanks for the patch.\n> \n> On 8/9/21 2:50 PM, Jean-Michel Hautbois wrote:\n>> Provide a modular IPU3 specific algorithm, and frame context\n>> structure so that algorithms can be built for the IPU3 in a\n>> component based structure.\n>>\n>> Signed-off-by: Kieran Bingham \n>> Signed-off-by: Jean-Michel Hautbois\n>> \n>> ---\n>>   src/ipa/ipu3/algorithms/algorithm.h | 30 ++++++++++++++++++\n>>   src/ipa/ipu3/algorithms/meson.build |  4 +++\n>>   src/ipa/ipu3/ipu3.cpp               | 47 +++++++++++++++++++++++++++++\n>>   src/ipa/ipu3/ipu3_agc.h             |  3 +-\n>>   src/ipa/ipu3/ipu3_awb.h             |  3 +-\n>>   src/ipa/ipu3/meson.build            |  4 +++\n>>   6 files changed, 89 insertions(+), 2 deletions(-)\n>>   create mode 100644 src/ipa/ipu3/algorithms/algorithm.h\n>>   create mode 100644 src/ipa/ipu3/algorithms/meson.build\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h\n>> b/src/ipa/ipu3/algorithms/algorithm.h\n>> new file mode 100644\n>> index 00000000..bac82b7c\n>> --- /dev/null\n>> +++ b/src/ipa/ipu3/algorithms/algorithm.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>> + * algorithm.h - IPU3 control algorithm interface\n>> + */\n>> +#ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__\n>> +#define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__\n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::ipu3 {\n>> +\n>> +class Algorithm\n>> +{\n>> +public:\n>> +    virtual ~Algorithm() {}\n>> +\n>> +    virtual int initialise([[maybe_unused]] IPAContext &context) {\n>> return 0; }\n>> +    virtual int configure([[maybe_unused]] IPAContext &context) {\n>> return 0; }\n>> +    virtual void process([[maybe_unused]] IPAContext &context) {}\n> Usually I have seen in libcamera, [[maybe_unused]] is part of function\n> definition or declaration. There are some exceptions of course, so I\n> won't worry about this too much\n>> +};\n>> +\n>> +} /* namespace ipa::ipu3 */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_IPU3_ALGORITHM_H__ */\n>> diff --git a/src/ipa/ipu3/algorithms/meson.build\n>> b/src/ipa/ipu3/algorithms/meson.build\n>> new file mode 100644\n>> index 00000000..67148333\n>> --- /dev/null\n>> +++ b/src/ipa/ipu3/algorithms/meson.build\n>> @@ -0,0 +1,4 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +ipu3_ipa_algorithms = files([\n>> +])\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index a714af85..55c4da72 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -22,6 +22,7 @@\n>>     #include \"libcamera/internal/framebuffer.h\"\n>>   +#include \"algorithms/algorithm.h\"\n>>   #include \"ipa_context.h\"\n>>     #include \"ipu3_agc.h\"\n>> @@ -53,6 +54,10 @@ public:\n>>   private:\n>>       void processControls(unsigned int frame, const ControlList\n>> &controls);\n>>       void fillParams(unsigned int frame, ipu3_uapi_params *params);\n>> +\n>> +    int initialiseAlgorithms();\n>> +    int configureAlgorithms();\n>> +    void processAlgorithms(const ipu3_uapi_stats_3a *stats);\n>>       void parseStatistics(unsigned int frame,\n>>                    int64_t frameTimestamp,\n>>                    const ipu3_uapi_stats_3a *stats);\n>> @@ -82,6 +87,9 @@ private:\n>>       /* Interface to the Camera Helper */\n>>       std::unique_ptr camHelper_;\n>>   +    /* Maintain the algorithms used by the IPA */\n>> +    std::list> algorithms_;\n>> +\n>>       /* Local parameter storage */\n>>       struct ipu3_uapi_grid_config bdsGrid_;\n>>       struct IPAContext context_;\n>> @@ -98,6 +106,8 @@ int IPAIPU3::init(const IPASettings &settings)\n>>       /* Reset all the hardware settings */\n>>       context_.params = {};\n>>   +    initialiseAlgorithms();\n>> +\n>>       return 0;\n>>   }\n>>   @@ -199,6 +209,8 @@ int IPAIPU3::configure(const IPAConfigInfo\n>> &configInfo)\n>>         calculateBdsGrid(configInfo.bdsOutputSize);\n>>   +    configureAlgorithms();\n>> +\n>>       awbAlgo_ = std::make_unique();\n>>       awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize,\n>> bdsGrid_);\n>>   @@ -288,12 +300,47 @@ void IPAIPU3::fillParams(unsigned int frame,\n>> ipu3_uapi_params *params)\n>>       queueFrameAction.emit(frame, op);\n>>   }\n>>   +int IPAIPU3::initialiseAlgorithms()\n>> +{\n>> +    for (auto const &algo : algorithms_) {\n>> +        int ret = algo->initialise(context_);\n>> +        if (ret)\n>> +            return ret;\n>> +    }\n>> +\n>> +    return 0;\n>> +}\n>> +\n>> +int IPAIPU3::configureAlgorithms()\n>> +{\n>> +    for (auto const &algo : algorithms_) {\n>> +        int ret = algo->configure(context_);\n>> +        if (ret)\n>> +            return ret;\n>> +    }\n>> +\n>> +    return 0;\n>> +}\n>> +\n>> +void IPAIPU3::processAlgorithms(const ipu3_uapi_stats_3a *stats)\n>> +{\n>> +    /* It might be better to pass the stats in as a parameter to\n>> process() ? */\n> I think so yes,\n>> +    context_.stats = stats;\n>> +\n>> +    for (auto const &algo : algorithms_) {\n>> +        algo->process(context_);\n> \n> I would expect context_ to remain unchanged during a single frame is\n> processed. I would expect the context_ to change *between* two frames.\n> So I guess, passing in stats explicitly and not populating inside\n> context_, makes sense to me.\n\nGiven that Context is not per-frame, but a \"current IPA context\"\nstructure, I think stats should be removed from it and passed to\nprocess() yes.\n\n> \n>  Reviewed-by: Umang Jain \n> \n>> +    }\n>> +}\n>> +\n>>   void IPAIPU3::parseStatistics(unsigned int frame,\n>>                     [[maybe_unused]] int64_t frameTimestamp,\n>>                     [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n>>   {\n>>       ControlList ctrls(controls::controls);\n>>   +    /* Run the process for each algorithm on the stats */\n>> +    processAlgorithms(stats);\n>> +\n>>       double gain = camHelper_->gain(gain_);\n>>       agcAlgo_->process(stats, exposure_, gain);\n>>       gain_ = camHelper_->gainCode(gain);\n>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n>> index 3deca3ae..e3e1d28b 100644\n>> --- a/src/ipa/ipu3/ipu3_agc.h\n>> +++ b/src/ipa/ipu3/ipu3_agc.h\n>> @@ -16,6 +16,7 @@\n>>     #include \n>>   +#include \"algorithms/algorithm.h\"\n>>   #include \"libipa/algorithm.h\"\n>>     namespace libcamera {\n>> @@ -26,7 +27,7 @@ namespace ipa::ipu3 {\n>>     using utils::Duration;\n>>   -class IPU3Agc : public Algorithm\n>> +class IPU3Agc : public ipa::Algorithm\n>>   {\n>>   public:\n>>       IPU3Agc();\n>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n>> index 122cf68c..284e3844 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.h\n>> +++ b/src/ipa/ipu3/ipu3_awb.h\n>> @@ -13,6 +13,7 @@\n>>     #include \n>>   +#include \"algorithms/algorithm.h\"\n>>   #include \"libipa/algorithm.h\"\n>>     namespace libcamera {\n>> @@ -23,7 +24,7 @@ namespace ipa::ipu3 {\n>>   static constexpr uint32_t kAwbStatsSizeX = 16;\n>>   static constexpr uint32_t kAwbStatsSizeY = 12;\n>>   -class IPU3Awb : public Algorithm\n>> +class IPU3Awb : public ipa::Algorithm\n>>   {\n>>   public:\n>>       IPU3Awb();\n>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build\n>> index b6364190..fcb27d68 100644\n>> --- a/src/ipa/ipu3/meson.build\n>> +++ b/src/ipa/ipu3/meson.build\n>> @@ -1,5 +1,7 @@\n>>   # SPDX-License-Identifier: CC0-1.0\n>>   +subdir('algorithms')\n>> +\n>>   ipa_name = 'ipa_ipu3'\n>>     ipu3_ipa_sources = files([\n>> @@ -8,6 +10,8 @@ ipu3_ipa_sources = files([\n>>       'ipu3_awb.cpp',\n>>   ])\n>>   +ipu3_ipa_sources += ipu3_ipa_algorithms\n>> +\n>>   mod = shared_module(ipa_name,\n>>                       [ipu3_ipa_sources,\n>> libcamera_generated_ipa_headers],\n>>                       name_prefix : '',","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 1E20CBD87D\n\tfor ;\n\tTue, 10 Aug 2021 07:24:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7485E6884F;\n\tTue, 10 Aug 2021 09:24:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 22EB3687DE\n\tfor ;\n\tTue, 10 Aug 2021 09:24:11 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:257d:f5fc:a3d5:6682])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C3E35EE;\n\tTue, 10 Aug 2021 09:24:10 +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=\"g5is+NVU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628580250;\n\tbh=z6NOiI4Mi94zchCSrCdKk/2II0HAM96dadIuNqS5yZU=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=g5is+NVUzE0A8b/Tss0D0PHikfBzdxvRkCyKIn/LbBXhWD2W8Goor1i0YiIxcMhXS\n\tP8kwPv7i4TW7djt23yhCR1E5bX/78WbytGU/vUF7hxv8mobDAXQ5TJJscS45Lq1wI4\n\tkeEmmORSQoLLPS8R/roMj6WP8iSHeFbtYs8o0leE=","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-3-jeanmichel.hautbois@ideasonboard.com>\n\t<2c1bf99e-678a-e853-1d47-7e2b50a773cc@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Tue, 10 Aug 2021 09:24:08 +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":"<2c1bf99e-678a-e853-1d47-7e2b50a773cc@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] ipa: ipu3: Introduce modular\n\talgorithms","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\" "}}]