| Message ID | 20190121172705.19985-6-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Mon, Jan 21, 2019 at 06:27:04PM +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 | 36 +++++++++++++++++++++++++--- > 1 file changed, 33 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index daf681c..0689ce8 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 { > > @@ -30,6 +32,9 @@ private: > MediaDevice *cio2_; > MediaDevice *imgu_; > > + std::vector<std::unique_ptr<V4L2Device>> videoDevices_; > + I think this is where you start needed per-camera data in the pipeline. I would already model it as such. > + void createVideoDevices(); > void registerCameras(CameraManager *manager); > }; > > @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer > cio2_->acquire(); > imgu_->acquire(); > > + /* Create video device nodes for CIO2 outputs */ > + if (cio2_->open()) > + goto error_release_mdev; > + Do you need to open the cio2 media device to create the V4L2Device instances ? > + createVideoDevices(); > + > /* > * Disable all links that are enabled by default on CIO2, as camera > * creation enables all valid links it finds. > @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer > * Close the CIO2 media device after, as links are enabled and should > * not need to be changed after. > */ > - if (cio2_->open()) > - goto error_release_mdev; > - > if (cio2_->disableLinks()) > goto error_close_cio2; > > @@ -120,6 +128,28 @@ error_release_mdev: > return false; > } > > +/* > + * Create video devices for the CIO2 unit capture nodes and cache them > + * for later reuse. > + */ > +void PipelineHandlerIPU3::createVideoDevices() > +{ > + for (unsigned int id = 0; id < 3; ++id) { I assume you meant < 4 ? > + std::string cio2Name = "ipu3-cio2 " + std::to_string(id); > + MediaEntity *cio2 = cio2_->getEntityByName(cio2Name); > + if (!cio2) > + continue; > + > + std::unique_ptr<V4L2Device> dev = > + utils::make_unique<V4L2Device>(*cio2); Do we really need to use std::unique_ptr<> for this ? Ownership of the V4L2Device will never be transferred, so I assume ths reason is to get the pointer deleted automatically. If you create a per-camera IPU3 pipeline object, you could embed V4L2Device in that object instead of allocating it manually, which would achieve the same without using std::unique_ptr<>. > + if (dev->open()) > + continue; > + dev->close(); > + > + videoDevices_.push_back(std::move(dev)); > + } You should only create V4L2 devices for the CIO2 channels associated with a camera, the other ones are not needed. I would advice splitting the creation of cameras from registerCameras() to a registerCamera() function, and moving creation of the V4L2 device there. > +} > + > /* > * Cameras are created associating an image sensor (represented by a > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
Hi Laurent, this patch was sketeched out mainly to test creation of V4L2 devices in the pipeline handler. When I wrote this I didn't think that each CIO2 device had to be associated with a Camera, but that's actually the case with the IPU3, so I welcome your suggestion to use this as an opportunity to start sketching out per camera specific data. Remember though, that currently Cameras do not have a reference to their pipeline handlers, but that will be soon added I assume. On Mon, Jan 21, 2019 at 10:53:52PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Jan 21, 2019 at 06:27:04PM +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 | 36 +++++++++++++++++++++++++--- > > 1 file changed, 33 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index daf681c..0689ce8 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 { > > > > @@ -30,6 +32,9 @@ private: > > MediaDevice *cio2_; > > MediaDevice *imgu_; > > > > + std::vector<std::unique_ptr<V4L2Device>> videoDevices_; > > + > > I think this is where you start needed per-camera data in the pipeline. > I would already model it as such. > > > + void createVideoDevices(); > > void registerCameras(CameraManager *manager); > > }; > > > > @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer > > cio2_->acquire(); > > imgu_->acquire(); > > > > + /* Create video device nodes for CIO2 outputs */ > > + if (cio2_->open()) > > + goto error_release_mdev; > > + > > Do you need to open the cio2 media device to create the V4L2Device > instances ? Possibly no, it has been enumerated already... > > > + createVideoDevices(); > > + > > /* > > * Disable all links that are enabled by default on CIO2, as camera > > * creation enables all valid links it finds. > > @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer > > * Close the CIO2 media device after, as links are enabled and should > > * not need to be changed after. > > */ > > - if (cio2_->open()) > > - goto error_release_mdev; > > - > > if (cio2_->disableLinks()) > > goto error_close_cio2; > > > > @@ -120,6 +128,28 @@ error_release_mdev: > > return false; > > } > > > > +/* > > + * Create video devices for the CIO2 unit capture nodes and cache them > > + * for later reuse. > > + */ > > +void PipelineHandlerIPU3::createVideoDevices() > > +{ > > + for (unsigned int id = 0; id < 3; ++id) { > > I assume you meant < 4 ? Yes, I got confused by the 2 and the end of "cio2" :) > > > + std::string cio2Name = "ipu3-cio2 " + std::to_string(id); > > + MediaEntity *cio2 = cio2_->getEntityByName(cio2Name); > > + if (!cio2) > > + continue; > > + > > + std::unique_ptr<V4L2Device> dev = > > + utils::make_unique<V4L2Device>(*cio2); > > Do we really need to use std::unique_ptr<> for this ? Ownership of the > V4L2Device will never be transferred, so I assume ths reason is to get > the pointer deleted automatically. If you create a per-camera IPU3 > pipeline object, you could embed V4L2Device in that object instead of > allocating it manually, which would achieve the same without using > std::unique_ptr<>. Yes, automatic deletion was the reason. I'll see how I should design this, but if I will need a sub-class of a generic base CameraData class, I should instantiate it with: camera.data = new IPU3CameraData() and the problem of having to keep track of that instance (which might contain the V4L2 device) will represent itself, won't it? > > > + if (dev->open()) > > + continue; > > + dev->close(); > > + > > + videoDevices_.push_back(std::move(dev)); > > + } > > You should only create V4L2 devices for the CIO2 channels associated > with a camera, the other ones are not needed. I would advice splitting > the creation of cameras from registerCameras() to a registerCamera() > function, and moving creation of the V4L2 device there. > Agreed, let's use this to model Camera specific data. Thanks j > > +} > > + > > /* > > * Cameras are created associating an image sensor (represented by a > > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Jan 22, 2019 at 03:55:05PM +0100, Jacopo Mondi wrote: > Hi Laurent, > this patch was sketeched out mainly to test creation of V4L2 > devices in the pipeline handler. When I wrote this I didn't think that > each CIO2 device had to be associated with a Camera, but that's actually the > case with the IPU3, so I welcome your suggestion to use this as an > opportunity to start sketching out per camera specific data. > > Remember though, that currently Cameras do not have a reference to > their pipeline handlers, but that will be soon added I assume. Yes, it is needed. Would you like to give it a try, solving all the lifetime management issues it creates as you go ? :-) > On Mon, Jan 21, 2019 at 10:53:52PM +0200, Laurent Pinchart wrote: > > On Mon, Jan 21, 2019 at 06:27:04PM +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 | 36 +++++++++++++++++++++++++--- > >> 1 file changed, 33 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index daf681c..0689ce8 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 { > >> > >> @@ -30,6 +32,9 @@ private: > >> MediaDevice *cio2_; > >> MediaDevice *imgu_; > >> > >> + std::vector<std::unique_ptr<V4L2Device>> videoDevices_; > >> + > > > > I think this is where you start needed per-camera data in the pipeline. > > I would already model it as such. > > > >> + void createVideoDevices(); > >> void registerCameras(CameraManager *manager); > >> }; > >> > >> @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer > >> cio2_->acquire(); > >> imgu_->acquire(); > >> > >> + /* Create video device nodes for CIO2 outputs */ > >> + if (cio2_->open()) > >> + goto error_release_mdev; > >> + > > > > Do you need to open the cio2 media device to create the V4L2Device > > instances ? > > Possibly no, it has been enumerated already... > > >> + createVideoDevices(); > >> + > >> /* > >> * Disable all links that are enabled by default on CIO2, as camera > >> * creation enables all valid links it finds. > >> @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer > >> * Close the CIO2 media device after, as links are enabled and should > >> * not need to be changed after. > >> */ > >> - if (cio2_->open()) > >> - goto error_release_mdev; > >> - > >> if (cio2_->disableLinks()) > >> goto error_close_cio2; > >> > >> @@ -120,6 +128,28 @@ error_release_mdev: > >> return false; > >> } > >> > >> +/* > >> + * Create video devices for the CIO2 unit capture nodes and cache them > >> + * for later reuse. > >> + */ > >> +void PipelineHandlerIPU3::createVideoDevices() > >> +{ > >> + for (unsigned int id = 0; id < 3; ++id) { > > > > I assume you meant < 4 ? > > Yes, I got confused by the 2 and the end of "cio2" :) > > >> + std::string cio2Name = "ipu3-cio2 " + std::to_string(id); > >> + MediaEntity *cio2 = cio2_->getEntityByName(cio2Name); > >> + if (!cio2) > >> + continue; > >> + > >> + std::unique_ptr<V4L2Device> dev = > >> + utils::make_unique<V4L2Device>(*cio2); > > > > Do we really need to use std::unique_ptr<> for this ? Ownership of the > > V4L2Device will never be transferred, so I assume ths reason is to get > > the pointer deleted automatically. If you create a per-camera IPU3 > > pipeline object, you could embed V4L2Device in that object instead of > > allocating it manually, which would achieve the same without using > > std::unique_ptr<>. > > Yes, automatic deletion was the reason. > > I'll see how I should design this, but if I will need a sub-class of a > generic base CameraData class, I should instantiate it with: > > camera.data = new IPU3CameraData() Or possibly camera.setData(new IPU3CameraData()), with a camera.data() accessor. This is especially important if you want to use std::unique_ptr<> to store the camera data, as we'll need to borrow references all the time, and having a data() accessor for that would be simpler. > and the problem of having to keep track of that instance (which might > contain the V4L2 device) will represent itself, won't it? You can either delete it in the destructor of the Camera class, or use a std::unique_ptr<> to store it. > >> + if (dev->open()) > >> + continue; > >> + dev->close(); > >> + > >> + videoDevices_.push_back(std::move(dev)); > >> + } > > > > You should only create V4L2 devices for the CIO2 channels associated > > with a camera, the other ones are not needed. I would advice splitting > > the creation of cameras from registerCameras() to a registerCamera() > > function, and moving creation of the V4L2 device there. > > Agreed, let's use this to model Camera specific data. > > >> +} > >> + > >> /* > >> * Cameras are created associating an image sensor (represented by a > >> * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
On 2019-01-22 17:15:43 +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Tue, Jan 22, 2019 at 03:55:05PM +0100, Jacopo Mondi wrote: > > Hi Laurent, > > this patch was sketeched out mainly to test creation of V4L2 > > devices in the pipeline handler. When I wrote this I didn't think that > > each CIO2 device had to be associated with a Camera, but that's actually the > > case with the IPU3, so I welcome your suggestion to use this as an > > opportunity to start sketching out per camera specific data. > > > > Remember though, that currently Cameras do not have a reference to > > their pipeline handlers, but that will be soon added I assume. > > Yes, it is needed. Would you like to give it a try, solving all the > lifetime management issues it creates as you go ? :-) I have had had a go at solving this, before you spend time of it have a look at my pipe branch. It might not solve the issue, but I attempt to do so :-) I feel there might be an improvement in there where we possibly could lift the responsibility hand managing the life-time issue from the specific PipelineHandler to the base class of all PipelineHandlers. But I will not attempt to solve this now as there are a flora of other issues to sort out first. > > > On Mon, Jan 21, 2019 at 10:53:52PM +0200, Laurent Pinchart wrote: > > > On Mon, Jan 21, 2019 at 06:27:04PM +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 | 36 +++++++++++++++++++++++++--- > > >> 1 file changed, 33 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >> index daf681c..0689ce8 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 { > > >> > > >> @@ -30,6 +32,9 @@ private: > > >> MediaDevice *cio2_; > > >> MediaDevice *imgu_; > > >> > > >> + std::vector<std::unique_ptr<V4L2Device>> videoDevices_; > > >> + > > > > > > I think this is where you start needed per-camera data in the pipeline. > > > I would already model it as such. > > > > > >> + void createVideoDevices(); > > >> void registerCameras(CameraManager *manager); > > >> }; > > >> > > >> @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer > > >> cio2_->acquire(); > > >> imgu_->acquire(); > > >> > > >> + /* Create video device nodes for CIO2 outputs */ > > >> + if (cio2_->open()) > > >> + goto error_release_mdev; > > >> + > > > > > > Do you need to open the cio2 media device to create the V4L2Device > > > instances ? > > > > Possibly no, it has been enumerated already... > > > > >> + createVideoDevices(); > > >> + > > >> /* > > >> * Disable all links that are enabled by default on CIO2, as camera > > >> * creation enables all valid links it finds. > > >> @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer > > >> * Close the CIO2 media device after, as links are enabled and should > > >> * not need to be changed after. > > >> */ > > >> - if (cio2_->open()) > > >> - goto error_release_mdev; > > >> - > > >> if (cio2_->disableLinks()) > > >> goto error_close_cio2; > > >> > > >> @@ -120,6 +128,28 @@ error_release_mdev: > > >> return false; > > >> } > > >> > > >> +/* > > >> + * Create video devices for the CIO2 unit capture nodes and cache them > > >> + * for later reuse. > > >> + */ > > >> +void PipelineHandlerIPU3::createVideoDevices() > > >> +{ > > >> + for (unsigned int id = 0; id < 3; ++id) { > > > > > > I assume you meant < 4 ? > > > > Yes, I got confused by the 2 and the end of "cio2" :) > > > > >> + std::string cio2Name = "ipu3-cio2 " + std::to_string(id); > > >> + MediaEntity *cio2 = cio2_->getEntityByName(cio2Name); > > >> + if (!cio2) > > >> + continue; > > >> + > > >> + std::unique_ptr<V4L2Device> dev = > > >> + utils::make_unique<V4L2Device>(*cio2); > > > > > > Do we really need to use std::unique_ptr<> for this ? Ownership of the > > > V4L2Device will never be transferred, so I assume ths reason is to get > > > the pointer deleted automatically. If you create a per-camera IPU3 > > > pipeline object, you could embed V4L2Device in that object instead of > > > allocating it manually, which would achieve the same without using > > > std::unique_ptr<>. > > > > Yes, automatic deletion was the reason. > > > > I'll see how I should design this, but if I will need a sub-class of a > > generic base CameraData class, I should instantiate it with: > > > > camera.data = new IPU3CameraData() > > Or possibly camera.setData(new IPU3CameraData()), with a camera.data() > accessor. This is especially important if you want to use > std::unique_ptr<> to store the camera data, as we'll need to borrow > references all the time, and having a data() accessor for that would be > simpler. > > > and the problem of having to keep track of that instance (which might > > contain the V4L2 device) will represent itself, won't it? > > You can either delete it in the destructor of the Camera class, or use a > std::unique_ptr<> to store it. > > > >> + if (dev->open()) > > >> + continue; > > >> + dev->close(); > > >> + > > >> + videoDevices_.push_back(std::move(dev)); > > >> + } > > > > > > You should only create V4L2 devices for the CIO2 channels associated > > > with a camera, the other ones are not needed. I would advice splitting > > > the creation of cameras from registerCameras() to a registerCamera() > > > function, and moving creation of the V4L2 device there. > > > > Agreed, let's use this to model Camera specific data. > > > > >> +} > > >> + > > >> /* > > >> * Cameras are created associating an image sensor (represented by a > > >> * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index daf681c..0689ce8 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 { @@ -30,6 +32,9 @@ private: MediaDevice *cio2_; MediaDevice *imgu_; + std::vector<std::unique_ptr<V4L2Device>> videoDevices_; + + void createVideoDevices(); void registerCameras(CameraManager *manager); }; @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer cio2_->acquire(); imgu_->acquire(); + /* Create video device nodes for CIO2 outputs */ + if (cio2_->open()) + goto error_release_mdev; + + createVideoDevices(); + /* * Disable all links that are enabled by default on CIO2, as camera * creation enables all valid links it finds. @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer * Close the CIO2 media device after, as links are enabled and should * not need to be changed after. */ - if (cio2_->open()) - goto error_release_mdev; - if (cio2_->disableLinks()) goto error_close_cio2; @@ -120,6 +128,28 @@ error_release_mdev: return false; } +/* + * Create video devices for the CIO2 unit capture nodes and cache them + * for later reuse. + */ +void PipelineHandlerIPU3::createVideoDevices() +{ + for (unsigned int id = 0; id < 3; ++id) { + std::string cio2Name = "ipu3-cio2 " + std::to_string(id); + MediaEntity *cio2 = cio2_->getEntityByName(cio2Name); + if (!cio2) + continue; + + std::unique_ptr<V4L2Device> dev = + utils::make_unique<V4L2Device>(*cio2); + if (dev->open()) + continue; + dev->close(); + + videoDevices_.push_back(std::move(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
Create V4L2 devices for the CIO2 capture video nodes. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 36 +++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-)