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

[Hans de Goede]

Hi Laurent,

On 8/25/24 3:01 AM, Laurent Pinchart wrote:
> 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 ?

Yes, but the notifiers are also destroyed again before any other code actually
tries to use use them.

Either the camera is already acquired and no open/close of video_ is done,
or it is not acquired and both open + close of video_ are done without
any other code-paths being able to use video_ in the mean time.

The openLock_ mutex protects against acquireDevice() / releaseDevice()
racing with the validate() call and all other users of video_ require
the camera to be acquired first.

Note no open-counting is done since there with the uvcvideo pipeline
handler there only ever is one camera.

Regards,

Hans




> 
>> 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;
>>  }
>>  
>