| Message ID | 20191104171330.24821-3-janusz.krzysztofik@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Calculate softpin offsets from minimum GTT alignment | expand |
On Mon, Nov 04, 2019 at 06:13:28PM +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. If incorrect GTT alignment will be 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 reported as occupied. > >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. >v4: Silently skip only those offsets which have been explicitly > reported as overlapping with shared GTT reserved space, not simply > all which raise failures other than -EINVAL (Chris), > - as an implementation of moving the probe out of line so it's not > easily confused with the central point of the test (Chris), use > object validation library helper just introduced. > >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> >Cc: Chris Wilson <chris@chris-wilson.co.uk> >--- > tests/i915/gem_exec_reloc.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > >diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c >index fdd9661d..e46a4df7 100644 >--- a/tests/i915/gem_exec_reloc.c >+++ b/tests/i915/gem_exec_reloc.c >@@ -23,6 +23,7 @@ > > #include "igt.h" > #include "igt_dummyload.h" >+#include "i915/gem_gtt_topology.c" > > IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations."); > >@@ -520,7 +521,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)); > >@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags) > obj[n].offset = (1ull << (i + 12)) - 4096; > 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)); >- execbuf.buffers_ptr = to_user_pointer(&obj[n]); >- execbuf.buffer_count = 1; >- if (__gem_execbuf(fd, &execbuf)) >+ err = gem_gtt_validate_object(fd, &obj[n]); Would it be better to name this function as gem_gtt_validate_object_offset? From the earlier code is it was easy to see that the intention of the test was to use gem_execbuf to map the object to the offset. From the new method name unless I check the code it's not straight forward that we are just checking the offset. >+ if (err) { >+ /* Iff using a shared GTT, some of it may be reserved */ Nit-pick: If, not Iff >+ igt_assert_eq(err, -ENOSPC); > continue; >+ } > > igt_debug("obj[%d] handle=%d, address=%llx\n", > n, obj[n].handle, (long long)obj[n].offset); >@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags) > obj[n].offset = 1ull << (i + 12); > 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)); >- execbuf.buffers_ptr = to_user_pointer(&obj[n]); >- execbuf.buffer_count = 1; >- if (__gem_execbuf(fd, &execbuf)) >+ err = gem_gtt_validate_object(fd, &obj[n]); >+ if (err) { >+ /* Iff using a shared GTT, some of it may be reserved */ Nit-pick: If, not Iff >+ igt_assert_eq(err, -ENOSPC); > continue; >+ } > > igt_debug("obj[%d] handle=%d, address=%llx\n", > n, obj[n].handle, (long long)obj[n].offset); >-- Other than the above comments, looks good to me. Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com> Vanshi >2.21.0 >
Hi, On Monday, November 4, 2019 9:46:28 PM CET Vanshidhar Konda wrote: > On Mon, Nov 04, 2019 at 06:13:28PM +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. If incorrect GTT alignment will be 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 reported as occupied. > > > >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. > >v4: Silently skip only those offsets which have been explicitly > > reported as overlapping with shared GTT reserved space, not simply > > all which raise failures other than -EINVAL (Chris), > > - as an implementation of moving the probe out of line so it's not > > easily confused with the central point of the test (Chris), use > > object validation library helper just introduced. > > > >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > >Cc: Chris Wilson <chris@chris-wilson.co.uk> > >--- > > tests/i915/gem_exec_reloc.c | 21 ++++++++++++--------- > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > >diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c > >index fdd9661d..e46a4df7 100644 > >--- a/tests/i915/gem_exec_reloc.c > >+++ b/tests/i915/gem_exec_reloc.c > >@@ -23,6 +23,7 @@ > > > > #include "igt.h" > > #include "igt_dummyload.h" > >+#include "i915/gem_gtt_topology.c" Auto correction: s/.c/.h/ > > > > IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations."); > > > >@@ -520,7 +521,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)); > > > >@@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags) > > obj[n].offset = (1ull << (i + 12)) - 4096; > > 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)); > >- execbuf.buffers_ptr = to_user_pointer(&obj[n]); > >- execbuf.buffer_count = 1; > >- if (__gem_execbuf(fd, &execbuf)) > >+ err = gem_gtt_validate_object(fd, &obj[n]); > > Would it be better to name this function as > gem_gtt_validate_object_offset? From the earlier code is it was easy to > see that the intention of the test was to use gem_execbuf to map the > object to the offset. From the new method name unless I check the code > it's not straight forward that we are just checking the offset. > > >+ if (err) { > >+ /* Iff using a shared GTT, some of it may be reserved */ > > Nit-pick: If, not Iff Hmm, I've assumed Chris' English is better than mine and I'm using his spelling from one of his comments. AFAICT, 'iff' is an abbreviation of 'if and only if' used in a math slang. Thanks, Janusz > >+ igt_assert_eq(err, -ENOSPC); > > continue; > >+ } > > > > igt_debug("obj[%d] handle=%d, address=%llx\n", > > n, obj[n].handle, (long long)obj[n].offset); > >@@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags) > > obj[n].offset = 1ull << (i + 12); > > 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)); > >- execbuf.buffers_ptr = to_user_pointer(&obj[n]); > >- execbuf.buffer_count = 1; > >- if (__gem_execbuf(fd, &execbuf)) > >+ err = gem_gtt_validate_object(fd, &obj[n]); > >+ if (err) { > >+ /* Iff using a shared GTT, some of it may be reserved */ > > Nit-pick: If, not Iff > > >+ igt_assert_eq(err, -ENOSPC); > > continue; > >+ } > > > > igt_debug("obj[%d] handle=%d, address=%llx\n", > > n, obj[n].handle, (long long)obj[n].offset); > > Other than the above comments, looks good to me. > Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com> > > Vanshi > > >2.21.0 > > >
diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c index fdd9661d..e46a4df7 100644 --- a/tests/i915/gem_exec_reloc.c +++ b/tests/i915/gem_exec_reloc.c @@ -23,6 +23,7 @@ #include "igt.h" #include "igt_dummyload.h" +#include "i915/gem_gtt_topology.c" IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl relocations."); @@ -520,7 +521,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)); @@ -539,11 +540,12 @@ static void basic_range(int fd, unsigned flags) obj[n].offset = (1ull << (i + 12)) - 4096; 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)); - execbuf.buffers_ptr = to_user_pointer(&obj[n]); - execbuf.buffer_count = 1; - if (__gem_execbuf(fd, &execbuf)) + err = gem_gtt_validate_object(fd, &obj[n]); + if (err) { + /* Iff using a shared GTT, some of it may be reserved */ + igt_assert_eq(err, -ENOSPC); continue; + } igt_debug("obj[%d] handle=%d, address=%llx\n", n, obj[n].handle, (long long)obj[n].offset); @@ -559,11 +561,12 @@ static void basic_range(int fd, unsigned flags) obj[n].offset = 1ull << (i + 12); 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)); - execbuf.buffers_ptr = to_user_pointer(&obj[n]); - execbuf.buffer_count = 1; - if (__gem_execbuf(fd, &execbuf)) + err = gem_gtt_validate_object(fd, &obj[n]); + if (err) { + /* Iff using a shared GTT, some of it may be reserved */ + igt_assert_eq(err, -ENOSPC); continue; + } igt_debug("obj[%d] handle=%d, address=%llx\n", n, obj[n].handle, (long long)obj[n].offset);
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. If incorrect GTT alignment will be 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 reported as occupied. 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. v4: Silently skip only those offsets which have been explicitly reported as overlapping with shared GTT reserved space, not simply all which raise failures other than -EINVAL (Chris), - as an implementation of moving the probe out of line so it's not easily confused with the central point of the test (Chris), use object validation library helper just introduced. Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- tests/i915/gem_exec_reloc.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)