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

[Laurent Pinchart]

Hi again,

On Wed, Jun 24, 2020 at 02:05:25AM +0300, Laurent Pinchart wrote:
> 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.

I take this back, see explanation in my review of 22/22.

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

-- 
Regards,

Laurent Pinchart