[libcamera-devel] [PATCH] libcamera: CameraManager, PipelineHandler: Allow multiple devnums per camera

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Jun 05, 2020 at 09:03:06PM +0900, Paul Elder wrote:
> The V4L2 compatibility layer uses devnum to match video device nodes to
> libcamera Cameras. Some pipeline handlers don't report a devnum for
> their camera, which prevents the V4L2 compatibility layer from matching
> video device nodes to these cameras. To fix this, we first allow the
> camera manager to map multiple devnums to a camera. Next, if the pipeline
> handler doesn't report a devnum for a camera, then walk the media device
> and entity list and tell the camera manager to map every one of these
> devnums to the camera.
> 
> We considered walking the media entity list and taking the devnum from
> just the one with the default flag set, but we found that some drivers
> (eg. vimc) don't set this flag for any entity.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  include/libcamera/camera_manager.h |  3 ++-
>  src/libcamera/camera_manager.cpp   | 14 ++++++++------
>  src/libcamera/pipeline_handler.cpp | 12 +++++++++++-
>  3 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 079f848a..6095b799 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -34,7 +34,8 @@ public:
>  	std::shared_ptr<Camera> get(const std::string &name);
>  	std::shared_ptr<Camera> get(dev_t devnum);
>  
> -	void addCamera(std::shared_ptr<Camera> camera, dev_t devnum);
> +	void addCamera(std::shared_ptr<Camera> camera,
> +		       std::vector<dev_t> devnums);

Make this a const std::vector<> & to avoid copies.

>  	void removeCamera(Camera *camera);
>  
>  	static const std::string &version() { return version_; }
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index da806fa7..fa0bd6a0 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -36,7 +36,8 @@ public:
>  	Private(CameraManager *cm);
>  
>  	int start();
> -	void addCamera(std::shared_ptr<Camera> &camera, dev_t devnum);
> +	void addCamera(std::shared_ptr<Camera> &camera,
> +		       std::vector<dev_t> devnums);

Same here.

>  	void removeCamera(Camera *camera);
>  
>  	/*
> @@ -168,7 +169,7 @@ void CameraManager::Private::cleanup()
>  }
>  
>  void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
> -				       dev_t devnum)
> +				       std::vector<dev_t> devnums)
>  {
>  	MutexLocker locker(mutex_);
>  
> @@ -183,7 +184,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
>  
>  	cameras_.push_back(std::move(camera));
>  
> -	if (devnum) {
> +	for (dev_t devnum : devnums) {
>  		unsigned int index = cameras_.size() - 1;

You can move this outside of the loop.

>  		camerasByDevnum_[devnum] = cameras_[index];
>  	}
> @@ -374,7 +375,7 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
>  /**
>   * \brief Add a camera to the camera manager
>   * \param[in] camera The camera to be added
> - * \param[in] devnum The device number to associate with \a camera
> + * \param[in] devnums The device numbers to associate with \a camera
>   *
>   * This function is called by pipeline handlers to register the cameras they
>   * handle with the camera manager. Registered cameras are immediately made
> @@ -385,11 +386,12 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
>   *
>   * \context This function shall be called from the CameraManager thread.
>   */
> -void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
> +void CameraManager::addCamera(std::shared_ptr<Camera> camera,
> +			      std::vector<dev_t> devnums)
>  {
>  	ASSERT(Thread::current() == p_.get());
>  
> -	p_->addCamera(camera, devnum);
> +	p_->addCamera(camera, devnums);
>  }
>  
>  /**
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 53aeebdc..b3824a5f 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -502,7 +502,17 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
>  	data->camera_ = camera.get();
>  	cameraData_[camera.get()] = std::move(data);
>  	cameras_.push_back(camera);
> -	manager_->addCamera(std::move(camera), devnum);
> +
> +	std::vector<dev_t> devnums;
> +	if (devnum != 0)
> +		devnums.push_back(devnum);

Should we allow this, or always use all the capture video nodes ?

> +	else
> +		for (const std::shared_ptr<MediaDevice> media : mediaDevices_)
> +			for (const MediaEntity *entity : media->entities())
> +				devnums.push_back(makedev(entity->deviceMajor(),
> +							  entity->deviceMinor()));

You need to limit entities to the capture video nodes. You can handle
that through a combination of entity flags (to check if it's a video
node) and pad flags (to check if it's a capture video node, by looking
at the direction of the pad).

A short comment to explain what's going on would be useful.

> +
> +	manager_->addCamera(std::move(camera), devnums);
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart