[v2,1/3] pipeline_handler: Add acquireDevice() method to mirror existing releaseDevice()

21045 diff mbox series

libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes
for a pipeline open after enumerating the camera.

This is a problem for the uvcvideo pipeline handler. Keeping /dev/video#
open stops the UVC USB device from being able to enter runtime-suspend
causing significant unnecessary power-usage.

Add a stub acquireDevice() method to the PipelineHandler class which
pipeline handlers can override.

The uvcvideo pipeline handler will use this to delay opening /dev/video#
until the device is acquired. This is a special case because the kernel
uvcvideo driver powers on the USB device as soon as /dev/video# is opened.
This behavior should *not* be copied by other pipeline handlers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add a note to both the doxygen documentation as well as to the commit
  message that opening/closing /dev/video# from acquire()/release()
  as done by the uvcvideo pipeline handler is an exception and that this
  behavior should not be copied by other pipeline handlers
- Other doxygen doc fixes / improvements
- Only unlock media devices on acquireDevice() failure if useCount_ == 0
---
 include/libcamera/internal/pipeline_handler.h |  3 +-
 src/libcamera/camera.cpp                      |  2 +-
 src/libcamera/pipeline_handler.cpp            | 48 +++++++++++++++----
 3 files changed, 43 insertions(+), 10 deletions(-)

Hi Hans,

Thank you for the patch.

On Tue, Aug 27, 2024 at 06:42:53PM +0200, Hans de Goede wrote:
> libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes
> for a pipeline open after enumerating the camera.
> 
> This is a problem for the uvcvideo pipeline handler. Keeping /dev/video#
> open stops the UVC USB device from being able to enter runtime-suspend
> causing significant unnecessary power-usage.
> 
> Add a stub acquireDevice() method to the PipelineHandler class which

s/method/function/

same in the commit message. I was surprised when I found out that the
C++ specification uses the term "member function" and not "method".

> pipeline handlers can override.
> 
> The uvcvideo pipeline handler will use this to delay opening /dev/video#
> until the device is acquired. This is a special case because the kernel
> uvcvideo driver powers on the USB device as soon as /dev/video# is opened.
> This behavior should *not* be copied by other pipeline handlers.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes in v2:
> - Add a note to both the doxygen documentation as well as to the commit
>   message that opening/closing /dev/video# from acquire()/release()
>   as done by the uvcvideo pipeline handler is an exception and that this
>   behavior should not be copied by other pipeline handlers
> - Other doxygen doc fixes / improvements
> - Only unlock media devices on acquireDevice() failure if useCount_ == 0
> ---
>  include/libcamera/internal/pipeline_handler.h |  3 +-
>  src/libcamera/camera.cpp                      |  2 +-
>  src/libcamera/pipeline_handler.cpp            | 48 +++++++++++++++----
>  3 files changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index cad5812f..597f7c3e 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -45,7 +45,7 @@ public:
>  	MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,
>  					const DeviceMatch &dm);
>  
> -	bool acquire();
> +	bool acquire(Camera *camera);
>  	void release(Camera *camera);
>  
>  	virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> @@ -79,6 +79,7 @@ protected:
>  	virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
>  	virtual void stopDevice(Camera *camera) = 0;
>  
> +	virtual bool acquireDevice(Camera *camera);
>  	virtual void releaseDevice(Camera *camera);
>  
>  	CameraManager *manager_;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 382a68f7..4e393f89 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -995,7 +995,7 @@ int Camera::acquire()
>  	if (ret < 0)
>  		return ret == -EACCES ? -EBUSY : ret;
>  
> -	if (!d->pipe_->acquire()) {
> +	if (!d->pipe_->acquire(this)) {
>  		LOG(Camera, Info)
>  			<< "Pipeline handler in use by another process";
>  		return -EBUSY;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 1fc22d6a..861815cb 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -163,20 +163,24 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
>   * has already acquired it
>   * \sa release()
>   */
> -bool PipelineHandler::acquire()
> +bool PipelineHandler::acquire(Camera *camera)
>  {
>  	MutexLocker locker(lock_);
>  
> -	if (useCount_) {
> -		++useCount_;
> -		return true;
> +	if (useCount_ == 0) {
> +		for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> +			if (!media->lock()) {
> +				unlockMediaDevices();
> +				return false;
> +			}
> +		}
>  	}
>  
> -	for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> -		if (!media->lock()) {
> +	if (!acquireDevice(camera)) {
> +		if (useCount_ == 0)
>  			unlockMediaDevices();
> -			return false;
> -		}
> +
> +		return false;
>  	}
>  
>  	++useCount_;
> @@ -213,12 +217,40 @@ void PipelineHandler::release(Camera *camera)
>  	--useCount_;
>  }
>  
> +/**
> + * \brief Acquire resources associated with this camera
> + * \param[in] camera The camera for which to acquire resources
> + *
> + * Pipeline handlers may override this in order to get resources such as opening
> + * devices and allocating buffers when a camera is acquired.
> + *
> + * This is used by the uvcvideo pipeline handler to delay opening /dev/video#
> + * until the camera is acquired to avoid excess power consumption. The delayed
> + * opening of /dev/video# is a special case because the kernel uvcvideo driver
> + * powers on the USB device as soon as /dev/video# is opened. This behavior
> + * should *not* be copied by other pipeline handlers.
> + *
> + * \return True on success, false on failure
> + * \sa releaseDevice()
> + */
> +bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)
> +{
> +	return true;
> +}
> +
>  /**
>   * \brief Release resources associated with this camera
>   * \param[in] camera The camera for which to release resources
>   *
>   * Pipeline handlers may override this in order to perform cleanup operations
>   * when a camera is released, such as freeing memory.
> + *
> + * This is called once for every camera that is released. If there are resources
> + * shared by multiple cameras then the pipeline handler must take care to not
> + * release them until releaseDevice() has been called for all previously
> + * acquired cameras.
> + *
> + * \sa acquireDevice()
>   */
>  void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
>  {

Thanks Hans for the patch.

Reviewed-by: Harvey Yang <chenghaoyang@chromium.org>

On Thu, Aug 29, 2024 at 11:34 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Hans,
>
> Thank you for the patch.
>
> On Tue, Aug 27, 2024 at 06:42:53PM +0200, Hans de Goede wrote:
> > libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes
> > for a pipeline open after enumerating the camera.
> >
> > This is a problem for the uvcvideo pipeline handler. Keeping /dev/video#
> > open stops the UVC USB device from being able to enter runtime-suspend
> > causing significant unnecessary power-usage.
> >
> > Add a stub acquireDevice() method to the PipelineHandler class which
>
> s/method/function/
>
> same in the commit message. I was surprised when I found out that the
> C++ specification uses the term "member function" and not "method".
>
> > pipeline handlers can override.
> >
> > The uvcvideo pipeline handler will use this to delay opening /dev/video#
> > until the device is acquired. This is a special case because the kernel
> > uvcvideo driver powers on the USB device as soon as /dev/video# is
> opened.
> > This behavior should *not* be copied by other pipeline handlers.
> >
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > ---
> > Changes in v2:
> > - Add a note to both the doxygen documentation as well as to the commit
> >   message that opening/closing /dev/video# from acquire()/release()
> >   as done by the uvcvideo pipeline handler is an exception and that this
> >   behavior should not be copied by other pipeline handlers
> > - Other doxygen doc fixes / improvements
> > - Only unlock media devices on acquireDevice() failure if useCount_ == 0
> > ---
> >  include/libcamera/internal/pipeline_handler.h |  3 +-
> >  src/libcamera/camera.cpp                      |  2 +-
> >  src/libcamera/pipeline_handler.cpp            | 48 +++++++++++++++----
> >  3 files changed, 43 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/libcamera/internal/pipeline_handler.h
> b/include/libcamera/internal/pipeline_handler.h
> > index cad5812f..597f7c3e 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -45,7 +45,7 @@ public:
> >       MediaDevice *acquireMediaDevice(DeviceEnumerator *enumerator,
> >                                       const DeviceMatch &dm);
> >
> > -     bool acquire();
> > +     bool acquire(Camera *camera);
> >       void release(Camera *camera);
> >
> >       virtual std::unique_ptr<CameraConfiguration>
> generateConfiguration(Camera *camera,
> > @@ -79,6 +79,7 @@ protected:
> >       virtual int queueRequestDevice(Camera *camera, Request *request) =
> 0;
> >       virtual void stopDevice(Camera *camera) = 0;
> >
> > +     virtual bool acquireDevice(Camera *camera);
> >       virtual void releaseDevice(Camera *camera);
> >
> >       CameraManager *manager_;
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 382a68f7..4e393f89 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -995,7 +995,7 @@ int Camera::acquire()
> >       if (ret < 0)
> >               return ret == -EACCES ? -EBUSY : ret;
> >
> > -     if (!d->pipe_->acquire()) {
> > +     if (!d->pipe_->acquire(this)) {
> >               LOG(Camera, Info)
> >                       << "Pipeline handler in use by another process";
> >               return -EBUSY;
> > diff --git a/src/libcamera/pipeline_handler.cpp
> b/src/libcamera/pipeline_handler.cpp
> > index 1fc22d6a..861815cb 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -163,20 +163,24 @@ MediaDevice
> *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
> >   * has already acquired it
> >   * \sa release()
> >   */
> > -bool PipelineHandler::acquire()
> > +bool PipelineHandler::acquire(Camera *camera)
> >  {
> >       MutexLocker locker(lock_);
> >
> > -     if (useCount_) {
> > -             ++useCount_;
> > -             return true;
> > +     if (useCount_ == 0) {
> > +             for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> > +                     if (!media->lock()) {
> > +                             unlockMediaDevices();
> > +                             return false;
> > +                     }
> > +             }
> >       }
> >
> > -     for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
> > -             if (!media->lock()) {
> > +     if (!acquireDevice(camera)) {
> > +             if (useCount_ == 0)
> >                       unlockMediaDevices();
> > -                     return false;
> > -             }
> > +
> > +             return false;
> >       }
> >
> >       ++useCount_;
> > @@ -213,12 +217,40 @@ void PipelineHandler::release(Camera *camera)
> >       --useCount_;
> >  }
> >
> > +/**
> > + * \brief Acquire resources associated with this camera
> > + * \param[in] camera The camera for which to acquire resources
> > + *
> > + * Pipeline handlers may override this in order to get resources such
> as opening
> > + * devices and allocating buffers when a camera is acquired.
> > + *
> > + * This is used by the uvcvideo pipeline handler to delay opening
> /dev/video#
> > + * until the camera is acquired to avoid excess power consumption. The
> delayed
> > + * opening of /dev/video# is a special case because the kernel uvcvideo
> driver
> > + * powers on the USB device as soon as /dev/video# is opened. This
> behavior
> > + * should *not* be copied by other pipeline handlers.
> > + *
> > + * \return True on success, false on failure
> > + * \sa releaseDevice()
> > + */
> > +bool PipelineHandler::acquireDevice([[maybe_unused]] Camera *camera)
> > +{
> > +     return true;
> > +}
> > +
> >  /**
> >   * \brief Release resources associated with this camera
> >   * \param[in] camera The camera for which to release resources
> >   *
> >   * Pipeline handlers may override this in order to perform cleanup
> operations
> >   * when a camera is released, such as freeing memory.
> > + *
> > + * This is called once for every camera that is released. If there are
> resources
> > + * shared by multiple cameras then the pipeline handler must take care
> to not
> > + * release them until releaseDevice() has been called for all previously
> > + * acquired cameras.
> > + *
> > + * \sa acquireDevice()
> >   */
> >  void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> >  {
>
> --
> Regards,
>
> Laurent Pinchart
>

BR,
Harvey