| Message ID | 20180411103929.27374-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Chris Wilson (2018-04-11 11:39:29) > We can refine our current execlists->queue_priority if we inspect > ELSP[1] rather than the head of the unsubmitted queue. Currently, we use > the unsubmitted queue and say that if a subsequent request is more > important than the current queue, we will rerun the submission tasklet > to evaluate the need for preemption. However, we only want to preempt if > we need to jump ahead of a currently executing request in ELSP. The > second reason for running the submission tasklet is amalgamate requests > into the active context on ELSP[0] to avoid a stall when ELSP[0] drains. > (Though repeatedly amalgamating requests into the active context and > triggering many lite-restore is off question gain, the goal really is to > put a context into ELSP[1] to cover the interrupt.) So if instead of > looking at the head of the queue, we look at the context in ELSP[1] we > can answer both of the questions more accurately -- we don't need to > rerun the submission tasklet unless our new request is important enough > to feed into, at least, ELSP[1]. > > v2: Add some comments from the discussion with Tvrtko. > v3: More commentary to cross-reference queue_request() > > References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> From the discussion thread, Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> I assume Tvrtko is happy with the comments added based on that conversation. -Chris
On 11/04/2018 12:34, Chris Wilson wrote: > Quoting Chris Wilson (2018-04-11 11:39:29) >> We can refine our current execlists->queue_priority if we inspect >> ELSP[1] rather than the head of the unsubmitted queue. Currently, we use >> the unsubmitted queue and say that if a subsequent request is more >> important than the current queue, we will rerun the submission tasklet >> to evaluate the need for preemption. However, we only want to preempt if >> we need to jump ahead of a currently executing request in ELSP. The >> second reason for running the submission tasklet is amalgamate requests >> into the active context on ELSP[0] to avoid a stall when ELSP[0] drains. >> (Though repeatedly amalgamating requests into the active context and >> triggering many lite-restore is off question gain, the goal really is to >> put a context into ELSP[1] to cover the interrupt.) So if instead of >> looking at the head of the queue, we look at the context in ELSP[1] we >> can answer both of the questions more accurately -- we don't need to >> rerun the submission tasklet unless our new request is important enough >> to feed into, at least, ELSP[1]. >> >> v2: Add some comments from the discussion with Tvrtko. >> v3: More commentary to cross-reference queue_request() >> >> References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports") >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> Cc: Michel Thierry <michel.thierry@intel.com> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > From the discussion thread, > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > I assume Tvrtko is happy with the comments added based on that > conversation. Yes, all fine! Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-04-11 13:03:00) > > On 11/04/2018 12:34, Chris Wilson wrote: > > Quoting Chris Wilson (2018-04-11 11:39:29) > >> We can refine our current execlists->queue_priority if we inspect > >> ELSP[1] rather than the head of the unsubmitted queue. Currently, we use > >> the unsubmitted queue and say that if a subsequent request is more > >> important than the current queue, we will rerun the submission tasklet > >> to evaluate the need for preemption. However, we only want to preempt if > >> we need to jump ahead of a currently executing request in ELSP. The > >> second reason for running the submission tasklet is amalgamate requests > >> into the active context on ELSP[0] to avoid a stall when ELSP[0] drains. > >> (Though repeatedly amalgamating requests into the active context and > >> triggering many lite-restore is off question gain, the goal really is to > >> put a context into ELSP[1] to cover the interrupt.) So if instead of > >> looking at the head of the queue, we look at the context in ELSP[1] we > >> can answer both of the questions more accurately -- we don't need to > >> rerun the submission tasklet unless our new request is important enough > >> to feed into, at least, ELSP[1]. > >> > >> v2: Add some comments from the discussion with Tvrtko. > >> v3: More commentary to cross-reference queue_request() > >> > >> References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports") > >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Michał Winiarski <michal.winiarski@intel.com> > >> Cc: Michel Thierry <michel.thierry@intel.com> > >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > From the discussion thread, > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > I assume Tvrtko is happy with the comments added based on that > > conversation. > > Yes, all fine! Forgot to say, thanks and pushed. If you are feeling better, there's the igt that purports to hit the issue as well: https://patchwork.freedesktop.org/series/41535/ Or one of the Ms! -Chris
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 12486d8f534b..a217b3fe5f0b 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -1687,6 +1687,9 @@ void intel_engines_park(struct drm_i915_private *i915) intel_engine_dump(engine, &p, NULL); } + /* Must be reset upon idling, or we may miss the busy wakeup. */ + GEM_BUG_ON(engine->execlists.queue_priority != INT_MIN); + if (engine->park) engine->park(engine); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 02b25bf2378a..665d9e82e954 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -713,8 +713,27 @@ static void execlists_dequeue(struct intel_engine_cs *engine) if (p->priority != I915_PRIORITY_NORMAL) kmem_cache_free(engine->i915->priorities, p); } + done: - execlists->queue_priority = rb ? to_priolist(rb)->priority : INT_MIN; + /* + * Here be a bit of magic! Or sleight-of-hand, whichever you prefer. + * + * We choose queue_priority such that if we add a request of greater + * priority than this, we kick the submission tasklet to decide on + * the right order of submitting the requests to hardware. We must + * also be prepared to reorder requests as they are in-flight on the + * HW. We derive the queue_priority then as the first "hole" in + * the HW submission ports and if there are no available slots, + * the priority of the lowest executing request, i.e. last. + * + * When we do receive a higher priority request ready to run from the + * user, see queue_request(), the queue_priority is bumped to that + * request triggering preemption on the next dequeue (or subsequent + * interrupt for secondary ports). + */ + execlists->queue_priority = + port != execlists->port ? rq_prio(last) : INT_MIN; + execlists->first = rb; if (submit) port_assign(port, last);
We can refine our current execlists->queue_priority if we inspect ELSP[1] rather than the head of the unsubmitted queue. Currently, we use the unsubmitted queue and say that if a subsequent request is more important than the current queue, we will rerun the submission tasklet to evaluate the need for preemption. However, we only want to preempt if we need to jump ahead of a currently executing request in ELSP. The second reason for running the submission tasklet is amalgamate requests into the active context on ELSP[0] to avoid a stall when ELSP[0] drains. (Though repeatedly amalgamating requests into the active context and triggering many lite-restore is off question gain, the goal really is to put a context into ELSP[1] to cover the interrupt.) So if instead of looking at the head of the queue, we look at the context in ELSP[1] we can answer both of the questions more accurately -- we don't need to rerun the submission tasklet unless our new request is important enough to feed into, at least, ELSP[1]. v2: Add some comments from the discussion with Tvrtko. v3: More commentary to cross-reference queue_request() References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michał Winiarski <michal.winiarski@intel.com> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 3 +++ drivers/gpu/drm/i915/intel_lrc.c | 21 ++++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-)