[libcamera-devel] [PATCH 4/4] libcamera: pipeline: Add Intel IPU3 pipeline

[Kieran Bingham]

Hi Jacopo,

On 15/01/2019 14:07, 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.
> 

This looks like a really good start, and I can start to understand how
the matching is going on now.

Aside from some discussion below I don't think there's anything really
major to block this IMO.

I left in my text about putting ->release in the destructor in case it
was actually relevant - but I think reading further down it became clear
that what I was suggesting is not a viable structure to the code.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp    | 249 ++++++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/meson.build |   3 +
>  src/libcamera/pipeline/meson.build      |   2 +
>  3 files changed, 254 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..4c24c79
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -0,0 +1,249 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipu3.cpp - Pipeline handler for Intel IPU3
> + */
> +
> +#include <map>
> +#include <vector>
> +
> +#include <libcamera/camera.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(DeviceEnumerator *enumerator);
> +
> +	unsigned int count();
> +	Camera *camera(unsigned int id) final;
> +
> +private:
> +	MediaDevice *cio2_;
> +	MediaDevice *imgu_;
> +
> +	unsigned int numCameras_;
> +	std::map<unsigned int, Camera *> cameras_;
> +
> +	unsigned int registerCameras();
> +};
> +
> +PipelineHandlerIPU3::PipelineHandlerIPU3()
> +	: cio2_(nullptr), imgu_(nullptr), numCameras_(0)
> +{
> +}
> +
> +PipelineHandlerIPU3::~PipelineHandlerIPU3()
> +{
> +	if (cio2_)
> +		cio2_->release();

Does the MediaDevice destructor not do this call to ->release()?
 (And should it?)

	<edit - I read below... :) >

Then these two would just be:

	delete cio2_;
	delete imgu_;

BTW: I'm not objecting to a direct call to release() here - just
considering options.


> +
> +	if (imgu_)
> +		imgu_->release();
> +
> +	for (auto camera : cameras_) {

Reading here - it's not clear what a camera.second is?
I'll go dig in the code - but as it's not obvious (unless I'm missing
something obvious) - perhaps it needs some description ?


> +		if (!camera.second)
> +			continue;
> +
> +		camera.second->put();
> +
> +		/*
> +		 * FIXME
> +		 * The lifetime management of Camera instances will be
> +		 * soon changed: as of now, the handler creates cameras, and
> +		 * -shall- destroy them as well to avoid leaks.
> +		 */
> +		delete camera.second;

Although - perhaps this '.second' is going to disappear - so it doesn't
matter.

> +	}
> +
> +	cio2_ = nullptr;
> +	imgu_ = nullptr;
> +	cameras_.clear();
> +}
> +
> +unsigned int PipelineHandlerIPU3::count()
> +{
> +	return numCameras_;
> +}
> +
> +Camera *PipelineHandlerIPU3::camera(unsigned int id)
> +{
> +	if (id >= numCameras_)
> +		return nullptr;
> +
> +	/*
> +	 * The requested camera id does not match the key index used to store
> +	 * Camera instances in the 'cameras_' map.
> +	 *
> +	 * The 'id' argument represent the n-th valid cameras registered
> +	 * in the system, while the indexing key is the CSI-2 receiver index
> +	 * the camera sensor is associated to, and some receiver might have no
> +	 * camera sensor connected.
> +	 */
> +	for (auto it = cameras_.begin(); it != cameras_.end(); ++it, --id) {
> +		if (id == 0)
> +			return (*it).second;
> +	}
> +
> +	return nullptr;
> +}
> +
> +bool PipelineHandlerIPU3::match(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");
> +
> +	cio2_ = enumerator->search(cio2_dm);
> +	if (!cio2_)
> +		return false;
> +
> +	cio2_->acquire();

Aha - never mind the above comments - We don't own the object to delete
it ... nor should it be deleted above :)

> +
> +	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");
> +
> +	imgu_ = enumerator->search(imgu_dm);
> +	if (!imgu_) {
> +		cio2_->release();
> +		return false;
> +	}
> +
> +	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()) {
> +		cio2_->release();
> +		imgu_->release();
> +		return false;
> +	}
> +	cio2_->disableLinks();
> +
> +	numCameras_ = registerCameras();
> +	LOG(Debug) << "\"Intel IPU3\" pipeline handler initialized with "
> +		   << numCameras_ << " cameras registered";
> +
> +	cio2_->close();
> +
> +	return true;
> +}
> +
> +/*
> + * 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.
> + *
> + * Cameras are here created and stored in the member field 'cameras_' map,
> + * indexed by the id of the CSI-2 receiver they are connected, to maintain
> + * an ordering that does not depend on the device enumeration order.
> + *
> + * The function returns the number of cameras found in the system.
> + */
> +unsigned int PipelineHandlerIPU3::registerCameras()
> +{
> +	const std::vector<MediaEntity *> entities = cio2_->entities();
> +	unsigned int numCameras = 0;
> +	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.
> +	 */
> +	for (auto csi2Receiver : csi2Receivers) {
> +		MediaEntity *csi2 = csi2Receiver.csi2;
> +		unsigned int id = csi2Receiver.id;
> +
> +		/* IPU3 CSI-2 receivers have a single sink pad. */
> +		std::vector<MediaPad *> pads = csi2->pads();
> +		MediaPad *sink;
> +		for (MediaPad *pad : pads) {
> +			if (!(pad->flags() & MEDIA_PAD_FL_SINK))
> +				continue;
> +
> +			sink = pad;
> +			break;
> +		}
> +
> +		/* Verify that the receiver is connected to a sensor. */
> +		std::vector<MediaLink *> links = sink->links();
> +		if (links.empty())
> +			continue;
> +
> +		/*
> +		 * FIXME
> +		 * This supports creating a single camera per CSI-2 receiver.
> +		 */
> +		for (MediaLink *link : links) {
> +			MediaEntity *sensor = link->source()->entity();
> +			if (!sensor)
> +				continue;
> +
> +			if (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)
> +				continue;
> +
> +			/* Enable the link between sensor and CSI-2 receiver. */
> +			if (link->setEnabled(true))
> +				continue;
> +
> +			/* Create the camera using the sensor name. */
> +			std::size_t pos = sensor->name().find(" ");
> +			std::string cameraName = sensor->name().substr(0, pos);

I presume we'll have to make sure that this string is unique in the
system somewhere / somehow - but for now this is a good way to map the
cameras.

> +
> +			cameras_[id] = new Camera(cameraName);
> +
> +			LOG(Debug) << "Registered Camera[" << numCameras
> +				   << "] \"" << cameraName << "\""
> +				   << " connected to CSI-2 receiver " << id;
> +
> +			numCameras++;
> +			break;
> +		}
> +	}
> +
> +	return numCameras;
> +}
> +
> +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