[libcamera-devel] [PATCH v2 04/10] pipeline: raspberrypi: Read config parameters from a file
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 2 17:19:03 CET 2022
Hi Naush,
On Fri, Dec 02, 2022 at 01:31:08PM +0000, Naushir Patuck wrote:
> On Fri, 2 Dec 2022 at 12:38, Laurent Pinchart wrote:
> > On Tue, Nov 29, 2022 at 01:45:28PM +0000, Naushir Patuck via libcamera-devel wrote:
> > > Read the platform configuration parameters from default.json config file. Use
> > > the PipelineHandler::configurationFile() helper to determine the full path of
> > > the file. The default.json filename can be overridden by the user setting the
> > > LIBCAMERA_RPI_CONFIG_FILE environment variable giving the full path of the
> > > custom file.
> >
> > How do you envision supporting different configurations for different
> > cameras (for instance for the cameras connected to the two Unicam
> > instances on the Raspberry Pi CM) ?
>
> Following on from the comments in patch 2, as long as the 2 cameras are
> running in 2 separate libcamera processes, we can use the LIBCAMERA_RPI_CONFIG_FILE
> env variable to set different configurations. Longer term, it would be nice to
> have an API to pass this (as well as a custom IPA turning file) from the
> application.
OK, works for me. I'm sure we'll improve all that in the future, it's
good enough for now.
> > > Add config parameter validation to ensure bad parameters cannot cause the
> > > pipeline handler to stop working.
> > >
> > > Currently three 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.
> > >
> > > "num_output0_buffers"
> > > Number of internal ISP Output0 buffers to allocate.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > > .../pipeline/raspberrypi/data/default.json | 20 ++++++
> > > .../pipeline/raspberrypi/data/meson.build | 8 +++
> > > .../pipeline/raspberrypi/meson.build | 2 +
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 61 +++++++++++++++++++
> > > 4 files changed, 91 insertions(+)
> > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/default.json
> > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json
> > > new file mode 100644
> > > index 000000000000..d709e31850af
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json
> > > @@ -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.
> >
> > Could you please wrap lines to 80 columns where possible ?
> >
> > Comments are not allowed by JSON. This works only because we use a YAML
> > person that doesn't enforce strict JSON compliance. I would thus name
> > the file with a .yaml extension, and possibly convert it to the native
> > YAML syntax. You can then add an SPDX license header.
>
> You are right.
>
> @David Plowman <david.plowman at raspberrypi.com> Do you have a preference
> here? Should we keep "json" syntax
> to match our tuning files? If so, I can rename this to .yaml only. Or
> should we replace
> this with full yaml syntax?
>
> > > + "min_unicam_buffers": 2,
> > > +
> > > + # 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,
> > > +
> > > + # The number of internal buffers used for ISP Output0. This must be set to 1.
> > > + "num_output0_buffers": 1
> > > + }
> > > +}
> > > diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build
> > > new file mode 100644
> > > index 000000000000..232f8b43c5fd
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build
> > > @@ -0,0 +1,8 @@
> > > +# SPDX-License-Identifier: CC0-1.0
> > > +
> > > +conf_files = files([
> > > + 'default.json',
> > > +])
> > > +
> > > +install_data(conf_files,
> > > + install_dir : libcamera_datadir / 'pipeline/raspberrypi')
> >
> > Should we add a pipeline_data_dir variable, the same way we have
> > ipa_data_dir ?
>
> Good idea.
>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build
> > > index f1a2f5ee72c2..48235f887501 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/meson.build
> > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build
> > > @@ -5,3 +5,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 f2b695af2399..ce411453db0a 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>
> > >
> > > @@ -40,6 +41,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 "dma_heaps.h"
> > > #include "rpi_stream.h"
> > > @@ -313,6 +315,7 @@ public:
> > > };
> > >
> > > Config config_;
> > > + unsigned int numUnicamBuffers;
> > >
> > > private:
> > > void checkRequestCompleted();
> > > @@ -1154,6 +1157,19 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> > > */
> > > stream->setExternalBuffer(buffer);
> > > }
> > > +
> > > + if (!buffer) {
> > > + if (stream == &data->isp_[Isp::Output0] && !data->config_.numOutput0Buffers) {
> > > + LOG(RPI, Error) << "Output0 buffer must be provided in the request.";
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (stream == &data->unicam_[Unicam::Image] && !data->numUnicamBuffers) {
> > > + LOG(RPI, Error) << "Unicam Image buffer must be provided in the request.";
> > > + return -EINVAL;
> > > + }
> > > + }
> >
> > This seems to belong to a separate patch. Same for the change in
> > PipelineHandlerRPi::prepareBuffers().
>
> The block above is to validate the Request has the stream buffers required based on the config
> params read from the file in this commit. If you prefer, I can move this and the lower bit of
> validation to a separate commit.
I would prefer that, with an explanation of the validation in the commit
message. You can move this change before this patch, as
Config::numOutput0Buffers already exists.
> > > +
> > > /*
> > > * 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
> > > @@ -1419,6 +1435,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> > > int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)
> > > {
> > > RPiCameraData::Config &config = data->config_;
> > > + std::string filename;
> > >
> > > config = {
> > > .minUnicamBuffers = 2,
> > > @@ -1426,6 +1443,49 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)
> > > .numOutput0Buffers = 1,
> > > };
> > >
> > > + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> > > + if (!configFromEnv || *configFromEnv == '\0')
> > > + filename = configurationFile("raspberrypi", "default.json");
> >
> > Ideally the first parameter should be deduced from the PipelineHandler
> > subclass automatically, but that can be done later.
>
> I did consider this, and we have PipelineHandler::name() that I thought we could use.
> However, the name field returned is based on the factory creation, and the strings are
> generated as "PipelineHandlerRPi", "PipelineHandlerIPU3", etc. Having the config
> subdirectory named like that seemed a bit ugly to me.
I agree. We'll improve this later.
> > > + else
> > > + filename = std::string(configFromEnv);
> > > +
> > > + if (filename.empty())
> > > + return -EINVAL;
> >
> > Do we need to require a default configuration file, given that config is
> > initialized to default values above ? I'm even tempted to consider
> > default.json as an example (possibly renamed to config.json.example)
> > only and not try to load it at runtime.
>
> I was a bit unsure of this myself. Happy to remove (or rename) default.json
> and use the struct for the default configuration if others think that is
> better.
I think I would prefer renaming (or removing) it. When using the default
configuration, parsing the file is wasting CPU cycles.
> > > +
> > > + 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!";
> >
> > No need to shout, you can drop the exclamation mark at the end of all
> > comments.
> >
> > > + 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.
> >
> > > + }
> > > +
> > > + ASSERT((*root)["version"].get<double>() == 1.0);
> >
> > I think this deserves similar treatment as parsing failures.
> >
> > > +
> > > + 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);
> > > + config.numOutput0Buffers =
> > > + phConfig["num_output0_buffers"].get<unsigned int>(config.numOutput0Buffers);
> > > +
> > > + 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;
> > > }
> > >
> > > @@ -1492,6 +1552,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > > */
> > > numBuffers = std::max<int>(data->config_.minUnicamBuffers,
> > > minBuffers - numRawBuffers);
> > > + data->numUnicamBuffers = numBuffers;
> > > } else if (stream == &data->isp_[Isp::Input]) {
> > > /*
> > > * ISP input buffers are imported from Unicam, so follow
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list