[PATCH v2 1/3] pipeline_handler: Add acquireDevice() method to mirror existing releaseDevice()

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Aug 27, 2024 at 06:42:53PM +0200, Hans de Goede wrote:
> libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes
> for a pipeline open after enumerating the camera.
> 
> This is a problem for the uvcvideo pipeline handler. Keeping /dev/video#
> open stops the UVC USB device from being able to enter runtime-suspend
> causing significant unnecessary power-usage.
> 
> Add a stub acquireDevice() method to the PipelineHandler class which

s/method/function/

same in the commit message. I was surprised when I found out that the
C++ specification uses the term "member function" and not "method".

> pipeline handlers can override.
> 
> The uvcvideo pipeline handler will use this to delay opening /dev/video#
> until the device is acquired. This is a special case because the kernel
> uvcvideo driver powers on the USB device as soon as /dev/video# is opened.
> This behavior should *not* be copied by other pipeline handlers.
> 
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>

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

> ---
> Changes in v2:
> - Add a note to both the doxygen documentation as well as to the commit
>   message that opening/closing /dev/video# from acquire()/release()
>   as done by the uvcvideo pipeline handler is an exception and that this
>   behavior should not be copied by other pipeline handlers
> - Other doxygen doc fixes / improvements
> - Only unlock media devices on acquireDevice() failure if useCount_ == 0
> ---
>  include/libcamera/internal/pipeline_handler.h |  3 +-
>  src/libcamera/camera.cpp                      |  2 +-
>  src/libcamera/pipeline_handler.cpp            | 48 +++++++++++++++----
>  3 files changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index cad5812f..597f7c3e 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -45,7 +45,7 @@ public:
>  	MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,
>  					const DeviceMatch &dm);
>  
> -	bool acquire();
> +	bool acquire(Camera *camera);
>  	void release(Camera *camera);
>  
>  	virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> @@ -79,6 +79,7 @@ protected:
>  	virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
>  	virtual void stopDevice(Camera *camera) = 0;
>  
> +	virtual bool acquireDevice(Camera *camera);
>  	virtual void releaseDevice(Camera *camera);
>  
>  	CameraManager *manager_;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 382a68f7..4e393f89 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -995,7 +995,7 @@ int Camera::acquire()
>  	if (ret < 0)
>  		return ret == -EACCES ? -EBUSY : ret;
>  
> -	if (!d->pipe_->acquire()) {
> +	if (!d->pipe_->acquire(this)) {
>  		LOG(Camera, Info)
>  			<< "Pipeline handler in use by another process";
>  		return -EBUSY;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 1fc22d6a..861815cb 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -163,20 +163,24 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
>   * has already acquired it
>   * \sa release()
>   */
> -bool PipelineHandler::acquire()
> +bool PipelineHandler::acquire(Camera *camera)
>  {
>  	MutexLocker locker(lock_);
>  
> -	if (useCount_) {
> -		++useCount_;
> -		return true;
> +	if (useCount_ == 0) {
> +		for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> +			if (!media->lock()) {
> +				unlockMediaDevices();
> +				return false;
> +			}
> +		}
>  	}
>  
> -	for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> -		if (!media->lock()) {
> +	if (!acquireDevice(camera)) {
> +		if (useCount_ == 0)
>  			unlockMediaDevices();
> -			return false;
> -		}
> +
> +		return false;
>  	}
>  
>  	++useCount_;
> @@ -213,12 +217,40 @@ void PipelineHandler::release(Camera *camera)
>  	--useCount_;
>  }
>  
> +/**
> + * \brief Acquire resources associated with this camera
> + * \param[in] camera The camera for which to acquire resources
> + *
> + * Pipeline handlers may override this in order to get resources such as opening
> + * devices and allocating buffers when a camera is acquired.
> + *
> + * This is used by the uvcvideo pipeline handler to delay opening /dev/video#
> + * until the camera is acquired to avoid excess power consumption. The delayed
> + * opening of /dev/video# is a special case because the kernel uvcvideo driver
> + * powers on the USB device as soon as /dev/video# is opened. This behavior
> + * should *not* be copied by other pipeline handlers.
> + *
> + * \return True on success, false on failure
> + * \sa releaseDevice()
> + */
> +bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)
> +{
> +	return true;
> +}
> +
>  /**
>   * \brief Release resources associated with this camera
>   * \param[in] camera The camera for which to release resources
>   *
>   * Pipeline handlers may override this in order to perform cleanup operations
>   * when a camera is released, such as freeing memory.
> + *
> + * This is called once for every camera that is released. If there are resources
> + * shared by multiple cameras then the pipeline handler must take care to not
> + * release them until releaseDevice() has been called for all previously
> + * acquired cameras.
> + *
> + * \sa acquireDevice()
>   */
>  void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
>  {

-- 
Regards,

Laurent Pinchart