[libcamera-devel] [PATCH v2 1/2] libcamera: media_device: Move recursive lock handling to pipeline handler

[Kieran Bingham]

Quoting Laurent Pinchart (2021-12-26 23:12:54)
> The MediaDevice lock is meant to prevent concurrent usage of multiple
> cameras from the same pipeline handlers. As media devices are acquired
> by pipeline handlers, we can't have multiple pipeline handlers trying to
> lock the same media device. The recursive locking detection can thus be
> moved to the pipeline handler. This simplifies the media device
> implementation that now implements true lock semantics, and prepares for
> support of concurrent camera usage.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
> Changes since v1:
> 
> - Fix lock owner check un PipelineHandler::unlock()
> ---
>  include/libcamera/internal/media_device.h     |  1 -
>  include/libcamera/internal/pipeline_handler.h |  2 ++
>  src/libcamera/media_device.cpp                | 14 +-------------
>  src/libcamera/pipeline_handler.cpp            | 13 ++++++++++++-
>  4 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h
> index af0b25b731eb..6e2a63f38229 100644
> --- a/include/libcamera/internal/media_device.h
> +++ b/include/libcamera/internal/media_device.h
> @@ -86,7 +86,6 @@ private:
>         UniqueFD fd_;
>         bool valid_;
>         bool acquired_;
> -       bool lockOwner_;
>  
>         std::map<unsigned int, MediaObject *> objects_;
>         std::vector<MediaEntity *> entities_;
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index ec986a518b5c..a7331cedfb16 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -88,6 +88,8 @@ private:
>  
>         const char *name_;
>  
> +       bool lockOwner_;
> +
>         friend class PipelineHandlerFactory;
>  };
>  
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 0b7940182d0c..941f86c25f66 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -63,8 +63,7 @@ LOG_DEFINE_CATEGORY(MediaDevice)
>   * populate() before the media graph can be queried.
>   */
>  MediaDevice::MediaDevice(const std::string &deviceNode)
> -       : deviceNode_(deviceNode), valid_(false), acquired_(false),
> -         lockOwner_(false)
> +       : deviceNode_(deviceNode), valid_(false), acquired_(false)
>  {
>  }
>  
> @@ -145,15 +144,9 @@ bool MediaDevice::lock()
>         if (!fd_.isValid())
>                 return false;
>  
> -       /* Do not allow nested locking in the same libcamera instance. */
> -       if (lockOwner_)
> -               return false;
> -
>         if (lockf(fd_.get(), F_TLOCK, 0))
>                 return false;
>  
> -       lockOwner_ = true;
> -
>         return true;
>  }
>  
> @@ -171,11 +164,6 @@ void MediaDevice::unlock()
>         if (!fd_.isValid())
>                 return;
>  
> -       if (!lockOwner_)
> -               return;
> -
> -       lockOwner_ = false;
> -
>         lockf(fd_.get(), F_ULOCK, 0);
>  }
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 0bc0e341b9e7..a9241ac62979 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -67,7 +67,7 @@ LOG_DEFINE_CATEGORY(Pipeline)
>   * respective factories.
>   */
>  PipelineHandler::PipelineHandler(CameraManager *manager)
> -       : manager_(manager)
> +       : manager_(manager), lockOwner_(false)
>  {
>  }
>  
> @@ -155,6 +155,10 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
>   */
>  bool PipelineHandler::lock()
>  {
> +       /* Do not allow nested locking in the same libcamera instance. */
> +       if (lockOwner_)
> +               return false;
> +
>         for (std::shared_ptr<MediaDevice> &media : mediaDevices_) {
>                 if (!media->lock()) {
>                         unlock();
> @@ -162,6 +166,8 @@ bool PipelineHandler::lock()
>                 }
>         }
>  
> +       lockOwner_ = true;
> +
>         return true;
>  }
>  
> @@ -177,8 +183,13 @@ bool PipelineHandler::lock()
>   */
>  void PipelineHandler::unlock()
>  {
> +       if (!lockOwner_)
> +               return;
> +
>         for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
>                 media->unlock();
> +
> +       lockOwner_ = false;
>  }
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
>