[i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
diff mbox series

Message ID 20191205012238.214924-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
Related show

Commit Message

Chris Wilson Dec. 5, 2019, 1:22 a.m. UTC
Correct the COND_BBEND instruction to perform the compare and apply the
relocation so that it looks at the correct address. In the process,
prepare for pipelined failures.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
 1 file changed, 61 insertions(+), 55 deletions(-)

Comments

Mika Kuoppala Dec. 5, 2019, 10:18 a.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Correct the COND_BBEND instruction to perform the compare and apply the
> relocation so that it looks at the correct address. In the process,
> prepare for pipelined failures.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
>  1 file changed, 61 insertions(+), 55 deletions(-)
>
> diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
> index 58d1b2b32..854c59863 100644
> --- a/tests/i915/gem_exec_parse_blt.c
> +++ b/tests/i915/gem_exec_parse_blt.c
> @@ -30,6 +30,7 @@
>  
>  #include "igt.h"
>  #include "i915/gem_submission.h"
> +#include "sw_sync.h"
>  
>  /* To help craft commands known to be invalid across all engines */
>  #define INSTR_CLIENT_SHIFT	29
> @@ -53,6 +54,30 @@
>  
>  #define HANDLE_SIZE  4096
>  
> +static int
> +__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
> +{
> +	int fence;
> +	int err;
> +
> +	igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));

s/!// ?

> +	eb->flags |= I915_EXEC_FENCE_OUT;
> +	err = __gem_execbuf_wr(i915, eb);
> +	eb->flags &= ~I915_EXEC_FENCE_OUT;
> +	if (err)
> +		return err;
> +
> +	fence = eb->rsvd2 >> 32;
> +
> +	sync_fence_wait(fence, -1);
> +	err = sync_fence_status(fence);
> +	close(fence);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +
>  static int
>  __exec_batch_patched(int i915, int engine,
>  		     uint32_t cmd_bo, const uint32_t *cmds, int size,
> @@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
>  	execbuf.batch_len = size;
>  	execbuf.flags = engine;
>  
> -	return __gem_execbuf(i915, &execbuf);
> +	return __checked_execbuf(i915, &execbuf);
>  }
>  
>  static void exec_batch_patched(int i915, int engine,
> @@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
>  	execbuf.batch_len = size;
>  	execbuf.flags = engine;
>  
> -	return  __gem_execbuf(i915, &execbuf);
> +	return  __checked_execbuf(i915, &execbuf);
>  }
>  
>  #if 0
> @@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
>  		      0x8);
>  	execbuf.flags = engine;
>  
> -	igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
> +	igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
>  
>  	gem_close(i915, cmd_bo);
>  }
> @@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
>  	execbuf.batch_len = sizeof(first_level_cmds);
>  	execbuf.flags = engine;
>  
> -	ret = __gem_execbuf(i915, &execbuf);
> +	ret = __checked_execbuf(i915, &execbuf);
>  	if (expected_return && ret == expected_return)
>  		goto out;
>  
> @@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
>  	execbuf.batch_len = sizeof(batch_secure);
>  	execbuf.flags = I915_EXEC_BLT;
>  
> -	igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
> +	igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
>  }
>  
>  #define BB_START_PARAM 0
> @@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>  {
>  	struct drm_i915_gem_execbuffer2 execbuf;
>  	struct drm_i915_gem_exec_object2 obj[2];
> -	struct drm_i915_gem_relocation_entry reloc[3];
> +	struct drm_i915_gem_relocation_entry reloc[4];
>  	const uint32_t target_bo = gem_create(i915, 4096);
> -	uint32_t *dst;
> -	int ret;
>  	unsigned int jump_off, footer_pos;
> -	const uint32_t batch_header[] = {
> +	uint32_t batch[1024] = {
>  		MI_NOOP,
>  		MI_NOOP,
>  		MI_NOOP,
> @@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>  		4,
>  		0,
>  		2,
> -		MI_COND_BATCH_BUFFER_END | 1,
> +		MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
>  		0,
>  		0,
> -		0
> +		0,
> +		MI_ARB_CHECK,
>  	};
>  	const uint32_t batch_footer[] = {
>  		MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
> @@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>  		0,
>  		MI_BATCH_BUFFER_END,
>  	};
> -	uint32_t batch[1024];
> +	uint32_t *dst;
>  
>  	igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
>  
> -	memset(batch, 0, sizeof(batch));
> -	memcpy(batch, batch_header, sizeof(batch_header));
> -
>  	switch (test) {
>  	case BB_START_PARAM:
>  		jump_off = 5 * sizeof(uint32_t);
> @@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>  		break;
>  	default:
>  		jump_off = 0xf00d0000;
> +		break;
>  	}
>  
>  	if (test == BB_START_FAR)
> -		footer_pos = (sizeof(batch) - sizeof(batch_footer));
> +		footer_pos = sizeof(batch) - sizeof(batch_footer);
>  	else
> -		footer_pos = sizeof(batch_header);
> +		footer_pos = 17 * sizeof(uint32_t);
>  
>  	memcpy(batch + footer_pos / sizeof(uint32_t),
>  	       batch_footer, sizeof(batch_footer));
> @@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>  	reloc[0].delta = 0;
>  	reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
>  	reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
> -	reloc[0].presumed_offset = -1;
>  
>  	reloc[1].offset = 9 * sizeof(uint32_t);
>  	reloc[1].target_handle = obj[0].handle;
>  	reloc[1].delta = 1 * sizeof(uint32_t);
>  	reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
>  	reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
> -	reloc[1].presumed_offset = -1;
>  
> -	reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
> -	reloc[2].target_handle = obj[1].handle;
> -	reloc[2].delta = jump_off;
> +	reloc[2].offset = 14 * sizeof(uint32_t);
> +	reloc[2].target_handle = obj[0].handle;
> +	reloc[2].delta = 0;
>  	reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
>  	reloc[2].write_domain = 0;
> -	reloc[2].presumed_offset = -1;
> +
> +	reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
> +	reloc[3].target_handle = obj[1].handle;
> +	reloc[3].delta = jump_off;
> +	reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
> +	reloc[3].write_domain = 0;
> +	reloc[3].presumed_offset = -1;

Why we need to set the presumed only in this last reloc?

-Mika

>  
>  	obj[1].relocs_ptr = to_user_pointer(reloc);
> -	obj[1].relocation_count = 3;
> +	obj[1].relocation_count = ARRAY_SIZE(reloc);
>  
>  	memset(&execbuf, 0, sizeof(execbuf));
>  	execbuf.buffers_ptr = to_user_pointer(obj);
> @@ -506,21 +532,20 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>  	execbuf.batch_len = sizeof(batch);
>  	execbuf.flags = I915_EXEC_BLT;
>  
> -	dst = gem_mmap__cpu(i915, obj[0].handle, 0, 4096,
> -			    PROT_READ | PROT_WRITE);
> +	dst = gem_mmap__wc(i915, obj[0].handle, 0, 4096, PROT_WRITE);
>  
>  	igt_assert_eq(dst[0], 0);
>  	igt_assert_eq(dst[1], 0);
>  
> -	ret = __gem_execbuf(i915, &execbuf);
> -
>  	switch (test) {
>  	case BB_START_PARAM:
> -		igt_assert_eq(ret, -EINVAL);
> +	case BB_START_OUT:
> +		igt_assert_eq(__checked_execbuf(i915, &execbuf), -EINVAL);
>  		break;
> +
>  	case BB_START_CMD:
>  	case BB_START_FAR:
> -		igt_assert_eq(ret, 0);
> +		gem_execbuf(i915, &execbuf);
>  
>  		while (READ_ONCE(dst[0]) == 0)
>  		       ;
> @@ -531,19 +556,8 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>  		igt_assert_eq(dst[0], 1);
>  		igt_assert_eq(dst[1], 2);
>  
> -		igt_info("values now %x %x\n", dst[0], dst[1]);
> -
>  		dst[0] = 0;
> -
> -		igt_info("values now %x %x\n", dst[0], dst[1]);
> -
> -		igt_assert_eq(dst[0], 0);
> -		igt_assert_eq(dst[1], 2);
> -
> -		break;
> -
> -	case BB_START_OUT:
> -		igt_assert_eq(ret, -EINVAL);
> +		__sync_synchronize();
>  		break;
>  	}
>  
> @@ -896,7 +910,7 @@ static void test_unaligned_jump(const int i915, const uint32_t handle)
>  		memcpy(&batch[1], &offset, sizeof(offset));
>  		gem_write(i915, handle, 4000, batch, sizeof(batch));
>  
> -		igt_assert_f(__gem_execbuf(i915, &execbuf) == -EINVAL,
> +		igt_assert_f(__checked_execbuf(i915, &execbuf) == -EINVAL,
>  			     "unaligned jump accepted to %d; batch=%08x\n",
>  			     reloc.delta, batch[reloc.delta / 4]);
>  	}
> @@ -1032,19 +1046,11 @@ igt_main
>  	igt_subtest("unaligned-jump")
>  		test_unaligned_jump(i915, handle);
>  
> -	igt_subtest_group {
> -		igt_hang_t hang;
> -
> -		igt_fixture igt_allow_hang(i915, 0, 0);
> +	igt_subtest("bb-start-cmd")
> +		test_bb_start(i915, handle, BB_START_CMD);
>  
> -		igt_subtest("bb-start-cmd")
> -			test_bb_start(i915, handle, BB_START_CMD);
> -
> -		igt_subtest("bb-start-far")
> -			test_bb_start(i915, handle, BB_START_FAR);
> -
> -		igt_fixture igt_disallow_hang(i915, hang);
> -	}
> +	igt_subtest("bb-start-far")
> +		test_bb_start(i915, handle, BB_START_FAR);
>  
>  	igt_fixture {
>  		igt_stop_hang_detector();
> -- 
> 2.24.0
Chris Wilson Dec. 5, 2019, 10:33 a.m. UTC | #2
Quoting Mika Kuoppala (2019-12-05 10:18:34)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Correct the COND_BBEND instruction to perform the compare and apply the
> > relocation so that it looks at the correct address. In the process,
> > prepare for pipelined failures.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >  tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
> >  1 file changed, 61 insertions(+), 55 deletions(-)
> >
> > diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
> > index 58d1b2b32..854c59863 100644
> > --- a/tests/i915/gem_exec_parse_blt.c
> > +++ b/tests/i915/gem_exec_parse_blt.c
> > @@ -30,6 +30,7 @@
> >  
> >  #include "igt.h"
> >  #include "i915/gem_submission.h"
> > +#include "sw_sync.h"
> >  
> >  /* To help craft commands known to be invalid across all engines */
> >  #define INSTR_CLIENT_SHIFT   29
> > @@ -53,6 +54,30 @@
> >  
> >  #define HANDLE_SIZE  4096
> >  
> > +static int
> > +__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
> > +{
> > +     int fence;
> > +     int err;
> > +
> > +     igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
> 
> s/!// ?

It's not a BUG_ON :)

We insist that the caller isn't expecting to use the out-fence
themselves.
 
> > +     eb->flags |= I915_EXEC_FENCE_OUT;
> > +     err = __gem_execbuf_wr(i915, eb);
> > +     eb->flags &= ~I915_EXEC_FENCE_OUT;
> > +     if (err)
> > +             return err;
> > +
> > +     fence = eb->rsvd2 >> 32;
> > +
> > +     sync_fence_wait(fence, -1);
> > +     err = sync_fence_status(fence);
> > +     close(fence);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     return 0;
> > +}
> > +
> >  static int
> >  __exec_batch_patched(int i915, int engine,
> >                    uint32_t cmd_bo, const uint32_t *cmds, int size,
> > @@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
> >       execbuf.batch_len = size;
> >       execbuf.flags = engine;
> >  
> > -     return __gem_execbuf(i915, &execbuf);
> > +     return __checked_execbuf(i915, &execbuf);
> >  }
> >  
> >  static void exec_batch_patched(int i915, int engine,
> > @@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
> >       execbuf.batch_len = size;
> >       execbuf.flags = engine;
> >  
> > -     return  __gem_execbuf(i915, &execbuf);
> > +     return  __checked_execbuf(i915, &execbuf);
> >  }
> >  
> >  #if 0
> > @@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
> >                     0x8);
> >       execbuf.flags = engine;
> >  
> > -     igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
> > +     igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
> >  
> >       gem_close(i915, cmd_bo);
> >  }
> > @@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
> >       execbuf.batch_len = sizeof(first_level_cmds);
> >       execbuf.flags = engine;
> >  
> > -     ret = __gem_execbuf(i915, &execbuf);
> > +     ret = __checked_execbuf(i915, &execbuf);
> >       if (expected_return && ret == expected_return)
> >               goto out;
> >  
> > @@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
> >       execbuf.batch_len = sizeof(batch_secure);
> >       execbuf.flags = I915_EXEC_BLT;
> >  
> > -     igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
> > +     igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
> >  }
> >  
> >  #define BB_START_PARAM 0
> > @@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> >  {
> >       struct drm_i915_gem_execbuffer2 execbuf;
> >       struct drm_i915_gem_exec_object2 obj[2];
> > -     struct drm_i915_gem_relocation_entry reloc[3];
> > +     struct drm_i915_gem_relocation_entry reloc[4];
> >       const uint32_t target_bo = gem_create(i915, 4096);
> > -     uint32_t *dst;
> > -     int ret;
> >       unsigned int jump_off, footer_pos;
> > -     const uint32_t batch_header[] = {
> > +     uint32_t batch[1024] = {
> >               MI_NOOP,
> >               MI_NOOP,
> >               MI_NOOP,
> > @@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> >               4,
> >               0,
> >               2,
> > -             MI_COND_BATCH_BUFFER_END | 1,
> > +             MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
> >               0,
> >               0,
> > -             0
> > +             0,
> > +             MI_ARB_CHECK,
> >       };
> >       const uint32_t batch_footer[] = {
> >               MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
> > @@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> >               0,
> >               MI_BATCH_BUFFER_END,
> >       };
> > -     uint32_t batch[1024];
> > +     uint32_t *dst;
> >  
> >       igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
> >  
> > -     memset(batch, 0, sizeof(batch));
> > -     memcpy(batch, batch_header, sizeof(batch_header));
> > -
> >       switch (test) {
> >       case BB_START_PARAM:
> >               jump_off = 5 * sizeof(uint32_t);
> > @@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> >               break;
> >       default:
> >               jump_off = 0xf00d0000;
> > +             break;
> >       }
> >  
> >       if (test == BB_START_FAR)
> > -             footer_pos = (sizeof(batch) - sizeof(batch_footer));
> > +             footer_pos = sizeof(batch) - sizeof(batch_footer);
> >       else
> > -             footer_pos = sizeof(batch_header);
> > +             footer_pos = 17 * sizeof(uint32_t);
> >  
> >       memcpy(batch + footer_pos / sizeof(uint32_t),
> >              batch_footer, sizeof(batch_footer));
> > @@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
> >       reloc[0].delta = 0;
> >       reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
> >       reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
> > -     reloc[0].presumed_offset = -1;
> >  
> >       reloc[1].offset = 9 * sizeof(uint32_t);
> >       reloc[1].target_handle = obj[0].handle;
> >       reloc[1].delta = 1 * sizeof(uint32_t);
> >       reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
> >       reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
> > -     reloc[1].presumed_offset = -1;
> >  
> > -     reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
> > -     reloc[2].target_handle = obj[1].handle;
> > -     reloc[2].delta = jump_off;
> > +     reloc[2].offset = 14 * sizeof(uint32_t);
> > +     reloc[2].target_handle = obj[0].handle;
> > +     reloc[2].delta = 0;
> >       reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
> >       reloc[2].write_domain = 0;
> > -     reloc[2].presumed_offset = -1;
> > +
> > +     reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
> > +     reloc[3].target_handle = obj[1].handle;
> > +     reloc[3].delta = jump_off;
> > +     reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
> > +     reloc[3].write_domain = 0;
> > +     reloc[3].presumed_offset = -1;
> 
> Why we need to set the presumed only in this last reloc?

It's the only one that is _not_ preset to presumed.offset + delta.
-Chris
Mika Kuoppala Dec. 5, 2019, 10:57 a.m. UTC | #3
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-12-05 10:18:34)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > Correct the COND_BBEND instruction to perform the compare and apply the
>> > relocation so that it looks at the correct address. In the process,
>> > prepare for pipelined failures.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> >  tests/i915/gem_exec_parse_blt.c | 116 +++++++++++++++++---------------
>> >  1 file changed, 61 insertions(+), 55 deletions(-)
>> >
>> > diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
>> > index 58d1b2b32..854c59863 100644
>> > --- a/tests/i915/gem_exec_parse_blt.c
>> > +++ b/tests/i915/gem_exec_parse_blt.c
>> > @@ -30,6 +30,7 @@
>> >  
>> >  #include "igt.h"
>> >  #include "i915/gem_submission.h"
>> > +#include "sw_sync.h"
>> >  
>> >  /* To help craft commands known to be invalid across all engines */
>> >  #define INSTR_CLIENT_SHIFT   29
>> > @@ -53,6 +54,30 @@
>> >  
>> >  #define HANDLE_SIZE  4096
>> >  
>> > +static int
>> > +__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
>> > +{
>> > +     int fence;
>> > +     int err;
>> > +
>> > +     igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
>> 
>> s/!// ?
>
> It's not a BUG_ON :)

Could be the exact thought pattern of why I did got it
wrong...spooky!

>
> We insist that the caller isn't expecting to use the out-fence
> themselves.
>  
>> > +     eb->flags |= I915_EXEC_FENCE_OUT;
>> > +     err = __gem_execbuf_wr(i915, eb);
>> > +     eb->flags &= ~I915_EXEC_FENCE_OUT;
>> > +     if (err)
>> > +             return err;
>> > +
>> > +     fence = eb->rsvd2 >> 32;
>> > +
>> > +     sync_fence_wait(fence, -1);
>> > +     err = sync_fence_status(fence);
>> > +     close(fence);
>> > +     if (err < 0)
>> > +             return err;
>> > +
>> > +     return 0;
>> > +}
>> > +
>> >  static int
>> >  __exec_batch_patched(int i915, int engine,
>> >                    uint32_t cmd_bo, const uint32_t *cmds, int size,
>> > @@ -85,7 +110,7 @@ __exec_batch_patched(int i915, int engine,
>> >       execbuf.batch_len = size;
>> >       execbuf.flags = engine;
>> >  
>> > -     return __gem_execbuf(i915, &execbuf);
>> > +     return __checked_execbuf(i915, &execbuf);
>> >  }
>> >  
>> >  static void exec_batch_patched(int i915, int engine,
>> > @@ -129,7 +154,7 @@ static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
>> >       execbuf.batch_len = size;
>> >       execbuf.flags = engine;
>> >  
>> > -     return  __gem_execbuf(i915, &execbuf);
>> > +     return  __checked_execbuf(i915, &execbuf);
>> >  }
>> >  
>> >  #if 0
>> > @@ -188,7 +213,7 @@ static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
>> >                     0x8);
>> >       execbuf.flags = engine;
>> >  
>> > -     igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
>> > +     igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
>> >  
>> >       gem_close(i915, cmd_bo);
>> >  }
>> > @@ -251,7 +276,7 @@ static void exec_batch_chained(int i915, int engine,
>> >       execbuf.batch_len = sizeof(first_level_cmds);
>> >       execbuf.flags = engine;
>> >  
>> > -     ret = __gem_execbuf(i915, &execbuf);
>> > +     ret = __checked_execbuf(i915, &execbuf);
>> >       if (expected_return && ret == expected_return)
>> >               goto out;
>> >  
>> > @@ -402,7 +427,7 @@ static void test_bb_secure(const int i915, const uint32_t handle)
>> >       execbuf.batch_len = sizeof(batch_secure);
>> >       execbuf.flags = I915_EXEC_BLT;
>> >  
>> > -     igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
>> > +     igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
>> >  }
>> >  
>> >  #define BB_START_PARAM 0
>> > @@ -414,12 +439,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> >  {
>> >       struct drm_i915_gem_execbuffer2 execbuf;
>> >       struct drm_i915_gem_exec_object2 obj[2];
>> > -     struct drm_i915_gem_relocation_entry reloc[3];
>> > +     struct drm_i915_gem_relocation_entry reloc[4];
>> >       const uint32_t target_bo = gem_create(i915, 4096);
>> > -     uint32_t *dst;
>> > -     int ret;
>> >       unsigned int jump_off, footer_pos;
>> > -     const uint32_t batch_header[] = {
>> > +     uint32_t batch[1024] = {
>> >               MI_NOOP,
>> >               MI_NOOP,
>> >               MI_NOOP,
>> > @@ -432,10 +455,11 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> >               4,
>> >               0,
>> >               2,
>> > -             MI_COND_BATCH_BUFFER_END | 1,
>> > +             MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
>> >               0,
>> >               0,
>> > -             0
>> > +             0,
>> > +             MI_ARB_CHECK,
>> >       };
>> >       const uint32_t batch_footer[] = {
>> >               MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
>> > @@ -443,13 +467,10 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> >               0,
>> >               MI_BATCH_BUFFER_END,
>> >       };
>> > -     uint32_t batch[1024];
>> > +     uint32_t *dst;
>> >  
>> >       igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
>> >  
>> > -     memset(batch, 0, sizeof(batch));
>> > -     memcpy(batch, batch_header, sizeof(batch_header));
>> > -
>> >       switch (test) {
>> >       case BB_START_PARAM:
>> >               jump_off = 5 * sizeof(uint32_t);
>> > @@ -460,12 +481,13 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> >               break;
>> >       default:
>> >               jump_off = 0xf00d0000;
>> > +             break;
>> >       }
>> >  
>> >       if (test == BB_START_FAR)
>> > -             footer_pos = (sizeof(batch) - sizeof(batch_footer));
>> > +             footer_pos = sizeof(batch) - sizeof(batch_footer);
>> >       else
>> > -             footer_pos = sizeof(batch_header);
>> > +             footer_pos = 17 * sizeof(uint32_t);
>> >  
>> >       memcpy(batch + footer_pos / sizeof(uint32_t),
>> >              batch_footer, sizeof(batch_footer));
>> > @@ -481,24 +503,28 @@ static void test_bb_start(const int i915, const uint32_t handle, int test)
>> >       reloc[0].delta = 0;
>> >       reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
>> >       reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
>> > -     reloc[0].presumed_offset = -1;
>> >  
>> >       reloc[1].offset = 9 * sizeof(uint32_t);
>> >       reloc[1].target_handle = obj[0].handle;
>> >       reloc[1].delta = 1 * sizeof(uint32_t);
>> >       reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
>> >       reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
>> > -     reloc[1].presumed_offset = -1;
>> >  
>> > -     reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
>> > -     reloc[2].target_handle = obj[1].handle;
>> > -     reloc[2].delta = jump_off;
>> > +     reloc[2].offset = 14 * sizeof(uint32_t);
>> > +     reloc[2].target_handle = obj[0].handle;
>> > +     reloc[2].delta = 0;
>> >       reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
>> >       reloc[2].write_domain = 0;
>> > -     reloc[2].presumed_offset = -1;
>> > +
>> > +     reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
>> > +     reloc[3].target_handle = obj[1].handle;
>> > +     reloc[3].delta = jump_off;
>> > +     reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
>> > +     reloc[3].write_domain = 0;
>> > +     reloc[3].presumed_offset = -1;
>> 
>> Why we need to set the presumed only in this last reloc?
>
> It's the only one that is _not_ preset to presumed.offset + delta.

Makes sense.
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Patch
diff mbox series

diff --git a/tests/i915/gem_exec_parse_blt.c b/tests/i915/gem_exec_parse_blt.c
index 58d1b2b32..854c59863 100644
--- a/tests/i915/gem_exec_parse_blt.c
+++ b/tests/i915/gem_exec_parse_blt.c
@@ -30,6 +30,7 @@ 
 
 #include "igt.h"
 #include "i915/gem_submission.h"
+#include "sw_sync.h"
 
 /* To help craft commands known to be invalid across all engines */
 #define INSTR_CLIENT_SHIFT	29
@@ -53,6 +54,30 @@ 
 
 #define HANDLE_SIZE  4096
 
+static int
+__checked_execbuf(int i915, struct drm_i915_gem_execbuffer2 *eb)
+{
+	int fence;
+	int err;
+
+	igt_assert(!(eb->flags & I915_EXEC_FENCE_OUT));
+	eb->flags |= I915_EXEC_FENCE_OUT;
+	err = __gem_execbuf_wr(i915, eb);
+	eb->flags &= ~I915_EXEC_FENCE_OUT;
+	if (err)
+		return err;
+
+	fence = eb->rsvd2 >> 32;
+
+	sync_fence_wait(fence, -1);
+	err = sync_fence_status(fence);
+	close(fence);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 static int
 __exec_batch_patched(int i915, int engine,
 		     uint32_t cmd_bo, const uint32_t *cmds, int size,
@@ -85,7 +110,7 @@  __exec_batch_patched(int i915, int engine,
 	execbuf.batch_len = size;
 	execbuf.flags = engine;
 
-	return __gem_execbuf(i915, &execbuf);
+	return __checked_execbuf(i915, &execbuf);
 }
 
 static void exec_batch_patched(int i915, int engine,
@@ -129,7 +154,7 @@  static int __exec_batch(int i915, int engine, uint32_t cmd_bo,
 	execbuf.batch_len = size;
 	execbuf.flags = engine;
 
-	return  __gem_execbuf(i915, &execbuf);
+	return  __checked_execbuf(i915, &execbuf);
 }
 
 #if 0
@@ -188,7 +213,7 @@  static void exec_split_batch(int i915, int engine, const uint32_t *cmds,
 		      0x8);
 	execbuf.flags = engine;
 
-	igt_assert_eq(__gem_execbuf(i915, &execbuf), expected_ret);
+	igt_assert_eq(__checked_execbuf(i915, &execbuf), expected_ret);
 
 	gem_close(i915, cmd_bo);
 }
@@ -251,7 +276,7 @@  static void exec_batch_chained(int i915, int engine,
 	execbuf.batch_len = sizeof(first_level_cmds);
 	execbuf.flags = engine;
 
-	ret = __gem_execbuf(i915, &execbuf);
+	ret = __checked_execbuf(i915, &execbuf);
 	if (expected_return && ret == expected_return)
 		goto out;
 
@@ -402,7 +427,7 @@  static void test_bb_secure(const int i915, const uint32_t handle)
 	execbuf.batch_len = sizeof(batch_secure);
 	execbuf.flags = I915_EXEC_BLT;
 
-	igt_assert_eq(__gem_execbuf(i915, &execbuf), -EACCES);
+	igt_assert_eq(__checked_execbuf(i915, &execbuf), -EACCES);
 }
 
 #define BB_START_PARAM 0
@@ -414,12 +439,10 @@  static void test_bb_start(const int i915, const uint32_t handle, int test)
 {
 	struct drm_i915_gem_execbuffer2 execbuf;
 	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_relocation_entry reloc[3];
+	struct drm_i915_gem_relocation_entry reloc[4];
 	const uint32_t target_bo = gem_create(i915, 4096);
-	uint32_t *dst;
-	int ret;
 	unsigned int jump_off, footer_pos;
-	const uint32_t batch_header[] = {
+	uint32_t batch[1024] = {
 		MI_NOOP,
 		MI_NOOP,
 		MI_NOOP,
@@ -432,10 +455,11 @@  static void test_bb_start(const int i915, const uint32_t handle, int test)
 		4,
 		0,
 		2,
-		MI_COND_BATCH_BUFFER_END | 1,
+		MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 2,
 		0,
 		0,
-		0
+		0,
+		MI_ARB_CHECK,
 	};
 	const uint32_t batch_footer[] = {
 		MI_BATCH_BUFFER_START | MI_BATCH_NON_SECURE_I965 | 1,
@@ -443,13 +467,10 @@  static void test_bb_start(const int i915, const uint32_t handle, int test)
 		0,
 		MI_BATCH_BUFFER_END,
 	};
-	uint32_t batch[1024];
+	uint32_t *dst;
 
 	igt_require(gem_can_store_dword(i915, I915_EXEC_BLT));
 
-	memset(batch, 0, sizeof(batch));
-	memcpy(batch, batch_header, sizeof(batch_header));
-
 	switch (test) {
 	case BB_START_PARAM:
 		jump_off = 5 * sizeof(uint32_t);
@@ -460,12 +481,13 @@  static void test_bb_start(const int i915, const uint32_t handle, int test)
 		break;
 	default:
 		jump_off = 0xf00d0000;
+		break;
 	}
 
 	if (test == BB_START_FAR)
-		footer_pos = (sizeof(batch) - sizeof(batch_footer));
+		footer_pos = sizeof(batch) - sizeof(batch_footer);
 	else
-		footer_pos = sizeof(batch_header);
+		footer_pos = 17 * sizeof(uint32_t);
 
 	memcpy(batch + footer_pos / sizeof(uint32_t),
 	       batch_footer, sizeof(batch_footer));
@@ -481,24 +503,28 @@  static void test_bb_start(const int i915, const uint32_t handle, int test)
 	reloc[0].delta = 0;
 	reloc[0].read_domains = I915_GEM_DOMAIN_COMMAND;
 	reloc[0].write_domain = I915_GEM_DOMAIN_COMMAND;
-	reloc[0].presumed_offset = -1;
 
 	reloc[1].offset = 9 * sizeof(uint32_t);
 	reloc[1].target_handle = obj[0].handle;
 	reloc[1].delta = 1 * sizeof(uint32_t);
 	reloc[1].read_domains = I915_GEM_DOMAIN_COMMAND;
 	reloc[1].write_domain = I915_GEM_DOMAIN_COMMAND;
-	reloc[1].presumed_offset = -1;
 
-	reloc[2].offset = footer_pos + 1 * sizeof(uint32_t);
-	reloc[2].target_handle = obj[1].handle;
-	reloc[2].delta = jump_off;
+	reloc[2].offset = 14 * sizeof(uint32_t);
+	reloc[2].target_handle = obj[0].handle;
+	reloc[2].delta = 0;
 	reloc[2].read_domains = I915_GEM_DOMAIN_COMMAND;
 	reloc[2].write_domain = 0;
-	reloc[2].presumed_offset = -1;
+
+	reloc[3].offset = footer_pos + 1 * sizeof(uint32_t);
+	reloc[3].target_handle = obj[1].handle;
+	reloc[3].delta = jump_off;
+	reloc[3].read_domains = I915_GEM_DOMAIN_COMMAND;
+	reloc[3].write_domain = 0;
+	reloc[3].presumed_offset = -1;
 
 	obj[1].relocs_ptr = to_user_pointer(reloc);
-	obj[1].relocation_count = 3;
+	obj[1].relocation_count = ARRAY_SIZE(reloc);
 
 	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = to_user_pointer(obj);
@@ -506,21 +532,20 @@  static void test_bb_start(const int i915, const uint32_t handle, int test)
 	execbuf.batch_len = sizeof(batch);
 	execbuf.flags = I915_EXEC_BLT;
 
-	dst = gem_mmap__cpu(i915, obj[0].handle, 0, 4096,
-			    PROT_READ | PROT_WRITE);
+	dst = gem_mmap__wc(i915, obj[0].handle, 0, 4096, PROT_WRITE);
 
 	igt_assert_eq(dst[0], 0);
 	igt_assert_eq(dst[1], 0);
 
-	ret = __gem_execbuf(i915, &execbuf);
-
 	switch (test) {
 	case BB_START_PARAM:
-		igt_assert_eq(ret, -EINVAL);
+	case BB_START_OUT:
+		igt_assert_eq(__checked_execbuf(i915, &execbuf), -EINVAL);
 		break;
+
 	case BB_START_CMD:
 	case BB_START_FAR:
-		igt_assert_eq(ret, 0);
+		gem_execbuf(i915, &execbuf);
 
 		while (READ_ONCE(dst[0]) == 0)
 		       ;
@@ -531,19 +556,8 @@  static void test_bb_start(const int i915, const uint32_t handle, int test)
 		igt_assert_eq(dst[0], 1);
 		igt_assert_eq(dst[1], 2);
 
-		igt_info("values now %x %x\n", dst[0], dst[1]);
-
 		dst[0] = 0;
-
-		igt_info("values now %x %x\n", dst[0], dst[1]);
-
-		igt_assert_eq(dst[0], 0);
-		igt_assert_eq(dst[1], 2);
-
-		break;
-
-	case BB_START_OUT:
-		igt_assert_eq(ret, -EINVAL);
+		__sync_synchronize();
 		break;
 	}
 
@@ -896,7 +910,7 @@  static void test_unaligned_jump(const int i915, const uint32_t handle)
 		memcpy(&batch[1], &offset, sizeof(offset));
 		gem_write(i915, handle, 4000, batch, sizeof(batch));
 
-		igt_assert_f(__gem_execbuf(i915, &execbuf) == -EINVAL,
+		igt_assert_f(__checked_execbuf(i915, &execbuf) == -EINVAL,
 			     "unaligned jump accepted to %d; batch=%08x\n",
 			     reloc.delta, batch[reloc.delta / 4]);
 	}
@@ -1032,19 +1046,11 @@  igt_main
 	igt_subtest("unaligned-jump")
 		test_unaligned_jump(i915, handle);
 
-	igt_subtest_group {
-		igt_hang_t hang;
-
-		igt_fixture igt_allow_hang(i915, 0, 0);
+	igt_subtest("bb-start-cmd")
+		test_bb_start(i915, handle, BB_START_CMD);
 
-		igt_subtest("bb-start-cmd")
-			test_bb_start(i915, handle, BB_START_CMD);
-
-		igt_subtest("bb-start-far")
-			test_bb_start(i915, handle, BB_START_FAR);
-
-		igt_fixture igt_disallow_hang(i915, hang);
-	}
+	igt_subtest("bb-start-far")
+		test_bb_start(i915, handle, BB_START_FAR);
 
 	igt_fixture {
 		igt_stop_hang_detector();