| Message ID | 20250624083612.27230-5-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 =
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 =
Hi Milan and Barnabás, On Fri, 27 Jun 2025 at 20:35, Milan Zamazal <mzamazal@redhat.com> wrote: > > 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. We definitely want to have different settings for bcm2835 and pisp. I like the structure Barnabás proposed above. I should (hopefully!) be relatively simple to get a target string for the target through the pipeline handler. Sorry this is more work than originally expected. Regards, Naush > > >> ... > >> 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 = >
Hi Naush, Naushir Patuck <naush@raspberrypi.com> writes: > Hi Milan and Barnabás, > > > On Fri, 27 Jun 2025 at 20:35, Milan Zamazal <mzamazal@redhat.com> wrote: >> >> 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. > > We definitely want to have different settings for bcm2835 and pisp. I > like the structure Barnabás proposed above. OK, thank you for clarification. > I should (hopefully!) be relatively simple to get a target string for > the target through the pipeline handler. Yes, it is. > Sorry this is more work than originally expected. No problem, it's not a complicated change. > Regards, > Naush > >> >> >> ... >> >> 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 = >>
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 =
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(-)