Re: [Intel-gfx] [PATCH 7/7] drm/i915/execlists: Remove preempt-to-busy roundtrip delay

2020-02-14 Thread Chris Wilson
Quoting Daniele Ceraolo Spurio (2020-02-12 01:08:30)
> 
> 
> On 2/10/20 12:57 PM, Chris Wilson wrote:
> > @@ -1934,6 +2002,8 @@ static void execlists_dequeue(struct intel_engine_cs 
> > *engine)
> >   
> >   return;
> >   }
> > +
> > + last = skip_lite_restore(engine, last, );
> >   }
> >   }
> >   
> > @@ -2155,10 +2225,11 @@ static void execlists_dequeue(struct 
> > intel_engine_cs *engine)
> >   WRITE_ONCE(execlists->yield, -1);
> >   execlists_submit_ports(engine);
> >   set_preempt_timeout(engine);
> > - } else {
> > + }
> > +
> > + if (intel_engine_has_tail_lrm(engine) || !submit)
> 
> Why do we skip here if intel_engine_has_tail_lrm is true? I see that if 
> we have work pending we either take the skip_lite_restore() or the 
> submit path above, but I can't see why we need to explicitly skip 
> re-starting the ring.

You mean if !has_lrm. We have to delay letting the RING_TAIL move past
the end of the request until the HW has acknowledged the preemption
request. This is required to avoid the ELSP submission from trying to
move the RING_TAIL backwards.

As it turns out, I can't special case has_lrm here since if we read the
new RING_TAIL before the ELSP event, we end up submitting the same
RING_TAIL again and trigging the HW bug.

> 
> >   skip_submit:
> >   ring_set_paused(engine, 0);
> > - }
> >   }
> >   
> >   static void
> > @@ -2325,7 +2396,8 @@ static void process_csb(struct intel_engine_cs 
> > *engine)
> >   
> >   GEM_BUG_ON(!assert_pending_valid(execlists, 
> > "promote"));
> >   
> > - ring_set_paused(engine, 0);
> > + if (!intel_engine_has_tail_lrm(engine))
> > + ring_set_paused(engine, 0);
> >   
> 
> here as well, although I'm assuming it has the same explanation as the 
> one above.

For has_lrm, it will have already seen the new RING_TAIL at the end of
the request regardless of the preempting ELSP.

However, as noted without only setting wa_tail in the context-image and
LRM normal tail, we end up hitting WaIdleLiteRestore and killing the HW.
Bleugh.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 7/7] drm/i915/execlists: Remove preempt-to-busy roundtrip delay

2020-02-11 Thread Daniele Ceraolo Spurio




On 2/10/20 12:57 PM, Chris Wilson wrote:

To prevent the context from proceeding past the end of the request as we
unwind, we embed a semaphore into the footer of each request. (If the
context were to skip past the end of the request as we perform the
preemption, next time we reload the context it's RING_HEAD would be past
the RING_TAIL and instead of replaying the commands it would read the
read of the uninitialised ringbuffer.)

However, this requires us to keep the ring paused at the end of the
request until we have a change to process the preemption ack and remove
the semaphore. Our processing of acks is at the whim of ksoftirqd, and
so it is entirely possible that the GPU has to wait for the tasklet
before it can proceed with the next request.

It was suggested that we could also embed a MI_LOAD_REGISTER_MEM into
the footer to read the current RING_TAIL from the context, which would
allow us to not only avoid this round trip (and so release the context
as soon as we had submitted the preemption request to in ELSP), but also
skip using ELSP for lite-restores entirely. That has the nice benefit of
dramatically reducing contention and the frequency of interrupts when a
client submits two or more execbufs in rapid succession.

However, mmio access to RING_TAIL was defeatured in gen11 so we can only
employ this handy trick for gen8/gen9.

Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Daniele Ceraolo Spurio 
---
  drivers/gpu/drm/i915/gt/intel_engine_types.h | 23 +++--
  drivers/gpu/drm/i915/gt/intel_lrc.c  | 99 +++-
  2 files changed, 109 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h 
b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 24cff658e6e5..ae8724915320 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -488,14 +488,15 @@ struct intel_engine_cs {
/* status_notifier: list of callbacks for context-switch changes */
struct atomic_notifier_head context_status_notifier;
  
-#define I915_ENGINE_USING_CMD_PARSER BIT(0)

-#define I915_ENGINE_SUPPORTS_STATS   BIT(1)
-#define I915_ENGINE_HAS_PREEMPTION   BIT(2)
-#define I915_ENGINE_HAS_SEMAPHORES   BIT(3)
-#define I915_ENGINE_NEEDS_BREADCRUMB_TASKLET BIT(4)
-#define I915_ENGINE_IS_VIRTUAL   BIT(5)
-#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(6)
-#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(7)
+#define I915_ENGINE_REQUIRES_CMD_PARSERBIT(0)
+#define I915_ENGINE_USING_CMD_PARSER   BIT(1)
+#define I915_ENGINE_SUPPORTS_STATS BIT(2)
+#define I915_ENGINE_HAS_PREEMPTION BIT(3)
+#define I915_ENGINE_HAS_SEMAPHORES BIT(4)
+#define I915_ENGINE_HAS_TAIL_LRM   BIT(5)
+#define I915_ENGINE_NEEDS_BREADCRUMB_TASKLET   BIT(6)
+#define I915_ENGINE_IS_VIRTUAL BIT(7)
+#define I915_ENGINE_HAS_RELATIVE_MMIO  BIT(8)
unsigned int flags;
  
  	/*

@@ -592,6 +593,12 @@ intel_engine_has_semaphores(const struct intel_engine_cs 
*engine)
return engine->flags & I915_ENGINE_HAS_SEMAPHORES;
  }
  
+static inline bool

+intel_engine_has_tail_lrm(const struct intel_engine_cs *engine)
+{
+   return engine->flags & I915_ENGINE_HAS_TAIL_LRM;
+}
+
  static inline bool
  intel_engine_needs_breadcrumb_tasklet(const struct intel_engine_cs *engine)
  {
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c 
b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 696f0b6b223c..5939672781fb 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1797,6 +1797,74 @@ static inline void clear_ports(struct i915_request 
**ports, int count)
memset_p((void **)ports, NULL, count);
  }
  
+static struct i915_request *

+skip_lite_restore(struct intel_engine_cs *const engine,
+ struct i915_request *first,
+ bool *submit)
+{
+   struct intel_engine_execlists *const execlists = >execlists;
+   struct i915_request *last = first;
+   struct rb_node *rb;
+
+   if (!intel_engine_has_tail_lrm(engine))
+   return last;
+
+   GEM_BUG_ON(*submit);
+   while ((rb = rb_first_cached(>queue))) {
+   struct i915_priolist *p = to_priolist(rb);
+   struct i915_request *rq, *rn;
+   int i;
+
+   priolist_for_each_request_consume(rq, rn, p, i) {
+   if (!can_merge_rq(last, rq))
+   goto out;
+
+   if (__i915_request_submit(rq)) {
+   *submit = true;
+   last = rq;
+   }
+   }
+
+   rb_erase_cached(>node, >queue);
+   i915_priolist_free(p);
+   }
+out:
+   if (*submit) {
+   ring_set_paused(engine, 1);
+
+   /*
+* If we are quick and the current context hasn't yet completed
+* its