[igt-dev] [PATCH i-g-t] tests/i915/gem_exec_schedule.c: Switch to gem_sync and gem_read

[Antonio Argenziano]

On 14/02/19 00:59, Chris Wilson wrote:
> Quoting Antonio Argenziano (2019-02-14 01:22:50)
>> use IOCTLs gem_read for reading and, gem_sync to wait on a batch.
> 
> At least give a hint as to why.
>   
>> Signed-off-by: Antonio Argenziano <antonio.argenziano at intel.com>
>>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>>   tests/i915/gem_exec_schedule.c | 113 +++++++++++++--------------------
>>   1 file changed, 45 insertions(+), 68 deletions(-)
>>
>> diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
>> index 00f9528a..5df48960 100644
>> --- a/tests/i915/gem_exec_schedule.c
>> +++ b/tests/i915/gem_exec_schedule.c
>> @@ -152,7 +152,7 @@ static void fifo(int fd, unsigned ring)
>>   {
>>          IGT_CORK_HANDLE(cork);
>>          uint32_t scratch, plug;
>> -       uint32_t *ptr;
>> +       uint32_t result;
>>   
>>          scratch = gem_create(fd, 4096);
>>   
>> @@ -165,13 +165,10 @@ static void fifo(int fd, unsigned ring)
>>          unplug_show_queue(fd, &cork, ring);
>>          gem_close(fd, plug);
>>   
>> -       ptr = gem_mmap__gtt(fd, scratch, 4096, PROT_READ);
>> -       gem_set_domain(fd, scratch, /* no write hazard lies! */
>> -                       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> 
> gem_sync.
> 
>> +       gem_read(fd, scratch, 0, &result, sizeof(result));
> 
> So I think something like a
> 
> static inline uint32_t
> sync_read_u32(int i915, uint32_t handle, uint64_t offset)
> {
> 	uint32_t result;
> 
> 	gem_sync(i915, handle); /* no write hazard lies! */
> 	gem_read(i915, handle, offset, &results, sizeof(result));
> 
> 	return result;
> }
> 
> result = sync_read_u32(fd, scratch, 0);
> 
> would tidy up most of the conversions.
> 
>>          gem_close(fd, scratch);
>>   
>> -       igt_assert_eq_u32(ptr[0], 2);
>> -       munmap(ptr, 4096);
>> +       igt_assert_eq_u32(result, 2);
>>   }
>>   
>>   static void independent(int fd, unsigned int engine)
>> @@ -249,7 +246,7 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
>>          unsigned nengine;
>>          unsigned engine;
>>          uint32_t scratch;
>> -       uint32_t *ptr;
>> +       uint32_t result[4096 / sizeof(uint32_t)];
> 
> Is ncpus not known at this point, could it be?
> uint32_t results[2 * ncpus];

I went with the size of the source mainly because it is easier to find 
at a glance.

Antonio

> 
>>          nengine = 0;
>>          if (ring == ALL_ENGINES) {
>> @@ -287,22 +284,19 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
>>          }
>>          igt_waitchildren();
>>   
>> -       ptr = gem_mmap__gtt(fd, scratch, 4096, PROT_READ);
>> -       gem_set_domain(fd, scratch, /* no write hazard lies! */
>> -                       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> 
> Anything that has /* no write hazard lies! */ must be replaced by
> gem_sync() with the same warning.
> 
>> +       gem_read(fd, scratch, 0, result, 4096);
> s/4096/sizeof(results))
>>          gem_close(fd, scratch);
> 
> [snip more of the same]
> -Chris
>