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

[Hans de Goede]

Hi,

On 8/21/24 6:55 PM, 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 <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 <mailto: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 <mailto: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();
> ```

Good point, fixed for v2.

Regards,

Hans