[libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read config parameters from a file
Naushir Patuck
naush at raspberrypi.com
Fri Jan 20 12:52:58 CET 2023
Hi Kieran,
Thank you for your review!
On Fri, 20 Jan 2023 at 11:05, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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
>
Ack.
>
> > 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.
>
min_total_unicam_buffers is the minimum amount of internal + application
allocated Unicam buffers, i.e. we want to have at least these many total
buffers available.
minUnicamBuffers is the minimum number of internal Unicam buffers, i.e. we
have to allocate these many for internal use.
So in that sense, it does act as an additional count... maybe? Really,
these params were defined to mirror what the existing buffer allocating
logic did.
> 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 ?
>
Correct, we will explicitly set the env variable (if needed) in
libcamera-apps/picamera2.
Other apps who don't set the env variable will get the default config,
which will guarantee to work for all cases at the expense of probably over
allocating buffers.
Regards,
Naush
>
>
> > +
> > + 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230120/e5fd7202/attachment.htm>
More information about the libcamera-devel
mailing list