[libcamera-devel] [PATCH v3 22/22] v4l2: v4l2_camera_proxy: Serialize accesses to the proxy

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jun 24, 2020 at 04:08:36AM +0900, Paul Elder wrote:
> Make the V4L2 compatibility layer thread-safe by serializing accesses to
> the V4L2CameraProxy with a lock. Release the lock when blocking for
> dqbuf.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> New in v3
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 21 +++++++++++++++++----
>  src/v4l2/v4l2_camera_proxy.h   |  5 ++++-
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index ed3bcbc..fb3a1fc 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -43,6 +43,8 @@ V4L2CameraProxy::V4L2CameraProxy(unsigned int index,
>  
>  int V4L2CameraProxy::open(V4L2CameraFile *file)
>  {
> +	MutexLocker locker(proxyMutex_);
> +

You could move the lock after the LOG() lines to slightly reduced the
amount of code protected by it.

>  	LOG(V4L2Compat, Debug) << "Servicing open fd = " << file->efd();
>  
>  	files_.insert(file);
> @@ -75,6 +77,8 @@ int V4L2CameraProxy::open(V4L2CameraFile *file)
>  
>  void V4L2CameraProxy::close(V4L2CameraFile *file)
>  {
> +	MutexLocker locker(proxyMutex_);
> +
>  	LOG(V4L2Compat, Debug) << "Servicing close fd = " << file->efd();
>  
>  	files_.erase(file);
> @@ -90,6 +94,8 @@ void V4L2CameraProxy::close(V4L2CameraFile *file)
>  void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
>  			    off64_t offset)
>  {
> +	MutexLocker locker(proxyMutex_);
> +
>  	LOG(V4L2Compat, Debug) << "Servicing mmap";
>  
>  	/* \todo Validate prot and flags properly. */
> @@ -124,6 +130,8 @@ void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
>  
>  int V4L2CameraProxy::munmap(void *addr, size_t length)
>  {
> +	MutexLocker locker(proxyMutex_);
> +
>  	LOG(V4L2Compat, Debug) << "Servicing munmap";
>  
>  	auto iter = mmaps_.find(addr);
> @@ -592,7 +600,8 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
>  	return ret;
>  }
>  
> -int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
> +int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file,
> +				  struct v4l2_buffer *arg, MutexLocker &locker)

We usually passed parameters that can be modified by pointer, not by
reference.

And you could keep the first two parameters on the first line.

>  {
>  	LOG(V4L2Compat, Debug) << "Servicing vidioc_dqbuf fd = " << file->efd();
>  
> @@ -609,9 +618,11 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
>  	    !validateMemoryType(arg->memory))
>  		return -EINVAL;
>  
> -	if (!(file->nonBlocking()))
> +	if (!(file->nonBlocking())) {
> +		locker.unlock();
>  		vcam_->waitForBufferAvailable();
> -	else if (!vcam_->isBufferAvailable())
> +		locker.lock();

There's a potential race condition here. waitForBufferAvailable() is
implemented as

void V4L2Camera::waitForBufferAvailable()
{
	MutexLocker locker(bufferMutex_);
        bufferCV_.wait(locker, [&] {
			return bufferAvailableCount_ >= 1 || !isRunning_;
			});
	if (isRunning_)
		bufferAvailableCount_--;
}

and in V4L2Camera::streamOff(), you have

	isRunning_ = false;
	bufferCV_.notify_all();

without taking the bufferMutex_ lock. streamOff() can run in a parallel
thread to waitForBufferAvailable() as you release the locker before
calling waitForBufferAvailable(). Unless I'm mistaken, the notify_all()
call doesn't count as a release operation (in the C++ memory model
sense, see https://people.cs.pitt.edu/~xianeizhang/notes/cpp11_mem.html
for a local resource :-)). The write to isRunning_ could then be visible
to the thread waiting on bufferCV_ only after bufferCV_.notify_all()
gets visible. The wait() would wake up, not see !isRunning_, go back to
sleep, and stay there forever. I initially thought the global lock would
solve this, but after further analysis, I don't think that's the case.

Fortunately, I think the fix should be a simple as

	{
		MutexLocker locker(buferMutex_);
		isRunning_ = false;
	}
	bufferCV_notify_all();

in V4L2Camera::streamOff() in the patch that dropped usage of the
Semaphore class.

I'll let you confirm my analysis :-)

For this patch,

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

> +	} else if (!vcam_->isBufferAvailable())
>  		return -EAGAIN;
>  
>  	/*
> @@ -706,6 +717,8 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
>  
>  int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)
>  {
> +	MutexLocker locker(proxyMutex_);
> +
>  	if (!arg && (_IOC_DIR(request) & _IOC_WRITE)) {
>  		errno = EFAULT;
>  		return -1;
> @@ -766,7 +779,7 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
>  		ret = vidioc_qbuf(file, static_cast<struct v4l2_buffer *>(arg));
>  		break;
>  	case VIDIOC_DQBUF:
> -		ret = vidioc_dqbuf(file, static_cast<struct v4l2_buffer *>(arg));
> +		ret = vidioc_dqbuf(file, static_cast<struct v4l2_buffer *>(arg), locker);
>  		break;
>  	case VIDIOC_STREAMON:
>  		ret = vidioc_streamon(file, static_cast<int *>(arg));
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index 041f536..b6d2c2c 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -61,7 +61,7 @@ private:
>  	int vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuffers *arg);
>  	int vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
>  	int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> -	int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> +	int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg, MutexLocker &locker);
>  	int vidioc_streamon(V4L2CameraFile *file, int *arg);
>  	int vidioc_streamoff(V4L2CameraFile *file, int *arg);
>  
> @@ -105,6 +105,9 @@ private:
>  	 * g/s_priority, enum/g/s_input
>  	 */
>  	V4L2CameraFile *owner_;
> +
> +	/* This mutex is to serialize access to the proxy. */
> +	Mutex proxyMutex_;
>  };
>  
>  #endif /* __V4L2_CAMERA_PROXY_H__ */

-- 
Regards,

Laurent Pinchart