[libcamera-devel] [PATCH v2 04/10] pipeline: raspberrypi: Read config parameters from a file

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

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.

How do you envision supporting different configurations for different
cameras (for instance for the cameras connected to the two Unicam
instances on the Raspberry Pi CM) ?

> 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.

Could you please wrap lines to 80 columns where possible ?

Comments are not allowed by JSON. This works only because we use a YAML
person that doesn't enforce strict JSON compliance. I would thus name
the file with a .yaml extension, and possibly convert it to the native
YAML syntax. You can then add an SPDX license header.

> +                "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')

Should we add a pipeline_data_dir variable, the same way we have
ipa_data_dir ?

> 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;
> +			}
> +		}

This seems to belong to a separate patch. Same for the change in
PipelineHandlerRPi::prepareBuffers().

> +
>  		/*
>  		 * 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");

Ideally the first parameter should be deduced from the PipelineHandler
subclass automatically, but that can be done later.

> +	else
> +		filename = std::string(configFromEnv);
> +
> +	if (filename.empty())
> +		return -EINVAL;

Do we need to require a default configuration file, given that config is
initialized to default values above ? I'm even tempted to consider
default.json as an example (possibly renamed to config.json.example)
only and not try to load it at runtime.

> +
> +	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!";

No need to shout, you can drop the exclamation mark at the end of all
comments.

> +		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.

> +	}
> +
> +	ASSERT((*root)["version"].get<double>() == 1.0);

I think this deserves similar treatment as parsing failures.

> +
> +	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