Message ID | 20250729073201.5369-5-mzamazal@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Milan, Thanks for the patch. Quoting Milan Zamazal (2025-07-29 16:31:52) > 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: > pipelines: > rpi: > bcm2835: > pipeline_handler: > ... > pisp: > 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> The commit title should be s/config:/pipeline: rpi:/ , as this touches rpi code and not global_configuration code. > --- > .../pipeline/rpi/common/pipeline_base.cpp | 59 +++++++++++-------- > .../pipeline/rpi/common/pipeline_base.h | 3 +- > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 26 +++----- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 26 +++----- > 4 files changed, 56 insertions(+), 58 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 563df198e..1effa85cd 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; > > @@ -1091,35 +1093,46 @@ int CameraData::loadPipelineConfiguration() > /* Initial configuration of the platform, in case no config file is present */ > platformPipelineConfigure({}); > > + 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 (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; > + } > > - 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 << "'"; > + root = YamlParser::parse(file); > + if (!root) { > + LOG(RPI, Warning) << "Failed to parse configuration file, using defaults"; > + return 0; > + } > > - std::unique_ptr<YamlObject> 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; > + std::optional<std::string> t = (*root)["target"].get<std::string>(); > + if (t != target()) { > + LOG(RPI, Error) << "Unexpected target reported: expected \"" > + << target() << "\", got " << (t ? t->c_str() : "(unknown)"); > + return -EINVAL; > + } > } > > - const YamlObject &phConfig = (*root)["pipeline_handler"]; > + GlobalConfiguration::Configuration config = > + pipe()->cameraManager()->_d()->configuration().configuration()["pipelines"]["rpi"]; Don't we need to check if configuration()["pipelines"] and configuration()["pipelines"]["rpi"] exist? > + const YamlObject &phConfig = (root > + ? (*root)["pipeline_handler"] > + : config[target()]["pipeline_handler"]); Same here, for config[target()] and config[target()]["pipeline_handler"] ? It looks like the raspberry pi config takes precedence. I suppose that's fine, since it will be removed. Maybe we should add a warning message that it is deprecated and should be removed or moved to the main configuration file. The rest looks good. Thanks, Paul > > if (phConfig.contains("disable_startup_frame_drops")) > LOG(RPI, Warning) > @@ -1135,7 +1148,7 @@ int CameraData::loadPipelineConfiguration() > frontendDevice()->setDequeueTimeout(config_.cameraTimeoutValue * 1ms); > } > > - return platformPipelineConfigure(root); > + return platformPipelineConfigure(phConfig); > } > > 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 4bce4ec4f..ffbede798 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h > @@ -95,8 +95,9 @@ public: > virtual V4L2VideoDevice::Formats ispFormats() const = 0; > virtual V4L2VideoDevice::Formats rawFormats() const = 0; > virtual V4L2VideoDevice *frontendDevice() = 0; > + virtual const std::string &target() const = 0; > > - virtual int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) = 0; > + virtual int platformPipelineConfigure(const YamlObject &phConfig) = 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 082724c5a..0fb7c3fe8 100644 > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > @@ -740,10 +740,16 @@ public: > return cfe_[Cfe::Output0].dev(); > } > > + const std::string &target() const override > + { > + static const std::string target = "pisp"; > + return target; > + } > + > CameraConfiguration::Status > platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override; > > - int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override; > + int platformPipelineConfigure(const YamlObject &phConfig) override; > > void platformStart() override; > void platformStop() override; > @@ -1331,7 +1337,7 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const > return status; > } > > -int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) > +int PiSPCameraData::platformPipelineConfigure(const YamlObject &phConfig) > { > config_ = { > .numCfeConfigStatsBuffers = 12, > @@ -1340,23 +1346,9 @@ 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 99d43bd0a..71c425373 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -61,13 +61,19 @@ public: > return unicam_[Unicam::Image].dev(); > } > > + const std::string &target() const override > + { > + static const std::string target = "bcm2835"; > + return target; > + } > + > void platformFreeBuffers() override > { > } > > CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override; > > - int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override; > + int platformPipelineConfigure(const YamlObject &phConfig) override; > > void platformStart() override; > void platformStop() override; > @@ -493,30 +499,16 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfig > return status; > } > > -int Vc4CameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) > +int Vc4CameraData::platformPipelineConfigure(const YamlObject &phConfig) > { > 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 = > -- > 2.50.1 >
Hi Paul, thank you for review. Paul Elder <paul.elder@ideasonboard.com> writes: > Hi Milan, > > Thanks for the patch. > > Quoting Milan Zamazal (2025-07-29 16:31:52) >> 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: >> pipelines: >> rpi: >> bcm2835: >> pipeline_handler: >> ... >> pisp: >> 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> > > The commit title should be s/config:/pipeline: rpi:/ , as this touches rpi code > and not global_configuration code. OK. >> --- >> .../pipeline/rpi/common/pipeline_base.cpp | 59 +++++++++++-------- >> .../pipeline/rpi/common/pipeline_base.h | 3 +- >> src/libcamera/pipeline/rpi/pisp/pisp.cpp | 26 +++----- >> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 26 +++----- >> 4 files changed, 56 insertions(+), 58 deletions(-) >> >> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp >> index 563df198e..1effa85cd 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; >> >> @@ -1091,35 +1093,46 @@ int CameraData::loadPipelineConfiguration() >> /* Initial configuration of the platform, in case no config file is present */ >> platformPipelineConfigure({}); >> >> + 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 (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; >> + } >> >> - 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 << "'"; >> + root = YamlParser::parse(file); >> + if (!root) { >> + LOG(RPI, Warning) << "Failed to parse configuration file, using defaults"; >> + return 0; >> + } >> >> - std::unique_ptr<YamlObject> 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; >> + std::optional<std::string> t = (*root)["target"].get<std::string>(); >> + if (t != target()) { >> + LOG(RPI, Error) << "Unexpected target reported: expected \"" >> + << target() << "\", got " << (t ? t->c_str() : "(unknown)"); >> + return -EINVAL; >> + } >> } >> >> - const YamlObject &phConfig = (*root)["pipeline_handler"]; >> + GlobalConfiguration::Configuration config = >> + pipe()->cameraManager()->_d()->configuration().configuration()["pipelines"]["rpi"]; > > Don't we need to check if configuration()["pipelines"] and configuration()["pipelines"]["rpi"] exist? YamlObject::operator[] returns an empty YamlObject if the given key doesn't exist, so it should be all fine. >> + const YamlObject &phConfig = (root >> + ? (*root)["pipeline_handler"] >> + : config[target()]["pipeline_handler"]); > > Same here, for config[target()] and config[target()]["pipeline_handler"] ? Again, YamlObject should do the right thing as well as YamlObject::get<>() with a default value, used later in the code. > It looks like the raspberry pi config takes precedence. Yes. > I suppose that's fine, since it will be removed. Maybe we should add a > warning message that it is deprecated and should be removed or moved > to the main configuration file. I don't plan the removal at the moment. If it is intended to be done in the future, I can add the warning. > The rest looks good. > > > Thanks, > > Paul > >> >> if (phConfig.contains("disable_startup_frame_drops")) >> LOG(RPI, Warning) >> @@ -1135,7 +1148,7 @@ int CameraData::loadPipelineConfiguration() >> frontendDevice()->setDequeueTimeout(config_.cameraTimeoutValue * 1ms); >> } >> >> - return platformPipelineConfigure(root); >> + return platformPipelineConfigure(phConfig); >> } >> >> 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 4bce4ec4f..ffbede798 100644 >> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h >> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h >> @@ -95,8 +95,9 @@ public: >> virtual V4L2VideoDevice::Formats ispFormats() const = 0; >> virtual V4L2VideoDevice::Formats rawFormats() const = 0; >> virtual V4L2VideoDevice *frontendDevice() = 0; >> + virtual const std::string &target() const = 0; >> >> - virtual int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) = 0; >> + virtual int platformPipelineConfigure(const YamlObject &phConfig) = 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 082724c5a..0fb7c3fe8 100644 >> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp >> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp >> @@ -740,10 +740,16 @@ public: >> return cfe_[Cfe::Output0].dev(); >> } >> >> + const std::string &target() const override >> + { >> + static const std::string target = "pisp"; >> + return target; >> + } >> + >> CameraConfiguration::Status >> platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override; >> >> - int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override; >> + int platformPipelineConfigure(const YamlObject &phConfig) override; >> >> void platformStart() override; >> void platformStop() override; >> @@ -1331,7 +1337,7 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const >> return status; >> } >> >> -int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) >> +int PiSPCameraData::platformPipelineConfigure(const YamlObject &phConfig) >> { >> config_ = { >> .numCfeConfigStatsBuffers = 12, >> @@ -1340,23 +1346,9 @@ 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 99d43bd0a..71c425373 100644 >> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> @@ -61,13 +61,19 @@ public: >> return unicam_[Unicam::Image].dev(); >> } >> >> + const std::string &target() const override >> + { >> + static const std::string target = "bcm2835"; >> + return target; >> + } >> + >> void platformFreeBuffers() override >> { >> } >> >> CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override; >> >> - int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override; >> + int platformPipelineConfigure(const YamlObject &phConfig) override; >> >> void platformStart() override; >> void platformStop() override; >> @@ -493,30 +499,16 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfig >> return status; >> } >> >> -int Vc4CameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) >> +int Vc4CameraData::platformPipelineConfigure(const YamlObject &phConfig) >> { >> 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 = >> -- >> 2.50.1 >>
Hi Milan, Quoting Milan Zamazal (2025-09-09 00:07:56) > Hi Paul, > > thank you for review. > > Paul Elder <paul.elder@ideasonboard.com> writes: > > > Hi Milan, > > > > Thanks for the patch. > > > > Quoting Milan Zamazal (2025-07-29 16:31:52) > >> 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: > >> pipelines: > >> rpi: > >> bcm2835: > >> pipeline_handler: > >> ... > >> pisp: > >> 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> > > > > The commit title should be s/config:/pipeline: rpi:/ , as this touches rpi code > > and not global_configuration code. > > OK. > > >> --- > >> .../pipeline/rpi/common/pipeline_base.cpp | 59 +++++++++++-------- > >> .../pipeline/rpi/common/pipeline_base.h | 3 +- > >> src/libcamera/pipeline/rpi/pisp/pisp.cpp | 26 +++----- > >> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 26 +++----- > >> 4 files changed, 56 insertions(+), 58 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > >> index 563df198e..1effa85cd 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; > >> > >> @@ -1091,35 +1093,46 @@ int CameraData::loadPipelineConfiguration() > >> /* Initial configuration of the platform, in case no config file is present */ > >> platformPipelineConfigure({}); > >> > >> + 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 (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; > >> + } > >> > >> - 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 << "'"; > >> + root = YamlParser::parse(file); > >> + if (!root) { > >> + LOG(RPI, Warning) << "Failed to parse configuration file, using defaults"; > >> + return 0; > >> + } > >> > >> - std::unique_ptr<YamlObject> 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; > >> + std::optional<std::string> t = (*root)["target"].get<std::string>(); > >> + if (t != target()) { > >> + LOG(RPI, Error) << "Unexpected target reported: expected \"" > >> + << target() << "\", got " << (t ? t->c_str() : "(unknown)"); > >> + return -EINVAL; > >> + } > >> } > >> > >> - const YamlObject &phConfig = (*root)["pipeline_handler"]; > >> + GlobalConfiguration::Configuration config = > >> + pipe()->cameraManager()->_d()->configuration().configuration()["pipelines"]["rpi"]; > > > > Don't we need to check if configuration()["pipelines"] and configuration()["pipelines"]["rpi"] exist? > > YamlObject::operator[] returns an empty YamlObject if the given key > doesn't exist, so it should be all fine. Oh ok I see what you mean. That's neat. > > >> + const YamlObject &phConfig = (root > >> + ? (*root)["pipeline_handler"] > >> + : config[target()]["pipeline_handler"]); > > > > Same here, for config[target()] and config[target()]["pipeline_handler"] ? > > Again, YamlObject should do the right thing as well as > YamlObject::get<>() with a default value, used later in the code. > > > It looks like the raspberry pi config takes precedence. > > Yes. > > > I suppose that's fine, since it will be removed. Maybe we should add a > > warning message that it is deprecated and should be removed or moved > > to the main configuration file. > > I don't plan the removal at the moment. If it is intended to be done in > the future, I can add the warning. Ok. > > > The rest looks good. Then with the commit title fix, Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > Thanks, > > > > Paul > > > >> > >> if (phConfig.contains("disable_startup_frame_drops")) > >> LOG(RPI, Warning) > >> @@ -1135,7 +1148,7 @@ int CameraData::loadPipelineConfiguration() > >> frontendDevice()->setDequeueTimeout(config_.cameraTimeoutValue * 1ms); > >> } > >> > >> - return platformPipelineConfigure(root); > >> + return platformPipelineConfigure(phConfig); > >> } > >> > >> 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 4bce4ec4f..ffbede798 100644 > >> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h > >> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h > >> @@ -95,8 +95,9 @@ public: > >> virtual V4L2VideoDevice::Formats ispFormats() const = 0; > >> virtual V4L2VideoDevice::Formats rawFormats() const = 0; > >> virtual V4L2VideoDevice *frontendDevice() = 0; > >> + virtual const std::string &target() const = 0; > >> > >> - virtual int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) = 0; > >> + virtual int platformPipelineConfigure(const YamlObject &phConfig) = 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 082724c5a..0fb7c3fe8 100644 > >> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > >> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > >> @@ -740,10 +740,16 @@ public: > >> return cfe_[Cfe::Output0].dev(); > >> } > >> > >> + const std::string &target() const override > >> + { > >> + static const std::string target = "pisp"; > >> + return target; > >> + } > >> + > >> CameraConfiguration::Status > >> platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override; > >> > >> - int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override; > >> + int platformPipelineConfigure(const YamlObject &phConfig) override; > >> > >> void platformStart() override; > >> void platformStop() override; > >> @@ -1331,7 +1337,7 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const > >> return status; > >> } > >> > >> -int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) > >> +int PiSPCameraData::platformPipelineConfigure(const YamlObject &phConfig) > >> { > >> config_ = { > >> .numCfeConfigStatsBuffers = 12, > >> @@ -1340,23 +1346,9 @@ 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 99d43bd0a..71c425373 100644 > >> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > >> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > >> @@ -61,13 +61,19 @@ public: > >> return unicam_[Unicam::Image].dev(); > >> } > >> > >> + const std::string &target() const override > >> + { > >> + static const std::string target = "bcm2835"; > >> + return target; > >> + } > >> + > >> void platformFreeBuffers() override > >> { > >> } > >> > >> CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override; > >> > >> - int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override; > >> + int platformPipelineConfigure(const YamlObject &phConfig) override; > >> > >> void platformStart() override; > >> void platformStop() override; > >> @@ -493,30 +499,16 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfig > >> return status; > >> } > >> > >> -int Vc4CameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) > >> +int Vc4CameraData::platformPipelineConfigure(const YamlObject &phConfig) > >> { > >> 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 = > >> -- > >> 2.50.1 > >> >
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index 563df198e..1effa85cd 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; @@ -1091,35 +1093,46 @@ int CameraData::loadPipelineConfiguration() /* Initial configuration of the platform, in case no config file is present */ platformPipelineConfigure({}); + 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 (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; + } - 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 << "'"; + root = YamlParser::parse(file); + if (!root) { + LOG(RPI, Warning) << "Failed to parse configuration file, using defaults"; + return 0; + } - std::unique_ptr<YamlObject> 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; + std::optional<std::string> t = (*root)["target"].get<std::string>(); + if (t != target()) { + LOG(RPI, Error) << "Unexpected target reported: expected \"" + << target() << "\", got " << (t ? t->c_str() : "(unknown)"); + return -EINVAL; + } } - const YamlObject &phConfig = (*root)["pipeline_handler"]; + GlobalConfiguration::Configuration config = + pipe()->cameraManager()->_d()->configuration().configuration()["pipelines"]["rpi"]; + const YamlObject &phConfig = (root + ? (*root)["pipeline_handler"] + : config[target()]["pipeline_handler"]); if (phConfig.contains("disable_startup_frame_drops")) LOG(RPI, Warning) @@ -1135,7 +1148,7 @@ int CameraData::loadPipelineConfiguration() frontendDevice()->setDequeueTimeout(config_.cameraTimeoutValue * 1ms); } - return platformPipelineConfigure(root); + return platformPipelineConfigure(phConfig); } 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 4bce4ec4f..ffbede798 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h @@ -95,8 +95,9 @@ public: virtual V4L2VideoDevice::Formats ispFormats() const = 0; virtual V4L2VideoDevice::Formats rawFormats() const = 0; virtual V4L2VideoDevice *frontendDevice() = 0; + virtual const std::string &target() const = 0; - virtual int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) = 0; + virtual int platformPipelineConfigure(const YamlObject &phConfig) = 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 082724c5a..0fb7c3fe8 100644 --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp @@ -740,10 +740,16 @@ public: return cfe_[Cfe::Output0].dev(); } + const std::string &target() const override + { + static const std::string target = "pisp"; + return target; + } + CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override; - int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override; + int platformPipelineConfigure(const YamlObject &phConfig) override; void platformStart() override; void platformStop() override; @@ -1331,7 +1337,7 @@ PiSPCameraData::platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const return status; } -int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) +int PiSPCameraData::platformPipelineConfigure(const YamlObject &phConfig) { config_ = { .numCfeConfigStatsBuffers = 12, @@ -1340,23 +1346,9 @@ 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 99d43bd0a..71c425373 100644 --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp @@ -61,13 +61,19 @@ public: return unicam_[Unicam::Image].dev(); } + const std::string &target() const override + { + static const std::string target = "bcm2835"; + return target; + } + void platformFreeBuffers() override { } CameraConfiguration::Status platformValidate(RPi::RPiCameraConfiguration *rpiConfig) const override; - int platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) override; + int platformPipelineConfigure(const YamlObject &phConfig) override; void platformStart() override; void platformStop() override; @@ -493,30 +499,16 @@ CameraConfiguration::Status Vc4CameraData::platformValidate(RPi::RPiCameraConfig return status; } -int Vc4CameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &root) +int Vc4CameraData::platformPipelineConfigure(const YamlObject &phConfig) { 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: pipelines: rpi: bcm2835: pipeline_handler: ... pisp: 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 | 59 +++++++++++-------- .../pipeline/rpi/common/pipeline_base.h | 3 +- src/libcamera/pipeline/rpi/pisp/pisp.cpp | 26 +++----- src/libcamera/pipeline/rpi/vc4/vc4.cpp | 26 +++----- 4 files changed, 56 insertions(+), 58 deletions(-)