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

14399 diff mbox series

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@jmondi.org>
---
 test/fence.cpp   | 148 +++++++++++++++++++++++++++++++++++++++++++++++
 test/meson.build |   1 +
 2 files changed, 149 insertions(+)
 create mode 100644 test/fence.cpp

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@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
>

Hi Kieran,

On Tue, Nov 09, 2021 at 01:12:34PM +0000, Kieran Bingham wrote:
> 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@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.
>

Ups

> > + *
> > + * 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...?
>

It would break the move-only semantic of the Fence class. Making sure
the underlying FileDescriptor is handled by a single component is the
key to make it easier to guarantee the Fence is either closed or has
to be handled by the application when returned from a completed
Request.

We could indeed make the FrameBuffer interface accept a &&FileDescriptor
to make sure application do not try to close it while it is being
handled by the library.

> > +               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?

As per the previous review comment, I should disconnect slots from a
moved fence.

Thanks
  j

>
>
> > +
> > +       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
> >