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

[Robert Foss]

On 2016-09-15 04:41 PM, Chris Wilson wrote:
> On Thu, Sep 15, 2016 at 02:40:12PM -0400, robert.foss at collabora.com wrote:
>> From: Robert Foss <robert.foss at 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 at collabora.com>
>> Reviewed-by: Eric Engestrom <eric at 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.