[libcamera-devel] [PATCH v2 04/10] pipeline: raspberrypi: Read config parameters from a file
Naushir Patuck
naush at raspberrypi.com
Mon Dec 5 10:20:18 CET 2022
Hi Laurent,
On Fri, 2 Dec 2022 at 20:44, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> 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.
Regards,
Naush
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221205/93188f1b/attachment.htm>
More information about the libcamera-devel
mailing list