[libcamera-devel] [PATCH 2/2] libcamera: ipu3: Create CIO2 V4L2 devices
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jan 25 16:36:12 CET 2019
Hi Jacopo,
On Thu, Jan 24, 2019 at 08:33:00PM +0100, Jacopo Mondi wrote:
> On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:
> > On 2019-01-24 12:30:20 +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_;
> >> + };
> >> +
> >> 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()));
> >
> > I'm not saying this is not needed, only that it looks a bit complex to
> > my feeble mind. Could you educate me why the following would not work?
> >
> > IPU3CameraData *data = new IPU3CameraData();
> > data->dev_ = videoDev;
> >
> > setCameraData(camera.get(), data);
>
> setCameraData wants a unique_ptr. On the reason why we're passing it
> by value (hence the std::move() ) instead than by reference and move()
> inside the function see the discussion on v1. Basically, it makes
> clear that after setCameraData() the local reference it's not
> valid anymore.
>
> That said, I could indeed have created a unique_ptr<> from an already
> existing reference, instead of using std::make_unique.
>
> What I have now looks like the following:
>
> V4L2Device *videoDev = createVideoDevice(id);
> if (!videoDev) {
> LOG(IPU3, Error)
> << "Failed to register camera["
> << numCameras << "] \"" << cameraName << "\"";
> continue;
> }
>
> IPU3CameraData *data = new IPU3CameraData(*videoDev);
> setCameraData(camera.get(),
> std::move(std::unique_ptr<IPU3CameraData>(data)));
> manager->addCamera(std::move(camera));
>
> Which is in my opinion nicer and equally safe. Thanks a lot for pointing
> this out. Is it any better?
How about
std::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>();
data->dev_ = createVideoDevice(id);
if (!data->dev_) {
LOG(IPU3, Error)
<< "Failed to register camera["
<< numCameras << "] \"" << cameraName << "\"";
continue;
}
setCameraData(camera.get(), std::move(data));
manager->addCamera(std::move(camera));
> > Apart from this I think this commit looks good.
> >
> >> +
> >> + /*
> >> + * 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;
> >> + }
> >> +
> >> + data->dev_ = videoDev;
> >> manager->addCamera(std::move(camera));
> >>
> >> LOG(IPU3, Info)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list