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

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

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

Hi Kieran,

On Mon, Jan 21, 2019 at 11:10:41AM +0000, Kieran Bingham wrote:
> 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.

Thanks, no worries
>
>
> 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@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()?

Yes to the first question, and imo no to the second one, as each
pipeline manager implementation shall be free to decide how to handle
the newly created shared_ptr ownership.

As you can see here, for now the pipeline handler is adding the camera
to the manager releasing its shared ownership. There will be cases where the
pipeline hanlder will want to retain that. True that it might be
indeed solved by tuning the lifetime of refernces to the shared object
created here, but sincerely, I don't see any benefit for that, but a
much more error-prone implementation.
>
> 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)
>

I think we'll have an answer as soon as we capture from a Camera :)

Thanks
  j

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

Hello,

On Mon, Jan 21, 2019 at 12:28:49PM +0100, Jacopo Mondi wrote:
>  On Mon, Jan 21, 2019 at 11:10:41AM +0000, Kieran Bingham wrote:
> > 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@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();

I think the MediaDevice should handle the acquire/release automatically.
This is unrelated to this patch, just something to keep in mind,
possible using some kind of smart pointer. We'll likely need
std::shared_ptr<MediaDevice> to handle hotplugging, and possible a
MediaDeviceReference class that would wrap the shared pointer with
automatic acquire/release the same way std::unique_ptr<> does.

> >> +	/*
> >> +	 * 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";

We already print in a message in CameraManager::start() when match()
returns true, do we need another one here ?

> >> +	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();

s/entities/&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;

Maybe you could simplify this with

	for (unsigned int id = 0; id < 4; ++id) {
		std::string csi2Name = "ipu3-csi2 " + std::to_string(i);
		MediaEntity *csi2 = cio2_->getEntityByName(csi2Name);

and remove the csi2Receivers array. Or maybe that's more complex :-)

> >> +
> >> +		/*
> >> +		 * 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();

s/std/const std/
s/pads/&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;
> >> +		}

Isn't there a guarantee that the sink pad is pad 0 ? You could the write
this

		MediaPad *sink = csi->pads()[0];

or, with the error check,

		const std::vector<MediaPad *> &pads = csi2->pads();
		if (pads.empty())
			continue;
		MediaPad *sink = pads[0];

> >> +
> >> +		std::vector<MediaLink *> links = sink->links();

s/std/const std/
s/links/&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) {

As we only support one link, how about just

		const std::vector<MediaLink *> links = sink->links();
		if (links.empty())
			continue;
		MediaLink *link = links[0];

> >> +			/* Again, this shall not happen, but better stay safe. */
> >> +			if (!link->source())
> >> +				continue;
> >> +
> >> +			MediaEntity *sensor = link->source()->entity();
> >> +			if (!sensor)
> >> +				continue;

Can this really happen ? It sounds even less likely than
!link->source(). With each check we're taking one more step towards the
impossible territoty :-) I think that even !link->source() is not
needed, as it would denote a serious bug in the media device handling,
and we should have test cases to catch this kind of issues.

> >> +			if (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)
> >> +				continue;

I wonder if a helper function in MediaOad that returns the first link to
an entity of a given function would be useful. An additional check that
could be added there would be for !IMMUTABLE || ENABLED to ignore links
that can't be enabled (pretty unlikely though, but this condition would
likely not be tested by pipeline handlers if it happened).

> >> +
> >> +			if (link->setEnabled(true))
> >> +				continue;
> >> +
> >> +			std::size_t pos = sensor->name().find(" ");
> >> +			std::string cameraName = sensor->name().substr(0, pos);

I assume you want to strip the I2C bus number, but what if the sensor
name itself contains a space ?

> >> +			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()?
>  
>  Yes to the first question, and imo no to the second one, as each
>  pipeline manager implementation shall be free to decide how to handle
>  the newly created shared_ptr ownership.
>  
>  As you can see here, for now the pipeline handler is adding the camera
>  to the manager releasing its shared ownership. There will be cases where the
>  pipeline hanlder will want to retain that. True that it might be
>  indeed solved by tuning the lifetime of refernces to the shared object
>  created here, but sincerely, I don't see any benefit for that, but a
>  much more error-prone implementation.

I would also add that a pipeline manager may well want to perform
further initialization on the camera before it registers it with the
camera manager, and it may as well want to create all cameras and
initialize them all before it registers any of them.

> > 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)
>  
>  I think we'll have an answer as soon as we capture from a Camera :)
>  
> >> +			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')
> >>