[libcamera-devel] [PATCH v2 04/10] pipeline: raspberrypi: Read config parameters from a file
David Plowman
david.plowman at raspberrypi.com
Fri Dec 2 14:43:54 CET 2022
Hi Naush, everyone
On Fri, 2 Dec 2022 at 13:31, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> 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 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?
I must confess I can't get too worked up about this either way. I
don't really see people editing these files like they might our tuning
files (they'll all just use our standard libcamera-apps/Picamera2
one), so I wouldn't be averse to going full YAML. I certainly like
having comments.
David
>
>>
>>
>> > + "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
More information about the libcamera-devel
mailing list