| Message ID | 20160915184018.9218-7-robert.foss@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 15, 2016 at 02:40:11PM -0400, robert.foss@collabora.com wrote: > From: Robert Foss <robert.foss@collabora.com> > > This subtest verifies that waiting on fences works properly. > > Signed-off-by: Robert Foss <robert.foss@collabora.com> > Reviewed-by: Eric Engestrom <eric@engestrom.ch> > --- > tests/sw_sync.c | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/tests/sw_sync.c b/tests/sw_sync.c > index fcb2f57..3061279 100644 > --- a/tests/sw_sync.c > +++ b/tests/sw_sync.c > @@ -81,6 +81,41 @@ static void test_alloc_merge_fence(void) > close(timeline[1]); > } > > +static void test_sync_wait(void) These are not testing waits but busy queries. static void test_sync_busy(void) > +{ > + int fence, ret; > + int timeline; > + > + timeline = sw_sync_timeline_create(); > + fence = sw_sync_fence_create(timeline, 5); > + > + /* Wait on fence until timeout */ Misleading comment > + ret = sw_sync_wait(fence, 0); > + igt_assert_f(ret == 0, "Failure waiting on fence until timeout\n"); igt_assert_f(ret == 0, "Fence created in an unexpectedly signaled state\n"); > + > + /* Advance timeline from 0 -> 1 */ > + sw_sync_timeline_inc(timeline, 1); > + > + /* Wait on fence until timeout */ > + ret = sw_sync_wait(fence, 0); > + igt_assert_f(ret == 0, "Failure waiting on fence until timeout\n"); igt_assert_f(ret == 0, "Fence signaled earlier (timeline value 1, fence seqno 5)\n"); > + > + /* Signal the fence */ /* Advance timeline from 1 -> 5: signaling the fence (seqno 5)*/ > + sw_sync_timeline_inc(timeline, 4); > + > + /* Wait successfully */ Usless comment > + ret = sw_sync_wait(fence, 0); > + igt_assert_f(ret > 0, "Failure waiting on fence\n"); igt_assert_f(ret == 0, "Fence not signaled (timeline value 5, fence seqno 5)\n"); If we have a timeline info, we could use %d to say the current value. Another test would be to then seqno = 0; for (i = 0; i < n_primes; i++) { seqno += primes[i]; sw_sync_timeline_inc(timeline, primes[i]); igt_assert_eq(sw_sync_timeline_get_seqno(timeline), seqno); } > + > + /* Go even further, and confirm wait still succeeds */ > + sw_sync_timeline_inc(timeline, 10); > + ret = sw_sync_wait(fence, 0); > + igt_assert_f(ret > 0, "Failure waiting ahead\n"); igt_assert_f(ret == 0, "Fence now not signaled! (timeline value 10, fence seqno 5)\n"); > + > + close(fence); > + close(timeline); > +}
On Thu, Sep 15, 2016 at 02:40:11PM -0400, robert.foss@collabora.com wrote: > From: Robert Foss <robert.foss@collabora.com> > > This subtest verifies that waiting on fences works properly. > > Signed-off-by: Robert Foss <robert.foss@collabora.com> > Reviewed-by: Eric Engestrom <eric@engestrom.ch> > --- > tests/sw_sync.c | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/tests/sw_sync.c b/tests/sw_sync.c > index fcb2f57..3061279 100644 > --- a/tests/sw_sync.c > +++ b/tests/sw_sync.c > @@ -81,6 +81,41 @@ static void test_alloc_merge_fence(void) > close(timeline[1]); > } > > +static void test_sync_wait(void) > +{ Ah, you don't have a real test_sync_wait() Nor do you check that fences are inherited across fork() or can be sent over unix sockets. For test_sync_wait(), some along the lines of create timeline in parent, fork + create fence in child (wakeup parent) wait(fence), .timeout = 10s), in parent sleep another 1s, increment timeline. Usual assertions. If the fence was created in the parent and forked to the child, the parent can assert the fence was signaled to ensure that we expect the wait to succeed. -Chris
On 2016-09-15 04:32 PM, Chris Wilson wrote: > On Thu, Sep 15, 2016 at 02:40:11PM -0400, robert.foss@collabora.com wrote: >> From: Robert Foss <robert.foss@collabora.com> >> >> This subtest verifies that waiting on fences works properly. >> >> Signed-off-by: Robert Foss <robert.foss@collabora.com> >> Reviewed-by: Eric Engestrom <eric@engestrom.ch> >> --- >> tests/sw_sync.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/tests/sw_sync.c b/tests/sw_sync.c >> index fcb2f57..3061279 100644 >> --- a/tests/sw_sync.c >> +++ b/tests/sw_sync.c >> @@ -81,6 +81,41 @@ static void test_alloc_merge_fence(void) >> close(timeline[1]); >> } >> >> +static void test_sync_wait(void) > > These are not testing waits but busy queries. test_sync_wait refers to sw_sync_wait, which may or may not be meaningful to refer to. Do you still prefer test_sync_busy? > > static void test_sync_busy(void) >> +{ >> + int fence, ret; >> + int timeline; >> + >> + timeline = sw_sync_timeline_create(); >> + fence = sw_sync_fence_create(timeline, 5); >> + >> + /* Wait on fence until timeout */ > > Misleading comment Agreed. > >> + ret = sw_sync_wait(fence, 0); >> + igt_assert_f(ret == 0, "Failure waiting on fence until timeout\n"); > > igt_assert_f(ret == 0, "Fence created in an unexpectedly signaled state\n"); Agreed. > >> + >> + /* Advance timeline from 0 -> 1 */ >> + sw_sync_timeline_inc(timeline, 1); >> + >> + /* Wait on fence until timeout */ >> + ret = sw_sync_wait(fence, 0); >> + igt_assert_f(ret == 0, "Failure waiting on fence until timeout\n"); > > igt_assert_f(ret == 0, "Fence signaled earlier (timeline value 1, fence seqno 5)\n"); Agreed. > >> + >> + /* Signal the fence */ > > /* Advance timeline from 1 -> 5: signaling the fence (seqno 5)*/ Agreed. >> + sw_sync_timeline_inc(timeline, 4); > >> + >> + /* Wait successfully */ > > Usless comment Agreed. > >> + ret = sw_sync_wait(fence, 0); >> + igt_assert_f(ret > 0, "Failure waiting on fence\n"); > > igt_assert_f(ret == 0, "Fence not signaled (timeline value 5, fence seqno 5)\n"); > > If we have a timeline info, we could use %d to say the current value. > > Another test would be to then > > seqno = 0; > for (i = 0; i < n_primes; i++) { > seqno += primes[i]; > sw_sync_timeline_inc(timeline, primes[i]); > igt_assert_eq(sw_sync_timeline_get_seqno(timeline), seqno); > } > This looks like a good addition, but primes has not previously been defined. Do you have preference for primes or would any increment, like random be ok? >> + >> + /* Go even further, and confirm wait still succeeds */ >> + sw_sync_timeline_inc(timeline, 10); >> + ret = sw_sync_wait(fence, 0); >> + igt_assert_f(ret > 0, "Failure waiting ahead\n"); > > igt_assert_f(ret == 0, "Fence now not signaled! (timeline value 10, fence seqno 5)\n"); Agreed. Thanks for the thorough review! Rob.
On Thu, Sep 15, 2016 at 06:25:13PM -0400, Robert Foss wrote: > > > On 2016-09-15 04:32 PM, Chris Wilson wrote: > >On Thu, Sep 15, 2016 at 02:40:11PM -0400, robert.foss@collabora.com wrote: > >>From: Robert Foss <robert.foss@collabora.com> > >> > >>This subtest verifies that waiting on fences works properly. > >> > >>Signed-off-by: Robert Foss <robert.foss@collabora.com> > >>Reviewed-by: Eric Engestrom <eric@engestrom.ch> > >>--- > >> tests/sw_sync.c | 38 ++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 38 insertions(+) > >> > >>diff --git a/tests/sw_sync.c b/tests/sw_sync.c > >>index fcb2f57..3061279 100644 > >>--- a/tests/sw_sync.c > >>+++ b/tests/sw_sync.c > >>@@ -81,6 +81,41 @@ static void test_alloc_merge_fence(void) > >> close(timeline[1]); > >> } > >> > >>+static void test_sync_wait(void) > > > >These are not testing waits but busy queries. > > test_sync_wait refers to sw_sync_wait, which may or may not be > meaningful to refer to. > Do you still prefer test_sync_busy? Yes. Querying the busy status (i.e. !signaled) is a common activity, and quite distinct to waiting. > >Another test would be to then > > > >seqno = 0; > >for (i = 0; i < n_primes; i++) { > > seqno += primes[i]; > > sw_sync_timeline_inc(timeline, primes[i]); > > igt_assert_eq(sw_sync_timeline_get_seqno(timeline), seqno); > >} > > > > This looks like a good addition, but primes has not previously been > defined. Do you have preference for primes or would any increment, > like random be ok? random would be ok, but for fun I just added igt_next_prime_number(x) and for_each_prime_number(prime, N). There are advantages to using primes and advantages to using random(), and advantages to using highly structured input. -Chris
diff --git a/tests/sw_sync.c b/tests/sw_sync.c index fcb2f57..3061279 100644 --- a/tests/sw_sync.c +++ b/tests/sw_sync.c @@ -81,6 +81,41 @@ static void test_alloc_merge_fence(void) close(timeline[1]); } +static void test_sync_wait(void) +{ + int fence, ret; + int timeline; + + timeline = sw_sync_timeline_create(); + fence = sw_sync_fence_create(timeline, 5); + + /* Wait on fence until timeout */ + ret = sw_sync_wait(fence, 0); + igt_assert_f(ret == 0, "Failure waiting on fence until timeout\n"); + + /* Advance timeline from 0 -> 1 */ + sw_sync_timeline_inc(timeline, 1); + + /* Wait on fence until timeout */ + ret = sw_sync_wait(fence, 0); + igt_assert_f(ret == 0, "Failure waiting on fence until timeout\n"); + + /* Signal the fence */ + sw_sync_timeline_inc(timeline, 4); + + /* Wait successfully */ + ret = sw_sync_wait(fence, 0); + igt_assert_f(ret > 0, "Failure waiting on fence\n"); + + /* Go even further, and confirm wait still succeeds */ + sw_sync_timeline_inc(timeline, 10); + ret = sw_sync_wait(fence, 0); + igt_assert_f(ret > 0, "Failure waiting ahead\n"); + + close(fence); + close(timeline); +} + igt_main { igt_subtest("alloc_timeline") @@ -94,5 +129,8 @@ igt_main igt_subtest("alloc_merge_fence") test_alloc_merge_fence(); + + igt_subtest("sync_wait") + test_sync_wait(); }