[libcamera-devel] [PATCH 2/2] libcamera: ipu3: Create CIO2 V4L2 devices
Jacopo Mondi
jacopo at jmondi.org
Fri Jan 25 16:51:47 CET 2019
HI Laurent,
On Fri, Jan 25, 2019 at 05:31:09PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jan 24, 2019 at 12:30:20PM +0100, Jacopo Mondi wrote:
> > Create V4L2 devices for the CIO2 capture video nodes and associate them
> > with the camera they are part of.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++
> > 1 file changed, 50 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 8cbc10a..9f5a40f 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -15,6 +15,8 @@
> > #include "log.h"
> > #include "media_device.h"
> > #include "pipeline_handler.h"
> > +#include "utils.h"
> > +#include "v4l2_device.h"
> >
> > namespace libcamera {
> >
> > @@ -29,9 +31,19 @@ public:
> > bool match(CameraManager *manager, DeviceEnumerator *enumerator);
> >
> > private:
> > + class IPU3CameraData : public CameraData
> > + {
> > + public:
> > + IPU3CameraData()
> > + : dev_(nullptr) { }
> > + ~IPU3CameraData() { delete dev_; }
> > + V4L2Device *dev_;
>
> You will soon need to add data to this, so I wouldn't inline the
As this is for a follow-up patch, let's do that once it is required
> constructor and destructor. As the class may also get used by other
> compilation units later on I'd define it outside of the
> PipelineHandlerIPU3 class, to make it easier to later move it to a
> header if necessary.
As this is for a follow-up patch, let's do that once it is required
once (if) we split the IPU3 handler in multiple source files.
>
> > + };
> > +
> > MediaDevice *cio2_;
> > MediaDevice *imgu_;
> >
> > + V4L2Device *createVideoDevice(unsigned int id);
> > void registerCameras(CameraManager *manager);
> > };
> >
> > @@ -122,6 +134,24 @@ error_release_mdev:
> > return false;
> > }
> >
> > +/* Create video devices for the CIO2 unit associated with a camera. */
> > +V4L2Device *PipelineHandlerIPU3::createVideoDevice(unsigned int id)
> > +{
> > + std::string cio2Name = "ipu3-cio2 " + std::to_string(id);
> > + MediaEntity *cio2 = cio2_->getEntityByName(cio2Name);
> > + if (!cio2)
> > + return nullptr;
> > +
> > + V4L2Device *dev = new V4L2Device(*cio2);
> > + if (dev->open()) {
> > + delete dev;
> > + return nullptr;
> > + }
> > + dev->close();
> > +
> > + return dev;
> > +}
> > +
> > /*
> > * Cameras are created associating an image sensor (represented by a
> > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
> > @@ -172,6 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)
> >
> > std::string cameraName = sensor->name() + " " + std::to_string(id);
> > std::shared_ptr<Camera> camera = Camera::create(cameraName);
> > +
> > + setCameraData(camera.get(),
> > + std::move(utils::make_unique<IPU3CameraData>()));
> > + IPU3CameraData *data =
> > + reinterpret_cast<IPU3CameraData *>(cameraData(camera.get()));
>
> As I expect you'll need to call this quite often, how about creating a
>
> IPU3CameraData *cameraData(const Camera *);
>
> function in the PipelineHandlerIPU3 class ?
>
As this is for a follow-up patch, let's do that once it is required
> > +
> > + /*
> > + * If V4L2 device creation fails, the Camera instance won't be
> > + * registered. The 'camera' shared pointer goes out of scope
> > + * and deletes the Camera it manages.
> > + */
> > + V4L2Device *videoDev = createVideoDevice(id);
> > + if (!videoDev) {
> > + LOG(IPU3, Error)
> > + << "Failed to register camera["
> > + << numCameras << "] \"" << cameraName << "\"";
> > + continue;
>
> If this fails the camera will be deleted (as the sole reference will the
> the shared pointer local to the loop), but the shared data will stay.
> It's no big deal, it will be unused and be deleted when the pipeline
> handler is deleted, but that still bothers me a bit. I wonder whether we
> shouldn't call setCameraData() right before manager->addCamera()
> instead.
>
I'll reply to this in the other comment, as this is not what has been
merged.
Thanks
j
> > + }
> > +
> > + data->dev_ = videoDev;
> > manager->addCamera(std::move(camera));
> >
> > LOG(IPU3, Info)
>
> --
> Regards,
>
> Laurent Pinchart
-------------- 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/20190125/235d9288/attachment.sig>
More information about the libcamera-devel
mailing list