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

[Antonio Argenziano]

On 14/02/19 16:15, Chris Wilson wrote:
> Quoting Antonio Argenziano (2019-02-15 00:04:48)
>> use IOCTLs gem_read for reading and, gem_sync to wait on a batch, this
>> will help testing platforms that might not expose gtt mapping.
>>
>> v2:
>>          - use a local helper for reading from BOs. (Chris)
>>          - Always sync before reading. (Chris)
>>
>> 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 | 132 ++++++++++++++++-----------------
>>   1 file changed, 64 insertions(+), 68 deletions(-)
>>
>> diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
>> index 00f9528a..5e48219e 100644
>> --- a/tests/i915/gem_exec_schedule.c
>> +++ b/tests/i915/gem_exec_schedule.c
>> @@ -49,6 +49,26 @@
>>   
>>   IGT_TEST_DESCRIPTION("Check that we can control the order of execution");
>>   
>> +static inline
>> +uint32_t __sync_read_u32(int fd, uint32_t handle, uint64_t offset)
>> +{
>> +       uint32_t value;
>> +
>> +       gem_sync(fd, handle); /* No write hazard lies! */
>> +       gem_read(fd, handle, 0, &value, sizeof(value));
> 
> Trying to subtly point out that offset is always 0?

Yes, absolutely not a copy-paste gone wrong.

> 
>> +
>> +       return value;
>> +
>> +}
>> +
>> +static inline
>> +void __sync_range_read_u32(int fd, uint32_t handle, uint32_t *dest, uint64_t size)
> 
> I probably would have gone with sync_read_u32_count, or
> sync_read(void *dst, uint64_t size)

Wouldn't you still need an handle in this ^

I like sync_read_u32_count so I'll go with that :).

Thanks,
Antonio

> 
> (I have no idea why people insist that there's an 'e' in dstination)
> 
>> @@ -287,22 +304,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);
>> +       __sync_range_read_u32(fd, scratch, result, sizeof(result));
>>          gem_close(fd, scratch);
>>   
>>          for (unsigned n = 0; n < ncpus; n++) {
>> -               igt_assert_eq_u32(ptr[2*n], ~n);
>> +               igt_assert_eq_u32(result[2 * n], ~n);
>>                  /*
>>                   * Note this count is approximate due to unconstrained
>>                   * ordering of the dword writes between engines.
>>                   *
>>                   * Take the result with a pinch of salt.
>>                   */
>> -               igt_info("Child[%d] completed %u cycles\n",  n, ptr[2*n+1]);
>> +               igt_info("Child[%d] completed %u cycles\n",  n, result[(2 * n) + 1]);
> 
> (More brackets) + yes, please. :)
> 
> Didn't spot anything else, looks technically correct and quite possibly an
> improvement for existing users.
> -Chris
>