[libcamera-devel] [PATCH v3 6/6] v4l2: v4l2-compat: add buffer state tracking to V4L2CameraProxy
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Dec 28 01:52:34 CET 2019
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 at 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_;
> };
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list