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

Message ID 20190124113020.7203-3-jacopo@jmondi.org
State Accepted
Headers show
Series
  • Add pipeline-specific per-camera data
Related show

Commit Message

Jacopo Mondi Jan. 24, 2019, 11:30 a.m. UTC
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@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 50 ++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Niklas Söderlund Jan. 24, 2019, 7:06 p.m. UTC | #1
Hi Jacopo,

Thanks for your work.

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@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);

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)
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Jan. 24, 2019, 7:33 p.m. UTC | #2
Hi Niklas
   thanks for review

On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> 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@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?

Thanks
  j

>
> 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)
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Niklas Söderlund Jan. 24, 2019, 8:03 p.m. UTC | #3
Hi Jacopo,

On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote:
> Hi Niklas
>    thanks for review
> 
> On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > 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@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?

Thanks for the explanation! This looks good to me, expect you are 
missing 'data->dev_ = videoDev' but I assume that is not critical to 
your demonstration ;-) With this fix,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
> Thanks
>   j
> 
> >
> > 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)
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
Jacopo Mondi Jan. 24, 2019, 8:12 p.m. UTC | #4
On Thu, Jan 24, 2019 at 09:03:54PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote:
> > Hi Niklas
> >    thanks for review
> >
> > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your work.
> > >
> > > 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@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?
>
> Thanks for the explanation! This looks good to me, expect you are
> missing 'data->dev_ = videoDev' but I assume that is not critical to
> your demonstration ;-) With this fix,

I added it to the constructor, sorry, it's not shown here.
Want me to remove that?

>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
> >
> > Thanks
> >   j
> >
> > >
> > > 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)
> > > > --
> > > > 2.20.1
> > > >
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
>
>
> --
> Regards,
> Niklas Söderlund
Niklas Söderlund Jan. 24, 2019, 8:17 p.m. UTC | #5
On 2019-01-24 21:12:49 +0100, Jacopo Mondi wrote:
> On Thu, Jan 24, 2019 at 09:03:54PM +0100, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote:
> > > Hi Niklas
> > >    thanks for review
> > >
> > > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thanks for your work.
> > > >
> > > > 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@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?
> >
> > Thanks for the explanation! This looks good to me, expect you are
> > missing 'data->dev_ = videoDev' but I assume that is not critical to
> > your demonstration ;-) With this fix,
> 
> I added it to the constructor, sorry, it's not shown here.
> Want me to remove that?

I leave that up to you as the future IPU3 pipeline master :-)

> 
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >
> > >
> > > Thanks
> > >   j
> > >
> > > >
> > > > 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)
> > > > > --
> > > > > 2.20.1
> > > > >
> > > > > _______________________________________________
> > > > > libcamera-devel mailing list
> > > > > libcamera-devel@lists.libcamera.org
> > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > > >
> > > > --
> > > > Regards,
> > > > Niklas Söderlund
> >
> >
> >
> > --
> > Regards,
> > Niklas Söderlund
Jacopo Mondi Jan. 25, 2019, 1:30 p.m. UTC | #6
Hi Niklas,

On Thu, Jan 24, 2019 at 09:17:53PM +0100, Niklas Söderlund wrote:
> On 2019-01-24 21:12:49 +0100, Jacopo Mondi wrote:
> > On Thu, Jan 24, 2019 at 09:03:54PM +0100, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > On 2019-01-24 20:33:00 +0100, Jacopo Mondi wrote:
> > > > Hi Niklas
> > > >    thanks for review
> > > >
> > > > On Thu, Jan 24, 2019 at 08:06:30PM +0100, Niklas Söderlund wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > Thanks for your work.
> > > > >
> > > > > 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@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?
> > >
> > > Thanks for the explanation! This looks good to me, expect you are
> > > missing 'data->dev_ = videoDev' but I assume that is not critical to
> > > your demonstration ;-) With this fix,
> >
> > I added it to the constructor, sorry, it's not shown here.
> > Want me to remove that?
>
> I leave that up to you as the future IPU3 pipeline master :-)
>

I happly abdicate from this, but in the meantime I have pushed these
patches.

I dropped the v4l2 device as constructor parameter, as it will
unlikely be the only per-camera object we'll have to store.

Thanks
  j

> >
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > >
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > > >
> > > > > 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)
> > > > > > --
> > > > > > 2.20.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > libcamera-devel mailing list
> > > > > > libcamera-devel@lists.libcamera.org
> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Niklas Söderlund
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
>
>
> --
> Regards,
> Niklas Söderlund
Laurent Pinchart Jan. 25, 2019, 3:31 p.m. UTC | #7
Hi Jacopo,

Thank you for the patch.

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@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
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.

> +	};
> +
>  	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 ?

> +
> +		/*
> +		 * 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.

> +		}
> +
> +		data->dev_ = videoDev;
>  		manager->addCamera(std::move(camera));
>  
>  		LOG(IPU3, Info)
Laurent Pinchart Jan. 25, 2019, 3:36 p.m. UTC | #8
Hi Jacopo,

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@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));

> > 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)
Jacopo Mondi Jan. 25, 2019, 3:51 p.m. UTC | #9
HI Laurent,

On Fri, Jan 25, 2019 at 05:31:09PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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@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.

>
> > +	};
> > +
> >  	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

> > +
> > +		/*
> > +		 * 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.

Thanks
  j

> > +		}
> > +
> > +		data->dev_ = videoDev;
> >  		manager->addCamera(std::move(camera));
> >
> >  		LOG(IPU3, Info)
>
> --
> Regards,
>
> Laurent Pinchart
Jacopo Mondi Jan. 25, 2019, 3:54 p.m. UTC | #10
Hi Laurent,

On Fri, Jan 25, 2019 at 05:36:12PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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@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.

Thanks
  j


> > > 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
Laurent Pinchart Jan. 25, 2019, 4:03 p.m. UTC | #11
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@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)
Laurent Pinchart Jan. 25, 2019, 4:08 p.m. UTC | #12
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@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)
Jacopo Mondi Jan. 25, 2019, 4:49 p.m. UTC | #13
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@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

Patch

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()));
+
+		/*
+		 * 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)