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

[Kieran Bingham]

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

-- 
Regards
--
Kieran