[libcamera-devel] [PATCH v2 5/5] libcamera: pipeline: Add Intel IPU3 pipeline
Jacopo Mondi
jacopo at jmondi.org
Wed Jan 16 17:39:40 CET 2019
Hi Niklas,
On Wed, Jan 16, 2019 at 04:47:29PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for the updated version.
>
> On 2019-01-16 14:59:49 +0100, 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 | 263 ++++++++++++++++++++++++
> > src/libcamera/pipeline/ipu3/meson.build | 3 +
> > src/libcamera/pipeline/meson.build | 2 +
> > 3 files changed, 268 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..066e9da
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -0,0 +1,263 @@
> > +/* 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_;
>
> Is this member needed? Can't you retrieve the number of items in
> camerasMap_ container?
I could, I don't see any benefit if not optimizing away a integer
variable.
>
> > + std::map<unsigned int, Camera *> camerasMap_;
>
> Does this need to be a map? Can't it be a vector instead? Looking at the
> usage you loop over the elements anyhow.
>
As the comments suggest, if this is a vector, the registration order
depends on the entity enumeration order, which might change between
driver versions, the probing order, and I suspect even through reboots
if there are timeouts involved in probing sensors.
Using a map makes sure the element are sorted according to their keys
value, and in this case I'm keying them on the CSI-2 receiver index
they are connected to.
> This is not a big deal as the camera registration is to be reworked and
> the count() and camera() functions will go away and the camera will
> register it self with the Camera manager.
>
I think the same problems of ordering cameras we have here will be
present again even if the issue is moved to the camera manager.
> > +
> > + unsigned int registerCameras();
> > +};
> > +
> > +PipelineHandlerIPU3::PipelineHandlerIPU3()
> > + : cio2_(nullptr), imgu_(nullptr), numCameras_(0)
> > +{
> > +}
> > +
> > +PipelineHandlerIPU3::~PipelineHandlerIPU3()
> > +{
> > + if (cio2_)
> > + cio2_->release();
> > +
> > + if (imgu_)
> > + imgu_->release();
> > +
> > + for (auto map : camerasMap_) {
> > + Camera *camera = map.second;
> > + if (camera)
> > + camera->put();
> > + }
> > +
> > + cio2_ = nullptr;
> > + imgu_ = nullptr;
> > + camerasMap_.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 'camerasMap_' 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 = camerasMap_.begin(); it != camerasMap_.end(); ++it, --id) {
> > + if (id == 0)
> > + return (*it).second;
> > + }
>
> If you keep camerasMap_ as a map I think you should use any of the
> std::map helpers to resolve the id (which is the key in the map) to the
> camera (the value).
>
Not sure. As the comment explains, the n-th camera in the system is
not always in position map[n].
That's why I have to iterate on map entries unregardles of theit key
value.
Eg.
CSI-2 index Camera#
0 0
1 -
2 1
3 -
When asked for camera(1) the pipeline manager will give the camera
connected to CSI-2 receiver 2.
Again, that's meaningful order, consistent through driver versions
(and reboots, if that's an issue) that only depends on the device.
Question: hot-plug/hot-remove
Suppose we can hot-plug a camera to CSI-2 receiver #1. What now? What
was camera(1) is now camera(2)... So this approach as surely issues
with that.
I also think the big question here is: how would cameras be identified
by applications? By name? By index?
> > +
> > + 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");
> > +
> > + 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;
> > +
> > + numCameras_ = registerCameras();
> > + LOG(Debug) << "\"Intel IPU3\" pipeline handler initialized with "
> > + << numCameras_ << " cameras registered";
> > +
> > + 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.
> > + *
> > + * Cameras are here created and stored in the member field 'camerasMap_' 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.
>
> Is keeping order between the cameras a issue? As we will support
> hot-plug/unplug the over all order of cameras if one looks from the
> applications point of view can be different anyhow between two listings.
>
> Maybe we should sort all cameras alphabetically before handing over a
> list to the application to provide some sort of consistency in list
> order?
>
Back to the big question then: how will cameras be identified by
applications?
> > + *
> > + * 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;
> > +
> > + /*
> > + * 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);
> > +
> > + camerasMap_[id] = new Camera(cameraName);
> > +
> > + LOG(Info) << "Registered Camera[" << numCameras
> > + << "] \"" << cameraName << "\""
> > + << " connected to CSI-2 receiver " << id;
> > +
> > + numCameras++;
> > + break;
> > + }
> > + }
> > +
> > + return numCameras;
> > +}
> > +
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3, "Intel IPU3 pipeline handler");
> > +
> > +} /* 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')
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- 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/20190116/a656952b/attachment.sig>
More information about the libcamera-devel
mailing list