[libcamera-devel] [PATCH v5 07/12] pipeline: raspberrypi: Read config parameters from a file
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jan 22 23:32:29 CET 2023
Hello,
On Fri, Jan 20, 2023 at 11:52:58AM +0000, Naushir Patuck via libcamera-devel wrote:
> On Fri, 20 Jan 2023 at 11:05, Kieran Bingham 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.
I'd appreciate if the documentation in this file could already be
improved, to explain the effects of the configuration parameters.
> > > +
> > > + # 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.
What happens if the application allocates buffers for the raw stream but
queues requests slowly ? The pipeline handler will have less than
min_total_unicam_buffers available at a time then. Is the parameter
ill-defined in such cases ?
> 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 ?
As this will be documentation for system integrators, I'd rather have it
in the example.yaml file, or possibly in .rst form somewhere in
Documentation/.
> > > + }
> > > +}
> > > 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 don't like this question, as I don't expect "users" in the traditional
sense of the term to set these parameters. End-users of camera
applications shouldn't need to know about any of this. These parameters
should be for "system integrators" instead. Keeping this in mind, I
would expect those parameters to be system-wide in the long term, even
if in the short term solution is different.
Of course, the boundary between "users" and "system integrators" is
quite fuzzy, especially on Raspberry Pi systems, but if anyone disagrees
conceptually, please let me know.
> > 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.
>
> > > +
> > > + 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;
Is this a valid use case or not ? If it's valid, you shouldn't return
-EINVAL as the caller will otherwise fail to register the camera, and I
would demote the message from Error to Warning. If it's no valid, then
you should drop the "using defaults" from the error message.
> > > + }
> > > +
> > > + 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;
> > > }
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list