[libcamera-devel,v3,6/6] v4l2: v4l2-compat: add buffer state tracking to V4L2CameraProxy

2449 diff mbox series

Add a way for V4L2CameraProxy to cache the state of all the completed
buffers as v4l2_buffers. This reduces the number of cross-thread calls,
since the newly added V4L2CameraProxy::updateBuffers(), which goes
through V4L2Camera::completedBuffers(), does not need to be called
across the thread boundary.

Also move the v4l2_buffer flag-setting logic to V4L2CameraProxy.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
New in v3
---
 src/v4l2/v4l2_camera.cpp       | 36 ++++++---------
 src/v4l2/v4l2_camera.h         |  7 ++-
 src/v4l2/v4l2_camera_proxy.cpp | 80 +++++++++++++++++++++++++++-------
 src/v4l2/v4l2_camera_proxy.h   |  3 ++
 4 files changed, 84 insertions(+), 42 deletions(-)

Hi Paul,

Thank you for the patch.

On Mon, Dec 23, 2019 at 01:26:20AM -0600, Paul Elder wrote:
> Add a way for V4L2CameraProxy to cache the state of all the completed
> buffers as v4l2_buffers. This reduces the number of cross-thread calls,
> since the newly added V4L2CameraProxy::updateBuffers(), which goes
> through V4L2Camera::completedBuffers(), does not need to be called
> across the thread boundary.
> 
> Also move the v4l2_buffer flag-setting logic to V4L2CameraProxy.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> New in v3

Any reason not to squash this with 5/6 ?

> ---
>  src/v4l2/v4l2_camera.cpp       | 36 ++++++---------
>  src/v4l2/v4l2_camera.h         |  7 ++-
>  src/v4l2/v4l2_camera_proxy.cpp | 80 +++++++++++++++++++++++++++-------
>  src/v4l2/v4l2_camera_proxy.h   |  3 ++
>  4 files changed, 84 insertions(+), 42 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 2d33be9f..403e24f6 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -70,6 +70,19 @@ void V4L2Camera::getStreamConfig(StreamConfiguration *streamConfig)
>  	*streamConfig = config_->at(0);
>  }
>  
> +std::vector<FrameMetadata> V4L2Camera::completedBuffers()
> +{
> +	std::vector<FrameMetadata> v;
> +
> +	bufferLock_.lock();
> +	for (std::unique_ptr<FrameMetadata> &fmd : completedBuffers_)
> +		v.push_back(*fmd.get());
> +	completedBuffers_.clear();
> +	bufferLock_.unlock();
> +
> +	return v;
> +}
> +
>  void V4L2Camera::requestComplete(Request *request)
>  {
>  	if (request->status() == Request::RequestCancelled)
> @@ -80,7 +93,7 @@ void V4L2Camera::requestComplete(Request *request)
>  	Buffer *buffer = request->buffers().begin()->second;
>  	std::unique_ptr<FrameMetadata> fmd =
>  		utils::make_unique<FrameMetadata>(buffer);
> -	completedBuffers_.push(std::move(fmd));
> +	completedBuffers_.push_back(std::move(fmd));
>  	bufferLock_.unlock();
>  
>  	bufferSema_.release();
> @@ -225,24 +238,3 @@ void V4L2Camera::qbuf(int *ret, unsigned int index)
>  
>  	*ret = 0;
>  }
> -
> -int V4L2Camera::dqbuf(struct v4l2_buffer *arg, bool nonblock)
> -{
> -	if (nonblock && !bufferSema_.tryAcquire())
> -		return -EAGAIN;
> -	else
> -		bufferSema_.acquire();
> -
> -	bufferLock_.lock();
> -	FrameMetadata *fmd = completedBuffers_.front().get();
> -	completedBuffers_.pop();
> -	bufferLock_.unlock();
> -
> -	arg->bytesused = fmd->bytesused();
> -	arg->field = V4L2_FIELD_NONE;
> -	arg->timestamp.tv_sec = fmd->timestamp() / 1000000000;
> -	arg->timestamp.tv_usec = fmd->timestamp() % 1000000;
> -	arg->sequence = fmd->sequence();
> -
> -	return 0;
> -}
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index 13418b6b..43ab8d02 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -11,7 +11,6 @@
>  #include <deque>
>  #include <linux/videodev2.h>
>  #include <mutex>
> -#include <queue>
>  
>  #include <libcamera/buffer.h>
>  #include <libcamera/camera.h>
> @@ -51,6 +50,7 @@ public:
>  	void open(int *ret);
>  	void close(int *ret);
>  	void getStreamConfig(StreamConfiguration *streamConfig);
> +	std::vector<FrameMetadata> completedBuffers();
>  
>  	void mmap(void **ret, unsigned int index);
>  
> @@ -63,8 +63,8 @@ public:
>  	void streamOff(int *ret);
>  
>  	void qbuf(int *ret, unsigned int index);
> -	int dqbuf(struct v4l2_buffer *arg, bool nonblock);
>  
> +	Semaphore bufferSema_;
>  private:
>  	void requestComplete(Request *request);
>  
> @@ -74,11 +74,10 @@ private:
>  	unsigned int bufferCount_;
>  	bool isRunning_;
>  
> -	Semaphore bufferSema_;
>  	std::mutex bufferLock_;
>  
>  	std::deque<std::unique_ptr<Request>> pendingRequests_;
> -	std::queue<std::unique_ptr<FrameMetadata>> completedBuffers_;
> +	std::deque<std::unique_ptr<FrameMetadata>> completedBuffers_;
>  };
>  
>  #endif /* __V4L2_CAMERA_H__ */
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index b0acd477..4e303500 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -101,6 +101,9 @@ void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
>  	void *val;
>  	vcam_->invokeMethod(&V4L2Camera::mmap, ConnectionTypeBlocking,
>  			    &val, index);
> +
> +	buffers_[index].flags |= V4L2_BUF_FLAG_MAPPED;

That's half of the job, you need to clear it on unmap() :-)

> +
>  	return val;
>  }
>  
> @@ -173,6 +176,35 @@ void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)
>  	memset(capabilities_.reserved, 0, sizeof(capabilities_.reserved));
>  }
>  
> +void V4L2CameraProxy::updateBuffers()
> +{
> +	std::vector<FrameMetadata> completedBuffers = vcam_->completedBuffers();
> +	for (FrameMetadata &fmd : completedBuffers) {
> +		/* \todo is this index valid if the buffer status != success? */

We'll have to rework this once Niklas' buffer rework lands. For now it
should be OK (but let's keep the \todo as a reminder).

> +		struct v4l2_buffer &buf = buffers_[fmd.index()];
> +
> +		switch (fmd.status()) {
> +		case Buffer::Status::BufferSuccess:
> +			buf.index = fmd.index();
> +			buf.bytesused = fmd.bytesused();
> +			buf.field = V4L2_FIELD_NONE;
> +			buf.timestamp.tv_sec = fmd.timestamp() / 1000000000;
> +			buf.timestamp.tv_usec = fmd.timestamp() % 1000000;
> +			buf.sequence = fmd.sequence();
> +
> +			buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +			buf.length = curV4L2Format_.fmt.pix.sizeimage;
> +			buf.memory = V4L2_MEMORY_MMAP;
> +			buf.m.offset = buf.index * curV4L2Format_.fmt.pix.sizeimage;
> +			break;
> +		case Buffer::Status::BufferError:
> +			buf.flags |= V4L2_BUF_FLAG_ERROR;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
>  int V4L2CameraProxy::vidioc_querycap(struct v4l2_capability *arg)
>  {
>  	LOG(V4L2Compat, Debug) << "Servicing vidioc_querycap";
> @@ -344,13 +376,21 @@ int V4L2CameraProxy::vidioc_querybuf(struct v4l2_buffer *arg)
>  	    arg->index >= stream->buffers().size())
>  		return -EINVAL;
>  
> -	unsigned int index = arg->index;
> -	memset(arg, 0, sizeof(*arg));
> -	arg->index = index;
> -	arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> -	arg->length = curV4L2Format_.fmt.pix.sizeimage;
> -	arg->memory = V4L2_MEMORY_MMAP;
> -	arg->m.offset = arg->index * curV4L2Format_.fmt.pix.sizeimage;
> +	/* \todo make updateBuffers() get only one buffer? */

No need to, keeping everything up-to-date unconditionally isn't a bad
idea.

> +	updateBuffers();
> +
> +	if (buffers_.size() <= arg->index) {
> +		struct v4l2_buffer buf;
> +		buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +		buf.length = curV4L2Format_.fmt.pix.sizeimage;
> +		buf.memory = V4L2_MEMORY_MMAP;
> +		buf.m.offset = arg->index * curV4L2Format_.fmt.pix.sizeimage;
> +
> +		buffers_.resize(arg->index + 1);
> +		buffers_[arg->index] = buf;
> +	}

Doesn't this belong to vidioc_reqbufs() ? I think you should only keep
updateBuffers() and the below memcpy() here.

> +
> +	memcpy(arg, &buffers_[arg->index], sizeof(*arg));
>  
>  	return 0;
>  }
> @@ -388,19 +428,23 @@ int V4L2CameraProxy::vidioc_dqbuf(struct v4l2_buffer *arg)
>  	    !validateMemoryType(arg->memory))
>  		return -EINVAL;
>  
> -	arg->index = currentBuf_;
> -	currentBuf_ = (currentBuf_ + 1) % bufferCount_;
> +	if (nonBlocking_ && !vcam_->bufferSema_.tryAcquire())
> +		return -EAGAIN;
> +	else
> +		vcam_->bufferSema_.acquire();
>  
> -	int ret = vcam_->dqbuf(arg, nonBlocking_);
> -	if (ret < 0)
> -		return ret;
> +	updateBuffers();
>  
> -	arg->flags &= ~V4L2_BUF_FLAG_QUEUED;
> -	arg->flags |= V4L2_BUF_FLAG_DONE;
> +	memcpy(arg, &buffers_[arg->index], sizeof(*arg));
>  
> -	arg->length = sizeimage_;
> +	struct v4l2_buffer &buf = buffers_[arg->index];

You can move this above the memcpy() and use buf in the memcpy().

> +	arg->index = currentBuf_;
> +	currentBuf_ = (currentBuf_ + 1) % bufferCount_;
> +	buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> +	buf.flags |= V4L2_BUF_FLAG_DONE;

The DONE flag should be moved to updateBuffers().

> +	buf.length = sizeimage_;

And length too I think. But do we really need to update it ?

The memcpy() should be moved here, otherwise you'll return stale values
for the QUEUED flag.

>  
> -	return ret;
> +	return 0;
>  }
>  
>  int V4L2CameraProxy::vidioc_streamon(int *arg)
> @@ -426,6 +470,10 @@ int V4L2CameraProxy::vidioc_streamoff(int *arg)
>  	int ret;
>  	vcam_->invokeMethod(&V4L2Camera::streamOff,
>  			    ConnectionTypeBlocking, &ret);
> +
> +	for (struct v4l2_buffer &buf : buffers_)
> +		buf.flags &= ~V4L2_BUF_FLAG_QUEUED;

Should you also clear the DONE flag ?

> +
>  	return ret;
>  }
>  
> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> index 51fdbe19..19688717 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -40,6 +40,7 @@ private:
>  	void setFmtFromConfig(StreamConfiguration &streamConfig);
>  	unsigned int calculateSizeImage(StreamConfiguration &streamConfig);
>  	void querycap(std::shared_ptr<Camera> camera);
> +	void updateBuffers();
>  
>  	int vidioc_querycap(struct v4l2_capability *arg);
>  	int vidioc_enum_fmt(struct v4l2_fmtdesc *arg);
> @@ -64,6 +65,8 @@ private:
>  	unsigned int currentBuf_;
>  	unsigned int sizeimage_;
>  
> +	std::vector<struct v4l2_buffer> buffers_;
> +
>  	std::unique_ptr<V4L2Camera> vcam_;
>  };
>