[libcamera-devel] [PATCH v3 1/2] libcamera: pipeline: Add Intel IPU3 pipeline

[Kieran Bingham]

Hi Jacopo,

It's my first time looking at this series, please forgive the
lateness... and my comments below are questions (for anybody) - not
blockers.


On 21/01/2019 10:57, Jacopo Mondi wrote:
> Add a pipeline handler for the Intel IPU3 device.
> 
> The pipeline handler creates a Camera for each image sensor it finds to be
> connected to an IPU3 CSI-2 receiver, and enables the link between the two.
> 
> Tested on Soraka, listing detected cameras on the system, verifying the
> pipeline handler gets matched and links properly enabled.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp    | 216 ++++++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/meson.build |   3 +
>  src/libcamera/pipeline/meson.build      |   2 +
>  3 files changed, 221 insertions(+)
>  create mode 100644 src/libcamera/pipeline/ipu3/ipu3.cpp
>  create mode 100644 src/libcamera/pipeline/ipu3/meson.build
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> new file mode 100644
> index 0000000..2081743
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -0,0 +1,216 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipu3.cpp - Pipeline handler for Intel IPU3
> + */
> +
> +#include <memory>
> +#include <vector>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
> +
> +#include "device_enumerator.h"
> +#include "log.h"
> +#include "media_device.h"
> +#include "pipeline_handler.h"
> +
> +namespace libcamera {
> +
> +class PipelineHandlerIPU3 : public PipelineHandler
> +{
> +public:
> +	PipelineHandlerIPU3();
> +	~PipelineHandlerIPU3();
> +
> +	bool match(CameraManager *manager, DeviceEnumerator *enumerator);
> +
> +private:
> +	MediaDevice *cio2_;
> +	MediaDevice *imgu_;
> +
> +	void registerCameras(CameraManager *manager);
> +};
> +
> +PipelineHandlerIPU3::PipelineHandlerIPU3()
> +	: cio2_(nullptr), imgu_(nullptr)
> +{
> +}
> +
> +PipelineHandlerIPU3::~PipelineHandlerIPU3()
> +{
> +	if (cio2_)
> +		cio2_->release();
> +
> +	if (imgu_)
> +		imgu_->release();
> +
> +	cio2_ = nullptr;
> +	imgu_ = nullptr;
> +}
> +
> +bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumerator)
> +{
> +	DeviceMatch cio2_dm("ipu3-cio2");
> +	cio2_dm.add("ipu3-csi2 0");
> +	cio2_dm.add("ipu3-cio2 0");
> +	cio2_dm.add("ipu3-csi2 1");
> +	cio2_dm.add("ipu3-cio2 1");
> +	cio2_dm.add("ipu3-csi2 2");
> +	cio2_dm.add("ipu3-cio2 2");
> +	cio2_dm.add("ipu3-csi2 3");
> +	cio2_dm.add("ipu3-cio2 3");
> +
> +	DeviceMatch imgu_dm("ipu3-imgu");
> +	imgu_dm.add("ipu3-imgu 0");
> +	imgu_dm.add("ipu3-imgu 0 input");
> +	imgu_dm.add("ipu3-imgu 0 parameters");
> +	imgu_dm.add("ipu3-imgu 0 output");
> +	imgu_dm.add("ipu3-imgu 0 viewfinder");
> +	imgu_dm.add("ipu3-imgu 0 3a stat");
> +	imgu_dm.add("ipu3-imgu 1");
> +	imgu_dm.add("ipu3-imgu 1 input");
> +	imgu_dm.add("ipu3-imgu 1 parameters");
> +	imgu_dm.add("ipu3-imgu 1 output");
> +	imgu_dm.add("ipu3-imgu 1 viewfinder");
> +	imgu_dm.add("ipu3-imgu 1 3a stat");
> +
> +	cio2_ = enumerator->search(cio2_dm);
> +	if (!cio2_)
> +		return false;
> +
> +	imgu_ = enumerator->search(imgu_dm);
> +	if (!imgu_)
> +		return false;
> +
> +	/*
> +	 * It is safe to acquire both media devices at this point as
> +	 * DeviceEnumerator::search() skips the busy ones for us.
> +	 */
> +	cio2_->acquire();
> +	imgu_->acquire();
> +
> +	/*
> +	 * Disable all links that are enabled by default on CIO2, as camera
> +	 * creation enables all valid links it finds.
> +	 *
> +	 * Close the CIO2 media device after, as links are enabled and should
> +	 * not need to be changed after.
> +	 */
> +	if (cio2_->open())
> +		goto error_release_mdev;
> +
> +	if (cio2_->disableLinks())
> +		goto error_close_cio2;
> +
> +	registerCameras(manager);
> +	LOG(Debug) << "\"Intel IPU3\" pipeline handler initialized";
> +
> +	cio2_->close();
> +
> +	return true;
> +
> +error_close_cio2:
> +	cio2_->close();
> +
> +error_release_mdev:
> +	cio2_->release();
> +	imgu_->release();
> +
> +	return false;
> +}
> +
> +/*
> + * Cameras are created associating an image sensor (represented by a
> + * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
> + * CIO2 CSI-2 receivers.
> + */
> +void PipelineHandlerIPU3::registerCameras(CameraManager *manager)
> +{
> +	const std::vector<MediaEntity *> entities = cio2_->entities();
> +	struct {
> +		unsigned int id;
> +		MediaEntity *csi2;
> +	} csi2Receivers[] = {
> +		{ 0, cio2_->getEntityByName("ipu3-csi2 0") },
> +		{ 1, cio2_->getEntityByName("ipu3-csi2 1") },
> +		{ 2, cio2_->getEntityByName("ipu3-csi2 2") },
> +		{ 3, cio2_->getEntityByName("ipu3-csi2 3") },
> +	};
> +
> +	/*
> +	 * For each CSI-2 receiver on the IPU3, create a Camera if an
> +	 * image sensor is connected to it.
> +	 */
> +	unsigned int numCameras = 0;
> +	for (auto csi2Receiver : csi2Receivers) {
> +		MediaEntity *csi2 = csi2Receiver.csi2;
> +		unsigned int id = csi2Receiver.id;
> +
> +		/*
> +		 * This shall not happen, as the device enumerator matched
> +		 * all entities described in the cio2_dm DeviceMatch.
> +		 *
> +		 * As this check is basically free, better stay safe than sorry.
> +		 */
> +		if (!csi2)
> +			continue;
> +
> +		std::vector<MediaPad *> pads = csi2->pads();
> +		MediaPad *sink;
> +		for (MediaPad *pad : pads) {
> +			if (!(pad->flags() & MEDIA_PAD_FL_SINK))
> +				continue;
> +
> +			/* IPU3 CSI-2 receivers have a single sink pad. */
> +			sink = pad;
> +			break;
> +		}
> +
> +		std::vector<MediaLink *> links = sink->links();
> +		if (links.empty())
> +			continue;
> +
> +		/*
> +		 * Verify that the receiver is connected to a sensor, enable
> +		 * the media link between the two, and create a Camera with
> +		 * a unique name.
> +		 *
> +		 * FIXME: This supports creating a single camera per CSI-2 receiver.
> +		 */
> +		for (MediaLink *link : links) {
> +			/* Again, this shall not happen, but better stay safe. */
> +			if (!link->source())
> +				continue;
> +
> +			MediaEntity *sensor = link->source()->entity();
> +			if (!sensor)
> +				continue;
> +
> +			if (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)
> +				continue;
> +
> +			if (link->setEnabled(true))
> +				continue;
> +
> +			std::size_t pos = sensor->name().find(" ");
> +			std::string cameraName = sensor->name().substr(0, pos);
> +			cameraName += " " + std::to_string(id);
> +
> +			std::shared_ptr<Camera> camera = Camera::create(cameraName);
> +			manager->addCamera(std::move(camera));


Should all Camera's be added to the manager? and if so - should that
happen inside Camera::create()?

I Realise that is not an IPU3 pipeline specific questions - so this is
just a place to ask the question...


How will we tie a Camera object to the links and sensors?

What does a Camera() need to know to operate on it's pipeline?
At least a reference to the PipelineHandler perhaps?

(Again - this is just an open question in my head - our Camera object is
very sparse at the moment, and I'm not expecting the implementation here
to change in this patch)


> +			LOG(Info) << "Registered Camera[" << numCameras
> +				  << "] \"" << cameraName << "\""
> +				  << " connected to CSI-2 receiver " << id;
> +
> +			numCameras++;
> +			break;
> +		}
> +	}
> +}
> +
> +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build
> new file mode 100644
> index 0000000..0ab766a
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/meson.build
> @@ -0,0 +1,3 @@
> +libcamera_sources += files([
> +    'ipu3.cpp',
> +])
> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> index 615ecd2..811c075 100644
> --- a/src/libcamera/pipeline/meson.build
> +++ b/src/libcamera/pipeline/meson.build
> @@ -1,3 +1,5 @@
>  libcamera_sources += files([
>      'vimc.cpp',
>  ])
> +
> +subdir('ipu3')
> 

-- 
Regards
--
Kieran