[libcamera-devel] [PATCH 2/2] libcamera: pipeline_handler: Make lock() and unlock() thread-safe

[Kieran Bingham]

Quoting Laurent Pinchart (2021-12-23 02:33:31)
> The PipelineHandler lock() and unlock() functions are documented as
> thread-safe, but they're not. Fix them using a mutex.

A lock for a lock ...

Seems fine though, but a comment below

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

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/internal/pipeline_handler.h | 2 ++
>  src/libcamera/pipeline_handler.cpp            | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index a7331cedfb16..3768bb8f6cb6 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -14,6 +14,7 @@
>  #include <sys/types.h>
>  #include <vector>
>  
> +#include <libcamera/base/mutex.h>
>  #include <libcamera/base/object.h>
>  
>  #include <libcamera/controls.h>
> @@ -88,6 +89,7 @@ private:
>  
>         const char *name_;
>  
> +       Mutex lock_;
>         bool lockOwner_;

Do we need to comment these at all? lock_ and lockOwner_ while closely
linked, could be confusing.

The lockOwner_ afterall does /not/ have any correspondance to the owner
of lock_.

A comment above each would probably suffice.

>  
>         friend class PipelineHandlerFactory;
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index b2ee4ec0ce87..07d9d387daff 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -11,6 +11,7 @@
>  #include <sys/sysmacros.h>
>  
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/mutex.h>
>  #include <libcamera/base/utils.h>
>  
>  #include <libcamera/camera.h>
> @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
>   */
>  bool PipelineHandler::lock()
>  {
> +       MutexLocker locker(lock_);
> +
>         /* Do not allow nested locking in the same libcamera instance. */
>         if (lockOwner_)
>                 return false;
> @@ -183,6 +186,8 @@ bool PipelineHandler::lock()
>   */
>  void PipelineHandler::unlock()
>  {
> +       MutexLocker locker(lock_);
> +
>         if (lockOwner_)
>                 return;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>