[i-g-t,3/3] igt: Use COND_BBEND for busy spinning on gen9
diff mbox series

Message ID 20191113182808.10780-3-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [i-g-t,1/3] i915: Skip if secure batches is not available
Related show

Commit Message

Chris Wilson Nov. 13, 2019, 6:28 p.m. UTC
From: Jon Bloomfield <jon.bloomfield@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@intel.com> (v2)
Cc: Joonas Lahtinen <joonas.lahtinen@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 lib/i830_reg.h                          |  2 +-
 lib/igt_dummyload.c                     | 17 ++++++++++++++++-
 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, 21 insertions(+), 5 deletions(-)

Comments

Chris Wilson Nov. 13, 2019, 6:42 p.m. UTC | #1
Quoting Chris Wilson (2019-11-13 18:28:08)
> From: Jon Bloomfield <jon.bloomfield@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@intel.com> (v2)
> Cc: Joonas Lahtinen <joonas.lahtinen@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  lib/i830_reg.h                          |  2 +-
>  lib/igt_dummyload.c                     | 17 ++++++++++++++++-
>  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, 21 insertions(+), 5 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 c079bd045..652d469a0 100644
> --- a/lib/igt_dummyload.c
> +++ b/lib/igt_dummyload.c
> @@ -208,7 +208,22 @@ 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;
> +
> +       if (gen >= 8) {

If we push this back to gen>7 it will allow us to use it with its
cmdparser as well.

Hmm? I guess we could just do gen>6 so that there's some systematic
cutoff point.

> +               r = &relocs[batch->relocation_count++];
> +
> +               r->presumed_offset = 0;
> +               r->target_handle = batch->handle;
> +               r->offset = (cs + 2 - batch_start) * sizeof(*cs);
> +               r->read_domains = I915_GEM_DOMAIN_COMMAND;
> +               r->delta = (spin->condition - batch_start) * sizeof(*cs);
> +
> +               *cs++ = MI_COND_BATCH_BUFFER_END | 1 << 21 | 5 << 12 | 2;
> +               *cs++ = MI_BATCH_BUFFER_END;
> +               *cs++ = r->delta;
> +               *cs++ = 0;

I think this approach should do the trick even for my attempts to
frustrate you.

With consensus on when to use COND_BB_END and verification I haven't
caused havoc...

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Chris Wilson Nov. 13, 2019, 7:43 p.m. UTC | #2
Quoting Chris Wilson (2019-11-13 18:42:06)
> Quoting Chris Wilson (2019-11-13 18:28:08)
> > From: Jon Bloomfield <jon.bloomfield@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@intel.com> (v2)
> > Cc: Joonas Lahtinen <joonas.lahtinen@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >  lib/i830_reg.h                          |  2 +-
> >  lib/igt_dummyload.c                     | 17 ++++++++++++++++-
> >  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, 21 insertions(+), 5 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 c079bd045..652d469a0 100644
> > --- a/lib/igt_dummyload.c
> > +++ b/lib/igt_dummyload.c
> > @@ -208,7 +208,22 @@ 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;
> > +
> > +       if (gen >= 8) {
> 
> If we push this back to gen>7 it will allow us to use it with its
> cmdparser as well.
> 
> Hmm? I guess we could just do gen>6 so that there's some systematic
> cutoff point.

Oh, well, it appears the magic compare mode doesn't exist until Skylake.
Fortunately, it exists!
-Chris

Patch
diff mbox series

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 c079bd045..652d469a0 100644
--- a/lib/igt_dummyload.c
+++ b/lib/igt_dummyload.c
@@ -208,7 +208,22 @@  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;
+
+	if (gen >= 8) {
+		r = &relocs[batch->relocation_count++];
+
+		r->presumed_offset = 0;
+		r->target_handle = batch->handle;
+		r->offset = (cs + 2 - batch_start) * sizeof(*cs);
+		r->read_domains = I915_GEM_DOMAIN_COMMAND;
+		r->delta = (spin->condition - batch_start) * sizeof(*cs);
+
+		*cs++ = MI_COND_BATCH_BUFFER_END | 1 << 21 | 5 << 12 | 2;
+		*cs++ = MI_BATCH_BUFFER_END;
+		*cs++ = r->delta;
+		*cs++ = 0;
+	}
 
 	/* recurse */
 	r = &relocs[batch->relocation_count++];
diff --git a/lib/intel_reg.h b/lib/intel_reg.h
index 069440cb7..10ca760a2 100644
--- a/lib/intel_reg.h
+++ b/lib/intel_reg.h
@@ -2593,6 +2593,9 @@  SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 #define MI_BATCH_BUFFER		((0x30 << 23) | 1)
 #define MI_BATCH_BUFFER_START	(0x31 << 23)
 #define MI_BATCH_BUFFER_END	(0xA << 23)
+#define MI_COND_BATCH_BUFFER_END	(0x36 << 23)
+#define MI_DO_COMPARE                   (1<<21)
+
 #define MI_BATCH_NON_SECURE		(1)
 #define MI_BATCH_NON_SECURE_I965	(1 << 8)
 #define MI_BATCH_NON_SECURE_HSW		(1<<13) /* Additional bit for RCS */
diff --git a/tests/i915/gem_double_irq_loop.c b/tests/i915/gem_double_irq_loop.c
index b326fc58a..f17f61c19 100644
--- a/tests/i915/gem_double_irq_loop.c
+++ b/tests/i915/gem_double_irq_loop.c
@@ -52,8 +52,6 @@  static drm_intel_bo *target_buffer, *blt_bo;
 IGT_TEST_DESCRIPTION("Basic check for missed IRQs on blt ring.");
 
 
-#define MI_COND_BATCH_BUFFER_END	(0x36<<23 | 1)
-#define MI_DO_COMPARE			(1<<21)
 static void
 dummy_reloc_loop(void)
 {
diff --git a/tests/i915/gem_write_read_ring_switch.c b/tests/i915/gem_write_read_ring_switch.c
index ef229cc59..095c13c34 100644
--- a/tests/i915/gem_write_read_ring_switch.c
+++ b/tests/i915/gem_write_read_ring_switch.c
@@ -115,7 +115,7 @@  static void run_test(int ring)
 	 * otherwise the obj->last_write_seqno will be updated. */
 	if (ring == I915_EXEC_RENDER) {
 		BEGIN_BATCH(4, 1);
-		OUT_BATCH(MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE);
+		OUT_BATCH(MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE | 1);
 		OUT_BATCH(0xffffffff); /* compare dword */
 		OUT_RELOC(target_bo, I915_GEM_DOMAIN_RENDER, 0, 0);
 		OUT_BATCH(MI_NOOP);