[{"id":35734,"web_url":"https://patchwork.libcamera.org/comment/35734/","msgid":"<175740459786.2127323.3227796486360043384@neptunite.rasen.tech>","date":"2025-09-09T07:56:37","subject":"Re: [PATCH v16 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-07-29 16:31:53)\n> This patch adds configuration options for environment variables used in\n> the IPA proxy. Two utility functions configuration retrieval functions\n\ns/Two.*$/Two utility functions for retrieving lists of configuration values/\n\n> are added, to retrieve lists of values.\n\ns/,.*$/\\./\n\nPersonally I feel like the retrieval functions should just all be added in\n3/12. Then in this patch we can just plumb it into the IPAs.\n\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__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\nI thought it would be fine (or even desirable) to allow configuring the path to\nthe tuning files in the configuration file? It does make sense to keep the\nenvironment variable though.\n\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 \n> ---\n> .../libcamera/internal/global_configuration.h | 21 +++++++-\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/global_configuration.cpp | 47 +++++++++++++++--\n> src/libcamera/ipa_manager.cpp | 39 ++++++++------\n> src/libcamera/ipa_proxy.cpp | 51 +++++++++----------\n> .../module_ipa_proxy.cpp.tmpl | 4 +-\n> .../module_ipa_proxy.h.tmpl | 2 +-\n> 9 files changed, 128 insertions(+), 53 deletions(-)\n> \n> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h\n> index 78bebff9f..3b500ee29 100644\n> --- a/include/libcamera/internal/global_configuration.h\n> +++ b/include/libcamera/internal/global_configuration.h\n> @@ -12,6 +12,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/yaml_parser.h\"\n> \n> namespace libcamera {\n> @@ -25,11 +27,28 @@ public:\n> \n> unsigned int version() const;\n> Configuration configuration() const;\n> - std::optional option(\n> +\n> + template\n> + std::optional option(\n> + const std::initializer_list confPath) const\n> + {\n> + const YamlObject *c = &configuration();\n> + for (auto part : confPath) {\n> + c = &(*c)[part];\n> + if (!*c)\n> + return {};\n> + }\n> + return c->get();\n> + }\n> +\n> + std::optional> listOption(\n> const std::initializer_list confPath) const;\n> std::optional envOption(\n> const char *const envVariable,\n> const std::initializer_list confPath) const;\n> + std::optional> envListOption(\n> + const char *const envVariable,\n> + const std::initializer_list confPath) const;\n> \n> private:\n> bool loadFile(const std::filesystem::path &fileName);\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 \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\n> @@ -42,7 +43,8 @@ public:\n> if (!m)\n> return nullptr;\n> \n> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n> + const GlobalConfiguration &configuration = cm->_d()->configuration();\n> + std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m), configuration);\n\nOk so we pass it in during construction and everybody that needs it keeps a\npointer to it, that looks good.\n\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 \n> #include \n> +#include \n> \n> #include \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 configPaths_;\n> + std::vector 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();\n> + ipaManager_ = std::make_unique(this->configuration());\n> }\n> \n> int CameraManager::Private::start()\n> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp\n> index 5be0f0701..1371392c5 100644\n> --- a/src/libcamera/global_configuration.cpp\n> +++ b/src/libcamera/global_configuration.cpp\n> @@ -9,9 +9,11 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -114,13 +116,22 @@ GlobalConfiguration::GlobalConfiguration()\n> */\n> \n> /**\n> + * \\fn std::optional GlobalConfiguration::option(const std::initializer_list &confPath) const\n> * \\brief Return value of the configuration option identified by \\a confPath\n> * \\param[in] confPath Sequence of the YAML section names (excluding\n> * `configuration') leading to the requested option\n> * \\return A value if an item corresponding to \\a confPath exists in the\n> * configuration file, no value otherwise\n> */\n> -std::optional GlobalConfiguration::option(\n> +\n> +/**\n> + * \\brief Return values of the configuration option identified by \\a confPath\n> + * \\tparam T The type of the retrieved configuration value\n> + * \\param[in] confPath Sequence of the YAML section names (excluding\n> + * `configuration') leading to the requested list option, separated by dots\n> + * \\return A vector of strings or no value if not found\n> + */\n> +std::optional> GlobalConfiguration::listOption(\n> const std::initializer_list confPath) const\n> {\n> const YamlObject *c = &configuration();\n> @@ -129,7 +140,7 @@ std::optional GlobalConfiguration::option(\n> if (!*c)\n> return {};\n> }\n> - return c->get();\n> + return c->getList();\n> }\n> \n> /**\n> @@ -156,7 +167,37 @@ std::optional GlobalConfiguration::envOption(\n> const char *envValue = utils::secure_getenv(envVariable);\n> if (envValue)\n> return std::optional{ std::string{ envValue } };\n> - return option(confPath);\n> + return option(confPath);\n> +}\n> +\n> +/**\n> + * \\brief Return values of the configuration option from a file or environment\n> + * \\param[in] envVariable Environment variable to get the value from\n> + * \\param[in] confPath The same as in GlobalConfiguration::option\n> + *\n> + * This helper looks first at the given environment variable and if it is\n> + * defined (even if it is empty) then it splits its value by semicolons and\n> + * returns the resulting list of strings. Otherwise it looks for \\a confPath the\n> + * same way as in GlobalConfiguration::option, value of which must be a list of\n> + * strings.\n\nSame comments from 3/12.\n\n> + *\n> + * \\note Support for using environment variables to configure libcamera behavior\n> + * is provided here mostly for backward compatibility reasons. Introducing new\n> + * configuration environment variables is discouraged.\n> + *\n> + * \\return A vector of strings retrieved from the given environment option or\n> + * configuration file or no value if not found; the vector may be empty\n> + */\n> +std::optional> GlobalConfiguration::envListOption(\n> + const char *const envVariable,\n> + const std::initializer_list confPath) const\n> +{\n> + const char *envValue = utils::secure_getenv(envVariable);\n> + if (envValue) {\n> + auto items = utils::split(envValue, \":\");\n> + return std::vector(items.begin(), items.end());\n> + }\n> + return listOption(confPath);\n> }\n> \n> /**\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 \n> #include \n> +#include \n> #include \n> +#include \n> #include \n> +#include \n> \n> #include \n> #include \n> #include \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({ \"ipa\", \"force_isolation\" })\n> + .value_or(false));\n\nAlthough the environment variable doesn't let us do it, I wonder if there's\nvalue in letting the configuration file configure which IPAs we want to\nforce-isolate and which we don't.\n\nAlso is there a reason why we don't use envOption() here? It is because we\ncan't get a bool out of env?\n\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())) {\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()), \":\")\n> + << \"'\";\n> +\n\nOk, looks good.\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 9907b9615..b845e9086 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 \n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> \n> +#include \"libcamera/internal/global_configuration.h\"\n> #include \"libcamera/internal/ipa_module.h\"\n> \n> /**\n> @@ -48,9 +51,12 @@ LOG_DEFINE_CATEGORY(IPAProxy)\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())),\n> + execPaths_(configuration.envListOption(\"LIBCAMERA_IPA_PROXY_PATH\", { \"ipa\", \"proxy_paths\" }).value_or(std::vector()))\n\nOh that's cool that we can do that now.\n\nThe rest looks good!\n\n\nThanks,\n\nPaul\n\n> {\n> }\n> \n> @@ -122,20 +128,15 @@ std::string IPAProxy::configurationFile(const std::string &name,\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> @@ -199,18 +200,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 9a3aadbd2..0f87e7976 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.50.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 ED95ABDB13\n\tfor ;\n\tTue, 9 Sep 2025 07:56:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAC446935F;\n\tTue, 9 Sep 2025 09:56:48 +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 07CA869339\n\tfor ;\n\tTue, 9 Sep 2025 09:56:44 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:6f3a:4f34:f1fa:8b3])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 23D7D6A6;\n\tTue, 9 Sep 2025 09:55: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=\"knuYHCpd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1757404532;\n\tbh=iDLTUFBqmDkeFVmoWVahXzs+jt5vgYGG7eJoNu/1jUg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=knuYHCpdEHHN8FFGoXot+/y6gMZN1C7ObPkH+1y+Qzj+c84dT2tB2x3B+GPDWici0\n\t4b4J+ZEHBW/Ct/j9rrRLgUvzjtw/lPDlQlRCnPCNxq8BCi39eGo6pa0wN7j+ix8ofZ\n\ttyflM+zlZLNEav6x30LjkaHgQaTuzfhdGikFLo6k=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250729073201.5369-6-mzamazal@redhat.com>","References":"<20250729073201.5369-1-mzamazal@redhat.com>\n\t<20250729073201.5369-6-mzamazal@redhat.com>","Subject":"Re: [PATCH v16 05/12] config: Look up IPA configurables in\n\tconfiguration file","From":"Paul Elder ","Cc":"Milan Zamazal , Kieran Bingham\n\t, =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t, Laurent Pinchart\n\t","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Sep 2025 16:56:37 +0900","Message-ID":"<175740459786.2127323.3227796486360043384@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":"","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":35742,"web_url":"https://patchwork.libcamera.org/comment/35742/","msgid":"<85cy7zrd0v.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-09T13:06:24","subject":"Re: [PATCH v16 05/12] config: Look up IPA configurables in\n\tconfiguration file","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Paul,\n\nthank you for review.\n\nPaul Elder writes:\n\n> Hi Milan,\n>\n> Thanks for the patch.\n>\n> Quoting Milan Zamazal (2025-07-29 16:31:53)\n>> This patch adds configuration options for environment variables used in\n>> the IPA proxy. Two utility functions configuration retrieval functions\n>\n> s/Two.*$/Two utility functions for retrieving lists of configuration values/\n\nOK.\n\n>> are added, to retrieve lists of values.\n>\n> s/,.*$/\\./\n>\n> Personally I feel like the retrieval functions should just all be added in\n> 3/12. Then in this patch we can just plumb it into the IPAs.\n\nSome reviewers (not necessarily in libcamera) don't like adding such\nstuff before it is used, I don't mind either way. I can move them.\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__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> I thought it would be fine (or even desirable) to allow configuring the path to\n> the tuning files in the configuration file? It does make sense to keep the\n> environment variable though.\n\nWell, this has been discussed in\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2025-June/051202.html,\nwhere Barnabás said:\n\n I think `LIBCAMERA_${IPA}_TUNING_FILE` is mainly a testing feature\n because it overrides the tuning file \"globally\" for the given IPA. My\n first impression was that it is probably not that useful to have in a\n configuration file. And I still think that, so my preference is not\n including it in the configuration file. With that, feel free to decide\n\nSo I removed the options (less things to review :-P) but I can put them\nback if they are useful. The naming issue doesn't exist any more.\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 \n>> ---\n>> .../libcamera/internal/global_configuration.h | 21 +++++++-\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/global_configuration.cpp | 47 +++++++++++++++--\n>> src/libcamera/ipa_manager.cpp | 39 ++++++++------\n>> src/libcamera/ipa_proxy.cpp | 51 +++++++++----------\n>> .../module_ipa_proxy.cpp.tmpl | 4 +-\n>> .../module_ipa_proxy.h.tmpl | 2 +-\n>> 9 files changed, 128 insertions(+), 53 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h\n>> index 78bebff9f..3b500ee29 100644\n>> --- a/include/libcamera/internal/global_configuration.h\n>> +++ b/include/libcamera/internal/global_configuration.h\n>> @@ -12,6 +12,8 @@\n>> #include \n>> #include \n>> \n>> +#include \n>> +\n>> #include \"libcamera/internal/yaml_parser.h\"\n>> \n>> namespace libcamera {\n>> @@ -25,11 +27,28 @@ public:\n>> \n>> unsigned int version() const;\n>> Configuration configuration() const;\n>> - std::optional option(\n>> +\n>> + template\n>> + std::optional option(\n>> + const std::initializer_list confPath) const\n>> + {\n>> + const YamlObject *c = &configuration();\n>> + for (auto part : confPath) {\n>> + c = &(*c)[part];\n>> + if (!*c)\n>> + return {};\n>> + }\n>> + return c->get();\n>> + }\n>> +\n>> + std::optional> listOption(\n>> const std::initializer_list confPath) const;\n>> std::optional envOption(\n>> const char *const envVariable,\n>> const std::initializer_list confPath) const;\n>> + std::optional> envListOption(\n>> + const char *const envVariable,\n>> + const std::initializer_list confPath) const;\n>> \n>> private:\n>> bool loadFile(const std::filesystem::path &fileName);\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 \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\n>> @@ -42,7 +43,8 @@ public:\n>> if (!m)\n>> return nullptr;\n>> \n>> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n>> + const GlobalConfiguration &configuration = cm->_d()->configuration();\n>> + std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m), configuration);\n>\n> Ok so we pass it in during construction and everybody that needs it keeps a\n> pointer to it, that looks good.\n>\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 \n>> #include \n>> +#include \n>> \n>> #include \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 configPaths_;\n>> + std::vector 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();\n>> + ipaManager_ = std::make_unique(this->configuration());\n>> }\n>> \n>> int CameraManager::Private::start()\n>> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp\n>> index 5be0f0701..1371392c5 100644\n>> --- a/src/libcamera/global_configuration.cpp\n>> +++ b/src/libcamera/global_configuration.cpp\n>> @@ -9,9 +9,11 @@\n>> \n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> +#include \n>> \n>> #include \n>> #include \n>> @@ -114,13 +116,22 @@ GlobalConfiguration::GlobalConfiguration()\n>> */\n>> \n>> /**\n>> + * \\fn std::optional GlobalConfiguration::option(const std::initializer_list &confPath) const\n>> * \\brief Return value of the configuration option identified by \\a confPath\n>> * \\param[in] confPath Sequence of the YAML section names (excluding\n>> * `configuration') leading to the requested option\n>> * \\return A value if an item corresponding to \\a confPath exists in the\n>> * configuration file, no value otherwise\n>> */\n>> -std::optional GlobalConfiguration::option(\n>> +\n>> +/**\n>> + * \\brief Return values of the configuration option identified by \\a confPath\n>> + * \\tparam T The type of the retrieved configuration value\n>> + * \\param[in] confPath Sequence of the YAML section names (excluding\n>> + * `configuration') leading to the requested list option, separated by dots\n>> + * \\return A vector of strings or no value if not found\n>> + */\n>> +std::optional> GlobalConfiguration::listOption(\n>> const std::initializer_list confPath) const\n>> {\n>> const YamlObject *c = &configuration();\n>> @@ -129,7 +140,7 @@ std::optional GlobalConfiguration::option(\n>> if (!*c)\n>> return {};\n>> }\n>> - return c->get();\n>> + return c->getList();\n>> }\n>> \n>> /**\n>> @@ -156,7 +167,37 @@ std::optional GlobalConfiguration::envOption(\n>> const char *envValue = utils::secure_getenv(envVariable);\n>> if (envValue)\n>> return std::optional{ std::string{ envValue } };\n>> - return option(confPath);\n>> + return option(confPath);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Return values of the configuration option from a file or environment\n>> + * \\param[in] envVariable Environment variable to get the value from\n>> + * \\param[in] confPath The same as in GlobalConfiguration::option\n>> + *\n>> + * This helper looks first at the given environment variable and if it is\n>> + * defined (even if it is empty) then it splits its value by semicolons and\n>> + * returns the resulting list of strings. Otherwise it looks for \\a confPath the\n>> + * same way as in GlobalConfiguration::option, value of which must be a list of\n>> + * strings.\n>\n> Same comments from 3/12.\n\nThe same responses :-).\n\n>> + *\n>> + * \\note Support for using environment variables to configure libcamera behavior\n>> + * is provided here mostly for backward compatibility reasons. Introducing new\n>> + * configuration environment variables is discouraged.\n>> + *\n>> + * \\return A vector of strings retrieved from the given environment option or\n>> + * configuration file or no value if not found; the vector may be empty\n>> + */\n>> +std::optional> GlobalConfiguration::envListOption(\n>> + const char *const envVariable,\n>> + const std::initializer_list confPath) const\n>> +{\n>> + const char *envValue = utils::secure_getenv(envVariable);\n>> + if (envValue) {\n>> + auto items = utils::split(envValue, \":\");\n>> + return std::vector(items.begin(), items.end());\n>> + }\n>> + return listOption(confPath);\n>> }\n>> \n>> /**\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 \n>> #include \n>> +#include \n>> #include \n>> +#include \n>> #include \n>> +#include \n>> \n>> #include \n>> #include \n>> #include \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({ \"ipa\", \"force_isolation\" })\n>> + .value_or(false));\n>\n> Although the environment variable doesn't let us do it, I wonder if there's\n> value in letting the configuration file configure which IPAs we want to\n> force-isolate and which we don't.\n\nI don't have any opinion on this.\n\n> Also is there a reason why we don't use envOption() here? It is because we\n> can't get a bool out of env?\n\nYes, IIRC I didn't want to introduce another helper for just a single use.\n\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())) {\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()), \":\")\n>> + << \"'\";\n>> +\n>\n> Ok, looks good.\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 9907b9615..b845e9086 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 \n>> #include \n>> #include \n>> #include \n>> +#include \n>> \n>> #include \n>> #include \n>> \n>> +#include \"libcamera/internal/global_configuration.h\"\n>> #include \"libcamera/internal/ipa_module.h\"\n>> \n>> /**\n>> @@ -48,9 +51,12 @@ LOG_DEFINE_CATEGORY(IPAProxy)\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())),\n>> + execPaths_(configuration.envListOption(\"LIBCAMERA_IPA_PROXY_PATH\", { \"ipa\", \"proxy_paths\" }).value_or(std::vector()))\n>\n> Oh that's cool that we can do that now.\n>\n> The rest looks good!\n>\n>\n> Thanks,\n>\n> Paul\n>\n>> {\n>> }\n>> \n>> @@ -122,20 +128,15 @@ std::string IPAProxy::configurationFile(const std::string &name,\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>> @@ -199,18 +200,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 9a3aadbd2..0f87e7976 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.50.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 118DFBDB13\n\tfor ;\n\tTue, 9 Sep 2025 13:06:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AC6069367;\n\tTue, 9 Sep 2025 15:06:34 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88ABC6934B\n\tfor ;\n\tTue, 9 Sep 2025 15:06:31 +0200 (CEST)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-678-7KtTfYs0PImWTWu3akmFtw-1; Tue, 09 Sep 2025 09:06:28 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-3e014bf8ebfso3252210f8f.0\n\tfor ;\n\tTue, 09 Sep 2025 06:06:27 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-3e7521bfdbbsm2794821f8f.10.2025.09.09.06.06.24\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 09 Sep 2025 06:06:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"gziRs3wv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1757423190;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=IJmG0pe4H77NNCDSWE9Qs7lQ3BOVWp/FtNUwhmvVJV4=;\n\tb=gziRs3wvCRrBvQYlYT1XGX+zwYNV7lUmhclC3rw79IFBCn0vyywTLpyNBAqy3+uBQ0vsTj\n\twNg/w++dgCfCuFEBSNZ3J/k6xPKgj4O/YFy3qXZluZSyPTak2imF5/szLhacfvNKd5PrH2\n\tFMaAd91bs5ckTn+OnmMA+HecP6Ps9mM=","X-MC-Unique":"7KtTfYs0PImWTWu3akmFtw-1","X-Mimecast-MFC-AGG-ID":"7KtTfYs0PImWTWu3akmFtw_1757423187","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1757423186; x=1758027986;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=9KJ77/xxOj30BR3+/WvWmT1yjix8O49is4KqaFN0Ets=;\n\tb=pMES8LuqdTEgZUocc/10RawhwQquRdZndDthCpU4kgXnxDMYR7c75GFVnGXq2eZQZl\n\twcte04zVEvl9IJPqc6bRx7CeEkLrpdeDatxkp/jtsntj9GTylrhg2Hs73PAvEN9ghiSd\n\trxU4wet5nGT0pSb3DyGza5dGj4yeyw+6bckBC3T8+KXd1OVJvzJs9EAQ2kgbuZ/kUopF\n\tu8bopY6QvVBmza1Dtw27s1Nn67vPdLsq1AuAn660EIWZZmeBrSSYGRAy0O0Qy0Gs8yvX\n\t9ShHaEU2Hx6ztc4r1vt1wuSzPTvFAorF/zNUYk23SOD9ubDTSgzCJPhRxTdKy9fGFjz9\n\ttOwg==","X-Gm-Message-State":"AOJu0YxF92qLxdDBMokz/rUy4qNiOnX//3nEm2DDw7X5n4GD6ovTyq7P\n\t+qG/lrHqjJajHIri0YXL6dXLLhbkY1rWxUP+F0xw4ERhBomd29XCLqntWOoT9lJtNdH7cpXUyyP\n\tSTW8TkE09kjpFrURUI9d1C4XqeD3R6lc9bLAF8vB9Ph6bp/jknDKw7CgLv8rBT/WxtzVlzaiXPV\n\t/imhld5Uk=","X-Gm-Gg":"ASbGnctYKDsyiqliThZzXWfOBG1CouOqBP0RKuy48mWxy1DT8yZGWI0OCYNbutfaSHg\n\t0bjU3gWeFJGd6ZUo7VEvIRccg5+o7TzpNXa7p9UhRL+Z4Qg0IUkNH5pKcYEfBEDPOSzBaRMDKA7\n\tNSwK5gWlF7v5roXwCML1qlIxFyRGN2qxzVu3BZ7ITa4thKQHmqRbXv/yTVS+pHpc508u/FUsxNe\n\tIMrlStjDr3SkgujjXBjSEcvS72D3j5qPbltYaaCqafGHUHt2+7W1ZAhLfpdr7Nf3cY7mtk/TJ1K\n\tvbHoc/wm+gJmkjU7E0AWFYuV46C1RhsXV5XY7Swlx2Ip7H0Me0yywWeM1ss9FlKGig==","X-Received":["by 2002:a05:6000:1a87:b0:3e2:e851:7f93 with SMTP id\n\tffacd0b85a97d-3e300ae3446mr14151515f8f.7.1757423186231; \n\tTue, 09 Sep 2025 06:06:26 -0700 (PDT)","by 2002:a05:6000:1a87:b0:3e2:e851:7f93 with SMTP id\n\tffacd0b85a97d-3e300ae3446mr14151479f8f.7.1757423185585; \n\tTue, 09 Sep 2025 06:06:25 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFX2FuHOFTPFbfW18lIOSxFCRjuG8EdO6tx+w8go/upJn9M/eL7BsbWrPGzqz1GJb53RaRYGA==","From":"Milan Zamazal ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham , =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?= ,\n\tLaurent Pinchart ","Subject":"Re: [PATCH v16 05/12] config: Look up IPA configurables in\n\tconfiguration file","In-Reply-To":"<175740459786.2127323.3227796486360043384@neptunite.rasen.tech>\n\t(Paul Elder's message of \"Tue, 09 Sep 2025 16:56:37 +0900\")","References":"<20250729073201.5369-1-mzamazal@redhat.com>\n\t<20250729073201.5369-6-mzamazal@redhat.com>\n\t<175740459786.2127323.3227796486360043384@neptunite.rasen.tech>","Date":"Tue, 09 Sep 2025 15:06:24 +0200","Message-ID":"<85cy7zrd0v.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"noWwN82P-RBzzWJIzO7sbz9nYwciaIYzyfy8MAvQv8w_1757423187","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":35753,"web_url":"https://patchwork.libcamera.org/comment/35753/","msgid":"<175742872226.2127323.14663302897797752341@neptunite.rasen.tech>","date":"2025-09-09T14:38:42","subject":"Re: [PATCH v16 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":"Quoting Milan Zamazal (2025-09-09 22:06:24)\n> Hi Paul,\n> \n> thank you for review.\n> \n> Paul Elder writes:\n> \n> > Hi Milan,\n> >\n> > Thanks for the patch.\n> >\n> > Quoting Milan Zamazal (2025-07-29 16:31:53)\n> >> This patch adds configuration options for environment variables used in\n> >> the IPA proxy. Two utility functions configuration retrieval functions\n> >\n> > s/Two.*$/Two utility functions for retrieving lists of configuration values/\n> \n> OK.\n> \n> >> are added, to retrieve lists of values.\n> >\n> > s/,.*$/\\./\n> >\n> > Personally I feel like the retrieval functions should just all be added in\n> > 3/12. Then in this patch we can just plumb it into the IPAs.\n> \n> Some reviewers (not necessarily in libcamera) don't like adding such\n> stuff before it is used, I don't mind either way. I can move them.\n\nOh I can imagine :S\n\nimo it's fine to add them before it's used as long as it gets used later in the\nsame series (and it's easy to review).\n\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__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> > I thought it would be fine (or even desirable) to allow configuring the path to\n> > the tuning files in the configuration file? It does make sense to keep the\n> > environment variable though.\n> \n> Well, this has been discussed in\n> https://lists.libcamera.org/pipermail/libcamera-devel/2025-June/051202.html,\n\nThanks for the link.\n\n> where Barnabás said:\n> \n> I think `LIBCAMERA_${IPA}_TUNING_FILE` is mainly a testing feature\n> because it overrides the tuning file \"globally\" for the given IPA. My\n> first impression was that it is probably not that useful to have in a\n> configuration file. And I still think that, so my preference is not\n> including it in the configuration file. With that, feel free to decide\n\nAh, I see.\n\n> \n> So I removed the options (less things to review :-P) but I can put them\n> back if they are useful. The naming issue doesn't exist any more.\n\nOne of the big features that we want from the configuration file is actually to\nbe able to select the tuning file. However, I don't think directly\nreimplementing LIBCAMERA_${IPA}_TUNING_FILE is the solution. So you can indeed\nkeep it out.\n\nAs for this feature that we want we can do it on top. I think it needs a bit\nmore refining how we define it that doesn't deserve to be a blocker to merge\nthis series.\n\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 \n> >> ---\n> >> .../libcamera/internal/global_configuration.h | 21 +++++++-\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/global_configuration.cpp | 47 +++++++++++++++--\n> >> src/libcamera/ipa_manager.cpp | 39 ++++++++------\n> >> src/libcamera/ipa_proxy.cpp | 51 +++++++++----------\n> >> .../module_ipa_proxy.cpp.tmpl | 4 +-\n> >> .../module_ipa_proxy.h.tmpl | 2 +-\n> >> 9 files changed, 128 insertions(+), 53 deletions(-)\n> >> \n> >> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h\n> >> index 78bebff9f..3b500ee29 100644\n> >> --- a/include/libcamera/internal/global_configuration.h\n> >> +++ b/include/libcamera/internal/global_configuration.h\n> >> @@ -12,6 +12,8 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \n> >> +\n> >> #include \"libcamera/internal/yaml_parser.h\"\n> >> \n> >> namespace libcamera {\n> >> @@ -25,11 +27,28 @@ public:\n> >> \n> >> unsigned int version() const;\n> >> Configuration configuration() const;\n> >> - std::optional option(\n> >> +\n> >> + template\n> >> + std::optional option(\n> >> + const std::initializer_list confPath) const\n> >> + {\n> >> + const YamlObject *c = &configuration();\n> >> + for (auto part : confPath) {\n> >> + c = &(*c)[part];\n> >> + if (!*c)\n> >> + return {};\n> >> + }\n> >> + return c->get();\n> >> + }\n> >> +\n> >> + std::optional> listOption(\n> >> const std::initializer_list confPath) const;\n> >> std::optional envOption(\n> >> const char *const envVariable,\n> >> const std::initializer_list confPath) const;\n> >> + std::optional> envListOption(\n> >> + const char *const envVariable,\n> >> + const std::initializer_list confPath) const;\n> >> \n> >> private:\n> >> bool loadFile(const std::filesystem::path &fileName);\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 \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\n> >> @@ -42,7 +43,8 @@ public:\n> >> if (!m)\n> >> return nullptr;\n> >> \n> >> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n> >> + const GlobalConfiguration &configuration = cm->_d()->configuration();\n> >> + std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m), configuration);\n> >\n> > Ok so we pass it in during construction and everybody that needs it keeps a\n> > pointer to it, that looks good.\n> >\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 \n> >> #include \n> >> +#include \n> >> \n> >> #include \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 configPaths_;\n> >> + std::vector 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();\n> >> + ipaManager_ = std::make_unique(this->configuration());\n> >> }\n> >> \n> >> int CameraManager::Private::start()\n> >> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp\n> >> index 5be0f0701..1371392c5 100644\n> >> --- a/src/libcamera/global_configuration.cpp\n> >> +++ b/src/libcamera/global_configuration.cpp\n> >> @@ -9,9 +9,11 @@\n> >> \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> \n> >> #include \n> >> #include \n> >> @@ -114,13 +116,22 @@ GlobalConfiguration::GlobalConfiguration()\n> >> */\n> >> \n> >> /**\n> >> + * \\fn std::optional GlobalConfiguration::option(const std::initializer_list &confPath) const\n> >> * \\brief Return value of the configuration option identified by \\a confPath\n> >> * \\param[in] confPath Sequence of the YAML section names (excluding\n> >> * `configuration') leading to the requested option\n> >> * \\return A value if an item corresponding to \\a confPath exists in the\n> >> * configuration file, no value otherwise\n> >> */\n> >> -std::optional GlobalConfiguration::option(\n> >> +\n> >> +/**\n> >> + * \\brief Return values of the configuration option identified by \\a confPath\n> >> + * \\tparam T The type of the retrieved configuration value\n> >> + * \\param[in] confPath Sequence of the YAML section names (excluding\n> >> + * `configuration') leading to the requested list option, separated by dots\n> >> + * \\return A vector of strings or no value if not found\n> >> + */\n> >> +std::optional> GlobalConfiguration::listOption(\n> >> const std::initializer_list confPath) const\n> >> {\n> >> const YamlObject *c = &configuration();\n> >> @@ -129,7 +140,7 @@ std::optional GlobalConfiguration::option(\n> >> if (!*c)\n> >> return {};\n> >> }\n> >> - return c->get();\n> >> + return c->getList();\n> >> }\n> >> \n> >> /**\n> >> @@ -156,7 +167,37 @@ std::optional GlobalConfiguration::envOption(\n> >> const char *envValue = utils::secure_getenv(envVariable);\n> >> if (envValue)\n> >> return std::optional{ std::string{ envValue } };\n> >> - return option(confPath);\n> >> + return option(confPath);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Return values of the configuration option from a file or environment\n> >> + * \\param[in] envVariable Environment variable to get the value from\n> >> + * \\param[in] confPath The same as in GlobalConfiguration::option\n> >> + *\n> >> + * This helper looks first at the given environment variable and if it is\n> >> + * defined (even if it is empty) then it splits its value by semicolons and\n> >> + * returns the resulting list of strings. Otherwise it looks for \\a confPath the\n> >> + * same way as in GlobalConfiguration::option, value of which must be a list of\n> >> + * strings.\n> >\n> > Same comments from 3/12.\n> \n> The same responses :-).\n> \n> >> + *\n> >> + * \\note Support for using environment variables to configure libcamera behavior\n> >> + * is provided here mostly for backward compatibility reasons. Introducing new\n> >> + * configuration environment variables is discouraged.\n> >> + *\n> >> + * \\return A vector of strings retrieved from the given environment option or\n> >> + * configuration file or no value if not found; the vector may be empty\n> >> + */\n> >> +std::optional> GlobalConfiguration::envListOption(\n> >> + const char *const envVariable,\n> >> + const std::initializer_list confPath) const\n> >> +{\n> >> + const char *envValue = utils::secure_getenv(envVariable);\n> >> + if (envValue) {\n> >> + auto items = utils::split(envValue, \":\");\n> >> + return std::vector(items.begin(), items.end());\n> >> + }\n> >> + return listOption(confPath);\n> >> }\n> >> \n> >> /**\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 \n> >> #include \n> >> +#include \n> >> #include \n> >> +#include \n> >> #include \n> >> +#include \n> >> \n> >> #include \n> >> #include \n> >> #include \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({ \"ipa\", \"force_isolation\" })\n> >> + .value_or(false));\n> >\n> > Although the environment variable doesn't let us do it, I wonder if there's\n> > value in letting the configuration file configure which IPAs we want to\n> > force-isolate and which we don't.\n> \n> I don't have any opinion on this.\n\nOk. I guess we can add this in the future if somebody wants it. This framework\nis well-designed that it should be easy to add.\n\n> \n> > Also is there a reason why we don't use envOption() here? It is because we\n> > can't get a bool out of env?\n> \n> Yes, IIRC I didn't want to introduce another helper for just a single use.\n\nOk, that's fine then.\n\n\nThanks,\n\nPaul\n\n> \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())) {\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()), \":\")\n> >> + << \"'\";\n> >> +\n> >\n> > Ok, looks good.\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 9907b9615..b845e9086 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 \n> >> #include \n> >> #include \n> >> #include \n> >> +#include \n> >> \n> >> #include \n> >> #include \n> >> \n> >> +#include \"libcamera/internal/global_configuration.h\"\n> >> #include \"libcamera/internal/ipa_module.h\"\n> >> \n> >> /**\n> >> @@ -48,9 +51,12 @@ LOG_DEFINE_CATEGORY(IPAProxy)\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())),\n> >> + execPaths_(configuration.envListOption(\"LIBCAMERA_IPA_PROXY_PATH\", { \"ipa\", \"proxy_paths\" }).value_or(std::vector()))\n> >\n> > Oh that's cool that we can do that now.\n> >\n> > The rest looks good!\n> >\n> >\n> > Thanks,\n> >\n> > Paul\n> >\n> >> {\n> >> }\n> >> \n> >> @@ -122,20 +128,15 @@ std::string IPAProxy::configurationFile(const std::string &name,\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> >> @@ -199,18 +200,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 9a3aadbd2..0f87e7976 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.50.1\n> >>\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 110D3BDB13\n\tfor ;\n\tTue, 9 Sep 2025 14:38:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D754C6936D;\n\tTue, 9 Sep 2025 16:38:50 +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 149E16934B\n\tfor ;\n\tTue, 9 Sep 2025 16:38:49 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:6f3a:4f34:f1fa:8b3])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 1B5295B3;\n\tTue, 9 Sep 2025 16:37:34 +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=\"l4nqQf6z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1757428655;\n\tbh=aDsq3bxDpcuPaLoSdfqb/WFozO9d30i0iuNysN5daVw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=l4nqQf6z7uYJDDfM5VYzVxyYsyij4E6Z9uX+G0QJ+ZvOlk9jKqHwgt4zhzLNxtVUq\n\tyjIW4VblVpQKAZ4Ep8JKQa6rk2X/k5bKob7Mxcn1ISXYmh4tqvNynWpWVAY9WEcuUe\n\tOYY17rF6LzvVm5Welbcwhz2WWiutfySNFThr36Lo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<85cy7zrd0v.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","References":"<20250729073201.5369-1-mzamazal@redhat.com>\n\t<20250729073201.5369-6-mzamazal@redhat.com>\n\t<175740459786.2127323.3227796486360043384@neptunite.rasen.tech>\n\t<85cy7zrd0v.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Subject":"Re: [PATCH v16 05/12] config: Look up IPA configurables in\n\tconfiguration file","From":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t, Laurent Pinchart\n\t","To":"Milan Zamazal ","Date":"Tue, 09 Sep 2025 23:38:42 +0900","Message-ID":"<175742872226.2127323.14663302897797752341@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":"","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":35758,"web_url":"https://patchwork.libcamera.org/comment/35758/","msgid":"<85qzwfpsg4.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-09T15:16:11","subject":"Re: [PATCH v16 05/12] config: Look up IPA configurables in\n\tconfiguration file","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Paul Elder writes:\n\n> Quoting Milan Zamazal (2025-09-09 22:06:24)\n>> Hi Paul,\n>> \n>\n>> thank you for review.\n>> \n>> Paul Elder writes:\n>> \n>> > Hi Milan,\n>> >\n>> > Thanks for the patch.\n>> >\n>> > Quoting Milan Zamazal (2025-07-29 16:31:53)\n>> >> This patch adds configuration options for environment variables used in\n>> >> the IPA proxy. Two utility functions configuration retrieval functions\n>> >\n>> > s/Two.*$/Two utility functions for retrieving lists of configuration values/\n>> \n>> OK.\n>> \n>> >> are added, to retrieve lists of values.\n>> >\n>> > s/,.*$/\\./\n>> >\n>> > Personally I feel like the retrieval functions should just all be added in\n>> > 3/12. Then in this patch we can just plumb it into the IPAs.\n>> \n>> Some reviewers (not necessarily in libcamera) don't like adding such\n>> stuff before it is used, I don't mind either way. I can move them.\n>\n> Oh I can imagine :S\n>\n> imo it's fine to add them before it's used as long as it gets used later in the\n> same series (and it's easy to review).\n>\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__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>> > I thought it would be fine (or even desirable) to allow configuring the path to\n>> > the tuning files in the configuration file? It does make sense to keep the\n>> > environment variable though.\n>> \n>> Well, this has been discussed in\n>> https://lists.libcamera.org/pipermail/libcamera-devel/2025-June/051202.html,\n>\n> Thanks for the link.\n>\n>> where Barnabás said:\n>> \n>> I think `LIBCAMERA_${IPA}_TUNING_FILE` is mainly a testing feature\n>> because it overrides the tuning file \"globally\" for the given IPA. My\n>> first impression was that it is probably not that useful to have in a\n>> configuration file. And I still think that, so my preference is not\n>> including it in the configuration file. With that, feel free to decide\n>\n> Ah, I see.\n>\n>> \n>> So I removed the options (less things to review :-P) but I can put them\n>> back if they are useful. The naming issue doesn't exist any more.\n>\n> One of the big features that we want from the configuration file is actually to\n> be able to select the tuning file. However, I don't think directly\n> reimplementing LIBCAMERA_${IPA}_TUNING_FILE is the solution. So you can indeed\n> keep it out.\n>\n> As for this feature that we want we can do it on top. I think it needs a bit\n> more refining how we define it that doesn't deserve to be a blocker to merge\n> this series.\n\nYes, considering how long this series sits in review and adjustments,\nit's better to focus on what's necessary right now and leave the rest\nfor followup work.\n\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 \n>> >> ---\n>> >> .../libcamera/internal/global_configuration.h | 21 +++++++-\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/global_configuration.cpp | 47 +++++++++++++++--\n>> >> src/libcamera/ipa_manager.cpp | 39 ++++++++------\n>> >> src/libcamera/ipa_proxy.cpp | 51 +++++++++----------\n>> >> .../module_ipa_proxy.cpp.tmpl | 4 +-\n>> >> .../module_ipa_proxy.h.tmpl | 2 +-\n>> >> 9 files changed, 128 insertions(+), 53 deletions(-)\n>> >> \n>> >> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h\n>> >> index 78bebff9f..3b500ee29 100644\n>> >> --- a/include/libcamera/internal/global_configuration.h\n>> >> +++ b/include/libcamera/internal/global_configuration.h\n>> >> @@ -12,6 +12,8 @@\n>> >> #include \n>> >> #include \n>> >> \n>> >> +#include \n>> >> +\n>> >> #include \"libcamera/internal/yaml_parser.h\"\n>> >> \n>> >> namespace libcamera {\n>> >> @@ -25,11 +27,28 @@ public:\n>> >> \n>> >> unsigned int version() const;\n>> >> Configuration configuration() const;\n>> >> - std::optional option(\n>> >> +\n>> >> + template\n>> >> + std::optional option(\n>> >> + const std::initializer_list confPath) const\n>> >> + {\n>> >> + const YamlObject *c = &configuration();\n>> >> + for (auto part : confPath) {\n>> >> + c = &(*c)[part];\n>> >> + if (!*c)\n>> >> + return {};\n>> >> + }\n>> >> + return c->get();\n>> >> + }\n>> >> +\n>> >> + std::optional> listOption(\n>> >> const std::initializer_list confPath) const;\n>> >> std::optional envOption(\n>> >> const char *const envVariable,\n>> >> const std::initializer_list confPath) const;\n>> >> + std::optional> envListOption(\n>> >> + const char *const envVariable,\n>> >> + const std::initializer_list confPath) const;\n>> >> \n>> >> private:\n>> >> bool loadFile(const std::filesystem::path &fileName);\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 \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\n>> >> @@ -42,7 +43,8 @@ public:\n>> >> if (!m)\n>> >> return nullptr;\n>> >> \n>> >> - std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m));\n>> >> + const GlobalConfiguration &configuration = cm->_d()->configuration();\n>> >> + std::unique_ptr proxy = std::make_unique(m, !self->isSignatureValid(m), configuration);\n>> >\n>> > Ok so we pass it in during construction and everybody that needs it keeps a\n>> > pointer to it, that looks good.\n>> >\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 \n>> >> #include \n>> >> +#include \n>> >> \n>> >> #include \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 configPaths_;\n>> >> + std::vector 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();\n>> >> + ipaManager_ = std::make_unique(this->configuration());\n>> >> }\n>> >> \n>> >> int CameraManager::Private::start()\n>> >> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp\n>> >> index 5be0f0701..1371392c5 100644\n>> >> --- a/src/libcamera/global_configuration.cpp\n>> >> +++ b/src/libcamera/global_configuration.cpp\n>> >> @@ -9,9 +9,11 @@\n>> >> \n>> >> #include \n>> >> #include \n>> >> +#include \n>> >> #include \n>> >> #include \n>> >> #include \n>> >> +#include \n>> >> \n>> >> #include \n>> >> #include \n>> >> @@ -114,13 +116,22 @@ GlobalConfiguration::GlobalConfiguration()\n>> >> */\n>> >> \n>> >> /**\n>> >> + * \\fn std::optional GlobalConfiguration::option(const std::initializer_list &confPath) const\n>> >> * \\brief Return value of the configuration option identified by \\a confPath\n>> >> * \\param[in] confPath Sequence of the YAML section names (excluding\n>> >> * `configuration') leading to the requested option\n>> >> * \\return A value if an item corresponding to \\a confPath exists in the\n>> >> * configuration file, no value otherwise\n>> >> */\n>> >> -std::optional GlobalConfiguration::option(\n>> >> +\n>> >> +/**\n>> >> + * \\brief Return values of the configuration option identified by \\a confPath\n>> >> + * \\tparam T The type of the retrieved configuration value\n>> >> + * \\param[in] confPath Sequence of the YAML section names (excluding\n>> >> + * `configuration') leading to the requested list option, separated by dots\n>> >> + * \\return A vector of strings or no value if not found\n>> >> + */\n>> >> +std::optional> GlobalConfiguration::listOption(\n>> >> const std::initializer_list confPath) const\n>> >> {\n>> >> const YamlObject *c = &configuration();\n>> >> @@ -129,7 +140,7 @@ std::optional GlobalConfiguration::option(\n>> >> if (!*c)\n>> >> return {};\n>> >> }\n>> >> - return c->get();\n>> >> + return c->getList();\n>> >> }\n>> >> \n>> >> /**\n>> >> @@ -156,7 +167,37 @@ std::optional GlobalConfiguration::envOption(\n>> >> const char *envValue = utils::secure_getenv(envVariable);\n>> >> if (envValue)\n>> >> return std::optional{ std::string{ envValue } };\n>> >> - return option(confPath);\n>> >> + return option(confPath);\n>> >> +}\n>> >> +\n>> >> +/**\n>> >> + * \\brief Return values of the configuration option from a file or environment\n>> >> + * \\param[in] envVariable Environment variable to get the value from\n>> >> + * \\param[in] confPath The same as in GlobalConfiguration::option\n>> >> + *\n>> >> + * This helper looks first at the given environment variable and if it is\n>> >> + * defined (even if it is empty) then it splits its value by semicolons and\n>> >> + * returns the resulting list of strings. Otherwise it looks for \\a confPath the\n>> >> + * same way as in GlobalConfiguration::option, value of which must be a list of\n>> >> + * strings.\n>> >\n>> > Same comments from 3/12.\n>> \n>> The same responses :-).\n>> \n>> >> + *\n>> >> + * \\note Support for using environment variables to configure libcamera behavior\n>> >> + * is provided here mostly for backward compatibility reasons. Introducing new\n>> >> + * configuration environment variables is discouraged.\n>> >> + *\n>> >> + * \\return A vector of strings retrieved from the given environment option or\n>> >> + * configuration file or no value if not found; the vector may be empty\n>> >> + */\n>> >> +std::optional> GlobalConfiguration::envListOption(\n>> >> + const char *const envVariable,\n>> >> + const std::initializer_list confPath) const\n>> >> +{\n>> >> + const char *envValue = utils::secure_getenv(envVariable);\n>> >> + if (envValue) {\n>> >> + auto items = utils::split(envValue, \":\");\n>> >> + return std::vector(items.begin(), items.end());\n>> >> + }\n>> >> + return listOption(confPath);\n>> >> }\n>> >> \n>> >> /**\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 \n>> >> #include \n>> >> +#include \n>> >> #include \n>> >> +#include \n>> >> #include \n>> >> +#include \n>> >> \n>> >> #include \n>> >> #include \n>> >> #include \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({ \"ipa\", \"force_isolation\" })\n>> >> + .value_or(false));\n>> >\n>> > Although the environment variable doesn't let us do it, I wonder if there's\n>> > value in letting the configuration file configure which IPAs we want to\n>> > force-isolate and which we don't.\n>> \n>> I don't have any opinion on this.\n>\n> Ok. I guess we can add this in the future if somebody wants it. This framework\n> is well-designed that it should be easy to add.\n>\n>> \n>> > Also is there a reason why we don't use envOption() here? It is because we\n>> > can't get a bool out of env?\n>> \n>> Yes, IIRC I didn't want to introduce another helper for just a single use.\n>\n> Ok, that's fine then.\n>\n>\n> Thanks,\n>\n> Paul\n>\n>> \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())) {\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()), \":\")\n>> >> + << \"'\";\n>> >> +\n>> >\n>> > Ok, looks good.\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 9907b9615..b845e9086 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 \n>> >> #include \n>> >> #include \n>> >> #include \n>> >> +#include \n>> >> \n>> >> #include \n>> >> #include \n>> >> \n>> >> +#include \"libcamera/internal/global_configuration.h\"\n>> >> #include \"libcamera/internal/ipa_module.h\"\n>> >> \n>> >> /**\n>> >> @@ -48,9 +51,12 @@ LOG_DEFINE_CATEGORY(IPAProxy)\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())),\n>> >> + execPaths_(configuration.envListOption(\"LIBCAMERA_IPA_PROXY_PATH\", { \"ipa\", \"proxy_paths\" }).value_or(std::vector()))\n>> >\n>> > Oh that's cool that we can do that now.\n>> >\n>> > The rest looks good!\n>> >\n>> >\n>> > Thanks,\n>> >\n>> > Paul\n>> >\n>> >> {\n>> >> }\n>> >> \n>> >> @@ -122,20 +128,15 @@ std::string IPAProxy::configurationFile(const std::string &name,\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>> >> @@ -199,18 +200,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 9a3aadbd2..0f87e7976 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.50.1\n>> >>\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 B337EC324E\n\tfor ;\n\tTue, 9 Sep 2025 15:16:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 482706936D;\n\tTue, 9 Sep 2025 17:16:21 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEE4A6934B\n\tfor ;\n\tTue, 9 Sep 2025 17:16:18 +0200 (CEST)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-629-ppmLYh8ePzaN_ulIDcpQ6w-1; Tue, 09 Sep 2025 11:16:15 -0400","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-45ddbdb92dfso22400565e9.1\n\tfor ;\n\tTue, 09 Sep 2025 08:16:15 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-45c6faad9cfsm317363165e9.0.2025.09.09.08.16.12\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 09 Sep 2025 08:16:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"bIutmCf2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1757430977;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=wHRjJ/6rg12lttuIZN8sqw9JOswRdgumHS0kpzrXmt8=;\n\tb=bIutmCf2vi/CoF39r79OuFPJZVC1fjG1MAeBUNwAkYlC8MicSony1v99pAzWFbB6P6SWVA\n\t8UMpGHxM8I++8YDK8OnFm+jRHI6MzaCC8VQBDuoTnakTbPCRfEz/WgSpl6iyMoA1dGlFmP\n\tt7cavs0Zo/ol0AIldgshEKZUY/DsWME=","X-MC-Unique":"ppmLYh8ePzaN_ulIDcpQ6w-1","X-Mimecast-MFC-AGG-ID":"ppmLYh8ePzaN_ulIDcpQ6w_1757430975","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1757430974; x=1758035774;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=b4R5lQjry7m0dP6VFNXHngrzTW5/EHBDjV9WlwAcHuQ=;\n\tb=Zs5yg5foW5gNXH6fxHSi3o/7U4vu6P967ICQvTKSUorWrNw8BJJMzjsHSl+uNuI3JE\n\tenGyl8R1fQyH0bqJLT5a5VUlFLI3YmZ3eBRmYtVKn1eo8jlIHf0rXApmeM4UivgMLBMf\n\tZGpTaVbtdu6/JFNir20xOz/suq2wkMoM073fBy8T6/UQy5jBaFGcuR0btKQDmMDHh9Ev\n\twbR7D6TRlW0VI4O2SXwa8itnAto8Mfh4uozw9kHSkmeZPg91KFwdTydgngAmtOkferKV\n\tXGANWRxvpdEYCJklgJo4/Cpu2UcA1L2R1g98Blb3mh94ygj8hNGgog5pixCvoetUS/Kc\n\tcVQw==","X-Gm-Message-State":"AOJu0Yz6DJF2pmkS8AW+rM8p3+tgyMWBIgM12GL0kNP/Dka7hAFgguGL\n\tZVy0JIwrcARIOahdTL6bLghttq3O9yIcFtnTMmK5xotVPDujZmcQ+D5N9jbxN6aYwhi6iZghRTZ\n\toxIoB4DUb8c2d4nimd/aqHunUtQR83XKsNhnJidOh7TaCVzTvPExZy4CbG80LGTfWge78KP2I1w\n\tg=","X-Gm-Gg":"ASbGncvhHmbNDSanUKWccCgMqUbd9+GoYglFKcroJsdh3HvHFgEj0E3E4XAtOTzD3jx\n\t1T6lOPN1yVqQ1ygBlTaf+eNT8q5IFt2naO7NaD1YB7ExP8L/jkQqDU+pP1Q+PDW1/rIcyCQt7Fn\n\t1uPOxsBlHQgxYDf293ad9XZIJuVO2kgNhDwTXqnY2yW+UDLa2ZSxBvOS2R5Zjcm+bAwsLjBcwHW\n\tasyhJsbmyCXnCyag+YuMlF6wVtgSAxR5CNO9Z4NfSnTP5y1lFGbtIP3NxpIT5fo+9QbxOuBrPtH\n\tStjD1qWe/rehVKcsRScXteU1JnrkmowafaIpHmXMCX97PDX3hCrEotNIFdBaDIqj+Q==","X-Received":["by 2002:a05:600c:1f1a:b0:45d:ddc6:74a9 with SMTP id\n\t5b1f17b1804b1-45ddde92ef4mr110731625e9.12.1757430974015; \n\tTue, 09 Sep 2025 08:16:14 -0700 (PDT)","by 2002:a05:600c:1f1a:b0:45d:ddc6:74a9 with SMTP id\n\t5b1f17b1804b1-45ddde92ef4mr110730995e9.12.1757430973232; \n\tTue, 09 Sep 2025 08:16:13 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGmLzW3I1AzxUqPzHQDrYiEJkM85OyUyouIu2X5jyxGhfiwpUhuW+bH8goi6OD1BR+lMhmQeA==","From":"Milan Zamazal ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham , =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?= ,\n\tLaurent Pinchart ","Subject":"Re: [PATCH v16 05/12] config: Look up IPA configurables in\n\tconfiguration file","In-Reply-To":"<175742872226.2127323.14663302897797752341@neptunite.rasen.tech>\n\t(Paul Elder's message of \"Tue, 09 Sep 2025 23:38:42 +0900\")","References":"<20250729073201.5369-1-mzamazal@redhat.com>\n\t<20250729073201.5369-6-mzamazal@redhat.com>\n\t<175740459786.2127323.3227796486360043384@neptunite.rasen.tech>\n\t<85cy7zrd0v.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<175742872226.2127323.14663302897797752341@neptunite.rasen.tech>","Date":"Tue, 09 Sep 2025 17:16:11 +0200","Message-ID":"<85qzwfpsg4.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"G2LYYWjd3UR5WZkDL01HOkFm9YHK1q74E7zsvPvz7CE_1757430975","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}}]