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

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tue, Aug 20, 2024 at 09:50:16PM +0200, Hans de Goede wrote:
> ATM the uvcvideo pipeline handler always keeps the uvcvideo /dev/video#

s/ATM/At the moment/

(or just drop it)

> 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.

Won't you hit the same issue that 4/5 is meant to fix, with the
notifiers being created from the wrong thread ?

> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 8a7409fc..d3eedfdc 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;
>  
>  	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.
> +	 */
> +	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;
>  
> @@ -411,6 +433,24 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	return true;
>  }
>  
> +bool PipelineHandlerUVC::acquireDevice(Camera *camera)
> +{
> +	UVCCameraData *data = cameraData(camera);
> +
> +	MutexLocker locker(data->openLock_);
> +
> +	int ret = data->video_->open();
> +	return ret == 0;

You can simplify this to

	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 +552,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().
> +	 */
> +	video_->close();
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart