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

[Niklas Söderlund]

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 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?
> >
> > 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 at 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 at lists.libcamera.org
> > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > > >
> > > > --
> > > > Regards,
> > > > Niklas Söderlund
> >
> >
> >
> > --
> > Regards,
> > Niklas Söderlund



-- 
Regards,
Niklas Söderlund