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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Jan 21, 2019 at 06:27:04PM +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 | 36 +++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index daf681c..0689ce8 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 {
>  
> @@ -30,6 +32,9 @@ private:
>  	MediaDevice *cio2_;
>  	MediaDevice *imgu_;
>  
> +	std::vector<std::unique_ptr<V4L2Device>> videoDevices_;
> +

I think this is where you start needed per-camera data in the pipeline.
I would already model it as such.

> +	void createVideoDevices();
>  	void registerCameras(CameraManager *manager);
>  };
>  
> @@ -91,6 +96,12 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer
>  	cio2_->acquire();
>  	imgu_->acquire();
>  
> +	/* Create video device nodes for CIO2 outputs */
> +	if (cio2_->open())
> +		goto error_release_mdev;
> +

Do you need to open the cio2 media device to create the V4L2Device
instances ?

> +	createVideoDevices();
> +
>  	/*
>  	 * Disable all links that are enabled by default on CIO2, as camera
>  	 * creation enables all valid links it finds.
> @@ -98,9 +109,6 @@ bool PipelineHandlerIPU3::match(CameraManager *manager, DeviceEnumerator *enumer
>  	 * Close the CIO2 media device after, as links are enabled and should
>  	 * not need to be changed after.
>  	 */
> -	if (cio2_->open())
> -		goto error_release_mdev;
> -
>  	if (cio2_->disableLinks())
>  		goto error_close_cio2;
>  
> @@ -120,6 +128,28 @@ error_release_mdev:
>  	return false;
>  }
>  
> +/*
> + * Create video devices for the CIO2 unit capture nodes and cache them
> + * for later reuse.
> + */
> +void PipelineHandlerIPU3::createVideoDevices()
> +{
> +	for (unsigned int id = 0; id < 3; ++id) {

I assume you meant < 4 ?

> +		std::string cio2Name = "ipu3-cio2 " + std::to_string(id);
> +		MediaEntity *cio2 = cio2_->getEntityByName(cio2Name);
> +		if (!cio2)
> +			continue;
> +
> +		std::unique_ptr<V4L2Device> dev =
> +			utils::make_unique<V4L2Device>(*cio2);

Do we really need to use std::unique_ptr<> for this ? Ownership of the
V4L2Device will never be transferred, so I assume ths reason is to get
the pointer deleted automatically. If you create a per-camera IPU3
pipeline object, you could embed V4L2Device in that object instead of
allocating it manually, which would achieve the same without using
std::unique_ptr<>.

> +		if (dev->open())
> +			continue;
> +		dev->close();
> +
> +		videoDevices_.push_back(std::move(dev));
> +	}

You should only create V4L2 devices for the CIO2 channels associated
with a camera, the other ones are not needed. I would advice splitting
the creation of cameras from registerCameras() to a registerCamera()
function, and moving creation of the V4L2 device there.

> +}
> +
>  /*
>   * Cameras are created associating an image sensor (represented by a
>   * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four

-- 
Regards,

Laurent Pinchart