[libcamera-devel] [PATCH 11/15] v4l2: v4l2_camera: Don't use libcamera::Semaphore for available buffers

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, Jun 16, 2020 at 10:12:40PM +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>
> ---
>  src/v4l2/v4l2_camera.cpp       | 32 ++++++++++++++++++++++++++++++--
>  src/v4l2/v4l2_camera.h         |  8 +++++++-
>  src/v4l2/v4l2_camera_proxy.cpp | 11 +++++++++--
>  3 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index fdbf461..f0ec54b 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();
>  }
>  
>  int V4L2Camera::configure(StreamConfiguration *streamConfigOut,
> @@ -151,6 +153,7 @@ int V4L2Camera::allocBuffers(unsigned int count)
>  void V4L2Camera::freeBuffers()
>  {
>  	Stream *stream = *camera_->streams().begin();
> +
>  	bufferAllocator_->free(stream);
>  }
>  
> @@ -168,6 +171,7 @@ FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
>  
>  int V4L2Camera::streamOn()
>  {
> +	MutexLocker locker(isRunningMutex_);
>  	if (isRunning_)
>  		return 0;
>  
> @@ -191,6 +195,7 @@ int V4L2Camera::streamOn()
>  
>  int V4L2Camera::streamOff()
>  {
> +	MutexLocker locker(isRunningMutex_);
>  	/* \todo Restore buffers to reqbufs state? */
>  	if (!isRunning_)
>  		return 0;
> @@ -200,12 +205,15 @@ int V4L2Camera::streamOff()
>  		return ret == -EACCES ? -EBUSY : ret;
>  
>  	isRunning_ = false;
> +	bufferCV_.notify_all();
>  
>  	return 0;
>  }
>  
>  int V4L2Camera::qbuf(unsigned int index)
>  {
> +	MutexLocker locker(isRunningMutex_);
> +
>  	std::unique_ptr<Request> request =
>  		std::unique_ptr<Request>(camera_->createRequest(index));
>  	if (!request) {
> @@ -234,3 +242,23 @@ 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;
> +}
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index 30114ed..81dcbe2 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();
>  
>  private:
>  	void requestComplete(Request *request);
> @@ -65,6 +66,7 @@ private:
>  	std::shared_ptr<Camera> camera_;
>  	std::unique_ptr<CameraConfiguration> config_;
>  
> +	Mutex isRunningMutex_;

Why do you use a separate mutex for this, especially given that
V4L2Camera::waitForBufferAvailable() accesses isRunning_ using
bufferMutex_, not isRunningMutex_ ?

>  	bool isRunning_;
>  
>  	std::mutex bufferLock_;
> @@ -74,6 +76,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 45e4656..079961a 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -648,10 +648,17 @@ int V4L2CameraProxy::vidioc_dqbuf(int fd, struct v4l2_buffer *arg)
>  		return -EINVAL;
>  
>  	if (!nonBlockingFds_[fd])
> -		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.
> +	 */
> +	if (!streaming_)
> +		return -EINVAL;
> +
>  	updateBuffers();
>  
>  	struct v4l2_buffer &buf = buffers_[currentBuf_];

-- 
Regards,

Laurent Pinchart