[libcamera-devel] [RFC 2/2] libcamera: ipu3: Create CIO2 V4L2 devices

[Jacopo Mondi]

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 at 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
-------------- 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/20190123/0fb23ddb/attachment.sig>