[igt-dev] [PATCH i-g-t v2] lib/hang_ctx: Make use of dummyload library to create recursive batch

[Antonio Argenziano]

On 10/07/18 08:38, Chris Wilson wrote:
> Quoting Antonio Argenziano (2018-07-10 16:31:55)
>> An hanging batch is nothing more than a spinning batch that never gets
>> stopped, so re-use the routines implemented in dummyload.c.
>>
>> v2:
>>          - Let caller decide spin loop size (Chris)
>>          - Now builds with meson.
>>
>> Signed-off-by: Antonio Argenziano <antonio.argenziano at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>>   lib/igt_gt.c                 | 65 +++++++++-----------------------------------
>>   lib/igt_gt.h                 |  3 +-
>>   lib/meson.build              |  1 +
>>   tests/gem_concurrent_all.c   |  6 ++--
>>   tests/gem_ctx_exec.c         |  2 +-
>>   tests/gem_mmap_gtt.c         |  2 +-
>>   tests/gem_pread_after_blit.c |  2 +-
>>   tests/gem_reloc_vs_gpu.c     |  2 +-
>>   tests/gem_ringfill.c         |  2 +-
>>   tests/gem_shrink.c           |  2 +-
>>   tests/kms_flip.c             |  2 +-
>>   tests/kms_vblank.c           |  2 +-
>>   12 files changed, 27 insertions(+), 64 deletions(-)
>>
>> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
>> index 4569fd36..4ca4beef 100644
>> --- a/lib/igt_gt.c
>> +++ b/lib/igt_gt.c
>> @@ -40,6 +40,7 @@
>>   #include "ioctl_wrappers.h"
>>   #include "intel_reg.h"
>>   #include "intel_chipset.h"
>> +#include "igt_dummyload.h"
>>   
>>   /**
>>    * SECTION:igt_gt
>> @@ -264,14 +265,10 @@ igt_hang_t igt_hang_ctx(int fd,
>>                          unsigned flags,
>>                          uint64_t *offset)
>>   {
>> -       struct drm_i915_gem_relocation_entry reloc;
>> -       struct drm_i915_gem_execbuffer2 execbuf;
>> -       struct drm_i915_gem_exec_object2 exec;
>> +       igt_spin_t *spinning_batch;
>> +       struct igt_spin_factory opts = {};
>>          struct drm_i915_gem_context_param param;
>> -       uint32_t b[16];
>>          unsigned ban;
>> -       unsigned len;
>> -       int gen;
>>   
>>          igt_require_hang_ring(fd, ring);
>>   
>> @@ -295,58 +292,22 @@ igt_hang_t igt_hang_ctx(int fd,
>>          if ((flags & HANG_ALLOW_BAN) == 0)
>>                  context_set_ban(fd, ctx, 0);
>>   
>> -       memset(&reloc, 0, sizeof(reloc));
>> -       memset(&exec, 0, sizeof(exec));
>> -       memset(&execbuf, 0, sizeof(execbuf));
>> -
>> -       exec.handle = gem_create(fd, 4096);
>> -       exec.relocation_count = 1;
>> -       exec.relocs_ptr = to_user_pointer(&reloc);
>> -
>> -       memset(b, 0xc5, sizeof(b));
>> -
>> -       len = 0;
>> -       gen = intel_gen(intel_get_drm_devid(fd));
>> -       if (gen >= 8) {
>> -               b[len++] = MI_BATCH_BUFFER_START | 1 << 8 | 1;
>> -               b[len++] = 0;
>> -               b[len++] = 0;
>> -       } else if (gen >= 6) {
>> -               b[len++] = MI_BATCH_BUFFER_START | 1 << 8;
>> -               b[len++] = 0;
>> -       } else {
>> -               b[len++] = MI_BATCH_BUFFER_START | 2 << 6;
>> -               b[len] = 0;
>> -               if (gen < 4) {
>> -                       b[len] |= 1;
>> -                       reloc.delta = 1;
>> -               }
>> -               len++;
>> -       }
>> -       b[len++] = MI_BATCH_BUFFER_END;
>> -       b[len] = MI_NOOP;
>> -       gem_write(fd, exec.handle, 0, b, sizeof(b));
>> -
>> -       reloc.offset = sizeof(uint32_t);
>> -       reloc.target_handle = exec.handle;
>> -       reloc.read_domains = I915_GEM_DOMAIN_COMMAND;
>> -
>> -       execbuf.buffers_ptr = to_user_pointer(&exec);
>> -       execbuf.buffer_count = 1;
>> -       execbuf.flags = ring;
>> -       i915_execbuffer2_set_context_id(execbuf, ctx);
>> -       gem_execbuf(fd, &execbuf);
>> +       opts.ctx = ctx;
>> +       opts.engine = ring;
>> +       opts.flags |= (flags & HANG_SPIN_FAST) ? IGT_SPIN_FAST : 0; /* hangs faster */
>> +       spinning_batch = igt_spin_batch_factory(fd, &opts);
>>   
>>          if (offset)
>> -               *offset = exec.offset;
>> +               *offset = (*spinning_batch).obj[1].offset; /* The batch is the last object */
> 
> K&R came up with this shorthand I think you should use, ->

Witchcraft!

> 
>> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
>> index d44b7552..12a83a1b 100644
>> --- a/lib/igt_gt.h
>> +++ b/lib/igt_gt.h
>> @@ -50,8 +50,9 @@ igt_hang_t igt_hang_ctx(int fd,
>>                          uint64_t *offset);
>>   #define HANG_ALLOW_BAN 1
>>   #define HANG_ALLOW_CAPTURE 2
>> +#define HANG_SPIN_FAST 3
> 
> Must be a power-of-two, or wrap in BIT() which doesn't exist yet...

Should probably be more awake when I do this things...

> 
> Overall I am still dubious there is any reason to set this flag. The
> ratelimiting step here isn't the speed at which the batch spins, but the
> time for hangcheck to kick in. Where a fast spinner is interesting is
> the latency in responding to terminating the spinner (which never
> happens for a hang!).

The flag makes sense to me because hangcheck will always sample the same 
head on a very tight loop like the fast spinner. The current 
implementation of hangcheck will also declare the engine hung much 
quicker. In general I think we should expose different hanging batch to 
the driver to make sure it can react to the different situations.

> -Chris
>