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

[Cheng-Hao Yang]

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
> + * acquired.
> + *
> + * Note this is called once for every camera which is acquired,
> + * so if there are shared resources the pipeline handler must take
> + * care to not release them until releaseDevice() has been called for
> + * all previously acquired cameras.
> + *
> + * \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)
>  {
> --
> 2.46.0
>
>
BR,
Harvey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240821/08601c9c/attachment.htm>