[libcamera-devel] [PATCH v1 04/10] pipeline: raspberrypi: Read config parameters from a file
Naushir Patuck
naush at raspberrypi.com
Tue Nov 29 11:44:18 CET 2022
Hi David,
Thank you for your feedback
On Tue, 1 Nov 2022 at 12:01, David Plowman <david.plowman at raspberrypi.com>
wrote:
> Hi Naush
>
> Thanks for the patch!
>
> On Fri, 14 Oct 2022 at 14:19, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> 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>
> > ---
> > .../pipeline/raspberrypi/data/default.json | 20 +++++++
> > .../pipeline/raspberrypi/data/meson.build | 8 +++
> > .../pipeline/raspberrypi/meson.build | 2 +
> > .../pipeline/raspberrypi/raspberrypi.cpp | 56 +++++++++++++++++++
> > 4 files changed, 86 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.
>
> Are we allowed comments in json files now? That's nice! Perhaps we
> should put some in our tuning files...
>
> > + "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 450029197b11..bac7a66ba900 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"
> > @@ -301,6 +303,7 @@ public:
> > };
> >
> > Config config_;
> > + unsigned int numUnicamBuffers;
> >
> > private:
> > void checkRequestCompleted();
> > @@ -1140,6 +1143,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;
> > + }
> > + }
> > +
> > /*
> > * 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
> > @@ -1398,6 +1414,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,
> > @@ -1405,6 +1422,44 @@ 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");
>
> I was a teeny bit nervous that in the non-installed case it might come
> back with "libcamera/src/ipa/raspberrypi/data/raspberrypi/default.json"
> which would be the wrong thing? Maybe you could just put my mind at
> ease here...
>
It should load things from
"libcamera/src/libcamera/pipeline/data/raspberrypi/default.json",
but I ought to check this again to make sure!
However, that does lead to some awkwardness, perhaps it should be read from
"libcamera/src/libcamera/pipeline/raspberrypi/data/default.json" to be
consistent with
the ipa config files...?
>
> > + 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 ||
> config.minTotalUnicamBuffers < 1) {
>
> I wonder a little bit about splitting up all these little checks so
> that we can be really specific in the error message? But I'm not too
> fussed.
>
Good point, I will split this up.
Regards,
Naush
>
> Subject to a check on that pathname question I had:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > + LOG(RPI, Error) << "Invalid Unicam buffer configuration
> used!";
> > + return -EINVAL;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1471,6 +1526,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
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221129/2b6a5b63/attachment.htm>
More information about the libcamera-devel
mailing list