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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Jan 25, 2019 at 04:54:58PM +0100, Jacopo Mondi wrote:
> On Fri, Jan 25, 2019 at 05:36:12PM +0200, Laurent Pinchart wrote:
> > On Thu, Jan 24, 2019 at 08:33:00PM +0100, Jacopo Mondi wrote:
> >> On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:
> >>> On 2019-01-24 12:30:20 +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_;
> >>>> +	};
> >>>> +
> >>>>  	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()));
> >>>
> >>> I'm not saying this is not needed, only that it looks a bit complex to
> >>> my feeble mind. Could you educate me why the following would not work?
> >>>
> >>>     IPU3CameraData *data = new IPU3CameraData();
> >>>     data->dev_ = videoDev;
> >>>
> >>>     setCameraData(camera.get(), data);
> >>
> >> setCameraData wants a unique_ptr. On the reason why we're passing it
> >> by value (hence the std::move() ) instead than by reference and move()
> >> inside the function see the discussion on v1. Basically, it makes
> >> clear that after setCameraData() the local reference it's not
> >> valid anymore.
> >>
> >> That said, I could indeed have created a unique_ptr<> from an already
> >> existing reference, instead of using std::make_unique.
> >>
> >> What I have now looks like the following:
> >>
> >> 		V4L2Device *videoDev = createVideoDevice(id);
> >> 		if (!videoDev) {
> >> 			LOG(IPU3, Error)
> >> 				<< "Failed to register camera["
> >> 				<< numCameras << "] \"" << cameraName << "\"";
> >> 			continue;
> >> 		}
> >>
> >> 		IPU3CameraData *data = new IPU3CameraData(*videoDev);
> >> 		setCameraData(camera.get(),
> >> 			      std::move(std::unique_ptr<IPU3CameraData>(data)));
> >> 		manager->addCamera(std::move(camera));
> >>
> >> Which is in my opinion nicer and equally safe. Thanks a lot for pointing
> >> this out. Is it any better?
> >
> > How about
> >
> > 		std::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>();
> >  		data->dev_ = createVideoDevice(id);
> >  		if (!data->dev_) {
> >  			LOG(IPU3, Error)
> >  				<< "Failed to register camera["
> >  				<< numCameras << "] \"" << cameraName << "\"";
> >  			continue;
> >  		}
> >
> >  		setCameraData(camera.get(), std::move(data));
> >  		manager->addCamera(std::move(camera));
> >
> 
> Ok, but why is it better? I see an allocation which in case of failure
> in creating the video device could be skipped.

First we allocate data as a unique pointer, so we know it will be either
given to setCameraData() or deleted. No risk of leak. Then we create the
video device and store it directly in the camera data structure, so we
know it will be deleted with the camera. No risk of leak. If additional
initialization was needed, it would follow at this point. Then we call
setCameraData() at the end, when we don't need the data anymore, which
avoids calling cameraData() in here to get a borrowed reference to
something we just gave away (as in your original version). Finally we
register the camera. The setCameraData() and addCamera() calls could
even be moved to a PipelineHandler::addCamera(std::unique_ptr<Camera>
camera, std::unique_ptr<CameraData> data) function that would do both,
simplifying the API for pipeline handlers.

> >>> Apart from this I think this commit looks good.
> >>>
> >>>> +
> >>>> +		/*
> >>>> +		 * 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;
> >>>> +		}
> >>>> +
> >>>> +		data->dev_ = videoDev;
> >>>>  		manager->addCamera(std::move(camera));
> >>>>
> >>>>  		LOG(IPU3, Info)

-- 
Regards,

Laurent Pinchart