| Message ID | 20190124113020.7203-3-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. 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@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); 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) > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas thanks for review On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > 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@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? Thanks j > > 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) > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote: > Hi Niklas > thanks for review > > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote: > > Hi Jacopo, > > > > Thanks for your work. > > > > 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@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? Thanks for the explanation! This looks good to me, expect you are missing 'data->dev_ = videoDev' but I assume that is not critical to your demonstration ;-) With this fix, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Thanks > j > > > > > 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) > > > -- > > > 2.20.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund
On Thu, Jan 24, 2019 at 09:03:54PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote: > > Hi Niklas > > thanks for review > > > > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote: > > > Hi Jacopo, > > > > > > Thanks for your work. > > > > > > 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@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? > > Thanks for the explanation! This looks good to me, expect you are > missing 'data->dev_ = videoDev' but I assume that is not critical to > your demonstration ;-) With this fix, I added it to the constructor, sorry, it's not shown here. Want me to remove that? > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > Thanks > > j > > > > > > > > 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) > > > > -- > > > > 2.20.1 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > > -- > Regards, > Niklas Söderlund
On 2019-01-24 21:12:49 +0100, Jacopo Mondi wrote: > On Thu, Jan 24, 2019 at 09:03:54PM +0100, Niklas Söderlund wrote: > > Hi Jacopo, > > > > On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote: > > > Hi Niklas > > > thanks for review > > > > > > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote: > > > > Hi Jacopo, > > > > > > > > Thanks for your work. > > > > > > > > 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@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? > > > > Thanks for the explanation! This looks good to me, expect you are > > missing 'data->dev_ = videoDev' but I assume that is not critical to > > your demonstration ;-) With this fix, > > I added it to the constructor, sorry, it's not shown here. > Want me to remove that? I leave that up to you as the future IPU3 pipeline master :-) > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > > > Thanks > > > j > > > > > > > > > > > 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) > > > > > -- > > > > > 2.20.1 > > > > > > > > > > _______________________________________________ > > > > > libcamera-devel mailing list > > > > > libcamera-devel@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > -- > > > > Regards, > > > > Niklas Söderlund > > > > > > > > -- > > Regards, > > Niklas Söderlund
Hi Niklas, On Thu, Jan 24, 2019 at 09:17:53PM +0100, Niklas Söderlund wrote: > On 2019-01-24 21:12:49 +0100, Jacopo Mondi wrote: > > On Thu, Jan 24, 2019 at 09:03:54PM +0100, Niklas Söderlund wrote: > > > Hi Jacopo, > > > > > > On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote: > > > > Hi Niklas > > > > thanks for review > > > > > > > > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote: > > > > > Hi Jacopo, > > > > > > > > > > Thanks for your work. > > > > > > > > > > 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@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? > > > > > > Thanks for the explanation! This looks good to me, expect you are > > > missing 'data->dev_ = videoDev' but I assume that is not critical to > > > your demonstration ;-) With this fix, > > > > I added it to the constructor, sorry, it's not shown here. > > Want me to remove that? > > I leave that up to you as the future IPU3 pipeline master :-) > I happly abdicate from this, but in the meantime I have pushed these patches. I dropped the v4l2 device as constructor parameter, as it will unlikely be the only per-camera object we'll have to store. Thanks j > > > > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > > > > > > Thanks > > > > j > > > > > > > > > > > > > > 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) > > > > > > -- > > > > > > 2.20.1 > > > > > > > > > > > > _______________________________________________ > > > > > > libcamera-devel mailing list > > > > > > libcamera-devel@lists.libcamera.org > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > > > -- > > > > > Regards, > > > > > Niklas Söderlund > > > > > > > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > > -- > Regards, > Niklas Söderlund
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@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 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. > + }; > + > 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 ? > + > + /* > + * 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. > + } > + > + data->dev_ = videoDev; > manager->addCamera(std::move(camera)); > > LOG(IPU3, Info)
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@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)
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@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
Hi Laurent, On Fri, Jan 25, 2019 at 05:36:12PM +0200, Laurent Pinchart wrote: > 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@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)); > Ok, but why is it better? I see an allocation which in case of failure in creating the video device could be skipped. Thanks j > > > 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
Hi Jacopo, On Fri, Jan 25, 2019 at 04:51:47PM +0100, Jacopo Mondi wrote: > On Fri, Jan 25, 2019 at 05:31:09PM +0200, Laurent Pinchart wrote: > > 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@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. I'm OK with that. > >> + }; > >> + > >> 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 This however I would fix already, to make sure the IPU3 pipeline handler follows all the best practices and can be used as an example. > >> + > >> + /* > >> + * 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. > > >> + } > >> + > >> + data->dev_ = videoDev; > >> manager->addCamera(std::move(camera)); > >> > >> LOG(IPU3, Info)
Hi Jacopo, On Fri, Jan 25, 2019 at 04:54:58PM +0100, Jacopo Mondi wrote: > On Fri, Jan 25, 2019 at 05:36:12PM +0200, Laurent Pinchart wrote: > > 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@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)); > > > > Ok, but why is it better? I see an allocation which in case of failure > in creating the video device could be skipped. First we allocate data as a unique pointer, so we know it will be either given to setCameraData() or deleted. No risk of leak. Then we create the video device and store it directly in the camera data structure, so we know it will be deleted with the camera. No risk of leak. If additional initialization was needed, it would follow at this point. Then we call setCameraData() at the end, when we don't need the data anymore, which avoids calling cameraData() in here to get a borrowed reference to something we just gave away (as in your original version). Finally we register the camera. The setCameraData() and addCamera() calls could even be moved to a PipelineHandler::addCamera(std::unique_ptr<Camera> camera, std::unique_ptr<CameraData> data) function that would do both, simplifying the API for pipeline handlers. > >>> 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)
Hi Laurent, On Fri, Jan 25, 2019 at 06:08:05PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Jan 25, 2019 at 04:54:58PM +0100, Jacopo Mondi wrote: > > On Fri, Jan 25, 2019 at 05:36:12PM +0200, Laurent Pinchart wrote: > > > 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@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)); > > > > > > > Ok, but why is it better? I see an allocation which in case of failure > > in creating the video device could be skipped. > > First we allocate data as a unique pointer, so we know it will be either > given to setCameraData() or deleted. No risk of leak. Then we create the > video device and store it directly in the camera data structure, so we > know it will be deleted with the camera. No risk of leak. If additional > initialization was needed, it would follow at this point. Then we call > setCameraData() at the end, when we don't need the data anymore, which > avoids calling cameraData() in here to get a borrowed reference to > something we just gave away (as in your original version). Finally we > register the camera. The setCameraData() and addCamera() calls could > even be moved to a PipelineHandler::addCamera(std::unique_ptr<Camera> > camera, std::unique_ptr<CameraData> data) function that would do both, > simplifying the API for pipeline handlers. > ok Thanks j
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())); + + /* + * 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)
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@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)