[libcamera-devel] [PATCH v3 09/11] libcamera: pipeline: Add a simple pipeline handler

[Andrey Konovalov]

Hi Laurent,

Thank you for this patch set!

I've been using it with a sensor which supports just one format, but
recently Loic volunteered to give it a try on his db410c with ov5640
sensor. (The kernel and the libcamera branches used have few quick hacks
not appropriate to be sent upstream but required for the things to work
with this hardware configuration; I am working on rewriting them in a
proper way).

One issue has revealed - see below.

On 20.03.2020 04:48, Laurent Pinchart wrote:
> From: Martijn Braam <martijn at brixit.nl>
> 
> This new pipeline handler aims at supporting any simple device without
> requiring any device-specific code. Simple devices are currently defined
> as a graph made of one or multiple camera sensors and a single video
> node, with each sensor connected to the video node through a linear
> pipeline.
> 
> The simple pipeline handler will automatically parse the media graph,
> enumerate sensors, build supported stream configurations, and configure
> the pipeline, without any device-specific knowledge. It doesn't support
> configuration of any processing in the pipeline at the moment, but may
> be extended to support simple processing such as format conversion or
> scaling in the future.
> 
> The only device-specific information in the pipeline handler is the list
> of supported drivers, required for device matching. We may be able to
> remove this in the future by matching with the simple pipeline handler
> as a last resort option, after all other pipeline handlers have been
> tried.
> 
> Signed-off-by: Martijn Braam <martijn at brixit.nl>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes since v2:
> 
> - Log an error when setupFormats() fail
> - Propagate getFormat() and setFormat() errors to the caller of
>    setupFormats()
> - Reorder variable declarations in validate()
> - Add \todo comment related to the selection of the default format
> - Use log Error instead of Info if pipeline isn't valid
> - Rebase on top of V4L2PixelFormat
> 
> Changes since v1:
> 
> - Rebase on top of buffer API rework
> - Expose stream formats
> - Rework camera data config
> ---
>   src/libcamera/pipeline/meson.build        |   1 +
>   src/libcamera/pipeline/simple/meson.build |   3 +
>   src/libcamera/pipeline/simple/simple.cpp  | 710 ++++++++++++++++++++++
>   3 files changed, 714 insertions(+)
>   create mode 100644 src/libcamera/pipeline/simple/meson.build
>   create mode 100644 src/libcamera/pipeline/simple/simple.cpp
> 
> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> index 0d466225a72e..606ba31a0319 100644
> --- a/src/libcamera/pipeline/meson.build
> +++ b/src/libcamera/pipeline/meson.build
> @@ -5,3 +5,4 @@ libcamera_sources += files([
>   
>   subdir('ipu3')
>   subdir('rkisp1')
> +subdir('simple')
> diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build
> new file mode 100644
> index 000000000000..4945a3e173cf
> --- /dev/null
> +++ b/src/libcamera/pipeline/simple/meson.build
> @@ -0,0 +1,3 @@
> +libcamera_sources += files([
> +    'simple.cpp',
> +])
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> new file mode 100644
> index 000000000000..545a99fe31ca
> --- /dev/null
> +++ b/src/libcamera/pipeline/simple/simple.cpp

<snip>

> +int SimpleCameraData::init()
> +{
> +	SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(pipe_);
> +	V4L2VideoDevice *video = pipe->video();
> +	int ret;
> +
> +	/*
> +	 * Enumerate the possible pipeline configurations. For each media bus
> +	 * format supported by the sensor, propagate the formats through the
> +	 * pipeline, and enumerate the corresponding possible V4L2 pixel
> +	 * formats on the video node.
> +	 */
> +	for (unsigned int code : sensor_->mbusCodes()) {
> +		V4L2SubdeviceFormat format{ code, sensor_->resolution() };
> +
> +		/*
> +		 * Setup links first as some subdev drivers take active links
> +		 * into account to propaget TRY formats. So is life :-(
> +		 */
> +		ret = setupLinks();
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = setupFormats(&format, V4L2Subdevice::TryFormat);
> +		if (ret < 0) {
> +			LOG(SimplePipeline, Error)
> +				<< "Failed to setup pipeline for media bus code "
> +				<< utils::hex(code, 4);
> +			return ret;

If some of the formats supported by the camera sensor are not supported by an
element down the pipeline, this return statement aborts the enumeration thus
skipping the rest of the formats which might work.

This has been observed with ov5640 sensor connected to db410c board
(Thanks Loic for your help with testing and debugging this on your hardware).
The first value assigned to the code variable is RGB565_2X8_BE, so the
codes enumeration is aborted immediately, and doesn't get to trying the YUV
formats.

Replacing the above 'return' statement with 'continue' works much better -
configurations for a few YUV formats are added OK.

Thanks,
Andrey

> +		}
> +
> +		std::map<V4L2PixelFormat, std::vector<SizeRange>> videoFormats =
> +			video->formats(format.mbus_code);
> +
> +		LOG(SimplePipeline, Debug)
> +			<< "Adding configuration for " << format.size.toString()
> +			<< " in pixel formats [ "
> +			<< utils::join(videoFormats, ", ",
> +				       [](const auto &f) {
> +					       return f.first.toString();
> +				       })
> +			<< " ]";
> +
> +		/*
> +		 * Store the configuration in the formats_ map, mapping
> +		 * PixelFormat to configuration. Any previously stored value is
> +		 * overwritten, as the pipeline handler currently doesn't care
> +		 * about how a particular PixelFormat is achieved.
> +		 */
> +		for (const auto &videoFormat : videoFormats) {
> +			PixelFormat pixelFormat = video->toPixelFormat(videoFormat.first);
> +			if (!pixelFormat)
> +				continue;
> +
> +			Configuration config;
> +			config.code = code;
> +			config.pixelFormat = pixelFormat;
> +			config.size = format.size;
> +
> +			formats_[pixelFormat] = config;
> +		}
> +	}
> +
> +	if (formats_.empty()) {
> +		LOG(SimplePipeline, Error) << "No valid configuration found";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

<snip>