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

[Laurent Pinchart]

Hi Kieran,

On Thu, Dec 23, 2021 at 01:35:46PM +0000, Kieran Bingham wrote:
> 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.

I'll add a comment to lockOwner_ and annotate it with
LIBCAMERA_TSA_GUARDED_BY(lock_).

> >  
> >         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