[libcamera-devel] [PATCH v2 04/10] pipeline: raspberrypi: Read config parameters from a file
Naushir Patuck
naush at raspberrypi.com
Fri Dec 2 14:31:08 CET 2022
Hi Laurent,
Thank you for your feedback!
On Fri, 2 Dec 2022 at 12:38, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> 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.
>
> > 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.
>
> > +
> > /*
> > * 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.
>
> > + 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.
Regards,
Naush
>
> > +
> > + 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221202/361d7d4b/attachment.htm>
More information about the libcamera-devel
mailing list