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

[Laurent Pinchart]

On Thu, Dec 23, 2021 at 01:31:57PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-12-23 02:33:30)
> > 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>
> > ---
> >  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..b2ee4ec0ce87 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_)
> 
>  if (!lockOwner_) ?

Oops, indeed. I'll fix it in v2.

> > +               return;
> > +
> >         for (std::shared_ptr<MediaDevice> &media : mediaDevices_)
> >                 media->unlock();
> > +
> > +       lockOwner_ = false;
> >  }
> >  
> >  /**

-- 
Regards,

Laurent Pinchart