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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart