[Intel-gfx] [igt-dev] [PATCH i-g-t] igt: Use COND_BBEND for busy spinning on gen9

[Chris Wilson]

Quoting Chris Wilson (2019-11-13 20:20:31)
> From: Jon Bloomfield <jon.bloomfield at intel.com>
> 
> gen9+ introduces a cmdparser for the BLT engine which copies the
> incoming BB to a kmd owned buffer for submission (to prevent changes
> being made after the bb has been safely scanned). This breaks the
> spin functionality because it relies on changing the submitted spin
> buffers in order to terminate them.
> 
> Instead, for gen9+, we change the semantics by introducing a COND_BB_END
> into the infinite loop, to wait until a memory flag (in anothe bo) is
> cleared.
> 
> v2: Correct nop length to avoid overwriting bb_end instr when using
>     a dependency bo (cork)
> v3: fix conflicts on igt_dummyload (Mika)
> v4: s/bool running/uint32_t running, fix r->delta (Mika)
> v5: remove overzealous assert (Mika)
> v6: rebase on top of lib changes (Mika)
> v7: rework on top of public igt lib changes (Mika)
> v8: rebase
> v9: simplify by using bb end as conditional (Chris)
> 
> Signed-off-by: Jon Bloomfield <jon.bloomfield at intel.com> (v2)
> Cc: Joonas Lahtinen <joonas.lahtinen at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
> Compile after rebase!
> ---
>  lib/i830_reg.h                          |  2 +-
>  lib/igt_dummyload.c                     | 27 +++++++++++++++++++++++--
>  lib/intel_reg.h                         |  3 +++
>  tests/i915/gem_double_irq_loop.c        |  2 --
>  tests/i915/gem_write_read_ring_switch.c |  2 +-
>  5 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/i830_reg.h b/lib/i830_reg.h
> index a57691c7e..410202567 100644
> --- a/lib/i830_reg.h
> +++ b/lib/i830_reg.h
> @@ -43,7 +43,7 @@
>  /* broadwater flush bits */
>  #define BRW_MI_GLOBAL_SNAPSHOT_RESET   (1 << 3)
>  
> -#define MI_COND_BATCH_BUFFER_END       (0x36<<23 | 1)
> +#define MI_COND_BATCH_BUFFER_END       (0x36 << 23)
>  #define MI_DO_COMPARE                  (1<<21)
>  
>  #define MI_BATCH_BUFFER_END    (0xA << 23)
> diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
> index b9e239db3..563a451da 100644
> --- a/lib/igt_dummyload.c
> +++ b/lib/igt_dummyload.c
> @@ -75,7 +75,7 @@ emit_recursive_batch(igt_spin_t *spin,
>  #define SCRATCH 0
>  #define BATCH IGT_SPIN_BATCH
>         const int gen = intel_gen(intel_get_drm_devid(fd));
> -       struct drm_i915_gem_relocation_entry relocs[2], *r;
> +       struct drm_i915_gem_relocation_entry relocs[3], *r;
>         struct drm_i915_gem_execbuffer2 *execbuf;
>         struct drm_i915_gem_exec_object2 *obj;
>         unsigned int flags[GEM_MAX_ENGINES];
> @@ -205,7 +205,30 @@ emit_recursive_batch(igt_spin_t *spin,
>          * trouble. See https://bugs.freedesktop.org/show_bug.cgi?id=102262
>          */
>         if (!(opts->flags & IGT_SPIN_FAST))
> -               cs += 1000;
> +               cs += 960;
> +
> +       /*
> +        * When using a cmdparser, the batch is copied into a read only location
> +        * and validated. We are then unable to alter the executing batch,
> +        * breaking the older *spin->condition = MI_BB_END termination.
> +        * Instead we can use a conditional MI_BB_END here that looks at
> +        * the user's copy of the batch and terminates when they modified it,
> +        * no matter how they modify it (from either the GPU or CPU).
> +        */
> +       if (gen >= 9) {
> +               r = &relocs[obj[BATCH].relocation_count++];
> +
> +               r->presumed_offset = 0;
> +               r->target_handle = obj[BATCH].handle;
> +               r->offset = (cs + 2 - batch) * sizeof(*cs);
> +               r->read_domains = I915_GEM_DOMAIN_COMMAND;
> +               r->delta = (spin->condition - batch) * sizeof(*cs);
> +
> +               *cs++ = MI_COND_BATCH_BUFFER_END | 1 << 21 | 5 << 12 | 2;

Great the magic 5<<12 worked on kbl in testing, but we did not fare so
lucky on skl! One slight adjustment later...
-Chris