[PATCH v2 3/3] uvcvideo: Implement acquireDevice() + releaseDevice()

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Aug 27, 2024 at 06:42:55PM +0200, Hans de Goede wrote:
> The uvcvideo pipeline handler always keeps the uvcvideo /dev/video# node
> for a pipeline open after enumerating the camera.
> 
> This is a problem for uvcvideo, as keeping the /dev/video# node open stops
> the underlying USB device and the USB bus controller from being able to
> enter runtime-suspend causing significant unnecessary power-usage.
> 
> Implement acquireDevice() + releaseDevice(), openening /dev/video# on
> acquire and closing it on release to fix this.
> 
> And make validate do a local video_->open() + close() around validate()
> when not open yet. To keep validate() working on unacquired cameras.

s/yet. To/yet, to/

> Link: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2669

I'd replace this with

Bug: https://bugs.libcamera.org/show_bug.cgi?id=168

which links to the above pipewire issue.

> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
> Changes in v2:
> - Minor commit msg + code improvements
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 45 ++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 8a7409fc..584f5230 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -13,6 +13,7 @@
>  #include <tuple>
>  
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/mutex.h>
>  #include <libcamera/base/utils.h>
>  
>  #include <libcamera/camera.h>
> @@ -48,6 +49,7 @@ public:
>  
>  	const std::string &id() const { return id_; }
>  
> +	Mutex openLock_;
>  	std::unique_ptr<V4L2VideoDevice> video_;
>  	Stream stream_;
>  	std::map<PixelFormat, std::vector<SizeRange>> formats_;
> @@ -93,6 +95,9 @@ private:
>  			   const ControlValue &value);
>  	int processControls(UVCCameraData *data, Request *request);
>  
> +	bool acquireDevice(Camera *camera) override;
> +	void releaseDevice(Camera *camera) override;
> +
>  	UVCCameraData *cameraData(Camera *camera)
>  	{
>  		return static_cast<UVCCameraData *>(camera->_d());
> @@ -107,6 +112,7 @@ UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data)
>  CameraConfiguration::Status UVCCameraConfiguration::validate()
>  {
>  	Status status = Valid;
> +	bool opened = false;

I would move this below.

>  
>  	if (config_.empty())
>  		return Invalid;
> @@ -158,7 +164,23 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
>  	format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
>  	format.size = cfg.size;
>  
> +	/*
> +	 * For power-consumption reasons video_ is closed when the camera
> +	 * is not acquired. Open it here if necessary.
> +	 */

Here.

> +	MutexLocker locker(data_->openLock_);
> +
> +	if (!data_->video_->isOpen()) {
> +		int ret = data_->video_->open();
> +		if (ret)
> +			return Invalid;
> +
> +		opened = true;
> +	}
> +
>  	int ret = data_->video_->tryFormat(&format);
> +	if (opened)
> +		data_->video_->close();
>  	if (ret)
>  		return Invalid;

The code below doesn't need to be protected by the lock.

	bool opened = false;
	int ret;

	MutexLocker locker(data_->openLock_);

	{
		if (!data_->video_->isOpen()) {
			ret = data_->video_->open();
			if (ret)
				return Invalid;

			opened = true;
		}

		ret = data_->video_->tryFormat(&format);
		if (opened)
			data_->video_->close();
	}

	if (ret)
		return Invalid;

>  
> @@ -411,6 +433,23 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	return true;
>  }
>  
> +bool PipelineHandlerUVC::acquireDevice(Camera *camera)
> +{
> +	UVCCameraData *data = cameraData(camera);
> +
> +	MutexLocker locker(data->openLock_);
> +
> +	return data->video_->open() == 0;
> +}
> +
> +void PipelineHandlerUVC::releaseDevice(Camera *camera)
> +{
> +	UVCCameraData *data = cameraData(camera);
> +
> +	MutexLocker locker(data->openLock_);
> +	data->video_->close();
> +}
> +
>  int UVCCameraData::init(MediaDevice *media)
>  {
>  	int ret;
> @@ -512,6 +551,12 @@ int UVCCameraData::init(MediaDevice *media)
>  
>  	controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
>  
> +	/*
> +	 * Close to allow camera to go into runtime-suspend, video_
> +	 * will be re-opened from acquireDevice() and validate().

You can reflow this to 80 columns.

With these small issues addressed,

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

> +	 */
> +	video_->close();
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart