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

[Laurent Pinchart]

Hi Hans,

On Mon, Aug 26, 2024 at 10:17:17AM +0200, Hans de Goede wrote:
> On 8/25/24 3:01 AM, Laurent Pinchart wrote:
> > 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.

True, it shouldn't cause any problem in practice, but it makes me feel a
bit uncomfortable still.

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

I can live with this for the uvcvideo pipeline handler for the time
being. Let's make sure it doesn't spread before the API improves. To be
honest, I'm a bit concerned than shifting the yak shaving to the second
user on the grounds that we need more than a single use case to do
things right will be met with unhappiness on the grounds that the first
user got away with it so it's not fair. I suppose doing things the right
but hard way will always be met with this kind of problems :-)

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