Re: [Intel-gfx] [PATCH 02/34] drm/i915/execlists: Suppress preempting self

2019-01-22 Thread Chris Wilson
Quoting John Harrison (2019-01-22 22:18:46)
> On 1/21/2019 14:20, Chris Wilson wrote:
> > In order to avoid preempting ourselves, we currently refuse to schedule
> > the tasklet if we reschedule an inflight context. However, this glosses
> > over a few issues such as what happens after a CS completion event and
> > we then preempt the newly executing context with itself, or if something
> > else causes a tasklet_schedule triggering the same evaluation to
> > preempt the active context with itself.
> >
> > To avoid the extra complications, after deciding that we have
> > potentially queued a request with higher priority than the currently
> > executing request, inspect the head of the queue to see if it is indeed
> > higher priority from another context.
> >
> > References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on 
> > the current context")
> > Signed-off-by: Chris Wilson 
> > Cc: Tvrtko Ursulin 
> 
> Can you explain what was wrong with the previous version of this patch 
> (drm/i915/execlists: Store the highest priority context)? It seemed simpler.

The goal here is to be a more general suppression mechanism than the
first version. queue_priority is a hint and can't be trusted as we may
have set it for an inflight request since completed. Given that it tells
us that a preemption point _was_ required, but we don't want to forcibly
inject an idle barrier, we inspect the queue instead and not take the
hint at face value. In that light, queue_context is superfluous as we
ignore the ELSP[0] context anyway.

The patch is slightly bigger than it needed to be because I was
refactoring out some changes for later, and a bit of paranoid asserts
from debugging that didn't really belong in the bugfix.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 02/34] drm/i915/execlists: Suppress preempting self

2019-01-22 Thread John Harrison

On 1/21/2019 14:20, Chris Wilson wrote:

In order to avoid preempting ourselves, we currently refuse to schedule
the tasklet if we reschedule an inflight context. However, this glosses
over a few issues such as what happens after a CS completion event and
we then preempt the newly executing context with itself, or if something
else causes a tasklet_schedule triggering the same evaluation to
preempt the active context with itself.

To avoid the extra complications, after deciding that we have
potentially queued a request with higher priority than the currently
executing request, inspect the head of the queue to see if it is indeed
higher priority from another context.

References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current 
context")
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 


Can you explain what was wrong with the previous version of this patch 
(drm/i915/execlists: Store the highest priority context)? It seemed simpler.


John.


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


[Intel-gfx] [PATCH 02/34] drm/i915/execlists: Suppress preempting self

2019-01-21 Thread Chris Wilson
In order to avoid preempting ourselves, we currently refuse to schedule
the tasklet if we reschedule an inflight context. However, this glosses
over a few issues such as what happens after a CS completion event and
we then preempt the newly executing context with itself, or if something
else causes a tasklet_schedule triggering the same evaluation to
preempt the active context with itself.

To avoid the extra complications, after deciding that we have
potentially queued a request with higher priority than the currently
executing request, inspect the head of the queue to see if it is indeed
higher priority from another context.

References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the 
current context")
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
---
 drivers/gpu/drm/i915/i915_scheduler.c | 20 ++
 drivers/gpu/drm/i915/intel_lrc.c  | 29 ++-
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_scheduler.c 
b/drivers/gpu/drm/i915/i915_scheduler.c
index 340faea6c08a..fb5d953430e5 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -239,6 +239,18 @@ sched_lock_engine(struct i915_sched_node *node, struct 
intel_engine_cs *locked)
return engine;
 }
 
+static bool inflight(const struct i915_request *rq,
+const struct intel_engine_cs *engine)
+{
+   const struct i915_request *active;
+
+   if (!rq->global_seqno)
+   return false;
+
+   active = port_request(engine->execlists.port);
+   return active->hw_context == rq->hw_context;
+}
+
 static void __i915_schedule(struct i915_request *rq,
const struct i915_sched_attr *attr)
 {
@@ -328,6 +340,7 @@ static void __i915_schedule(struct i915_request *rq,
INIT_LIST_HEAD(&dep->dfs_link);
 
engine = sched_lock_engine(node, engine);
+   lockdep_assert_held(&engine->timeline.lock);
 
/* Recheck after acquiring the engine->timeline.lock */
if (prio <= node->attr.priority || node_signaled(node))
@@ -356,17 +369,16 @@ static void __i915_schedule(struct i915_request *rq,
if (prio <= engine->execlists.queue_priority)
continue;
 
+   engine->execlists.queue_priority = prio;
+
/*
 * If we are already the currently executing context, don't
 * bother evaluating if we should preempt ourselves.
 */
-   if (node_to_request(node)->global_seqno &&
-   
i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
- node_to_request(node)->global_seqno))
+   if (inflight(node_to_request(node), engine))
continue;
 
/* Defer (tasklet) submission until after all of our updates. */
-   engine->execlists.queue_priority = prio;
tasklet_hi_schedule(&engine->execlists.tasklet);
}
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b74f25420683..28d183439952 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -190,6 +190,30 @@ static inline bool need_preempt(const struct 
intel_engine_cs *engine,
!i915_request_completed(last));
 }
 
+static inline bool check_preempt(const struct intel_engine_cs *engine,
+const struct i915_request *rq)
+{
+   const struct intel_context *ctx = rq->hw_context;
+   const int prio = rq_prio(rq);
+   struct rb_node *rb;
+   int idx;
+
+   list_for_each_entry_continue(rq, &engine->timeline.requests, link) {
+   GEM_BUG_ON(rq->hw_context == ctx);
+   if (rq_prio(rq) > prio)
+   return true;
+   }
+
+   rb = rb_first_cached(&engine->execlists.queue);
+   if (!rb)
+   return false;
+
+   priolist_for_each_request(rq, to_priolist(rb), idx)
+   return rq->hw_context != ctx && rq_prio(rq) > prio;
+
+   return false;
+}
+
 /*
  * The context descriptor encodes various attributes of a context,
  * including its GTT address and some flags. Because it's fairly
@@ -580,7 +604,8 @@ static void execlists_dequeue(struct intel_engine_cs 
*engine)
if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
return;
 
-   if (need_preempt(engine, last, execlists->queue_priority)) {
+   if (need_preempt(engine, last, execlists->queue_priority) &&
+   check_preempt(engine, last)) {
inject_preempt_context(engine);
return;
}
@@ -872,6 +897,8 @@ static void process_csb(struct intel_engine_cs *engine)
const u32 * const buf = execlists->csb_status;