[libcamera-devel] [PATCH 08/11] libcamera: media_device: Add functions to lock device for other processes

[Kieran Bingham]

Hi Niklas,

On 29/04/2019 20:17, Niklas Söderlund wrote:
> Add lock() and unlock() which backed by lockf() allows an instance of

"which are backed by lockf() and allows an"

> libcamera to claim exclusive access to a media device. These functions
> is the base to allow multiple users of libcamera to coexist in the

"are the base of allowing multiple users..."

> system without stepping on each others toes.
> 

Nice - lockf makes this look pretty easy!

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>




> ---
>  src/libcamera/include/media_device.h |  4 +++
>  src/libcamera/media_device.cpp       | 54 +++++++++++++++++++++++++++-
>  2 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index e513a2fee1b91a1b..7b88e2875d59dfc3 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -30,6 +30,9 @@ public:
>  	void release();
>  	bool busy() const { return acquired_; }
>  
> +	bool lock();
> +	void unlock();
> +
>  	int populate();
>  	bool valid() const { return valid_; }
>  
> @@ -58,6 +61,7 @@ private:
>  	int fd_;
>  	bool valid_;
>  	bool acquired_;
> +	bool lockOwner_;
>  
>  	int open();
>  	void close();
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index cd6a01ed452073e2..9116330570d7b8c2 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -63,7 +63,8 @@ LOG_DEFINE_CATEGORY(MediaDevice)
>   * populate() before the media graph can be queried.
>   */
>  MediaDevice::MediaDevice(const std::string &deviceNode)
> -	: deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false)
> +	: deviceNode_(deviceNode), fd_(-1), valid_(false), acquired_(false),
> +	  lockOwner_(false)
>  {
>  }
>  
> @@ -118,6 +119,57 @@ void MediaDevice::release()
>  	acquired_ = false;
>  }
>  
> +/**
> + * \brief Lock the device for use by other instances of libcamera
> + *
> + * Multiple instances of libcamera might be running on the same system, at the
> + * same time. To allow the different instances to coexists system resources in

s/coexists/coexist,/


> + * the form of media devices must be accessible for enumerating the cameras
> + * they provide at all times, while still allowing an instance to lock a
> + * resource while it prepares to actively use a camera from the resource.
> + *
> + * This method shall not be called from a pipeline handler implementation
> + * directly, as the base PipelineHandler implementation handles this on the
> + * behalf of the specified implementation.
> + *
> + * \return True if the device could be locked, false otherwise
> + * \sa unlock()
> + */
> +bool MediaDevice::lock()
> +{
> +	if (fd_ == -1)
> +		return false;
> +
> +	if (lockOwner_)
> +		return true;

Does this encourage nested locking?

(I don't yet know if nested locking is a good or bad thing for us)

If it's something we want to discourage, could we print a debug message
or warning stating that the file is locked twice perhaps?


If it's something we desire - then I think the nested locks would
require some reference counting and only release on the last unlock call.


> +
> +	if (lockf(fd_, F_TLOCK, 0))
> +		return false;
> +
> +	lockOwner_ = true;
> +
> +	return true;
> +}
> +
> +/**
> + * \brief Unlock the device and free it for use for libcamera instances
> + *
> + * This method shall not be called from a pipeline handler implementation
> + * directly, as the base PipelineHandler implementation handles this on the
> + * behalf of the specified implementation.
> + *
> + * \sa lock()
> + */
> +void MediaDevice::unlock()
> +{
> +	if (fd_ == -1)
> +		return;
> +
> +	lockOwner_ = false;
> +

MediaDevice::lock() "supports" being called on a locked file;

What happens if you try to unlock an unlocked file? (man lockf does not
really detail what happens there). Presumably it will just return an
error and take no action?


> +	lockf(fd_, F_ULOCK, 0);
> +}
> +
>  /**
>   * \fn MediaDevice::busy()
>   * \brief Check if a device is in use
> 

-- 
Regards
--
Kieran