[libcamera-devel] [PATCH 5/6] libcamera: ipu3: Create CIO2 V4L2 devices

[Jacopo Mondi]

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 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()

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190122/3ce31df3/attachment.sig>