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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Dec 27, 2021 at 10:03:43AM +0100, Jacopo Mondi wrote:
> On Mon, Dec 27, 2021 at 01:12:54AM +0200, Laurent Pinchart wrote:
> > 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.
> 
> Out of curiosity: what do we talk about recursive locking ?
> Where is the recursive part ? I though this is just about preventing
> to lock the same media device twice.

It's recursive in the sense that the lock is taken again (with a call to
PipelineHandler::lock()) for a second camera while already held for a
first camera. That corresponds to the definition of
std::recursive_mutex, but it's not recursive in the sense of calling
lock() from within a lock() call.

> How do you envision per-camera locking going forward ? Currently
> Camera::acquire() locks the pipeline handlers, if the locking should
> be moved to be per-camera, shouldn't the whole pipeline handler
> locking mechanism be reworked ?

Yes, it will need to be reworked. This patch only prepares for that by
giving true lock semantics to MediaDevice::lock(). The rest will need to
be modified, with an API for pipeline handlers to expose mutual
exclusion constraints between different cameras to applications. I'm not
sure yet how that will look like.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart 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