[{"id":35790,"web_url":"https://patchwork.libcamera.org/comment/35790/","msgid":"<175766693084.2127323.1861641841473360224@neptunite.rasen.tech>","date":"2025-09-12T08:48:50","subject":"Re: [PATCH v17 05/12] config: Look up IPA configurables in\n\tconfiguration file","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Milan,\n\nThanks for the patch.\n\nQuoting Milan Zamazal (2025-09-11 18:29:35)\n> This patch adds configuration options for environment variables used in\n> the IPA proxy.\n> \n> The configuration snippet:\n> \n>   configuration:\n>     ipa:\n>       config_paths:\n>       - config path 1\n>       - config path 2\n>       - ...\n>       module_paths:\n>       - module path 1\n>       - module path 2\n>       - ...\n>       proxy_paths:\n>       - proxy path 1\n>       - proxy path 2\n>       - ...\n>       force_isolation: BOOL\n> \n> LIBCAMERA_<IPA_NAME>_TUNING_FILE remains configurable only via the\n> environment variable; this is supposed to be used only for testing and\n> debugging and it's not clear what to do about IPA names like \"rpi/vc4\"\n> and \"rpi/pisp\" exactly.\n> \n> There are two ways to pass the configuration to the places where it is\n> needed: Either to pass it as an argument to the method calls that need\n> it, or to pass it to the class constructors and extract the needed\n> configuration from there.  This patch uses the second method as it is\n> less polluting the code.\n> \n> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>\n\nThe commit title should be s/config:/ipa:/\n\n> ---\n>  include/libcamera/internal/ipa_manager.h      |  7 ++-\n>  include/libcamera/internal/ipa_proxy.h        |  8 ++-\n>  src/libcamera/camera_manager.cpp              |  2 +-\n>  src/libcamera/ipa_manager.cpp                 | 39 ++++++++-----\n>  src/libcamera/ipa_proxy.cpp                   | 58 +++++++++----------\n>  .../module_ipa_proxy.cpp.tmpl                 |  4 +-\n>  .../module_ipa_proxy.h.tmpl                   |  2 +-\n>  7 files changed, 68 insertions(+), 52 deletions(-)\n> \n> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> index a0d448cf9..b0b44c74a 100644\n> --- a/include/libcamera/internal/ipa_manager.h\n> +++ b/include/libcamera/internal/ipa_manager.h\n> @@ -17,6 +17,7 @@\n>  #include <libcamera/ipa/ipa_module_info.h>\n>  \n>  #include \"libcamera/internal/camera_manager.h\"\n> +#include \"libcamera/internal/global_configuration.h\"\n>  #include \"libcamera/internal/ipa_module.h\"\n>  #include \"libcamera/internal/pipeline_handler.h\"\n>  #include \"libcamera/internal/pub_key.h\"\n> @@ -28,7 +29,7 @@ LOG_DECLARE_CATEGORY(IPAManager)\n>  class IPAManager\n>  {\n>  public:\n> -       IPAManager();\n> +       IPAManager(const GlobalConfiguration &configuration);\n>         ~IPAManager();\n>  \n>         template<typename T>\n> @@ -42,7 +43,8 @@ public:\n>                 if (!m)\n>                         return nullptr;\n>  \n> -               std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));\n> +               const GlobalConfiguration &configuration = cm->_d()->configuration();\n> +               std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m), configuration);\n>                 if (!proxy->isValid()) {\n>                         LOG(IPAManager, Error) << \"Failed to load proxy\";\n>                         return nullptr;\n> @@ -73,6 +75,7 @@ private:\n>  #if HAVE_IPA_PUBKEY\n>         static const uint8_t publicKeyData_[];\n>         static const PubKey pubKey_;\n> +       bool forceIsolation_;\n>  #endif\n>  };\n>  \n> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h\n> index 983bcc5fa..f1865d67e 100644\n> --- a/include/libcamera/internal/ipa_proxy.h\n> +++ b/include/libcamera/internal/ipa_proxy.h\n> @@ -7,10 +7,14 @@\n>  \n>  #pragma once\n>  \n> +#include <optional>\n>  #include <string>\n> +#include <vector>\n>  \n>  #include <libcamera/ipa/ipa_interface.h>\n>  \n> +#include \"libcamera/internal/global_configuration.h\"\n> +\n>  namespace libcamera {\n>  \n>  class IPAModule;\n> @@ -24,7 +28,7 @@ public:\n>                 ProxyRunning,\n>         };\n>  \n> -       IPAProxy(IPAModule *ipam);\n> +       IPAProxy(IPAModule *ipam, const GlobalConfiguration &configuration);\n>         ~IPAProxy();\n>  \n>         bool isValid() const { return valid_; }\n> @@ -40,6 +44,8 @@ protected:\n>  \n>  private:\n>         IPAModule *ipam_;\n> +       std::vector<std::string> configPaths_;\n> +       std::vector<std::string> execPaths_;\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index dca3d9a83..64df62444 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -41,7 +41,7 @@ LOG_DEFINE_CATEGORY(Camera)\n>  CameraManager::Private::Private()\n>         : initialized_(false)\n>  {\n> -       ipaManager_ = std::make_unique<IPAManager>();\n> +       ipaManager_ = std::make_unique<IPAManager>(this->configuration());\n>  }\n>  \n>  int CameraManager::Private::start()\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index 830750dcc..000d56efa 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -9,13 +9,17 @@\n>  \n>  #include <algorithm>\n>  #include <dirent.h>\n> +#include <numeric>\n>  #include <string.h>\n> +#include <string>\n>  #include <sys/types.h>\n> +#include <vector>\n>  \n>  #include <libcamera/base/file.h>\n>  #include <libcamera/base/log.h>\n>  #include <libcamera/base/utils.h>\n>  \n> +#include \"libcamera/internal/global_configuration.h\"\n>  #include \"libcamera/internal/ipa_module.h\"\n>  #include \"libcamera/internal/ipa_proxy.h\"\n>  #include \"libcamera/internal/pipeline_handler.h\"\n> @@ -101,30 +105,36 @@ LOG_DEFINE_CATEGORY(IPAManager)\n>   * The IPAManager class is meant to only be instantiated once, by the\n>   * CameraManager.\n>   */\n> -IPAManager::IPAManager()\n> +IPAManager::IPAManager(const GlobalConfiguration &configuration)\n>  {\n>  #if HAVE_IPA_PUBKEY\n>         if (!pubKey_.isValid())\n>                 LOG(IPAManager, Warning) << \"Public key not valid\";\n> +\n> +       char *force = utils::secure_getenv(\"LIBCAMERA_IPA_FORCE_ISOLATION\");\n> +       forceIsolation_ = (force && force[0] != '\\0') ||\n> +                         (!force && configuration.option<bool>({ \"ipa\", \"force_isolation\" })\n> +                                            .value_or(false));\n>  #endif\n>  \n>         unsigned int ipaCount = 0;\n>  \n>         /* User-specified paths take precedence. */\n> -       const char *modulePaths = utils::secure_getenv(\"LIBCAMERA_IPA_MODULE_PATH\");\n> -       if (modulePaths) {\n> -               for (const auto &dir : utils::split(modulePaths, \":\")) {\n> -                       if (dir.empty())\n> -                               continue;\n> -\n> -                       ipaCount += addDir(dir.c_str());\n> -               }\n> +       const auto modulePaths =\n> +               configuration.envListOption(\n> +                       \"LIBCAMERA_IPA_MODULE_PATH\", { \"ipa\", \"module_paths\" });\n> +       for (const auto &dir : modulePaths.value_or(std::vector<std::string>())) {\n\nI just noticed that this is duplicated...\n\n> +               if (dir.empty())\n> +                       continue;\n>  \n> -               if (!ipaCount)\n> -                       LOG(IPAManager, Warning)\n> -                               << \"No IPA found in '\" << modulePaths << \"'\";\n> +               ipaCount += addDir(dir.c_str());\n>         }\n>  \n> +       if (!ipaCount)\n> +               LOG(IPAManager, Warning) << \"No IPA found in '\"\n> +                                        << utils::join(modulePaths.value_or(std::vector<std::string>()), \":\")\n\n...here. Maybe it's better to move the .value_or(std::vector<std::string>()) to\nthe modulePaths definition?\n\nWith these changes,\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n> +                                        << \"'\";\n> +\n>         /*\n>          * When libcamera is used before it is installed, load IPAs from the\n>          * same build directory as the libcamera library itself.\n> @@ -279,11 +289,10 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n>  bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const\n>  {\n>  #if HAVE_IPA_PUBKEY\n> -       char *force = utils::secure_getenv(\"LIBCAMERA_IPA_FORCE_ISOLATION\");\n> -       if (force && force[0] != '\\0') {\n> +       if (forceIsolation_) {\n>                 LOG(IPAManager, Debug)\n>                         << \"Isolation of IPA module \" << ipa->path()\n> -                       << \" forced through environment variable\";\n> +                       << \" forced through configuration\";\n>                 return false;\n>         }\n>  \n> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp\n> index b5c13a30f..9d2acd716 100644\n> --- a/src/libcamera/ipa_proxy.cpp\n> +++ b/src/libcamera/ipa_proxy.cpp\n> @@ -7,13 +7,16 @@\n>  \n>  #include \"libcamera/internal/ipa_proxy.h\"\n>  \n> +#include <string>\n>  #include <sys/stat.h>\n>  #include <sys/types.h>\n>  #include <unistd.h>\n> +#include <vector>\n>  \n>  #include <libcamera/base/log.h>\n>  #include <libcamera/base/utils.h>\n>  \n> +#include \"libcamera/internal/global_configuration.h\"\n>  #include \"libcamera/internal/ipa_module.h\"\n>  \n>  /**\n> @@ -27,7 +30,8 @@ LOG_DEFINE_CATEGORY(IPAProxy)\n>  \n>  namespace {\n>  \n> -std::string ipaConfigurationFile(const std::string &ipaName, const std::string &name)\n> +std::string ipaConfigurationFile(const std::string &ipaName, const std::string &name,\n> +                                const std::vector<std::string> &configPaths)\n>  {\n>         /*\n>          * Start with any user override through the module-specific environment\n> @@ -47,20 +51,15 @@ std::string ipaConfigurationFile(const std::string &ipaName, const std::string &\n>         int ret;\n>  \n>         /*\n> -        * Check the directory pointed to by the IPA config path environment\n> -        * variable next.\n> +        * Check the directory pointed to by the IPA config path next.\n>          */\n> -       const char *confPaths = utils::secure_getenv(\"LIBCAMERA_IPA_CONFIG_PATH\");\n> -       if (confPaths) {\n> -               for (const auto &dir : utils::split(confPaths, \":\")) {\n> -                       if (dir.empty())\n> -                               continue;\n> -\n> -                       std::string confPath = dir + \"/\" + ipaName + \"/\" + name;\n> -                       ret = stat(confPath.c_str(), &statbuf);\n> -                       if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)\n> -                               return confPath;\n> -               }\n> +       for (const auto &dir : configPaths) {\n> +               if (dir.empty())\n> +                       continue;\n> +               std::string confPath = dir + \"/\" + ipaName + \"/\" + name;\n> +               ret = stat(confPath.c_str(), &statbuf);\n> +               if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)\n> +                       return confPath;\n>         }\n>  \n>         std::string root = utils::libcameraSourcePath();\n> @@ -120,9 +119,12 @@ std::string ipaConfigurationFile(const std::string &ipaName, const std::string &\n>  /**\n>   * \\brief Construct an IPAProxy instance\n>   * \\param[in] ipam The IPA module\n> + * \\param[in] configuration The global configuration\n>   */\n> -IPAProxy::IPAProxy(IPAModule *ipam)\n> -       : valid_(false), state_(ProxyStopped), ipam_(ipam)\n> +IPAProxy::IPAProxy(IPAModule *ipam, const GlobalConfiguration &configuration)\n> +       : valid_(false), state_(ProxyStopped), ipam_(ipam),\n> +         configPaths_(configuration.envListOption(\"LIBCAMERA_IPA_CONFIG_PATH\", { \"ipa\", \"config_paths\" }).value_or(std::vector<std::string>())),\n> +         execPaths_(configuration.envListOption(\"LIBCAMERA_IPA_PROXY_PATH\", { \"ipa\", \"proxy_paths\" }).value_or(std::vector<std::string>()))\n>  {\n>  }\n>  \n> @@ -175,7 +177,7 @@ std::string IPAProxy::configurationFile(const std::string &name,\n>          * has been validated when loading the module.\n>          */\n>         const std::string ipaName = ipam_->info().name;\n> -       std::string confPath = ipaConfigurationFile(ipaName, name);\n> +       std::string confPath = ipaConfigurationFile(ipaName, name, configPaths_);\n>         if (!confPath.empty()) {\n>                 LOG(IPAProxy, Info) << \"Using tuning file \" << confPath;\n>                 return confPath;\n> @@ -188,7 +190,7 @@ std::string IPAProxy::configurationFile(const std::string &name,\n>                 return std::string();\n>         }\n>  \n> -       confPath = ipaConfigurationFile(ipaName, fallbackName);\n> +       confPath = ipaConfigurationFile(ipaName, fallbackName, configPaths_);\n>         LOG(IPAProxy, Warning)\n>                 << \"Configuration file '\" << name\n>                 << \"' not found for IPA module '\" << ipaName\n> @@ -214,18 +216,14 @@ std::string IPAProxy::resolvePath(const std::string &file) const\n>  {\n>         std::string proxyFile = \"/\" + file;\n>  \n> -       /* Check env variable first. */\n> -       const char *execPaths = utils::secure_getenv(\"LIBCAMERA_IPA_PROXY_PATH\");\n> -       if (execPaths) {\n> -               for (const auto &dir : utils::split(execPaths, \":\")) {\n> -                       if (dir.empty())\n> -                               continue;\n> -\n> -                       std::string proxyPath = dir;\n> -                       proxyPath += proxyFile;\n> -                       if (!access(proxyPath.c_str(), X_OK))\n> -                               return proxyPath;\n> -               }\n> +       /* Check the configuration first. */\n> +       for (const auto &dir : execPaths_) {\n> +               if (dir.empty())\n> +                       continue;\n> +\n> +               std::string proxyPath = dir + proxyFile;\n> +               if (!access(proxyPath.c_str(), X_OK))\n> +                       return proxyPath;\n>         }\n>  \n>         /*\n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index beb646e2d..18b4ab5e5 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -45,8 +45,8 @@ namespace {{ns}} {\n>  {% endfor %}\n>  {%- endif %}\n>  \n> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> -       : IPAProxy(ipam), isolate_(isolate),\n> +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration)\n> +       : IPAProxy(ipam, configuration), isolate_(isolate),\n>           controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)\n>  {\n>         LOG(IPAProxy, Debug)\n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> index a0312a7c1..057c3ab03 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -37,7 +37,7 @@ namespace {{ns}} {\n>  class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n>  {\n>  public:\n> -       {{proxy_name}}(IPAModule *ipam, bool isolate);\n> +       {{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration);\n>         ~{{proxy_name}}();\n>  \n>  {% for method in interface_main.methods %}\n> -- \n> 2.51.0\n>","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 079F0C324E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 12 Sep 2025 08:49:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4CBC6936E;\n\tFri, 12 Sep 2025 10:48:59 +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 5E172613A3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 12 Sep 2025 10:48:58 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:5c83:71e0:e0c7:1670])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 388D26DC;\n\tFri, 12 Sep 2025 10:47:41 +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=\"ggreV5zM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1757666862;\n\tbh=+mHqg/D04Bbq7pbIlTQANzYgUyr/bQtM82YYt3/8O6M=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ggreV5zMUccg8guwe3QGuwjYDxryd5VUYwJ3oRx5Qm9Ja27VDiMA++GICsc3G4HzR\n\twRbHEO99We9Vs+j2AwxKUzq8JQJ5DSrPXyX4oKx9Zns19pRDejxzybra8+uRj0b0AO\n\tzC5eeNt9itsCdaRJyancEM72QN4LGxJps2FK0rAY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250911092945.16517-6-mzamazal@redhat.com>","References":"<20250911092945.16517-1-mzamazal@redhat.com>\n\t<20250911092945.16517-6-mzamazal@redhat.com>","Subject":"Re: [PATCH v17 05/12] config: Look up IPA configurables in\n\tconfiguration file","From":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"Milan Zamazal <mzamazal@redhat.com>, Kieran Bingham\n\t<kieran.bingham@ideasonboard.com>, =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t<barnabas.pocze@ideasonboard.com>, Laurent Pinchart\n\t<laurent.pinchart@ideasonboard.com>","To":"Milan Zamazal <mzamazal@redhat.com>, libcamera-devel@lists.libcamera.org","Date":"Fri, 12 Sep 2025 17:48:50 +0900","Message-ID":"<175766693084.2127323.1861641841473360224@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]