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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Jan 25, 2019 at 04:51:47PM +0100, Jacopo Mondi wrote:
> On Fri, Jan 25, 2019 at 05:31:09PM +0200, Laurent Pinchart wrote:
> > On Thu, Jan 24, 2019 at 12:30:20PM +0100, Jacopo Mondi wrote:
> >> Create V4L2 devices for the CIO2 capture video nodes and associate them
> >> with the camera they are part of.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++
> >>  1 file changed, 50 insertions(+)
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index 8cbc10a..9f5a40f 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 {
> >>
> >> @@ -29,9 +31,19 @@ public:
> >>  	bool match(CameraManager *manager, DeviceEnumerator *enumerator);
> >>
> >>  private:
> >> +	class IPU3CameraData : public CameraData
> >> +	{
> >> +	public:
> >> +		IPU3CameraData()
> >> +			: dev_(nullptr) { }
> >> +		~IPU3CameraData() { delete dev_; }
> >> +		V4L2Device *dev_;
> >
> > You will soon need to add data to this, so I wouldn't inline the
> 
> As this is for a follow-up patch, let's do that once it is required
> 
> > constructor and destructor. As the class may also get used by other
> > compilation units later on I'd define it outside of the
> > PipelineHandlerIPU3 class, to make it easier to later move it to a
> > header if necessary.
> 
> As this is for a follow-up patch, let's do that once it is required
> once (if) we split the IPU3 handler in multiple source files.

I'm OK with that.

> >> +	};
> >> +
> >>  	MediaDevice *cio2_;
> >>  	MediaDevice *imgu_;
> >>
> >> +	V4L2Device *createVideoDevice(unsigned int id);
> >>  	void registerCameras(CameraManager *manager);
> >>  };
> >>
> >> @@ -122,6 +134,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 +202,26 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)
> >>
> >>  		std::string cameraName = sensor->name() + " " + std::to_string(id);
> >>  		std::shared_ptr<Camera> camera = Camera::create(cameraName);
> >> +
> >> +		setCameraData(camera.get(),
> >> +			      std::move(utils::make_unique<IPU3CameraData>()));
> >> +		IPU3CameraData *data =
> >> +			reinterpret_cast<IPU3CameraData *>(cameraData(camera.get()));
> >
> > As I expect you'll need to call this quite often, how about creating a
> >
> > 	IPU3CameraData *cameraData(const Camera *);
> >
> > function in the PipelineHandlerIPU3 class ?
> >
> 
> As this is for a follow-up patch, let's do that once it is required

This however I would fix already, to make sure the IPU3 pipeline handler
follows all the best practices and can be used as an example.

> >> +
> >> +		/*
> >> +		 * If V4L2 device creation fails, the Camera instance won't be
> >> +		 * registered. The 'camera' shared pointer goes out of scope
> >> +		 * and deletes the Camera it manages.
> >> +		 */
> >> +		V4L2Device *videoDev = createVideoDevice(id);
> >> +		if (!videoDev) {
> >> +			LOG(IPU3, Error)
> >> +				<< "Failed to register camera["
> >> +				<< numCameras << "] \"" << cameraName << "\"";
> >> +			continue;
> >
> > If this fails the camera will be deleted (as the sole reference will the
> > the shared pointer local to the loop), but the shared data will stay.
> > It's no big deal, it will be unused and be deleted when the pipeline
> > handler is deleted, but that still bothers me a bit. I wonder whether we
> > shouldn't call setCameraData() right before manager->addCamera()
> > instead.
> >
> 
> I'll reply to this in the other comment, as this is not what has been
> merged.
> 
> >> +		}
> >> +
> >> +		data->dev_ = videoDev;
> >>  		manager->addCamera(std::move(camera));
> >>
> >>  		LOG(IPU3, Info)

-- 
Regards,

Laurent Pinchart