| Message ID | 20190122181225.12922-3-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Jan 22, 2019 at 07:12:25PM +0100, Jacopo Mondi wrote: > Create V4L2 devices for the CIO2 capture video nodes. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 42 ++++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 8cbc10a..58053ea 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -15,11 +15,25 @@ > #include "log.h" > #include "media_device.h" > #include "pipeline_handler.h" > +#include "v4l2_device.h" > > namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > +class IPU3CameraData : public CameraData > +{ > +public: > + IPU3CameraData() : dev_(nullptr) { } > + ~IPU3CameraData() { delete dev_; } > + > + void setV4L2Device(V4L2Device *dev) { dev_ = dev; } > + V4L2Device *device() const { return dev_; } > + As this class is only used internally by the IPU3 pipeline manager I would just make dev_ public and remove the accessors, especially given that you implement both direct read and write without any side effect. > +private: > + V4L2Device *dev_; > +}; > + > class PipelineHandlerIPU3 : public PipelineHandler > { > public: > @@ -32,6 +46,7 @@ private: > MediaDevice *cio2_; > MediaDevice *imgu_; > > + V4L2Device *createVideoDevice(unsigned int id); > void registerCameras(CameraManager *manager); > }; > > @@ -122,6 +137,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(); Unrelated to this patch, I wonder if we should have a V4L2Device::init() method that would perform the open + 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 +205,15 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager) > > std::string cameraName = sensor->name() + " " + std::to_string(id); > std::shared_ptr<Camera> camera = Camera::create(cameraName); > + > + /* Store pipeline private data in the Camera object. */ > + V4L2Device *videoDev = createVideoDevice(id); > + if (videoDev) { > + IPU3CameraData *ipu3Data = new IPU3CameraData(); > + ipu3Data->setV4L2Device(videoDev); > + camera->setCameraData(ipu3Data); > + } > + I think you can do it the other way around, creating the camera data first, and then the V4L2 device, as you will have more than just a V4L2 device to associate with the camera. camera->setCameraData(std::move(utils::make_unique<IPU3CameraData>())); IPU3CameraData *data = camera->cameraData(); V4L2Device *videoDev = createVideoDevice(id); if (!videoDev) { /* Error handling */ } data->videoDev = createVideoDevice(id); > manager->addCamera(std::move(camera)); > > LOG(IPU3, Info)
Hi Laurent, On Wed, Jan 23, 2019 at 12:36:30PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Jan 22, 2019 at 07:12:25PM +0100, Jacopo Mondi wrote: > > Create V4L2 devices for the CIO2 capture video nodes. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 42 ++++++++++++++++++++++++++++ > > 1 file changed, 42 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 8cbc10a..58053ea 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -15,11 +15,25 @@ > > #include "log.h" > > #include "media_device.h" > > #include "pipeline_handler.h" > > +#include "v4l2_device.h" > > > > namespace libcamera { > > > > LOG_DEFINE_CATEGORY(IPU3) > > > > +class IPU3CameraData : public CameraData > > +{ > > +public: > > + IPU3CameraData() : dev_(nullptr) { } > > + ~IPU3CameraData() { delete dev_; } > > + > > + void setV4L2Device(V4L2Device *dev) { dev_ = dev; } > > + V4L2Device *device() const { return dev_; } > > + > > As this class is only used internally by the IPU3 pipeline manager I > would just make dev_ public and remove the accessors, especially given > that you implement both direct read and write without any side effect. > > > +private: > > + V4L2Device *dev_; > > +}; > > + > > class PipelineHandlerIPU3 : public PipelineHandler > > { > > public: > > @@ -32,6 +46,7 @@ private: > > MediaDevice *cio2_; > > MediaDevice *imgu_; > > > > + V4L2Device *createVideoDevice(unsigned int id); > > void registerCameras(CameraManager *manager); > > }; > > > > @@ -122,6 +137,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(); > > Unrelated to this patch, I wonder if we should have a V4L2Device::init() > method that would perform the open + 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 +205,15 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager) > > > > std::string cameraName = sensor->name() + " " + std::to_string(id); > > std::shared_ptr<Camera> camera = Camera::create(cameraName); > > + > > + /* Store pipeline private data in the Camera object. */ > > + V4L2Device *videoDev = createVideoDevice(id); > > + if (videoDev) { > > + IPU3CameraData *ipu3Data = new IPU3CameraData(); > > + ipu3Data->setV4L2Device(videoDev); > > + camera->setCameraData(ipu3Data); > > + } > > + > > I think you can do it the other way around, creating the camera data > first, and then the V4L2 device, as you will have more than just a V4L2 > device to associate with the camera. > > camera->setCameraData(std::move(utils::make_unique<IPU3CameraData>())); > IPU3CameraData *data = camera->cameraData(); > > V4L2Device *videoDev = createVideoDevice(id); > if (!videoDev) { > /* Error handling */ > } > > data->videoDev = createVideoDevice(id); Is this intentional? Why Call createVideoDevice() twice? Can't I just re-use videoDev? By the way, my intention was originally to skip creating pipeline data and associate them at all, if videoDev creation fails. In future we might have more data, but for now we don't. I will change it anyway. Thanks j > > > manager->addCamera(std::move(camera)); > > > > LOG(IPU3, Info) > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Wed, Jan 23, 2019 at 03:26:51PM +0100, Jacopo Mondi wrote: > On Wed, Jan 23, 2019 at 12:36:30PM +0200, Laurent Pinchart wrote: > > On Tue, Jan 22, 2019 at 07:12:25PM +0100, Jacopo Mondi wrote: > >> Create V4L2 devices for the CIO2 capture video nodes. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 42 ++++++++++++++++++++++++++++ > >> 1 file changed, 42 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 8cbc10a..58053ea 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -15,11 +15,25 @@ > >> #include "log.h" > >> #include "media_device.h" > >> #include "pipeline_handler.h" > >> +#include "v4l2_device.h" > >> > >> namespace libcamera { > >> > >> LOG_DEFINE_CATEGORY(IPU3) > >> > >> +class IPU3CameraData : public CameraData > >> +{ > >> +public: > >> + IPU3CameraData() : dev_(nullptr) { } > >> + ~IPU3CameraData() { delete dev_; } > >> + > >> + void setV4L2Device(V4L2Device *dev) { dev_ = dev; } > >> + V4L2Device *device() const { return dev_; } > >> + > > > > As this class is only used internally by the IPU3 pipeline manager I > > would just make dev_ public and remove the accessors, especially given > > that you implement both direct read and write without any side effect. > > > >> +private: > >> + V4L2Device *dev_; > >> +}; > >> + > >> class PipelineHandlerIPU3 : public PipelineHandler > >> { > >> public: > >> @@ -32,6 +46,7 @@ private: > >> MediaDevice *cio2_; > >> MediaDevice *imgu_; > >> > >> + V4L2Device *createVideoDevice(unsigned int id); > >> void registerCameras(CameraManager *manager); > >> }; > >> > >> @@ -122,6 +137,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(); > > > > Unrelated to this patch, I wonder if we should have a V4L2Device::init() > > method that would perform the open + 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 +205,15 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager) > >> > >> std::string cameraName = sensor->name() + " " + std::to_string(id); > >> std::shared_ptr<Camera> camera = Camera::create(cameraName); > >> + > >> + /* Store pipeline private data in the Camera object. */ > >> + V4L2Device *videoDev = createVideoDevice(id); > >> + if (videoDev) { > >> + IPU3CameraData *ipu3Data = new IPU3CameraData(); > >> + ipu3Data->setV4L2Device(videoDev); > >> + camera->setCameraData(ipu3Data); > >> + } > >> + > > > > I think you can do it the other way around, creating the camera data > > first, and then the V4L2 device, as you will have more than just a V4L2 > > device to associate with the camera. > > > > camera->setCameraData(std::move(utils::make_unique<IPU3CameraData>())); > > IPU3CameraData *data = camera->cameraData(); > > > > V4L2Device *videoDev = createVideoDevice(id); > > if (!videoDev) { > > /* Error handling */ > > } > > > > data->videoDev = createVideoDevice(id); > > Is this intentional? Why Call createVideoDevice() twice? Can't I just > re-use videoDev? Clearly not intentional :-) I meant data->videoDev = videoDev; > By the way, my intention was originally to skip creating pipeline data > and associate them at all, if videoDev creation fails. In future we > might have more data, but for now we don't. I will change it anyway. Failure to create a V4L2Device for the camera is fatal, isn't it ? The camera shouldn't be added to the camera manager in that case, and all created objects should be properly destroyed, but it's an error path so we don't need to optimize it. On a side note, using std::unique_ptr<> as proposed ensures that the IPU3CameraData gets destroyed in case of failure (assuming that the camera itself gets destroyed of course), so the pattern shouldn't be too difficult for pipeline handlers, even if it required calling camera->data() right after camera->setData(). > >> manager->addCamera(std::move(camera)); > >> > >> LOG(IPU3, Info)
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 8cbc10a..58053ea 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -15,11 +15,25 @@ #include "log.h" #include "media_device.h" #include "pipeline_handler.h" +#include "v4l2_device.h" namespace libcamera { LOG_DEFINE_CATEGORY(IPU3) +class IPU3CameraData : public CameraData +{ +public: + IPU3CameraData() : dev_(nullptr) { } + ~IPU3CameraData() { delete dev_; } + + void setV4L2Device(V4L2Device *dev) { dev_ = dev; } + V4L2Device *device() const { return dev_; } + +private: + V4L2Device *dev_; +}; + class PipelineHandlerIPU3 : public PipelineHandler { public: @@ -32,6 +46,7 @@ private: MediaDevice *cio2_; MediaDevice *imgu_; + V4L2Device *createVideoDevice(unsigned int id); void registerCameras(CameraManager *manager); }; @@ -122,6 +137,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 +205,15 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager) std::string cameraName = sensor->name() + " " + std::to_string(id); std::shared_ptr<Camera> camera = Camera::create(cameraName); + + /* Store pipeline private data in the Camera object. */ + V4L2Device *videoDev = createVideoDevice(id); + if (videoDev) { + IPU3CameraData *ipu3Data = new IPU3CameraData(); + ipu3Data->setV4L2Device(videoDev); + camera->setCameraData(ipu3Data); + } + manager->addCamera(std::move(camera)); LOG(IPU3, Info)
Create V4L2 devices for the CIO2 capture video nodes. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 42 ++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)