[libcamera-devel] [PATCH v2 04/10] pipeline: raspberrypi: Read config parameters from a file
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 5 10:36:46 CET 2022
Hi Naush,
On Mon, Dec 05, 2022 at 09:20:18AM +0000, Naushir Patuck wrote:
> On Fri, 2 Dec 2022 at 20:44, Laurent Pinchart wrote:
> > It's me again :-) Please see below for another comment.
> >
> > 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.
> > >
> > > 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.
> > > + "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')
> > > 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;
> > > + }
> >
> > The numOutput0Buffers and minUnicamBuffers configuration parameters seem
> > to me to be a contract between the application and libcamera, where
> > setting them to 0 requires the application to always pass buffers for
> > the corresponding streams. As we discussed previously, while I'm fine
> > with the concept of a configuration file to tune the behaviour of the
> > camera, I'm bothered by conveing information about such an API contract
> > through a file.
> >
> > Could we instead add a hint to the StreamConfiguration class to let
> > applications tell about their side of the contract ? Something along the
> > lines of
>
> Sure, I'll prototype something along these lines for the next revision of the
> series.
Thanks a lot, that's very appreciated.
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 29235ddf0d8a..616f6dd174f1 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -13,6 +13,8 @@
> > #include <string>
> > #include <vector>
> >
> > +#include <libcamera/base/flags.h>
> > +
> > #include <libcamera/color_space.h>
> > #include <libcamera/framebuffer.h>
> > #include <libcamera/geometry.h>
> > @@ -39,6 +41,13 @@ private:
> > };
> >
> > struct StreamConfiguration {
> > + enum class Hint {
> > + None = 0,
> > + MandatoryBuffer = (1 << 0),
> > + };
> > +
> > + using Hints = Flags<Hint>;
> > +
> > StreamConfiguration();
> > StreamConfiguration(const StreamFormats &formats);
> >
> > @@ -50,6 +59,7 @@ struct StreamConfiguration {
> > unsigned int bufferCount;
> >
> > std::optional<ColorSpace> colorSpace;
> > + Hints hints;
> >
> > Stream *stream() const { return stream_; }
> > void setStream(Stream *stream) { stream_ = stream; }
> >
> > (names to be bikeshedded of course)
> >
> > > + }
> > > +
> > > /*
> > > * 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");
> > > + else
> > > + filename = std::string(configFromEnv);
> > > +
> > > + if (filename.empty())
> > > + return -EINVAL;
> > > +
> > > + 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;
> > > + }
> > > +
> > > + ASSERT((*root)["version"].get<double>() == 1.0);
> > > +
> > > + 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