| Message ID | 20160915184018.9218-8-robert.foss@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 15, 2016 at 02:40:12PM -0400, robert.foss@collabora.com wrote: > From: Robert Foss <robert.foss@collabora.com> > > Add subtest test_sync_merge that tests merging fences and the validity of the > resulting merged fence. > > Signed-off-by: Robert Foss <robert.foss@collabora.com> > Reviewed-by: Eric Engestrom <eric@engestrom.ch> > --- > tests/sw_sync.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 67 insertions(+) > > diff --git a/tests/sw_sync.c b/tests/sw_sync.c > index 3061279..26226bd 100644 > --- a/tests/sw_sync.c > +++ b/tests/sw_sync.c > @@ -116,6 +116,70 @@ static void test_sync_wait(void) > close(timeline); > } > > +static void test_sync_merge(void) > +{ > + int in_fence[3]; > + int fence_merge; > + int timeline; > + int active, signaled; > + > + timeline = sw_sync_timeline_create(); > + in_fence[0] = sw_sync_fence_create(timeline, 1); > + in_fence[1] = sw_sync_fence_create(timeline, 2); > + in_fence[2] = sw_sync_fence_create(timeline, 3); > + > + fence_merge = sw_sync_merge(in_fence[0], in_fence[1]); > + fence_merge = sw_sync_merge(in_fence[2], fence_merge); sw_sync_merge() really does need the negative tests: invalid fd (-1), device fd (/dev/dri/card0), file fd. should cover the most common errors (fuzz testing will hit the rest!) > + > + /* confirm all fences have one active point (even d) */ > + active = sw_sync_fence_count_status(in_fence[0], > + SW_SYNC_FENCE_STATUS_ACTIVE); > + igt_assert_f(active == 1, "in_fence[0] has too many active fences\n"); > + active = sw_sync_fence_count_status(in_fence[1], > + SW_SYNC_FENCE_STATUS_ACTIVE); > + igt_assert_f(active == 1, "in_fence[1] has too many active fences\n"); > + active = sw_sync_fence_count_status(in_fence[2], > + SW_SYNC_FENCE_STATUS_ACTIVE); > + igt_assert_f(active == 1, "in_fence[2] has too many active fences\n"); > + active = sw_sync_fence_count_status(fence_merge, > + SW_SYNC_FENCE_STATUS_ACTIVE); > + igt_assert_f(active == 1, "fence_merge has too many active fences\n"); > + > + /* confirm that fence_merge is not signaled until the max of fence 0,1,2 */ > + sw_sync_timeline_inc(timeline, 1); > + signaled = sw_sync_fence_count_status(in_fence[0], > + SW_SYNC_FENCE_STATUS_SIGNALED); This is missing from the earlier test_sync_busy(). > + active = sw_sync_fence_count_status(fence_merge, > + SW_SYNC_FENCE_STATUS_ACTIVE); > + igt_assert_f(signaled == 1, "in_fence[0] did not signal\n"); > + igt_assert_f(active == 1, "fence_merge signaled too early\n"); > + > + sw_sync_timeline_inc(timeline, 1); > + signaled = sw_sync_fence_count_status(in_fence[1], > + SW_SYNC_FENCE_STATUS_SIGNALED); > + active = sw_sync_fence_count_status(fence_merge, > + SW_SYNC_FENCE_STATUS_ACTIVE); > + igt_assert_f(signaled == 1, "in_fence[1] did not signal\n"); > + igt_assert_f(active == 1, "fence_merge signaled too early\n"); > + > + sw_sync_timeline_inc(timeline, 1); > + signaled = sw_sync_fence_count_status(in_fence[2], > + SW_SYNC_FENCE_STATUS_SIGNALED); > + igt_assert_f(signaled == 1, "in_fence[2] did not signal\n"); > + signaled = sw_sync_fence_count_status(fence_merge, > + SW_SYNC_FENCE_STATUS_SIGNALED); > + active = sw_sync_fence_count_status(fence_merge, > + SW_SYNC_FENCE_STATUS_ACTIVE); > + igt_assert_f(active == 0 && signaled == 1, > + "fence_merge did not signal\n"); Hmm, counting STATUS_SIGNALED / STATUS_ACTIVE is not behaving how I would intuitively expect. At this point, timeline.seqno = 3, I would expect count_signaled(merge) == 3 (and count_fences(merge) == 3 => merge is now signaled). But that's just my expectations -Chris
On 2016-09-15 04:41 PM, Chris Wilson wrote: > On Thu, Sep 15, 2016 at 02:40:12PM -0400, robert.foss@collabora.com wrote: >> From: Robert Foss <robert.foss@collabora.com> >> >> Add subtest test_sync_merge that tests merging fences and the validity of the >> resulting merged fence. >> >> Signed-off-by: Robert Foss <robert.foss@collabora.com> >> Reviewed-by: Eric Engestrom <eric@engestrom.ch> >> --- >> tests/sw_sync.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 67 insertions(+) >> >> diff --git a/tests/sw_sync.c b/tests/sw_sync.c >> index 3061279..26226bd 100644 >> --- a/tests/sw_sync.c >> +++ b/tests/sw_sync.c >> @@ -116,6 +116,70 @@ static void test_sync_wait(void) >> close(timeline); >> } >> >> +static void test_sync_merge(void) >> +{ >> + int in_fence[3]; >> + int fence_merge; >> + int timeline; >> + int active, signaled; >> + >> + timeline = sw_sync_timeline_create(); >> + in_fence[0] = sw_sync_fence_create(timeline, 1); >> + in_fence[1] = sw_sync_fence_create(timeline, 2); >> + in_fence[2] = sw_sync_fence_create(timeline, 3); >> + >> + fence_merge = sw_sync_merge(in_fence[0], in_fence[1]); >> + fence_merge = sw_sync_merge(in_fence[2], fence_merge); > > sw_sync_merge() really does need the negative tests: > > invalid fd (-1), > device fd (/dev/dri/card0), > file fd. Open other descriptors sounds like a good idea, but for device and file fds, which ones will always be available and open-able on any system? > > should cover the most common errors (fuzz testing will hit the rest!) > >> + >> + /* confirm all fences have one active point (even d) */ >> + active = sw_sync_fence_count_status(in_fence[0], >> + SW_SYNC_FENCE_STATUS_ACTIVE); >> + igt_assert_f(active == 1, "in_fence[0] has too many active fences\n"); >> + active = sw_sync_fence_count_status(in_fence[1], >> + SW_SYNC_FENCE_STATUS_ACTIVE); >> + igt_assert_f(active == 1, "in_fence[1] has too many active fences\n"); >> + active = sw_sync_fence_count_status(in_fence[2], >> + SW_SYNC_FENCE_STATUS_ACTIVE); >> + igt_assert_f(active == 1, "in_fence[2] has too many active fences\n"); >> + active = sw_sync_fence_count_status(fence_merge, >> + SW_SYNC_FENCE_STATUS_ACTIVE); >> + igt_assert_f(active == 1, "fence_merge has too many active fences\n"); >> + >> + /* confirm that fence_merge is not signaled until the max of fence 0,1,2 */ >> + sw_sync_timeline_inc(timeline, 1); > >> + signaled = sw_sync_fence_count_status(in_fence[0], >> + SW_SYNC_FENCE_STATUS_SIGNALED); > > This is missing from the earlier test_sync_busy(). > >> + active = sw_sync_fence_count_status(fence_merge, >> + SW_SYNC_FENCE_STATUS_ACTIVE); >> + igt_assert_f(signaled == 1, "in_fence[0] did not signal\n"); >> + igt_assert_f(active == 1, "fence_merge signaled too early\n"); >> + >> + sw_sync_timeline_inc(timeline, 1); >> + signaled = sw_sync_fence_count_status(in_fence[1], >> + SW_SYNC_FENCE_STATUS_SIGNALED); >> + active = sw_sync_fence_count_status(fence_merge, >> + SW_SYNC_FENCE_STATUS_ACTIVE); >> + igt_assert_f(signaled == 1, "in_fence[1] did not signal\n"); >> + igt_assert_f(active == 1, "fence_merge signaled too early\n"); >> + >> + sw_sync_timeline_inc(timeline, 1); >> + signaled = sw_sync_fence_count_status(in_fence[2], >> + SW_SYNC_FENCE_STATUS_SIGNALED); >> + igt_assert_f(signaled == 1, "in_fence[2] did not signal\n"); >> + signaled = sw_sync_fence_count_status(fence_merge, >> + SW_SYNC_FENCE_STATUS_SIGNALED); >> + active = sw_sync_fence_count_status(fence_merge, >> + SW_SYNC_FENCE_STATUS_ACTIVE); >> + igt_assert_f(active == 0 && signaled == 1, >> + "fence_merge did not signal\n"); > > Hmm, counting STATUS_SIGNALED / STATUS_ACTIVE is not behaving how I > would intuitively expect. > > At this point, timeline.seqno = 3, I would expect count_signaled(merge) > == 3 (and count_fences(merge) == 3 => merge is now signaled). > But that's just my expectations I'll have a look into this. Rob.
On Thu, Sep 15, 2016 at 08:27:15PM -0400, Robert Foss wrote: > > > On 2016-09-15 04:41 PM, Chris Wilson wrote: > >On Thu, Sep 15, 2016 at 02:40:12PM -0400, robert.foss@collabora.com wrote: > >>From: Robert Foss <robert.foss@collabora.com> > >> > >>Add subtest test_sync_merge that tests merging fences and the validity of the > >>resulting merged fence. > >> > >>Signed-off-by: Robert Foss <robert.foss@collabora.com> > >>Reviewed-by: Eric Engestrom <eric@engestrom.ch> > >>--- > >> tests/sw_sync.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 67 insertions(+) > >> > >>diff --git a/tests/sw_sync.c b/tests/sw_sync.c > >>index 3061279..26226bd 100644 > >>--- a/tests/sw_sync.c > >>+++ b/tests/sw_sync.c > >>@@ -116,6 +116,70 @@ static void test_sync_wait(void) > >> close(timeline); > >> } > >> > >>+static void test_sync_merge(void) > >>+{ > >>+ int in_fence[3]; > >>+ int fence_merge; > >>+ int timeline; > >>+ int active, signaled; > >>+ > >>+ timeline = sw_sync_timeline_create(); > >>+ in_fence[0] = sw_sync_fence_create(timeline, 1); > >>+ in_fence[1] = sw_sync_fence_create(timeline, 2); > >>+ in_fence[2] = sw_sync_fence_create(timeline, 3); > >>+ > >>+ fence_merge = sw_sync_merge(in_fence[0], in_fence[1]); > >>+ fence_merge = sw_sync_merge(in_fence[2], fence_merge); > > > >sw_sync_merge() really does need the negative tests: > > > >invalid fd (-1), > >device fd (/dev/dri/card0), > >file fd. > > Open other descriptors sounds like a good idea, but for device and fd = drm_open_driver(DRIVER_ANY) tmppath[] = "/tmp/igt-XXXXXX"; fd = mkstemp(tmppath); if (fd != -1) { unlink(tmppath); test_with_invalid_fd(fd); close(fd); } -Chris
diff --git a/tests/sw_sync.c b/tests/sw_sync.c index 3061279..26226bd 100644 --- a/tests/sw_sync.c +++ b/tests/sw_sync.c @@ -116,6 +116,70 @@ static void test_sync_wait(void) close(timeline); } +static void test_sync_merge(void) +{ + int in_fence[3]; + int fence_merge; + int timeline; + int active, signaled; + + timeline = sw_sync_timeline_create(); + in_fence[0] = sw_sync_fence_create(timeline, 1); + in_fence[1] = sw_sync_fence_create(timeline, 2); + in_fence[2] = sw_sync_fence_create(timeline, 3); + + fence_merge = sw_sync_merge(in_fence[0], in_fence[1]); + fence_merge = sw_sync_merge(in_fence[2], fence_merge); + + /* confirm all fences have one active point (even d) */ + active = sw_sync_fence_count_status(in_fence[0], + SW_SYNC_FENCE_STATUS_ACTIVE); + igt_assert_f(active == 1, "in_fence[0] has too many active fences\n"); + active = sw_sync_fence_count_status(in_fence[1], + SW_SYNC_FENCE_STATUS_ACTIVE); + igt_assert_f(active == 1, "in_fence[1] has too many active fences\n"); + active = sw_sync_fence_count_status(in_fence[2], + SW_SYNC_FENCE_STATUS_ACTIVE); + igt_assert_f(active == 1, "in_fence[2] has too many active fences\n"); + active = sw_sync_fence_count_status(fence_merge, + SW_SYNC_FENCE_STATUS_ACTIVE); + igt_assert_f(active == 1, "fence_merge has too many active fences\n"); + + /* confirm that fence_merge is not signaled until the max of fence 0,1,2 */ + sw_sync_timeline_inc(timeline, 1); + signaled = sw_sync_fence_count_status(in_fence[0], + SW_SYNC_FENCE_STATUS_SIGNALED); + active = sw_sync_fence_count_status(fence_merge, + SW_SYNC_FENCE_STATUS_ACTIVE); + igt_assert_f(signaled == 1, "in_fence[0] did not signal\n"); + igt_assert_f(active == 1, "fence_merge signaled too early\n"); + + sw_sync_timeline_inc(timeline, 1); + signaled = sw_sync_fence_count_status(in_fence[1], + SW_SYNC_FENCE_STATUS_SIGNALED); + active = sw_sync_fence_count_status(fence_merge, + SW_SYNC_FENCE_STATUS_ACTIVE); + igt_assert_f(signaled == 1, "in_fence[1] did not signal\n"); + igt_assert_f(active == 1, "fence_merge signaled too early\n"); + + sw_sync_timeline_inc(timeline, 1); + signaled = sw_sync_fence_count_status(in_fence[2], + SW_SYNC_FENCE_STATUS_SIGNALED); + igt_assert_f(signaled == 1, "in_fence[2] did not signal\n"); + signaled = sw_sync_fence_count_status(fence_merge, + SW_SYNC_FENCE_STATUS_SIGNALED); + active = sw_sync_fence_count_status(fence_merge, + SW_SYNC_FENCE_STATUS_ACTIVE); + igt_assert_f(active == 0 && signaled == 1, + "fence_merge did not signal\n"); + + sw_sync_fence_destroy(in_fence[0]); + sw_sync_fence_destroy(in_fence[1]); + sw_sync_fence_destroy(in_fence[2]); + sw_sync_fence_destroy(fence_merge); + sw_sync_timeline_destroy(timeline); +} + igt_main { igt_subtest("alloc_timeline") @@ -132,5 +196,8 @@ igt_main igt_subtest("sync_wait") test_sync_wait(); + + igt_subtest("sync_merge") + test_sync_merge(); }