| Message ID | 20180909124308.24391-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] igt/gem_exec_capture: Fix command emission for gen3 | expand |
Quoting Chris Wilson (2018-09-09 13:43:08) > A missing no-op causing us to emit the wrong address when relocation was > required for BB_START. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106078 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106028 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/gem_exec_capture.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tests/gem_exec_capture.c b/tests/gem_exec_capture.c > index 2dc06ce43..9c26e12c9 100644 > --- a/tests/gem_exec_capture.c > +++ b/tests/gem_exec_capture.c > @@ -117,7 +117,7 @@ static void __capture(int fd, int dir, unsigned ring, uint32_t target) > batch[++i] = 0; > } > batch[++i] = 0xc0ffee; > - if (gen < 3) > + if (gen <= 3) > batch[++i] = MI_NOOP; > > batch[++i] = MI_BATCH_BUFFER_START; /* not crashed? try again! */ > @@ -144,10 +144,12 @@ static void __capture(int fd, int dir, unsigned ring, uint32_t target) > execbuf.flags = ring; > if (gen > 3 && gen < 6) > execbuf.flags |= I915_EXEC_SECURE; > + > + igt_assert(!READ_ONCE(*seqno)); > gem_execbuf(fd, &execbuf); > > /* Wait for the request to start */ > - while (*(volatile uint32_t *)seqno != 0xc0ffee) > + while (READ_ONCE(*seqno) != 0xc0ffee) > igt_assert(gem_bo_busy(fd, obj[SCRATCH].handle)); > munmap(seqno, 4096); > > -- > 2.19.0.rc2 >
Quoting Chris Wilson (2018-09-09 15:43:08) > A missing no-op causing us to emit the wrong address when relocation was > required for BB_START. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/gem_exec_capture.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tests/gem_exec_capture.c b/tests/gem_exec_capture.c > index 2dc06ce43..9c26e12c9 100644 > --- a/tests/gem_exec_capture.c > +++ b/tests/gem_exec_capture.c > @@ -117,7 +117,7 @@ static void __capture(int fd, int dir, unsigned ring, uint32_t target) > batch[++i] = 0; > } > batch[++i] = 0xc0ffee; > - if (gen < 3) > + if (gen <= 3) "gen < 4" would be more consistent with the usual checks. One would assume the evolution of the code to be that this if was added during development of Gen4 when it's not needed anymore. Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas > batch[++i] = MI_NOOP; >
On Sun, Sep 09, 2018 at 01:43:08PM +0100, Chris Wilson wrote: > A missing no-op causing us to emit the wrong address when relocation was > required for BB_START. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/gem_exec_capture.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tests/gem_exec_capture.c b/tests/gem_exec_capture.c > index 2dc06ce43..9c26e12c9 100644 > --- a/tests/gem_exec_capture.c > +++ b/tests/gem_exec_capture.c > @@ -117,7 +117,7 @@ static void __capture(int fd, int dir, unsigned ring, uint32_t target) > batch[++i] = 0; > } > batch[++i] = 0xc0ffee; > - if (gen < 3) > + if (gen <= 3) > batch[++i] = MI_NOOP; This code is rather well obfuscated. Removing the length from MI_STORE_DWORD_IMM and just setting it correctly in each branch would go a long way towards making things more obvious. > > batch[++i] = MI_BATCH_BUFFER_START; /* not crashed? try again! */ > @@ -144,10 +144,12 @@ static void __capture(int fd, int dir, unsigned ring, uint32_t target) > execbuf.flags = ring; > if (gen > 3 && gen < 6) > execbuf.flags |= I915_EXEC_SECURE; > + > + igt_assert(!READ_ONCE(*seqno)); > gem_execbuf(fd, &execbuf); > > /* Wait for the request to start */ > - while (*(volatile uint32_t *)seqno != 0xc0ffee) > + while (READ_ONCE(*seqno) != 0xc0ffee) > igt_assert(gem_bo_busy(fd, obj[SCRATCH].handle)); > munmap(seqno, 4096); > > -- > 2.19.0.rc2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/tests/gem_exec_capture.c b/tests/gem_exec_capture.c index 2dc06ce43..9c26e12c9 100644 --- a/tests/gem_exec_capture.c +++ b/tests/gem_exec_capture.c @@ -117,7 +117,7 @@ static void __capture(int fd, int dir, unsigned ring, uint32_t target) batch[++i] = 0; } batch[++i] = 0xc0ffee; - if (gen < 3) + if (gen <= 3) batch[++i] = MI_NOOP; batch[++i] = MI_BATCH_BUFFER_START; /* not crashed? try again! */ @@ -144,10 +144,12 @@ static void __capture(int fd, int dir, unsigned ring, uint32_t target) execbuf.flags = ring; if (gen > 3 && gen < 6) execbuf.flags |= I915_EXEC_SECURE; + + igt_assert(!READ_ONCE(*seqno)); gem_execbuf(fd, &execbuf); /* Wait for the request to start */ - while (*(volatile uint32_t *)seqno != 0xc0ffee) + while (READ_ONCE(*seqno) != 0xc0ffee) igt_assert(gem_bo_busy(fd, obj[SCRATCH].handle)); munmap(seqno, 4096);
A missing no-op causing us to emit the wrong address when relocation was required for BB_START. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- tests/gem_exec_capture.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)