lib/i915/gem_mman: Add functions to get mmap and gtt versions

Do not limit to the GTT for tests which are not specifically testing capability in the GTT.

Signed-off-by Stuart Summers <[email protected]>

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

lib/i915/gem_mman.c +5 -0

index a0e34aef..d37840f7 100644
@@ -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;
+

lib/i915/gem_mman.c +3 -0

+       return ptr;
+}
+
 /**
  * __gem_mmap__gtt:
  * @fd: open i915 drm file descriptor

lib/i915/gem_mman.h +1 -0

index 096ff592..e1714360 100644
@@ -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);
 

tests/i915/gem_ctx_shared.c +0 -0

index a6eee16d..74fccd95 100644

tests/i915/gem_ctx_shared.c +3 -9

                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]);

Do not limit to the GTT for tests which are not specifically testing capability in the GTT.

Signed-off-by Stuart Summers <[email protected]>

---

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

lib/i915/gem_mman.c +0 -0

index a0e34aef..d37840f7 100644

lib/i915/gem_mman.c +18 -0

+{
+	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);
+
+	return ptr;
+}
+
 /**
 * __gem_mmap__gtt:
 * @fd: open i915 drm file descriptor

lib/i915/gem_mman.h +1 -0

index 096ff592..e1714360 100644
@@ -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);
 

tests/i915/gem_ctx_shared.c +5 -15

index a6eee16d..74fccd95 100644
@@ -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]);
 
 		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);
 
 	for (unsigned n = 0; n < ncpus; n++) {

tests/i915/gem_exec_schedule.c +1 -2

index dc1c3a92..7f55a0a8 100644
@@ -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);
 
 		for (int m = 0; m < ring_size; m++) {
 			uint64_t addr;

Signature · 6 lines

2.21.0.5.gaeb582a983

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Do not limit to the GTT for tests which are not specifically testing capability in the GTT.

Signed-off-by Stuart Summers <[email protected]>

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

lib/i915/gem_mman.c +5 -0

index a0e34aef..d37840f7 100644
@@ -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;
+

Quoting Stuart Summers (2019-11-19 18:16:19)

Quoted 33 lines

Do not limit to the GTT for tests which are not specifically testing
capability in the GTT.

Signed-off-by Stuart Summers <[email protected]>

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

lib/i915/gem_mman.c +12 -0

index a0e34aef..d37840f7 100644
@@ -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;
+       }

lib/i915/gem_mman.c line 57

Quoted 3 lines

lib/i915/gem_mman.c +3 -0

+
+       gem_set_domain(fd, handle, /* no write hazard lies! */
+                      domain, domain);

And be very careful putting the set-domain in here; as the lies are integral to the test and not suitable for libraries. Domain handling is
sadly of paramount importance for most tests.

-Chris

Quoted 34 lines

Quoting Stuart Summers (2019-11-19 18:16:19)

Quoted 33 lines

Do not limit to the GTT for tests which are not specifically testing
capability in the GTT.

Signed-off-by Stuart Summers <[email protected]>

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

lib/i915/gem_mman.c +12 -0

index a0e34aef..d37840f7 100644
@@ -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;
+       }

Quoted 8 lines

lib/i915/gem_mman.c line 57

Quoted 3 lines

lib/i915/gem_mman.c +3 -0

+
+       gem_set_domain(fd, handle, /* no write hazard lies! */
+                      domain, domain);

And be very careful putting the set-domain in here; as the lies are integral to the test and not suitable for libraries. Domain handling is
sadly of paramount importance for most tests.

Hm.. so maybe a little more in the kernel to safeguard here. I was thinking of doing that anyway. Then we can adopt the tests as needed.

And for these, maybe the right way to go is to use the new mmap_offset Zbigniew is implementing. The IOCTL posted by Abdiel already has a WC case for that.

Quoting Summers, Stuart (2019-11-19 18:58:31)

Quoted 46 lines · Chris Wilson

Quoted 36 lines

Quoting Stuart Summers (2019-11-19 18:16:19)

Quoted 35 lines

Do not limit to the GTT for tests which are not specifically testing
capability in the GTT.

Signed-off-by Stuart Summers <[email protected]>

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

lib/i915/gem_mman.c +14 -0

index a0e34aef..d37840f7 100644
@@ -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! */

Quoted 8 lines

lib/i915/gem_mman.c line 57

Quoted 1 line

lib/i915/gem_mman.c +1 -0

+                      domain, domain);

And be very careful putting the set-domain in here; as the lies are
integral to the test and not suitable for libraries. Domain handling
is
sadly of paramount importance for most tests.

Quoted 3 lines

Hm.. so maybe a little more in the kernel to safeguard here. I was thinking of doing that anyway. Then we can adopt the tests as needed.

/* no write hazard lies! */

is all about how we are circumventing the kernel's opinion on correct behaviour to get at the behaviour we require ;)

Quoted 5 lines

And for these, maybe the right way to go is to use the new mmap_offset
Zbigniew is implementing. The IOCTL posted by Abdiel already has a WC
case for that.

Yes. Pretty much all of these should be using gem_mmap__wc(), and whether that is implemented using mmap_offset_ioctl or mmap_ioctl is immaterial. The CPUs / platforms that cannot use WC are immaterial to most of these tests (these test in particular are only applicable to recent gen), but for a select few tests we will need to make sure that
they are still usable with gem_mmap__gtt.

So a possible gem_mmap__device_coherent() {
	ptr = gem_mmap__offset(.type = WC);
	if (ptr)
		return ptr;

	ptr = gem_mmap__wc();
	if (ptr)
		return ptr;

	return gem_mmap__gt();
}

Names and details left to the reader. We did decide that library functions like gem_mmap__wc() could use either mmap_offset_ioctl or mmap_ioctl as they saw fit; and tests that required exact ioctls would
call those ioctls explicitly.
-Chris

Quoted 11 lines

So a possible gem_mmap__device_coherent() {
      ptr = gem_mmap__offset(.type = WC);
      if (ptr)
              return ptr;

      ptr = gem_mmap__wc();
      if (ptr)
              return ptr;

      return gem_mmap__gt();
}

Yeah I've been playing around locally with something like this (or at least with the mmap_offset/mmap_gtt, your suggestion is cleaner). The problem is in application. Do we apply this to all tests which aren't expliclity doing GGTT testing? Or do we start with something like the gem_ctx_shared and expand over time? I don't have all of the history across the tests, so really appreciate the feedback here Chris.

Quoting Summers, Stuart (2019-11-19 22:00:17)

Quoted 13 lines · Chris Wilson

Quoted 11 lines

So a possible gem_mmap__device_coherent() {
      ptr = gem_mmap__offset(.type = WC);
      if (ptr)
              return ptr;

      ptr = gem_mmap__wc();
      if (ptr)
              return ptr;

      return gem_mmap__gt();
}

Quoted 11 lines

Yeah I've been playing around locally with something like this (or at
least with the mmap_offset/mmap_gtt, your suggestion is cleaner). The
problem is in application. Do we apply this to all tests which aren't
expliclity doing GGTT testing? Or do we start with something like the
gem_ctx_shared and expand over time? I don't have all of the history
across the tests, so really appreciate the feedback here Chris.

In almost all cases, gtt was being used only because it provided coherent [uncached] access. There are a few tests that are explicitly testing GTT features (and they are easy to spot). The other day I think
I counted around 100 test.c using gem_mmap__gtt of which a quick inspection said only 10% of those were looking at GTT features.

The biggest challenge is choosing the name and semantics of the
replacement: device/pci_coherent or something like that. And then define
whether or not we use gem_sync() + manual clflush for nonspecific testing, or stick with gem_set_domain(GTT) as the lowest common denominator. There's pros/cons for either choice (and there will always
be corner cases that explicitly need one or the other) , just patches need to be written.

-Chris