Re: [Intel-gfx] [PATCH i-g-t v5 06/13] tests/sw_sync: Add subtest test_sync_wait

[Chris Wilson]

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.f...@collabora.com wrote:
> >>From: Robert Foss 
> >>
> >>This subtest verifies that waiting on fences works properly.
> >>
> >>Signed-off-by: Robert Foss 
> >>Reviewed-by: Eric Engestrom 
> >>---
> >> 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

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v5 06/13] tests/sw_sync: Add subtest test_sync_wait

[Robert Foss]

On 2016-09-15 04:32 PM, Chris Wilson wrote:

On Thu, Sep 15, 2016 at 02:40:11PM -0400, robert.f...@collabora.com wrote:

From: Robert Foss 

This subtest verifies that waiting on fences works properly.

Signed-off-by: Robert Foss 
Reviewed-by: Eric Engestrom 
---
 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.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v5 06/13] tests/sw_sync: Add subtest test_sync_wait

[Chris Wilson]

On Thu, Sep 15, 2016 at 02:40:11PM -0400, robert.f...@collabora.com wrote:
> From: Robert Foss 
> 
> This subtest verifies that waiting on fences works properly.
> 
> Signed-off-by: Robert Foss 
> Reviewed-by: Eric Engestrom 
> ---
>  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

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v5 06/13] tests/sw_sync: Add subtest test_sync_wait

[Chris Wilson]

On Thu, Sep 15, 2016 at 02:40:11PM -0400, robert.f...@collabora.com wrote:
> From: Robert Foss 
> 
> This subtest verifies that waiting on fences works properly.
> 
> Signed-off-by: Robert Foss 
> Reviewed-by: Eric Engestrom 
> ---
>  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);
> +}

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t v5 06/13] tests/sw_sync: Add subtest test_sync_wait

[robert . foss]

From: Robert Foss 

This subtest verifies that waiting on fences works properly.

Signed-off-by: Robert Foss 
Reviewed-by: Eric Engestrom 
---
 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)
+{
+   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();
 }
 
-- 
2.9.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx