[libcamera-devel,v2] libcamera: CameraManager, PipelineHandler: Automatically map devnums to Camera

3960 diff mbox series

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, we walk the
media device and entity list and tell the camera manager to map every
one of these devnums that is a video capture node to the camera.

Since we decided that all video capture nodes that belong to a camera
can be opened via the V4L2 compatibility layer to map to that camera, it
would cause confusion for users if some pipeline handlers decided that
only specific device nodes would map to the camera. To prevent this
confusion, remove the ability for pipeline handlers to declare their own
devnum-to-camera mapping. The only pipeline handler that declares the
devnum mapping is the UVC pipeline handler, so remove the devnum there.

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. Instead, we take all the
video capture nodes (entities with the sink pad flag set).

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

---
Changes in v2:
- pipeline handlers can no longer declare their own devnum
  mapping.
  - remove the uvc pipeline handler devnum mapping
- cosmetic changes
---
 include/libcamera/camera_manager.h            |  3 ++-
 include/libcamera/internal/pipeline_handler.h |  2 +-
 src/libcamera/camera_manager.cpp              | 17 ++++++-------
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  4 +---
 src/libcamera/pipeline_handler.cpp            | 24 ++++++++++++-------
 5 files changed, 28 insertions(+), 22 deletions(-)

Hi Paul,

Thank you for the patch.

On Sat, Jun 06, 2020 at 05:07: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, we walk the
> media device and entity list and tell the camera manager to map every
> one of these devnums that is a video capture node to the camera.
> 
> Since we decided that all video capture nodes that belong to a camera
> can be opened via the V4L2 compatibility layer to map to that camera, it
> would cause confusion for users if some pipeline handlers decided that
> only specific device nodes would map to the camera. To prevent this
> confusion, remove the ability for pipeline handlers to declare their own
> devnum-to-camera mapping. The only pipeline handler that declares the
> devnum mapping is the UVC pipeline handler, so remove the devnum there.
> 
> 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. Instead, we take all the
> video capture nodes (entities with the sink pad flag set).
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> ---
> Changes in v2:
> - pipeline handlers can no longer declare their own devnum
>   mapping.
>   - remove the uvc pipeline handler devnum mapping
> - cosmetic changes
> ---
>  include/libcamera/camera_manager.h            |  3 ++-
>  include/libcamera/internal/pipeline_handler.h |  2 +-
>  src/libcamera/camera_manager.cpp              | 17 ++++++-------
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  4 +---
>  src/libcamera/pipeline_handler.cpp            | 24 ++++++++++++-------
>  5 files changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 079f848a..95dc6360 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,
> +		       const std::vector<dev_t> &devnums);
>  	void removeCamera(Camera *camera);
>  
>  	static const std::string &version() { return version_; }
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 56968d14..22e629a8 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -91,7 +91,7 @@ public:
>  
>  protected:
>  	void registerCamera(std::shared_ptr<Camera> camera,
> -			    std::unique_ptr<CameraData> data, dev_t devnum = 0);
> +			    std::unique_ptr<CameraData> data);
>  	void hotplugMediaDevice(MediaDevice *media);
>  
>  	virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index da806fa7..3d055f78 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,
> +		       const std::vector<dev_t> &devnums);
>  	void removeCamera(Camera *camera);
>  
>  	/*
> @@ -168,7 +169,7 @@ void CameraManager::Private::cleanup()
>  }
>  
>  void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
> -				       dev_t devnum)
> +				       const std::vector<dev_t> &devnums)
>  {
>  	MutexLocker locker(mutex_);
>  
> @@ -183,10 +184,9 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
>  
>  	cameras_.push_back(std::move(camera));
>  
> -	if (devnum) {
> -		unsigned int index = cameras_.size() - 1;
> +	unsigned int index = cameras_.size() - 1;
> +	for (dev_t devnum : devnums)
>  		camerasByDevnum_[devnum] = cameras_[index];
> -	}
>  }
>  
>  void CameraManager::Private::removeCamera(Camera *camera)
> @@ -374,7 +374,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 +385,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,
> +			      const std::vector<dev_t> &devnums)
>  {
>  	ASSERT(Thread::current() == p_.get());
>  
> -	p_->addCamera(camera, devnum);
> +	p_->addCamera(camera, devnums);
>  }
>  
>  /**
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index a0749094..396bbe9b 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -396,12 +396,10 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	if (data->init(*entity))
>  		return false;
>  
> -	dev_t devnum = makedev((*entity)->deviceMajor(), (*entity)->deviceMinor());
> -

I think you can then drop inclusion of sys/sysmacros.h.

>  	/* Create and register the camera. */
>  	std::set<Stream *> streams{ &data->stream_ };
>  	std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> -	registerCamera(std::move(camera), std::move(data), devnum);
> +	registerCamera(std::move(camera), std::move(data));
>  
>  	/* Enable hot-unplug notifications. */
>  	hotplugMediaDevice(media);
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 53aeebdc..125e1779 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -481,28 +481,34 @@ void PipelineHandler::completeRequest(Camera *camera, Request *request)
>   * \brief Register a camera to the camera manager and pipeline handler
>   * \param[in] camera The camera to be added
>   * \param[in] data Pipeline-specific data for the camera
> - * \param[in] devnum Device number of the camera (optional)
>   *
>   * This method is called by pipeline handlers to register the cameras they
>   * handle with the camera manager. It associates the pipeline-specific \a data
>   * with the camera, for later retrieval with cameraData(). Ownership of \a data
>   * is transferred to the PipelineHandler.
>   *
> - * \a devnum is the device number (as returned by makedev) that the \a camera
> - * is to be associated with. This is for the V4L2 compatibility layer to map
> - * device nodes to Camera instances based on the device number
> - * registered by this method in \a devnum.
> - *
>   * \context This function shall be called from the CameraManager thread.
>   */
>  void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> -				     std::unique_ptr<CameraData> data,
> -				     dev_t devnum)
> +				     std::unique_ptr<CameraData> data)
>  {
>  	data->camera_ = camera.get();
>  	cameraData_[camera.get()] = std::move(data);
>  	cameras_.push_back(camera);
> -	manager_->addCamera(std::move(camera), devnum);
> +
> +	/*
> +	 * Walk the entity list and map the devnums of all capture video nodes
> +	 * to the camera.
> +	 */
> +	std::vector<dev_t> devnums;
> +	for (const std::shared_ptr<MediaDevice> media : mediaDevices_)
> +		for (const MediaEntity *entity : media->entities())

Even if not technically required, we tend to use curly braces around
statements that have substatements. That would apply to the two for
loops here.

> +			if (entity->pads().size() == 1 &&
> +			    (entity->pads()[0]->flags() & MEDIA_PAD_FL_SINK))
> +				devnums.push_back(makedev(entity->deviceMajor(),
> +							  entity->deviceMinor()));

Checking the number of pads isn't enough, there could be subdevs with a
single pads. You need to check that entity->function() ==
MEDIA_ENT_F_IO_V4L. I would keep the pads size check though, even if
likely redundant, to avoid a crash in case a video node would have no
pads.

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