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

[Jacopo Mondi]

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 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()?

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190121/0d2d52af/attachment.sig>