[libcamera-devel] [PATCH 04/10] test: Add test for the Fence class

[Kieran Bingham]

Quoting Jacopo Mondi (2021-10-28 12:15:14)
> Add a test for the Fence class by testing completion and expiration of
> a Fence and testing that move semantic is implemented correctly.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  test/fence.cpp   | 148 +++++++++++++++++++++++++++++++++++++++++++++++
>  test/meson.build |   1 +
>  2 files changed, 149 insertions(+)
>  create mode 100644 test/fence.cpp
> 
> diff --git a/test/fence.cpp b/test/fence.cpp
> new file mode 100644
> index 000000000000..9fb92b1c250b
> --- /dev/null
> +++ b/test/fence.cpp
> @@ -0,0 +1,148 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.

2021 perhaps.

> + *
> + * fence.cpp - Fence test
> + */
> +
> +#include <iostream>
> +#include <sys/eventfd.h>
> +#include <unistd.h>
> +
> +#include <libcamera/base/event_dispatcher_poll.h>
> +#include <libcamera/base/thread.h>
> +#include <libcamera/base/timer.h>
> +#include <libcamera/internal/fence.h>

Definitely something wrong with checkstyle missing these:

$ clang-format-diff-file test/fence.cpp
--- test/fence.cpp
+++ test/fence.cpp.clang
@@ -12,6 +12,7 @@
 #include <libcamera/base/event_dispatcher_poll.h>
 #include <libcamera/base/thread.h>
 #include <libcamera/base/timer.h>
+
 #include <libcamera/internal/fence.h>

 #include "test.h"

> +
> +#include "test.h"
> +
> +using namespace libcamera;
> +using namespace std;
> +
> +class FenceTest : public Test
> +{
> +protected:
> +       int init();
> +       int run();
> +       void cleanup();
> +
> +private:
> +       void timeout();
> +       void completed();
> +
> +       int eventfd_;
> +       Timer timer_;
> +       EventDispatcher *dispatcher;
> +       Fence *fence_;
> +};
> +
> +int FenceTest::init()
> +{
> +       timer_.timeout.connect(this, &FenceTest::timeout);
> +       dispatcher = Thread::current()->eventDispatcher();
> +
> +       eventfd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
> +       if (eventfd_ < 0) {
> +               cerr << "Unable to create eventfd" << endl;
> +               return TestFail;
> +       }
> +
> +       fence_ = new Fence(eventfd_);
> +       if (!eventfd_) {
> +               cerr << "Fence creation should not reset the file descriptor" << endl;
> +               return TestFail;
> +       }
> +
> +       if (fence_->fd() != eventfd_) {
> +               cerr << "Fence file descriptor should not be duplicated" << endl;

Why not? What happens if someone closes the underlying eventfd? Can we
have a copy so that ours stays with us until we're done with it? Or does
that break some semantics somewhere...?

> +               return TestFail;
> +       }
> +
> +       fence_->complete.connect(this, &FenceTest::completed);
> +
> +       return 0;
> +}
> +

aha, the timeout() below is a helper to 'activate' the fence. Can that
be highlighted with a banner comment for the function? It was really
easy to miss - as it sounded like a timeout event handler - not an event
'generator'.


> +void FenceTest::timeout()
> +{
> +       uint64_t value = 1;
> +       ssize_t ret = write(eventfd_, &value, sizeof(value));
> +       if (ret != sizeof(value))
> +               cerr << "Failed to write to event fd" << endl;
> +
> +       /* Let the fence complete on the eventfd read event. */
> +       dispatcher->processEvents();
> +}
> +

Same here...

> +void FenceTest::completed()
> +{
> +       if (fence_->expired())
> +               return;
> +
> +       uint64_t value;
> +       ssize_t ret = read(eventfd_, &value, sizeof(value));
> +       if (ret != sizeof(value))
> +               cerr << "Failed to read from event fd" << endl;
> +}
> +
> +int FenceTest::run()
> +{
> +       /* Activate the fence and schedule a wake-up before it expires. */
> +       fence_->enable();
> +       timer_.start(50);

Does this activate the underlying eventfd somehow?
(Edit, yes, I see it now - but it wasn't obvious to start with).

> +
> +       dispatcher->processEvents();
> +
> +       if (fence_->expired()) {
> +               cerr << "Fence should not have expired" << endl;
> +               return TestFail;
> +       }
> +
> +       if (!fence_->completed()) {
> +               cerr << "Fence should have completed" << endl;
> +               return TestFail;
> +       }
> +
> +       /* Now let the fence expire. */
> +       fence_->enable();
> +       dispatcher->processEvents();
> +
> +       if (fence_->completed()) {
> +               cerr << "Fence should not have completed" << endl;
> +               return TestFail;
> +       }
> +
> +       if (!fence_->expired()) {
> +               cerr << "Fence should have expired" << endl;
> +               return TestFail;
> +       }
> +
> +       /*
> +        * Test fence move semantic.
> +        *
> +        * Create two temporary fences and verify we can move them.
> +        */
> +       Fence fence1(eventfd_, 500);
> +       Fence fence2(0, 500);
> +       fence2 = std::move(fence1);
> +
> +       if (fence1.fd() != -1) {
> +               cerr << "A moved fence should have an invalid fd" << endl;
> +               return TestFail;
> +       }
> +
> +       if (fence2.fd() != eventfd_ || fence2.timeout() != 500) {
> +               cerr << "Faile to move fence" << endl;
> +               return TestFail;
> +       }

What happens to the signals that are connected to fence1 and fence2. Is
there anything we can do to test those, and their expected interactions
after a move?


> +
> +       return 0;
> +}
> +
> +void FenceTest::cleanup()
> +{
> +       close(eventfd_);
> +       delete fence_;
> +}
> +
> +TEST_REGISTER(FenceTest)
> diff --git a/test/meson.build b/test/meson.build
> index d0466f17d7b6..377e392628bf 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -40,6 +40,7 @@ internal_tests = [
>      ['event-dispatcher',                'event-dispatcher.cpp'],
>      ['event-thread',                    'event-thread.cpp'],
>      ['file',                            'file.cpp'],
> +    ['fence',                           'fence.cpp'],
>      ['file-descriptor',                 'file-descriptor.cpp'],
>      ['flags',                           'flags.cpp'],
>      ['hotplug-cameras',                 'hotplug-cameras.cpp'],
> -- 
> 2.33.1
>