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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:

> Quoting Mika Kuoppala (2019-12-05 10:18:34)
>> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
>> > ---
>> >  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@xxxxxxxxxxxxxxx>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux