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

[Jacopo Mondi]

Hi Laurent,

On Fri, Jan 25, 2019 at 06:08:05PM +0200, Laurent Pinchart wrote:
> 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.
>

ok

Thanks
   j
-------------- 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/20190125/59455bed/attachment-0001.sig>