[libcamera-devel] [PATCH v3 5/5] libcamera: pipeline: Register device numbers with camera

[Jacopo Mondi]

Hi Kieran

On Mon, May 15, 2023 at 01:45:50PM +0100, Kieran Bingham via libcamera-devel wrote:
> Register the identified device numbers with each camera as the Devices
> property.
>
> This facilitates camera daemons or other systems to identify which
> devices are being managed by libcamera, and can prevent duplication of
> camera resources.
>
> As the Devices property now provides this list of devices, use it
> directly from within the CameraManager when adding a Camera rather than
> passing it through the internal API.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/camera_manager.h |  3 +--
>  src/libcamera/camera_manager.cpp            | 14 +++++++++-----
>  src/libcamera/pipeline_handler.cpp          | 12 ++++++++++--
>  3 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> index 885bb2825687..680e1f58cb02 100644
> --- a/include/libcamera/internal/camera_manager.h
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -29,8 +29,7 @@ public:
>  	Private();
>
>  	int start();
> -	void addCamera(std::shared_ptr<Camera> camera,
> -		       const std::vector<dev_t> &devnums) LIBCAMERA_TSA_EXCLUDES(mutex_);
> +	void addCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
>  	void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
>
>  	/*
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 70eb4e455e54..7e499def9ddc 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -15,7 +15,9 @@
>  #include <libcamera/base/utils.h>
>
>  #include <libcamera/camera.h>
> +#include <libcamera/property_ids.h>
>
> +#include "libcamera/internal/camera.h"

This already includes <libcamera/camera.h>

doesn't hurt though

>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/pipeline_handler.h"
>
> @@ -150,19 +152,17 @@ void CameraManager::Private::cleanup()
>  /**
>   * \brief Add a camera to the camera manager
>   * \param[in] camera The camera to be added
> - * \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
>   * available to the system.
>   *
> - * \a devnums are used by the V4L2 compatibility layer to map V4L2 device nodes
> - * to Camera instances.
> + * Device numbers from the Devices property are used by the V4L2 compatibility
> + * layer to map V4L2 device nodes to Camera instances.
>   *
>   * \context This function shall be called from the CameraManager thread.
>   */
> -void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
> -				       const std::vector<dev_t> &devnums)
> +void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera)
>  {
>  	ASSERT(Thread::current() == this);
>
> @@ -177,6 +177,10 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera,
>  		}
>  	}
>
> +	auto devnums = camera->properties()
> +			       .get(properties::Devices)
> +			       .value_or(Span<int64_t>{});
> +

Weird indent

	auto devnums = camera->properties().get(properties::Devices)
			                   .value_or(Span<int64_t>{});

?

I guess we don't need to make sure devnums is populated, right ?

>  	cameras_.push_back(std::move(camera));
>
>  	unsigned int index = cameras_.size() - 1;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 49092ea88a58..0b5fac7ad113 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -17,6 +17,7 @@
>
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer.h>
> +#include <libcamera/property_ids.h>
>
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_manager.h"
> @@ -612,7 +613,7 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
>  	 * Walk the entity list and map the devnums of all capture video nodes
>  	 * to the camera.
>  	 */
> -	std::vector<dev_t> devnums;
> +	std::vector<int64_t> devnums;
>  	for (const std::shared_ptr<MediaDevice> &media : mediaDevices_) {
>  		for (const MediaEntity *entity : media->entities()) {
>  			if (entity->pads().size() == 1 &&
> @@ -624,7 +625,14 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
>  		}
>  	}
>
> -	manager_->_d()->addCamera(std::move(camera), devnums);
> +	/*
> +	 * Store the associated devices as a property of the camera to allow
> +	 * systems to identify which devices are managed by libcamera.
> +	 */
> +	Camera::Private *data = camera->_d();
> +	data->properties_.set(properties::Devices, devnums);
> +
> +	manager_->_d()->addCamera(std::move(camera));

The rest looks good
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>  }
>
>  /**
> --
> 2.34.1
>