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

1193 diff mbox series

Add lock() and unlock() which are backed by lockf() and allows an
instance of libcamera to claim exclusive access to a media device.
These functions are the base of allowing multiple user of libcamera to
coexist in the system without stepping on each others toes.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/include/media_device.h |  4 ++
 src/libcamera/media_device.cpp       | 58 +++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

Hi Niklas,

Thank you for the patch.

On Sat, May 11, 2019 at 11:19:04AM +0200, Niklas Söderlund wrote:
> Add lock() and unlock() which are backed by lockf() and allows an

s/allows/allow/

> instance of libcamera to claim exclusive access to a media device.
> These functions are the base of allowing multiple user of libcamera to
> coexist in the system without stepping on each others toes.

s/others/other's/

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/include/media_device.h |  4 ++
>  src/libcamera/media_device.cpp       | 58 +++++++++++++++++++++++++++-
>  2 files changed, 61 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 62c59e7e8fc0299f..874ecc06e4bd9c94 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,61 @@ void MediaDevice::release()
>  	acquired_ = false;
>  }
>  
> +/**
> + * \brief Lock the device for use by other instances of libcamera

This sounds a bit weird to me. Maybe "Lock the device from being used by
other instances of libcamera" ? Feedback from a native speaker would be
appreciated here (and for the unlock() method below). Apart from this,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + *
> + * Multiple instances of libcamera might be running on the same system, at the
> + * same time. To allow the different instances to coexist, system resources in
> + * 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;
> +
> +	/* Do not allow nested locking in the same libcamera instance. */
> +	if (lockOwner_)
> +		return false;
> +
> +	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;
> +
> +	if (!lockOwner_)
> +		return;
> +
> +	lockOwner_ = false;
> +
> +	lockf(fd_, F_ULOCK, 0);
> +}
> +
>  /**
>   * \fn MediaDevice::busy()
>   * \brief Check if a device is in use