[libcamera-devel] [PATCH v3 16/22] v4l2: v4l2_camera: Don't use libcamera::Semaphore for available buffers

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jun 24, 2020 at 04:08:30AM +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 v3:
> - don't notify all with the lock held
> 
> Changes in v2:
> - remove mutex for isRunning_
> - use V4L2Camera::isRunning() instead of V4L2CameraProxy::streaming_ in
>   the dqbuf ioctl handler
> ---
>  src/v4l2/v4l2_camera.cpp       | 30 ++++++++++++++++++++++++++++--
>  src/v4l2/v4l2_camera.h         |  9 +++++++--
>  src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++--
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 177b1ea..bdf0036 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,11 @@ 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();
>  }
>  
>  int V4L2Camera::configure(StreamConfiguration *streamConfigOut,
> @@ -144,6 +148,7 @@ int V4L2Camera::allocBuffers(unsigned int count)
>  void V4L2Camera::freeBuffers()
>  {
>  	Stream *stream = *camera_->streams().begin();
> +

This seems unrelated.

>  	bufferAllocator_->free(stream);
>  }
>  
> @@ -193,6 +198,7 @@ int V4L2Camera::streamOff()
>  		return ret == -EACCES ? -EBUSY : ret;
>  
>  	isRunning_ = false;
> +	bufferCV_.notify_all();
>  
>  	return 0;
>  }
> @@ -228,6 +234,26 @@ int V4L2Camera::qbuf(unsigned int index)
>  	return 0;
>  }
>  
> +void V4L2Camera::waitForBufferAvailable()
> +{
> +	MutexLocker locker(bufferMutex_);
> +	bufferCV_.wait(locker, [&] {
> +			return bufferAvailableCount_ >= 1 || !isRunning_;
> +			});

The correct indentation would be

	bufferCV_.wait(locker, [&] {
			       return bufferAvailableCount_ >= 1 || !isRunning_;
		       });

(indenting lambdas is not very nice :-S)

> +	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 ee53c2b..515e906 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -57,9 +57,10 @@ public:
>  
>  	int qbuf(unsigned int index);
>  
> -	bool isRunning();
> +	void waitForBufferAvailable();
> +	bool isBufferAvailable();
>  
> -	Semaphore bufferSema_;
> +	bool isRunning();
>  
>  private:
>  	void requestComplete(Request *request);
> @@ -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 fa58585..8420e23 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -594,10 +594,17 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
>  		return -EINVAL;
>  
>  	if (!(file->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 waitForBufferAvailable.

s/waitForBufferAvailable/waitForBufferAvailable()/

> +	 */
> +	if (!vcam_->isRunning())
> +		return -EINVAL;

I think there's a small race here in the non-blocking case, as
isBufferAvailable() could decrement the buffer count and the !running
case could ignore that. I'm not sure it would have consequences as we
would be stopping streaming anyway in that case, but regardless, patch
22/22 will address that, so

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

as I'm not concerned about bisection breakages before this series lands.

> +
>  	updateBuffers();
>  
>  	struct v4l2_buffer &buf = buffers_[currentBuf_];

-- 
Regards,

Laurent Pinchart