[libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read config parameters from a file

[Kieran Bingham]

Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48)
> Add the ability to read the platform configuration parameters from a config
> file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE environment
> variable. Use the PipelineHandler::configurationFile() helper to determine the
> full path of the file.

I think this needs an addition to Documentation/environment_variables.rst
 
> Provide an example configuration file named example.yaml. Currently two
> parameters are available through the json file:
> 
> "min_unicam_buffers"
> The minimum number of internal Unicam buffers to allocate.
> 
> "min_total_unicam_buffers"
> The minimum number of internal + external Unicam buffers that must be allocated.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/data/example.yaml    | 20 +++++++++
>  .../pipeline/raspberrypi/data/meson.build     |  8 ++++
>  .../pipeline/raspberrypi/meson.build          |  2 +
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 45 +++++++++++++++++++
>  4 files changed, 75 insertions(+)
>  create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml
>  create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml
> new file mode 100644
> index 000000000000..001a906af528
> --- /dev/null
> +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml
> @@ -0,0 +1,20 @@
> +{
> +        "version": 1.0,
> +        "target": "bcm2835",
> +
> +        "pipeline_handler":
> +        {
> +                # The minimum number of internal buffers to be allocated for
> +                # Unicam. This value must be greater than 0, but less than or
> +                # equal to min_total_unicam_buffers.
> +                "min_unicam_buffers": 2,

I anticipate we'll want somewhere specific to document every
configuration file option. I expect this is ok for the moment, but we
need to do better at documenting the public API.




> +
> +                # The minimum total (internal + external) buffer count used for
> +                # Unicam. The number of internal buffers allocated for Unicam is
> +                # given by:
> +                #
> +                # internal buffer count = max(min_unicam_buffers,
> +                #         min_total_unicam_buffers - external buffer count)
> +                "min_total_unicam_buffers": 4

Reading above as min_unicam_buffers must be less than or equal to
min_total_unicam_buffers really makes me wonder why this isn't set up so
it's an additional buffer count rather than a total buffer count.

Also we now have different documentation here in an example file, vs
what's in the code...

What will be the definitive source of information ?

I expect it will be the code - so hopefully we can generate
public API documentation from that ?

> +        }
> +}
> diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build
> new file mode 100644
> index 000000000000..1c70433bbcbc
> --- /dev/null
> +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +conf_files = files([
> +    'example.yaml',
> +])
> +
> +install_data(conf_files,
> +             install_dir : pipeline_data_dir / 'raspberrypi')
> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build
> index 6064a3f00122..59e8fb18c555 100644
> --- a/src/libcamera/pipeline/raspberrypi/meson.build
> +++ b/src/libcamera/pipeline/raspberrypi/meson.build
> @@ -6,3 +6,5 @@ libcamera_sources += files([
>      'raspberrypi.cpp',
>      'rpi_stream.cpp',
>  ])
> +
> +subdir('data')
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 6ed4cc2c7ba7..2b396f1db9b6 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -14,6 +14,7 @@
>  #include <unordered_set>
>  #include <utility>
>  
> +#include <libcamera/base/file.h>
>  #include <libcamera/base/shared_fd.h>
>  #include <libcamera/base/utils.h>
>  
> @@ -39,6 +40,7 @@
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
> +#include "libcamera/internal/yaml_parser.h"
>  
>  #include "delayed_controls.h"
>  #include "dma_heaps.h"
> @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>                          */
>                         stream->setExternalBuffer(buffer);
>                 }
> +
>                 /*
>                  * If no buffer is provided by the request for this stream, we
>                  * queue a nullptr to the stream to signify that it must use an
> @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline()
>                 .minTotalUnicamBuffers = 4,
>         };
>  
> +       char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> +       if (!configFromEnv || *configFromEnv == '\0')
> +               return 0;

This seems like the sort of thing users might want to set for the whole
system and make sure it stays configured. Which will warrant development
of a 'libcamera configuration file' - which would then have the ability
to set a default file here perhaps?

How do you expect users to currently handle this ? I guess your
libcamera-apps will set this env var perhaps? But will that then cause
users to have a different experience vs libcamera-apps and say any
generic app or the gstreamer pipeline ?


> +
> +       std::string filename = std::string(configFromEnv);
> +       File file(filename);
> +
> +       if (!file.open(File::OpenModeFlag::ReadOnly)) {
> +               LOG(RPI, Error) << "Failed to open configuration file '" << filename << "'";
> +               return -EIO;
> +       }
> +
> +       LOG(RPI, Info) << "Using configuration file '" << filename << "'";
> +
> +       std::unique_ptr<YamlObject> root = YamlParser::parse(file);
> +       if (!root) {
> +               LOG(RPI, Error) << "Failed to parse configuration file, using defaults";
> +               return -EINVAL;
> +       }
> +
> +       std::optional<double> ver = (*root)["version"].get<double>();
> +       if (!ver || *ver != 1.0) {
> +               LOG(RPI, Error) << "Unexpected configuration file version reported";
> +               return -EINVAL;
> +       }
> +
> +       const YamlObject &phConfig = (*root)["pipeline_handler"];
> +       config_.minUnicamBuffers =
> +               phConfig["min_unicam_buffers"].get<unsigned int>(config_.minUnicamBuffers);
> +       config_.minTotalUnicamBuffers =
> +               phConfig["min_total_unicam_buffers"].get<unsigned int>(config_.minTotalUnicamBuffers);
> +
> +       if (config_.minTotalUnicamBuffers < config_.minUnicamBuffers) {
> +               LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers";
> +               return -EINVAL;
> +       }
> +
> +       if (config_.minTotalUnicamBuffers < 1) {
> +               LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= 1";
> +               return -EINVAL;
> +       }
> +
>         return 0;
>  }
>  
> -- 
> 2.25.1
>