[v17,04/12] pipeline: rpi: Look up rpi configuration in the configuration file

24311 diff mbox series

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(-)

Hi Milan,

Thanks for the patch.

Quoting Milan Zamazal (2025-09-11 18:29:34)
> 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>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.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(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index c209aa596..a4a018268 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;
>  
> @@ -1093,35 +1095,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)
> @@ -1137,7 +1150,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.51.0
>