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

[Robert Foss <robert.foss@collabora.com>]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx