[libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2 devices
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Jan 22 20:12:33 CET 2019
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list