[libcamera-devel] [PATCH v2 12/17] v4l2: v4l2_camera: Don't use libcamera::Semaphore for available buffers

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Jun 19, 2020 at 02:41:18PM +0900, Paul Elder wrote:
> In V4L2, a blocked dqbuf should not not also block a streamoff. This
> means that on streamoff, the blocked dqbuf must return (with error). We
> cannot do this with the libcamera semaphore, so pull out the necessary
> components of a semaphore, and put them into V4L2Camera, so that dqbuf
> from V4L2CameraProxy can wait on a disjunct condition of the
> availability of the semaphore or the stopping of the stream.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> Changes in v2:
> - remove mutex for isRunning_
> - use V4L2Camera::isRunning() instead of V4L2CameraProxy::streaming_ in
>   the dqbuf ioctl handler
> ---
>  src/v4l2/v4l2_camera.cpp       | 28 ++++++++++++++++++++++++++--
>  src/v4l2/v4l2_camera.h         |  7 ++++++-
>  src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++--
>  3 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 177b1ea..99d34b9 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -18,7 +18,7 @@ LOG_DECLARE_CATEGORY(V4L2Compat);
>  
>  V4L2Camera::V4L2Camera(std::shared_ptr<Camera> camera)
>  	: camera_(camera), isRunning_(false), bufferAllocator_(nullptr),
> -	  efd_(-1)
> +	  efd_(-1), bufferAvailableCount_(0)
>  {
>  	camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);
>  }
> @@ -100,7 +100,9 @@ void V4L2Camera::requestComplete(Request *request)
>  	if (ret != sizeof(data))
>  		LOG(V4L2Compat, Error) << "Failed to signal eventfd POLLIN";
>  
> -	bufferSema_.release();
> +	MutexLocker locker(bufferMutex_);
> +	bufferAvailableCount_++;
> +	bufferCV_.notify_all();

Calling notify_all() with the lock held will hinder performances. See
https://en.cppreference.com/w/cpp/thread/condition_variable/notify_all.

	{
		MutexLocker locker(bufferMutex_);
		bufferAvailableCount_++;
	}
	bufferCV_.notify_all();

>  }
>  
>  int V4L2Camera::configure(StreamConfiguration *streamConfigOut,
> @@ -144,6 +146,7 @@ int V4L2Camera::allocBuffers(unsigned int count)
>  void V4L2Camera::freeBuffers()
>  {
>  	Stream *stream = *camera_->streams().begin();
> +
>  	bufferAllocator_->free(stream);
>  }
>  
> @@ -193,6 +196,7 @@ int V4L2Camera::streamOff()
>  		return ret == -EACCES ? -EBUSY : ret;
>  
>  	isRunning_ = false;
> +	bufferCV_.notify_all();


Isn't this racy, can't the compiler (or the CPU) reorder these two lines
? I think you need to lock access to isRunning_ with bufferMutex_. On
the other hand, if we want to support multi-threaded applications, we
will have to add locking to the V4L2 compat layer. This will likely be
done with a lock in V4L2CameraProxy, taken in V4L2CameraProxy::ioctl().
isRunning_ would thus be protected by that lock, so we could skip taking
the bufferMutex_ here. I think I'd skip it for now, otherwise we would
need to use bufferMutex_ to lock other accesses to isRunning_, and it
would become messy.

Now that I've written this, it seems being thread-safe would be quite
simple with the V4L2CameraProxy lock. You would just need to be careful
to unlock it before calling V4L2Camera::waitForBufferAvailable(), and
relock it right after. Could you give it a try on top of this series ?

>  
>  	return 0;
>  }
> @@ -228,6 +232,26 @@ int V4L2Camera::qbuf(unsigned int index)
>  	return 0;
>  }
>  
> +void V4L2Camera::waitForBufferAvailable()
> +{
> +	MutexLocker locker(bufferMutex_);
> +	bufferCV_.wait(locker, [&] {
> +			return bufferAvailableCount_ >= 1 || !isRunning_;
> +			});
> +	if (isRunning_)
> +		bufferAvailableCount_--;
> +}
> +
> +bool V4L2Camera::isBufferAvailable()
> +{
> +	MutexLocker locker(bufferMutex_);
> +	if (bufferAvailableCount_ < 1)
> +		return false;
> +
> +	bufferAvailableCount_--;
> +	return true;
> +}
> +
>  bool V4L2Camera::isRunning()
>  {
>  	return isRunning_;
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index c157a80..515e906 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -57,7 +57,8 @@ public:
>  
>  	int qbuf(unsigned int index);
>  
> -	Semaphore bufferSema_;
> +	void waitForBufferAvailable();
> +	bool isBufferAvailable();
>  
>  	bool isRunning();
>  
> @@ -76,6 +77,10 @@ private:
>  	std::deque<std::unique_ptr<Buffer>> completedBuffers_;
>  
>  	int efd_;
> +
> +	Mutex bufferMutex_;
> +	std::condition_variable bufferCV_;
> +	unsigned int bufferAvailableCount_;
>  };
>  
>  #endif /* __V4L2_CAMERA_H__ */
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index ea9222e..2723450 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -595,10 +595,17 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg)
>  		return -EINVAL;
>  
>  	if (!(cf->nonBlocking()))
> -		vcam_->bufferSema_.acquire();
> -	else if (!vcam_->bufferSema_.tryAcquire())
> +		vcam_->waitForBufferAvailable();
> +	else if (!vcam_->isBufferAvailable())
>  		return -EAGAIN;
>  
> +	/*
> +	 * We need to check here again in case stream was turned off while we
> +	 * were blocked on dqbuf.

s/dqbuf/waitForBufferAvailable()/ ?

I'm not sure this is checking "again". Well, technically it is, but it's
really about checking which of the two conditions is true.

> +	 */
> +	if (!vcam_->isRunning())
> +		return -EINVAL;
> +
>  	updateBuffers();
>  
>  	struct v4l2_buffer &buf = buffers_[currentBuf_];

-- 
Regards,

Laurent Pinchart