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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart