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

[Cheng-Hao Yang]

Thanks Hans for the patch.

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

On Thu, Aug 29, 2024 at 11:34 PM Laurent Pinchart <
laurent.pinchart at 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 at redhat.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240829/1743b342/attachment.htm>