[libcamera-devel] [PATCH 4/4] libcamera: pipeline: Add Intel IPU3 pipeline
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 15 18:37:55 CET 2019
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..
>
> > + 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
-------------- 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/20190115/f93a52c7/attachment-0001.sig>
More information about the libcamera-devel
mailing list