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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 22, 2019 at 07:12:25PM +0100, Jacopo Mondi wrote:
> Create V4L2 devices for the CIO2 capture video nodes.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 42 ++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 8cbc10a..58053ea 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -15,11 +15,25 @@
>  #include "log.h"
>  #include "media_device.h"
>  #include "pipeline_handler.h"
> +#include "v4l2_device.h"
>  
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPU3)
>  
> +class IPU3CameraData : public CameraData
> +{
> +public:
> +	IPU3CameraData() : dev_(nullptr) { }
> +	~IPU3CameraData() { delete dev_; }
> +
> +	void setV4L2Device(V4L2Device *dev) { dev_ = dev; }
> +	V4L2Device *device() const { return dev_; }
> +

As this class is only used internally by the IPU3 pipeline manager I
would just make dev_ public and remove the accessors, especially given
that you implement both direct read and write without any side effect.

> +private:
> +	V4L2Device *dev_;
> +};
> +
>  class PipelineHandlerIPU3 : public PipelineHandler
>  {
>  public:
> @@ -32,6 +46,7 @@ private:
>  	MediaDevice *cio2_;
>  	MediaDevice *imgu_;
>  
> +	V4L2Device *createVideoDevice(unsigned int id);
>  	void registerCameras(CameraManager *manager);
>  };
>  
> @@ -122,6 +137,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();

Unrelated to this patch, I wonder if we should have a V4L2Device::init()
method that would perform the open + 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 +205,15 @@ void PipelineHandlerIPU3::registerCameras(CameraManager *manager)
>  
>  		std::string cameraName = sensor->name() + " " + std::to_string(id);
>  		std::shared_ptr<Camera> camera = Camera::create(cameraName);
> +
> +		/* Store pipeline private data in the Camera object. */
> +		V4L2Device *videoDev = createVideoDevice(id);
> +		if (videoDev) {
> +			IPU3CameraData *ipu3Data = new IPU3CameraData();
> +			ipu3Data->setV4L2Device(videoDev);
> +			camera->setCameraData(ipu3Data);
> +		}
> +

I think you can do it the other way around, creating the camera data
first, and then the V4L2 device, as you will have more than just a V4L2
device to associate with the camera.

		camera->setCameraData(std::move(utils::make_unique<IPU3CameraData>()));
		IPU3CameraData *data = camera->cameraData();

		V4L2Device *videoDev = createVideoDevice(id);
		if (!videoDev) {
			/* Error handling */
		}

		data->videoDev = createVideoDevice(id);

>  		manager->addCamera(std::move(camera));
>  
>  		LOG(IPU3, Info)

-- 
Regards,

Laurent Pinchart