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

[Hans de Goede]

Hi Laurent,

On 8/29/24 11:58 PM, Laurent Pinchart wrote:
> 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();
> 	}

I assume you meant to move the "MutexLocker locker(data_->openLock_);"
to inside the { } scope here, so that its lifetime is limited to
that scope ?

> 	if (ret)
> 		return Invalid;

I would prefer to have this inside the { } scope since that is where
it logically belongs and this check will be so fast that it does not
matter wrt holding the lock longer.

With those remarks I agree with your suggestion and I'll implement
this change for v3.

Regards,

Hans




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