[libcamera-devel] [PATCH v4 06/11] test: fence: Add test for the Fence class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 8 11:38:02 CET 2021
Hi Jacopo,
Thank you for the patch.
On Wed, Dec 01, 2021 at 03:29:31PM +0100, Jacopo Mondi wrote:
> Add a test for the Fence class by testing a Fence failure case, and
> by testing a successfully signalled fence capture cycle.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> test/fence.cpp | 340 +++++++++++++++++++++++++++++++++++++++++++++++
> test/meson.build | 1 +
> 2 files changed, 341 insertions(+)
> create mode 100644 test/fence.cpp
>
> diff --git a/test/fence.cpp b/test/fence.cpp
> new file mode 100644
> index 000000000000..ee1c8343e01c
> --- /dev/null
> +++ b/test/fence.cpp
> @@ -0,0 +1,340 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * fence.cpp - Fence test
> + */
> +
> +#include <iostream>
> +#include <memory>
> +#include <sys/eventfd.h>
> +#include <unistd.h>
> +
> +#include <libcamera/base/event_dispatcher.h>
> +#include <libcamera/base/thread.h>
> +#include <libcamera/base/timer.h>
> +#include <libcamera/base/unique_fd.h>
> +#include <libcamera/base/utils.h>
> +
> +#include <libcamera/fence.h>
> +#include <libcamera/framebuffer_allocator.h>
> +
> +#include "camera_test.h"
> +#include "test.h"
> +
> +using namespace std::chrono_literals;
> +using namespace libcamera;
> +using namespace std;
> +
> +class FenceTest : public CameraTest, public Test
> +{
> +public:
> + FenceTest();
> +
> +protected:
> + int init() override;
> + int run() override;
> + void cleanup() override;
> +
> +private:
> + int validateExpiredRequest(Request *request);
> + void requestComplete(Request *request);
> + void signalFence();
> +
> + std::unique_ptr<Fence> fence_;
> + EventDispatcher *dispatcher_;
> + UniqueFD eventFd_;
> + Timer fenceTimer_;
> +
> + std::vector<std::unique_ptr<Request>> requests_;
> + std::unique_ptr<CameraConfiguration> config_;
> + FrameBufferAllocator *allocator_;
With a unique_ptr you could avoid the cleanup() function (and also avoid
a leak in a failure path in init()).
> +
> + Stream *stream_;
> +
> + bool expectedCompletionResult_ = true;
> + bool expectFenceFailure_ = true;
> +
> + unsigned int completedRequest_;
> +
> + unsigned int signalledRequestId_;
> + unsigned int expiredRequestId_;
> + unsigned int nbuffers_;
> +
> + int efd2_;
> + int efd_;
> +};
> +
> +FenceTest::FenceTest()
> + : CameraTest("platform/vimc.0 Sensor B")
> +{
> +}
> +
> +int FenceTest::init()
> +{
> + dispatcher_ = Thread::current()->eventDispatcher();
> +
I'd add a comment here to record a need for future development:
/*
* Create an eventfd() to model the fence. This is enough to support the
* needs of libcamera which only needs to wait for read events through
* poll(). Once native support for fences will be available in the
* backend kernel APIs this will need to be replaced by a sw_sync fence,
* but that requires debugfs.
*/
> + eventFd_ = UniqueFD(eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK));
> + if (!eventFd_.isValid()) {
> + cerr << "Unable to create eventfd" << endl;
> + return TestFail;
> + }
> +
> + /*
> + * Keep a copy of the original fd value to validate that an expired
> + * fence has the same fd value.
> + */
> + efd_ = eventFd_.get();
> +
> + config_ = camera_->generateConfiguration({ StreamRole::Viewfinder });
> + if (!config_ || config_->size() != 1) {
> + cerr << "Failed to generate default configuration" << endl;
> + return TestFail;
> + }
> +
> + if (camera_->acquire()) {
> + cerr << "Failed to acquire the camera" << endl;
> + return TestFail;
> + }
> +
> + if (camera_->configure(config_.get())) {
> + cerr << "Failed to set default configuration" << endl;
> + return TestFail;
> + }
> +
> + StreamConfiguration &cfg = config_->at(0);
> + stream_ = cfg.stream();
> +
> + allocator_ = new FrameBufferAllocator(camera_);
> + if (allocator_->allocate(stream_) < 0)
> + return TestFail;
> +
> + nbuffers_ = allocator_->buffers(stream_).size();
> + if (nbuffers_ < 2) {
> + cerr << "Not enough buffers available" << endl;
> + return TestFail;
> + }
> +
> + signalledRequestId_ = nbuffers_ - 2;
> + expiredRequestId_ = nbuffers_ - 1;
> +
> + return TestPass;
> +}
> +
> +int FenceTest::validateExpiredRequest(Request *request)
> +{
> + FrameBuffer *buffer = request->buffers().begin()->second;
> +
> + std::unique_ptr<Fence> fence = buffer->releaseFence();
> + if (!fence) {
> + cerr << "The expired fence should be present" << endl;
> + return TestFail;
> + }
> +
> + if (!fence->isValid()) {
> + cerr << "The expired fence should be valid" << endl;
> + return TestFail;
> + }
> +
> + UniqueFD fd = fence->release();
> + if (fd.get() != efd_) {
> + cerr << "The expired fence file descriptor should not change" << endl;
> + return TestFail;
> + }
> +
> + expectFenceFailure_ = false;
> +
> + return 0;
> +}
> +
> +/* Callback to signal a fence waiting on the eventfd file descriptor. */
> +void FenceTest::signalFence()
> +{
> + uint64_t value = 1;
> + write(efd2_, &value, sizeof(value));
> + dispatcher_->processEvents();
> +}
> +
> +void FenceTest::requestComplete(Request *request)
> +{
> + uint64_t cookie = request->cookie();
> + completedRequest_ = cookie;
> +
> + /* We expect the last request to fail, if it doesn't happen, test fails. */
> + if (expectFenceFailure_ && cookie == expiredRequestId_ &&
> + request->status() != Request::RequestCancelled) {
> + cerr << "The last request should have failed: " << cookie << endl;
> +
> + expectedCompletionResult_ = false;
> + dispatcher_->interrupt();
> + return;
> + }
> +
> + /* All other requests but the last one should be completed. */
> + if (expectFenceFailure_ && cookie < expiredRequestId_ &&
> + request->status() != Request::RequestComplete) {
> + cerr << "All requests but last should complete: " << cookie << endl;
> +
> + expectedCompletionResult_ = false;
> + dispatcher_->interrupt();
> + return;
> + }
> +
> + /*
> + * If the last request has failed already, all the ones queued after
> + * that shall complete.
> + */
> + if (!expectFenceFailure_ &&
> + request->status() != Request::RequestComplete) {
> + cerr << "Unexpected request failure: " << cookie << endl;
> +
> + expectedCompletionResult_ = false;
> + dispatcher_->interrupt();
> + return;
> + }
> +
> + /*
> + * The last request has failed.
> + * Validate its fence status and do not re-queue it.
> + */
> + if (cookie == expiredRequestId_) {
> + int ret = validateExpiredRequest(request);
> + if (ret)
> + expectedCompletionResult_ = false;
> +
> + dispatcher_->interrupt();
> + return;
> + }
> +
> + const Request::BufferMap &buffers = request->buffers();
> + const Stream *stream = buffers.begin()->first;
> + FrameBuffer *buffer = buffers.begin()->second;
> +
> + /* A succesfully completed request should have the Fence closed. */
> + std::unique_ptr<Fence> bufferFence = buffer->releaseFence();
> + if (bufferFence) {
> + cerr << "Unexpected valid fence in completed request" << endl;
> +
> + expectedCompletionResult_ = false;
> + dispatcher_->interrupt();
> + return;
> + }
> +
> + request->reuse();
> +
> + if (cookie == signalledRequestId_) {
> + /*
> + * The second time this request is about to be queued add
> + * a fence to it.
> + *
> + * The main loop should signal it by using a timer to write to
> + * the eventfd file descriptor before the fence expires.
> + */
> + UniqueFD eventFd2(eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK));
> + if (!eventFd2.isValid()) {
> + cerr << "Unable to create eventfd" << endl;
> + expectedCompletionResult_ = false;
Shouldn't the dispatcher be interrupted here too ? I'm tempted to wrap
all the validation code (up to request->reuse()) in a separate
validateRequest() function, with requestComplete() doing
uint64_t cookie = request->cookie();
if (validateRequest(request) != TestPass) {
expectedCompletionResult_ = false;
completedRequest_ = cookie;
dispatcher_->interrupt();
return;
}
request->reuse();
...
camera_->queueRequest(request);
completedRequest_ = cookie;
dispatcher_->interrupt();
This would also avoid setting completedRequest_ too early, which could
possibly race with the code in the main loop (not entirely sure about
that though).
> + return;
> + }
> + efd2_ = eventFd2.get();
> +
> + std::unique_ptr<Fence> fence =
> + std::make_unique<Fence>(std::move(eventFd2));
It looks like this whole code block could be moved to a separate
function and reused at init() time (or even at run() time when creating
the requests).
> + request->addBuffer(stream, buffer, std::move(fence));
> + } else {
> + /* All the other requests continue to operate without fences. */
> + request->addBuffer(stream, buffer);
> + }
> +
> + camera_->queueRequest(request);
> +
> + /*
> + * Interrupt the dispatcher to return control to the main loop and
> + * activate the fenceTimer
> + . */
That looks weird.
> + dispatcher_->interrupt();
> +}
> +
> +int FenceTest::run()
> +{
> + for (const auto &[i, buffer] : utils::enumerate(allocator_->buffers(stream_))) {
> + std::unique_ptr<Request> request = camera_->createRequest(i);
> + if (!request) {
> + cerr << "Failed to create request" << endl;
> + return TestFail;
> + }
> +
> + int ret;
> + if (i == expiredRequestId_) {
> + /* This request will have a fence, and it will expire. */
> + fence_ = std::make_unique<Fence>(std::move(eventFd_));
> + if (!fence_->isValid()) {
> + cerr << "Fence should be valid" << endl;
> + return TestFail;
> + }
> +
> + ret = request->addBuffer(stream_, buffer.get(), std::move(fence_));
> + } else {
> + /* All other requests will have no Fence. */
> + ret = request->addBuffer(stream_, buffer.get());
> + }
> +
> + if (ret) {
> + cerr << "Failed to associate buffer with request" << endl;
> + return TestFail;
> + }
> +
> + requests_.push_back(std::move(request));
> + }
> +
> + camera_->requestCompleted.connect(this, &FenceTest::requestComplete);
> +
> + if (camera_->start()) {
> + cerr << "Failed to start camera" << endl;
> + return TestFail;
> + }
> +
> + for (std::unique_ptr<Request> &request : requests_) {
> + if (camera_->queueRequest(request.get())) {
> + cerr << "Failed to queue request" << endl;
> + return TestFail;
> + }
> + }
> +
> + expectedCompletionResult_ = true;
> +
> + /* This timer serves to signal fences associated with "signalledRequestId_" */
> + Timer fenceTimer;
> + fenceTimer.timeout.connect(this, &FenceTest::signalFence);
> +
> + /* Loop for one second. */
> + Timer timer;
> + timer.start(1000);
> + while (timer.isRunning() && expectedCompletionResult_) {
> + if (completedRequest_ == signalledRequestId_)
> + /*
> + * signalledRequestId_ has just completed and it has
> + * been re-queued with a fence. Start the timer to
> + * signal the fence in 10 msec.
> + */
> + fenceTimer.start(10);
> +
> + dispatcher_->processEvents();
> + }
> +
> + camera_->requestCompleted.disconnect();
Is this needed ?
> +
> + if (camera_->stop()) {
> + cerr << "Failed to stop camera" << endl;
> + return TestFail;
> + }
> +
> + return expectedCompletionResult_ ? TestPass : TestFail;
> +}
> +
> +void FenceTest::cleanup()
> +{
> + delete allocator_;
> +}
> +
> +TEST_REGISTER(FenceTest)
> diff --git a/test/meson.build b/test/meson.build
> index daaa3862cdd6..2c9487e203e2 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -39,6 +39,7 @@ internal_tests = [
> ['event', 'event.cpp'],
> ['event-dispatcher', 'event-dispatcher.cpp'],
> ['event-thread', 'event-thread.cpp'],
> + ['fence', 'fence.cpp'],
> ['file', 'file.cpp'],
> ['flags', 'flags.cpp'],
> ['hotplug-cameras', 'hotplug-cameras.cpp'],
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list