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

[Umang Jain]

Hi Laurent,

Thank you for the patch

On 12/27/21 4:42 AM, 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.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


Reviewed-by: Umang Jain <umang.jain 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;
>   }
>   
>   /**