<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your review!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 20 Jan 2023 at 11:05, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@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">Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48)<br>
> Add the ability to read the platform configuration parameters from a config<br>
> file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE environment<br>
> variable. Use the PipelineHandler::configurationFile() helper to determine the<br>
> full path of the file.<br>
<br>
I think this needs an addition to Documentation/environment_variables.rst<br></blockquote><div><br></div><div>Ack.</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>
> Provide an example configuration file named example.yaml. Currently two<br>
> 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>
> 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/example.yaml | 20 +++++++++<br>
> .../pipeline/raspberrypi/data/meson.build | 8 ++++<br>
> .../pipeline/raspberrypi/meson.build | 2 +<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++++++++++<br>
> 4 files changed, 75 insertions(+)<br>
> create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml<br>
> new file mode 100644<br>
> index 000000000000..001a906af528<br>
> --- /dev/null<br>
> +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml<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<br>
> + # Unicam. This value must be greater than 0, but less than or<br>
> + # equal to min_total_unicam_buffers.<br>
> + "min_unicam_buffers": 2,<br>
<br>
I anticipate we'll want somewhere specific to document every<br>
configuration file option. I expect this is ok for the moment, but we<br>
need to do better at documenting the public API.<br>
<br>
<br>
<br>
<br>
> +<br>
> + # The minimum total (internal + external) buffer count used for<br>
> + # Unicam. The number of internal buffers allocated for Unicam is<br>
> + # given by:<br>
> + #<br>
> + # internal buffer count = max(min_unicam_buffers,<br>
> + # min_total_unicam_buffers - external buffer count)<br>
> + "min_total_unicam_buffers": 4<br>
<br>
Reading above as min_unicam_buffers must be less than or equal to<br>
min_total_unicam_buffers really makes me wonder why this isn't set up so<br>
it's an additional buffer count rather than a total buffer count.<br></blockquote><div><br></div><div>min_total_unicam_buffers is the minimum amount of internal + application allocated Unicam buffers, i.e. we want to have at least these many total buffers available.<br></div><div>minUnicamBuffers is the minimum number of internal Unicam buffers, i.e. we have to allocate these many for internal use.<br></div><div> </div><div>So in that sense, it does act as an additional count... maybe? Really, these params were defined to mirror what the existing buffer allocating logic did.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Also we now have different documentation here in an example file, vs<br>
what's in the code...<br>
<br>
What will be the definitive source of information ?<br>
<br>
I expect it will be the code - so hopefully we can generate<br>
public API documentation from that ?<br>
<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..1c70433bbcbc<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>
> + 'example.yaml',<br>
> +])<br>
> +<br>
> +install_data(conf_files,<br>
> + install_dir : pipeline_data_dir / 'raspberrypi')<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build<br>
> index 6064a3f00122..59e8fb18c555 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/meson.build<br>
> +++ b/src/libcamera/pipeline/raspberrypi/meson.build<br>
> @@ -6,3 +6,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 6ed4cc2c7ba7..2b396f1db9b6 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>
> @@ -39,6 +40,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 "delayed_controls.h"<br>
> #include "dma_heaps.h"<br>
> @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)<br>
> */<br>
> stream->setExternalBuffer(buffer);<br>
> }<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>
> @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline()<br>
> .minTotalUnicamBuffers = 4,<br>
> };<br>
> <br>
> + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");<br>
> + if (!configFromEnv || *configFromEnv == '\0')<br>
> + return 0;<br>
<br>
This seems like the sort of thing users might want to set for the whole<br>
system and make sure it stays configured. Which will warrant development<br>
of a 'libcamera configuration file' - which would then have the ability<br>
to set a default file here perhaps?<br>
<br>
How do you expect users to currently handle this ? I guess your<br>
libcamera-apps will set this env var perhaps? But will that then cause<br>
users to have a different experience vs libcamera-apps and say any<br>
generic app or the gstreamer pipeline ?<br></blockquote><div><br></div><div>Correct, we will explicitly set the env variable (if needed) in libcamera-apps/picamera2.</div><div>Other apps who don't set the env variable will get the default config, which will guarantee to work for all cases at the expense of probably over allocating buffers.</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>
> +<br>
> + std::string filename = std::string(configFromEnv);<br>
> + File file(filename);<br>
> +<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>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + std::optional<double> ver = (*root)["version"].get<double>();<br>
> + if (!ver || *ver != 1.0) {<br>
> + LOG(RPI, Error) << "Unexpected configuration file version reported";<br>
> + return -EINVAL;<br>
> + }<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>
> +<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>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>