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

231 diff mbox series

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

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@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo@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')
>

Hi Kieran,

On Tue, Jan 15, 2019 at 03:15:23PM +0000, Kieran Bingham wrote:
> 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@ideasonboard.com>
>
> > Signed-off-by: Jacopo Mondi <jacopo@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.

As you found out below, I shall not delete MediaDevices from here.

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

Cameras_' a std::map, just look at the type of the template.
I can add a small comments, even if I have commented extensively on
the indexing of cameras in the class..

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

Lifetime management of cameras is not IPU3 specific. The use of a map
to index cameras according to their CSI-2 receiver indexes (so that
camera connected to CSI-2 0 always comes first than camera connected
to CSI-2 3) is specific of this implementation. If that does not get
changed after this review round, the '.second' stays.

I could:
        Camera *cam = camera->second;

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

I cut off the bus address, if we keep that the name would be unique
indeed. Otherwise yes, two sensors with the same name would give two
cameras with the same name -> very bad. This needs to be changed, but
instead of the bus address I would use the CSI-2 receiver index to
form a unique name...

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

Hi Jacopo,

On 15/01/2019 17:37, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Tue, Jan 15, 2019 at 03:15:23PM +0000, Kieran Bingham wrote:
>> 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@ideasonboard.com>
>>
>>> Signed-off-by: Jacopo Mondi <jacopo@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.
> 
> As you found out below, I shall not delete MediaDevices from here.
> 
>>
>>
>>> +
>>> +	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 ?
>>
> 
> Cameras_' a std::map, just look at the type of the template.
> I can add a small comments, even if I have commented extensively on
> the indexing of cameras in the class..

So I'm still really confused by the naming.

Why is it called a second? Does it represent a period of time? Is it a
second instance of the camera? Why does the camera need a second instance...


OH. *it's not a libcamera::Camera*


You know what -  I think I'm afraid I've just validated Laurent's
reasoning not to use auto.


I read: for (auto camera : cameras_) { ... as
	'for each "libcamera::Camera" in cameras_ ... so I had no idea what a
'second' was.

But we're not dealing with libcamera::Camera here - we're dealing with a
std::pair from the map iterator.

I fear that's really not clear without naming the type here...



>>
>>> +		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.
>>
> 
> Lifetime management of cameras is not IPU3 specific. The use of a map
> to index cameras according to their CSI-2 receiver indexes (so that
> camera connected to CSI-2 0 always comes first than camera connected
> to CSI-2 3) is specific of this implementation. If that does not get
> changed after this review round, the '.second' stays.
> 
> I could:
>         Camera *cam = camera->second;

Yes - that would make things a lot clearer too.
Perhaps we could also rename the iterator?


> 
>>> +	}
>>> +
>>> +	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.
>>
> 
> I cut off the bus address, if we keep that the name would be unique
> indeed. Otherwise yes, two sensors with the same name would give two
> cameras with the same name -> very bad. This needs to be changed, but
> instead of the bus address I would use the CSI-2 receiver index to
> form a unique name...

I'm not too worried here at the moment.
We can add in assertions to make sure we don't add a camera name which
already exists, and we can work out a unique naming later because it's
not going to be an ABI string. It's just a representation.

Might be worth adding a "Make sure camera names must be unique" task to
our task list though.



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