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

[Jacopo Mondi]

Hi Laurent,

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.

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 ?

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