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

[Chris Wilson]

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?

> +
> +       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)

(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