[v11,04/12] config: Look up rpi configuration in the configuration file
diff mbox series

Message ID 20250624083612.27230-5-mzamazal@redhat.com
State New
Headers show
Series
  • Add global configuration file
Related show

Commit Message

Milan Zamazal June 24, 2025, 8:36 a.m. UTC 
Let's make rpi configuration available in the global configuration file.
It may be arguable whether pipeline specific configurations belong to
the global configuration file.  But:

- Having a single configuration file is generally easier for the user.
- The original configuration via environment variables can be already
  considered global.
- This option points to other configuration files and it makes little
  sense to add another configuration file to the chain.

The rpi configuration is currently placed in a separate file, specified
by LIBCAMERA_RPI_CONFIG_FILE environment variable.  Let's support the
content of the file in the global configuration file.  `version' field
is omitted as the global configuration file has its own version and the
required version in the original file has been 1.0 anyway.

The configuration snippet:

  configuration:
    pipeline:
      rpi:
        target: TARGET
        pipeline_handler:
          ...

To not break current user setups, LIBCAMERA_RPI_CONFIG_FILE environment
variable is still supported and works as before, pointing to a separate
configuration file.  Just the duplicate check for the configuration file
version in platformPipelineConfigure methods is removed.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../pipeline/rpi/common/pipeline_base.cpp     | 60 +++++++++++--------
 .../pipeline/rpi/common/pipeline_base.h       |  2 +-
 src/libcamera/pipeline/rpi/pisp/pisp.cpp      | 14 +----
 src/libcamera/pipeline/rpi/vc4/vc4.cpp        | 14 +----
 4 files changed, 42 insertions(+), 48 deletions(-)

Comments

Barnabás Pőcze June 27, 2025, 2 p.m. UTC | #1 
Hi

2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:
> Let's make rpi configuration available in the global configuration file.
> It may be arguable whether pipeline specific configurations belong to
> the global configuration file.  But:
> 
> - Having a single configuration file is generally easier for the user.
> - The original configuration via environment variables can be already
>    considered global.
> - This option points to other configuration files and it makes little
>    sense to add another configuration file to the chain.
> 
> The rpi configuration is currently placed in a separate file, specified
> by LIBCAMERA_RPI_CONFIG_FILE environment variable.  Let's support the
> content of the file in the global configuration file.  `version' field
> is omitted as the global configuration file has its own version and the
> required version in the original file has been 1.0 anyway.
> 
> The configuration snippet:
> 
>    configuration:
>      pipeline:
>        rpi:
>          target: TARGET
>          pipeline_handler:

This seems a bit limiting to me. The same configuration cannot be used
for vc4 and pisp. I feel like something like

   rpi:
     bcm2835:
       ...
     pisp:
       ...

would be better. I am not sure how the two could be best reconciled.
Maybe a `platformName` variable/function in the case class that specifies
a string_view/const char * describing the target. It could then be used
to select the right object in the dictionary and/or to validate the target
in $LIBCAMERA_RPI_CONFIG_FILE.


>            ...
> 
> To not break current user setups, LIBCAMERA_RPI_CONFIG_FILE environment
> variable is still supported and works as before, pointing to a separate
> configuration file.  Just the duplicate check for the configuration file
> version in platformPipelineConfigure methods is removed.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   .../pipeline/rpi/common/pipeline_base.cpp     | 60 +++++++++++--------
>   .../pipeline/rpi/common/pipeline_base.h       |  2 +-
>   src/libcamera/pipeline/rpi/pisp/pisp.cpp      | 14 +----
>   src/libcamera/pipeline/rpi/vc4/vc4.cpp        | 14 +----
>   4 files changed, 42 insertions(+), 48 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index e14d3b913..a316ef297 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -20,8 +20,10 @@
>   #include <libcamera/property_ids.h>
>   
>   #include "libcamera/internal/camera_lens.h"
> +#include "libcamera/internal/global_configuration.h"
>   #include "libcamera/internal/ipa_manager.h"
>   #include "libcamera/internal/v4l2_subdevice.h"
> +#include "libcamera/internal/yaml_parser.h"
>   
>   using namespace std::chrono_literals;
>   
> @@ -1085,37 +1087,42 @@ int CameraData::loadPipelineConfiguration()
>   	};
>   
>   	/* Initial configuration of the platform, in case no config file is present */
> -	platformPipelineConfigure({});
> +	std::optional<std::string> empty;
> +	platformPipelineConfigure({}, empty);

Why does this not take `const std::optional<std::string>&`?
Then you could just do `platformPipelineConfigure({}, {})`.


Regards,
Barnabás Pőcze


>   
> +	std::unique_ptr<YamlObject> root;
>   	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> -	if (!configFromEnv || *configFromEnv == '\0')
> -		return 0;
> -
> -	std::string filename = std::string(configFromEnv);
> -	File file(filename);
> -
> -	if (!file.open(File::OpenModeFlag::ReadOnly)) {
> -		LOG(RPI, Warning) << "Failed to open configuration file '" << filename << "'"
> -				  << ", using defaults";
> -		return 0;
> -	}
> +	if (configFromEnv && *configFromEnv != '\0') {
> +		std::string filename = std::string(configFromEnv);
> +		File file(filename);
> +
> +		if (!file.open(File::OpenModeFlag::ReadOnly)) {
> +			LOG(RPI, Warning) << "Failed to open configuration file '" << filename << "'"
> +					  << ", using defaults";
> +			return 0;
> +		}
>   
> -	LOG(RPI, Info) << "Using configuration file '" << filename << "'";
> +		LOG(RPI, Info) << "Using configuration file '" << filename << "'";
>   
> -	std::unique_ptr<YamlObject> root = YamlParser::parse(file);
> -	if (!root) {
> -		LOG(RPI, Warning) << "Failed to parse configuration file, using defaults";
> -		return 0;
> -	}
> +		root = YamlParser::parse(file);
> +		if (!root) {
> +			LOG(RPI, Warning) << "Failed to parse configuration file, using defaults";
> +			return 0;
> +		}
>   
> -	std::optional<double> ver = (*root)["version"].get<double>();
> -	if (!ver || *ver != 1.0) {
> -		LOG(RPI, Warning) << "Unexpected configuration file version reported: "
> -				  << *ver;
> -		return 0;
> +		std::optional<double> ver = (*root)["version"].get<double>();
> +		if (!ver || *ver != 1.0) {
> +			LOG(RPI, Warning) << "Unexpected configuration file version reported: "
> +					  << *ver;
> +			return 0;
> +		}
>   	}
>   
> -	const YamlObject &phConfig = (*root)["pipeline_handler"];
> +	GlobalConfiguration::Configuration config =
> +		pipe()->cameraManager()->_d()->configuration().configuration()["pipeline"]["rpi"];
> +	const YamlObject &phConfig = (root
> +					      ? (*root)["pipeline_handler"]
> +					      : config["pipeline_handler"]);
>   
>   	if (phConfig.contains("disable_startup_frame_drops"))
>   		LOG(RPI, Warning)
> @@ -1131,7 +1138,10 @@ int CameraData::loadPipelineConfiguration()
>   		frontendDevice()->setDequeueTimeout(config_.cameraTimeoutValue * 1ms);
>   	}
>   
> -	return platformPipelineConfigure(root);
> +	std::optional<std::string> target = (root
> +						     ? (*root)["target"].get<std::string>()
> +						     : config.get<std::string>("target"));
> +	return platformPipelineConfigure(phConfig, target);
>   }
>   
>   int CameraData::loadIPA(ipa::RPi::InitResult *result)
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index 4c5743e04..34684d882 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -96,7 +96,7 @@ public:
>   	virtual V4L2VideoDevice::Formats rawFormats() const = 0;
>   	virtual V4L2VideoDevice *frontendDevice() = 0;
>   
> -	virtual int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) = 0;
> +	virtual int platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target) = 0;
>   
>   	std::unique_ptr<ipa::RPi::IPAProxyRPi> ipa_;
>   
> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> index 2df91bacf..54d885c8a 100644
> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> @@ -743,7 +743,7 @@ public:
>   	CameraConfiguration::Status
>   	platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override;
>   
> -	int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override;
> +	int platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target) override;
>   
>   	void platformStart() override;
>   	void platformStop() override;
> @@ -1331,7 +1331,7 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
>   	return status;
>   }
>   
> -int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root)
> +int PiSPCameraData::platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target)
>   {
>   	config_ = {
>   		.numCfeConfigStatsBuffers = 12,
> @@ -1340,23 +1340,15 @@ int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject>
>   		.disableHdr = false,
>   	};
>   
> -	if (!root)
> +	if (!phConfig)
>   		return 0;
>   
> -	std::optional<double> ver = (*root)["version"].get<double>();
> -	if (!ver || *ver != 1.0) {
> -		LOG(RPI, Error) << "Unexpected configuration file version reported";
> -		return -EINVAL;
> -	}
> -
> -	std::optional<std::string> target = (*root)["target"].get<std::string>();
>   	if (target != "pisp") {
>   		LOG(RPI, Error) << "Unexpected target reported: expected \"pisp\", got "
>   				<< (target ? target->c_str() : "(unknown)");
>   		return -EINVAL;
>   	}
>   
> -	const YamlObject &phConfig = (*root)["pipeline_handler"];
>   	config_.numCfeConfigStatsBuffers =
>   		phConfig["num_cfe_config_stats_buffers"].get<unsigned int>(config_.numCfeConfigStatsBuffers);
>   	config_.numCfeConfigQueue =
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index e99a7edf8..714110c7e 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -67,7 +67,7 @@ public:
>   
>   	CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override;
>   
> -	int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override;
> +	int platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target) override;
>   
>   	void platformStart() override;
>   	void platformStop() override;
> @@ -493,30 +493,22 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfig
>   	return status;
>   }
>   
> -int Vc4CameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root)
> +int Vc4CameraData::platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target)
>   {
>   	config_ = {
>   		.minUnicamBuffers = 2,
>   		.minTotalUnicamBuffers = 4,
>   	};
>   
> -	if (!root)
> +	if (!phConfig)
>   		return 0;
>   
> -	std::optional<double> ver = (*root)["version"].get<double>();
> -	if (!ver || *ver != 1.0) {
> -		LOG(RPI, Error) << "Unexpected configuration file version reported";
> -		return -EINVAL;
> -	}
> -
> -	std::optional<std::string> target = (*root)["target"].get<std::string>();
>   	if (target != "bcm2835") {
>   		LOG(RPI, Error) << "Unexpected target reported: expected \"bcm2835\", got "
>   				<< (target ? target->c_str() : "(unknown)");
>   		return -EINVAL;
>   	}
>   
> -	const YamlObject &phConfig = (*root)["pipeline_handler"];
>   	config_.minUnicamBuffers =
>   		phConfig["min_unicam_buffers"].get<unsigned int>(config_.minUnicamBuffers);
>   	config_.minTotalUnicamBuffers =
Milan Zamazal June 27, 2025, 7:34 p.m. UTC | #2 
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:
>> Let's make rpi configuration available in the global configuration file.
>> It may be arguable whether pipeline specific configurations belong to
>> the global configuration file.  But:
>> - Having a single configuration file is generally easier for the user.
>> - The original configuration via environment variables can be already
>>    considered global.
>> - This option points to other configuration files and it makes little
>>    sense to add another configuration file to the chain.
>> The rpi configuration is currently placed in a separate file, specified
>> by LIBCAMERA_RPI_CONFIG_FILE environment variable.  Let's support the
>> content of the file in the global configuration file.  `version' field
>> is omitted as the global configuration file has its own version and the
>> required version in the original file has been 1.0 anyway.
>> The configuration snippet:
>>    configuration:
>>      pipeline:
>>        rpi:
>>          target: TARGET
>>          pipeline_handler:
>
> This seems a bit limiting to me. The same configuration cannot be used
> for vc4 and pisp. I feel like something like
>
>   rpi:
>     bcm2835:
>       ...
>     pisp:
>       ...
>
> would be better. I am not sure how the two could be best reconciled.
> Maybe a `platformName` variable/function in the case class that specifies
> a string_view/const char * describing the target. It could then be used
> to select the right object in the dictionary and/or to validate the target
> in $LIBCAMERA_RPI_CONFIG_FILE.

I'm not sure either, which is why I kept it simple in the first version.
I'd like to hear opinion from the rpi people here before messing with
the implementation, what structure of the YAML configuration they
prefer.

>>            ...
>> To not break current user setups, LIBCAMERA_RPI_CONFIG_FILE environment
>> variable is still supported and works as before, pointing to a separate
>> configuration file.  Just the duplicate check for the configuration file
>> version in platformPipelineConfigure methods is removed.
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   .../pipeline/rpi/common/pipeline_base.cpp     | 60 +++++++++++--------
>>   .../pipeline/rpi/common/pipeline_base.h       |  2 +-
>>   src/libcamera/pipeline/rpi/pisp/pisp.cpp      | 14 +----
>>   src/libcamera/pipeline/rpi/vc4/vc4.cpp        | 14 +----
>>   4 files changed, 42 insertions(+), 48 deletions(-)
>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> index e14d3b913..a316ef297 100644
>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> @@ -20,8 +20,10 @@
>>   #include <libcamera/property_ids.h>
>>     #include "libcamera/internal/camera_lens.h"
>> +#include "libcamera/internal/global_configuration.h"
>>   #include "libcamera/internal/ipa_manager.h"
>>   #include "libcamera/internal/v4l2_subdevice.h"
>> +#include "libcamera/internal/yaml_parser.h"
>>     using namespace std::chrono_literals;
>>   @@ -1085,37 +1087,42 @@ int CameraData::loadPipelineConfiguration()
>>   	};
>>     	/* Initial configuration of the platform, in case no config file is present */
>> -	platformPipelineConfigure({});
>> +	std::optional<std::string> empty;
>> +	platformPipelineConfigure({}, empty);
>
> Why does this not take `const std::optional<std::string>&`?
> Then you could just do `platformPipelineConfigure({}, {})`.

Ah, right, thanks.

> Regards,
> Barnabás Pőcze
>
>
>>   +	std::unique_ptr<YamlObject> root;
>>   	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
>> -	if (!configFromEnv || *configFromEnv == '\0')
>> -		return 0;
>> -
>> -	std::string filename = std::string(configFromEnv);
>> -	File file(filename);
>> -
>> -	if (!file.open(File::OpenModeFlag::ReadOnly)) {
>> -		LOG(RPI, Warning) << "Failed to open configuration file '" << filename << "'"
>> -				  << ", using defaults";
>> -		return 0;
>> -	}
>> +	if (configFromEnv && *configFromEnv != '\0') {
>> +		std::string filename = std::string(configFromEnv);
>> +		File file(filename);
>> +
>> +		if (!file.open(File::OpenModeFlag::ReadOnly)) {
>> +			LOG(RPI, Warning) << "Failed to open configuration file '" << filename << "'"
>> +					  << ", using defaults";
>> +			return 0;
>> +		}
>>   -	LOG(RPI, Info) << "Using configuration file '" << filename << "'";
>> +		LOG(RPI, Info) << "Using configuration file '" << filename << "'";
>>   -	std::unique_ptr<YamlObject> root = YamlParser::parse(file);
>> -	if (!root) {
>> -		LOG(RPI, Warning) << "Failed to parse configuration file, using defaults";
>> -		return 0;
>> -	}
>> +		root = YamlParser::parse(file);
>> +		if (!root) {
>> +			LOG(RPI, Warning) << "Failed to parse configuration file, using defaults";
>> +			return 0;
>> +		}
>>   -	std::optional<double> ver = (*root)["version"].get<double>();
>> -	if (!ver || *ver != 1.0) {
>> -		LOG(RPI, Warning) << "Unexpected configuration file version reported: "
>> -				  << *ver;
>> -		return 0;
>> +		std::optional<double> ver = (*root)["version"].get<double>();
>> +		if (!ver || *ver != 1.0) {
>> +			LOG(RPI, Warning) << "Unexpected configuration file version reported: "
>> +					  << *ver;
>> +			return 0;
>> +		}
>>   	}
>>   -	const YamlObject &phConfig = (*root)["pipeline_handler"];
>> +	GlobalConfiguration::Configuration config =
>> +		pipe()->cameraManager()->_d()->configuration().configuration()["pipeline"]["rpi"];
>> +	const YamlObject &phConfig = (root
>> +					      ? (*root)["pipeline_handler"]
>> +					      : config["pipeline_handler"]);
>>     	if (phConfig.contains("disable_startup_frame_drops"))
>>   		LOG(RPI, Warning)
>> @@ -1131,7 +1138,10 @@ int CameraData::loadPipelineConfiguration()
>>   		frontendDevice()->setDequeueTimeout(config_.cameraTimeoutValue * 1ms);
>>   	}
>>   -	return platformPipelineConfigure(root);
>> +	std::optional<std::string> target = (root
>> +						     ? (*root)["target"].get<std::string>()
>> +						     : config.get<std::string>("target"));
>> +	return platformPipelineConfigure(phConfig, target);
>>   }
>>     int CameraData::loadIPA(ipa::RPi::InitResult *result)
>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
>> index 4c5743e04..34684d882 100644
>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
>> @@ -96,7 +96,7 @@ public:
>>   	virtual V4L2VideoDevice::Formats rawFormats() const = 0;
>>   	virtual V4L2VideoDevice *frontendDevice() = 0;
>>   -	virtual int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) = 0;
>> +	virtual int platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target) = 0;
>>     	std::unique_ptr<ipa::RPi::IPAProxyRPi> ipa_;
>>   diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
>> index 2df91bacf..54d885c8a 100644
>> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
>> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
>> @@ -743,7 +743,7 @@ public:
>>   	CameraConfiguration::Status
>>   	platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override;
>>   -	int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override;
>> +	int platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target) override;
>>     	void platformStart() override;
>>   	void platformStop() override;
>> @@ -1331,7 +1331,7 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
>>   	return status;
>>   }
>>   -int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root)
>> +int PiSPCameraData::platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target)
>>   {
>>   	config_ = {
>>   		.numCfeConfigStatsBuffers = 12,
>> @@ -1340,23 +1340,15 @@ int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject>
>>   		.disableHdr = false,
>>   	};
>>   -	if (!root)
>> +	if (!phConfig)
>>   		return 0;
>>   -	std::optional<double> ver = (*root)["version"].get<double>();
>> -	if (!ver || *ver != 1.0) {
>> -		LOG(RPI, Error) << "Unexpected configuration file version reported";
>> -		return -EINVAL;
>> -	}
>> -
>> -	std::optional<std::string> target = (*root)["target"].get<std::string>();
>>   	if (target != "pisp") {
>>   		LOG(RPI, Error) << "Unexpected target reported: expected \"pisp\", got "
>>   				<< (target ? target->c_str() : "(unknown)");
>>   		return -EINVAL;
>>   	}
>>   -	const YamlObject &phConfig = (*root)["pipeline_handler"];
>>   	config_.numCfeConfigStatsBuffers =
>>   		phConfig["num_cfe_config_stats_buffers"].get<unsigned int>(config_.numCfeConfigStatsBuffers);
>>   	config_.numCfeConfigQueue =
>> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
>> index e99a7edf8..714110c7e 100644
>> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
>> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
>> @@ -67,7 +67,7 @@ public:
>>     	CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
>> override;
>>   -	int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override;
>> +	int platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target) override;
>>     	void platformStart() override;
>>   	void platformStop() override;
>> @@ -493,30 +493,22 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfig
>>   	return status;
>>   }
>>   -int Vc4CameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root)
>> +int Vc4CameraData::platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target)
>>   {
>>   	config_ = {
>>   		.minUnicamBuffers = 2,
>>   		.minTotalUnicamBuffers = 4,
>>   	};
>>   -	if (!root)
>> +	if (!phConfig)
>>   		return 0;
>>   -	std::optional<double> ver = (*root)["version"].get<double>();
>> -	if (!ver || *ver != 1.0) {
>> -		LOG(RPI, Error) << "Unexpected configuration file version reported";
>> -		return -EINVAL;
>> -	}
>> -
>> -	std::optional<std::string> target = (*root)["target"].get<std::string>();
>>   	if (target != "bcm2835") {
>>   		LOG(RPI, Error) << "Unexpected target reported: expected \"bcm2835\", got "
>>   				<< (target ? target->c_str() : "(unknown)");
>>   		return -EINVAL;
>>   	}
>>   -	const YamlObject &phConfig = (*root)["pipeline_handler"];
>>   	config_.minUnicamBuffers =
>>   		phConfig["min_unicam_buffers"].get<unsigned int>(config_.minUnicamBuffers);
>>   	config_.minTotalUnicamBuffers =

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index e14d3b913..a316ef297 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -20,8 +20,10 @@ 
 #include <libcamera/property_ids.h>
 
 #include "libcamera/internal/camera_lens.h"
+#include "libcamera/internal/global_configuration.h"
 #include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/v4l2_subdevice.h"
+#include "libcamera/internal/yaml_parser.h"
 
 using namespace std::chrono_literals;
 
@@ -1085,37 +1087,42 @@  int CameraData::loadPipelineConfiguration()
 	};
 
 	/* Initial configuration of the platform, in case no config file is present */
-	platformPipelineConfigure({});
+	std::optional<std::string> empty;
+	platformPipelineConfigure({}, empty);
 
+	std::unique_ptr<YamlObject> root;
 	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
-	if (!configFromEnv || *configFromEnv == '\0')
-		return 0;
-
-	std::string filename = std::string(configFromEnv);
-	File file(filename);
-
-	if (!file.open(File::OpenModeFlag::ReadOnly)) {
-		LOG(RPI, Warning) << "Failed to open configuration file '" << filename << "'"
-				  << ", using defaults";
-		return 0;
-	}
+	if (configFromEnv && *configFromEnv != '\0') {
+		std::string filename = std::string(configFromEnv);
+		File file(filename);
+
+		if (!file.open(File::OpenModeFlag::ReadOnly)) {
+			LOG(RPI, Warning) << "Failed to open configuration file '" << filename << "'"
+					  << ", using defaults";
+			return 0;
+		}
 
-	LOG(RPI, Info) << "Using configuration file '" << filename << "'";
+		LOG(RPI, Info) << "Using configuration file '" << filename << "'";
 
-	std::unique_ptr<YamlObject> root = YamlParser::parse(file);
-	if (!root) {
-		LOG(RPI, Warning) << "Failed to parse configuration file, using defaults";
-		return 0;
-	}
+		root = YamlParser::parse(file);
+		if (!root) {
+			LOG(RPI, Warning) << "Failed to parse configuration file, using defaults";
+			return 0;
+		}
 
-	std::optional<double> ver = (*root)["version"].get<double>();
-	if (!ver || *ver != 1.0) {
-		LOG(RPI, Warning) << "Unexpected configuration file version reported: "
-				  << *ver;
-		return 0;
+		std::optional<double> ver = (*root)["version"].get<double>();
+		if (!ver || *ver != 1.0) {
+			LOG(RPI, Warning) << "Unexpected configuration file version reported: "
+					  << *ver;
+			return 0;
+		}
 	}
 
-	const YamlObject &phConfig = (*root)["pipeline_handler"];
+	GlobalConfiguration::Configuration config =
+		pipe()->cameraManager()->_d()->configuration().configuration()["pipeline"]["rpi"];
+	const YamlObject &phConfig = (root
+					      ? (*root)["pipeline_handler"]
+					      : config["pipeline_handler"]);
 
 	if (phConfig.contains("disable_startup_frame_drops"))
 		LOG(RPI, Warning)
@@ -1131,7 +1138,10 @@  int CameraData::loadPipelineConfiguration()
 		frontendDevice()->setDequeueTimeout(config_.cameraTimeoutValue * 1ms);
 	}
 
-	return platformPipelineConfigure(root);
+	std::optional<std::string> target = (root
+						     ? (*root)["target"].get<std::string>()
+						     : config.get<std::string>("target"));
+	return platformPipelineConfigure(phConfig, target);
 }
 
 int CameraData::loadIPA(ipa::RPi::InitResult *result)
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
index 4c5743e04..34684d882 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
@@ -96,7 +96,7 @@  public:
 	virtual V4L2VideoDevice::Formats rawFormats() const = 0;
 	virtual V4L2VideoDevice *frontendDevice() = 0;
 
-	virtual int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) = 0;
+	virtual int platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target) = 0;
 
 	std::unique_ptr<ipa::RPi::IPAProxyRPi> ipa_;
 
diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
index 2df91bacf..54d885c8a 100644
--- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
+++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
@@ -743,7 +743,7 @@  public:
 	CameraConfiguration::Status
 	platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override;
 
-	int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override;
+	int platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target) override;
 
 	void platformStart() override;
 	void platformStop() override;
@@ -1331,7 +1331,7 @@  PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const
 	return status;
 }
 
-int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root)
+int PiSPCameraData::platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target)
 {
 	config_ = {
 		.numCfeConfigStatsBuffers = 12,
@@ -1340,23 +1340,15 @@  int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject>
 		.disableHdr = false,
 	};
 
-	if (!root)
+	if (!phConfig)
 		return 0;
 
-	std::optional<double> ver = (*root)["version"].get<double>();
-	if (!ver || *ver != 1.0) {
-		LOG(RPI, Error) << "Unexpected configuration file version reported";
-		return -EINVAL;
-	}
-
-	std::optional<std::string> target = (*root)["target"].get<std::string>();
 	if (target != "pisp") {
 		LOG(RPI, Error) << "Unexpected target reported: expected \"pisp\", got "
 				<< (target ? target->c_str() : "(unknown)");
 		return -EINVAL;
 	}
 
-	const YamlObject &phConfig = (*root)["pipeline_handler"];
 	config_.numCfeConfigStatsBuffers =
 		phConfig["num_cfe_config_stats_buffers"].get<unsigned int>(config_.numCfeConfigStatsBuffers);
 	config_.numCfeConfigQueue =
diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
index e99a7edf8..714110c7e 100644
--- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
+++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
@@ -67,7 +67,7 @@  public:
 
 	CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override;
 
-	int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override;
+	int platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target) override;
 
 	void platformStart() override;
 	void platformStop() override;
@@ -493,30 +493,22 @@  CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfig
 	return status;
 }
 
-int Vc4CameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root)
+int Vc4CameraData::platformPipelineConfigure(const YamlObject &phConfig, std::optional<std::string> &target)
 {
 	config_ = {
 		.minUnicamBuffers = 2,
 		.minTotalUnicamBuffers = 4,
 	};
 
-	if (!root)
+	if (!phConfig)
 		return 0;
 
-	std::optional<double> ver = (*root)["version"].get<double>();
-	if (!ver || *ver != 1.0) {
-		LOG(RPI, Error) << "Unexpected configuration file version reported";
-		return -EINVAL;
-	}
-
-	std::optional<std::string> target = (*root)["target"].get<std::string>();
 	if (target != "bcm2835") {
 		LOG(RPI, Error) << "Unexpected target reported: expected \"bcm2835\", got "
 				<< (target ? target->c_str() : "(unknown)");
 		return -EINVAL;
 	}
 
-	const YamlObject &phConfig = (*root)["pipeline_handler"];
 	config_.minUnicamBuffers =
 		phConfig["min_unicam_buffers"].get<unsigned int>(config_.minUnicamBuffers);
 	config_.minTotalUnicamBuffers =