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

[Laurent Pinchart]

On Wed, Aug 21, 2024 at 06:55:45PM +0200, Cheng-Hao Yang wrote:
> Hi Hans,
> 
> Thanks for the patch. Actually in mtkisp7, we did something very similar
> [1].
> (And I just caught a bug by checking your changes. Thanks :)
> 
> [1]:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900
> 
> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede <hdegoede at redhat.com> wrote:
> 
> > ATM libcamera always keeps all /dev/video# and /dev/v4l2_subdev# nodes
> > for a pipeline open after enumerating the camera.
> >
> > This is a problem for uvcvideo and atomisp /dev/video# nodes as well as
> > for VCM /dev/v4l2_subdev# nodes. Keeping these open stops the underlying
> > hw-devices 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 to delay opening /dev nodes until
> > the camera is acquired.
> >
> > Note pipeline handlers typically also will need access to /dev nodes
> > for their CameraConfig::validate() implementation for tryFormat() calls.
> > Making sure that /dev nodes are opened as necessary from validate() is
> > left up to the pipeline handler implementation.
> >
> > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > ---
> >  include/libcamera/internal/pipeline_handler.h |  3 +-
> >  src/libcamera/camera.cpp                      |  2 +-
> >  src/libcamera/pipeline_handler.cpp            | 43 +++++++++++++++----
> >  3 files changed, 37 insertions(+), 11 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..e1342306 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -163,20 +163,22 @@ 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()) {
> > -                       unlockMediaDevices();
> > -                       return false;
> > -               }
> > +       if (!acquireDevice(camera)) {
> > +               unlockMediaDevices();
> >
> 
> I think we should only unlock media devices if it's the first acquire
> being called, as there might be other ongoing usages on other
> cameras [2]. Therefore:
> ```
>                     if (useCount_ == 0)
>                             unlockMediaDevices();
> ```
> 
> [2]:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5525900/7/src/libcamera/pipeline_handler.cpp#181
> 
> +               return false;
> >         }
> >
> >         ++useCount_;
> > @@ -213,12 +215,35 @@ 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 open /dev nodes are allocate buffers when a camera is

s at open /dev nodes at opening devices/
s/are allocate/and allocating/

As discussed previously, I think we need to improve the
application-facing API to make resource and power management more
explicit. Camera::acquire() was never meant for this purpose. I
understand we need a short term solution to the power consumption issue
for UVC cameras without an enormous amount of yak shaving, and I'm fine
with this patch series as an interim measure, but I don't want to see
acquireDevice() being used in random ways by pipeline handlers other
than UVC before we address the more general problem. This should be
documented here.

> > + * acquired.

Please reflow these paragraphs to 80 columns.

> > + *
> > + * Note this is called once for every camera which is acquired,

s/which/that/

> > + * so if there are shared resources the pipeline handler must take

s/shared resources/resources shared by multiple cameras/

> > + * care to not release them until releaseDevice() has been called for
> > + * all previously acquired cameras.

Doesn't this paragraph belong to releaseDevice() ?

> > + *
> > + * \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.
> > + *
> > + * \sa acquireDevice()
> >   */
> >  void PipelineHandler::releaseDevice([[maybe_unused]] Camera *camera)
> >  {

-- 
Regards,

Laurent Pinchart