[igt-dev] [PATCH i-g-t] i915/gem_exec_parse_blt: Fix COND_BBEND used by bb-start-(cmd|far)
Chris Wilson
chris at chris-wilson.co.uk
Thu Dec 5 10:33:46 UTC 2019
Quoting Mika Kuoppala (2019-12-05 10:18:34)
> Chris Wilson <chris at 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 at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at 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
More information about the igt-dev
mailing list