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

[Laurent Pinchart]

On Fri, Aug 30, 2024 at 10:01:01AM +0200, Hans de Goede wrote:
> 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 ?

Yes that's what I meant sorry.

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

Fine with me.

> With those remarks I agree with your suggestion and I'll implement
> this change for v3.
> 
> >> @@ -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