[libcamera-devel,v4,2/4] libcamera: camera_manager, pipeline_handler: allow retrieving cameras by device numbers

2479 diff mbox series

The V4L2 compatibility layer will need a way to map device numbers to
libcamera Camera instances. Expose a method in the camera manager to
retrieve Camera instances by devnum. The mapping from device numbers to
Camera instances is optionally declared by pipeline handlers when they
register cameras with the camera manager.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

---
Changes in v4:
- squashed 4/6 from v3
- simplified registering devnum -> camera mappings
  - old (v3) system: pipeline handlers have their own map, that camera
    manager aggregates
  - new (v4) system: pipeline handlers, when they register the camera
    with the camera manager, (optionally) declare the devnum along with it

New in v3
---
 include/libcamera/camera_manager.h       |  5 +++-
 src/libcamera/camera_manager.cpp         | 37 +++++++++++++++++++++++-
 src/libcamera/include/pipeline_handler.h |  3 +-
 src/libcamera/pipeline_handler.cpp       | 13 +++++++--
 4 files changed, 53 insertions(+), 5 deletions(-)

Hi Paul,

Thank you for the patch.

On Mon, Dec 30, 2019 at 11:33:12PM -0600, Paul Elder wrote:
> The V4L2 compatibility layer will need a way to map device numbers to
> libcamera Camera instances. Expose a method in the camera manager to
> retrieve Camera instances by devnum. The mapping from device numbers to
> Camera instances is optionally declared by pipeline handlers when they
> register cameras with the camera manager.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> ---
> Changes in v4:
> - squashed 4/6 from v3
> - simplified registering devnum -> camera mappings
>   - old (v3) system: pipeline handlers have their own map, that camera
>     manager aggregates
>   - new (v4) system: pipeline handlers, when they register the camera
>     with the camera manager, (optionally) declare the devnum along with it
> 
> New in v3
> ---
>  include/libcamera/camera_manager.h       |  5 +++-
>  src/libcamera/camera_manager.cpp         | 37 +++++++++++++++++++++++-
>  src/libcamera/include/pipeline_handler.h |  3 +-
>  src/libcamera/pipeline_handler.cpp       | 13 +++++++--
>  4 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 8331898c..d2d6e443 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_CAMERA_MANAGER_H__
>  #define __LIBCAMERA_CAMERA_MANAGER_H__
>  
> +#include <map>
>  #include <memory>
>  #include <string>

Should you include <sys/types.h> for dev_t ?

>  #include <vector>
> @@ -33,8 +34,9 @@ public:
>  
>  	const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
>  	std::shared_ptr<Camera> get(const std::string &name);
> +	std::shared_ptr<Camera> get(dev_t devnum);
>  
> -	void addCamera(std::shared_ptr<Camera> camera);
> +	void addCamera(std::shared_ptr<Camera> camera, dev_t devnum);
>  	void removeCamera(Camera *camera);
>  
>  	static const std::string &version() { return version_; }
> @@ -46,6 +48,7 @@ private:
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>  	std::vector<std::shared_ptr<Camera>> cameras_;
> +	std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
>  
>  	static const std::string version_;
>  	static CameraManager *self_;
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 7c6f72bb..b6204872 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -180,15 +180,45 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &name)
>  	return nullptr;
>  }
>  
> +/**
> + * \brief Retrieve a camera based on device number
> + * \param[in] devnum Device number of camera to get
> + *
> + * Retrieving a camera based on device number is done by the V4L2 compatibility
> + * layer to map device nodes to libcamera Camera instances.
> + *
> + * Regular applications are recommended to retrieve libcamera Camera instances
> + * by name instead.

I'd make this a stronger requirement:

 * This method is meant solely for the use of the V4L2 compatibility
 * layer, to map device nodes to Camera instances. Applications shall
 * not use it and shall instead retrieve cameras by name.

> + *
> + * Before calling this function the caller is responsible for ensuring that
> + * the camera manager is running.
> + *
> + * \return Shared pointer to Camera object, which is empty if the camera is
> + * not found
> + */
> +std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> +{
> +	auto iter = camerasByDevnum_.find(devnum);
> +

I think you can drop this blank line.

> +	if (iter == camerasByDevnum_.end())
> +		return nullptr;
> +
> +	return iter->second.lock();
> +}
> +
>  /**
>   * \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
>   *
>   * This function is called by pipeline handlers to register the cameras they
>   * handle with the camera manager. Registered cameras are immediately made
>   * available to the system.
> + *
> + * \a devnum is used by the V4L2 compatibility layer to map V4L2 device nodes
> + * to libcamera Camera instances.

s/libcamera // as we're in libcamera, so this is the default :-)

>   */
> -void CameraManager::addCamera(std::shared_ptr<Camera> camera)
> +void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
>  {
>  	for (std::shared_ptr<Camera> c : cameras_) {
>  		if (c->name() == camera->name()) {
> @@ -200,6 +230,11 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera)
>  	}
>  
>  	cameras_.push_back(std::move(camera));
> +
> +	if (devnum) {
> +		unsigned int index = cameras_.size() - 1;
> +		camerasByDevnum_[devnum] = cameras_[index];
> +	}
>  }

You also need to update CameraManager::removeCamera.

diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 7c6f72bbe120..d45d81effa8d 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -212,15 +212,17 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera)
  */
 void CameraManager::removeCamera(Camera *camera)
 {
-	for (auto iter = cameras_.begin(); iter != cameras_.end(); ++iter) {
-		if (iter->get() == camera) {
-			LOG(Camera, Debug)
-				<< "Unregistering camera '"
-				<< camera->name() << "'";
-			cameras_.erase(iter);
-			return;
-		}
-	}
+	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
+				 [camera](std::shared_ptr<Camera> &c) {
+					return c.get() == camera;
+				 });
+	if (iter == cameras_.end())
+		return;
+
+	LOG(Camera, Debug)
+		<< "Unregistering camera '" << camera->name() << "'";
+
+	cameras_.erase(iter);
 }

 /**

(untested) and an additional std::find_if at the end for
camerasByDevnum_.

>  
>  /**
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index f3622631..067baef5 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -12,6 +12,7 @@
>  #include <memory>
>  #include <set>
>  #include <string>
> +#include <sys/sysmacros.h>
>  #include <vector>
>  
>  #include <ipa/ipa_interface.h>
> @@ -86,7 +87,7 @@ public:
>  
>  protected:
>  	void registerCamera(std::shared_ptr<Camera> camera,
> -			    std::unique_ptr<CameraData> data);
> +			    std::unique_ptr<CameraData> data, dev_t devnum = 0);
>  	void hotplugMediaDevice(MediaDevice *media);
>  
>  	virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 5badf31c..942b1a30 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -7,6 +7,8 @@
>  
>  #include "pipeline_handler.h"
>  
> +#include <sys/sysmacros.h>
> +
>  #include <libcamera/buffer.h>
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
> @@ -438,19 +440,26 @@ 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 libcamera Camera instances based on the device number

s/libcamera //

> + * registered by this method in \a devnum.
>   */
>  void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera,
> -				     std::unique_ptr<CameraData> data)
> +				     std::unique_ptr<CameraData> data,
> +				     dev_t devnum)
>  {
>  	data->camera_ = camera.get();
>  	cameraData_[camera.get()] = std::move(data);
>  	cameras_.push_back(camera);
> -	manager_->addCamera(std::move(camera));
> +	manager_->addCamera(std::move(camera), devnum);
>  }
>  
>  /**

With the above issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>