| Message ID | 20230118085953.7027-8-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48) > Add the ability to read the platform configuration parameters from a config > file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE environment > variable. Use the PipelineHandler::configurationFile() helper to determine the > full path of the file. I think this needs an addition to Documentation/environment_variables.rst > Provide an example configuration file named example.yaml. Currently two > 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. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/data/example.yaml | 20 +++++++++ > .../pipeline/raspberrypi/data/meson.build | 8 ++++ > .../pipeline/raspberrypi/meson.build | 2 + > .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++++++++++ > 4 files changed, 75 insertions(+) > create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build > > diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml > new file mode 100644 > index 000000000000..001a906af528 > --- /dev/null > +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml > @@ -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, I anticipate we'll want somewhere specific to document every configuration file option. I expect this is ok for the moment, but we need to do better at documenting the public API. > + > + # 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 Reading above as min_unicam_buffers must be less than or equal to min_total_unicam_buffers really makes me wonder why this isn't set up so it's an additional buffer count rather than a total buffer count. Also we now have different documentation here in an example file, vs what's in the code... What will be the definitive source of information ? I expect it will be the code - so hopefully we can generate public API documentation from that ? > + } > +} > diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build > new file mode 100644 > index 000000000000..1c70433bbcbc > --- /dev/null > +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build > @@ -0,0 +1,8 @@ > +# SPDX-License-Identifier: CC0-1.0 > + > +conf_files = files([ > + 'example.yaml', > +]) > + > +install_data(conf_files, > + install_dir : pipeline_data_dir / 'raspberrypi') > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build > index 6064a3f00122..59e8fb18c555 100644 > --- a/src/libcamera/pipeline/raspberrypi/meson.build > +++ b/src/libcamera/pipeline/raspberrypi/meson.build > @@ -6,3 +6,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 6ed4cc2c7ba7..2b396f1db9b6 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> > > @@ -39,6 +40,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 "delayed_controls.h" > #include "dma_heaps.h" > @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) > */ > stream->setExternalBuffer(buffer); > } > + > /* > * 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 > @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline() > .minTotalUnicamBuffers = 4, > }; > > + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE"); > + if (!configFromEnv || *configFromEnv == '\0') > + return 0; This seems like the sort of thing users might want to set for the whole system and make sure it stays configured. Which will warrant development of a 'libcamera configuration file' - which would then have the ability to set a default file here perhaps? How do you expect users to currently handle this ? I guess your libcamera-apps will set this env var perhaps? But will that then cause users to have a different experience vs libcamera-apps and say any generic app or the gstreamer pipeline ? > + > + std::string filename = std::string(configFromEnv); > + 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; > + } > + > + std::optional<double> ver = (*root)["version"].get<double>(); > + if (!ver || *ver != 1.0) { > + LOG(RPI, Error) << "Unexpected configuration file version reported"; > + return -EINVAL; > + } > + > + 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); > + > + 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; > } > > -- > 2.25.1 >
Hi Kieran, Thank you for your review! On Fri, 20 Jan 2023 at 11:05, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48) > > Add the ability to read the platform configuration parameters from a > config > > file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE > environment > > variable. Use the PipelineHandler::configurationFile() helper to > determine the > > full path of the file. > > I think this needs an addition to Documentation/environment_variables.rst > Ack. > > > Provide an example configuration file named example.yaml. Currently two > > 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. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/data/example.yaml | 20 +++++++++ > > .../pipeline/raspberrypi/data/meson.build | 8 ++++ > > .../pipeline/raspberrypi/meson.build | 2 + > > .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++++++++++ > > 4 files changed, 75 insertions(+) > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml > b/src/libcamera/pipeline/raspberrypi/data/example.yaml > > new file mode 100644 > > index 000000000000..001a906af528 > > --- /dev/null > > +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml > > @@ -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, > > I anticipate we'll want somewhere specific to document every > configuration file option. I expect this is ok for the moment, but we > need to do better at documenting the public API. > > > > > > + > > + # 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 > > Reading above as min_unicam_buffers must be less than or equal to > min_total_unicam_buffers really makes me wonder why this isn't set up so > it's an additional buffer count rather than a total buffer count. > 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. minUnicamBuffers is the minimum number of internal Unicam buffers, i.e. we have to allocate these many for internal use. 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. > Also we now have different documentation here in an example file, vs > what's in the code... > > What will be the definitive source of information ? > > I expect it will be the code - so hopefully we can generate > public API documentation from that ? > > > + } > > +} > > diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build > b/src/libcamera/pipeline/raspberrypi/data/meson.build > > new file mode 100644 > > index 000000000000..1c70433bbcbc > > --- /dev/null > > +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build > > @@ -0,0 +1,8 @@ > > +# SPDX-License-Identifier: CC0-1.0 > > + > > +conf_files = files([ > > + 'example.yaml', > > +]) > > + > > +install_data(conf_files, > > + install_dir : pipeline_data_dir / 'raspberrypi') > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build > b/src/libcamera/pipeline/raspberrypi/meson.build > > index 6064a3f00122..59e8fb18c555 100644 > > --- a/src/libcamera/pipeline/raspberrypi/meson.build > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build > > @@ -6,3 +6,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 6ed4cc2c7ba7..2b396f1db9b6 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> > > > > @@ -39,6 +40,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 "delayed_controls.h" > > #include "dma_heaps.h" > > @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera > *camera, Request *request) > > */ > > stream->setExternalBuffer(buffer); > > } > > + > > /* > > * 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 > > @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline() > > .minTotalUnicamBuffers = 4, > > }; > > > > + char const *configFromEnv = > utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE"); > > + if (!configFromEnv || *configFromEnv == '\0') > > + return 0; > > This seems like the sort of thing users might want to set for the whole > system and make sure it stays configured. Which will warrant development > of a 'libcamera configuration file' - which would then have the ability > to set a default file here perhaps? > > How do you expect users to currently handle this ? I guess your > libcamera-apps will set this env var perhaps? But will that then cause > users to have a different experience vs libcamera-apps and say any > generic app or the gstreamer pipeline ? > Correct, we will explicitly set the env variable (if needed) in libcamera-apps/picamera2. 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. Regards, Naush > > > > + > > + std::string filename = std::string(configFromEnv); > > + 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; > > + } > > + > > + std::optional<double> ver = (*root)["version"].get<double>(); > > + if (!ver || *ver != 1.0) { > > + LOG(RPI, Error) << "Unexpected configuration file > version reported"; > > + return -EINVAL; > > + } > > + > > + 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); > > + > > + 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; > > } > > > > -- > > 2.25.1 > > >
Hello, On Fri, Jan 20, 2023 at 11:52:58AM +0000, Naushir Patuck via libcamera-devel wrote: > On Fri, 20 Jan 2023 at 11:05, Kieran Bingham wrote: > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48) > > > Add the ability to read the platform configuration parameters from a config > > > file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE environment > > > variable. Use the PipelineHandler::configurationFile() helper to determine the > > > full path of the file. > > > > I think this needs an addition to Documentation/environment_variables.rst > > Ack. > > > > Provide an example configuration file named example.yaml. Currently two > > > 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. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/data/example.yaml | 20 +++++++++ > > > .../pipeline/raspberrypi/data/meson.build | 8 ++++ > > > .../pipeline/raspberrypi/meson.build | 2 + > > > .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++++++++++ > > > 4 files changed, 75 insertions(+) > > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml > > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml > > > new file mode 100644 > > > index 000000000000..001a906af528 > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml > > > @@ -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, > > > > I anticipate we'll want somewhere specific to document every > > configuration file option. I expect this is ok for the moment, but we > > need to do better at documenting the public API. I'd appreciate if the documentation in this file could already be improved, to explain the effects of the configuration parameters. > > > + > > > + # 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 > > > > Reading above as min_unicam_buffers must be less than or equal to > > min_total_unicam_buffers really makes me wonder why this isn't set up so > > it's an additional buffer count rather than a total buffer count. > > 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. What happens if the application allocates buffers for the raw stream but queues requests slowly ? The pipeline handler will have less than min_total_unicam_buffers available at a time then. Is the parameter ill-defined in such cases ? > minUnicamBuffers is the minimum number of internal Unicam buffers, i.e. we > have to allocate these many for internal use. > > 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. > > > Also we now have different documentation here in an example file, vs > > what's in the code... > > > > What will be the definitive source of information ? > > > > I expect it will be the code - so hopefully we can generate > > public API documentation from that ? As this will be documentation for system integrators, I'd rather have it in the example.yaml file, or possibly in .rst form somewhere in Documentation/. > > > + } > > > +} > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build > > > new file mode 100644 > > > index 000000000000..1c70433bbcbc > > > --- /dev/null > > > +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build > > > @@ -0,0 +1,8 @@ > > > +# SPDX-License-Identifier: CC0-1.0 > > > + > > > +conf_files = files([ > > > + 'example.yaml', > > > +]) > > > + > > > +install_data(conf_files, > > > + install_dir : pipeline_data_dir / 'raspberrypi') > > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build > > > index 6064a3f00122..59e8fb18c555 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/meson.build > > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build > > > @@ -6,3 +6,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 6ed4cc2c7ba7..2b396f1db9b6 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> > > > > > > @@ -39,6 +40,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 "delayed_controls.h" > > > #include "dma_heaps.h" > > > @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) > > > */ > > > stream->setExternalBuffer(buffer); > > > } > > > + > > > /* > > > * 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 > > > @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline() > > > .minTotalUnicamBuffers = 4, > > > }; > > > > > > + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE"); > > > + if (!configFromEnv || *configFromEnv == '\0') > > > + return 0; > > > > This seems like the sort of thing users might want to set for the whole > > system and make sure it stays configured. Which will warrant development > > of a 'libcamera configuration file' - which would then have the ability > > to set a default file here perhaps? > > > > How do you expect users to currently handle this ? I don't like this question, as I don't expect "users" in the traditional sense of the term to set these parameters. End-users of camera applications shouldn't need to know about any of this. These parameters should be for "system integrators" instead. Keeping this in mind, I would expect those parameters to be system-wide in the long term, even if in the short term solution is different. Of course, the boundary between "users" and "system integrators" is quite fuzzy, especially on Raspberry Pi systems, but if anyone disagrees conceptually, please let me know. > > I guess your > > libcamera-apps will set this env var perhaps? But will that then cause > > users to have a different experience vs libcamera-apps and say any > > generic app or the gstreamer pipeline ? > > Correct, we will explicitly set the env variable (if needed) in > libcamera-apps/picamera2. > 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. > > > > + > > > + std::string filename = std::string(configFromEnv); > > > + 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; Is this a valid use case or not ? If it's valid, you shouldn't return -EINVAL as the caller will otherwise fail to register the camera, and I would demote the message from Error to Warning. If it's no valid, then you should drop the "using defaults" from the error message. > > > + } > > > + > > > + std::optional<double> ver = (*root)["version"].get<double>(); > > > + if (!ver || *ver != 1.0) { > > > + LOG(RPI, Error) << "Unexpected configuration file version reported"; > > > + return -EINVAL; > > > + } > > > + > > > + 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); > > > + > > > + 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; > > > } > > >
This is true. Apart from the buffer allocation optimisations, the goal of this series was to provide our "power users" a way to control some of these intricate bits.Hi Laurent, Thank you for your feedback. On Sun, 22 Jan 2023 at 22:32, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Fri, Jan 20, 2023 at 11:52:58AM +0000, Naushir Patuck via libcamera-devel wrote: > > On Fri, 20 Jan 2023 at 11:05, Kieran Bingham wrote: > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:48) > > > > Add the ability to read the platform configuration parameters from a config > > > > file provided by the user through the LIBCAMERA_RPI_CONFIG_FILE environment > > > > variable. Use the PipelineHandler::configurationFile() helper to determine the > > > > full path of the file. > > > > > > I think this needs an addition to Documentation/environment_variables.rst > > > > Ack. > > > > > > Provide an example configuration file named example.yaml. Currently two > > > > 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. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > .../pipeline/raspberrypi/data/example.yaml | 20 +++++++++ > > > > .../pipeline/raspberrypi/data/meson.build | 8 ++++ > > > > .../pipeline/raspberrypi/meson.build | 2 + > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++++++++++ > > > > 4 files changed, 75 insertions(+) > > > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/example.yaml > > > > create mode 100644 src/libcamera/pipeline/raspberrypi/data/meson.build > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml > > > > new file mode 100644 > > > > index 000000000000..001a906af528 > > > > --- /dev/null > > > > +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml > > > > @@ -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, > > > > > > I anticipate we'll want somewhere specific to document every > > > configuration file option. I expect this is ok for the moment, but we > > > need to do better at documenting the public API. > > I'd appreciate if the documentation in this file could already be > improved, to explain the effects of the configuration parameters. Sure, I can add some more details to the config files. > > > > > + > > > > + # 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 > > > > > > Reading above as min_unicam_buffers must be less than or equal to > > > min_total_unicam_buffers really makes me wonder why this isn't set up so > > > it's an additional buffer count rather than a total buffer count. > > > > 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. > > What happens if the application allocates buffers for the raw stream but > queues requests slowly ? The pipeline handler will have less than > min_total_unicam_buffers available at a time then. Is the parameter > ill-defined in such cases ? min_total_unicam_buffers refers to the total number of buffers allocated, not necessarily the number of buffers in active use. Maybe this needs to be made more explicit in the above comments as well? > > > minUnicamBuffers is the minimum number of internal Unicam buffers, i.e. we > > have to allocate these many for internal use. > > > > 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. > > > > > Also we now have different documentation here in an example file, vs > > > what's in the code... > > > > > > What will be the definitive source of information ? > > > > > > I expect it will be the code - so hopefully we can generate > > > public API documentation from that ? > > As this will be documentation for system integrators, I'd rather have it > in the example.yaml file, or possibly in .rst form somewhere in > Documentation/. > > > > > + } > > > > +} > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build > > > > new file mode 100644 > > > > index 000000000000..1c70433bbcbc > > > > --- /dev/null > > > > +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build > > > > @@ -0,0 +1,8 @@ > > > > +# SPDX-License-Identifier: CC0-1.0 > > > > + > > > > +conf_files = files([ > > > > + 'example.yaml', > > > > +]) > > > > + > > > > +install_data(conf_files, > > > > + install_dir : pipeline_data_dir / 'raspberrypi') > > > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build > > > > index 6064a3f00122..59e8fb18c555 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/meson.build > > > > +++ b/src/libcamera/pipeline/raspberrypi/meson.build > > > > @@ -6,3 +6,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 6ed4cc2c7ba7..2b396f1db9b6 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> > > > > > > > > @@ -39,6 +40,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 "delayed_controls.h" > > > > #include "dma_heaps.h" > > > > @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) > > > > */ > > > > stream->setExternalBuffer(buffer); > > > > } > > > > + > > > > /* > > > > * 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 > > > > @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline() > > > > .minTotalUnicamBuffers = 4, > > > > }; > > > > > > > > + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE"); > > > > + if (!configFromEnv || *configFromEnv == '\0') > > > > + return 0; > > > > > > This seems like the sort of thing users might want to set for the whole > > > system and make sure it stays configured. Which will warrant development > > > of a 'libcamera configuration file' - which would then have the ability > > > to set a default file here perhaps? > > > > > > How do you expect users to currently handle this ? > > I don't like this question, as I don't expect "users" in the traditional > sense of the term to set these parameters. End-users of camera > applications shouldn't need to know about any of this. These parameters > should be for "system integrators" instead. Keeping this in mind, I > would expect those parameters to be system-wide in the long term, even > if in the short term solution is different. > > Of course, the boundary between "users" and "system integrators" is > quite fuzzy, especially on Raspberry Pi systems, but if anyone disagrees > conceptually, please let me know. This is true. Apart from the buffer allocation optimisations, the goal of this series was to provide our "power users" a way to control some of these intricate bits. > > > > I guess your > > > libcamera-apps will set this env var perhaps? But will that then cause > > > users to have a different experience vs libcamera-apps and say any > > > generic app or the gstreamer pipeline ? > > > > Correct, we will explicitly set the env variable (if needed) in > > libcamera-apps/picamera2. > > 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. > > > > > > + > > > > + std::string filename = std::string(configFromEnv); > > > > + 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; > > Is this a valid use case or not ? If it's valid, you shouldn't return > -EINVAL as the caller will otherwise fail to register the camera, and I > would demote the message from Error to Warning. If it's no valid, then > you should drop the "using defaults" from the error message. This should be a valid condition. I'll make the correction as suggested. Regards, Naush > > > > > + } > > > > + > > > > + std::optional<double> ver = (*root)["version"].get<double>(); > > > > + if (!ver || *ver != 1.0) { > > > > + LOG(RPI, Error) << "Unexpected configuration file version reported"; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + 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); > > > > + > > > > + 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; > > > > } > > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/raspberrypi/data/example.yaml new file mode 100644 index 000000000000..001a906af528 --- /dev/null +++ b/src/libcamera/pipeline/raspberrypi/data/example.yaml @@ -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 + } +} diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/raspberrypi/data/meson.build new file mode 100644 index 000000000000..1c70433bbcbc --- /dev/null +++ b/src/libcamera/pipeline/raspberrypi/data/meson.build @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: CC0-1.0 + +conf_files = files([ + 'example.yaml', +]) + +install_data(conf_files, + install_dir : pipeline_data_dir / 'raspberrypi') diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build index 6064a3f00122..59e8fb18c555 100644 --- a/src/libcamera/pipeline/raspberrypi/meson.build +++ b/src/libcamera/pipeline/raspberrypi/meson.build @@ -6,3 +6,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 6ed4cc2c7ba7..2b396f1db9b6 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> @@ -39,6 +40,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 "delayed_controls.h" #include "dma_heaps.h" @@ -1150,6 +1152,7 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) */ stream->setExternalBuffer(buffer); } + /* * 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 @@ -1683,6 +1686,48 @@ int RPiCameraData::configurePipeline() .minTotalUnicamBuffers = 4, }; + char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE"); + if (!configFromEnv || *configFromEnv == '\0') + return 0; + + std::string filename = std::string(configFromEnv); + 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; + } + + std::optional<double> ver = (*root)["version"].get<double>(); + if (!ver || *ver != 1.0) { + LOG(RPI, Error) << "Unexpected configuration file version reported"; + return -EINVAL; + } + + 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); + + 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; }