[PATCH i-g-t] igt/gem_exec_capture: Fix command emission for gen3

[PATCH i-g-t] igt/gem_exec_capture: Fix command emission for gen3

[Chris Wilson]

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

Re: [PATCH i-g-t] igt/gem_exec_capture: Fix command emission for gen3

[Chris Wilson]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] igt/gem_exec_capture: Fix command emission for gen3

[Joonas Lahtinen]

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;
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] igt/gem_exec_capture: Fix command emission for gen3

[Ville Syrjälä]

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

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