[RFC PATCH i-g-t v4 0/4] Calculate softpin offsets from minimum GTT alignment

[RFC PATCH i-g-t v4 0/4] Calculate softpin offsets from minimum GTT alignment

[Janusz Krzysztofik]

Some tests assume 4kB page size while using softpin.  That assumption
may be wrong on future GEM backends with possibly larger minimum page
sizes.  As a result, those tests may either fail on softpin at offsets
which are incorrectly aligned, may silently skip such incorrectly
aligned addresses assuming them occupied by other users, or may always
succeed when examining invalid use patterns.

Provide a helper function that detects minimum page size and returns
the size order.  Use it in test which perform softpin to calculate
offsets suitable for actually used backing store.

Changelog:
v2: Don't skip failing offsets only when on full PPGTT,
  - simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Drop patch 1/2 "Don't filter out addresses when on PPGTT" - I don't
    know how to detect if the kernel is interfering with the user's GTT,
  - introduce patch 1/4 "lib: Move redundant local helpers to lib/",
    subsequent patch will use the helper,
  - introduce patch 2/4 "lib: Add GEM minimum page size helper",
    subsequent patches will use the new helper (inspired by Chris),
  - in former patch 2/2, now 3/4, initialize page size order with an
    actual minimum returned by the new helper (inspired by Chris),
  - add a new fix for gem_ctx_shared test (patch 4/4).
v4: Rename the helper, use 'minimum GTT alignment' term across the
    changes (Chris),
  - update variable names accordingly,
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users, then
  - simplify the helper code (Chris),
  - drop no longer required patch 1/4 "lib: Move redundant local
    helpers to lib/",
  - reintroduce former patch 1/2 as 1/4 "tests/gem_exec_reloc: Don't
    filter out invalid addresses" with the former not on full PPGTT
    requirement for skipping now replaced with error code checking.

Janusz Krzysztofik (4):
  tests/gem_exec_reloc: Don't filter out invalid addresses
  lib: Add minimum GTT alignment helper
  tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
  tests/gem_ctx_shared: Align objects using minimum GTT alignment

 lib/ioctl_wrappers.c        | 46 +++++++++++++++++++++++++++++++++++++
 lib/ioctl_wrappers.h        |  2 ++
 tests/i915/gem_ctx_shared.c |  6 +++--
 tests/i915/gem_exec_reloc.c | 22 ++++++++++++------
 4 files changed, 67 insertions(+), 9 deletions(-)

-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC PATCH i-g-t v4 0/4] Calculate softpin offsets from minimum GTT alignment

[Janusz Krzysztofik]

Some tests assume 4kB page size while using softpin.  That assumption
may be wrong on future GEM backends with possibly larger minimum page
sizes.  As a result, those tests may either fail on softpin at offsets
which are incorrectly aligned, may silently skip such incorrectly
aligned addresses assuming them occupied by other users, or may always
succeed when examining invalid use patterns.

Provide a helper function that detects minimum page size and returns
the size order.  Use it in test which perform softpin to calculate
offsets suitable for actually used backing store.

Changelog:
v2: Don't skip failing offsets only when on full PPGTT,
  - simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Drop patch 1/2 "Don't filter out addresses when on PPGTT" - I don't
    know how to detect if the kernel is interfering with the user's GTT,
  - introduce patch 1/4 "lib: Move redundant local helpers to lib/",
    subsequent patch will use the helper,
  - introduce patch 2/4 "lib: Add GEM minimum page size helper",
    subsequent patches will use the new helper (inspired by Chris),
  - in former patch 2/2, now 3/4, initialize page size order with an
    actual minimum returned by the new helper (inspired by Chris),
  - add a new fix for gem_ctx_shared test (patch 4/4).
v4: Rename the helper, use 'minimum GTT alignment' term across the
    changes (Chris),
  - update variable names accordingly,
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users, then
  - simplify the helper code (Chris),
  - drop no longer required patch 1/4 "lib: Move redundant local
    helpers to lib/",
  - reintroduce former patch 1/2 as 1/4 "tests/gem_exec_reloc: Don't
    filter out invalid addresses" with the former not on full PPGTT
    requirement for skipping now replaced with error code checking.

Janusz Krzysztofik (4):
  tests/gem_exec_reloc: Don't filter out invalid addresses
  lib: Add minimum GTT alignment helper
  tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment
  tests/gem_ctx_shared: Align objects using minimum GTT alignment

 lib/ioctl_wrappers.c        | 46 +++++++++++++++++++++++++++++++++++++
 lib/ioctl_wrappers.h        |  2 ++
 tests/i915/gem_ctx_shared.c |  6 +++--
 tests/i915/gem_exec_reloc.c | 22 ++++++++++++------
 4 files changed, 67 insertions(+), 9 deletions(-)

-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Janusz Krzysztofik]

Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
addresses for !ppgtt") introduced filtering of addresses possibly
occupied by other users of shared GTT.  Unfortunately, that filtering
doesn't distinguish between actually occupied addresses and otherwise
invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
when running on future backends with possibly larger minimum page
sizes, a half of calculated offsets to be tested will be incorrectly
detected as occupied by other users and silently skipped instead of
reported as a problem.  That will significantly distort the intended
test pattern.

Filter out failing addresses only if not reported as invalid.

v2: Skip unavailable addresses only when not running on full PPGTT.
v3: Replace the not on full PPGTT requirement for skipping with error
    code checking.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index 5f59fe99..423fed8b 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
-	int count, n;
+	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
@@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
 		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = __gem_execbuf(fd, &execbuf);
+		if (err) {
+			igt_assert(err != -EINVAL);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
@@ -562,8 +565,11 @@ static void basic_range(int fd, unsigned flags)
 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
 		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = __gem_execbuf(fd, &execbuf);
+		if (err) {
+			igt_assert(err != -EINVAL);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Janusz Krzysztofik]

Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
addresses for !ppgtt") introduced filtering of addresses possibly
occupied by other users of shared GTT.  Unfortunately, that filtering
doesn't distinguish between actually occupied addresses and otherwise
invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
when running on future backends with possibly larger minimum page
sizes, a half of calculated offsets to be tested will be incorrectly
detected as occupied by other users and silently skipped instead of
reported as a problem.  That will significantly distort the intended
test pattern.

Filter out failing addresses only if not reported as invalid.

v2: Skip unavailable addresses only when not running on full PPGTT.
v3: Replace the not on full PPGTT requirement for skipping with error
    code checking.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index 5f59fe99..423fed8b 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
-	int count, n;
+	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
@@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
 		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = __gem_execbuf(fd, &execbuf);
+		if (err) {
+			igt_assert(err != -EINVAL);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
@@ -562,8 +565,11 @@ static void basic_range(int fd, unsigned flags)
 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
 		execbuf.buffer_count = 1;
-		if (__gem_execbuf(fd, &execbuf))
+		err = __gem_execbuf(fd, &execbuf);
+		if (err) {
+			igt_assert(err != -EINVAL);
 			continue;
+		}
 
 		igt_debug("obj[%d] handle=%d, address=%llx\n",
 			  n, obj[n].handle, (long long)obj[n].offset);
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

Some tests assume 4kB offset alignment while using softpin.  That
assumption may be wrong on future GEM backends with possibly larger
minimum page sizes.  As a result, those tests may either fail on
softpin at offsets which are incorrectly aligned, may silently skip
such incorrectly aligned addresses assuming them occupied by other
users if incorrect detection method is used, or may always succeed
when examining invalid use patterns.

Provide a helper function that detects minimum GTT alignment.  Tests
may use it to calculate softpin offsets valid for actually used backing
store.

v2: Rename the helper, use 'minimum GTT alignment' term across the
    change (Chris),
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users, then
  - simplify the code (Chris).

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
---
 lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
 lib/ioctl_wrappers.h |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 628f8b83..f0ef8b2e 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -54,6 +54,7 @@
 #include "intel_io.h"
 #include "igt_debugfs.h"
 #include "igt_sysfs.h"
+#include "igt_x86.h"
 #include "config.h"
 
 #ifdef HAVE_VALGRIND
@@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
 	return has_softpin;
 }
 
+/**
+ * gem_gtt_min_alignment_order:
+ * @fd: open i915 drm file descriptor
+ *
+ * This function detects the minimum possible alignment of a soft-pinned gem
+ * object allocated from a default backing store.  It is useful for calculating
+ * correctly aligned softpin offsets.
+ * Since size order to size conversion (size = 1 << order) is less trivial
+ * than the opposite, the function returns the alignment order as more handy.
+ *
+ * Returns:
+ * Size order of the minimum GTT alignment
+ */
+int gem_gtt_min_alignment_order(int fd)
+{
+	struct drm_i915_gem_exec_object2 obj;
+	struct drm_i915_gem_execbuffer2 eb;
+	const uint32_t bbe = MI_BATCH_BUFFER_END;
+	int order;
+
+	/* no softpin => 4kB page size */
+	if (!gem_has_softpin(fd))
+		return 12;
+
+	memset(&obj, 0, sizeof(obj));
+	memset(&eb, 0, sizeof(eb));
+
+	obj.handle = gem_create(fd, 4096);
+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
+	eb.buffers_ptr = to_user_pointer(&obj);
+	eb.buffer_count = 1;
+	gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
+
+	for (order = 12; order < 64; order++) {
+		obj.offset = 1ull << order;
+		if (__gem_execbuf(fd, &eb) != -EINVAL)
+			break;
+	}
+	igt_assert(obj.offset < gem_aperture_size(fd));
+
+	gem_close(fd, obj.handle);
+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
+	return order;
+}
+
 /**
  * gem_has_exec_fence:
  * @fd: open i915 drm file descriptor
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index 03211c97..c8d57a7c 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -138,6 +138,8 @@ uint64_t gem_aperture_size(int fd);
 uint64_t gem_global_aperture_size(int fd);
 uint64_t gem_mappable_aperture_size(void);
 bool gem_has_softpin(int fd);
+int gem_gtt_min_alignment_order(int fd);
+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
 bool gem_has_exec_fence(int fd);
 
 /* check functions which auto-skip tests by calling igt_skip() */
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

Some tests assume 4kB offset alignment while using softpin.  That
assumption may be wrong on future GEM backends with possibly larger
minimum page sizes.  As a result, those tests may either fail on
softpin at offsets which are incorrectly aligned, may silently skip
such incorrectly aligned addresses assuming them occupied by other
users if incorrect detection method is used, or may always succeed
when examining invalid use patterns.

Provide a helper function that detects minimum GTT alignment.  Tests
may use it to calculate softpin offsets valid for actually used backing
store.

v2: Rename the helper, use 'minimum GTT alignment' term across the
    change (Chris),
  - use error numbers to distinguish between invalid offsets and
    addresses occupied by other users, then
  - simplify the code (Chris).

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
---
 lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
 lib/ioctl_wrappers.h |  2 ++
 2 files changed, 48 insertions(+)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 628f8b83..f0ef8b2e 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -54,6 +54,7 @@
 #include "intel_io.h"
 #include "igt_debugfs.h"
 #include "igt_sysfs.h"
+#include "igt_x86.h"
 #include "config.h"
 
 #ifdef HAVE_VALGRIND
@@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
 	return has_softpin;
 }
 
+/**
+ * gem_gtt_min_alignment_order:
+ * @fd: open i915 drm file descriptor
+ *
+ * This function detects the minimum possible alignment of a soft-pinned gem
+ * object allocated from a default backing store.  It is useful for calculating
+ * correctly aligned softpin offsets.
+ * Since size order to size conversion (size = 1 << order) is less trivial
+ * than the opposite, the function returns the alignment order as more handy.
+ *
+ * Returns:
+ * Size order of the minimum GTT alignment
+ */
+int gem_gtt_min_alignment_order(int fd)
+{
+	struct drm_i915_gem_exec_object2 obj;
+	struct drm_i915_gem_execbuffer2 eb;
+	const uint32_t bbe = MI_BATCH_BUFFER_END;
+	int order;
+
+	/* no softpin => 4kB page size */
+	if (!gem_has_softpin(fd))
+		return 12;
+
+	memset(&obj, 0, sizeof(obj));
+	memset(&eb, 0, sizeof(eb));
+
+	obj.handle = gem_create(fd, 4096);
+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
+	eb.buffers_ptr = to_user_pointer(&obj);
+	eb.buffer_count = 1;
+	gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
+
+	for (order = 12; order < 64; order++) {
+		obj.offset = 1ull << order;
+		if (__gem_execbuf(fd, &eb) != -EINVAL)
+			break;
+	}
+	igt_assert(obj.offset < gem_aperture_size(fd));
+
+	gem_close(fd, obj.handle);
+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
+	return order;
+}
+
 /**
  * gem_has_exec_fence:
  * @fd: open i915 drm file descriptor
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index 03211c97..c8d57a7c 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -138,6 +138,8 @@ uint64_t gem_aperture_size(int fd);
 uint64_t gem_global_aperture_size(int fd);
 uint64_t gem_mappable_aperture_size(void);
 bool gem_has_softpin(int fd);
+int gem_gtt_min_alignment_order(int fd);
+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
 bool gem_has_exec_fence(int fd);
 
 /* check functions which auto-skip tests by calling igt_skip() */
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH i-g-t v4 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment

[Janusz Krzysztofik]

The basic-range subtest assumes 4kB GTT alignment while calculating
softpin offsets.  On future backends with possibly larger minimum page
sizes the test will fail as a half of calculated offsets to be tested
will be incorrectly aligned.

Replace hardcoded constants corresponding to the assumed 4kB GTT
alignment with variables initialized with actual minimum GTT alignment
size and order.

v2: Simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Reword commit message after removal of patch "Don't filter out
    addresses on full PPGTT" from the series,
  - initialize page size order with an actual minimum returned by a new
    helper (inspired by Chris).
v4: Update the helper name, use the term 'minimum GTT alignment' across
    the change, adjust variable names,
  - refresh the commit message on top of the reintroduced patch that
    fixes invalid offsets incorrectly assumed as occupied.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index 423fed8b..8050cd3e 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -520,14 +520,16 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
+	int alignment_order = gem_gtt_min_alignment_order(fd);
+	uint64_t alignment = 1ull << alignment_order;
 	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
-	for (count = 12; gtt_size >> (count + 1); count++)
+	for (count = alignment_order; gtt_size >> (count + 1); count++)
 		;
 
-	count -= 12;
+	count -= alignment_order;
 
 	memset(obj, 0, sizeof(obj));
 	memset(reloc, 0, sizeof(reloc));
@@ -536,7 +538,7 @@ static void basic_range(int fd, unsigned flags)
 	n = 0;
 	for (int i = 0; i <= count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = (1ull << (i + 12)) - 4096;
+		obj[n].offset = (1ull << (i + alignment_order)) - alignment;
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
@@ -559,7 +561,7 @@ static void basic_range(int fd, unsigned flags)
 	}
 	for (int i = 1; i < count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = 1ull << (i + 12);
+		obj[n].offset = 1ull << (i + alignment_order);
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC PATCH i-g-t v4 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment

[Janusz Krzysztofik]

The basic-range subtest assumes 4kB GTT alignment while calculating
softpin offsets.  On future backends with possibly larger minimum page
sizes the test will fail as a half of calculated offsets to be tested
will be incorrectly aligned.

Replace hardcoded constants corresponding to the assumed 4kB GTT
alignment with variables initialized with actual minimum GTT alignment
size and order.

v2: Simplify the code by reversing the size->order conversion,
  - drop irrelevant modifications of requested object sizes.
v3: Reword commit message after removal of patch "Don't filter out
    addresses on full PPGTT" from the series,
  - initialize page size order with an actual minimum returned by a new
    helper (inspired by Chris).
v4: Update the helper name, use the term 'minimum GTT alignment' across
    the change, adjust variable names,
  - refresh the commit message on top of the reintroduced patch that
    fixes invalid offsets incorrectly assumed as occupied.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Stuart Summers <stuart.summers@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_reloc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index 423fed8b..8050cd3e 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -520,14 +520,16 @@ static void basic_range(int fd, unsigned flags)
 	uint64_t gtt_size = gem_aperture_size(fd);
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	igt_spin_t *spin = NULL;
+	int alignment_order = gem_gtt_min_alignment_order(fd);
+	uint64_t alignment = 1ull << alignment_order;
 	int count, n, err;
 
 	igt_require(gem_has_softpin(fd));
 
-	for (count = 12; gtt_size >> (count + 1); count++)
+	for (count = alignment_order; gtt_size >> (count + 1); count++)
 		;
 
-	count -= 12;
+	count -= alignment_order;
 
 	memset(obj, 0, sizeof(obj));
 	memset(reloc, 0, sizeof(reloc));
@@ -536,7 +538,7 @@ static void basic_range(int fd, unsigned flags)
 	n = 0;
 	for (int i = 0; i <= count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = (1ull << (i + 12)) - 4096;
+		obj[n].offset = (1ull << (i + alignment_order)) - alignment;
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
@@ -559,7 +561,7 @@ static void basic_range(int fd, unsigned flags)
 	}
 	for (int i = 1; i < count; i++) {
 		obj[n].handle = gem_create(fd, 4096);
-		obj[n].offset = 1ull << (i + 12);
+		obj[n].offset = 1ull << (i + alignment_order);
 		obj[n].offset = gen8_canonical_address(obj[n].offset);
 		obj[n].flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Janusz Krzysztofik]

exec-shared-gtt-* subtests use hardcoded values for object size and
softpin offset, based on 4kB GTT alignment assumption.  That may result
in those subtests failing when run on future backing stores with
possibly larger minimum page sizes.

Replace hardcoded constants with values calculated from minimum GTT
alignment of actual backing store the test is running on.

v2: Update helper name, use 'minimum GTT alignment' term across the
    change, adjust variable name.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_ctx_shared.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
index 6d8cbcce..1e9c7f78 100644
--- a/tests/i915/gem_ctx_shared.c
+++ b/tests/i915/gem_ctx_shared.c
@@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	uint32_t scratch, *s;
 	uint32_t batch, cs[16];
 	uint64_t offset;
+	uint64_t alignment;
 	int i;
 
 	gem_require_ring(i915, ring);
@@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
 
 	/* Find a hole big enough for both objects later */
-	scratch = gem_create(i915, 16384);
+	alignment = 2 * gem_gtt_min_alignment(i915);
+	scratch = gem_create(i915, 2 * alignment);
 	gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
 	obj.handle = scratch;
 	gem_execbuf(i915, &execbuf);
@@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	gem_write(i915, batch, 0, cs, sizeof(cs));
 
 	obj.handle = batch;
-	obj.offset += 8192; /* make sure we don't cause an eviction! */
+	obj.offset += alignment; /* make sure we don't cause an eviction! */
 	execbuf.rsvd1 = clone;
 	if (gen > 3 && gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Janusz Krzysztofik]

exec-shared-gtt-* subtests use hardcoded values for object size and
softpin offset, based on 4kB GTT alignment assumption.  That may result
in those subtests failing when run on future backing stores with
possibly larger minimum page sizes.

Replace hardcoded constants with values calculated from minimum GTT
alignment of actual backing store the test is running on.

v2: Update helper name, use 'minimum GTT alignment' term across the
    change, adjust variable name.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_ctx_shared.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
index 6d8cbcce..1e9c7f78 100644
--- a/tests/i915/gem_ctx_shared.c
+++ b/tests/i915/gem_ctx_shared.c
@@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	uint32_t scratch, *s;
 	uint32_t batch, cs[16];
 	uint64_t offset;
+	uint64_t alignment;
 	int i;
 
 	gem_require_ring(i915, ring);
@@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
 
 	/* Find a hole big enough for both objects later */
-	scratch = gem_create(i915, 16384);
+	alignment = 2 * gem_gtt_min_alignment(i915);
+	scratch = gem_create(i915, 2 * alignment);
 	gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
 	obj.handle = scratch;
 	gem_execbuf(i915, &execbuf);
@@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
 	gem_write(i915, batch, 0, cs, sizeof(cs));
 
 	obj.handle = batch;
-	obj.offset += 8192; /* make sure we don't cause an eviction! */
+	obj.offset += alignment; /* make sure we don't cause an eviction! */
 	execbuf.rsvd1 = clone;
 	if (gen > 3 && gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[igt-dev] ✓ Fi.CI.BAT: success for Calculate softpin offsets from minimum GTT alignment

[Patchwork]

== Series Details ==

Series: Calculate softpin offsets from minimum GTT alignment
URL   : https://patchwork.freedesktop.org/series/68831/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7231 -> IGTPW_3638
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/index.html

Known issues
------------

  Here are the changes found in IGTPW_3638 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@prime_busy@basic-wait-before-default:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/fi-icl-u3/igt@prime_busy@basic-wait-before-default.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/fi-icl-u3/igt@prime_busy@basic-wait-before-default.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-icl-u3:          [DMESG-WARN][3] ([fdo#107724]) -> [PASS][4] +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/fi-icl-u3/igt@gem_exec_suspend@basic-s4-devices.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/fi-icl-u3/igt@gem_exec_suspend@basic-s4-devices.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569


Participating hosts (52 -> 44)
------------------------------

  Missing    (8): fi-ilk-m540 fi-hsw-4200u fi-tgl-u2 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_5255 -> IGTPW_3638

  CI-20190529: 20190529
  CI_DRM_7231: 9c304eaf3dae009ccb96d56eca58a81837303fc6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3638: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/index.html
  IGT_5255: b21b6a7aaa0db2159f22ee4427804e5a16fe2261 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Vanshidhar Konda]

On Thu, Oct 31, 2019 at 04:28:55PM +0100, Janusz Krzysztofik wrote:
>Some tests assume 4kB offset alignment while using softpin.  That
>assumption may be wrong on future GEM backends with possibly larger
>minimum page sizes.  As a result, those tests may either fail on
>softpin at offsets which are incorrectly aligned, may silently skip
>such incorrectly aligned addresses assuming them occupied by other
>users if incorrect detection method is used, or may always succeed
>when examining invalid use patterns.
>
>Provide a helper function that detects minimum GTT alignment.  Tests
>may use it to calculate softpin offsets valid for actually used backing
>store.
>
>v2: Rename the helper, use 'minimum GTT alignment' term across the
>    change (Chris),
>  - use error numbers to distinguish between invalid offsets and
>    addresses occupied by other users, then
>  - simplify the code (Chris).
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>Cc: Stuart Summers <stuart.summers@intel.com>
>---
> lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> lib/ioctl_wrappers.h |  2 ++
> 2 files changed, 48 insertions(+)
>
>diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
>index 628f8b83..f0ef8b2e 100644
>--- a/lib/ioctl_wrappers.c
>+++ b/lib/ioctl_wrappers.c
>@@ -54,6 +54,7 @@
> #include "intel_io.h"
> #include "igt_debugfs.h"
> #include "igt_sysfs.h"
>+#include "igt_x86.h"
> #include "config.h"
>
> #ifdef HAVE_VALGRIND
>@@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> 	return has_softpin;
> }
>
>+/**
>+ * gem_gtt_min_alignment_order:
>+ * @fd: open i915 drm file descriptor
>+ *
>+ * This function detects the minimum possible alignment of a soft-pinned gem
>+ * object allocated from a default backing store.  It is useful for calculating
>+ * correctly aligned softpin offsets.
>+ * Since size order to size conversion (size = 1 << order) is less trivial
>+ * than the opposite, the function returns the alignment order as more handy.
>+ *
>+ * Returns:
>+ * Size order of the minimum GTT alignment
>+ */
>+int gem_gtt_min_alignment_order(int fd)
>+{
>+	struct drm_i915_gem_exec_object2 obj;
>+	struct drm_i915_gem_execbuffer2 eb;
>+	const uint32_t bbe = MI_BATCH_BUFFER_END;
>+	int order;
>+
>+	/* no softpin => 4kB page size */
>+	if (!gem_has_softpin(fd))
>+		return 12;
>+
>+	memset(&obj, 0, sizeof(obj));
>+	memset(&eb, 0, sizeof(eb));
>+
>+	obj.handle = gem_create(fd, 4096);
>+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>+	eb.buffers_ptr = to_user_pointer(&obj);
>+	eb.buffer_count = 1;
>+	gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));

I think it will be safer to create a new context to execute this
execbuffer. For a new context the address space should be empty reducing
the chance that there is another object mapped by the caller of the
helper function at the address we start testing.

Otherwise it looks good to me.

Vanshi

>+
>+	for (order = 12; order < 64; order++) {
>+		obj.offset = 1ull << order;
>+		if (__gem_execbuf(fd, &eb) != -EINVAL)
>+			break;
>+	}
>+	igt_assert(obj.offset < gem_aperture_size(fd));
>+
>+	gem_close(fd, obj.handle);
>+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
>+	return order;
>+}
>+
> /**
>  * gem_has_exec_fence:
>  * @fd: open i915 drm file descriptor
>diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
>index 03211c97..c8d57a7c 100644
>--- a/lib/ioctl_wrappers.h
>+++ b/lib/ioctl_wrappers.h
>@@ -138,6 +138,8 @@ uint64_t gem_aperture_size(int fd);
> uint64_t gem_global_aperture_size(int fd);
> uint64_t gem_mappable_aperture_size(void);
> bool gem_has_softpin(int fd);
>+int gem_gtt_min_alignment_order(int fd);
>+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
> bool gem_has_exec_fence(int fd);
>
> /* check functions which auto-skip tests by calling igt_skip() */
>-- 
>2.21.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Vanshidhar Konda]

On Thu, Oct 31, 2019 at 04:28:55PM +0100, Janusz Krzysztofik wrote:
>Some tests assume 4kB offset alignment while using softpin.  That
>assumption may be wrong on future GEM backends with possibly larger
>minimum page sizes.  As a result, those tests may either fail on
>softpin at offsets which are incorrectly aligned, may silently skip
>such incorrectly aligned addresses assuming them occupied by other
>users if incorrect detection method is used, or may always succeed
>when examining invalid use patterns.
>
>Provide a helper function that detects minimum GTT alignment.  Tests
>may use it to calculate softpin offsets valid for actually used backing
>store.
>
>v2: Rename the helper, use 'minimum GTT alignment' term across the
>    change (Chris),
>  - use error numbers to distinguish between invalid offsets and
>    addresses occupied by other users, then
>  - simplify the code (Chris).
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>Cc: Stuart Summers <stuart.summers@intel.com>
>---
> lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> lib/ioctl_wrappers.h |  2 ++
> 2 files changed, 48 insertions(+)
>
>diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
>index 628f8b83..f0ef8b2e 100644
>--- a/lib/ioctl_wrappers.c
>+++ b/lib/ioctl_wrappers.c
>@@ -54,6 +54,7 @@
> #include "intel_io.h"
> #include "igt_debugfs.h"
> #include "igt_sysfs.h"
>+#include "igt_x86.h"
> #include "config.h"
>
> #ifdef HAVE_VALGRIND
>@@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> 	return has_softpin;
> }
>
>+/**
>+ * gem_gtt_min_alignment_order:
>+ * @fd: open i915 drm file descriptor
>+ *
>+ * This function detects the minimum possible alignment of a soft-pinned gem
>+ * object allocated from a default backing store.  It is useful for calculating
>+ * correctly aligned softpin offsets.
>+ * Since size order to size conversion (size = 1 << order) is less trivial
>+ * than the opposite, the function returns the alignment order as more handy.
>+ *
>+ * Returns:
>+ * Size order of the minimum GTT alignment
>+ */
>+int gem_gtt_min_alignment_order(int fd)
>+{
>+	struct drm_i915_gem_exec_object2 obj;
>+	struct drm_i915_gem_execbuffer2 eb;
>+	const uint32_t bbe = MI_BATCH_BUFFER_END;
>+	int order;
>+
>+	/* no softpin => 4kB page size */
>+	if (!gem_has_softpin(fd))
>+		return 12;
>+
>+	memset(&obj, 0, sizeof(obj));
>+	memset(&eb, 0, sizeof(eb));
>+
>+	obj.handle = gem_create(fd, 4096);
>+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>+	eb.buffers_ptr = to_user_pointer(&obj);
>+	eb.buffer_count = 1;
>+	gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));

I think it will be safer to create a new context to execute this
execbuffer. For a new context the address space should be empty reducing
the chance that there is another object mapped by the caller of the
helper function at the address we start testing.

Otherwise it looks good to me.

Vanshi

>+
>+	for (order = 12; order < 64; order++) {
>+		obj.offset = 1ull << order;
>+		if (__gem_execbuf(fd, &eb) != -EINVAL)
>+			break;
>+	}
>+	igt_assert(obj.offset < gem_aperture_size(fd));
>+
>+	gem_close(fd, obj.handle);
>+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
>+	return order;
>+}
>+
> /**
>  * gem_has_exec_fence:
>  * @fd: open i915 drm file descriptor
>diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
>index 03211c97..c8d57a7c 100644
>--- a/lib/ioctl_wrappers.h
>+++ b/lib/ioctl_wrappers.h
>@@ -138,6 +138,8 @@ uint64_t gem_aperture_size(int fd);
> uint64_t gem_global_aperture_size(int fd);
> uint64_t gem_mappable_aperture_size(void);
> bool gem_has_softpin(int fd);
>+int gem_gtt_min_alignment_order(int fd);
>+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
> bool gem_has_exec_fence(int fd);
>
> /* check functions which auto-skip tests by calling igt_skip() */
>-- 
>2.21.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Vanshidhar Konda]

May be this patch can just be merged with the other patch in this series
that changes gem_exec_reloc.

Vanshi

On Thu, Oct 31, 2019 at 04:28:54PM +0100, Janusz Krzysztofik wrote:
>Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
>addresses for !ppgtt") introduced filtering of addresses possibly
>occupied by other users of shared GTT.  Unfortunately, that filtering
>doesn't distinguish between actually occupied addresses and otherwise
>invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
>when running on future backends with possibly larger minimum page
>sizes, a half of calculated offsets to be tested will be incorrectly
>detected as occupied by other users and silently skipped instead of
>reported as a problem.  That will significantly distort the intended
>test pattern.
>
>Filter out failing addresses only if not reported as invalid.
>
>v2: Skip unavailable addresses only when not running on full PPGTT.
>v3: Replace the not on full PPGTT requirement for skipping with error
>    code checking.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_exec_reloc.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>index 5f59fe99..423fed8b 100644
>--- a/tests/i915/gem_exec_reloc.c
>+++ b/tests/i915/gem_exec_reloc.c
>@@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> 	uint64_t gtt_size = gem_aperture_size(fd);
> 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> 	igt_spin_t *spin = NULL;
>-	int count, n;
>+	int count, n, err;
>
> 	igt_require(gem_has_softpin(fd));
>
>@@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> 		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = __gem_execbuf(fd, &execbuf);
>+		if (err) {
>+			igt_assert(err != -EINVAL);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>@@ -562,8 +565,11 @@ static void basic_range(int fd, unsigned flags)
> 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> 		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = __gem_execbuf(fd, &execbuf);
>+		if (err) {
>+			igt_assert(err != -EINVAL);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>-- 
>2.21.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Vanshidhar Konda]

May be this patch can just be merged with the other patch in this series
that changes gem_exec_reloc.

Vanshi

On Thu, Oct 31, 2019 at 04:28:54PM +0100, Janusz Krzysztofik wrote:
>Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
>addresses for !ppgtt") introduced filtering of addresses possibly
>occupied by other users of shared GTT.  Unfortunately, that filtering
>doesn't distinguish between actually occupied addresses and otherwise
>invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
>when running on future backends with possibly larger minimum page
>sizes, a half of calculated offsets to be tested will be incorrectly
>detected as occupied by other users and silently skipped instead of
>reported as a problem.  That will significantly distort the intended
>test pattern.
>
>Filter out failing addresses only if not reported as invalid.
>
>v2: Skip unavailable addresses only when not running on full PPGTT.
>v3: Replace the not on full PPGTT requirement for skipping with error
>    code checking.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_exec_reloc.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>index 5f59fe99..423fed8b 100644
>--- a/tests/i915/gem_exec_reloc.c
>+++ b/tests/i915/gem_exec_reloc.c
>@@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> 	uint64_t gtt_size = gem_aperture_size(fd);
> 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> 	igt_spin_t *spin = NULL;
>-	int count, n;
>+	int count, n, err;
>
> 	igt_require(gem_has_softpin(fd));
>
>@@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> 		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = __gem_execbuf(fd, &execbuf);
>+		if (err) {
>+			igt_assert(err != -EINVAL);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>@@ -562,8 +565,11 @@ static void basic_range(int fd, unsigned flags)
> 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> 		execbuf.buffer_count = 1;
>-		if (__gem_execbuf(fd, &execbuf))
>+		err = __gem_execbuf(fd, &execbuf);
>+		if (err) {
>+			igt_assert(err != -EINVAL);
> 			continue;
>+		}
>
> 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> 			  n, obj[n].handle, (long long)obj[n].offset);
>-- 
>2.21.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Vanshidhar Konda]

On Thu, Oct 31, 2019 at 04:28:57PM +0100, Janusz Krzysztofik wrote:
>exec-shared-gtt-* subtests use hardcoded values for object size and
>softpin offset, based on 4kB GTT alignment assumption.  That may result
>in those subtests failing when run on future backing stores with
>possibly larger minimum page sizes.
>
>Replace hardcoded constants with values calculated from minimum GTT
>alignment of actual backing store the test is running on.
>
>v2: Update helper name, use 'minimum GTT alignment' term across the
>    change, adjust variable name.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_ctx_shared.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
>index 6d8cbcce..1e9c7f78 100644
>--- a/tests/i915/gem_ctx_shared.c
>+++ b/tests/i915/gem_ctx_shared.c
>@@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	uint32_t scratch, *s;
> 	uint32_t batch, cs[16];
> 	uint64_t offset;
>+	uint64_t alignment;
> 	int i;
>
> 	gem_require_ring(i915, ring);
>@@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
>
> 	/* Find a hole big enough for both objects later */
>-	scratch = gem_create(i915, 16384);
>+	alignment = 2 * gem_gtt_min_alignment(i915);
>+	scratch = gem_create(i915, 2 * alignment);
> 	gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> 	obj.handle = scratch;
> 	gem_execbuf(i915, &execbuf);
>@@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	gem_write(i915, batch, 0, cs, sizeof(cs));
>
> 	obj.handle = batch;
>-	obj.offset += 8192; /* make sure we don't cause an eviction! */
>+	obj.offset += alignment; /* make sure we don't cause an eviction! */
> 	execbuf.rsvd1 = clone;
> 	if (gen > 3 && gen < 6)
> 		execbuf.flags |= I915_EXEC_SECURE;
>-- 
>2.21.0

Looks good to me.

Vanshi
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Vanshidhar Konda]

On Thu, Oct 31, 2019 at 04:28:57PM +0100, Janusz Krzysztofik wrote:
>exec-shared-gtt-* subtests use hardcoded values for object size and
>softpin offset, based on 4kB GTT alignment assumption.  That may result
>in those subtests failing when run on future backing stores with
>possibly larger minimum page sizes.
>
>Replace hardcoded constants with values calculated from minimum GTT
>alignment of actual backing store the test is running on.
>
>v2: Update helper name, use 'minimum GTT alignment' term across the
>    change, adjust variable name.
>
>Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>---
> tests/i915/gem_ctx_shared.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
>index 6d8cbcce..1e9c7f78 100644
>--- a/tests/i915/gem_ctx_shared.c
>+++ b/tests/i915/gem_ctx_shared.c
>@@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	uint32_t scratch, *s;
> 	uint32_t batch, cs[16];
> 	uint64_t offset;
>+	uint64_t alignment;
> 	int i;
>
> 	gem_require_ring(i915, ring);
>@@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
>
> 	/* Find a hole big enough for both objects later */
>-	scratch = gem_create(i915, 16384);
>+	alignment = 2 * gem_gtt_min_alignment(i915);
>+	scratch = gem_create(i915, 2 * alignment);
> 	gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> 	obj.handle = scratch;
> 	gem_execbuf(i915, &execbuf);
>@@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> 	gem_write(i915, batch, 0, cs, sizeof(cs));
>
> 	obj.handle = batch;
>-	obj.offset += 8192; /* make sure we don't cause an eviction! */
>+	obj.offset += alignment; /* make sure we don't cause an eviction! */
> 	execbuf.rsvd1 = clone;
> 	if (gen > 3 && gen < 6)
> 		execbuf.flags |= I915_EXEC_SECURE;
>-- 
>2.21.0

Looks good to me.

Vanshi
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> exec-shared-gtt-* subtests use hardcoded values for object size and
> softpin offset, based on 4kB GTT alignment assumption.  That may result
> in those subtests failing when run on future backing stores with
> possibly larger minimum page sizes.
> 
> Replace hardcoded constants with values calculated from minimum GTT
> alignment of actual backing store the test is running on.
> 
> v2: Update helper name, use 'minimum GTT alignment' term across the
>     change, adjust variable name.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/i915/gem_ctx_shared.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> index 6d8cbcce..1e9c7f78 100644
> --- a/tests/i915/gem_ctx_shared.c
> +++ b/tests/i915/gem_ctx_shared.c
> @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
>         uint32_t scratch, *s;
>         uint32_t batch, cs[16];
>         uint64_t offset;
> +       uint64_t alignment;
>         int i;
>  
>         gem_require_ring(i915, ring);
> @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
>         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
>  
>         /* Find a hole big enough for both objects later */
> -       scratch = gem_create(i915, 16384);
> +       alignment = 2 * gem_gtt_min_alignment(i915);
> +       scratch = gem_create(i915, 2 * alignment);
>         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
>         obj.handle = scratch;
>         gem_execbuf(i915, &execbuf);
> @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
>         gem_write(i915, batch, 0, cs, sizeof(cs));
>  
>         obj.handle = batch;
> -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> +       obj.offset += alignment; /* make sure we don't cause an eviction! */

It's 'stride' here. It's leaving a guard page in between, just in case
page coloring demands it.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> exec-shared-gtt-* subtests use hardcoded values for object size and
> softpin offset, based on 4kB GTT alignment assumption.  That may result
> in those subtests failing when run on future backing stores with
> possibly larger minimum page sizes.
> 
> Replace hardcoded constants with values calculated from minimum GTT
> alignment of actual backing store the test is running on.
> 
> v2: Update helper name, use 'minimum GTT alignment' term across the
>     change, adjust variable name.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/i915/gem_ctx_shared.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> index 6d8cbcce..1e9c7f78 100644
> --- a/tests/i915/gem_ctx_shared.c
> +++ b/tests/i915/gem_ctx_shared.c
> @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
>         uint32_t scratch, *s;
>         uint32_t batch, cs[16];
>         uint64_t offset;
> +       uint64_t alignment;
>         int i;
>  
>         gem_require_ring(i915, ring);
> @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
>         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
>  
>         /* Find a hole big enough for both objects later */
> -       scratch = gem_create(i915, 16384);
> +       alignment = 2 * gem_gtt_min_alignment(i915);
> +       scratch = gem_create(i915, 2 * alignment);
>         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
>         obj.handle = scratch;
>         gem_execbuf(i915, &execbuf);
> @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
>         gem_write(i915, batch, 0, cs, sizeof(cs));
>  
>         obj.handle = batch;
> -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> +       obj.offset += alignment; /* make sure we don't cause an eviction! */

It's 'stride' here. It's leaving a guard page in between, just in case
page coloring demands it.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:54)
> Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> addresses for !ppgtt") introduced filtering of addresses possibly
> occupied by other users of shared GTT.  Unfortunately, that filtering
> doesn't distinguish between actually occupied addresses and otherwise
> invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> when running on future backends with possibly larger minimum page
> sizes, a half of calculated offsets to be tested will be incorrectly
> detected as occupied by other users and silently skipped instead of
> reported as a problem.  That will significantly distort the intended
> test pattern.
> 
> Filter out failing addresses only if not reported as invalid.
> 
> v2: Skip unavailable addresses only when not running on full PPGTT.
> v3: Replace the not on full PPGTT requirement for skipping with error
>     code checking.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/i915/gem_exec_reloc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> index 5f59fe99..423fed8b 100644
> --- a/tests/i915/gem_exec_reloc.c
> +++ b/tests/i915/gem_exec_reloc.c
> @@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
>         uint64_t gtt_size = gem_aperture_size(fd);
>         const uint32_t bbe = MI_BATCH_BUFFER_END;
>         igt_spin_t *spin = NULL;
> -       int count, n;
> +       int count, n, err;
>  
>         igt_require(gem_has_softpin(fd));
>  
> @@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
>                 gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
>                 execbuf.buffers_ptr = to_user_pointer(&obj[n]);
>                 execbuf.buffer_count = 1;
> -               if (__gem_execbuf(fd, &execbuf))
> +               err = __gem_execbuf(fd, &execbuf);
> +               if (err) {

> +                       igt_assert(err != -EINVAL);

I've been thinking about this and I think the right approach is

/* Iff using a shared GTT, some of it may be reserved */
igt_assert_eq(err, -ENOSPC);

>                         continue;
> +               }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:54)
> Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> addresses for !ppgtt") introduced filtering of addresses possibly
> occupied by other users of shared GTT.  Unfortunately, that filtering
> doesn't distinguish between actually occupied addresses and otherwise
> invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> when running on future backends with possibly larger minimum page
> sizes, a half of calculated offsets to be tested will be incorrectly
> detected as occupied by other users and silently skipped instead of
> reported as a problem.  That will significantly distort the intended
> test pattern.
> 
> Filter out failing addresses only if not reported as invalid.
> 
> v2: Skip unavailable addresses only when not running on full PPGTT.
> v3: Replace the not on full PPGTT requirement for skipping with error
>     code checking.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/i915/gem_exec_reloc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> index 5f59fe99..423fed8b 100644
> --- a/tests/i915/gem_exec_reloc.c
> +++ b/tests/i915/gem_exec_reloc.c
> @@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
>         uint64_t gtt_size = gem_aperture_size(fd);
>         const uint32_t bbe = MI_BATCH_BUFFER_END;
>         igt_spin_t *spin = NULL;
> -       int count, n;
> +       int count, n, err;
>  
>         igt_require(gem_has_softpin(fd));
>  
> @@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
>                 gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
>                 execbuf.buffers_ptr = to_user_pointer(&obj[n]);
>                 execbuf.buffer_count = 1;
> -               if (__gem_execbuf(fd, &execbuf))
> +               err = __gem_execbuf(fd, &execbuf);
> +               if (err) {

> +                       igt_assert(err != -EINVAL);

I've been thinking about this and I think the right approach is

/* Iff using a shared GTT, some of it may be reserved */
igt_assert_eq(err, -ENOSPC);

>                         continue;
> +               }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:54)
> Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> addresses for !ppgtt") introduced filtering of addresses possibly
> occupied by other users of shared GTT.  Unfortunately, that filtering
> doesn't distinguish between actually occupied addresses and otherwise
> invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> when running on future backends with possibly larger minimum page
> sizes, a half of calculated offsets to be tested will be incorrectly
> detected as occupied by other users and silently skipped instead of
> reported as a problem.  That will significantly distort the intended
> test pattern.
> 
> Filter out failing addresses only if not reported as invalid.
> 
> v2: Skip unavailable addresses only when not running on full PPGTT.
> v3: Replace the not on full PPGTT requirement for skipping with error
>     code checking.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/i915/gem_exec_reloc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> index 5f59fe99..423fed8b 100644
> --- a/tests/i915/gem_exec_reloc.c
> +++ b/tests/i915/gem_exec_reloc.c
> @@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
>         uint64_t gtt_size = gem_aperture_size(fd);
>         const uint32_t bbe = MI_BATCH_BUFFER_END;
>         igt_spin_t *spin = NULL;
> -       int count, n;
> +       int count, n, err;
>  
>         igt_require(gem_has_softpin(fd));
>  
> @@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
>                 gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
>                 execbuf.buffers_ptr = to_user_pointer(&obj[n]);
>                 execbuf.buffer_count = 1;
> -               if (__gem_execbuf(fd, &execbuf))
> +               err = __gem_execbuf(fd, &execbuf);
> +               if (err) {

> +                       igt_assert(err != -EINVAL);

I've been thinking about this and I think the right approach is

/* Iff using a shared GTT, some of it may be reserved */
igt_assert_eq(err, -ENOSPC);

>                         continue;
> +               }
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> Some tests assume 4kB offset alignment while using softpin.  That
> assumption may be wrong on future GEM backends with possibly larger
> minimum page sizes.  As a result, those tests may either fail on
> softpin at offsets which are incorrectly aligned, may silently skip
> such incorrectly aligned addresses assuming them occupied by other
> users if incorrect detection method is used, or may always succeed
> when examining invalid use patterns.
> 
> Provide a helper function that detects minimum GTT alignment.  Tests
> may use it to calculate softpin offsets valid for actually used backing
> store.
> 
> v2: Rename the helper, use 'minimum GTT alignment' term across the
>     change (Chris),
>   - use error numbers to distinguish between invalid offsets and
>     addresses occupied by other users, then
>   - simplify the code (Chris).
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> ---
>  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/ioctl_wrappers.h |  2 ++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 628f8b83..f0ef8b2e 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -54,6 +54,7 @@
>  #include "intel_io.h"
>  #include "igt_debugfs.h"
>  #include "igt_sysfs.h"
> +#include "igt_x86.h"
>  #include "config.h"
>  
>  #ifdef HAVE_VALGRIND
> @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
>         return has_softpin;
>  }
>  
> +/**
> + * gem_gtt_min_alignment_order:
> + * @fd: open i915 drm file descriptor
> + *
> + * This function detects the minimum possible alignment of a soft-pinned gem
> + * object allocated from a default backing store.  It is useful for calculating
> + * correctly aligned softpin offsets.
> + * Since size order to size conversion (size = 1 << order) is less trivial
> + * than the opposite, the function returns the alignment order as more handy.
> + *
> + * Returns:
> + * Size order of the minimum GTT alignment
> + */
> +int gem_gtt_min_alignment_order(int fd)

But not part of ioctl_wrappers.c!

lib/i915/gem_gtt_topology.c? _query.c? _probe.c?

> +{
> +       struct drm_i915_gem_exec_object2 obj;
> +       struct drm_i915_gem_execbuffer2 eb;
> +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> +       int order;
> +
> +       /* no softpin => 4kB page size */
> +       if (!gem_has_softpin(fd))
> +               return 12;
> +
> +       memset(&obj, 0, sizeof(obj));
> +       memset(&eb, 0, sizeof(eb));
> +
> +       obj.handle = gem_create(fd, 4096);
> +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> +       eb.buffers_ptr = to_user_pointer(&obj);
> +       eb.buffer_count = 1;
> +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> +
> +       for (order = 12; order < 64; order++) {
> +               obj.offset = 1ull << order;
> +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> +                       break;
> +       }
> +       igt_assert(obj.offset < gem_aperture_size(fd));

Hmm, there is one more trick we can apply here: an invalid reloc.

memset(&reloc, 0, sizeof(reloc));
obj.relocs_ptr = to_user_pointer(&reloc);
obj.relocation_count = 1;

We first process the buffers, then we process the relocations. So we
will get the -EINVAL for illegal softpin before we get the -ENOENT for
invalid reloc handle.

That just saves actually emitting a request.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> Some tests assume 4kB offset alignment while using softpin.  That
> assumption may be wrong on future GEM backends with possibly larger
> minimum page sizes.  As a result, those tests may either fail on
> softpin at offsets which are incorrectly aligned, may silently skip
> such incorrectly aligned addresses assuming them occupied by other
> users if incorrect detection method is used, or may always succeed
> when examining invalid use patterns.
> 
> Provide a helper function that detects minimum GTT alignment.  Tests
> may use it to calculate softpin offsets valid for actually used backing
> store.
> 
> v2: Rename the helper, use 'minimum GTT alignment' term across the
>     change (Chris),
>   - use error numbers to distinguish between invalid offsets and
>     addresses occupied by other users, then
>   - simplify the code (Chris).
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> ---
>  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/ioctl_wrappers.h |  2 ++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 628f8b83..f0ef8b2e 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -54,6 +54,7 @@
>  #include "intel_io.h"
>  #include "igt_debugfs.h"
>  #include "igt_sysfs.h"
> +#include "igt_x86.h"
>  #include "config.h"
>  
>  #ifdef HAVE_VALGRIND
> @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
>         return has_softpin;
>  }
>  
> +/**
> + * gem_gtt_min_alignment_order:
> + * @fd: open i915 drm file descriptor
> + *
> + * This function detects the minimum possible alignment of a soft-pinned gem
> + * object allocated from a default backing store.  It is useful for calculating
> + * correctly aligned softpin offsets.
> + * Since size order to size conversion (size = 1 << order) is less trivial
> + * than the opposite, the function returns the alignment order as more handy.
> + *
> + * Returns:
> + * Size order of the minimum GTT alignment
> + */
> +int gem_gtt_min_alignment_order(int fd)

But not part of ioctl_wrappers.c!

lib/i915/gem_gtt_topology.c? _query.c? _probe.c?

> +{
> +       struct drm_i915_gem_exec_object2 obj;
> +       struct drm_i915_gem_execbuffer2 eb;
> +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> +       int order;
> +
> +       /* no softpin => 4kB page size */
> +       if (!gem_has_softpin(fd))
> +               return 12;
> +
> +       memset(&obj, 0, sizeof(obj));
> +       memset(&eb, 0, sizeof(eb));
> +
> +       obj.handle = gem_create(fd, 4096);
> +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> +       eb.buffers_ptr = to_user_pointer(&obj);
> +       eb.buffer_count = 1;
> +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> +
> +       for (order = 12; order < 64; order++) {
> +               obj.offset = 1ull << order;
> +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> +                       break;
> +       }
> +       igt_assert(obj.offset < gem_aperture_size(fd));

Hmm, there is one more trick we can apply here: an invalid reloc.

memset(&reloc, 0, sizeof(reloc));
obj.relocs_ptr = to_user_pointer(&reloc);
obj.relocation_count = 1;

We first process the buffers, then we process the relocations. So we
will get the -EINVAL for illegal softpin before we get the -ENOENT for
invalid reloc handle.

That just saves actually emitting a request.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> Some tests assume 4kB offset alignment while using softpin.  That
> assumption may be wrong on future GEM backends with possibly larger
> minimum page sizes.  As a result, those tests may either fail on
> softpin at offsets which are incorrectly aligned, may silently skip
> such incorrectly aligned addresses assuming them occupied by other
> users if incorrect detection method is used, or may always succeed
> when examining invalid use patterns.
> 
> Provide a helper function that detects minimum GTT alignment.  Tests
> may use it to calculate softpin offsets valid for actually used backing
> store.
> 
> v2: Rename the helper, use 'minimum GTT alignment' term across the
>     change (Chris),
>   - use error numbers to distinguish between invalid offsets and
>     addresses occupied by other users, then
>   - simplify the code (Chris).
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> ---
>  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/ioctl_wrappers.h |  2 ++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 628f8b83..f0ef8b2e 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -54,6 +54,7 @@
>  #include "intel_io.h"
>  #include "igt_debugfs.h"
>  #include "igt_sysfs.h"
> +#include "igt_x86.h"
>  #include "config.h"
>  
>  #ifdef HAVE_VALGRIND
> @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
>         return has_softpin;
>  }
>  
> +/**
> + * gem_gtt_min_alignment_order:
> + * @fd: open i915 drm file descriptor
> + *
> + * This function detects the minimum possible alignment of a soft-pinned gem
> + * object allocated from a default backing store.  It is useful for calculating
> + * correctly aligned softpin offsets.
> + * Since size order to size conversion (size = 1 << order) is less trivial
> + * than the opposite, the function returns the alignment order as more handy.
> + *
> + * Returns:
> + * Size order of the minimum GTT alignment
> + */
> +int gem_gtt_min_alignment_order(int fd)

But not part of ioctl_wrappers.c!

lib/i915/gem_gtt_topology.c? _query.c? _probe.c?

> +{
> +       struct drm_i915_gem_exec_object2 obj;
> +       struct drm_i915_gem_execbuffer2 eb;
> +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> +       int order;
> +
> +       /* no softpin => 4kB page size */
> +       if (!gem_has_softpin(fd))
> +               return 12;
> +
> +       memset(&obj, 0, sizeof(obj));
> +       memset(&eb, 0, sizeof(eb));
> +
> +       obj.handle = gem_create(fd, 4096);
> +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> +       eb.buffers_ptr = to_user_pointer(&obj);
> +       eb.buffer_count = 1;
> +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> +
> +       for (order = 12; order < 64; order++) {
> +               obj.offset = 1ull << order;
> +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> +                       break;
> +       }
> +       igt_assert(obj.offset < gem_aperture_size(fd));

Hmm, there is one more trick we can apply here: an invalid reloc.

memset(&reloc, 0, sizeof(reloc));
obj.relocs_ptr = to_user_pointer(&reloc);
obj.relocation_count = 1;

We first process the buffers, then we process the relocations. So we
will get the -EINVAL for illegal softpin before we get the -ENOENT for
invalid reloc handle.

That just saves actually emitting a request.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [RFC PATCH i-g-t v4 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:56)
> The basic-range subtest assumes 4kB GTT alignment while calculating
> softpin offsets.  On future backends with possibly larger minimum page
> sizes the test will fail as a half of calculated offsets to be tested
> will be incorrectly aligned.
> 
> Replace hardcoded constants corresponding to the assumed 4kB GTT
> alignment with variables initialized with actual minimum GTT alignment
> size and order.
> 
> v2: Simplify the code by reversing the size->order conversion,
>   - drop irrelevant modifications of requested object sizes.
> v3: Reword commit message after removal of patch "Don't filter out
>     addresses on full PPGTT" from the series,
>   - initialize page size order with an actual minimum returned by a new
>     helper (inspired by Chris).
> v4: Update the helper name, use the term 'minimum GTT alignment' across
>     the change, adjust variable names,
>   - refresh the commit message on top of the reintroduced patch that
>     fixes invalid offsets incorrectly assumed as occupied.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:56)
> The basic-range subtest assumes 4kB GTT alignment while calculating
> softpin offsets.  On future backends with possibly larger minimum page
> sizes the test will fail as a half of calculated offsets to be tested
> will be incorrectly aligned.
> 
> Replace hardcoded constants corresponding to the assumed 4kB GTT
> alignment with variables initialized with actual minimum GTT alignment
> size and order.
> 
> v2: Simplify the code by reversing the size->order conversion,
>   - drop irrelevant modifications of requested object sizes.
> v3: Reword commit message after removal of patch "Don't filter out
>     addresses on full PPGTT" from the series,
>   - initialize page size order with an actual minimum returned by a new
>     helper (inspired by Chris).
> v4: Update the helper name, use the term 'minimum GTT alignment' across
>     the change, adjust variable names,
>   - refresh the commit message on top of the reintroduced patch that
>     fixes invalid offsets incorrectly assumed as occupied.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [RFC PATCH i-g-t v4 3/4] tests/gem_exec_reloc: Calculate offsets from minimum GTT alignment

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-10-31 15:28:56)
> The basic-range subtest assumes 4kB GTT alignment while calculating
> softpin offsets.  On future backends with possibly larger minimum page
> sizes the test will fail as a half of calculated offsets to be tested
> will be incorrectly aligned.
> 
> Replace hardcoded constants corresponding to the assumed 4kB GTT
> alignment with variables initialized with actual minimum GTT alignment
> size and order.
> 
> v2: Simplify the code by reversing the size->order conversion,
>   - drop irrelevant modifications of requested object sizes.
> v3: Reword commit message after removal of patch "Don't filter out
>     addresses on full PPGTT" from the series,
>   - initialize page size order with an actual minimum returned by a new
>     helper (inspired by Chris).
> v4: Update the helper name, use the term 'minimum GTT alignment' across
>     the change, adjust variable names,
>   - refresh the commit message on top of the reintroduced patch that
>     fixes invalid offsets incorrectly assumed as occupied.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Chris Wilson]

Quoting Chris Wilson (2019-11-01 09:42:18)
> Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> > exec-shared-gtt-* subtests use hardcoded values for object size and
> > softpin offset, based on 4kB GTT alignment assumption.  That may result
> > in those subtests failing when run on future backing stores with
> > possibly larger minimum page sizes.
> > 
> > Replace hardcoded constants with values calculated from minimum GTT
> > alignment of actual backing store the test is running on.
> > 
> > v2: Update helper name, use 'minimum GTT alignment' term across the
> >     change, adjust variable name.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/i915/gem_ctx_shared.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > index 6d8cbcce..1e9c7f78 100644
> > --- a/tests/i915/gem_ctx_shared.c
> > +++ b/tests/i915/gem_ctx_shared.c
> > @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> >         uint32_t scratch, *s;
> >         uint32_t batch, cs[16];
> >         uint64_t offset;
> > +       uint64_t alignment;
> >         int i;
> >  
> >         gem_require_ring(i915, ring);
> > @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> >         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
> >  
> >         /* Find a hole big enough for both objects later */
> > -       scratch = gem_create(i915, 16384);
> > +       alignment = 2 * gem_gtt_min_alignment(i915);
> > +       scratch = gem_create(i915, 2 * alignment);
> >         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> >         obj.handle = scratch;
> >         gem_execbuf(i915, &execbuf);
> > @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> >         gem_write(i915, batch, 0, cs, sizeof(cs));
> >  
> >         obj.handle = batch;
> > -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> > +       obj.offset += alignment; /* make sure we don't cause an eviction! */
> 
> It's 'stride' here. It's leaving a guard page in between, just in case
> page coloring demands it.

Other than that,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Chris Wilson]

Quoting Chris Wilson (2019-11-01 09:42:18)
> Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> > exec-shared-gtt-* subtests use hardcoded values for object size and
> > softpin offset, based on 4kB GTT alignment assumption.  That may result
> > in those subtests failing when run on future backing stores with
> > possibly larger minimum page sizes.
> > 
> > Replace hardcoded constants with values calculated from minimum GTT
> > alignment of actual backing store the test is running on.
> > 
> > v2: Update helper name, use 'minimum GTT alignment' term across the
> >     change, adjust variable name.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/i915/gem_ctx_shared.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > index 6d8cbcce..1e9c7f78 100644
> > --- a/tests/i915/gem_ctx_shared.c
> > +++ b/tests/i915/gem_ctx_shared.c
> > @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> >         uint32_t scratch, *s;
> >         uint32_t batch, cs[16];
> >         uint64_t offset;
> > +       uint64_t alignment;
> >         int i;
> >  
> >         gem_require_ring(i915, ring);
> > @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> >         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
> >  
> >         /* Find a hole big enough for both objects later */
> > -       scratch = gem_create(i915, 16384);
> > +       alignment = 2 * gem_gtt_min_alignment(i915);
> > +       scratch = gem_create(i915, 2 * alignment);
> >         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> >         obj.handle = scratch;
> >         gem_execbuf(i915, &execbuf);
> > @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> >         gem_write(i915, batch, 0, cs, sizeof(cs));
> >  
> >         obj.handle = batch;
> > -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> > +       obj.offset += alignment; /* make sure we don't cause an eviction! */
> 
> It's 'stride' here. It's leaving a guard page in between, just in case
> page coloring demands it.

Other than that,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Chris Wilson]

Quoting Chris Wilson (2019-11-01 09:42:18)
> Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> > exec-shared-gtt-* subtests use hardcoded values for object size and
> > softpin offset, based on 4kB GTT alignment assumption.  That may result
> > in those subtests failing when run on future backing stores with
> > possibly larger minimum page sizes.
> > 
> > Replace hardcoded constants with values calculated from minimum GTT
> > alignment of actual backing store the test is running on.
> > 
> > v2: Update helper name, use 'minimum GTT alignment' term across the
> >     change, adjust variable name.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/i915/gem_ctx_shared.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > index 6d8cbcce..1e9c7f78 100644
> > --- a/tests/i915/gem_ctx_shared.c
> > +++ b/tests/i915/gem_ctx_shared.c
> > @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> >         uint32_t scratch, *s;
> >         uint32_t batch, cs[16];
> >         uint64_t offset;
> > +       uint64_t alignment;
> >         int i;
> >  
> >         gem_require_ring(i915, ring);
> > @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> >         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
> >  
> >         /* Find a hole big enough for both objects later */
> > -       scratch = gem_create(i915, 16384);
> > +       alignment = 2 * gem_gtt_min_alignment(i915);
> > +       scratch = gem_create(i915, 2 * alignment);
> >         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> >         obj.handle = scratch;
> >         gem_execbuf(i915, &execbuf);
> > @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int ring)
> >         gem_write(i915, batch, 0, cs, sizeof(cs));
> >  
> >         obj.handle = batch;
> > -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> > +       obj.offset += alignment; /* make sure we don't cause an eviction! */
> 
> It's 'stride' here. It's leaving a guard page in between, just in case
> page coloring demands it.

Other than that,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for Calculate softpin offsets from minimum GTT alignment

[Patchwork]

== Series Details ==

Series: Calculate softpin offsets from minimum GTT alignment
URL   : https://patchwork.freedesktop.org/series/68831/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7231_full -> IGTPW_3638_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_3638_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_color@pipe-a-gamma:
    - {shard-tglb}:       NOTRUN -> [FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-tglb4/igt@kms_color@pipe-a-gamma.html

  
Known issues
------------

  Here are the changes found in IGTPW_3638_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_exec@basic-invalid-context-vcs1:
    - shard-iclb:         [PASS][2] -> [SKIP][3] ([fdo#112080]) +10 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb4/igt@gem_ctx_exec@basic-invalid-context-vcs1.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb5/igt@gem_ctx_exec@basic-invalid-context-vcs1.html

  * igt@gem_ctx_isolation@bcs0-s3:
    - shard-kbl:          [PASS][4] -> [DMESG-WARN][5] ([fdo#108566]) +4 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-kbl6/igt@gem_ctx_isolation@bcs0-s3.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-kbl4/igt@gem_ctx_isolation@bcs0-s3.html

  * igt@gem_ctx_isolation@vcs1-s3:
    - shard-iclb:         [PASS][6] -> [SKIP][7] ([fdo#109276] / [fdo#112080])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb4/igt@gem_ctx_isolation@vcs1-s3.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb5/igt@gem_ctx_isolation@vcs1-s3.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [PASS][8] -> [SKIP][9] ([fdo#110854])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb4/igt@gem_exec_balancer@smoke.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb6/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_schedule@preempt-queue-contexts-chain-bsd:
    - shard-iclb:         [PASS][10] -> [SKIP][11] ([fdo#112146]) +3 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb5/igt@gem_exec_schedule@preempt-queue-contexts-chain-bsd.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb1/igt@gem_exec_schedule@preempt-queue-contexts-chain-bsd.html

  * igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing:
    - shard-hsw:          [PASS][12] -> [FAIL][13] ([fdo#112037])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-hsw8/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-hsw5/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html

  * igt@gem_persistent_relocs@forked-interruptible-thrashing:
    - shard-iclb:         [PASS][14] -> [FAIL][15] ([fdo#112037])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb1/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb3/igt@gem_persistent_relocs@forked-interruptible-thrashing.html

  * igt@gem_persistent_relocs@forked-thrashing:
    - shard-kbl:          [PASS][16] -> [FAIL][17] ([fdo#112037])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-kbl6/igt@gem_persistent_relocs@forked-thrashing.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-kbl6/igt@gem_persistent_relocs@forked-thrashing.html

  * igt@gem_userptr_blits@dmabuf-unsync:
    - shard-snb:          [PASS][18] -> [DMESG-WARN][19] ([fdo#110789] / [fdo#111870])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-snb7/igt@gem_userptr_blits@dmabuf-unsync.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-snb5/igt@gem_userptr_blits@dmabuf-unsync.html

  * igt@gem_userptr_blits@sync-unmap-cycles:
    - shard-snb:          [PASS][20] -> [DMESG-WARN][21] ([fdo#111870]) +1 similar issue
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-snb1/igt@gem_userptr_blits@sync-unmap-cycles.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-snb6/igt@gem_userptr_blits@sync-unmap-cycles.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          [PASS][22] -> [DMESG-WARN][23] ([fdo#108566]) +2 similar issues
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-apl4/igt@kms_flip@flip-vs-suspend-interruptible.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-apl1/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-cpu:
    - shard-iclb:         [PASS][24] -> [FAIL][25] ([fdo#103167]) +5 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-cpu.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-cpu.html

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-indfb-draw-blt:
    - shard-glk:          [PASS][26] -> [FAIL][27] ([fdo#103167])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-glk3/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-indfb-draw-blt.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-glk7/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-indfb-draw-blt.html

  * igt@kms_psr@psr2_basic:
    - shard-iclb:         [PASS][28] -> [SKIP][29] ([fdo#109441]) +2 similar issues
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb2/igt@kms_psr@psr2_basic.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb7/igt@kms_psr@psr2_basic.html

  * igt@kms_setmode@basic:
    - shard-hsw:          [PASS][30] -> [FAIL][31] ([fdo#99912])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-hsw1/igt@kms_setmode@basic.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-hsw1/igt@kms_setmode@basic.html

  * igt@prime_vgem@fence-wait-bsd2:
    - shard-iclb:         [PASS][32] -> [SKIP][33] ([fdo#109276]) +16 similar issues
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb4/igt@prime_vgem@fence-wait-bsd2.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb5/igt@prime_vgem@fence-wait-bsd2.html

  
#### Possible fixes ####

  * {igt@gem_ctx_persistence@vcs1-queued}:
    - shard-iclb:         [SKIP][34] ([fdo#109276] / [fdo#112080]) -> [PASS][35] +4 similar issues
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb5/igt@gem_ctx_persistence@vcs1-queued.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb2/igt@gem_ctx_persistence@vcs1-queued.html

  * igt@gem_eio@unwedge-stress:
    - shard-snb:          [FAIL][36] ([fdo#109661]) -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-snb1/igt@gem_eio@unwedge-stress.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-snb2/igt@gem_eio@unwedge-stress.html

  * igt@gem_exec_parallel@vcs1-fds:
    - shard-iclb:         [SKIP][38] ([fdo#112080]) -> [PASS][39] +9 similar issues
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb5/igt@gem_exec_parallel@vcs1-fds.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb2/igt@gem_exec_parallel@vcs1-fds.html

  * igt@gem_exec_schedule@preempt-queue-bsd1:
    - shard-iclb:         [SKIP][40] ([fdo#109276]) -> [PASS][41] +17 similar issues
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb7/igt@gem_exec_schedule@preempt-queue-bsd1.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb2/igt@gem_exec_schedule@preempt-queue-bsd1.html

  * igt@gem_exec_schedule@preempt-queue-contexts-chain-render:
    - {shard-tglb}:       [INCOMPLETE][42] ([fdo#111677]) -> [PASS][43]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-tglb6/igt@gem_exec_schedule@preempt-queue-contexts-chain-render.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-tglb8/igt@gem_exec_schedule@preempt-queue-contexts-chain-render.html

  * igt@gem_exec_schedule@wide-bsd:
    - shard-iclb:         [SKIP][44] ([fdo#112146]) -> [PASS][45] +2 similar issues
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb2/igt@gem_exec_schedule@wide-bsd.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb7/igt@gem_exec_schedule@wide-bsd.html

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          [DMESG-WARN][46] ([fdo#108566]) -> [PASS][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-apl6/igt@gem_softpin@noreloc-s3.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-apl2/igt@gem_softpin@noreloc-s3.html

  * igt@gem_sync@basic-store-each:
    - {shard-tglb}:       [INCOMPLETE][48] ([fdo#111647] / [fdo#111747]) -> [PASS][49]
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-tglb4/igt@gem_sync@basic-store-each.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-tglb3/igt@gem_sync@basic-store-each.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy:
    - shard-snb:          [DMESG-WARN][50] ([fdo#111870]) -> [PASS][51] +2 similar issues
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-snb6/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-busy.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy-gup:
    - shard-apl:          [INCOMPLETE][52] ([fdo#103927]) -> [PASS][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-apl3/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-apl7/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup:
    - shard-hsw:          [DMESG-WARN][54] ([fdo#110789] / [fdo#111870]) -> [PASS][55]
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-hsw5/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-hsw4/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html

  * igt@gem_userptr_blits@sync-unmap-after-close:
    - shard-hsw:          [DMESG-WARN][56] ([fdo#111870]) -> [PASS][57] +1 similar issue
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-hsw6/igt@gem_userptr_blits@sync-unmap-after-close.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-hsw5/igt@gem_userptr_blits@sync-unmap-after-close.html

  * igt@gem_workarounds@suspend-resume:
    - {shard-tglb}:       [INCOMPLETE][58] ([fdo#111832] / [fdo#111850]) -> [PASS][59] +1 similar issue
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-tglb7/igt@gem_workarounds@suspend-resume.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-tglb4/igt@gem_workarounds@suspend-resume.html

  * igt@i915_pm_rpm@modeset-stress-extra-wait:
    - shard-glk:          [DMESG-WARN][60] ([fdo#105763] / [fdo#106538]) -> [PASS][61]
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-glk8/igt@i915_pm_rpm@modeset-stress-extra-wait.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-glk1/igt@i915_pm_rpm@modeset-stress-extra-wait.html

  * igt@i915_selftest@live_execlists:
    - shard-glk:          [INCOMPLETE][62] ([fdo#103359] / [k.org#198133]) -> [PASS][63]
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-glk6/igt@i915_selftest@live_execlists.html
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-glk2/igt@i915_selftest@live_execlists.html

  * {igt@i915_selftest@live_gt_timelines}:
    - {shard-tglb}:       [INCOMPLETE][64] ([fdo#111831]) -> [PASS][65]
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-tglb6/igt@i915_selftest@live_gt_timelines.html
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-tglb8/igt@i915_selftest@live_gt_timelines.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x128-random:
    - shard-hsw:          [INCOMPLETE][66] ([fdo#103540]) -> [PASS][67] +1 similar issue
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-hsw4/igt@kms_cursor_crc@pipe-a-cursor-128x128-random.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-hsw8/igt@kms_cursor_crc@pipe-a-cursor-128x128-random.html

  * igt@kms_cursor_crc@pipe-a-cursor-dpms:
    - shard-kbl:          [FAIL][68] ([fdo#103232]) -> [PASS][69]
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-kbl6/igt@kms_cursor_crc@pipe-a-cursor-dpms.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-dpms.html
    - shard-apl:          [FAIL][70] ([fdo#103232]) -> [PASS][71]
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-apl4/igt@kms_cursor_crc@pipe-a-cursor-dpms.html
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-apl4/igt@kms_cursor_crc@pipe-a-cursor-dpms.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][72] ([fdo#108566]) -> [PASS][73] +6 similar issues
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-kbl6/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic:
    - shard-glk:          [FAIL][74] ([fdo#104873]) -> [PASS][75]
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-glk5/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-glk1/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [FAIL][76] ([fdo#103167]) -> [PASS][77] +4 similar issues
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt:
    - {shard-tglb}:       [FAIL][78] ([fdo#103167]) -> [PASS][79]
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-tglb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-tglb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [SKIP][80] ([fdo#109642] / [fdo#111068]) -> [PASS][81]
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb4/igt@kms_psr2_su@frontbuffer.html
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb2/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@psr2_cursor_blt:
    - shard-iclb:         [SKIP][82] ([fdo#109441]) -> [PASS][83] +3 similar issues
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb5/igt@kms_psr@psr2_cursor_blt.html
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html

  
#### Warnings ####

  * igt@gem_ctx_isolation@vcs1-nonpriv:
    - shard-iclb:         [FAIL][84] ([fdo#111329]) -> [SKIP][85] ([fdo#109276] / [fdo#112080])
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb4/igt@gem_ctx_isolation@vcs1-nonpriv.html
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb7/igt@gem_ctx_isolation@vcs1-nonpriv.html

  * igt@gem_mocs_settings@mocs-isolation-bsd2:
    - shard-iclb:         [SKIP][86] ([fdo#109276]) -> [FAIL][87] ([fdo#111330]) +1 similar issue
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb6/igt@gem_mocs_settings@mocs-isolation-bsd2.html
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb4/igt@gem_mocs_settings@mocs-isolation-bsd2.html

  * igt@gem_mocs_settings@mocs-settings-bsd2:
    - shard-iclb:         [FAIL][88] ([fdo#111330]) -> [SKIP][89] ([fdo#109276])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb4/igt@gem_mocs_settings@mocs-settings-bsd2.html
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb7/igt@gem_mocs_settings@mocs-settings-bsd2.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
    - shard-iclb:         [INCOMPLETE][90] ([fdo#106978] / [fdo#107713]) -> [FAIL][91] ([fdo#103167])
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7231/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104873]: https://bugs.freedesktop.org/show_bug.cgi?id=104873
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
  [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#109661]: https://bugs.freedesktop.org/show_bug.cgi?id=109661
  [fdo#110789]: https://bugs.freedesktop.org/show_bug.cgi?id=110789
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111329]: https://bugs.freedesktop.org/show_bug.cgi?id=111329
  [fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
  [fdo#111646]: https://bugs.freedesktop.org/show_bug.cgi?id=111646
  [fdo#111647]: https://bugs.freedesktop.org/show_bug.cgi?id=111647
  [fdo#111671]: https://bugs.freedesktop.org/show_bug.cgi?id=111671
  [fdo#111672]: https://bugs.freedesktop.org/show_bug.cgi?id=111672
  [fdo#111677]: https://bugs.freedesktop.org/show_bug.cgi?id=111677
  [fdo#111747]: https://bugs.freedesktop.org/show_bug.cgi?id=111747
  [fdo#111830 ]: https://bugs.freedesktop.org/show_bug.cgi?id=111830 
  [fdo#111831]: https://bugs.freedesktop.org/show_bug.cgi?id=111831
  [fdo#111832]: https://bugs.freedesktop.org/show_bug.cgi?id=111832
  [fdo#111850]: https://bugs.freedesktop.org/show_bug.cgi?id=111850
  [fdo#111865]: https://bugs.freedesktop.org/show_bug.cgi?id=111865
  [fdo#111870]: https://bugs.freedesktop.org/show_bug.cgi?id=111870
  [fdo#112037]: https://bugs.freedesktop.org/show_bug.cgi?id=112037
  [fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
  [fdo#112

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3638/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Janusz Krzysztofik]

Hi Chris,

On Friday, November 1, 2019 11:02:45 AM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-10-31 15:28:54)
> > Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> > addresses for !ppgtt") introduced filtering of addresses possibly
> > occupied by other users of shared GTT.  Unfortunately, that filtering
> > doesn't distinguish between actually occupied addresses and otherwise
> > invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> > when running on future backends with possibly larger minimum page
> > sizes, a half of calculated offsets to be tested will be incorrectly
> > detected as occupied by other users and silently skipped instead of
> > reported as a problem.  That will significantly distort the intended
> > test pattern.
> > 
> > Filter out failing addresses only if not reported as invalid.
> > 
> > v2: Skip unavailable addresses only when not running on full PPGTT.
> > v3: Replace the not on full PPGTT requirement for skipping with error
> >     code checking.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/i915/gem_exec_reloc.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> > index 5f59fe99..423fed8b 100644
> > --- a/tests/i915/gem_exec_reloc.c
> > +++ b/tests/i915/gem_exec_reloc.c
> > @@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> >         uint64_t gtt_size = gem_aperture_size(fd);
> >         const uint32_t bbe = MI_BATCH_BUFFER_END;
> >         igt_spin_t *spin = NULL;
> > -       int count, n;
> > +       int count, n, err;
> >  
> >         igt_require(gem_has_softpin(fd));
> >  
> > @@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> >                 gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> >                 execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> >                 execbuf.buffer_count = 1;
> > -               if (__gem_execbuf(fd, &execbuf))
> > +               err = __gem_execbuf(fd, &execbuf);
> > +               if (err) {
> 
> > +                       igt_assert(err != -EINVAL);
> 
> I've been thinking about this and I think the right approach is
> 
> /* Iff using a shared GTT, some of it may be reserved */
> igt_assert_eq(err, -ENOSPC);

Thanks for your help, I'll follow your approach.

Shouldn't we also use the trick with invalid reloc here to save request 
emission?

Thanks,
Janusz

> 
> >                         continue;
> > +               }
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Janusz Krzysztofik]

Hi Chris,

On Friday, November 1, 2019 11:02:45 AM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-10-31 15:28:54)
> > Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> > addresses for !ppgtt") introduced filtering of addresses possibly
> > occupied by other users of shared GTT.  Unfortunately, that filtering
> > doesn't distinguish between actually occupied addresses and otherwise
> > invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> > when running on future backends with possibly larger minimum page
> > sizes, a half of calculated offsets to be tested will be incorrectly
> > detected as occupied by other users and silently skipped instead of
> > reported as a problem.  That will significantly distort the intended
> > test pattern.
> > 
> > Filter out failing addresses only if not reported as invalid.
> > 
> > v2: Skip unavailable addresses only when not running on full PPGTT.
> > v3: Replace the not on full PPGTT requirement for skipping with error
> >     code checking.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/i915/gem_exec_reloc.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> > index 5f59fe99..423fed8b 100644
> > --- a/tests/i915/gem_exec_reloc.c
> > +++ b/tests/i915/gem_exec_reloc.c
> > @@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> >         uint64_t gtt_size = gem_aperture_size(fd);
> >         const uint32_t bbe = MI_BATCH_BUFFER_END;
> >         igt_spin_t *spin = NULL;
> > -       int count, n;
> > +       int count, n, err;
> >  
> >         igt_require(gem_has_softpin(fd));
> >  
> > @@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> >                 gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> >                 execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> >                 execbuf.buffer_count = 1;
> > -               if (__gem_execbuf(fd, &execbuf))
> > +               err = __gem_execbuf(fd, &execbuf);
> > +               if (err) {
> 
> > +                       igt_assert(err != -EINVAL);
> 
> I've been thinking about this and I think the right approach is
> 
> /* Iff using a shared GTT, some of it may be reserved */
> igt_assert_eq(err, -ENOSPC);

Thanks for your help, I'll follow your approach.

Shouldn't we also use the trick with invalid reloc here to save request 
emission?

Thanks,
Janusz

> 
> >                         continue;
> > +               }
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Janusz Krzysztofik]

Hi Vanshi,

On Thursday, October 31, 2019 5:59:50 PM CET Vanshidhar Konda wrote:
> May be this patch can just be merged with the other patch in this series
> that changes gem_exec_reloc.

Even if both patches are closely related to possibly incorrect alignment in 
use, I think each one resolves it own distinct issue, that's why I think they 
should be kept separate.

Thanks,
Janusz


> Vanshi
> 
> On Thu, Oct 31, 2019 at 04:28:54PM +0100, Janusz Krzysztofik wrote:
> >Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> >addresses for !ppgtt") introduced filtering of addresses possibly
> >occupied by other users of shared GTT.  Unfortunately, that filtering
> >doesn't distinguish between actually occupied addresses and otherwise
> >invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> >when running on future backends with possibly larger minimum page
> >sizes, a half of calculated offsets to be tested will be incorrectly
> >detected as occupied by other users and silently skipped instead of
> >reported as a problem.  That will significantly distort the intended
> >test pattern.
> >
> >Filter out failing addresses only if not reported as invalid.
> >
> >v2: Skip unavailable addresses only when not running on full PPGTT.
> >v3: Replace the not on full PPGTT requirement for skipping with error
> >    code checking.
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> > tests/i915/gem_exec_reloc.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> >diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> >index 5f59fe99..423fed8b 100644
> >--- a/tests/i915/gem_exec_reloc.c
> >+++ b/tests/i915/gem_exec_reloc.c
> >@@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> > 	uint64_t gtt_size = gem_aperture_size(fd);
> > 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> > 	igt_spin_t *spin = NULL;
> >-	int count, n;
> >+	int count, n, err;
> >
> > 	igt_require(gem_has_softpin(fd));
> >
> >@@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> > 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> > 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> > 		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = __gem_execbuf(fd, &execbuf);
> >+		if (err) {
> >+			igt_assert(err != -EINVAL);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long long)obj[n].offset);
> >@@ -562,8 +565,11 @@ static void basic_range(int fd, unsigned flags)
> > 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> > 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> > 		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = __gem_execbuf(fd, &execbuf);
> >+		if (err) {
> >+			igt_assert(err != -EINVAL);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long long)obj[n].offset);
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Janusz Krzysztofik]

Hi Vanshi,

On Thursday, October 31, 2019 5:59:50 PM CET Vanshidhar Konda wrote:
> May be this patch can just be merged with the other patch in this series
> that changes gem_exec_reloc.

Even if both patches are closely related to possibly incorrect alignment in 
use, I think each one resolves it own distinct issue, that's why I think they 
should be kept separate.

Thanks,
Janusz


> Vanshi
> 
> On Thu, Oct 31, 2019 at 04:28:54PM +0100, Janusz Krzysztofik wrote:
> >Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> >addresses for !ppgtt") introduced filtering of addresses possibly
> >occupied by other users of shared GTT.  Unfortunately, that filtering
> >doesn't distinguish between actually occupied addresses and otherwise
> >invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> >when running on future backends with possibly larger minimum page
> >sizes, a half of calculated offsets to be tested will be incorrectly
> >detected as occupied by other users and silently skipped instead of
> >reported as a problem.  That will significantly distort the intended
> >test pattern.
> >
> >Filter out failing addresses only if not reported as invalid.
> >
> >v2: Skip unavailable addresses only when not running on full PPGTT.
> >v3: Replace the not on full PPGTT requirement for skipping with error
> >    code checking.
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> > tests/i915/gem_exec_reloc.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> >diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> >index 5f59fe99..423fed8b 100644
> >--- a/tests/i915/gem_exec_reloc.c
> >+++ b/tests/i915/gem_exec_reloc.c
> >@@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> > 	uint64_t gtt_size = gem_aperture_size(fd);
> > 	const uint32_t bbe = MI_BATCH_BUFFER_END;
> > 	igt_spin_t *spin = NULL;
> >-	int count, n;
> >+	int count, n, err;
> >
> > 	igt_require(gem_has_softpin(fd));
> >
> >@@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> > 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> > 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> > 		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = __gem_execbuf(fd, &execbuf);
> >+		if (err) {
> >+			igt_assert(err != -EINVAL);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long long)obj[n].offset);
> >@@ -562,8 +565,11 @@ static void basic_range(int fd, unsigned flags)
> > 		gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> > 		execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> > 		execbuf.buffer_count = 1;
> >-		if (__gem_execbuf(fd, &execbuf))
> >+		err = __gem_execbuf(fd, &execbuf);
> >+		if (err) {
> >+			igt_assert(err != -EINVAL);
> > 			continue;
> >+		}
> >
> > 		igt_debug("obj[%d] handle=%d, address=%llx\n",
> > 			  n, obj[n].handle, (long long)obj[n].offset);
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-11-04 09:13:28)
> Hi Chris,
> 
> On Friday, November 1, 2019 11:02:45 AM CET Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-10-31 15:28:54)
> > > Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> > > addresses for !ppgtt") introduced filtering of addresses possibly
> > > occupied by other users of shared GTT.  Unfortunately, that filtering
> > > doesn't distinguish between actually occupied addresses and otherwise
> > > invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> > > when running on future backends with possibly larger minimum page
> > > sizes, a half of calculated offsets to be tested will be incorrectly
> > > detected as occupied by other users and silently skipped instead of
> > > reported as a problem.  That will significantly distort the intended
> > > test pattern.
> > > 
> > > Filter out failing addresses only if not reported as invalid.
> > > 
> > > v2: Skip unavailable addresses only when not running on full PPGTT.
> > > v3: Replace the not on full PPGTT requirement for skipping with error
> > >     code checking.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  tests/i915/gem_exec_reloc.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> > > index 5f59fe99..423fed8b 100644
> > > --- a/tests/i915/gem_exec_reloc.c
> > > +++ b/tests/i915/gem_exec_reloc.c
> > > @@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> > >         uint64_t gtt_size = gem_aperture_size(fd);
> > >         const uint32_t bbe = MI_BATCH_BUFFER_END;
> > >         igt_spin_t *spin = NULL;
> > > -       int count, n;
> > > +       int count, n, err;
> > >  
> > >         igt_require(gem_has_softpin(fd));
> > >  
> > > @@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> > >                 gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> > >                 execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> > >                 execbuf.buffer_count = 1;
> > > -               if (__gem_execbuf(fd, &execbuf))
> > > +               err = __gem_execbuf(fd, &execbuf);
> > > +               if (err) {
> > 
> > > +                       igt_assert(err != -EINVAL);
> > 
> > I've been thinking about this and I think the right approach is
> > 
> > /* Iff using a shared GTT, some of it may be reserved */
> > igt_assert_eq(err, -ENOSPC);
> 
> Thanks for your help, I'll follow your approach.
> 
> Shouldn't we also use the trick with invalid reloc here to save request 
> emission?

Could do. If you move the whole probe out of line so it's not easily
confused with the central point of the test, that'll be great.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-11-04 09:13:28)
> Hi Chris,
> 
> On Friday, November 1, 2019 11:02:45 AM CET Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-10-31 15:28:54)
> > > Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> > > addresses for !ppgtt") introduced filtering of addresses possibly
> > > occupied by other users of shared GTT.  Unfortunately, that filtering
> > > doesn't distinguish between actually occupied addresses and otherwise
> > > invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> > > when running on future backends with possibly larger minimum page
> > > sizes, a half of calculated offsets to be tested will be incorrectly
> > > detected as occupied by other users and silently skipped instead of
> > > reported as a problem.  That will significantly distort the intended
> > > test pattern.
> > > 
> > > Filter out failing addresses only if not reported as invalid.
> > > 
> > > v2: Skip unavailable addresses only when not running on full PPGTT.
> > > v3: Replace the not on full PPGTT requirement for skipping with error
> > >     code checking.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  tests/i915/gem_exec_reloc.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> > > index 5f59fe99..423fed8b 100644
> > > --- a/tests/i915/gem_exec_reloc.c
> > > +++ b/tests/i915/gem_exec_reloc.c
> > > @@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> > >         uint64_t gtt_size = gem_aperture_size(fd);
> > >         const uint32_t bbe = MI_BATCH_BUFFER_END;
> > >         igt_spin_t *spin = NULL;
> > > -       int count, n;
> > > +       int count, n, err;
> > >  
> > >         igt_require(gem_has_softpin(fd));
> > >  
> > > @@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> > >                 gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> > >                 execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> > >                 execbuf.buffer_count = 1;
> > > -               if (__gem_execbuf(fd, &execbuf))
> > > +               err = __gem_execbuf(fd, &execbuf);
> > > +               if (err) {
> > 
> > > +                       igt_assert(err != -EINVAL);
> > 
> > I've been thinking about this and I think the right approach is
> > 
> > /* Iff using a shared GTT, some of it may be reserved */
> > igt_assert_eq(err, -ENOSPC);
> 
> Thanks for your help, I'll follow your approach.
> 
> Shouldn't we also use the trick with invalid reloc here to save request 
> emission?

Could do. If you move the whole probe out of line so it's not easily
confused with the central point of the test, that'll be great.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [RFC PATCH i-g-t v4 1/4] tests/gem_exec_reloc: Don't filter out invalid addresses

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-11-04 09:13:28)
> Hi Chris,
> 
> On Friday, November 1, 2019 11:02:45 AM CET Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-10-31 15:28:54)
> > > Commit a355b2d6eb42 ("igt/gem_exec_reloc: Filter out unavailable
> > > addresses for !ppgtt") introduced filtering of addresses possibly
> > > occupied by other users of shared GTT.  Unfortunately, that filtering
> > > doesn't distinguish between actually occupied addresses and otherwise
> > > invalid softpin offsets.  As soon as incorrect GTT alignment is assumed
> > > when running on future backends with possibly larger minimum page
> > > sizes, a half of calculated offsets to be tested will be incorrectly
> > > detected as occupied by other users and silently skipped instead of
> > > reported as a problem.  That will significantly distort the intended
> > > test pattern.
> > > 
> > > Filter out failing addresses only if not reported as invalid.
> > > 
> > > v2: Skip unavailable addresses only when not running on full PPGTT.
> > > v3: Replace the not on full PPGTT requirement for skipping with error
> > >     code checking.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  tests/i915/gem_exec_reloc.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> > > index 5f59fe99..423fed8b 100644
> > > --- a/tests/i915/gem_exec_reloc.c
> > > +++ b/tests/i915/gem_exec_reloc.c
> > > @@ -520,7 +520,7 @@ static void basic_range(int fd, unsigned flags)
> > >         uint64_t gtt_size = gem_aperture_size(fd);
> > >         const uint32_t bbe = MI_BATCH_BUFFER_END;
> > >         igt_spin_t *spin = NULL;
> > > -       int count, n;
> > > +       int count, n, err;
> > >  
> > >         igt_require(gem_has_softpin(fd));
> > >  
> > > @@ -542,8 +542,11 @@ static void basic_range(int fd, unsigned flags)
> > >                 gem_write(fd, obj[n].handle, 0, &bbe, sizeof(bbe));
> > >                 execbuf.buffers_ptr = to_user_pointer(&obj[n]);
> > >                 execbuf.buffer_count = 1;
> > > -               if (__gem_execbuf(fd, &execbuf))
> > > +               err = __gem_execbuf(fd, &execbuf);
> > > +               if (err) {
> > 
> > > +                       igt_assert(err != -EINVAL);
> > 
> > I've been thinking about this and I think the right approach is
> > 
> > /* Iff using a shared GTT, some of it may be reserved */
> > igt_assert_eq(err, -ENOSPC);
> 
> Thanks for your help, I'll follow your approach.
> 
> Shouldn't we also use the trick with invalid reloc here to save request 
> emission?

Could do. If you move the whole probe out of line so it's not easily
confused with the central point of the test, that'll be great.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

Hi Chris,

On Friday, November 1, 2019 11:08:45 AM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> > Some tests assume 4kB offset alignment while using softpin.  That
> > assumption may be wrong on future GEM backends with possibly larger
> > minimum page sizes.  As a result, those tests may either fail on
> > softpin at offsets which are incorrectly aligned, may silently skip
> > such incorrectly aligned addresses assuming them occupied by other
> > users if incorrect detection method is used, or may always succeed
> > when examining invalid use patterns.
> > 
> > Provide a helper function that detects minimum GTT alignment.  Tests
> > may use it to calculate softpin offsets valid for actually used backing
> > store.
> > 
> > v2: Rename the helper, use 'minimum GTT alignment' term across the
> >     change (Chris),
> >   - use error numbers to distinguish between invalid offsets and
> >     addresses occupied by other users, then
> >   - simplify the code (Chris).
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> > ---
> >  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> >  lib/ioctl_wrappers.h |  2 ++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > index 628f8b83..f0ef8b2e 100644
> > --- a/lib/ioctl_wrappers.c
> > +++ b/lib/ioctl_wrappers.c
> > @@ -54,6 +54,7 @@
> >  #include "intel_io.h"
> >  #include "igt_debugfs.h"
> >  #include "igt_sysfs.h"
> > +#include "igt_x86.h"
> >  #include "config.h"
> >  
> >  #ifdef HAVE_VALGRIND
> > @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> >         return has_softpin;
> >  }
> >  
> > +/**
> > + * gem_gtt_min_alignment_order:
> > + * @fd: open i915 drm file descriptor
> > + *
> > + * This function detects the minimum possible alignment of a soft-pinned 
gem
> > + * object allocated from a default backing store.  It is useful for 
calculating
> > + * correctly aligned softpin offsets.
> > + * Since size order to size conversion (size = 1 << order) is less 
trivial
> > + * than the opposite, the function returns the alignment order as more 
handy.
> > + *
> > + * Returns:
> > + * Size order of the minimum GTT alignment
> > + */
> > +int gem_gtt_min_alignment_order(int fd)
> 
> But not part of ioctl_wrappers.c!
> 
> lib/i915/gem_gtt_topology.c? _query.c? _probe.c?

I was thinking about that but couldn't find a good name.  I'll use one of your 
proposed, thanks.

> > +{
> > +       struct drm_i915_gem_exec_object2 obj;
> > +       struct drm_i915_gem_execbuffer2 eb;
> > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > +       int order;
> > +
> > +       /* no softpin => 4kB page size */
> > +       if (!gem_has_softpin(fd))
> > +               return 12;
> > +
> > +       memset(&obj, 0, sizeof(obj));
> > +       memset(&eb, 0, sizeof(eb));
> > +
> > +       obj.handle = gem_create(fd, 4096);
> > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > +       eb.buffers_ptr = to_user_pointer(&obj);
> > +       eb.buffer_count = 1;
> > +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> > +
> > +       for (order = 12; order < 64; order++) {
> > +               obj.offset = 1ull << order;
> > +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> > +                       break;

Should I also follow your advice of checking for -ENOSPC here rather than 
!-EINVAL?

> > +       }
> > +       igt_assert(obj.offset < gem_aperture_size(fd));
> 
> Hmm, there is one more trick we can apply here: an invalid reloc.
> 
> memset(&reloc, 0, sizeof(reloc));
> obj.relocs_ptr = to_user_pointer(&reloc);
> obj.relocation_count = 1;
> 
> We first process the buffers, then we process the relocations. So we
> will get the -EINVAL for illegal softpin before we get the -ENOENT for
> invalid reloc handle.
> 
> That just saves actually emitting a request.

I like this idea.

Thanks,
Janusz

> -Chris
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

Hi Chris,

On Friday, November 1, 2019 11:08:45 AM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> > Some tests assume 4kB offset alignment while using softpin.  That
> > assumption may be wrong on future GEM backends with possibly larger
> > minimum page sizes.  As a result, those tests may either fail on
> > softpin at offsets which are incorrectly aligned, may silently skip
> > such incorrectly aligned addresses assuming them occupied by other
> > users if incorrect detection method is used, or may always succeed
> > when examining invalid use patterns.
> > 
> > Provide a helper function that detects minimum GTT alignment.  Tests
> > may use it to calculate softpin offsets valid for actually used backing
> > store.
> > 
> > v2: Rename the helper, use 'minimum GTT alignment' term across the
> >     change (Chris),
> >   - use error numbers to distinguish between invalid offsets and
> >     addresses occupied by other users, then
> >   - simplify the code (Chris).
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Stuart Summers <stuart.summers@intel.com>
> > ---
> >  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> >  lib/ioctl_wrappers.h |  2 ++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > index 628f8b83..f0ef8b2e 100644
> > --- a/lib/ioctl_wrappers.c
> > +++ b/lib/ioctl_wrappers.c
> > @@ -54,6 +54,7 @@
> >  #include "intel_io.h"
> >  #include "igt_debugfs.h"
> >  #include "igt_sysfs.h"
> > +#include "igt_x86.h"
> >  #include "config.h"
> >  
> >  #ifdef HAVE_VALGRIND
> > @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> >         return has_softpin;
> >  }
> >  
> > +/**
> > + * gem_gtt_min_alignment_order:
> > + * @fd: open i915 drm file descriptor
> > + *
> > + * This function detects the minimum possible alignment of a soft-pinned 
gem
> > + * object allocated from a default backing store.  It is useful for 
calculating
> > + * correctly aligned softpin offsets.
> > + * Since size order to size conversion (size = 1 << order) is less 
trivial
> > + * than the opposite, the function returns the alignment order as more 
handy.
> > + *
> > + * Returns:
> > + * Size order of the minimum GTT alignment
> > + */
> > +int gem_gtt_min_alignment_order(int fd)
> 
> But not part of ioctl_wrappers.c!
> 
> lib/i915/gem_gtt_topology.c? _query.c? _probe.c?

I was thinking about that but couldn't find a good name.  I'll use one of your 
proposed, thanks.

> > +{
> > +       struct drm_i915_gem_exec_object2 obj;
> > +       struct drm_i915_gem_execbuffer2 eb;
> > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > +       int order;
> > +
> > +       /* no softpin => 4kB page size */
> > +       if (!gem_has_softpin(fd))
> > +               return 12;
> > +
> > +       memset(&obj, 0, sizeof(obj));
> > +       memset(&eb, 0, sizeof(eb));
> > +
> > +       obj.handle = gem_create(fd, 4096);
> > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > +       eb.buffers_ptr = to_user_pointer(&obj);
> > +       eb.buffer_count = 1;
> > +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> > +
> > +       for (order = 12; order < 64; order++) {
> > +               obj.offset = 1ull << order;
> > +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> > +                       break;

Should I also follow your advice of checking for -ENOSPC here rather than 
!-EINVAL?

> > +       }
> > +       igt_assert(obj.offset < gem_aperture_size(fd));
> 
> Hmm, there is one more trick we can apply here: an invalid reloc.
> 
> memset(&reloc, 0, sizeof(reloc));
> obj.relocs_ptr = to_user_pointer(&reloc);
> obj.relocation_count = 1;
> 
> We first process the buffers, then we process the relocations. So we
> will get the -EINVAL for illegal softpin before we get the -ENOENT for
> invalid reloc handle.
> 
> That just saves actually emitting a request.

I like this idea.

Thanks,
Janusz

> -Chris
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-11-04 09:23:12)
> Hi Chris,
> 
> On Friday, November 1, 2019 11:08:45 AM CET Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> > > Some tests assume 4kB offset alignment while using softpin.  That
> > > assumption may be wrong on future GEM backends with possibly larger
> > > minimum page sizes.  As a result, those tests may either fail on
> > > softpin at offsets which are incorrectly aligned, may silently skip
> > > such incorrectly aligned addresses assuming them occupied by other
> > > users if incorrect detection method is used, or may always succeed
> > > when examining invalid use patterns.
> > > 
> > > Provide a helper function that detects minimum GTT alignment.  Tests
> > > may use it to calculate softpin offsets valid for actually used backing
> > > store.
> > > 
> > > v2: Rename the helper, use 'minimum GTT alignment' term across the
> > >     change (Chris),
> > >   - use error numbers to distinguish between invalid offsets and
> > >     addresses occupied by other users, then
> > >   - simplify the code (Chris).
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > ---
> > >  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/ioctl_wrappers.h |  2 ++
> > >  2 files changed, 48 insertions(+)
> > > 
> > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > > index 628f8b83..f0ef8b2e 100644
> > > --- a/lib/ioctl_wrappers.c
> > > +++ b/lib/ioctl_wrappers.c
> > > @@ -54,6 +54,7 @@
> > >  #include "intel_io.h"
> > >  #include "igt_debugfs.h"
> > >  #include "igt_sysfs.h"
> > > +#include "igt_x86.h"
> > >  #include "config.h"
> > >  
> > >  #ifdef HAVE_VALGRIND
> > > @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> > >         return has_softpin;
> > >  }
> > >  
> > > +/**
> > > + * gem_gtt_min_alignment_order:
> > > + * @fd: open i915 drm file descriptor
> > > + *
> > > + * This function detects the minimum possible alignment of a soft-pinned 
> gem
> > > + * object allocated from a default backing store.  It is useful for 
> calculating
> > > + * correctly aligned softpin offsets.
> > > + * Since size order to size conversion (size = 1 << order) is less 
> trivial
> > > + * than the opposite, the function returns the alignment order as more 
> handy.
> > > + *
> > > + * Returns:
> > > + * Size order of the minimum GTT alignment
> > > + */
> > > +int gem_gtt_min_alignment_order(int fd)
> > 
> > But not part of ioctl_wrappers.c!
> > 
> > lib/i915/gem_gtt_topology.c? _query.c? _probe.c?
> 
> I was thinking about that but couldn't find a good name.  I'll use one of your 
> proposed, thanks.
> 
> > > +{
> > > +       struct drm_i915_gem_exec_object2 obj;
> > > +       struct drm_i915_gem_execbuffer2 eb;
> > > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > > +       int order;
> > > +
> > > +       /* no softpin => 4kB page size */
> > > +       if (!gem_has_softpin(fd))
> > > +               return 12;
> > > +
> > > +       memset(&obj, 0, sizeof(obj));
> > > +       memset(&eb, 0, sizeof(eb));
> > > +
> > > +       obj.handle = gem_create(fd, 4096);
> > > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > > +       eb.buffers_ptr = to_user_pointer(&obj);
> > > +       eb.buffer_count = 1;
> > > +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> > > +
> > > +       for (order = 12; order < 64; order++) {
> > > +               obj.offset = 1ull << order;
> > > +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> > > +                       break;
> 
> Should I also follow your advice of checking for -ENOSPC here rather than 
> !-EINVAL?

This time we expect EINVAL for the invalid non-aligned offset provided by
the user. -ENOSPC comes later when we fail to evict -- but we will only
try that with a valid GTT node.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-11-04 09:23:12)
> Hi Chris,
> 
> On Friday, November 1, 2019 11:08:45 AM CET Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> > > Some tests assume 4kB offset alignment while using softpin.  That
> > > assumption may be wrong on future GEM backends with possibly larger
> > > minimum page sizes.  As a result, those tests may either fail on
> > > softpin at offsets which are incorrectly aligned, may silently skip
> > > such incorrectly aligned addresses assuming them occupied by other
> > > users if incorrect detection method is used, or may always succeed
> > > when examining invalid use patterns.
> > > 
> > > Provide a helper function that detects minimum GTT alignment.  Tests
> > > may use it to calculate softpin offsets valid for actually used backing
> > > store.
> > > 
> > > v2: Rename the helper, use 'minimum GTT alignment' term across the
> > >     change (Chris),
> > >   - use error numbers to distinguish between invalid offsets and
> > >     addresses occupied by other users, then
> > >   - simplify the code (Chris).
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > ---
> > >  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/ioctl_wrappers.h |  2 ++
> > >  2 files changed, 48 insertions(+)
> > > 
> > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > > index 628f8b83..f0ef8b2e 100644
> > > --- a/lib/ioctl_wrappers.c
> > > +++ b/lib/ioctl_wrappers.c
> > > @@ -54,6 +54,7 @@
> > >  #include "intel_io.h"
> > >  #include "igt_debugfs.h"
> > >  #include "igt_sysfs.h"
> > > +#include "igt_x86.h"
> > >  #include "config.h"
> > >  
> > >  #ifdef HAVE_VALGRIND
> > > @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> > >         return has_softpin;
> > >  }
> > >  
> > > +/**
> > > + * gem_gtt_min_alignment_order:
> > > + * @fd: open i915 drm file descriptor
> > > + *
> > > + * This function detects the minimum possible alignment of a soft-pinned 
> gem
> > > + * object allocated from a default backing store.  It is useful for 
> calculating
> > > + * correctly aligned softpin offsets.
> > > + * Since size order to size conversion (size = 1 << order) is less 
> trivial
> > > + * than the opposite, the function returns the alignment order as more 
> handy.
> > > + *
> > > + * Returns:
> > > + * Size order of the minimum GTT alignment
> > > + */
> > > +int gem_gtt_min_alignment_order(int fd)
> > 
> > But not part of ioctl_wrappers.c!
> > 
> > lib/i915/gem_gtt_topology.c? _query.c? _probe.c?
> 
> I was thinking about that but couldn't find a good name.  I'll use one of your 
> proposed, thanks.
> 
> > > +{
> > > +       struct drm_i915_gem_exec_object2 obj;
> > > +       struct drm_i915_gem_execbuffer2 eb;
> > > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > > +       int order;
> > > +
> > > +       /* no softpin => 4kB page size */
> > > +       if (!gem_has_softpin(fd))
> > > +               return 12;
> > > +
> > > +       memset(&obj, 0, sizeof(obj));
> > > +       memset(&eb, 0, sizeof(eb));
> > > +
> > > +       obj.handle = gem_create(fd, 4096);
> > > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > > +       eb.buffers_ptr = to_user_pointer(&obj);
> > > +       eb.buffer_count = 1;
> > > +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> > > +
> > > +       for (order = 12; order < 64; order++) {
> > > +               obj.offset = 1ull << order;
> > > +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> > > +                       break;
> 
> Should I also follow your advice of checking for -ENOSPC here rather than 
> !-EINVAL?

This time we expect EINVAL for the invalid non-aligned offset provided by
the user. -ENOSPC comes later when we fail to evict -- but we will only
try that with a valid GTT node.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Janusz Krzysztofik]

Hi Chris,

On Friday, November 1, 2019 10:42:18 AM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> > exec-shared-gtt-* subtests use hardcoded values for object size and
> > softpin offset, based on 4kB GTT alignment assumption.  That may result
> > in those subtests failing when run on future backing stores with
> > possibly larger minimum page sizes.
> > 
> > Replace hardcoded constants with values calculated from minimum GTT
> > alignment of actual backing store the test is running on.
> > 
> > v2: Update helper name, use 'minimum GTT alignment' term across the
> >     change, adjust variable name.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/i915/gem_ctx_shared.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > index 6d8cbcce..1e9c7f78 100644
> > --- a/tests/i915/gem_ctx_shared.c
> > +++ b/tests/i915/gem_ctx_shared.c
> > @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int 
ring)
> >         uint32_t scratch, *s;
> >         uint32_t batch, cs[16];
> >         uint64_t offset;
> > +       uint64_t alignment;
> >         int i;
> >  
> >         gem_require_ring(i915, ring);
> > @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int 
ring)
> >         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
> >  
> >         /* Find a hole big enough for both objects later */
> > -       scratch = gem_create(i915, 16384);
> > +       alignment = 2 * gem_gtt_min_alignment(i915);

alignment = one page for an object + one page for stride

> > +       scratch = gem_create(i915, 2 * alignment);

space reserved = 2 * (one page for an object + one page for stride)

> >         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> >         obj.handle = scratch;
> >         gem_execbuf(i915, &execbuf);
> > @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int 
ring)
> >         gem_write(i915, batch, 0, cs, sizeof(cs));
> >  
> >         obj.handle = batch;
> > -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> > +       obj.offset += alignment; /* make sure we don't cause an eviction! 
*/

offset += (one page for an object + one page for stride)

> It's 'stride' here. It's leaving a guard page in between, just in case
> page coloring demands it.

Assuming I correctly understood the intentions here but my implementation 
is not readable clearly enough, how do you think this should be fixed?  How 
about a comment '/* one page for an object + one page for stride */' above the 
line where the 'alignment' variable is assigned the value of 2 * minimum GTT 
alignment?

Thanks,
Janusz

> -Chris
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Janusz Krzysztofik]

Hi Chris,

On Friday, November 1, 2019 10:42:18 AM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> > exec-shared-gtt-* subtests use hardcoded values for object size and
> > softpin offset, based on 4kB GTT alignment assumption.  That may result
> > in those subtests failing when run on future backing stores with
> > possibly larger minimum page sizes.
> > 
> > Replace hardcoded constants with values calculated from minimum GTT
> > alignment of actual backing store the test is running on.
> > 
> > v2: Update helper name, use 'minimum GTT alignment' term across the
> >     change, adjust variable name.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/i915/gem_ctx_shared.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > index 6d8cbcce..1e9c7f78 100644
> > --- a/tests/i915/gem_ctx_shared.c
> > +++ b/tests/i915/gem_ctx_shared.c
> > @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int 
ring)
> >         uint32_t scratch, *s;
> >         uint32_t batch, cs[16];
> >         uint64_t offset;
> > +       uint64_t alignment;
> >         int i;
> >  
> >         gem_require_ring(i915, ring);
> > @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int 
ring)
> >         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
> >  
> >         /* Find a hole big enough for both objects later */
> > -       scratch = gem_create(i915, 16384);
> > +       alignment = 2 * gem_gtt_min_alignment(i915);

alignment = one page for an object + one page for stride

> > +       scratch = gem_create(i915, 2 * alignment);

space reserved = 2 * (one page for an object + one page for stride)

> >         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> >         obj.handle = scratch;
> >         gem_execbuf(i915, &execbuf);
> > @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int 
ring)
> >         gem_write(i915, batch, 0, cs, sizeof(cs));
> >  
> >         obj.handle = batch;
> > -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> > +       obj.offset += alignment; /* make sure we don't cause an eviction! 
*/

offset += (one page for an object + one page for stride)

> It's 'stride' here. It's leaving a guard page in between, just in case
> page coloring demands it.

Assuming I correctly understood the intentions here but my implementation 
is not readable clearly enough, how do you think this should be fixed?  How 
about a comment '/* one page for an object + one page for stride */' above the 
line where the 'alignment' variable is assigned the value of 2 * minimum GTT 
alignment?

Thanks,
Janusz

> -Chris
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-11-04 09:39:32)
> Hi Chris,
> 
> On Friday, November 1, 2019 10:42:18 AM CET Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> > > exec-shared-gtt-* subtests use hardcoded values for object size and
> > > softpin offset, based on 4kB GTT alignment assumption.  That may result
> > > in those subtests failing when run on future backing stores with
> > > possibly larger minimum page sizes.
> > > 
> > > Replace hardcoded constants with values calculated from minimum GTT
> > > alignment of actual backing store the test is running on.
> > > 
> > > v2: Update helper name, use 'minimum GTT alignment' term across the
> > >     change, adjust variable name.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  tests/i915/gem_ctx_shared.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > > index 6d8cbcce..1e9c7f78 100644
> > > --- a/tests/i915/gem_ctx_shared.c
> > > +++ b/tests/i915/gem_ctx_shared.c
> > > @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         uint32_t scratch, *s;
> > >         uint32_t batch, cs[16];
> > >         uint64_t offset;
> > > +       uint64_t alignment;
> > >         int i;
> > >  
> > >         gem_require_ring(i915, ring);
> > > @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
> > >  
> > >         /* Find a hole big enough for both objects later */
> > > -       scratch = gem_create(i915, 16384);
> > > +       alignment = 2 * gem_gtt_min_alignment(i915);
> 
> alignment = one page for an object + one page for stride
> 
> > > +       scratch = gem_create(i915, 2 * alignment);
> 
> space reserved = 2 * (one page for an object + one page for stride)
> 
> > >         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> > >         obj.handle = scratch;
> > >         gem_execbuf(i915, &execbuf);
> > > @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         gem_write(i915, batch, 0, cs, sizeof(cs));
> > >  
> > >         obj.handle = batch;
> > > -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> > > +       obj.offset += alignment; /* make sure we don't cause an eviction! 
> */
> 
> offset += (one page for an object + one page for stride)
> 
> > It's 'stride' here. It's leaving a guard page in between, just in case
> > page coloring demands it.
> 
> Assuming I correctly understood the intentions here but my implementation 
> is not readable clearly enough, how do you think this should be fixed?  How 
> about a comment '/* one page for an object + one page for stride */' above the 
> line where the 'alignment' variable is assigned the value of 2 * minimum GTT 
> alignment?

I think "stride = 2 * min_alignment()" concisely describes the intent.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-11-04 09:39:32)
> Hi Chris,
> 
> On Friday, November 1, 2019 10:42:18 AM CET Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> > > exec-shared-gtt-* subtests use hardcoded values for object size and
> > > softpin offset, based on 4kB GTT alignment assumption.  That may result
> > > in those subtests failing when run on future backing stores with
> > > possibly larger minimum page sizes.
> > > 
> > > Replace hardcoded constants with values calculated from minimum GTT
> > > alignment of actual backing store the test is running on.
> > > 
> > > v2: Update helper name, use 'minimum GTT alignment' term across the
> > >     change, adjust variable name.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  tests/i915/gem_ctx_shared.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > > index 6d8cbcce..1e9c7f78 100644
> > > --- a/tests/i915/gem_ctx_shared.c
> > > +++ b/tests/i915/gem_ctx_shared.c
> > > @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         uint32_t scratch, *s;
> > >         uint32_t batch, cs[16];
> > >         uint64_t offset;
> > > +       uint64_t alignment;
> > >         int i;
> > >  
> > >         gem_require_ring(i915, ring);
> > > @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
> > >  
> > >         /* Find a hole big enough for both objects later */
> > > -       scratch = gem_create(i915, 16384);
> > > +       alignment = 2 * gem_gtt_min_alignment(i915);
> 
> alignment = one page for an object + one page for stride
> 
> > > +       scratch = gem_create(i915, 2 * alignment);
> 
> space reserved = 2 * (one page for an object + one page for stride)
> 
> > >         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> > >         obj.handle = scratch;
> > >         gem_execbuf(i915, &execbuf);
> > > @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         gem_write(i915, batch, 0, cs, sizeof(cs));
> > >  
> > >         obj.handle = batch;
> > > -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> > > +       obj.offset += alignment; /* make sure we don't cause an eviction! 
> */
> 
> offset += (one page for an object + one page for stride)
> 
> > It's 'stride' here. It's leaving a guard page in between, just in case
> > page coloring demands it.
> 
> Assuming I correctly understood the intentions here but my implementation 
> is not readable clearly enough, how do you think this should be fixed?  How 
> about a comment '/* one page for an object + one page for stride */' above the 
> line where the 'alignment' variable is assigned the value of 2 * minimum GTT 
> alignment?

I think "stride = 2 * min_alignment()" concisely describes the intent.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-11-04 09:39:32)
> Hi Chris,
> 
> On Friday, November 1, 2019 10:42:18 AM CET Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> > > exec-shared-gtt-* subtests use hardcoded values for object size and
> > > softpin offset, based on 4kB GTT alignment assumption.  That may result
> > > in those subtests failing when run on future backing stores with
> > > possibly larger minimum page sizes.
> > > 
> > > Replace hardcoded constants with values calculated from minimum GTT
> > > alignment of actual backing store the test is running on.
> > > 
> > > v2: Update helper name, use 'minimum GTT alignment' term across the
> > >     change, adjust variable name.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  tests/i915/gem_ctx_shared.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > > index 6d8cbcce..1e9c7f78 100644
> > > --- a/tests/i915/gem_ctx_shared.c
> > > +++ b/tests/i915/gem_ctx_shared.c
> > > @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         uint32_t scratch, *s;
> > >         uint32_t batch, cs[16];
> > >         uint64_t offset;
> > > +       uint64_t alignment;
> > >         int i;
> > >  
> > >         gem_require_ring(i915, ring);
> > > @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
> > >  
> > >         /* Find a hole big enough for both objects later */
> > > -       scratch = gem_create(i915, 16384);
> > > +       alignment = 2 * gem_gtt_min_alignment(i915);
> 
> alignment = one page for an object + one page for stride
> 
> > > +       scratch = gem_create(i915, 2 * alignment);
> 
> space reserved = 2 * (one page for an object + one page for stride)
> 
> > >         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> > >         obj.handle = scratch;
> > >         gem_execbuf(i915, &execbuf);
> > > @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         gem_write(i915, batch, 0, cs, sizeof(cs));
> > >  
> > >         obj.handle = batch;
> > > -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> > > +       obj.offset += alignment; /* make sure we don't cause an eviction! 
> */
> 
> offset += (one page for an object + one page for stride)
> 
> > It's 'stride' here. It's leaving a guard page in between, just in case
> > page coloring demands it.
> 
> Assuming I correctly understood the intentions here but my implementation 
> is not readable clearly enough, how do you think this should be fixed?  How 
> about a comment '/* one page for an object + one page for stride */' above the 
> line where the 'alignment' variable is assigned the value of 2 * minimum GTT 
> alignment?

I think "stride = 2 * min_alignment()" concisely describes the intent.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

On Monday, November 4, 2019 10:28:37 AM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-11-04 09:23:12)
> > Hi Chris,
> > 
> > On Friday, November 1, 2019 11:08:45 AM CET Chris Wilson wrote:
> > > Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> > > > Some tests assume 4kB offset alignment while using softpin.  That
> > > > assumption may be wrong on future GEM backends with possibly larger
> > > > minimum page sizes.  As a result, those tests may either fail on
> > > > softpin at offsets which are incorrectly aligned, may silently skip
> > > > such incorrectly aligned addresses assuming them occupied by other
> > > > users if incorrect detection method is used, or may always succeed
> > > > when examining invalid use patterns.
> > > > 
> > > > Provide a helper function that detects minimum GTT alignment.  Tests
> > > > may use it to calculate softpin offsets valid for actually used backing
> > > > store.
> > > > 
> > > > v2: Rename the helper, use 'minimum GTT alignment' term across the
> > > >     change (Chris),
> > > >   - use error numbers to distinguish between invalid offsets and
> > > >     addresses occupied by other users, then
> > > >   - simplify the code (Chris).
> > > > 
> > > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > > ---
> > > >  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  lib/ioctl_wrappers.h |  2 ++
> > > >  2 files changed, 48 insertions(+)
> > > > 
> > > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > > > index 628f8b83..f0ef8b2e 100644
> > > > --- a/lib/ioctl_wrappers.c
> > > > +++ b/lib/ioctl_wrappers.c
> > > > @@ -54,6 +54,7 @@
> > > >  #include "intel_io.h"
> > > >  #include "igt_debugfs.h"
> > > >  #include "igt_sysfs.h"
> > > > +#include "igt_x86.h"
> > > >  #include "config.h"
> > > >  
> > > >  #ifdef HAVE_VALGRIND
> > > > @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> > > >         return has_softpin;
> > > >  }
> > > >  
> > > > +/**
> > > > + * gem_gtt_min_alignment_order:
> > > > + * @fd: open i915 drm file descriptor
> > > > + *
> > > > + * This function detects the minimum possible alignment of a soft-pinned 
> > gem
> > > > + * object allocated from a default backing store.  It is useful for 
> > calculating
> > > > + * correctly aligned softpin offsets.
> > > > + * Since size order to size conversion (size = 1 << order) is less 
> > trivial
> > > > + * than the opposite, the function returns the alignment order as more 
> > handy.
> > > > + *
> > > > + * Returns:
> > > > + * Size order of the minimum GTT alignment
> > > > + */
> > > > +int gem_gtt_min_alignment_order(int fd)
> > > 
> > > But not part of ioctl_wrappers.c!
> > > 
> > > lib/i915/gem_gtt_topology.c? _query.c? _probe.c?
> > 
> > I was thinking about that but couldn't find a good name.  I'll use one of your 
> > proposed, thanks.
> > 
> > > > +{
> > > > +       struct drm_i915_gem_exec_object2 obj;
> > > > +       struct drm_i915_gem_execbuffer2 eb;
> > > > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > > > +       int order;
> > > > +
> > > > +       /* no softpin => 4kB page size */
> > > > +       if (!gem_has_softpin(fd))
> > > > +               return 12;
> > > > +
> > > > +       memset(&obj, 0, sizeof(obj));
> > > > +       memset(&eb, 0, sizeof(eb));
> > > > +
> > > > +       obj.handle = gem_create(fd, 4096);
> > > > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > > > +       eb.buffers_ptr = to_user_pointer(&obj);
> > > > +       eb.buffer_count = 1;
> > > > +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> > > > +
> > > > +       for (order = 12; order < 64; order++) {
> > > > +               obj.offset = 1ull << order;
> > > > +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> > > > +                       break;
> > 
> > Should I also follow your advice of checking for -ENOSPC here rather than 
> > !-EINVAL?
> 
> This time we expect EINVAL for the invalid non-aligned offset provided by
> the user. -ENOSPC comes later when we fail to evict -- but we will only
> try that with a valid GTT node.

OK, I thought that was more about proper handling of possible error codes 
other than -EINVAL or -ENOSPC.

Thanks,
Janusz

> -Chris
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

On Monday, November 4, 2019 10:28:37 AM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-11-04 09:23:12)
> > Hi Chris,
> > 
> > On Friday, November 1, 2019 11:08:45 AM CET Chris Wilson wrote:
> > > Quoting Janusz Krzysztofik (2019-10-31 15:28:55)
> > > > Some tests assume 4kB offset alignment while using softpin.  That
> > > > assumption may be wrong on future GEM backends with possibly larger
> > > > minimum page sizes.  As a result, those tests may either fail on
> > > > softpin at offsets which are incorrectly aligned, may silently skip
> > > > such incorrectly aligned addresses assuming them occupied by other
> > > > users if incorrect detection method is used, or may always succeed
> > > > when examining invalid use patterns.
> > > > 
> > > > Provide a helper function that detects minimum GTT alignment.  Tests
> > > > may use it to calculate softpin offsets valid for actually used backing
> > > > store.
> > > > 
> > > > v2: Rename the helper, use 'minimum GTT alignment' term across the
> > > >     change (Chris),
> > > >   - use error numbers to distinguish between invalid offsets and
> > > >     addresses occupied by other users, then
> > > >   - simplify the code (Chris).
> > > > 
> > > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > > ---
> > > >  lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  lib/ioctl_wrappers.h |  2 ++
> > > >  2 files changed, 48 insertions(+)
> > > > 
> > > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > > > index 628f8b83..f0ef8b2e 100644
> > > > --- a/lib/ioctl_wrappers.c
> > > > +++ b/lib/ioctl_wrappers.c
> > > > @@ -54,6 +54,7 @@
> > > >  #include "intel_io.h"
> > > >  #include "igt_debugfs.h"
> > > >  #include "igt_sysfs.h"
> > > > +#include "igt_x86.h"
> > > >  #include "config.h"
> > > >  
> > > >  #ifdef HAVE_VALGRIND
> > > > @@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> > > >         return has_softpin;
> > > >  }
> > > >  
> > > > +/**
> > > > + * gem_gtt_min_alignment_order:
> > > > + * @fd: open i915 drm file descriptor
> > > > + *
> > > > + * This function detects the minimum possible alignment of a soft-pinned 
> > gem
> > > > + * object allocated from a default backing store.  It is useful for 
> > calculating
> > > > + * correctly aligned softpin offsets.
> > > > + * Since size order to size conversion (size = 1 << order) is less 
> > trivial
> > > > + * than the opposite, the function returns the alignment order as more 
> > handy.
> > > > + *
> > > > + * Returns:
> > > > + * Size order of the minimum GTT alignment
> > > > + */
> > > > +int gem_gtt_min_alignment_order(int fd)
> > > 
> > > But not part of ioctl_wrappers.c!
> > > 
> > > lib/i915/gem_gtt_topology.c? _query.c? _probe.c?
> > 
> > I was thinking about that but couldn't find a good name.  I'll use one of your 
> > proposed, thanks.
> > 
> > > > +{
> > > > +       struct drm_i915_gem_exec_object2 obj;
> > > > +       struct drm_i915_gem_execbuffer2 eb;
> > > > +       const uint32_t bbe = MI_BATCH_BUFFER_END;
> > > > +       int order;
> > > > +
> > > > +       /* no softpin => 4kB page size */
> > > > +       if (!gem_has_softpin(fd))
> > > > +               return 12;
> > > > +
> > > > +       memset(&obj, 0, sizeof(obj));
> > > > +       memset(&eb, 0, sizeof(eb));
> > > > +
> > > > +       obj.handle = gem_create(fd, 4096);
> > > > +       obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > > > +       eb.buffers_ptr = to_user_pointer(&obj);
> > > > +       eb.buffer_count = 1;
> > > > +       gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> > > > +
> > > > +       for (order = 12; order < 64; order++) {
> > > > +               obj.offset = 1ull << order;
> > > > +               if (__gem_execbuf(fd, &eb) != -EINVAL)
> > > > +                       break;
> > 
> > Should I also follow your advice of checking for -ENOSPC here rather than 
> > !-EINVAL?
> 
> This time we expect EINVAL for the invalid non-aligned offset provided by
> the user. -ENOSPC comes later when we fail to evict -- but we will only
> try that with a valid GTT node.

OK, I thought that was more about proper handling of possible error codes 
other than -EINVAL or -ENOSPC.

Thanks,
Janusz

> -Chris
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

Hi Vanshi,

On Thursday, October 31, 2019 5:58:31 PM CET Vanshidhar Konda wrote:
> On Thu, Oct 31, 2019 at 04:28:55PM +0100, Janusz Krzysztofik wrote:
> >Some tests assume 4kB offset alignment while using softpin.  That
> >assumption may be wrong on future GEM backends with possibly larger
> >minimum page sizes.  As a result, those tests may either fail on
> >softpin at offsets which are incorrectly aligned, may silently skip
> >such incorrectly aligned addresses assuming them occupied by other
> >users if incorrect detection method is used, or may always succeed
> >when examining invalid use patterns.
> >
> >Provide a helper function that detects minimum GTT alignment.  Tests
> >may use it to calculate softpin offsets valid for actually used backing
> >store.
> >
> >v2: Rename the helper, use 'minimum GTT alignment' term across the
> >    change (Chris),
> >  - use error numbers to distinguish between invalid offsets and
> >    addresses occupied by other users, then
> >  - simplify the code (Chris).
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >Cc: Stuart Summers <stuart.summers@intel.com>
> >---
> > lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > lib/ioctl_wrappers.h |  2 ++
> > 2 files changed, 48 insertions(+)
> >
> >diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> >index 628f8b83..f0ef8b2e 100644
> >--- a/lib/ioctl_wrappers.c
> >+++ b/lib/ioctl_wrappers.c
> >@@ -54,6 +54,7 @@
> > #include "intel_io.h"
> > #include "igt_debugfs.h"
> > #include "igt_sysfs.h"
> >+#include "igt_x86.h"
> > #include "config.h"
> >
> > #ifdef HAVE_VALGRIND
> >@@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> > 	return has_softpin;
> > }
> >
> >+/**
> >+ * gem_gtt_min_alignment_order:
> >+ * @fd: open i915 drm file descriptor
> >+ *
> >+ * This function detects the minimum possible alignment of a soft-pinned gem
> >+ * object allocated from a default backing store.  It is useful for calculating
> >+ * correctly aligned softpin offsets.
> >+ * Since size order to size conversion (size = 1 << order) is less trivial
> >+ * than the opposite, the function returns the alignment order as more handy.
> >+ *
> >+ * Returns:
> >+ * Size order of the minimum GTT alignment
> >+ */
> >+int gem_gtt_min_alignment_order(int fd)
> >+{
> >+	struct drm_i915_gem_exec_object2 obj;
> >+	struct drm_i915_gem_execbuffer2 eb;
> >+	const uint32_t bbe = MI_BATCH_BUFFER_END;
> >+	int order;
> >+
> >+	/* no softpin => 4kB page size */
> >+	if (!gem_has_softpin(fd))
> >+		return 12;
> >+
> >+	memset(&obj, 0, sizeof(obj));
> >+	memset(&eb, 0, sizeof(eb));
> >+
> >+	obj.handle = gem_create(fd, 4096);
> >+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >+	eb.buffers_ptr = to_user_pointer(&obj);
> >+	eb.buffer_count = 1;
> >+	gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> 
> I think it will be safer to create a new context to execute this
> execbuffer. For a new context the address space should be empty reducing
> the chance that there is another object mapped by the caller of the
> helper function at the address we start testing.

AFAICU, that shouldn't matter.  Object attributes are validated and possibly 
-EINVAL is returned already before an eviction is possibly attempted.  The 
algorithm assumes an offset is valid if any error code other than -EINVAL 
(including no error) is returned.

Thanks,
Janusz

> 
> Otherwise it looks good to me.
> 
> Vanshi
> 
> >+
> >+	for (order = 12; order < 64; order++) {
> >+		obj.offset = 1ull << order;
> >+		if (__gem_execbuf(fd, &eb) != -EINVAL)
> >+			break;
> >+	}
> >+	igt_assert(obj.offset < gem_aperture_size(fd));
> >+
> >+	gem_close(fd, obj.handle);
> >+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
> >+	return order;
> >+}
> >+
> > /**
> >  * gem_has_exec_fence:
> >  * @fd: open i915 drm file descriptor
> >diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> >index 03211c97..c8d57a7c 100644
> >--- a/lib/ioctl_wrappers.h
> >+++ b/lib/ioctl_wrappers.h
> >@@ -138,6 +138,8 @@ uint64_t gem_aperture_size(int fd);
> > uint64_t gem_global_aperture_size(int fd);
> > uint64_t gem_mappable_aperture_size(void);
> > bool gem_has_softpin(int fd);
> >+int gem_gtt_min_alignment_order(int fd);
> >+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
> > bool gem_has_exec_fence(int fd);
> >
> > /* check functions which auto-skip tests by calling igt_skip() */
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t v4 2/4] lib: Add minimum GTT alignment helper

[Janusz Krzysztofik]

Hi Vanshi,

On Thursday, October 31, 2019 5:58:31 PM CET Vanshidhar Konda wrote:
> On Thu, Oct 31, 2019 at 04:28:55PM +0100, Janusz Krzysztofik wrote:
> >Some tests assume 4kB offset alignment while using softpin.  That
> >assumption may be wrong on future GEM backends with possibly larger
> >minimum page sizes.  As a result, those tests may either fail on
> >softpin at offsets which are incorrectly aligned, may silently skip
> >such incorrectly aligned addresses assuming them occupied by other
> >users if incorrect detection method is used, or may always succeed
> >when examining invalid use patterns.
> >
> >Provide a helper function that detects minimum GTT alignment.  Tests
> >may use it to calculate softpin offsets valid for actually used backing
> >store.
> >
> >v2: Rename the helper, use 'minimum GTT alignment' term across the
> >    change (Chris),
> >  - use error numbers to distinguish between invalid offsets and
> >    addresses occupied by other users, then
> >  - simplify the code (Chris).
> >
> >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >Cc: Stuart Summers <stuart.summers@intel.com>
> >---
> > lib/ioctl_wrappers.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
> > lib/ioctl_wrappers.h |  2 ++
> > 2 files changed, 48 insertions(+)
> >
> >diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> >index 628f8b83..f0ef8b2e 100644
> >--- a/lib/ioctl_wrappers.c
> >+++ b/lib/ioctl_wrappers.c
> >@@ -54,6 +54,7 @@
> > #include "intel_io.h"
> > #include "igt_debugfs.h"
> > #include "igt_sysfs.h"
> >+#include "igt_x86.h"
> > #include "config.h"
> >
> > #ifdef HAVE_VALGRIND
> >@@ -1158,6 +1159,51 @@ bool gem_has_softpin(int fd)
> > 	return has_softpin;
> > }
> >
> >+/**
> >+ * gem_gtt_min_alignment_order:
> >+ * @fd: open i915 drm file descriptor
> >+ *
> >+ * This function detects the minimum possible alignment of a soft-pinned gem
> >+ * object allocated from a default backing store.  It is useful for calculating
> >+ * correctly aligned softpin offsets.
> >+ * Since size order to size conversion (size = 1 << order) is less trivial
> >+ * than the opposite, the function returns the alignment order as more handy.
> >+ *
> >+ * Returns:
> >+ * Size order of the minimum GTT alignment
> >+ */
> >+int gem_gtt_min_alignment_order(int fd)
> >+{
> >+	struct drm_i915_gem_exec_object2 obj;
> >+	struct drm_i915_gem_execbuffer2 eb;
> >+	const uint32_t bbe = MI_BATCH_BUFFER_END;
> >+	int order;
> >+
> >+	/* no softpin => 4kB page size */
> >+	if (!gem_has_softpin(fd))
> >+		return 12;
> >+
> >+	memset(&obj, 0, sizeof(obj));
> >+	memset(&eb, 0, sizeof(eb));
> >+
> >+	obj.handle = gem_create(fd, 4096);
> >+	obj.flags = EXEC_OBJECT_PINNED | EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >+	eb.buffers_ptr = to_user_pointer(&obj);
> >+	eb.buffer_count = 1;
> >+	gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
> 
> I think it will be safer to create a new context to execute this
> execbuffer. For a new context the address space should be empty reducing
> the chance that there is another object mapped by the caller of the
> helper function at the address we start testing.

AFAICU, that shouldn't matter.  Object attributes are validated and possibly 
-EINVAL is returned already before an eviction is possibly attempted.  The 
algorithm assumes an offset is valid if any error code other than -EINVAL 
(including no error) is returned.

Thanks,
Janusz

> 
> Otherwise it looks good to me.
> 
> Vanshi
> 
> >+
> >+	for (order = 12; order < 64; order++) {
> >+		obj.offset = 1ull << order;
> >+		if (__gem_execbuf(fd, &eb) != -EINVAL)
> >+			break;
> >+	}
> >+	igt_assert(obj.offset < gem_aperture_size(fd));
> >+
> >+	gem_close(fd, obj.handle);
> >+	igt_debug("minimum GTT alignment is %#llx\n", (long long)obj.offset);
> >+	return order;
> >+}
> >+
> > /**
> >  * gem_has_exec_fence:
> >  * @fd: open i915 drm file descriptor
> >diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> >index 03211c97..c8d57a7c 100644
> >--- a/lib/ioctl_wrappers.h
> >+++ b/lib/ioctl_wrappers.h
> >@@ -138,6 +138,8 @@ uint64_t gem_aperture_size(int fd);
> > uint64_t gem_global_aperture_size(int fd);
> > uint64_t gem_mappable_aperture_size(void);
> > bool gem_has_softpin(int fd);
> >+int gem_gtt_min_alignment_order(int fd);
> >+#define gem_gtt_min_alignment(fd) (1ull << gem_gtt_min_alignment_order(fd))
> > bool gem_has_exec_fence(int fd);
> >
> > /* check functions which auto-skip tests by calling igt_skip() */
> 




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx