[{"id":23538,"web_url":"https://patchwork.libcamera.org/comment/23538/","msgid":"<20220623085500.GG3309559@pyrite.rasen.tech>","date":"2022-06-23T08:55:00","subject":"Re: [libcamera-devel] [PATCH v4 01/12] ipa: libipa: Introduce a\n\tModule class template","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Laurent,\n\nOn Mon, Jun 20, 2022 at 04:42:54AM +0300, Laurent Pinchart wrote:\n> libipa defines an abstract Algorithm class template that is specialized\n> by IPA modules. IPA modules then instantiate and manage algorithms\n> internally, without help from libipa. With ongoing work on tuning data\n> support for the RkISP1, and future similar work for the IPU3, more code\n> duplication for algorithms management is expected.\n> \n> To address this and share code between multiple IPA modules, introduce a\n> new Module class template that will define and manage top-level concepts\n> for the IPA module.\n> \n> The Module class template needs to be specialized with the same types as\n> the Algorithm class. To avoid manual specialization of both classes,\n> store the types in the Module class, and replace the template arguments\n> of the Algorithm class with a single Module argument from which the\n> other types are retrieved.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n> ---\n>  src/ipa/ipu3/algorithms/algorithm.h   |  8 +--\n>  src/ipa/ipu3/module.h                 | 27 ++++++++++\n>  src/ipa/libipa/algorithm.cpp          | 17 ++++---\n>  src/ipa/libipa/algorithm.h            | 17 +++----\n>  src/ipa/libipa/meson.build            |  5 +-\n>  src/ipa/libipa/module.cpp             | 73 +++++++++++++++++++++++++++\n>  src/ipa/libipa/module.h               | 30 +++++++++++\n>  src/ipa/rkisp1/algorithms/algorithm.h | 10 +---\n>  src/ipa/rkisp1/module.h               | 27 ++++++++++\n>  9 files changed, 183 insertions(+), 31 deletions(-)\n>  create mode 100644 src/ipa/ipu3/module.h\n>  create mode 100644 src/ipa/libipa/module.cpp\n>  create mode 100644 src/ipa/libipa/module.h\n>  create mode 100644 src/ipa/rkisp1/module.h\n> \n> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> index 234b2bd77f72..ae134a9404fe 100644\n> --- a/src/ipa/ipu3/algorithms/algorithm.h\n> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n> @@ -7,19 +7,15 @@\n>  \n>  #pragma once\n>  \n> -#include <libcamera/ipa/ipu3_ipa_interface.h>\n> -\n>  #include <libipa/algorithm.h>\n>  \n> -#include \"ipa_context.h\"\n> +#include \"module.h\"\n>  \n>  namespace libcamera {\n>  \n>  namespace ipa::ipu3 {\n>  \n> -using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPAFrameContext,\n> -\t\t\t\t\t    IPAConfigInfo, ipu3_uapi_params,\n> -\t\t\t\t\t    ipu3_uapi_stats_3a>;\n> +using Algorithm = libcamera::ipa::Algorithm<Module>;\n>  \n>  } /* namespace ipa::ipu3 */\n>  \n> diff --git a/src/ipa/ipu3/module.h b/src/ipa/ipu3/module.h\n> new file mode 100644\n> index 000000000000..d94fc4594871\n> --- /dev/null\n> +++ b/src/ipa/ipu3/module.h\n> @@ -0,0 +1,27 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas On Board\n> + *\n> + * module.h - IPU3 IPA Module\n> + */\n> +\n> +#pragma once\n> +\n> +#include <linux/intel-ipu3.h>\n> +\n> +#include <libcamera/ipa/ipu3_ipa_interface.h>\n> +\n> +#include <libipa/module.h>\n> +\n> +#include \"ipa_context.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::ipu3 {\n> +\n> +using Module = ipa::Module<IPAContext, IPAFrameContext, IPAConfigInfo,\n> +\t\t\t   ipu3_uapi_params, ipu3_uapi_stats_3a>;\n> +\n> +} /* namespace ipa::ipu3 */\n> +\n> +} /* namespace libcamera*/\n> diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> index cce2ed62986d..2df91e5d8fed 100644\n> --- a/src/ipa/libipa/algorithm.cpp\n> +++ b/src/ipa/libipa/algorithm.cpp\n> @@ -19,14 +19,17 @@ namespace ipa {\n>  /**\n>   * \\class Algorithm\n>   * \\brief The base class for all IPA algorithms\n> - * \\tparam Context The type of shared IPA context\n> - * \\tparam Config The type of the IPA configuration data\n> - * \\tparam Params The type of the ISP specific parameters\n> - * \\tparam Stats The type of the IPA statistics and ISP results\n> + * \\tparam Module The IPA module type for this class of algorithms\n>   *\n> - * The Algorithm class defines a standard interface for IPA algorithms. By\n> - * abstracting algorithms, it makes possible the implementation of generic code\n> - * to manage algorithms regardless of their specific type.\n> + * The Algorithm class defines a standard interface for IPA algorithms\n> + * compatible with the \\a Module. By abstracting algorithms, it makes possible\n> + * the implementation of generic code to manage algorithms regardless of their\n> + * specific type.\n> + *\n> + * To specialize the Algorithm class template, an IPA module shall specialize\n> + * the Module class template with module-specific context and configuration\n> + * types, and pass the specialized Module class as the \\a Module template\n> + * argument.\n>   */\n>  \n>  /**\n> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> index 032a05b56635..fd2ffcfbc900 100644\n> --- a/src/ipa/libipa/algorithm.h\n> +++ b/src/ipa/libipa/algorithm.h\n> @@ -10,27 +10,26 @@ namespace libcamera {\n>  \n>  namespace ipa {\n>  \n> -template<typename Context, typename FrameContext, typename Config,\n> -\t typename Params, typename Stats>\n> +template<typename Module>\n>  class Algorithm\n>  {\n>  public:\n>  \tvirtual ~Algorithm() {}\n>  \n> -\tvirtual int configure([[maybe_unused]] Context &context,\n> -\t\t\t      [[maybe_unused]] const Config &configInfo)\n> +\tvirtual int configure([[maybe_unused]] typename Module::Context &context,\n> +\t\t\t      [[maybe_unused]] const typename Module::Config &configInfo)\n>  \t{\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tvirtual void prepare([[maybe_unused]] Context &context,\n> -\t\t\t     [[maybe_unused]] Params *params)\n> +\tvirtual void prepare([[maybe_unused]] typename Module::Context &context,\n> +\t\t\t     [[maybe_unused]] typename Module::Params *params)\n>  \t{\n>  \t}\n>  \n> -\tvirtual void process([[maybe_unused]] Context &context,\n> -\t\t\t     [[maybe_unused]] FrameContext *frameContext,\n> -\t\t\t     [[maybe_unused]] const Stats *stats)\n> +\tvirtual void process([[maybe_unused]] typename Module::Context &context,\n> +\t\t\t     [[maybe_unused]] typename Module::FrameContext *frameContext,\n> +\t\t\t     [[maybe_unused]] const typename Module::Stats *stats)\n>  \t{\n>  \t}\n>  };\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 161cc5a1f0d0..465cf7d6c4a7 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -3,13 +3,16 @@\n>  libipa_headers = files([\n>      'algorithm.h',\n>      'camera_sensor_helper.h',\n> -    'histogram.h'\n> +    'histogram.h',\n> +    'module.h',\n>  ])\n>  \n>  libipa_sources = files([\n> +    'algorithm.cpp',\n>      'camera_sensor_helper.cpp',\n>      'histogram.cpp',\n>      'libipa.cpp',\n> +    'module.cpp',\n>  ])\n>  \n>  libipa_includes = include_directories('..')\n> diff --git a/src/ipa/libipa/module.cpp b/src/ipa/libipa/module.cpp\n> new file mode 100644\n> index 000000000000..5a6f49a80e6d\n> --- /dev/null\n> +++ b/src/ipa/libipa/module.cpp\n> @@ -0,0 +1,73 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas On Board\n> + *\n> + * module.cpp - IPA Module\n> + */\n> +\n> +#include \"module.h\"\n> +\n> +/**\n> + * \\file module.h\n> + * \\brief IPA Module common interface\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class Module\n> + * \\brief The base class for all IPA modules\n> + * \\tparam Context The type of the shared IPA context\n> + * \\tparam FrameContext The type of the frame context\n> + * \\tparam Config The type of the IPA configuration data\n> + * \\tparam Params The type of the ISP specific parameters\n> + * \\tparam Stats The type of the IPA statistics and ISP results\n> + *\n> + * The Module class template defines a standard internal interface between IPA\n> + * modules and libipa.\n> + *\n> + * While IPA modules are platform-specific, many of their internal functions are\n> + * conceptually similar, even if they take different types of platform-specifc\n> + * parameters. For instance, IPA modules could share code that instantiates,\n> + * initializes and run algorithms if it wasn't for the fact that the the format\n> + * of ISP parameters or statistics passed to the related functions is\n> + * device-dependent.\n> + *\n> + * To enable a shared implementation of those common tasks in libipa, the Module\n> + * class template defines a standard internal interface between IPA modules and\n> + * libipa. The template parameters specify the types of module-dependent data.\n> + * IPA modules shall create a specialization of the Module class template in\n> + * their namespace, and use it to specialize other classes of libipa, such as\n> + * the Algorithm class.\n> + */\n> +\n> +/**\n> + * \\typedef Module::Context\n> + * \\brief The type of the shared IPA context\n> + */\n> +\n> +/**\n> + * \\typedef Module::FrameContext\n> + * \\brief The type of the frame context\n> + */\n> +\n> +/**\n> + * \\typedef Module::Config\n> + * \\brief The type of the IPA configuration data\n> + */\n> +\n> +/**\n> + * \\typedef Module::Params\n> + * \\brief The type of the ISP specific parameters\n> + */\n> +\n> +/**\n> + * \\typedef Module::Stats\n> + * \\brief The type of the IPA statistics and ISP results\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h\n> new file mode 100644\n> index 000000000000..c4d778120408\n> --- /dev/null\n> +++ b/src/ipa/libipa/module.h\n> @@ -0,0 +1,30 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas On Board\n> + *\n> + * module.h - IPA module\n> + */\n> +\n> +#pragma once\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +template<typename _Context, typename _FrameContext, typename _Config,\n> +\t typename _Params, typename _Stats>\n> +class Module\n> +{\n> +public:\n> +\tusing Context = _Context;\n> +\tusing FrameContext = _FrameContext;\n> +\tusing Config = _Config;\n> +\tusing Params = _Params;\n> +\tusing Stats = _Stats;\n> +\n> +\tvirtual ~Module() {}\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> index 68e3a44e19b4..c3212cff76fe 100644\n> --- a/src/ipa/rkisp1/algorithms/algorithm.h\n> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> @@ -7,21 +7,15 @@\n>  \n>  #pragma once\n>  \n> -#include <linux/rkisp1-config.h>\n> -\n> -#include <libcamera/ipa/rkisp1_ipa_interface.h>\n> -\n>  #include <libipa/algorithm.h>\n>  \n> -#include \"ipa_context.h\"\n> +#include \"module.h\"\n>  \n>  namespace libcamera {\n>  \n>  namespace ipa::rkisp1 {\n>  \n> -using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPAFrameContext,\n> -\t\t\t\t\t    IPACameraSensorInfo, rkisp1_params_cfg,\n> -\t\t\t\t\t    rkisp1_stat_buffer>;\n> +using Algorithm = libcamera::ipa::Algorithm<Module>;\n>  \n>  } /* namespace ipa::rkisp1 */\n>  \n> diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h\n> new file mode 100644\n> index 000000000000..89f83208a75c\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/module.h\n> @@ -0,0 +1,27 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Ideas On Board\n> + *\n> + * module.h - RkISP1 IPA Module\n> + */\n> +\n> +#pragma once\n> +\n> +#include <linux/rkisp1-config.h>\n> +\n> +#include <libcamera/ipa/rkisp1_ipa_interface.h>\n> +\n> +#include <libipa/module.h>\n> +\n> +#include \"ipa_context.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1 {\n> +\n> +using Module = ipa::Module<IPAContext, IPAFrameContext, IPACameraSensorInfo,\n> +\t\t\t   rkisp1_params_cfg, rkisp1_stat_buffer>;\n> +\n> +} /* namespace ipa::rkisp1 */\n> +\n> +} /* namespace libcamera*/","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 AD537BD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 23 Jun 2022 08:55:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D28FD65635;\n\tThu, 23 Jun 2022 10:55:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AB4360412\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 23 Jun 2022 10:55:08 +0200 (CEST)","from pyrite.rasen.tech (softbank114048060123.bbtec.net\n\t[114.48.60.123])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F5E98F;\n\tThu, 23 Jun 2022 10:55:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655974509;\n\tbh=YubeAuIx41GlZgt1GUI2aCW3k446uOEc8avhgZGiOE4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Vf+TCZS9u1sQTw81nvmk/aCriCXmZCaI186yoGkI52mrKCvd2mMwcOqzf/JhckqTT\n\tTJViE2AofMsAMXHAsgPZFU4X+vUvS6Y98mGG9i2z1/p0q+RUMJ5qwgEE5ZE04kwmyv\n\t23FCm7pM6qL4OvPboRIJt4h9fBY3HZo4kYFOXjVK8moHN44EzGdDYAhjhg15RZhKj0\n\tfayddisaENTd6tu6a/A4h4gxf3KLFuY1jf35ozeJdHdoDM5cIrp6lLgZMISu4kBw/0\n\tZ+Ht2WoG72oY8iEGAHDjGQbPKiCt0Uj4P7A6qownkCoWuqnnfpsjvcbH96eizq/euK\n\thl4Qu5BzsNlxQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655974507;\n\tbh=YubeAuIx41GlZgt1GUI2aCW3k446uOEc8avhgZGiOE4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l1Q1Po/wWcAfpI7ybRBMniSydOmJv0AUjT4cTkXOPOKkPh03qGLChQbAyFImXS74+\n\t2UvvKRdImnUlmYNvuKTtPCrcrqwklpQvQwSEbyrVC5gHXKFdDzPZ7iipa5Zfzc1D6z\n\t+axs+raVNOBBTw8Tjhc5lGOTibzHddjI4565wOQM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"l1Q1Po/w\"; dkim-atps=neutral","Date":"Thu, 23 Jun 2022 17:55:00 +0900","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20220623085500.GG3309559@pyrite.rasen.tech>","References":"<20220620014305.26778-1-laurent.pinchart@ideasonboard.com>\n\t<20220620014305.26778-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220620014305.26778-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 01/12] ipa: libipa: Introduce a\n\tModule class template","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Paul Elder via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]