<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 2 Dec 2022 at 12:38, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Tue, Nov 29, 2022 at 01:45:28PM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> Read the platform configuration parameters from default.json config file. Use<br>
> the PipelineHandler::configurationFile() helper to determine the full path of<br>
> the file. The default.json filename can be overridden by the user setting the<br>
> LIBCAMERA_RPI_CONFIG_FILE environment variable giving the full path of the<br>
> custom file.<br>
<br>
How do you envision supporting different configurations for different<br>
cameras (for instance for the cameras connected to the two Unicam<br>
instances on the Raspberry Pi CM) ?<br></blockquote><div><br></div><div>Following on from the comments in patch 2, as long as the 2 cameras are</div><div>running in 2 separate libcamera processes, we can use the LIBCAMERA_RPI_CONFIG_FILE</div><div>env variable to set different configurations. Longer term, it would be nice to</div><div>have an API to pass this (as well as a custom IPA turning file) from the application.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Add config parameter validation to ensure bad parameters cannot cause the<br>
> pipeline handler to stop working.<br>
> <br>
> Currently three parameters are available through the json file:<br>
> <br>
> "min_unicam_buffers"<br>
> The minimum number of internal Unicam buffers to allocate.<br>
> <br>
> "min_total_unicam_buffers"<br>
> The minimum number of internal + external Unicam buffers that must be allocated.<br>
> <br>
> "num_output0_buffers"<br>
> Number of internal ISP Output0 buffers to allocate.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> .../pipeline/raspberrypi/data/default.json | 20 ++++++<br>
> .../pipeline/raspberrypi/data/meson.build | 8 +++<br>
> .../pipeline/raspberrypi/meson.build | 2 +<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 61 +++++++++++++++++++<br>
> 4 files changed, 91 insertions(+)<br>
> create mode 100644 src/libcamera/pipeline/raspberrypi/data/default.json<br>
> create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json<br>
> new file mode 100644<br>
> index 000000000000..d709e31850af<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/raspberrypi/data/default.json<br>
> @@ -0,0 +1,20 @@<br>
> +{<br>
> + "version": 1.0,<br>
> + "target": "bcm2835",<br>
> +<br>
> + "pipeline_handler":<br>
> + {<br>
> + # The minimum number of internal buffers to be allocated for Unicam.<br>
> + # This value must be greater than 0, but less than or equal to min_total_unicam_buffers.<br>
<br>
Could you please wrap lines to 80 columns where possible ?<br>
<br>
Comments are not allowed by JSON. This works only because we use a YAML<br>
person that doesn't enforce strict JSON compliance. I would thus name<br>
the file with a .yaml extension, and possibly convert it to the native<br>
YAML syntax. You can then add an SPDX license header.<br></blockquote><div><br></div><div>You are right.</div><div><br></div><div><a class="gmail_plusreply" id="m_5985401236197532982m_824494572982853958plusReplyChip-0" href="mailto:david.plowman@raspberrypi.com" target="_blank">@David Plowman</a> Do you have a preference here? Should we keep "json" syntax<br></div><div>to match our tuning files? If so, I can rename this to .yaml only. Or should we replace</div><div>this with full yaml syntax?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + "min_unicam_buffers": 2,<br>
> +<br>
> + # The minimum total (internal + external) buffer count used for Unicam.<br>
> + # The number of internal buffers allocated for Unicam is given by:<br>
> + # internal buffer count = max(min_unicam_buffers,<br>
> + # min_total_unicam_buffers - external buffer count)<br>
> + "min_total_unicam_buffers": 4,<br>
> + <br>
> + # The number of internal buffers used for ISP Output0. This must be set to 1.<br>
> + "num_output0_buffers": 1<br>
> + }<br>
> +}<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build<br>
> new file mode 100644<br>
> index 000000000000..232f8b43c5fd<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build<br>
> @@ -0,0 +1,8 @@<br>
> +# SPDX-License-Identifier: CC0-1.0<br>
> +<br>
> +conf_files = files([<br>
> + 'default.json',<br>
> +])<br>
> +<br>
> +install_data(conf_files,<br>
> + install_dir : libcamera_datadir / 'pipeline/raspberrypi')<br>
<br>
Should we add a pipeline_data_dir variable, the same way we have<br>
ipa_data_dir ?<br></blockquote><div><br></div><div>Good idea.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build<br>
> index f1a2f5ee72c2..48235f887501 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/meson.build<br>
> +++ b/src/libcamera/pipeline/raspberrypi/meson.build<br>
> @@ -5,3 +5,5 @@ libcamera_sources += files([<br>
> 'raspberrypi.cpp',<br>
> 'rpi_stream.cpp',<br>
> ])<br>
> +<br>
> +subdir('data')<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index f2b695af2399..ce411453db0a 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -14,6 +14,7 @@<br>
> #include <unordered_set><br>
> #include <utility><br>
> <br>
> +#include <libcamera/base/file.h><br>
> #include <libcamera/base/shared_fd.h><br>
> #include <libcamera/base/utils.h><br>
> <br>
> @@ -40,6 +41,7 @@<br>
> #include "libcamera/internal/media_device.h"<br>
> #include "libcamera/internal/pipeline_handler.h"<br>
> #include "libcamera/internal/v4l2_videodevice.h"<br>
> +#include "libcamera/internal/yaml_parser.h"<br>
> <br>
> #include "dma_heaps.h"<br>
> #include "rpi_stream.h"<br>
> @@ -313,6 +315,7 @@ public:<br>
> };<br>
> <br>
> Config config_;<br>
> + unsigned int numUnicamBuffers;<br>
> <br>
> private:<br>
> void checkRequestCompleted();<br>
> @@ -1154,6 +1157,19 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)<br>
> */<br>
> stream->setExternalBuffer(buffer);<br>
> }<br>
> +<br>
> + if (!buffer) {<br>
> + if (stream == &data->isp_[Isp::Output0] && !data->config_.numOutput0Buffers) {<br>
> + LOG(RPI, Error) << "Output0 buffer must be provided in the request.";<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + if (stream == &data->unicam_[Unicam::Image] && !data->numUnicamBuffers) {<br>
> + LOG(RPI, Error) << "Unicam Image buffer must be provided in the request.";<br>
> + return -EINVAL;<br>
> + }<br>
> + }<br>
<br>
This seems to belong to a separate patch. Same for the change in<br>
PipelineHandlerRPi::prepareBuffers().<br></blockquote><div><br></div><div>The block above is to validate the Request has the stream buffers required based on the config</div><div>params read from the file in this commit. If you prefer, I can move this and the lower bit of</div><div>validation to a separate commit.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> /*<br>
> * If no buffer is provided by the request for this stream, we<br>
> * queue a nullptr to the stream to signify that it must use an<br>
> @@ -1419,6 +1435,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
> int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)<br>
> {<br>
> RPiCameraData::Config &config = data->config_;<br>
> + std::string filename;<br>
> <br>
> config = {<br>
> .minUnicamBuffers = 2,<br>
> @@ -1426,6 +1443,49 @@ int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)<br>
> .numOutput0Buffers = 1,<br>
> };<br>
> <br>
> + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");<br>
> + if (!configFromEnv || *configFromEnv == '\0')<br>
> + filename = configurationFile("raspberrypi", "default.json");<br>
<br>
Ideally the first parameter should be deduced from the PipelineHandler<br>
subclass automatically, but that can be done later.<br></blockquote><div><br></div><div>I did consider this, and we have PipelineHandler::name() that I thought we could use.</div><div>However, the name field returned is based on the factory creation, and the strings are<br></div><div>generated as "PipelineHandlerRPi", "PipelineHandlerIPU3", etc. Having the config</div><div>subdirectory named like that seemed a bit ugly to me.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + else<br>
> + filename = std::string(configFromEnv);<br>
> +<br>
> + if (filename.empty())<br>
> + return -EINVAL;<br>
<br>
Do we need to require a default configuration file, given that config is<br>
initialized to default values above ? I'm even tempted to consider<br>
default.json as an example (possibly renamed to config.json.example)<br>
only and not try to load it at runtime.<br></blockquote><div><br></div><div>I was a bit unsure of this myself. Happy to remove (or rename) default.json</div><div>and use the struct for the default configuration if others think that is better.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + File file(filename);<br>
> + if (!file.open(File::OpenModeFlag::ReadOnly)) {<br>
> + LOG(RPI, Error) << "Failed to open configuration file '" << filename << "'";<br>
> + return -EIO;<br>
> + }<br>
> +<br>
> + LOG(RPI, Info) << "Using configuration file '" << filename << "'";<br>
> +<br>
> + std::unique_ptr<YamlObject> root = YamlParser::parse(file);<br>
> + if (!root) {<br>
> + LOG(RPI, Error) << "Failed to parse configuration file, using defaults!";<br>
<br>
No need to shout, you can drop the exclamation mark at the end of all<br>
comments.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
> + return -EINVAL;<br>
<br>
Is this a valid use case or not ? If it's valid, you shouldn't return<br>
-EINVAL as the caller will otherwise fail to register the camera, and I<br>
would demote the message from Error to Warning. If it's no valid, then<br>
you should drop the "using defaults" from the error message.<br>
<br>
> + }<br>
> +<br>
> + ASSERT((*root)["version"].get<double>() == 1.0);<br>
<br>
I think this deserves similar treatment as parsing failures.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
> +<br>
> + const YamlObject &phConfig = (*root)["pipeline_handler"];<br>
> + config.minUnicamBuffers =<br>
> + phConfig["min_unicam_buffers"].get<unsigned int>(config.minUnicamBuffers);<br>
> + config.minTotalUnicamBuffers =<br>
> + phConfig["min_total_unicam_buffers"].get<unsigned int>(config.minTotalUnicamBuffers);<br>
> + config.numOutput0Buffers =<br>
> + phConfig["num_output0_buffers"].get<unsigned int>(config.numOutput0Buffers);<br>
> +<br>
> + if (config.minTotalUnicamBuffers < config.minUnicamBuffers) {<br>
> + LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= min_unicam_buffers!";<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + if (config.minTotalUnicamBuffers < 1) {<br>
> + LOG(RPI, Error) << "Invalid configuration: min_total_unicam_buffers must be >= 1!";<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> return 0;<br>
> }<br>
> <br>
> @@ -1492,6 +1552,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)<br>
> */<br>
> numBuffers = std::max<int>(data->config_.minUnicamBuffers,<br>
> minBuffers - numRawBuffers);<br>
> + data->numUnicamBuffers = numBuffers;<br>
> } else if (stream == &data->isp_[Isp::Input]) {<br>
> /*<br>
> * ISP input buffers are imported from Unicam, so follow<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>