[PATCH v5 03/15] config: Introduce global runtime configuration

[Milan Zamazal]

Barnabás Pőcze <pobrn at protonmail.com> writes:

> Hi
>
>
> 2024. december 5., csütörtök 12:33 keltezéssel, Milan Zamazal <mzamazal at redhat.com> írta:
>
>> Hi Barnabás,
>> 
>> thank you for review.
>> 
>> Barnabás Pőcze <pobrn at protonmail.com> writes:
>> 
>> > Hi
>> >
>> >
>> > 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal at redhat.com> írta:
>> >
>> >> Currently, libcamera can be configured in runtime using several
>> >> environment variables.  With introducing more and more variables, this
>> >> mechanism reaches its limits.  It would be simpler and more flexible if
>> >> it was possible to configure libcamera in a single file.
>> >>
>> >> For example, there have been a request for defining pipeline precedence
>> >> in runtime.  We want to compile in multiple pipelines, in order to have
>> >> them accessible within single packages in distributions.  And then being
>> >> able to select among the pipelines manually as needed based on the
>> >> particular hardware or operating system environment.  Having the
>> >> configuration file then allows easy switching between hardware, GPU or
>> >> CPU IPAs.  Another possible use case is tuning image output, especially
>> >> with software ISP, to user liking.  For example, some users may prefer
>> >> higher contrast without the need to use the corresponding knobs, if
>> >> present at all, in every application.  The configuration file can also
>> >> be used to enable or disable experimental features and avoid the need to
>> >> track local patches changing configuration options hard-wired in the
>> >> code when working on new features.
>> >>
>> >> This patch introduces basic support for configuration files.
>> >> GlobalConfiguration class reads, stores and accesses the configuration.
>> >> Its instance is stored as a private singleton accessed using a static
>> >> method of the class.  There is currently no reason to have more than one
>> >> instance.
>> >>
>> >> libcamera configuration can be specified using a system-wide
>> >> configuration file or a user configuration file.  The user configuration
>> >> file takes precedence if present.  There is currently no way to merge
>> >> multiple configuration files, the one found is used as the only
>> >> configuration file.  If no configuration file is present, nothing
>> >> changes to the current libcamera behavior (except for some log
>> >> messages related to configuration file lookup).
>> >>
>> >> The configuration file is a YAML file.  We already have a mechanism for
>> >> handling YAML configuration files in libcamera and the given
>> >> infrastructure can be reused for the purpose.  However, the
>> >> configuration type is abstracted to make contingent future change of the
>> >> underlying class easier while retaining (most of) the original API.
>> >>
>> >> The configuration is versioned.  This has currently no particular
>> >> meaning but is likely to have its purpose in future, especially once
>> >> configuration validation is introduced.
>> >>
>> >> The configuration YAML file looks as follows:
>> >>
>> >>   ---
>> >>   version: 1
>> >>   configuration:
>> >>     WHATEVER CONFIGURATION NEEDED
>> >>
>> >> There is no logging about reading the configuration file and contingent
>> >> errors.  This is on purpose because logging will query configuration,
>> >> which can lead to various problems when done during configuration
>> >> initialization.  Reporting the errors will be added later.
>> >>
>> >> The global configuration is intended to be used as a hidden singleton,
>> >> with static class methods around it providing the configuration
>> >> information.  The constructor must be still public so that unique_ptr
>> >> can be used.
>> >>
>> >> A complication arises from the fact that when the configuration is
>> >> loaded, logging may be called and logging will ask for the
>> >> configuration.  This is error-prone and may lead to subtle problems.
>> >> For this reason, the global configuration is instantiated to a pointer,
>> >> with an empty configuration initially.  The real configuration will be
>> >> created through initialize() method.  It will be clearer how it helps in
>> >> the followup patch introducing logging configuration.
>> >>
>> >> Logging is also the most notable component from base that uses global
>> >> configuration.  In order to be able to do it, the global configuration
>> >> must be put to base.
>> >>
>> >> This patch introduces just the basic idea.  Actually using the
>> >> configuration in the corresponding places (everything what is currently
>> >> configurable via environment variables should be configurable in the
>> >> file configuration) and other enhancements are implemented in the
>> >> followup patches.
>> >>
>> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> >> ---
>> >>  .../libcamera/internal/global_configuration.h |  41 +++++
>> >>  include/libcamera/internal/meson.build        |   1 +
>> >>  src/libcamera/base/global_configuration.cpp   | 154 ++++++++++++++++++
>> >>  src/libcamera/base/meson.build                |   1 +
>> >>  4 files changed, 197 insertions(+)
>> >>  create mode 100644 include/libcamera/internal/global_configuration.h
>> >>  create mode 100644 src/libcamera/base/global_configuration.cpp
>> >>
>> >> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>> >> new file mode 100644
>> >> index 000000000..628d85cb8
>> >> --- /dev/null
>> >> +++ b/include/libcamera/internal/global_configuration.h
>> >> @@ -0,0 +1,41 @@
>> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> >> +/*
>> >> + * Copyright (C) 2024 Red Hat, inc.
>> >> + *
>> >> + * global_configuration.h - Global configuration handling
>> >> + */
>> >> +
>> >> +#pragma once
>> >> +
>> >> +#include <filesystem>
>> >> +#include <vector>
>> >> +
>> >> +#include "libcamera/internal/yaml_parser.h"
>> >> +
>> >> +namespace libcamera {
>> >> +
>> >> +class GlobalConfiguration
>> >> +{
>> >> +public:
>> >> +	using Configuration = const YamlObject &;
>> >> +
>> >> +	/* The constructor must be public to be able to use unique_ptr. */
>> >
>> > If one does
>> >
>> >   std::unique_ptr<T>(new T)
>> >
>> > then the constructor does not need to be public.
>> 
>> Ah, cool.
>> 
>> >> +	GlobalConfiguration();
>> >> +	static void initialize();
>> >> +
>> >> +	static unsigned int version();
>> >> +	static Configuration configuration();
>> >> +
>> >> +private:
>> >> +	static const std::vector<std::filesystem::path> globalConfigurationFiles;
>> >> +
>> >> +	static std::unique_ptr<GlobalConfiguration> instance_;
>> >> +
>> >> +	std::unique_ptr<YamlObject> configuration_;
>> >> +
>> >> +	bool loadFile(const std::filesystem::path &fileName);
>> >> +	void load();
>> >> +	static Configuration get();
>> >> +};
>> >> +
>> >> +} /* namespace libcamera */
>> >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> >> index 1c5eef9ca..6fc6406d7 100644
>> >> --- a/include/libcamera/internal/meson.build
>> >> +++ b/include/libcamera/internal/meson.build
>> >> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([
>> >>      'dma_buf_allocator.h',
>> >>      'formats.h',
>> >>      'framebuffer.h',
>> >> +    'global_configuration.h',
>> >>      'ipa_data_serializer.h',
>> >>      'ipa_manager.h',
>> >>      'ipa_module.h',
>> >> diff --git a/src/libcamera/base/global_configuration.cpp
>> >> b/src/libcamera/base/global_configuration.cpp
>> >> new file mode 100644
>> >> index 000000000..7dcf97265
>> >> --- /dev/null
>> >> +++ b/src/libcamera/base/global_configuration.cpp
>> >> @@ -0,0 +1,154 @@
>> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> >> +/*
>> >> + * Copyright (C) 2024 Red Hat, inc.
>> >> + *
>> >> + * global_configuration.cpp - Global configuration handling
>> >> + */
>> >> +
>> >> +#include "libcamera/internal/global_configuration.h"
>> >> +
>> >> +#include <filesystem>
>> >> +#include <memory>
>> >> +#include <sys/types.h>
>> >> +
>> >> +#include <libcamera/base/file.h>
>> >> +#include <libcamera/base/log.h>
>> >> +#include <libcamera/base/utils.h>
>> >> +
>> >> +#include "libcamera/internal/yaml_parser.h"
>> >> +
>> >> +namespace libcamera {
>> >> +
>> >> +std::unique_ptr<GlobalConfiguration> GlobalConfiguration::instance_ =
>> >> +	std::make_unique<GlobalConfiguration>();
>> >
>> > Why is a unique_ptr needed here?
>> 
>> Most notably to stick with the libcamera policy of avoiding using plain
>> pointers.  And for the instance disposal in initialize() below.
>> [...]
>
> I was thinking that there is no need to dynamically allocate a `GlobalConfiguration`
> instance. In fact, now I think `GlobalConfiguration` could simply be a namespace.
> All private members of the current class would go into an anonymous namespace,
> and all public members would just be functions in the namespace.

I see, we can get rid of `instance_' this way and just store the
configuration.  OK, this would be probably better.

>
> Regards,
> Barnabás Pőcze