| Message ID | 20191031152857.17143-2-janusz.krzysztofik@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Calculate softpin offsets from minimum GTT alignment | expand |
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 >
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; > + }
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; > > + } >
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); >
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
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);
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(-)