[v11,05/12] config: Look up IPA configurables in configuration file

23634 diff mbox series

This patch adds configuration options for environment variables used in
the IPA proxy.  Two utility functions configuration retrieval functions
are added, to retrieve lists of values.

The configuration snippet:

  configuration:
    ipa:
      config_paths:
      - config path 1
      - config path 2
      - ...
      module_paths:
      - module path 1
      - module path 2
      - ...
      proxy_paths:
      - proxy path 1
      - proxy path 2
      - ...
      force_isolation: BOOL
      ipas:
        IPA_NAME:
          tuning_file: TUNING FILE

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../libcamera/internal/global_configuration.h | 35 ++++++++-
 include/libcamera/internal/ipa_manager.h      |  8 ++-
 include/libcamera/internal/ipa_proxy.h        |  5 +-
 src/libcamera/camera_manager.cpp              |  2 +-
 src/libcamera/global_configuration.cpp        | 47 +++++++++++-
 src/libcamera/ipa_manager.cpp                 | 37 ++++++----
 src/libcamera/ipa_proxy.cpp                   | 72 ++++++++++---------
 src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 +-
 src/libcamera/pipeline/mali-c55/mali-c55.cpp  |  2 +
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  4 +-
 .../pipeline/rpi/common/pipeline_base.cpp     |  3 +-
 src/libcamera/pipeline/vimc/vimc.cpp          |  3 +-
 src/libcamera/software_isp/software_isp.cpp   |  4 +-
 test/ipa/ipa_interface_test.cpp               |  3 +-
 .../module_ipa_proxy.cpp.tmpl                 |  4 +-
 .../module_ipa_proxy.h.tmpl                   |  2 +-
 16 files changed, 169 insertions(+), 65 deletions(-)

Hi

2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:
> This patch adds configuration options for environment variables used in
> the IPA proxy.  Two utility functions configuration retrieval functions
> are added, to retrieve lists of values.
> 
> The configuration snippet:
> 
>    configuration:
>      ipa:
>        config_paths:
>        - config path 1
>        - config path 2
>        - ...
>        module_paths:
>        - module path 1
>        - module path 2
>        - ...
>        proxy_paths:
>        - proxy path 1
>        - proxy path 2
>        - ...
>        force_isolation: BOOL
>        ipas:
>          IPA_NAME:
>            tuning_file: TUNING FILE
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   .../libcamera/internal/global_configuration.h | 35 ++++++++-
>   include/libcamera/internal/ipa_manager.h      |  8 ++-
>   include/libcamera/internal/ipa_proxy.h        |  5 +-
>   src/libcamera/camera_manager.cpp              |  2 +-
>   src/libcamera/global_configuration.cpp        | 47 +++++++++++-
>   src/libcamera/ipa_manager.cpp                 | 37 ++++++----
>   src/libcamera/ipa_proxy.cpp                   | 72 ++++++++++---------
>   src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 +-
>   src/libcamera/pipeline/mali-c55/mali-c55.cpp  |  2 +
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  4 +-
>   .../pipeline/rpi/common/pipeline_base.cpp     |  3 +-
>   src/libcamera/pipeline/vimc/vimc.cpp          |  3 +-
>   src/libcamera/software_isp/software_isp.cpp   |  4 +-
>   test/ipa/ipa_interface_test.cpp               |  3 +-
>   .../module_ipa_proxy.cpp.tmpl                 |  4 +-
>   .../module_ipa_proxy.h.tmpl                   |  2 +-
>   16 files changed, 169 insertions(+), 65 deletions(-)
> 
> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
> index 357119628..cc436703e 100644
> --- a/include/libcamera/internal/global_configuration.h
> +++ b/include/libcamera/internal/global_configuration.h
> @@ -11,6 +11,8 @@
>   #include <optional>
>   #include <string>
>   
> +#include <libcamera/base/utils.h>
> +
>   #include "libcamera/internal/yaml_parser.h"
>   
>   namespace libcamera {
> @@ -24,9 +26,40 @@ public:
>   
>   	unsigned int version() const;
>   	Configuration configuration() const;
> -	std::optional<std::string> option(const std::string &confPath) const;
> +
> +#ifndef __DOXYGEN__
> +	template<typename T,
> +		 std::enable_if_t<
> +			 std::is_same_v<bool, T> ||
> +			 std::is_same_v<double, T> ||
> +			 std::is_same_v<int8_t, T> ||
> +			 std::is_same_v<uint8_t, T> ||
> +			 std::is_same_v<int16_t, T> ||
> +			 std::is_same_v<uint16_t, T> ||
> +			 std::is_same_v<int32_t, T> ||
> +			 std::is_same_v<uint32_t, T> ||
> +			 std::is_same_v<std::string, T> ||
> +			 std::is_same_v<Size, T>> * = nullptr>

Is this needed? `YamlObject::get()` does not have any constraints,
so I think it can be omitted here as well?


> +#else
> +	template<typename T>
> +#endif
> +	std::optional<T> option(const std::string &confPath) const
> +	{
> +		const YamlObject *c = &configuration();
> +		for (auto part : utils::split(confPath, "/")) {
> +			c = &(*c)[part];
> +			if (!*c)
> +				return {};
> +		}
> +		return c->get<T>();
> +	}
> +
> +	std::vector<std::string> listOption(const std::string &confPath) const;
>   	std::optional<std::string> envOption(const char *const envVariable,
>   					     const std::string &confPath) const;
> +	std::vector<std::string> envListOption(
> +		const char *const envVariable,
> +		const std::string &confPath) const;
>   
>   private:
>   	bool loadFile(const std::filesystem::path &fileName);
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index a0d448cf9..8ad717919 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -17,6 +17,7 @@
>   #include <libcamera/ipa/ipa_module_info.h>
>   
>   #include "libcamera/internal/camera_manager.h"
> +#include "libcamera/internal/global_configuration.h"
>   #include "libcamera/internal/ipa_module.h"
>   #include "libcamera/internal/pipeline_handler.h"
>   #include "libcamera/internal/pub_key.h"
> @@ -28,7 +29,7 @@ LOG_DECLARE_CATEGORY(IPAManager)
>   class IPAManager
>   {
>   public:
> -	IPAManager();
> +	IPAManager(const GlobalConfiguration &configuration);
>   	~IPAManager();
>   
>   	template<typename T>
> @@ -42,7 +43,8 @@ public:
>   		if (!m)
>   			return nullptr;
>   
> -		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
> +		const GlobalConfiguration &configuration = cm->_d()->configuration();
> +		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m, configuration), configuration);
>   		if (!proxy->isValid()) {
>   			LOG(IPAManager, Error) << "Failed to load proxy";
>   			return nullptr;
> @@ -66,7 +68,7 @@ private:
>   	IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,
>   			  uint32_t maxVersion);
>   
> -	bool isSignatureValid(IPAModule *ipa) const;
> +	bool isSignatureValid(IPAModule *ipa, const GlobalConfiguration &configuration) const;
>   
>   	std::vector<std::unique_ptr<IPAModule>> modules_;
>   
> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> index 983bcc5fa..01cc5deff 100644
> --- a/include/libcamera/internal/ipa_proxy.h
> +++ b/include/libcamera/internal/ipa_proxy.h
> @@ -11,6 +11,8 @@
>   
>   #include <libcamera/ipa/ipa_interface.h>
>   
> +#include "libcamera/internal/global_configuration.h"
> +
>   namespace libcamera {
>   
>   class IPAModule;
> @@ -30,10 +32,11 @@ public:
>   	bool isValid() const { return valid_; }
>   
>   	std::string configurationFile(const std::string &name,
> +				      const GlobalConfiguration &configuration,
>   				      const std::string &fallbackName = std::string()) const;
>   
>   protected:
> -	std::string resolvePath(const std::string &file) const;
> +	std::string resolvePath(const std::string &file, const GlobalConfiguration &configuration) const;

I feel like `IPAProxy` could take the `GlobalConfiguration`
in the constructor?


>   
>   	bool valid_;
>   	ProxyState state_;
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index f3b4ec708..c47fd3677 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -41,7 +41,7 @@ LOG_DEFINE_CATEGORY(Camera)
>   CameraManager::Private::Private()
>   	: initialized_(false)
>   {
> -	ipaManager_ = std::make_unique<IPAManager>();
> +	ipaManager_ = std::make_unique<IPAManager>(this->configuration());
>   }
>   
>   int CameraManager::Private::start()
> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
> index 068ddd5a5..f7c69890c 100644
> --- a/src/libcamera/global_configuration.cpp
> +++ b/src/libcamera/global_configuration.cpp
> @@ -9,9 +9,11 @@
>   
>   #include <filesystem>
>   #include <memory>
> +#include <optional>
>   #include <string>
>   #include <string_view>
>   #include <sys/types.h>
> +#include <vector>
>   
>   #include <libcamera/base/file.h>
>   #include <libcamera/base/log.h>
> @@ -116,13 +118,22 @@ GlobalConfiguration::GlobalConfiguration()
>    */
>   
>   /**
> + * \fn std::optional<T> GlobalConfiguration::option(const std::string &confPath) const
>    * \brief Return value of the configuration option identified by \a confPath
>    * \param[in] confPath Sequence of the YAML section names (excluding
>    * `configuration') leading to the requested option separated by slashes
>    * \return A value if an item corresponding to \a confPath exists in the
>    * configuration file, no value otherwise
>    */
> -std::optional<std::string> GlobalConfiguration::option(const std::string &confPath) const
> +
> +/**
> + * \brief Return values of the configuration option identified by \a confPath
> + * \tparam T The type of the retrieved configuration value
> + * \param[in] confPath Sequence of the YAML section names (excluding
> + * `configuration') leading to the requested list option, separated by dots
> + * \return A vector of strings (empty one if the option is not found)
> + */
> +std::vector<std::string> GlobalConfiguration::listOption(const std::string &confPath) const
>   {
>   	const YamlObject *c = &configuration();
>   	for (auto part : utils::split(confPath, "/")) {
> @@ -130,7 +141,7 @@ std::optional<std::string> GlobalConfiguration::option(const std::string &confPa
>   		if (!*c)
>   			return {};
>   	}
> -	return c->get<std::string>();
> +	return c->getList<std::string>().value_or(std::vector<std::string>());
>   }
>   
>   /**
> @@ -157,7 +168,37 @@ std::optional<std::string> GlobalConfiguration::envOption(
>   	const char *envValue = utils::secure_getenv(envVariable);
>   	if (envValue)
>   		return std::optional{ std::string{ envValue } };
> -	return option(confPath);
> +	return option<std::string>(confPath);
> +}
> +
> +/**
> + * \brief Return values of the configuration option from a file or environment
> + * \param[in] envVariable Environment variable to get the value from
> + * \param[in] confPath The same as in GlobalConfiguration::option
> + *
> + * This helper looks first at the given environment variable and if it is
> + * defined (even if it is empty) then it splits its value by semicolons and
> + * returns the resulting list of strings. Otherwise it looks for \a confPath the
> + * same way as in GlobalConfiguration::option, value of which must be a list of
> + * strings.
> + *
> + * \note Support for using environment variables to configure libcamera behavior
> + * is provided here mostly for backward compatibility reasons. Introducing new
> + * configuration environment variables is discouraged.
> + *
> + * \return A vector of strings retrieved from the given environment option or
> + * configuration file (an empty vector is returned if nothing is found)

I think we want to distinguish "not present" and "empty". I could imagine
that someone might want to inhibit some behaviour by setting the env var
to empty. `envOption()` also differentiates the two. For example, with
the new "layers" proposal, I could image that we can set the layers in
a configuration file, etc, as well as an environmental variable. In that
case doing `LIBCAMERA_LAYERS= cam ...` might be useful for testing to
temporarily disable all layers.


> + */
> +std::vector<std::string> GlobalConfiguration::envListOption(
> +	const char *const envVariable,
> +	const std::string &confPath) const
> +{
> +	const char *envValue = utils::secure_getenv(envVariable);
> +	if (envValue) {
> +		auto items = utils::split(envValue, ":");
> +		return std::vector<std::string>(items.begin(), items.end());
> +	}
> +	return listOption(confPath);
>   }
>   
>   /**
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 830750dcc..67c1b23d2 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -9,6 +9,7 @@
>   
>   #include <algorithm>
>   #include <dirent.h>
> +#include <numeric>
>   #include <string.h>
>   #include <sys/types.h>
>   
> @@ -16,6 +17,7 @@
>   #include <libcamera/base/log.h>
>   #include <libcamera/base/utils.h>
>   
> +#include "libcamera/internal/global_configuration.h"
>   #include "libcamera/internal/ipa_module.h"
>   #include "libcamera/internal/ipa_proxy.h"
>   #include "libcamera/internal/pipeline_handler.h"
> @@ -101,7 +103,7 @@ LOG_DEFINE_CATEGORY(IPAManager)
>    * The IPAManager class is meant to only be instantiated once, by the
>    * CameraManager.
>    */
> -IPAManager::IPAManager()
> +IPAManager::IPAManager(const GlobalConfiguration &configuration)
>   {
>   #if HAVE_IPA_PUBKEY
>   	if (!pubKey_.isValid())
> @@ -111,18 +113,25 @@ IPAManager::IPAManager()
>   	unsigned int ipaCount = 0;
>   
>   	/* User-specified paths take precedence. */
> -	const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
> -	if (modulePaths) {
> -		for (const auto &dir : utils::split(modulePaths, ":")) {
> -			if (dir.empty())
> -				continue;
> +	const auto modulePaths =
> +		configuration.envListOption(
> +			"LIBCAMERA_IPA_MODULE_PATH", "ipa/module_paths");
> +	for (const auto &dir : modulePaths) {
> +		if (dir.empty())
> +			continue;
>   
> -			ipaCount += addDir(dir.c_str());
> -		}
> +		ipaCount += addDir(dir.c_str());
> +	}
>   
> -		if (!ipaCount)
> -			LOG(IPAManager, Warning)
> -				<< "No IPA found in '" << modulePaths << "'";
> +	if (!ipaCount) {
> +		std::string paths;
> +		if (!modulePaths.empty()) {
> +			paths = std::accumulate(std::next(modulePaths.begin()),
> +						modulePaths.end(),
> +						modulePaths[0],
> +						[](std::string s1, std::string s2) { return s1 + ":" + s2; });
> +		}
> +		LOG(IPAManager, Warning) << "No IPA found in '" << paths << "'";

   #include <libcamera/base/utils.h>
   ...
   LOG(IPAManager, Warning) << "No IPA found in '" << utils::join(modulePaths, ":") << "'";

?


>   	}
>   
>   	/*
> @@ -276,11 +285,13 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>    */
>   #endif
>   
> -bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
> +bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa, const GlobalConfiguration &configuration) const

Maybe `configuration.option<bool>("ipa/force_isolation").value_or(false)` could be
saved into a member variable in the constructor? Then this won't need
the configuration parameter.


>   {
>   #if HAVE_IPA_PUBKEY
>   	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
> -	if (force && force[0] != '\0') {
> +	if ((force && force[0] != '\0') ||
> +	    (!force && configuration.option<bool>("ipa/force_isolation")
> +			       .value_or(false))) {
>   		LOG(IPAManager, Debug)
>   			<< "Isolation of IPA module " << ipa->path()
>   			<< " forced through environment variable";
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> index 9907b9615..77927b0d4 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -14,6 +14,7 @@
>   #include <libcamera/base/log.h>
>   #include <libcamera/base/utils.h>
>   
> +#include "libcamera/internal/global_configuration.h"
>   #include "libcamera/internal/ipa_module.h"
>   
>   /**
> @@ -71,6 +72,7 @@ IPAProxy::~IPAProxy()
>   /**
>    * \brief Retrieve the absolute path to an IPA configuration file
>    * \param[in] name The configuration file name
> + * \param[in] configuration The global configuration
>    * \param[in] fallbackName The name of a fallback configuration file
>    *
>    * This function locates the configuration file for an IPA and returns its
> @@ -96,6 +98,7 @@ IPAProxy::~IPAProxy()
>    * no configuration file can be found
>    */
>   std::string IPAProxy::configurationFile(const std::string &name,
> +					const GlobalConfiguration &configuration,
>   					const std::string &fallbackName) const
>   {
>   	/*
> @@ -105,37 +108,37 @@ std::string IPAProxy::configurationFile(const std::string &name,
>   	const std::string ipaName = ipam_->info().name;
>   
>   	/*
> -	 * Start with any user override through the module-specific environment
> -	 * variable. Use the name of the IPA module up to the first '/' to
> -	 * construct the variable name.
> +	 * Start with any user override through the module-specific configuration or
> +	 * environment variable. Use the name of the IPA module up to the first '/'
> +	 * to construct the configuration and variable names.
>   	 */
> -	std::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));
> +	std::string ipaBaseName = ipaName.substr(0, ipaName.find('/'));
> +	std::string ipaConfigName = "ipa/ipas/" + ipaBaseName + "/tuning_file";

I feel like we're unnecessarily limiting outself wrt. rpi here. Since the IPA
names are "rpi/vc4" and "rpi/pisp", thus `ipaBaseName == "rpi"`. Which means
that it is impossible to specify different tuning files for the different
platforms. I think this is fine for an env var, but for a configuration file
I think this flexibility is expected.

But I don't quite see the point of overriding the tuning file like this from a
configuration file. So maybe leaving just the env var override is sufficient?



> +	std::string ipaEnvName = ipaBaseName;
>   	std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),
>   		       [](unsigned char c) { return std::toupper(c); });
>   	ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE";
>   
> -	char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());
> -	if (configFromEnv && *configFromEnv != '\0')
> -		return { configFromEnv };
> +	auto config = configuration.envOption(ipaEnvName.c_str(), ipaConfigName);
> +	if (config)
> +		return { config.value() };
>   
>   	struct stat statbuf;
>   	int ret;
>   
>   	/*
> -	 * Check the directory pointed to by the IPA config path environment
> -	 * variable next.
> +	 * Check the directory pointed to by the IPA config path next.
>   	 */
> -	const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
> -	if (confPaths) {
> -		for (const auto &dir : utils::split(confPaths, ":")) {
> -			if (dir.empty())
> -				continue;
> -
> -			std::string confPath = dir + "/" + ipaName + "/" + name;
> -			ret = stat(confPath.c_str(), &statbuf);
> -			if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
> -				return confPath;
> -		}
> +	auto confPaths =
> +		configuration.envListOption(
> +			"LIBCAMERA_IPA_CONFIG_PATH", "ipa/config_paths");
> +	for (const auto &dir : confPaths) {
> +		if (dir.empty())
> +			continue;
> +		std::string confPath = dir + "/" + ipaName + "/" + name;
> +		ret = stat(confPath.c_str(), &statbuf);
> +		if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
> +			return confPath;
>   	}
>   
>   	std::string root = utils::libcameraSourcePath();
> @@ -178,12 +181,13 @@ std::string IPAProxy::configurationFile(const std::string &name,
>   		<< "Configuration file '" << name
>   		<< "' not found for IPA module '" << ipaName
>   		<< "', falling back to '" << fallbackName << "'";
> -	return configurationFile(fallbackName);
> +	return configurationFile(fallbackName, configuration);
>   }
>   
>   /**
>    * \brief Find a valid full path for a proxy worker for a given executable name
>    * \param[in] file File name of proxy worker executable
> + * \param[in] configuration The global configuration
>    *
>    * A proxy worker's executable could be found in either the global installation
>    * directory, or in the paths specified by the environment variable
> @@ -195,22 +199,22 @@ std::string IPAProxy::configurationFile(const std::string &name,
>    * \return The full path to the proxy worker executable, or an empty string if
>    * no valid executable path
>    */
> -std::string IPAProxy::resolvePath(const std::string &file) const
> +std::string IPAProxy::resolvePath(const std::string &file, const GlobalConfiguration &configuration) const
>   {
>   	std::string proxyFile = "/" + file;
>   
> -	/* Check env variable first. */
> -	const char *execPaths = utils::secure_getenv("LIBCAMERA_IPA_PROXY_PATH");
> -	if (execPaths) {
> -		for (const auto &dir : utils::split(execPaths, ":")) {
> -			if (dir.empty())
> -				continue;
> -
> -			std::string proxyPath = dir;
> -			proxyPath += proxyFile;
> -			if (!access(proxyPath.c_str(), X_OK))
> -				return proxyPath;
> -		}
> +	/* Check the configuration first. */
> +	const auto execPaths =
> +		configuration.envListOption(
> +			"LIBCAMERA_IPA_PROXY_PATH", "ipa/proxy_paths");
> +	for (const auto &dir : execPaths) {
> +		if (dir.empty())
> +			continue;
> +
> +		std::string proxyPath = dir;

std::string proxyPath = dir + proxyFile;


Regards,
Barnabás Pőcze


> +		proxyPath += proxyFile;
> +		if (!access(proxyPath.c_str(), X_OK))
> +			return proxyPath;
>   	}
>   
>   	/*
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index e31e3879d..723f7665b 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1183,8 +1183,9 @@ int IPU3CameraData::loadIPA()
>   	 * The API tuning file is made from the sensor name. If the tuning file
>   	 * isn't found, fall back to the 'uncalibrated' file.
>   	 */
> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>   	std::string ipaTuningFile =
> -		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
> +		ipa_->configurationFile(sensor->model() + ".yaml", configuration, "uncalibrated.yaml");
>   
>   	ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
>   			 sensorInfo, sensor->controls(), &ipaControls_);
> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> index 4acc091bd..d346fa25f 100644
> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> @@ -388,7 +388,9 @@ int MaliC55CameraData::loadIPA()
>   
>   	ipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls);
>   
> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>   	std::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + ".yaml",
> +							    configuration,
>   							    "uncalibrated.yaml");
>   
>   	/* We need to inform the IPA of the sensor configuration */
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 675f0a749..aded4cb43 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -40,6 +40,7 @@
>   #include "libcamera/internal/delayed_controls.h"
>   #include "libcamera/internal/device_enumerator.h"
>   #include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/global_configuration.h"
>   #include "libcamera/internal/ipa_manager.h"
>   #include "libcamera/internal/media_device.h"
>   #include "libcamera/internal/media_pipeline.h"
> @@ -383,8 +384,9 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>   	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
>   
>   	/* The IPA tuning file is made from the sensor name. */
> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>   	std::string ipaTuningFile =
> -		ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
> +		ipa_->configurationFile(sensor_->model() + ".yaml", configuration, "uncalibrated.yaml");
>   
>   	IPACameraSensorInfo sensorInfo{};
>   	int ret = sensor_->sensorInfo(&sensorInfo);
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index a316ef297..3da919ee7 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -1157,7 +1157,8 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
>   	std::string model = sensor_->model();
>   	if (isMonoSensor(sensor_))
>   		model += "_mono";
> -	std::string configurationFile = ipa_->configurationFile(model + ".json");
> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
> +	std::string configurationFile = ipa_->configurationFile(model + ".json", configuration);
>   
>   	IPASettings settings(configurationFile, sensor_->model());
>   	ipa::RPi::InitParams params;
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 07273bd2b..555d51bb6 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -494,7 +494,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>   
>   	data->ipa_->paramsComputed.connect(data.get(), &VimcCameraData::paramsComputed);
>   
> -	std::string conf = data->ipa_->configurationFile("vimc.conf");
> +	const GlobalConfiguration &configuration = manager_->_d()->configuration();
> +	std::string conf = data->ipa_->configurationFile("vimc.conf", configuration);
>   	Flags<ipa::vimc::TestFlag> inFlags = ipa::vimc::TestFlag::Flag2;
>   	Flags<ipa::vimc::TestFlag> outFlags;
>   	data->ipa_->init(IPASettings{ conf, data->sensor_->model() },
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 28e2a360e..3ce354111 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -21,6 +21,7 @@
>   #include <libcamera/stream.h>
>   
>   #include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/global_configuration.h"
>   #include "libcamera/internal/ipa_manager.h"
>   #include "libcamera/internal/software_isp/debayer_params.h"
>   
> @@ -130,8 +131,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
>   	 * The API tuning file is made from the sensor name. If the tuning file
>   	 * isn't found, fall back to the 'uncalibrated' file.
>   	 */
> +	const GlobalConfiguration &configuration = pipe->cameraManager()->_d()->configuration();
>   	std::string ipaTuningFile =
> -		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
> +		ipa_->configurationFile(sensor->model() + ".yaml", configuration, "uncalibrated.yaml");
>   
>   	IPACameraSensorInfo sensorInfo{};
>   	int ret = sensor->sensorInfo(&sensorInfo);
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index b81783664..1afcbf106 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -106,7 +106,8 @@ protected:
>   		}
>   
>   		/* Test initialization of IPA module. */
> -		std::string conf = ipa_->configurationFile("vimc.conf");
> +		const GlobalConfiguration configuration;
> +		std::string conf = ipa_->configurationFile("vimc.conf", configuration);
>   		Flags<ipa::vimc::TestFlag> inFlags;
>   		Flags<ipa::vimc::TestFlag> outFlags;
>   		int ret = ipa_->init(IPASettings{ conf, "vimc" },
> 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
> index 9a3aadbd2..71e920eb1 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -45,7 +45,7 @@ namespace {{ns}} {
>   {% endfor %}
>   {%- endif %}
>   
> -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
> +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration)
>   	: IPAProxy(ipam), isolate_(isolate),
>   	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
>   {
> @@ -54,7 +54,7 @@ namespace {{ns}} {
>   		<< ipam->path();
>   
>   	if (isolate_) {
> -		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
> +		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy", configuration);
>   		if (proxyWorkerPath.empty()) {
>   			LOG(IPAProxy, Error)
>   				<< "Failed to get proxy worker path";
> 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
> index a0312a7c1..057c3ab03 100644
> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> @@ -37,7 +37,7 @@ namespace {{ns}} {
>   class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
>   {
>   public:
> -	{{proxy_name}}(IPAModule *ipam, bool isolate);
> +	{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration);
>   	~{{proxy_name}}();
>   
>   {% for method in interface_main.methods %}

Hi Barnabás,

thank you for review.

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Hi
>
> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:
>> This patch adds configuration options for environment variables used in
>> the IPA proxy.  Two utility functions configuration retrieval functions
>> are added, to retrieve lists of values.
>> The configuration snippet:
>>    configuration:
>>      ipa:
>>        config_paths:
>>        - config path 1
>>        - config path 2
>>        - ...
>>        module_paths:
>>        - module path 1
>>        - module path 2
>>        - ...
>>        proxy_paths:
>>        - proxy path 1
>>        - proxy path 2
>>        - ...
>>        force_isolation: BOOL
>>        ipas:
>>          IPA_NAME:
>>            tuning_file: TUNING FILE
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   .../libcamera/internal/global_configuration.h | 35 ++++++++-
>>   include/libcamera/internal/ipa_manager.h      |  8 ++-
>>   include/libcamera/internal/ipa_proxy.h        |  5 +-
>>   src/libcamera/camera_manager.cpp              |  2 +-
>>   src/libcamera/global_configuration.cpp        | 47 +++++++++++-
>>   src/libcamera/ipa_manager.cpp                 | 37 ++++++----
>>   src/libcamera/ipa_proxy.cpp                   | 72 ++++++++++---------
>>   src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 +-
>>   src/libcamera/pipeline/mali-c55/mali-c55.cpp  |  2 +
>>   src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  4 +-
>>   .../pipeline/rpi/common/pipeline_base.cpp     |  3 +-
>>   src/libcamera/pipeline/vimc/vimc.cpp          |  3 +-
>>   src/libcamera/software_isp/software_isp.cpp   |  4 +-
>>   test/ipa/ipa_interface_test.cpp               |  3 +-
>>   .../module_ipa_proxy.cpp.tmpl                 |  4 +-
>>   .../module_ipa_proxy.h.tmpl                   |  2 +-
>>   16 files changed, 169 insertions(+), 65 deletions(-)
>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>> index 357119628..cc436703e 100644
>> --- a/include/libcamera/internal/global_configuration.h
>> +++ b/include/libcamera/internal/global_configuration.h
>> @@ -11,6 +11,8 @@
>>   #include <optional>
>>   #include <string>
>>   +#include <libcamera/base/utils.h>
>> +
>>   #include "libcamera/internal/yaml_parser.h"
>>     namespace libcamera {
>> @@ -24,9 +26,40 @@ public:
>>     	unsigned int version() const;
>>   	Configuration configuration() const;
>> -	std::optional<std::string> option(const std::string &confPath) const;
>> +
>> +#ifndef __DOXYGEN__
>> +	template<typename T,
>> +		 std::enable_if_t<
>> +			 std::is_same_v<bool, T> ||
>> +			 std::is_same_v<double, T> ||
>> +			 std::is_same_v<int8_t, T> ||
>> +			 std::is_same_v<uint8_t, T> ||
>> +			 std::is_same_v<int16_t, T> ||
>> +			 std::is_same_v<uint16_t, T> ||
>> +			 std::is_same_v<int32_t, T> ||
>> +			 std::is_same_v<uint32_t, T> ||
>> +			 std::is_same_v<std::string, T> ||
>> +			 std::is_same_v<Size, T>> * = nullptr>
>
> Is this needed? `YamlObject::get()` does not have any constraints,
> so I think it can be omitted here as well?

Yes; YamlObject::get() used to have the constraints but doesn't have
them any more.

>> +#else
>> +	template<typename T>
>> +#endif
>> +	std::optional<T> option(const std::string &confPath) const
>> +	{
>> +		const YamlObject *c = &configuration();
>> +		for (auto part : utils::split(confPath, "/")) {
>> +			c = &(*c)[part];
>> +			if (!*c)
>> +				return {};
>> +		}
>> +		return c->get<T>();
>> +	}
>> +
>> +	std::vector<std::string> listOption(const std::string &confPath) const;
>>   	std::optional<std::string> envOption(const char *const envVariable,
>>   					     const std::string &confPath) const;
>> +	std::vector<std::string> envListOption(
>> +		const char *const envVariable,
>> +		const std::string &confPath) const;
>>     private:
>>   	bool loadFile(const std::filesystem::path &fileName);
>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
>> index a0d448cf9..8ad717919 100644
>> --- a/include/libcamera/internal/ipa_manager.h
>> +++ b/include/libcamera/internal/ipa_manager.h
>> @@ -17,6 +17,7 @@
>>   #include <libcamera/ipa/ipa_module_info.h>
>>     #include "libcamera/internal/camera_manager.h"
>> +#include "libcamera/internal/global_configuration.h"
>>   #include "libcamera/internal/ipa_module.h"
>>   #include "libcamera/internal/pipeline_handler.h"
>>   #include "libcamera/internal/pub_key.h"
>> @@ -28,7 +29,7 @@ LOG_DECLARE_CATEGORY(IPAManager)
>>   class IPAManager
>>   {
>>   public:
>> -	IPAManager();
>> +	IPAManager(const GlobalConfiguration &configuration);
>>   	~IPAManager();
>>     	template<typename T>
>> @@ -42,7 +43,8 @@ public:
>>   		if (!m)
>>   			return nullptr;
>>   -		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
>> +		const GlobalConfiguration &configuration = cm->_d()->configuration();
>> +		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m, configuration), configuration);
>>   		if (!proxy->isValid()) {
>>   			LOG(IPAManager, Error) << "Failed to load proxy";
>>   			return nullptr;
>> @@ -66,7 +68,7 @@ private:
>>   	IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,
>>   			  uint32_t maxVersion);
>>   -	bool isSignatureValid(IPAModule *ipa) const;
>> +	bool isSignatureValid(IPAModule *ipa, const GlobalConfiguration &configuration) const;
>>     	std::vector<std::unique_ptr<IPAModule>> modules_;
>>   diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
>> index 983bcc5fa..01cc5deff 100644
>> --- a/include/libcamera/internal/ipa_proxy.h
>> +++ b/include/libcamera/internal/ipa_proxy.h
>> @@ -11,6 +11,8 @@
>>     #include <libcamera/ipa/ipa_interface.h>
>>   +#include "libcamera/internal/global_configuration.h"
>> +
>>   namespace libcamera {
>>     class IPAModule;
>> @@ -30,10 +32,11 @@ public:
>>   	bool isValid() const { return valid_; }
>>     	std::string configurationFile(const std::string &name,
>> +				      const GlobalConfiguration &configuration,
>>   				      const std::string &fallbackName = std::string()) const;
>>     protected:
>> -	std::string resolvePath(const std::string &file) const;
>> +	std::string resolvePath(const std::string &file, const GlobalConfiguration &configuration) const;
>
> I feel like `IPAProxy` could take the `GlobalConfiguration`
> in the constructor?

I.e. copy the configuration and changing std::unique_ptr<YamlObject>
inside to shared_ptr?  Possible, I think, and I don't have a particular
preference -- up to your judgement what's better.

>>     	bool valid_;
>>   	ProxyState state_;
>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>> index f3b4ec708..c47fd3677 100644
>> --- a/src/libcamera/camera_manager.cpp
>> +++ b/src/libcamera/camera_manager.cpp
>> @@ -41,7 +41,7 @@ LOG_DEFINE_CATEGORY(Camera)
>>   CameraManager::Private::Private()
>>   	: initialized_(false)
>>   {
>> -	ipaManager_ = std::make_unique<IPAManager>();
>> +	ipaManager_ = std::make_unique<IPAManager>(this->configuration());
>>   }
>>     int CameraManager::Private::start()
>> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
>> index 068ddd5a5..f7c69890c 100644
>> --- a/src/libcamera/global_configuration.cpp
>> +++ b/src/libcamera/global_configuration.cpp
>> @@ -9,9 +9,11 @@
>>     #include <filesystem>
>>   #include <memory>
>> +#include <optional>
>>   #include <string>
>>   #include <string_view>
>>   #include <sys/types.h>
>> +#include <vector>
>>     #include <libcamera/base/file.h>
>>   #include <libcamera/base/log.h>
>> @@ -116,13 +118,22 @@ GlobalConfiguration::GlobalConfiguration()
>>    */
>>     /**
>> + * \fn std::optional<T> GlobalConfiguration::option(const std::string &confPath) const
>>    * \brief Return value of the configuration option identified by \a confPath
>>    * \param[in] confPath Sequence of the YAML section names (excluding
>>    * `configuration') leading to the requested option separated by slashes
>>    * \return A value if an item corresponding to \a confPath exists in the
>>    * configuration file, no value otherwise
>>    */
>> -std::optional<std::string> GlobalConfiguration::option(const std::string &confPath) const
>> +
>> +/**
>> + * \brief Return values of the configuration option identified by \a confPath
>> + * \tparam T The type of the retrieved configuration value
>> + * \param[in] confPath Sequence of the YAML section names (excluding
>> + * `configuration') leading to the requested list option, separated by dots
>> + * \return A vector of strings (empty one if the option is not found)
>> + */
>> +std::vector<std::string> GlobalConfiguration::listOption(const std::string &confPath) const
>>   {
>>   	const YamlObject *c = &configuration();
>>   	for (auto part : utils::split(confPath, "/")) {
>> @@ -130,7 +141,7 @@ std::optional<std::string> GlobalConfiguration::option(const std::string &confPa
>>   		if (!*c)
>>   			return {};
>>   	}
>> -	return c->get<std::string>();
>> +	return c->getList<std::string>().value_or(std::vector<std::string>());
>>   }
>>     /**
>> @@ -157,7 +168,37 @@ std::optional<std::string> GlobalConfiguration::envOption(
>>   	const char *envValue = utils::secure_getenv(envVariable);
>>   	if (envValue)
>>   		return std::optional{ std::string{ envValue } };
>> -	return option(confPath);
>> +	return option<std::string>(confPath);
>> +}
>> +
>> +/**
>> + * \brief Return values of the configuration option from a file or environment
>> + * \param[in] envVariable Environment variable to get the value from
>> + * \param[in] confPath The same as in GlobalConfiguration::option
>> + *
>> + * This helper looks first at the given environment variable and if it is
>> + * defined (even if it is empty) then it splits its value by semicolons and
>> + * returns the resulting list of strings. Otherwise it looks for \a confPath the
>> + * same way as in GlobalConfiguration::option, value of which must be a list of
>> + * strings.
>> + *
>> + * \note Support for using environment variables to configure libcamera behavior
>> + * is provided here mostly for backward compatibility reasons. Introducing new
>> + * configuration environment variables is discouraged.
>> + *
>> + * \return A vector of strings retrieved from the given environment option or
>> + * configuration file (an empty vector is returned if nothing is found)
>
> I think we want to distinguish "not present" and "empty". I could imagine
> that someone might want to inhibit some behaviour by setting the env var
> to empty.

Well, introducing new environment variables for configuration is
discouraged.

> `envOption()` also differentiates the two. For example, with the new
> "layers" proposal, I could image that we can set the layers in a
> configuration file, etc, as well as an environmental variable. In that
> case doing `LIBCAMERA_LAYERS= cam ...` might be useful for testing to
> temporarily disable all layers.

Hmm, sounds reasonable but I'm still not sure we want to encourage this.
But technically, I can change the method signature.

>> + */
>> +std::vector<std::string> GlobalConfiguration::envListOption(
>> +	const char *const envVariable,
>> +	const std::string &confPath) const
>> +{
>> +	const char *envValue = utils::secure_getenv(envVariable);
>> +	if (envValue) {
>> +		auto items = utils::split(envValue, ":");
>> +		return std::vector<std::string>(items.begin(), items.end());
>> +	}
>> +	return listOption(confPath);
>>   }
>>     /**
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index 830750dcc..67c1b23d2 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -9,6 +9,7 @@
>>     #include <algorithm>
>>   #include <dirent.h>
>> +#include <numeric>
>>   #include <string.h>
>>   #include <sys/types.h>
>>   @@ -16,6 +17,7 @@
>>   #include <libcamera/base/log.h>
>>   #include <libcamera/base/utils.h>
>>   +#include "libcamera/internal/global_configuration.h"
>>   #include "libcamera/internal/ipa_module.h"
>>   #include "libcamera/internal/ipa_proxy.h"
>>   #include "libcamera/internal/pipeline_handler.h"
>> @@ -101,7 +103,7 @@ LOG_DEFINE_CATEGORY(IPAManager)
>>    * The IPAManager class is meant to only be instantiated once, by the
>>    * CameraManager.
>>    */
>> -IPAManager::IPAManager()
>> +IPAManager::IPAManager(const GlobalConfiguration &configuration)
>>   {
>>   #if HAVE_IPA_PUBKEY
>>   	if (!pubKey_.isValid())
>> @@ -111,18 +113,25 @@ IPAManager::IPAManager()
>>   	unsigned int ipaCount = 0;
>>     	/* User-specified paths take precedence. */
>> -	const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
>> -	if (modulePaths) {
>> -		for (const auto &dir : utils::split(modulePaths, ":")) {
>> -			if (dir.empty())
>> -				continue;
>> +	const auto modulePaths =
>> +		configuration.envListOption(
>> +			"LIBCAMERA_IPA_MODULE_PATH", "ipa/module_paths");
>> +	for (const auto &dir : modulePaths) {
>> +		if (dir.empty())
>> +			continue;
>>   -			ipaCount += addDir(dir.c_str());
>> -		}
>> +		ipaCount += addDir(dir.c_str());
>> +	}
>>   -		if (!ipaCount)
>> -			LOG(IPAManager, Warning)
>> -				<< "No IPA found in '" << modulePaths << "'";
>> +	if (!ipaCount) {
>> +		std::string paths;
>> +		if (!modulePaths.empty()) {
>> +			paths = std::accumulate(std::next(modulePaths.begin()),
>> +						modulePaths.end(),
>> +						modulePaths[0],
>> +						[](std::string s1, std::string s2) { return s1 + ":" + s2; });
>> +		}
>> +		LOG(IPAManager, Warning) << "No IPA found in '" << paths << "'";
>
>   #include <libcamera/base/utils.h>
>   ...
>   LOG(IPAManager, Warning) << "No IPA found in '" << utils::join(modulePaths, ":") << "'";
>
> ?

OK.

>>   	}
>>     	/*
>> @@ -276,11 +285,13 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>>    */
>>   #endif
>>   -bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
>> +bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa, const GlobalConfiguration &configuration) const
>
> Maybe `configuration.option<bool>("ipa/force_isolation").value_or(false)` could be
> saved into a member variable in the constructor? Then this won't need
> the configuration parameter.

Yes.  Assuming the `pipe' CameraManager instance is always the same that
was used to create the IPAManager instance.

>>   {
>>   #if HAVE_IPA_PUBKEY
>>   	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
>> -	if (force && force[0] != '\0') {
>> +	if ((force && force[0] != '\0') ||
>> +	    (!force && configuration.option<bool>("ipa/force_isolation")
>> +			       .value_or(false))) {
>>   		LOG(IPAManager, Debug)
>>   			<< "Isolation of IPA module " << ipa->path()
>>   			<< " forced through environment variable";
>> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
>> index 9907b9615..77927b0d4 100644
>> --- a/src/libcamera/ipa_proxy.cpp
>> +++ b/src/libcamera/ipa_proxy.cpp
>> @@ -14,6 +14,7 @@
>>   #include <libcamera/base/log.h>
>>   #include <libcamera/base/utils.h>
>>   +#include "libcamera/internal/global_configuration.h"
>>   #include "libcamera/internal/ipa_module.h"
>>     /**
>> @@ -71,6 +72,7 @@ IPAProxy::~IPAProxy()
>>   /**
>>    * \brief Retrieve the absolute path to an IPA configuration file
>>    * \param[in] name The configuration file name
>> + * \param[in] configuration The global configuration
>>    * \param[in] fallbackName The name of a fallback configuration file
>>    *
>>    * This function locates the configuration file for an IPA and returns its
>> @@ -96,6 +98,7 @@ IPAProxy::~IPAProxy()
>>    * no configuration file can be found
>>    */
>>   std::string IPAProxy::configurationFile(const std::string &name,
>> +					const GlobalConfiguration &configuration,
>>   					const std::string &fallbackName) const
>>   {
>>   	/*
>> @@ -105,37 +108,37 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>   	const std::string ipaName = ipam_->info().name;
>>     	/*
>> -	 * Start with any user override through the module-specific environment
>> -	 * variable. Use the name of the IPA module up to the first '/' to
>> -	 * construct the variable name.
>> +	 * Start with any user override through the module-specific configuration or
>> +	 * environment variable. Use the name of the IPA module up to the first '/'
>> +	 * to construct the configuration and variable names.
>>   	 */
>> -	std::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));
>> +	std::string ipaBaseName = ipaName.substr(0, ipaName.find('/'));
>> +	std::string ipaConfigName = "ipa/ipas/" + ipaBaseName + "/tuning_file";
>
> I feel like we're unnecessarily limiting outself wrt. rpi here. Since the IPA
> names are "rpi/vc4" and "rpi/pisp", thus `ipaBaseName == "rpi"`. Which means
> that it is impossible to specify different tuning files for the different
> platforms. I think this is fine for an env var, but for a configuration file
> I think this flexibility is expected.

Right; so rather than trimming, let's replace the slash with another
character here?

> But I don't quite see the point of overriding the tuning file like this from a
> configuration file. So maybe leaving just the env var override is sufficient?

I think the initial idea was to be able to configure everything in the
configuration file (why not?).  Logging is currently a noticeable
exception, although hopefully only "temporary".

>> +	std::string ipaEnvName = ipaBaseName;
>>   	std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),
>>   		       [](unsigned char c) { return std::toupper(c); });
>>   	ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE";
>>   -	char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());
>> -	if (configFromEnv && *configFromEnv != '\0')
>> -		return { configFromEnv };
>> +	auto config = configuration.envOption(ipaEnvName.c_str(), ipaConfigName);
>> +	if (config)
>> +		return { config.value() };
>>     	struct stat statbuf;
>>   	int ret;
>>     	/*
>> -	 * Check the directory pointed to by the IPA config path environment
>> -	 * variable next.
>> +	 * Check the directory pointed to by the IPA config path next.
>>   	 */
>> -	const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
>> -	if (confPaths) {
>> -		for (const auto &dir : utils::split(confPaths, ":")) {
>> -			if (dir.empty())
>> -				continue;
>> -
>> -			std::string confPath = dir + "/" + ipaName + "/" + name;
>> -			ret = stat(confPath.c_str(), &statbuf);
>> -			if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
>> -				return confPath;
>> -		}
>> +	auto confPaths =
>> +		configuration.envListOption(
>> +			"LIBCAMERA_IPA_CONFIG_PATH", "ipa/config_paths");
>> +	for (const auto &dir : confPaths) {
>> +		if (dir.empty())
>> +			continue;
>> +		std::string confPath = dir + "/" + ipaName + "/" + name;
>> +		ret = stat(confPath.c_str(), &statbuf);
>> +		if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
>> +			return confPath;
>>   	}
>>     	std::string root = utils::libcameraSourcePath();
>> @@ -178,12 +181,13 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>   		<< "Configuration file '" << name
>>   		<< "' not found for IPA module '" << ipaName
>>   		<< "', falling back to '" << fallbackName << "'";
>> -	return configurationFile(fallbackName);
>> +	return configurationFile(fallbackName, configuration);
>>   }
>>     /**
>>    * \brief Find a valid full path for a proxy worker for a given executable name
>>    * \param[in] file File name of proxy worker executable
>> + * \param[in] configuration The global configuration
>>    *
>>    * A proxy worker's executable could be found in either the global installation
>>    * directory, or in the paths specified by the environment variable
>> @@ -195,22 +199,22 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>    * \return The full path to the proxy worker executable, or an empty string if
>>    * no valid executable path
>>    */
>> -std::string IPAProxy::resolvePath(const std::string &file) const
>> +std::string IPAProxy::resolvePath(const std::string &file, const GlobalConfiguration &configuration) const
>>   {
>>   	std::string proxyFile = "/" + file;
>>   -	/* Check env variable first. */
>> -	const char *execPaths = utils::secure_getenv("LIBCAMERA_IPA_PROXY_PATH");
>> -	if (execPaths) {
>> -		for (const auto &dir : utils::split(execPaths, ":")) {
>> -			if (dir.empty())
>> -				continue;
>> -
>> -			std::string proxyPath = dir;
>> -			proxyPath += proxyFile;
>> -			if (!access(proxyPath.c_str(), X_OK))
>> -				return proxyPath;
>> -		}
>> +	/* Check the configuration first. */
>> +	const auto execPaths =
>> +		configuration.envListOption(
>> +			"LIBCAMERA_IPA_PROXY_PATH", "ipa/proxy_paths");
>> +	for (const auto &dir : execPaths) {
>> +		if (dir.empty())
>> +			continue;
>> +
>> +		std::string proxyPath = dir;
>
> std::string proxyPath = dir + proxyFile;

OK.

> Regards,
> Barnabás Pőcze
>
>
>> +		proxyPath += proxyFile;
>> +		if (!access(proxyPath.c_str(), X_OK))
>> +			return proxyPath;
>>   	}
>>     	/*
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index e31e3879d..723f7665b 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -1183,8 +1183,9 @@ int IPU3CameraData::loadIPA()
>>   	 * The API tuning file is made from the sensor name. If the tuning file
>>   	 * isn't found, fall back to the 'uncalibrated' file.
>>   	 */
>> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>   	std::string ipaTuningFile =
>> -		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
>> +		ipa_->configurationFile(sensor->model() + ".yaml", configuration, "uncalibrated.yaml");
>>     	ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
>>   			 sensorInfo, sensor->controls(), &ipaControls_);
>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> index 4acc091bd..d346fa25f 100644
>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> @@ -388,7 +388,9 @@ int MaliC55CameraData::loadIPA()
>>     	ipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls);
>>   +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>   	std::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + ".yaml",
>> +							    configuration,
>>   							    "uncalibrated.yaml");
>>     	/* We need to inform the IPA of the sensor configuration */
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index 675f0a749..aded4cb43 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -40,6 +40,7 @@
>>   #include "libcamera/internal/delayed_controls.h"
>>   #include "libcamera/internal/device_enumerator.h"
>>   #include "libcamera/internal/framebuffer.h"
>> +#include "libcamera/internal/global_configuration.h"
>>   #include "libcamera/internal/ipa_manager.h"
>>   #include "libcamera/internal/media_device.h"
>>   #include "libcamera/internal/media_pipeline.h"
>> @@ -383,8 +384,9 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>>   	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
>>     	/* The IPA tuning file is made from the sensor name. */
>> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>   	std::string ipaTuningFile =
>> -		ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
>> +		ipa_->configurationFile(sensor_->model() + ".yaml", configuration, "uncalibrated.yaml");
>>     	IPACameraSensorInfo sensorInfo{};
>>   	int ret = sensor_->sensorInfo(&sensorInfo);
>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> index a316ef297..3da919ee7 100644
>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> @@ -1157,7 +1157,8 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
>>   	std::string model = sensor_->model();
>>   	if (isMonoSensor(sensor_))
>>   		model += "_mono";
>> -	std::string configurationFile = ipa_->configurationFile(model + ".json");
>> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>> +	std::string configurationFile = ipa_->configurationFile(model + ".json", configuration);
>>     	IPASettings settings(configurationFile, sensor_->model());
>>   	ipa::RPi::InitParams params;
>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>> index 07273bd2b..555d51bb6 100644
>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>> @@ -494,7 +494,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>>     	data->ipa_->paramsComputed.connect(data.get(), &VimcCameraData::paramsComputed);
>>   -	std::string conf = data->ipa_->configurationFile("vimc.conf");
>> +	const GlobalConfiguration &configuration = manager_->_d()->configuration();
>> +	std::string conf = data->ipa_->configurationFile("vimc.conf", configuration);
>>   	Flags<ipa::vimc::TestFlag> inFlags = ipa::vimc::TestFlag::Flag2;
>>   	Flags<ipa::vimc::TestFlag> outFlags;
>>   	data->ipa_->init(IPASettings{ conf, data->sensor_->model() },
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index 28e2a360e..3ce354111 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -21,6 +21,7 @@
>>   #include <libcamera/stream.h>
>>     #include "libcamera/internal/framebuffer.h"
>> +#include "libcamera/internal/global_configuration.h"
>>   #include "libcamera/internal/ipa_manager.h"
>>   #include "libcamera/internal/software_isp/debayer_params.h"
>>   @@ -130,8 +131,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
>>   	 * The API tuning file is made from the sensor name. If the tuning file
>>   	 * isn't found, fall back to the 'uncalibrated' file.
>>   	 */
>> +	const GlobalConfiguration &configuration = pipe->cameraManager()->_d()->configuration();
>>   	std::string ipaTuningFile =
>> -		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
>> +		ipa_->configurationFile(sensor->model() + ".yaml", configuration, "uncalibrated.yaml");
>>     	IPACameraSensorInfo sensorInfo{};
>>   	int ret = sensor->sensorInfo(&sensorInfo);
>> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
>> index b81783664..1afcbf106 100644
>> --- a/test/ipa/ipa_interface_test.cpp
>> +++ b/test/ipa/ipa_interface_test.cpp
>> @@ -106,7 +106,8 @@ protected:
>>   		}
>>     		/* Test initialization of IPA module. */
>> -		std::string conf = ipa_->configurationFile("vimc.conf");
>> +		const GlobalConfiguration configuration;
>> +		std::string conf = ipa_->configurationFile("vimc.conf", configuration);
>>   		Flags<ipa::vimc::TestFlag> inFlags;
>>   		Flags<ipa::vimc::TestFlag> outFlags;
>>   		int ret = ipa_->init(IPASettings{ conf, "vimc" },
>> 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
>> index 9a3aadbd2..71e920eb1 100644
>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>> @@ -45,7 +45,7 @@ namespace {{ns}} {
>>   {% endfor %}
>>   {%- endif %}
>>   -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
>> +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration)
>>   	: IPAProxy(ipam), isolate_(isolate),
>>   	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
>>   {
>> @@ -54,7 +54,7 @@ namespace {{ns}} {
>>   		<< ipam->path();
>>     	if (isolate_) {
>> -		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
>> +		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy", configuration);
>>   		if (proxyWorkerPath.empty()) {
>>   			LOG(IPAProxy, Error)
>>   				<< "Failed to get proxy worker path";
>> 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
>> index a0312a7c1..057c3ab03 100644
>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
>> @@ -37,7 +37,7 @@ namespace {{ns}} {
>>   class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
>>   {
>>   public:
>> -	{{proxy_name}}(IPAModule *ipam, bool isolate);
>> +	{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration);
>>   	~{{proxy_name}}();
>>     {% for method in interface_main.methods %}

Hi

2025. 06. 27. 23:04 keltezéssel, Milan Zamazal írta:
> Hi Barnabás,
> 
> thank you for review.
> 
> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
> 
>> Hi
>>
>> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:
>>> This patch adds configuration options for environment variables used in
>>> the IPA proxy.  Two utility functions configuration retrieval functions
>>> are added, to retrieve lists of values.
>>> The configuration snippet:
>>>     configuration:
>>>       ipa:
>>>         config_paths:
>>>         - config path 1
>>>         - config path 2
>>>         - ...
>>>         module_paths:
>>>         - module path 1
>>>         - module path 2
>>>         - ...
>>>         proxy_paths:
>>>         - proxy path 1
>>>         - proxy path 2
>>>         - ...
>>>         force_isolation: BOOL
>>>         ipas:
>>>           IPA_NAME:
>>>             tuning_file: TUNING FILE
>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>> ---
>>>    .../libcamera/internal/global_configuration.h | 35 ++++++++-
>>>    include/libcamera/internal/ipa_manager.h      |  8 ++-
>>>    include/libcamera/internal/ipa_proxy.h        |  5 +-
>>>    src/libcamera/camera_manager.cpp              |  2 +-
>>>    src/libcamera/global_configuration.cpp        | 47 +++++++++++-
>>>    src/libcamera/ipa_manager.cpp                 | 37 ++++++----
>>>    src/libcamera/ipa_proxy.cpp                   | 72 ++++++++++---------
>>>    src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 +-
>>>    src/libcamera/pipeline/mali-c55/mali-c55.cpp  |  2 +
>>>    src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  4 +-
>>>    .../pipeline/rpi/common/pipeline_base.cpp     |  3 +-
>>>    src/libcamera/pipeline/vimc/vimc.cpp          |  3 +-
>>>    src/libcamera/software_isp/software_isp.cpp   |  4 +-
>>>    test/ipa/ipa_interface_test.cpp               |  3 +-
>>>    .../module_ipa_proxy.cpp.tmpl                 |  4 +-
>>>    .../module_ipa_proxy.h.tmpl                   |  2 +-
>>>    16 files changed, 169 insertions(+), 65 deletions(-)
>>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>>> index 357119628..cc436703e 100644
>>> --- a/include/libcamera/internal/global_configuration.h
>>> +++ b/include/libcamera/internal/global_configuration.h
>>> @@ -11,6 +11,8 @@
>>>    #include <optional>
>>>    #include <string>
>>>    +#include <libcamera/base/utils.h>
>>> +
>>>    #include "libcamera/internal/yaml_parser.h"
>>>      namespace libcamera {
>>> @@ -24,9 +26,40 @@ public:
>>>      	unsigned int version() const;
>>>    	Configuration configuration() const;
>>> -	std::optional<std::string> option(const std::string &confPath) const;
>>> +
>>> +#ifndef __DOXYGEN__
>>> +	template<typename T,
>>> +		 std::enable_if_t<
>>> +			 std::is_same_v<bool, T> ||
>>> +			 std::is_same_v<double, T> ||
>>> +			 std::is_same_v<int8_t, T> ||
>>> +			 std::is_same_v<uint8_t, T> ||
>>> +			 std::is_same_v<int16_t, T> ||
>>> +			 std::is_same_v<uint16_t, T> ||
>>> +			 std::is_same_v<int32_t, T> ||
>>> +			 std::is_same_v<uint32_t, T> ||
>>> +			 std::is_same_v<std::string, T> ||
>>> +			 std::is_same_v<Size, T>> * = nullptr>
>>
>> Is this needed? `YamlObject::get()` does not have any constraints,
>> so I think it can be omitted here as well?
> 
> Yes; YamlObject::get() used to have the constraints but doesn't have
> them any more.
> 
>>> +#else
>>> +	template<typename T>
>>> +#endif
>>> +	std::optional<T> option(const std::string &confPath) const
>>> +	{
>>> +		const YamlObject *c = &configuration();
>>> +		for (auto part : utils::split(confPath, "/")) {
>>> +			c = &(*c)[part];
>>> +			if (!*c)
>>> +				return {};
>>> +		}
>>> +		return c->get<T>();
>>> +	}
>>> +
>>> +	std::vector<std::string> listOption(const std::string &confPath) const;
>>>    	std::optional<std::string> envOption(const char *const envVariable,
>>>    					     const std::string &confPath) const;
>>> +	std::vector<std::string> envListOption(
>>> +		const char *const envVariable,
>>> +		const std::string &confPath) const;
>>>      private:
>>>    	bool loadFile(const std::filesystem::path &fileName);
>>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
>>> index a0d448cf9..8ad717919 100644
>>> --- a/include/libcamera/internal/ipa_manager.h
>>> +++ b/include/libcamera/internal/ipa_manager.h
>>> @@ -17,6 +17,7 @@
>>>    #include <libcamera/ipa/ipa_module_info.h>
>>>      #include "libcamera/internal/camera_manager.h"
>>> +#include "libcamera/internal/global_configuration.h"
>>>    #include "libcamera/internal/ipa_module.h"
>>>    #include "libcamera/internal/pipeline_handler.h"
>>>    #include "libcamera/internal/pub_key.h"
>>> @@ -28,7 +29,7 @@ LOG_DECLARE_CATEGORY(IPAManager)
>>>    class IPAManager
>>>    {
>>>    public:
>>> -	IPAManager();
>>> +	IPAManager(const GlobalConfiguration &configuration);
>>>    	~IPAManager();
>>>      	template<typename T>
>>> @@ -42,7 +43,8 @@ public:
>>>    		if (!m)
>>>    			return nullptr;
>>>    -		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
>>> +		const GlobalConfiguration &configuration = cm->_d()->configuration();
>>> +		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m, configuration), configuration);
>>>    		if (!proxy->isValid()) {
>>>    			LOG(IPAManager, Error) << "Failed to load proxy";
>>>    			return nullptr;
>>> @@ -66,7 +68,7 @@ private:
>>>    	IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,
>>>    			  uint32_t maxVersion);
>>>    -	bool isSignatureValid(IPAModule *ipa) const;
>>> +	bool isSignatureValid(IPAModule *ipa, const GlobalConfiguration &configuration) const;
>>>      	std::vector<std::unique_ptr<IPAModule>> modules_;
>>>    diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
>>> index 983bcc5fa..01cc5deff 100644
>>> --- a/include/libcamera/internal/ipa_proxy.h
>>> +++ b/include/libcamera/internal/ipa_proxy.h
>>> @@ -11,6 +11,8 @@
>>>      #include <libcamera/ipa/ipa_interface.h>
>>>    +#include "libcamera/internal/global_configuration.h"
>>> +
>>>    namespace libcamera {
>>>      class IPAModule;
>>> @@ -30,10 +32,11 @@ public:
>>>    	bool isValid() const { return valid_; }
>>>      	std::string configurationFile(const std::string &name,
>>> +				      const GlobalConfiguration &configuration,
>>>    				      const std::string &fallbackName = std::string()) const;
>>>      protected:
>>> -	std::string resolvePath(const std::string &file) const;
>>> +	std::string resolvePath(const std::string &file, const GlobalConfiguration &configuration) const;
>>
>> I feel like `IPAProxy` could take the `GlobalConfiguration`
>> in the constructor?
> 
> I.e. copy the configuration and changing std::unique_ptr<YamlObject>
> inside to shared_ptr?  Possible, I think, and I don't have a particular
> preference -- up to your judgement what's better.

I had two options in mind:
   
   (1) Extract everything from the configuration that you need and store them
       in member variables. This would mean moving the envListOption() calls to
       the constructor.
   (2) Just store the `GlobalConfiguration::Configuration` object. Since the lifetime
       of IPAManager is tied to CameraManager, this shouldn't cause issue.

But now I see that while (2) is simpler, `GlobalConfiguration::Configuration` is just
a reference, so it does not have the `{env,}{list,}option()` functions that would
be needed. And (1) is a more significant change.

I would really prefer if you didn't have to pass a configuration instance to every
method that might need it. But I don't have a good suggestion at the moment.



> 
>>>      	bool valid_;
>>>    	ProxyState state_;
>>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>>> index f3b4ec708..c47fd3677 100644
>>> --- a/src/libcamera/camera_manager.cpp
>>> +++ b/src/libcamera/camera_manager.cpp
>>> @@ -41,7 +41,7 @@ LOG_DEFINE_CATEGORY(Camera)
>>>    CameraManager::Private::Private()
>>>    	: initialized_(false)
>>>    {
>>> -	ipaManager_ = std::make_unique<IPAManager>();
>>> +	ipaManager_ = std::make_unique<IPAManager>(this->configuration());
>>>    }
>>>      int CameraManager::Private::start()
>>> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
>>> index 068ddd5a5..f7c69890c 100644
>>> --- a/src/libcamera/global_configuration.cpp
>>> +++ b/src/libcamera/global_configuration.cpp
>>> @@ -9,9 +9,11 @@
>>>      #include <filesystem>
>>>    #include <memory>
>>> +#include <optional>
>>>    #include <string>
>>>    #include <string_view>
>>>    #include <sys/types.h>
>>> +#include <vector>
>>>      #include <libcamera/base/file.h>
>>>    #include <libcamera/base/log.h>
>>> @@ -116,13 +118,22 @@ GlobalConfiguration::GlobalConfiguration()
>>>     */
>>>      /**
>>> + * \fn std::optional<T> GlobalConfiguration::option(const std::string &confPath) const
>>>     * \brief Return value of the configuration option identified by \a confPath
>>>     * \param[in] confPath Sequence of the YAML section names (excluding
>>>     * `configuration') leading to the requested option separated by slashes
>>>     * \return A value if an item corresponding to \a confPath exists in the
>>>     * configuration file, no value otherwise
>>>     */
>>> -std::optional<std::string> GlobalConfiguration::option(const std::string &confPath) const
>>> +
>>> +/**
>>> + * \brief Return values of the configuration option identified by \a confPath
>>> + * \tparam T The type of the retrieved configuration value
>>> + * \param[in] confPath Sequence of the YAML section names (excluding
>>> + * `configuration') leading to the requested list option, separated by dots
>>> + * \return A vector of strings (empty one if the option is not found)
>>> + */
>>> +std::vector<std::string> GlobalConfiguration::listOption(const std::string &confPath) const
>>>    {
>>>    	const YamlObject *c = &configuration();
>>>    	for (auto part : utils::split(confPath, "/")) {
>>> @@ -130,7 +141,7 @@ std::optional<std::string> GlobalConfiguration::option(const std::string &confPa
>>>    		if (!*c)
>>>    			return {};
>>>    	}
>>> -	return c->get<std::string>();
>>> +	return c->getList<std::string>().value_or(std::vector<std::string>());
>>>    }
>>>      /**
>>> @@ -157,7 +168,37 @@ std::optional<std::string> GlobalConfiguration::envOption(
>>>    	const char *envValue = utils::secure_getenv(envVariable);
>>>    	if (envValue)
>>>    		return std::optional{ std::string{ envValue } };
>>> -	return option(confPath);
>>> +	return option<std::string>(confPath);
>>> +}
>>> +
>>> +/**
>>> + * \brief Return values of the configuration option from a file or environment
>>> + * \param[in] envVariable Environment variable to get the value from
>>> + * \param[in] confPath The same as in GlobalConfiguration::option
>>> + *
>>> + * This helper looks first at the given environment variable and if it is
>>> + * defined (even if it is empty) then it splits its value by semicolons and
>>> + * returns the resulting list of strings. Otherwise it looks for \a confPath the
>>> + * same way as in GlobalConfiguration::option, value of which must be a list of
>>> + * strings.
>>> + *
>>> + * \note Support for using environment variables to configure libcamera behavior
>>> + * is provided here mostly for backward compatibility reasons. Introducing new
>>> + * configuration environment variables is discouraged.
>>> + *
>>> + * \return A vector of strings retrieved from the given environment option or
>>> + * configuration file (an empty vector is returned if nothing is found)
>>
>> I think we want to distinguish "not present" and "empty". I could imagine
>> that someone might want to inhibit some behaviour by setting the env var
>> to empty.
> 
> Well, introducing new environment variables for configuration is
> discouraged.

I think for some things the convenience for testing/debugging/etc. will
outweigh the drawbacks and we will decide to add a new env var.


> 
>> `envOption()` also differentiates the two. For example, with the new
>> "layers" proposal, I could image that we can set the layers in a
>> configuration file, etc, as well as an environmental variable. In that
>> case doing `LIBCAMERA_LAYERS= cam ...` might be useful for testing to
>> temporarily disable all layers.
> 
> Hmm, sounds reasonable but I'm still not sure we want to encourage this.
> But technically, I can change the method signature.

Encourage env vars in general, or the use of empty env vars specifically?
In any case, I think it would be nice if `envOption()` and `envListOption`
were consistent wrt. empty env vars.


> 
>>> + */
>>> +std::vector<std::string> GlobalConfiguration::envListOption(
>>> +	const char *const envVariable,
>>> +	const std::string &confPath) const
>>> +{
>>> +	const char *envValue = utils::secure_getenv(envVariable);
>>> +	if (envValue) {
>>> +		auto items = utils::split(envValue, ":");
>>> +		return std::vector<std::string>(items.begin(), items.end());
>>> +	}
>>> +	return listOption(confPath);
>>>    }
>>>      /**
>>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>>> index 830750dcc..67c1b23d2 100644
>>> --- a/src/libcamera/ipa_manager.cpp
>>> +++ b/src/libcamera/ipa_manager.cpp
>>> @@ -9,6 +9,7 @@
>>>      #include <algorithm>
>>>    #include <dirent.h>
>>> +#include <numeric>
>>>    #include <string.h>
>>>    #include <sys/types.h>
>>>    @@ -16,6 +17,7 @@
>>>    #include <libcamera/base/log.h>
>>>    #include <libcamera/base/utils.h>
>>>    +#include "libcamera/internal/global_configuration.h"
>>>    #include "libcamera/internal/ipa_module.h"
>>>    #include "libcamera/internal/ipa_proxy.h"
>>>    #include "libcamera/internal/pipeline_handler.h"
>>> @@ -101,7 +103,7 @@ LOG_DEFINE_CATEGORY(IPAManager)
>>>     * The IPAManager class is meant to only be instantiated once, by the
>>>     * CameraManager.
>>>     */
>>> -IPAManager::IPAManager()
>>> +IPAManager::IPAManager(const GlobalConfiguration &configuration)
>>>    {
>>>    #if HAVE_IPA_PUBKEY
>>>    	if (!pubKey_.isValid())
>>> @@ -111,18 +113,25 @@ IPAManager::IPAManager()
>>>    	unsigned int ipaCount = 0;
>>>      	/* User-specified paths take precedence. */
>>> -	const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
>>> -	if (modulePaths) {
>>> -		for (const auto &dir : utils::split(modulePaths, ":")) {
>>> -			if (dir.empty())
>>> -				continue;
>>> +	const auto modulePaths =
>>> +		configuration.envListOption(
>>> +			"LIBCAMERA_IPA_MODULE_PATH", "ipa/module_paths");
>>> +	for (const auto &dir : modulePaths) {
>>> +		if (dir.empty())
>>> +			continue;
>>>    -			ipaCount += addDir(dir.c_str());
>>> -		}
>>> +		ipaCount += addDir(dir.c_str());
>>> +	}
>>>    -		if (!ipaCount)
>>> -			LOG(IPAManager, Warning)
>>> -				<< "No IPA found in '" << modulePaths << "'";
>>> +	if (!ipaCount) {
>>> +		std::string paths;
>>> +		if (!modulePaths.empty()) {
>>> +			paths = std::accumulate(std::next(modulePaths.begin()),
>>> +						modulePaths.end(),
>>> +						modulePaths[0],
>>> +						[](std::string s1, std::string s2) { return s1 + ":" + s2; });
>>> +		}
>>> +		LOG(IPAManager, Warning) << "No IPA found in '" << paths << "'";
>>
>>    #include <libcamera/base/utils.h>
>>    ...
>>    LOG(IPAManager, Warning) << "No IPA found in '" << utils::join(modulePaths, ":") << "'";
>>
>> ?
> 
> OK.
> 
>>>    	}
>>>      	/*
>>> @@ -276,11 +285,13 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>>>     */
>>>    #endif
>>>    -bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
>>> +bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa, const GlobalConfiguration &configuration) const
>>
>> Maybe `configuration.option<bool>("ipa/force_isolation").value_or(false)` could be
>> saved into a member variable in the constructor? Then this won't need
>> the configuration parameter.
> 
> Yes.  Assuming the `pipe' CameraManager instance is always the same that
> was used to create the IPAManager instance.

IpaManager::createIPA() goes from PipelineHandler -> CameraManager -> IPAManager,
so I am quite certain there is no room for mixup.


> 
>>>    {
>>>    #if HAVE_IPA_PUBKEY
>>>    	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
>>> -	if (force && force[0] != '\0') {
>>> +	if ((force && force[0] != '\0') ||
>>> +	    (!force && configuration.option<bool>("ipa/force_isolation")
>>> +			       .value_or(false))) {
>>>    		LOG(IPAManager, Debug)
>>>    			<< "Isolation of IPA module " << ipa->path()
>>>    			<< " forced through environment variable";
>>> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
>>> index 9907b9615..77927b0d4 100644
>>> --- a/src/libcamera/ipa_proxy.cpp
>>> +++ b/src/libcamera/ipa_proxy.cpp
>>> @@ -14,6 +14,7 @@
>>>    #include <libcamera/base/log.h>
>>>    #include <libcamera/base/utils.h>
>>>    +#include "libcamera/internal/global_configuration.h"
>>>    #include "libcamera/internal/ipa_module.h"
>>>      /**
>>> @@ -71,6 +72,7 @@ IPAProxy::~IPAProxy()
>>>    /**
>>>     * \brief Retrieve the absolute path to an IPA configuration file
>>>     * \param[in] name The configuration file name
>>> + * \param[in] configuration The global configuration
>>>     * \param[in] fallbackName The name of a fallback configuration file
>>>     *
>>>     * This function locates the configuration file for an IPA and returns its
>>> @@ -96,6 +98,7 @@ IPAProxy::~IPAProxy()
>>>     * no configuration file can be found
>>>     */
>>>    std::string IPAProxy::configurationFile(const std::string &name,
>>> +					const GlobalConfiguration &configuration,
>>>    					const std::string &fallbackName) const
>>>    {
>>>    	/*
>>> @@ -105,37 +108,37 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>>    	const std::string ipaName = ipam_->info().name;
>>>      	/*
>>> -	 * Start with any user override through the module-specific environment
>>> -	 * variable. Use the name of the IPA module up to the first '/' to
>>> -	 * construct the variable name.
>>> +	 * Start with any user override through the module-specific configuration or
>>> +	 * environment variable. Use the name of the IPA module up to the first '/'
>>> +	 * to construct the configuration and variable names.
>>>    	 */
>>> -	std::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));
>>> +	std::string ipaBaseName = ipaName.substr(0, ipaName.find('/'));
>>> +	std::string ipaConfigName = "ipa/ipas/" + ipaBaseName + "/tuning_file";
>>
>> I feel like we're unnecessarily limiting outself wrt. rpi here. Since the IPA
>> names are "rpi/vc4" and "rpi/pisp", thus `ipaBaseName == "rpi"`. Which means
>> that it is impossible to specify different tuning files for the different
>> platforms. I think this is fine for an env var, but for a configuration file
>> I think this flexibility is expected.
> 
> Right; so rather than trimming, let's replace the slash with another
> character here?

I am not sure what the best option is. Replacing `/` with `_` or `-` seems
like a good candidate. But note that the env var has to keep its name, so
the trimming will still have to be done.


> 
>> But I don't quite see the point of overriding the tuning file like this from a
>> configuration file. So maybe leaving just the env var override is sufficient?
> 
> I think the initial idea was to be able to configure everything in the
> configuration file (why not?).  Logging is currently a noticeable
> exception, although hopefully only "temporary".

I think `LIBCAMERA_${IPA}_TUNING_FILE` is mainly a testing feature because
it overrides the tuning file "globally" for the given IPA. My first impression
was that it is probably not that useful to have in a configuration file. And
I still think that, so my preference is not including it in the configuration
file. With that, feel free to decide, but please address the above rpi naming
issue if you want to include it.


Regards,
Barnabás Pőcze


> 
>>> +	std::string ipaEnvName = ipaBaseName;
>>>    	std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),
>>>    		       [](unsigned char c) { return std::toupper(c); });
>>>    	ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE";
>>>    -	char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());
>>> -	if (configFromEnv && *configFromEnv != '\0')
>>> -		return { configFromEnv };
>>> +	auto config = configuration.envOption(ipaEnvName.c_str(), ipaConfigName);
>>> +	if (config)
>>> +		return { config.value() };
>>>      	struct stat statbuf;
>>>    	int ret;
>>>      	/*
>>> -	 * Check the directory pointed to by the IPA config path environment
>>> -	 * variable next.
>>> +	 * Check the directory pointed to by the IPA config path next.
>>>    	 */
>>> -	const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
>>> -	if (confPaths) {
>>> -		for (const auto &dir : utils::split(confPaths, ":")) {
>>> -			if (dir.empty())
>>> -				continue;
>>> -
>>> -			std::string confPath = dir + "/" + ipaName + "/" + name;
>>> -			ret = stat(confPath.c_str(), &statbuf);
>>> -			if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
>>> -				return confPath;
>>> -		}
>>> +	auto confPaths =
>>> +		configuration.envListOption(
>>> +			"LIBCAMERA_IPA_CONFIG_PATH", "ipa/config_paths");
>>> +	for (const auto &dir : confPaths) {
>>> +		if (dir.empty())
>>> +			continue;
>>> +		std::string confPath = dir + "/" + ipaName + "/" + name;
>>> +		ret = stat(confPath.c_str(), &statbuf);
>>> +		if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
>>> +			return confPath;
>>>    	}
>>>      	std::string root = utils::libcameraSourcePath();
>>> @@ -178,12 +181,13 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>>    		<< "Configuration file '" << name
>>>    		<< "' not found for IPA module '" << ipaName
>>>    		<< "', falling back to '" << fallbackName << "'";
>>> -	return configurationFile(fallbackName);
>>> +	return configurationFile(fallbackName, configuration);
>>>    }
>>>      /**
>>>     * \brief Find a valid full path for a proxy worker for a given executable name
>>>     * \param[in] file File name of proxy worker executable
>>> + * \param[in] configuration The global configuration
>>>     *
>>>     * A proxy worker's executable could be found in either the global installation
>>>     * directory, or in the paths specified by the environment variable
>>> @@ -195,22 +199,22 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>>     * \return The full path to the proxy worker executable, or an empty string if
>>>     * no valid executable path
>>>     */
>>> -std::string IPAProxy::resolvePath(const std::string &file) const
>>> +std::string IPAProxy::resolvePath(const std::string &file, const GlobalConfiguration &configuration) const
>>>    {
>>>    	std::string proxyFile = "/" + file;
>>>    -	/* Check env variable first. */
>>> -	const char *execPaths = utils::secure_getenv("LIBCAMERA_IPA_PROXY_PATH");
>>> -	if (execPaths) {
>>> -		for (const auto &dir : utils::split(execPaths, ":")) {
>>> -			if (dir.empty())
>>> -				continue;
>>> -
>>> -			std::string proxyPath = dir;
>>> -			proxyPath += proxyFile;
>>> -			if (!access(proxyPath.c_str(), X_OK))
>>> -				return proxyPath;
>>> -		}
>>> +	/* Check the configuration first. */
>>> +	const auto execPaths =
>>> +		configuration.envListOption(
>>> +			"LIBCAMERA_IPA_PROXY_PATH", "ipa/proxy_paths");
>>> +	for (const auto &dir : execPaths) {
>>> +		if (dir.empty())
>>> +			continue;
>>> +
>>> +		std::string proxyPath = dir;
>>
>> std::string proxyPath = dir + proxyFile;
> 
> OK.
> 
>> Regards,
>> Barnabás Pőcze
>>
>>
>>> +		proxyPath += proxyFile;
>>> +		if (!access(proxyPath.c_str(), X_OK))
>>> +			return proxyPath;
>>>    	}
>>>      	/*
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index e31e3879d..723f7665b 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -1183,8 +1183,9 @@ int IPU3CameraData::loadIPA()
>>>    	 * The API tuning file is made from the sensor name. If the tuning file
>>>    	 * isn't found, fall back to the 'uncalibrated' file.
>>>    	 */
>>> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>>    	std::string ipaTuningFile =
>>> -		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
>>> +		ipa_->configurationFile(sensor->model() + ".yaml", configuration, "uncalibrated.yaml");
>>>      	ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
>>>    			 sensorInfo, sensor->controls(), &ipaControls_);
>>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>>> index 4acc091bd..d346fa25f 100644
>>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>>> @@ -388,7 +388,9 @@ int MaliC55CameraData::loadIPA()
>>>      	ipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls);
>>>    +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>>    	std::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + ".yaml",
>>> +							    configuration,
>>>    							    "uncalibrated.yaml");
>>>      	/* We need to inform the IPA of the sensor configuration */
>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> index 675f0a749..aded4cb43 100644
>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> @@ -40,6 +40,7 @@
>>>    #include "libcamera/internal/delayed_controls.h"
>>>    #include "libcamera/internal/device_enumerator.h"
>>>    #include "libcamera/internal/framebuffer.h"
>>> +#include "libcamera/internal/global_configuration.h"
>>>    #include "libcamera/internal/ipa_manager.h"
>>>    #include "libcamera/internal/media_device.h"
>>>    #include "libcamera/internal/media_pipeline.h"
>>> @@ -383,8 +384,9 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>>>    	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
>>>      	/* The IPA tuning file is made from the sensor name. */
>>> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>>    	std::string ipaTuningFile =
>>> -		ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
>>> +		ipa_->configurationFile(sensor_->model() + ".yaml", configuration, "uncalibrated.yaml");
>>>      	IPACameraSensorInfo sensorInfo{};
>>>    	int ret = sensor_->sensorInfo(&sensorInfo);
>>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>>> index a316ef297..3da919ee7 100644
>>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>>> @@ -1157,7 +1157,8 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
>>>    	std::string model = sensor_->model();
>>>    	if (isMonoSensor(sensor_))
>>>    		model += "_mono";
>>> -	std::string configurationFile = ipa_->configurationFile(model + ".json");
>>> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>> +	std::string configurationFile = ipa_->configurationFile(model + ".json", configuration);
>>>      	IPASettings settings(configurationFile, sensor_->model());
>>>    	ipa::RPi::InitParams params;
>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>>> index 07273bd2b..555d51bb6 100644
>>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>>> @@ -494,7 +494,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>>>      	data->ipa_->paramsComputed.connect(data.get(), &VimcCameraData::paramsComputed);
>>>    -	std::string conf = data->ipa_->configurationFile("vimc.conf");
>>> +	const GlobalConfiguration &configuration = manager_->_d()->configuration();
>>> +	std::string conf = data->ipa_->configurationFile("vimc.conf", configuration);
>>>    	Flags<ipa::vimc::TestFlag> inFlags = ipa::vimc::TestFlag::Flag2;
>>>    	Flags<ipa::vimc::TestFlag> outFlags;
>>>    	data->ipa_->init(IPASettings{ conf, data->sensor_->model() },
>>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>>> index 28e2a360e..3ce354111 100644
>>> --- a/src/libcamera/software_isp/software_isp.cpp
>>> +++ b/src/libcamera/software_isp/software_isp.cpp
>>> @@ -21,6 +21,7 @@
>>>    #include <libcamera/stream.h>
>>>      #include "libcamera/internal/framebuffer.h"
>>> +#include "libcamera/internal/global_configuration.h"
>>>    #include "libcamera/internal/ipa_manager.h"
>>>    #include "libcamera/internal/software_isp/debayer_params.h"
>>>    @@ -130,8 +131,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
>>>    	 * The API tuning file is made from the sensor name. If the tuning file
>>>    	 * isn't found, fall back to the 'uncalibrated' file.
>>>    	 */
>>> +	const GlobalConfiguration &configuration = pipe->cameraManager()->_d()->configuration();
>>>    	std::string ipaTuningFile =
>>> -		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
>>> +		ipa_->configurationFile(sensor->model() + ".yaml", configuration, "uncalibrated.yaml");
>>>      	IPACameraSensorInfo sensorInfo{};
>>>    	int ret = sensor->sensorInfo(&sensorInfo);
>>> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
>>> index b81783664..1afcbf106 100644
>>> --- a/test/ipa/ipa_interface_test.cpp
>>> +++ b/test/ipa/ipa_interface_test.cpp
>>> @@ -106,7 +106,8 @@ protected:
>>>    		}
>>>      		/* Test initialization of IPA module. */
>>> -		std::string conf = ipa_->configurationFile("vimc.conf");
>>> +		const GlobalConfiguration configuration;
>>> +		std::string conf = ipa_->configurationFile("vimc.conf", configuration);
>>>    		Flags<ipa::vimc::TestFlag> inFlags;
>>>    		Flags<ipa::vimc::TestFlag> outFlags;
>>>    		int ret = ipa_->init(IPASettings{ conf, "vimc" },
>>> 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
>>> index 9a3aadbd2..71e920eb1 100644
>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>>> @@ -45,7 +45,7 @@ namespace {{ns}} {
>>>    {% endfor %}
>>>    {%- endif %}
>>>    -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
>>> +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration)
>>>    	: IPAProxy(ipam), isolate_(isolate),
>>>    	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
>>>    {
>>> @@ -54,7 +54,7 @@ namespace {{ns}} {
>>>    		<< ipam->path();
>>>      	if (isolate_) {
>>> -		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
>>> +		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy", configuration);
>>>    		if (proxyWorkerPath.empty()) {
>>>    			LOG(IPAProxy, Error)
>>>    				<< "Failed to get proxy worker path";
>>> 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
>>> index a0312a7c1..057c3ab03 100644
>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
>>> @@ -37,7 +37,7 @@ namespace {{ns}} {
>>>    class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
>>>    {
>>>    public:
>>> -	{{proxy_name}}(IPAModule *ipam, bool isolate);
>>> +	{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration);
>>>    	~{{proxy_name}}();
>>>      {% for method in interface_main.methods %}
>

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Hi
>
> 2025. 06. 27. 23:04 keltezéssel, Milan Zamazal írta:
>> Hi Barnabás,
>> thank you for review.
>> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:
>> 
>>> Hi
>>>
>>> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:
>>>> This patch adds configuration options for environment variables used in
>>>> the IPA proxy.  Two utility functions configuration retrieval functions
>>>> are added, to retrieve lists of values.
>>>> The configuration snippet:
>>>>     configuration:
>>>>       ipa:
>>>>         config_paths:
>>>>         - config path 1
>>>>         - config path 2
>>>>         - ...
>>>>         module_paths:
>>>>         - module path 1
>>>>         - module path 2
>>>>         - ...
>>>>         proxy_paths:
>>>>         - proxy path 1
>>>>         - proxy path 2
>>>>         - ...
>>>>         force_isolation: BOOL
>>>>         ipas:
>>>>           IPA_NAME:
>>>>             tuning_file: TUNING FILE
>>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>>> ---
>>>>    .../libcamera/internal/global_configuration.h | 35 ++++++++-
>>>>    include/libcamera/internal/ipa_manager.h      |  8 ++-
>>>>    include/libcamera/internal/ipa_proxy.h        |  5 +-
>>>>    src/libcamera/camera_manager.cpp              |  2 +-
>>>>    src/libcamera/global_configuration.cpp        | 47 +++++++++++-
>>>>    src/libcamera/ipa_manager.cpp                 | 37 ++++++----
>>>>    src/libcamera/ipa_proxy.cpp                   | 72 ++++++++++---------
>>>>    src/libcamera/pipeline/ipu3/ipu3.cpp          |  3 +-
>>>>    src/libcamera/pipeline/mali-c55/mali-c55.cpp  |  2 +
>>>>    src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  4 +-
>>>>    .../pipeline/rpi/common/pipeline_base.cpp     |  3 +-
>>>>    src/libcamera/pipeline/vimc/vimc.cpp          |  3 +-
>>>>    src/libcamera/software_isp/software_isp.cpp   |  4 +-
>>>>    test/ipa/ipa_interface_test.cpp               |  3 +-
>>>>    .../module_ipa_proxy.cpp.tmpl                 |  4 +-
>>>>    .../module_ipa_proxy.h.tmpl                   |  2 +-
>>>>    16 files changed, 169 insertions(+), 65 deletions(-)
>>>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>>>> index 357119628..cc436703e 100644
>>>> --- a/include/libcamera/internal/global_configuration.h
>>>> +++ b/include/libcamera/internal/global_configuration.h
>>>> @@ -11,6 +11,8 @@
>>>>    #include <optional>
>>>>    #include <string>
>>>>    +#include <libcamera/base/utils.h>
>>>> +
>>>>    #include "libcamera/internal/yaml_parser.h"
>>>>      namespace libcamera {
>>>> @@ -24,9 +26,40 @@ public:
>>>>      	unsigned int version() const;
>>>>    	Configuration configuration() const;
>>>> -	std::optional<std::string> option(const std::string &confPath) const;
>>>> +
>>>> +#ifndef __DOXYGEN__
>>>> +	template<typename T,
>>>> +		 std::enable_if_t<
>>>> +			 std::is_same_v<bool, T> ||
>>>> +			 std::is_same_v<double, T> ||
>>>> +			 std::is_same_v<int8_t, T> ||
>>>> +			 std::is_same_v<uint8_t, T> ||
>>>> +			 std::is_same_v<int16_t, T> ||
>>>> +			 std::is_same_v<uint16_t, T> ||
>>>> +			 std::is_same_v<int32_t, T> ||
>>>> +			 std::is_same_v<uint32_t, T> ||
>>>> +			 std::is_same_v<std::string, T> ||
>>>> +			 std::is_same_v<Size, T>> * = nullptr>
>>>
>>> Is this needed? `YamlObject::get()` does not have any constraints,
>>> so I think it can be omitted here as well?
>> Yes; YamlObject::get() used to have the constraints but doesn't have
>> them any more.
>> 
>>>> +#else
>>>> +	template<typename T>
>>>> +#endif
>>>> +	std::optional<T> option(const std::string &confPath) const
>>>> +	{
>>>> +		const YamlObject *c = &configuration();
>>>> +		for (auto part : utils::split(confPath, "/")) {
>>>> +			c = &(*c)[part];
>>>> +			if (!*c)
>>>> +				return {};
>>>> +		}
>>>> +		return c->get<T>();
>>>> +	}
>>>> +
>>>> +	std::vector<std::string> listOption(const std::string &confPath) const;
>>>>    	std::optional<std::string> envOption(const char *const envVariable,
>>>>    					     const std::string &confPath) const;
>>>> +	std::vector<std::string> envListOption(
>>>> +		const char *const envVariable,
>>>> +		const std::string &confPath) const;
>>>>      private:
>>>>    	bool loadFile(const std::filesystem::path &fileName);
>>>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
>>>> index a0d448cf9..8ad717919 100644
>>>> --- a/include/libcamera/internal/ipa_manager.h
>>>> +++ b/include/libcamera/internal/ipa_manager.h
>>>> @@ -17,6 +17,7 @@
>>>>    #include <libcamera/ipa/ipa_module_info.h>
>>>>      #include "libcamera/internal/camera_manager.h"
>>>> +#include "libcamera/internal/global_configuration.h"
>>>>    #include "libcamera/internal/ipa_module.h"
>>>>    #include "libcamera/internal/pipeline_handler.h"
>>>>    #include "libcamera/internal/pub_key.h"
>>>> @@ -28,7 +29,7 @@ LOG_DECLARE_CATEGORY(IPAManager)
>>>>    class IPAManager
>>>>    {
>>>>    public:
>>>> -	IPAManager();
>>>> +	IPAManager(const GlobalConfiguration &configuration);
>>>>    	~IPAManager();
>>>>      	template<typename T>
>>>> @@ -42,7 +43,8 @@ public:
>>>>    		if (!m)
>>>>    			return nullptr;
>>>>    -		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
>>>> +		const GlobalConfiguration &configuration = cm->_d()->configuration();
>>>> +		std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m, configuration), configuration);
>>>>    		if (!proxy->isValid()) {
>>>>    			LOG(IPAManager, Error) << "Failed to load proxy";
>>>>    			return nullptr;
>>>> @@ -66,7 +68,7 @@ private:
>>>>    	IPAModule *module(PipelineHandler *pipe, uint32_t minVersion,
>>>>    			  uint32_t maxVersion);
>>>>    -	bool isSignatureValid(IPAModule *ipa) const;
>>>> +	bool isSignatureValid(IPAModule *ipa, const GlobalConfiguration &configuration) const;
>>>>      	std::vector<std::unique_ptr<IPAModule>> modules_;
>>>>    diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
>>>> index 983bcc5fa..01cc5deff 100644
>>>> --- a/include/libcamera/internal/ipa_proxy.h
>>>> +++ b/include/libcamera/internal/ipa_proxy.h
>>>> @@ -11,6 +11,8 @@
>>>>      #include <libcamera/ipa/ipa_interface.h>
>>>>    +#include "libcamera/internal/global_configuration.h"
>>>> +
>>>>    namespace libcamera {
>>>>      class IPAModule;
>>>> @@ -30,10 +32,11 @@ public:
>>>>    	bool isValid() const { return valid_; }
>>>>      	std::string configurationFile(const std::string &name,
>>>> +				      const GlobalConfiguration &configuration,
>>>>    				      const std::string &fallbackName = std::string()) const;
>>>>      protected:
>>>> -	std::string resolvePath(const std::string &file) const;
>>>> +	std::string resolvePath(const std::string &file, const GlobalConfiguration &configuration) const;
>>>
>>> I feel like `IPAProxy` could take the `GlobalConfiguration`
>>> in the constructor?
>> I.e. copy the configuration and changing std::unique_ptr<YamlObject>
>> inside to shared_ptr?  Possible, I think, and I don't have a particular
>> preference -- up to your judgement what's better.
>
> I had two options in mind:
>      (1) Extract everything from the configuration that you need and store them
>       in member variables. This would mean moving the envListOption() calls to
>       the constructor.
>   (2) Just store the `GlobalConfiguration::Configuration` object. Since the lifetime
>       of IPAManager is tied to CameraManager, this shouldn't cause issue.
>
> But now I see that while (2) is simpler, `GlobalConfiguration::Configuration` is just
> a reference, so it does not have the `{env,}{list,}option()` functions that would
> be needed. 

Hmm, right.

> And (1) is a more significant change.

I may try.

> I would really prefer if you didn't have to pass a configuration instance to every
> method that might need it. 

Me too, but things like this are the price for not having a globally
accessible configuration.  It's manageable some way here but I'm really
curious about how to make logging non-global.

> But I don't have a good suggestion at the moment.
>
>
>
>> 
>>>>      	bool valid_;
>>>>    	ProxyState state_;
>>>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>>>> index f3b4ec708..c47fd3677 100644
>>>> --- a/src/libcamera/camera_manager.cpp
>>>> +++ b/src/libcamera/camera_manager.cpp
>>>> @@ -41,7 +41,7 @@ LOG_DEFINE_CATEGORY(Camera)
>>>>    CameraManager::Private::Private()
>>>>    	: initialized_(false)
>>>>    {
>>>> -	ipaManager_ = std::make_unique<IPAManager>();
>>>> +	ipaManager_ = std::make_unique<IPAManager>(this->configuration());
>>>>    }
>>>>      int CameraManager::Private::start()
>>>> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
>>>> index 068ddd5a5..f7c69890c 100644
>>>> --- a/src/libcamera/global_configuration.cpp
>>>> +++ b/src/libcamera/global_configuration.cpp
>>>> @@ -9,9 +9,11 @@
>>>>      #include <filesystem>
>>>>    #include <memory>
>>>> +#include <optional>
>>>>    #include <string>
>>>>    #include <string_view>
>>>>    #include <sys/types.h>
>>>> +#include <vector>
>>>>      #include <libcamera/base/file.h>
>>>>    #include <libcamera/base/log.h>
>>>> @@ -116,13 +118,22 @@ GlobalConfiguration::GlobalConfiguration()
>>>>     */
>>>>      /**
>>>> + * \fn std::optional<T> GlobalConfiguration::option(const std::string &confPath) const
>>>>     * \brief Return value of the configuration option identified by \a confPath
>>>>     * \param[in] confPath Sequence of the YAML section names (excluding
>>>>     * `configuration') leading to the requested option separated by slashes
>>>>     * \return A value if an item corresponding to \a confPath exists in the
>>>>     * configuration file, no value otherwise
>>>>     */
>>>> -std::optional<std::string> GlobalConfiguration::option(const std::string &confPath) const
>>>> +
>>>> +/**
>>>> + * \brief Return values of the configuration option identified by \a confPath
>>>> + * \tparam T The type of the retrieved configuration value
>>>> + * \param[in] confPath Sequence of the YAML section names (excluding
>>>> + * `configuration') leading to the requested list option, separated by dots
>>>> + * \return A vector of strings (empty one if the option is not found)
>>>> + */
>>>> +std::vector<std::string> GlobalConfiguration::listOption(const std::string &confPath) const
>>>>    {
>>>>    	const YamlObject *c = &configuration();
>>>>    	for (auto part : utils::split(confPath, "/")) {
>>>> @@ -130,7 +141,7 @@ std::optional<std::string> GlobalConfiguration::option(const std::string &confPa
>>>>    		if (!*c)
>>>>    			return {};
>>>>    	}
>>>> -	return c->get<std::string>();
>>>> +	return c->getList<std::string>().value_or(std::vector<std::string>());
>>>>    }
>>>>      /**
>>>> @@ -157,7 +168,37 @@ std::optional<std::string> GlobalConfiguration::envOption(
>>>>    	const char *envValue = utils::secure_getenv(envVariable);
>>>>    	if (envValue)
>>>>    		return std::optional{ std::string{ envValue } };
>>>> -	return option(confPath);
>>>> +	return option<std::string>(confPath);
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Return values of the configuration option from a file or environment
>>>> + * \param[in] envVariable Environment variable to get the value from
>>>> + * \param[in] confPath The same as in GlobalConfiguration::option
>>>> + *
>>>> + * This helper looks first at the given environment variable and if it is
>>>> + * defined (even if it is empty) then it splits its value by semicolons and
>>>> + * returns the resulting list of strings. Otherwise it looks for \a confPath the
>>>> + * same way as in GlobalConfiguration::option, value of which must be a list of
>>>> + * strings.
>>>> + *
>>>> + * \note Support for using environment variables to configure libcamera behavior
>>>> + * is provided here mostly for backward compatibility reasons. Introducing new
>>>> + * configuration environment variables is discouraged.
>>>> + *
>>>> + * \return A vector of strings retrieved from the given environment option or
>>>> + * configuration file (an empty vector is returned if nothing is found)
>>>
>>> I think we want to distinguish "not present" and "empty". I could imagine
>>> that someone might want to inhibit some behaviour by setting the env var
>>> to empty.
>> Well, introducing new environment variables for configuration is
>> discouraged.
>
> I think for some things the convenience for testing/debugging/etc. will
> outweigh the drawbacks and we will decide to add a new env var.
>
>
>> 
>>> `envOption()` also differentiates the two. For example, with the new
>>> "layers" proposal, I could image that we can set the layers in a
>>> configuration file, etc, as well as an environmental variable. In that
>>> case doing `LIBCAMERA_LAYERS= cam ...` might be useful for testing to
>>> temporarily disable all layers.
>> Hmm, sounds reasonable but I'm still not sure we want to encourage this.
>> But technically, I can change the method signature.
>
> Encourage env vars in general, or the use of empty env vars specifically?

Encourage env vars in general.

> In any case, I think it would be nice if `envOption()` and `envListOption`
> were consistent wrt. empty env vars.

OK, let envListOption and listOption return std::optional.

>>>> + */
>>>> +std::vector<std::string> GlobalConfiguration::envListOption(
>>>> +	const char *const envVariable,
>>>> +	const std::string &confPath) const
>>>> +{
>>>> +	const char *envValue = utils::secure_getenv(envVariable);
>>>> +	if (envValue) {
>>>> +		auto items = utils::split(envValue, ":");
>>>> +		return std::vector<std::string>(items.begin(), items.end());
>>>> +	}
>>>> +	return listOption(confPath);
>>>>    }
>>>>      /**
>>>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>>>> index 830750dcc..67c1b23d2 100644
>>>> --- a/src/libcamera/ipa_manager.cpp
>>>> +++ b/src/libcamera/ipa_manager.cpp
>>>> @@ -9,6 +9,7 @@
>>>>      #include <algorithm>
>>>>    #include <dirent.h>
>>>> +#include <numeric>
>>>>    #include <string.h>
>>>>    #include <sys/types.h>
>>>>    @@ -16,6 +17,7 @@
>>>>    #include <libcamera/base/log.h>
>>>>    #include <libcamera/base/utils.h>
>>>>    +#include "libcamera/internal/global_configuration.h"
>>>>    #include "libcamera/internal/ipa_module.h"
>>>>    #include "libcamera/internal/ipa_proxy.h"
>>>>    #include "libcamera/internal/pipeline_handler.h"
>>>> @@ -101,7 +103,7 @@ LOG_DEFINE_CATEGORY(IPAManager)
>>>>     * The IPAManager class is meant to only be instantiated once, by the
>>>>     * CameraManager.
>>>>     */
>>>> -IPAManager::IPAManager()
>>>> +IPAManager::IPAManager(const GlobalConfiguration &configuration)
>>>>    {
>>>>    #if HAVE_IPA_PUBKEY
>>>>    	if (!pubKey_.isValid())
>>>> @@ -111,18 +113,25 @@ IPAManager::IPAManager()
>>>>    	unsigned int ipaCount = 0;
>>>>      	/* User-specified paths take precedence. */
>>>> -	const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
>>>> -	if (modulePaths) {
>>>> -		for (const auto &dir : utils::split(modulePaths, ":")) {
>>>> -			if (dir.empty())
>>>> -				continue;
>>>> +	const auto modulePaths =
>>>> +		configuration.envListOption(
>>>> +			"LIBCAMERA_IPA_MODULE_PATH", "ipa/module_paths");
>>>> +	for (const auto &dir : modulePaths) {
>>>> +		if (dir.empty())
>>>> +			continue;
>>>>    -			ipaCount += addDir(dir.c_str());
>>>> -		}
>>>> +		ipaCount += addDir(dir.c_str());
>>>> +	}
>>>>    -		if (!ipaCount)
>>>> -			LOG(IPAManager, Warning)
>>>> -				<< "No IPA found in '" << modulePaths << "'";
>>>> +	if (!ipaCount) {
>>>> +		std::string paths;
>>>> +		if (!modulePaths.empty()) {
>>>> +			paths = std::accumulate(std::next(modulePaths.begin()),
>>>> +						modulePaths.end(),
>>>> +						modulePaths[0],
>>>> +						[](std::string s1, std::string s2) { return s1 + ":" + s2; });
>>>> +		}
>>>> +		LOG(IPAManager, Warning) << "No IPA found in '" << paths << "'";
>>>
>>>    #include <libcamera/base/utils.h>
>>>    ...
>>>    LOG(IPAManager, Warning) << "No IPA found in '" << utils::join(modulePaths, ":") << "'";
>>>
>>> ?
>> OK.
>> 
>>>>    	}
>>>>      	/*
>>>> @@ -276,11 +285,13 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>>>>     */
>>>>    #endif
>>>>    -bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
>>>> +bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa, const GlobalConfiguration &configuration) const
>>>
>>> Maybe `configuration.option<bool>("ipa/force_isolation").value_or(false)` could be
>>> saved into a member variable in the constructor? Then this won't need
>>> the configuration parameter.
>> Yes.  Assuming the `pipe' CameraManager instance is always the same that
>> was used to create the IPAManager instance.
>
> IpaManager::createIPA() goes from PipelineHandler -> CameraManager -> IPAManager,
> so I am quite certain there is no room for mixup.

OK.

>>>>    {
>>>>    #if HAVE_IPA_PUBKEY
>>>>    	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
>>>> -	if (force && force[0] != '\0') {
>>>> +	if ((force && force[0] != '\0') ||
>>>> +	    (!force && configuration.option<bool>("ipa/force_isolation")
>>>> +			       .value_or(false))) {
>>>>    		LOG(IPAManager, Debug)
>>>>    			<< "Isolation of IPA module " << ipa->path()
>>>>    			<< " forced through environment variable";
>>>> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
>>>> index 9907b9615..77927b0d4 100644
>>>> --- a/src/libcamera/ipa_proxy.cpp
>>>> +++ b/src/libcamera/ipa_proxy.cpp
>>>> @@ -14,6 +14,7 @@
>>>>    #include <libcamera/base/log.h>
>>>>    #include <libcamera/base/utils.h>
>>>>    +#include "libcamera/internal/global_configuration.h"
>>>>    #include "libcamera/internal/ipa_module.h"
>>>>      /**
>>>> @@ -71,6 +72,7 @@ IPAProxy::~IPAProxy()
>>>>    /**
>>>>     * \brief Retrieve the absolute path to an IPA configuration file
>>>>     * \param[in] name The configuration file name
>>>> + * \param[in] configuration The global configuration
>>>>     * \param[in] fallbackName The name of a fallback configuration file
>>>>     *
>>>>     * This function locates the configuration file for an IPA and returns its
>>>> @@ -96,6 +98,7 @@ IPAProxy::~IPAProxy()
>>>>     * no configuration file can be found
>>>>     */
>>>>    std::string IPAProxy::configurationFile(const std::string &name,
>>>> +					const GlobalConfiguration &configuration,
>>>>    					const std::string &fallbackName) const
>>>>    {
>>>>    	/*
>>>> @@ -105,37 +108,37 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>>>    	const std::string ipaName = ipam_->info().name;
>>>>      	/*
>>>> -	 * Start with any user override through the module-specific environment
>>>> -	 * variable. Use the name of the IPA module up to the first '/' to
>>>> -	 * construct the variable name.
>>>> +	 * Start with any user override through the module-specific configuration or
>>>> +	 * environment variable. Use the name of the IPA module up to the first '/'
>>>> +	 * to construct the configuration and variable names.
>>>>    	 */
>>>> -	std::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));
>>>> +	std::string ipaBaseName = ipaName.substr(0, ipaName.find('/'));
>>>> +	std::string ipaConfigName = "ipa/ipas/" + ipaBaseName + "/tuning_file";
>>>
>>> I feel like we're unnecessarily limiting outself wrt. rpi here. Since the IPA
>>> names are "rpi/vc4" and "rpi/pisp", thus `ipaBaseName == "rpi"`. Which means
>>> that it is impossible to specify different tuning files for the different
>>> platforms. I think this is fine for an env var, but for a configuration file
>>> I think this flexibility is expected.
>> Right; so rather than trimming, let's replace the slash with another
>> character here?
>
> I am not sure what the best option is. Replacing `/` with `_` or `-` seems
> like a good candidate. But note that the env var has to keep its name, so
> the trimming will still have to be done.
>
>
>> 
>>> But I don't quite see the point of overriding the tuning file like this from a
>>> configuration file. So maybe leaving just the env var override is sufficient?
>> I think the initial idea was to be able to configure everything in the
>> configuration file (why not?).  Logging is currently a noticeable
>> exception, although hopefully only "temporary".
>
> I think `LIBCAMERA_${IPA}_TUNING_FILE` is mainly a testing feature because
> it overrides the tuning file "globally" for the given IPA. My first impression
> was that it is probably not that useful to have in a configuration file. And
> I still think that, so my preference is not including it in the configuration
> file. With that, feel free to decide, but please address the above rpi naming
> issue if you want to include it.

OK, let's drop this to simplify the things.

> Regards,
> Barnabás Pőcze
>
>
>> 
>>>> +	std::string ipaEnvName = ipaBaseName;
>>>>    	std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),
>>>>    		       [](unsigned char c) { return std::toupper(c); });
>>>>    	ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE";
>>>>    -	char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());
>>>> -	if (configFromEnv && *configFromEnv != '\0')
>>>> -		return { configFromEnv };
>>>> +	auto config = configuration.envOption(ipaEnvName.c_str(), ipaConfigName);
>>>> +	if (config)
>>>> +		return { config.value() };
>>>>      	struct stat statbuf;
>>>>    	int ret;
>>>>      	/*
>>>> -	 * Check the directory pointed to by the IPA config path environment
>>>> -	 * variable next.
>>>> +	 * Check the directory pointed to by the IPA config path next.
>>>>    	 */
>>>> -	const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
>>>> -	if (confPaths) {
>>>> -		for (const auto &dir : utils::split(confPaths, ":")) {
>>>> -			if (dir.empty())
>>>> -				continue;
>>>> -
>>>> -			std::string confPath = dir + "/" + ipaName + "/" + name;
>>>> -			ret = stat(confPath.c_str(), &statbuf);
>>>> -			if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
>>>> -				return confPath;
>>>> -		}
>>>> +	auto confPaths =
>>>> +		configuration.envListOption(
>>>> +			"LIBCAMERA_IPA_CONFIG_PATH", "ipa/config_paths");
>>>> +	for (const auto &dir : confPaths) {
>>>> +		if (dir.empty())
>>>> +			continue;
>>>> +		std::string confPath = dir + "/" + ipaName + "/" + name;
>>>> +		ret = stat(confPath.c_str(), &statbuf);
>>>> +		if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
>>>> +			return confPath;
>>>>    	}
>>>>      	std::string root = utils::libcameraSourcePath();
>>>> @@ -178,12 +181,13 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>>>    		<< "Configuration file '" << name
>>>>    		<< "' not found for IPA module '" << ipaName
>>>>    		<< "', falling back to '" << fallbackName << "'";
>>>> -	return configurationFile(fallbackName);
>>>> +	return configurationFile(fallbackName, configuration);
>>>>    }
>>>>      /**
>>>>     * \brief Find a valid full path for a proxy worker for a given executable name
>>>>     * \param[in] file File name of proxy worker executable
>>>> + * \param[in] configuration The global configuration
>>>>     *
>>>>     * A proxy worker's executable could be found in either the global installation
>>>>     * directory, or in the paths specified by the environment variable
>>>> @@ -195,22 +199,22 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>>>     * \return The full path to the proxy worker executable, or an empty string if
>>>>     * no valid executable path
>>>>     */
>>>> -std::string IPAProxy::resolvePath(const std::string &file) const
>>>> +std::string IPAProxy::resolvePath(const std::string &file, const GlobalConfiguration &configuration) const
>>>>    {
>>>>    	std::string proxyFile = "/" + file;
>>>>    -	/* Check env variable first. */
>>>> -	const char *execPaths = utils::secure_getenv("LIBCAMERA_IPA_PROXY_PATH");
>>>> -	if (execPaths) {
>>>> -		for (const auto &dir : utils::split(execPaths, ":")) {
>>>> -			if (dir.empty())
>>>> -				continue;
>>>> -
>>>> -			std::string proxyPath = dir;
>>>> -			proxyPath += proxyFile;
>>>> -			if (!access(proxyPath.c_str(), X_OK))
>>>> -				return proxyPath;
>>>> -		}
>>>> +	/* Check the configuration first. */
>>>> +	const auto execPaths =
>>>> +		configuration.envListOption(
>>>> +			"LIBCAMERA_IPA_PROXY_PATH", "ipa/proxy_paths");
>>>> +	for (const auto &dir : execPaths) {
>>>> +		if (dir.empty())
>>>> +			continue;
>>>> +
>>>> +		std::string proxyPath = dir;
>>>
>>> std::string proxyPath = dir + proxyFile;
>> OK.

This actually fixes the original code but it's not worth of a separate commit.

>>> Regards,
>>> Barnabás Pőcze
>>>
>>>
>>>> +		proxyPath += proxyFile;
>>>> +		if (!access(proxyPath.c_str(), X_OK))
>>>> +			return proxyPath;
>>>>    	}
>>>>      	/*
>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> index e31e3879d..723f7665b 100644
>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> @@ -1183,8 +1183,9 @@ int IPU3CameraData::loadIPA()
>>>>    	 * The API tuning file is made from the sensor name. If the tuning file
>>>>    	 * isn't found, fall back to the 'uncalibrated' file.
>>>>    	 */
>>>> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>>>    	std::string ipaTuningFile =
>>>> -		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
>>>> +		ipa_->configurationFile(sensor->model() + ".yaml", configuration, "uncalibrated.yaml");
>>>>      	ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
>>>>    			 sensorInfo, sensor->controls(), &ipaControls_);
>>>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>>>> index 4acc091bd..d346fa25f 100644
>>>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>>>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>>>> @@ -388,7 +388,9 @@ int MaliC55CameraData::loadIPA()
>>>>      	ipa_->setSensorControls.connect(this, &MaliC55CameraData::setSensorControls);
>>>>    +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>>>    	std::string ipaTuningFile = ipa_->configurationFile(sensor_->model() + ".yaml",
>>>> +							    configuration,
>>>>    							    "uncalibrated.yaml");
>>>>      	/* We need to inform the IPA of the sensor configuration */
>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> index 675f0a749..aded4cb43 100644
>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> @@ -40,6 +40,7 @@
>>>>    #include "libcamera/internal/delayed_controls.h"
>>>>    #include "libcamera/internal/device_enumerator.h"
>>>>    #include "libcamera/internal/framebuffer.h"
>>>> +#include "libcamera/internal/global_configuration.h"
>>>>    #include "libcamera/internal/ipa_manager.h"
>>>>    #include "libcamera/internal/media_device.h"
>>>>    #include "libcamera/internal/media_pipeline.h"
>>>> @@ -383,8 +384,9 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>>>>    	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
>>>>      	/* The IPA tuning file is made from the sensor name. */
>>>> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>>>    	std::string ipaTuningFile =
>>>> -		ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
>>>> +		ipa_->configurationFile(sensor_->model() + ".yaml", configuration, "uncalibrated.yaml");
>>>>      	IPACameraSensorInfo sensorInfo{};
>>>>    	int ret = sensor_->sensorInfo(&sensorInfo);
>>>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>>>> index a316ef297..3da919ee7 100644
>>>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>>>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>>>> @@ -1157,7 +1157,8 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
>>>>    	std::string model = sensor_->model();
>>>>    	if (isMonoSensor(sensor_))
>>>>    		model += "_mono";
>>>> -	std::string configurationFile = ipa_->configurationFile(model + ".json");
>>>> +	const GlobalConfiguration &configuration = pipe()->cameraManager()->_d()->configuration();
>>>> +	std::string configurationFile = ipa_->configurationFile(model + ".json", configuration);
>>>>      	IPASettings settings(configurationFile, sensor_->model());
>>>>    	ipa::RPi::InitParams params;
>>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>>>> index 07273bd2b..555d51bb6 100644
>>>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>>>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>>>> @@ -494,7 +494,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>>>>      	data->ipa_->paramsComputed.connect(data.get(), &VimcCameraData::paramsComputed);
>>>>    -	std::string conf = data->ipa_->configurationFile("vimc.conf");
>>>> +	const GlobalConfiguration &configuration = manager_->_d()->configuration();
>>>> +	std::string conf = data->ipa_->configurationFile("vimc.conf", configuration);
>>>>    	Flags<ipa::vimc::TestFlag> inFlags = ipa::vimc::TestFlag::Flag2;
>>>>    	Flags<ipa::vimc::TestFlag> outFlags;
>>>>    	data->ipa_->init(IPASettings{ conf, data->sensor_->model() },
>>>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>>>> index 28e2a360e..3ce354111 100644
>>>> --- a/src/libcamera/software_isp/software_isp.cpp
>>>> +++ b/src/libcamera/software_isp/software_isp.cpp
>>>> @@ -21,6 +21,7 @@
>>>>    #include <libcamera/stream.h>
>>>>      #include "libcamera/internal/framebuffer.h"
>>>> +#include "libcamera/internal/global_configuration.h"
>>>>    #include "libcamera/internal/ipa_manager.h"
>>>>    #include "libcamera/internal/software_isp/debayer_params.h"
>>>>    @@ -130,8 +131,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
>>>>    	 * The API tuning file is made from the sensor name. If the tuning file
>>>>    	 * isn't found, fall back to the 'uncalibrated' file.
>>>>    	 */
>>>> +	const GlobalConfiguration &configuration = pipe->cameraManager()->_d()->configuration();
>>>>    	std::string ipaTuningFile =
>>>> -		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
>>>> +		ipa_->configurationFile(sensor->model() + ".yaml", configuration, "uncalibrated.yaml");
>>>>      	IPACameraSensorInfo sensorInfo{};
>>>>    	int ret = sensor->sensorInfo(&sensorInfo);
>>>> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
>>>> index b81783664..1afcbf106 100644
>>>> --- a/test/ipa/ipa_interface_test.cpp
>>>> +++ b/test/ipa/ipa_interface_test.cpp
>>>> @@ -106,7 +106,8 @@ protected:
>>>>    		}
>>>>      		/* Test initialization of IPA module. */
>>>> -		std::string conf = ipa_->configurationFile("vimc.conf");
>>>> +		const GlobalConfiguration configuration;
>>>> +		std::string conf = ipa_->configurationFile("vimc.conf", configuration);
>>>>    		Flags<ipa::vimc::TestFlag> inFlags;
>>>>    		Flags<ipa::vimc::TestFlag> outFlags;
>>>>    		int ret = ipa_->init(IPASettings{ conf, "vimc" },
>>>> 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
>>>> index 9a3aadbd2..71e920eb1 100644
>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>>>> @@ -45,7 +45,7 @@ namespace {{ns}} {
>>>>    {% endfor %}
>>>>    {%- endif %}
>>>>    -{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
>>>> +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration)
>>>>    	: IPAProxy(ipam), isolate_(isolate),
>>>>    	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
>>>>    {
>>>> @@ -54,7 +54,7 @@ namespace {{ns}} {
>>>>    		<< ipam->path();
>>>>      	if (isolate_) {
>>>> -		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy");
>>>> +		const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy", configuration);
>>>>    		if (proxyWorkerPath.empty()) {
>>>>    			LOG(IPAProxy, Error)
>>>>    				<< "Failed to get proxy worker path";
>>>> 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
>>>> index a0312a7c1..057c3ab03 100644
>>>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
>>>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
>>>> @@ -37,7 +37,7 @@ namespace {{ns}} {
>>>>    class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
>>>>    {
>>>>    public:
>>>> -	{{proxy_name}}(IPAModule *ipam, bool isolate);
>>>> +	{{proxy_name}}(IPAModule *ipam, bool isolate, const GlobalConfiguration &configuration);
>>>>    	~{{proxy_name}}();
>>>>      {% for method in interface_main.methods %}
>>