[igt-dev] [PATCH i-g-t 2/2] lib/i915/gem_mman: mmap to the CPU instead of the GTT in some tests

[Chris Wilson]

Quoting Stuart Summers (2019-11-19 18:16:19)
> Do not limit to the GTT for tests which are not specifically testing
> capability in the GTT.
> 
> Signed-off-by: Stuart Summers <stuart.summers at intel.com>
> ---
>  lib/i915/gem_mman.c            | 19 +++++++++++++++++++
>  lib/i915/gem_mman.h            |  1 +
>  tests/i915/gem_ctx_shared.c    | 20 +++++---------------
>  tests/i915/gem_exec_schedule.c |  3 +--
>  4 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> index a0e34aef..d37840f7 100644
> --- a/lib/i915/gem_mman.c
> +++ b/lib/i915/gem_mman.c
> @@ -40,6 +40,25 @@
>  #define VG(x) do {} while (0)
>  #endif
>  
> +void *gem_mmap_host(int fd, uint32_t handle, uint64_t size, unsigned prot)
> +{
> +       void *ptr;
> +       uint32_t domain;
> +
> +       if (gem_has_mappable_ggtt(fd)) {
> +               ptr = gem_mmap__gtt(fd, handle, size, prot);
> +               domain = I915_GEM_DOMAIN_GTT;
> +       } else {
> +               ptr = gem_mmap__cpu(fd, handle, 0, size, prot);
> +               domain = I915_GEM_DOMAIN_CPU;
> +       }
> +
> +       gem_set_domain(fd, handle, /* no write hazard lies! */
> +                      domain, domain);

Ah, no. There's a huge difference in coherency between these 2 modes.
The test has to be prepared for handle to be snoopable.

> +       return ptr;
> +}
> +
>  /**
>   * __gem_mmap__gtt:
>   * @fd: open i915 drm file descriptor
> diff --git a/lib/i915/gem_mman.h b/lib/i915/gem_mman.h
> index 096ff592..e1714360 100644
> --- a/lib/i915/gem_mman.h
> +++ b/lib/i915/gem_mman.h
> @@ -25,6 +25,7 @@
>  #ifndef GEM_MMAN_H
>  #define GEM_MMAN_H
>  
> +void *gem_mmap_host(int fd, uint32_t handle, uint64_t size, unsigned prot);
>  void *gem_mmap__gtt(int fd, uint32_t handle, uint64_t size, unsigned prot);
>  void *gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t size, unsigned prot);
>  
> diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> index a6eee16d..74fccd95 100644
> --- a/tests/i915/gem_ctx_shared.c
> +++ b/tests/i915/gem_ctx_shared.c
> @@ -613,9 +613,7 @@ static void independent(int i915, unsigned ring, unsigned flags)
>         for (int i = 0; i < ARRAY_SIZE(priorities); i++) {
>                 uint32_t *ptr;
>  
> -               ptr = gem_mmap__gtt(i915, handle[i], 4096, PROT_READ);
> -               gem_set_domain(i915, handle[i], /* no write hazard lies! */
> -                              I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +               ptr = gem_mmap_host(i915, handle[i], 4096, PROT_READ);
>                 gem_close(i915, handle[i]);

There's also fun side-effects for test like these where mixing GTT vma
with ppGTT may be interesting.

>                 handle[i] = ptr[TIMESTAMP];
> @@ -658,9 +656,7 @@ static void reorder(int i915, unsigned ring, unsigned flags)
>         gem_context_destroy(i915, ctx[LO]);
>         gem_context_destroy(i915, ctx[HI]);
>  
> -       ptr = gem_mmap__gtt(i915, scratch, 4096, PROT_READ);
> -       gem_set_domain(i915, scratch, /* no write hazard lies! */
> -                      I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +       ptr = gem_mmap_host(i915, scratch, 4096, PROT_READ);
>         gem_close(i915, scratch);
>  
>         if (flags & EQUAL) /* equal priority, result will be fifo */
> @@ -713,17 +709,13 @@ static void promotion(int i915, unsigned ring)
>         gem_context_destroy(i915, ctx[LO]);
>         gem_context_destroy(i915, ctx[HI]);
>  
> -       ptr = gem_mmap__gtt(i915, dep, 4096, PROT_READ);
> -       gem_set_domain(i915, dep, /* no write hazard lies! */
> -                       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +       ptr = gem_mmap_host(i915, dep, 4096, PROT_READ);
>         gem_close(i915, dep);
>  
>         igt_assert_eq_u32(ptr[0], ctx[HI]);
>         munmap(ptr, 4096);
>  
> -       ptr = gem_mmap__gtt(i915, result, 4096, PROT_READ);
> -       gem_set_domain(i915, result, /* no write hazard lies! */
> -                       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +       ptr = gem_mmap_host(i915, result, 4096, PROT_READ);
>         gem_close(i915, result);
>  
>         igt_assert_eq_u32(ptr[0], ctx[NOISE]);
> @@ -776,9 +768,7 @@ static void smoketest(int i915, unsigned ring, unsigned timeout)
>         }
>         igt_waitchildren();
>  
> -       ptr = gem_mmap__gtt(i915, scratch, 4096, PROT_READ);
> -       gem_set_domain(i915, scratch, /* no write hazard lies! */
> -                       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +       ptr = gem_mmap_host(i915, scratch, 4096, PROT_READ);
>         gem_close(i915, scratch);

I can't think of any immediate reason why this couldn't be gem_mmap__wc.

> diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
> index dc1c3a92..7f55a0a8 100644
> --- a/tests/i915/gem_exec_schedule.c
> +++ b/tests/i915/gem_exec_schedule.c
> @@ -1404,8 +1404,7 @@ static void reorder_wide(int fd, unsigned ring)
>                 gem_context_set_priority(fd, execbuf.rsvd1, n);
>  
>                 obj[1].handle = gem_create(fd, sz);
> -               batch = gem_mmap__gtt(fd, obj[1].handle, sz, PROT_WRITE);
> -               gem_set_domain(fd, obj[1].handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +               gem_mmap_host(fd, obj[1].handle, sz, PROT_WRITE);

But this one rings alarms bells that we tried and failed to make it WC,
but I can't remember what went wrong.

However, WB is definitely wrong for these tests across all current
platforms.
-Chris