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

[Niklas Söderlund]

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

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