| Message ID | 20201124114219.29020-7-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] drm/i915/gem: Drop free_work for GEM contexts | expand |
On 24/11/2020 11:42, Chris Wilson wrote: > Pull the repeated check for the last active request being completed to a > single spot, when deciding whether or not execlist preemption is > required. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/intel_lrc.c | 15 ++++----------- > 1 file changed, 4 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index cf11cbac241b..43703efb36d1 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -2141,12 +2141,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > */ > > if ((last = *active)) { > - if (need_preempt(engine, last, rb)) { > - if (i915_request_completed(last)) { > - tasklet_hi_schedule(&execlists->tasklet); > - return; > - } > - > + if (i915_request_completed(last)) { > + goto check_secondary; > + } else if (need_preempt(engine, last, rb)) { > ENGINE_TRACE(engine, > "preempting last=%llx:%lld, prio=%d, hint=%d\n", > last->fence.context, > @@ -2174,11 +2171,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > last = NULL; > } else if (need_timeslice(engine, last, rb) && > timeslice_expired(execlists, last)) { > - if (i915_request_completed(last)) { > - tasklet_hi_schedule(&execlists->tasklet); > - return; > - } > - > ENGINE_TRACE(engine, > "expired last=%llx:%lld, prio=%d, hint=%d, yield?=%s\n", > last->fence.context, > @@ -2214,6 +2206,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > * we hopefully coalesce several updates into a single > * submission. > */ > +check_secondary: > if (!list_is_last(&last->sched.link, > &engine->active.requests)) { Is there a tasklet_hi_schedule in here? I see set_timeslice in my checkout. Regards, Tvrtko > /* >
Quoting Tvrtko Ursulin (2020-11-24 17:19:23) > > On 24/11/2020 11:42, Chris Wilson wrote: > > Pull the repeated check for the last active request being completed to a > > single spot, when deciding whether or not execlist preemption is > > required. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/gt/intel_lrc.c | 15 ++++----------- > > 1 file changed, 4 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > > index cf11cbac241b..43703efb36d1 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > @@ -2141,12 +2141,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > > */ > > > > if ((last = *active)) { > > - if (need_preempt(engine, last, rb)) { > > - if (i915_request_completed(last)) { > > - tasklet_hi_schedule(&execlists->tasklet); > > - return; > > - } > > - > > + if (i915_request_completed(last)) { > > + goto check_secondary; > > + } else if (need_preempt(engine, last, rb)) { > > ENGINE_TRACE(engine, > > "preempting last=%llx:%lld, prio=%d, hint=%d\n", > > last->fence.context, > > @@ -2174,11 +2171,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > > last = NULL; > > } else if (need_timeslice(engine, last, rb) && > > timeslice_expired(execlists, last)) { > > - if (i915_request_completed(last)) { > > - tasklet_hi_schedule(&execlists->tasklet); > > - return; > > - } > > - > > ENGINE_TRACE(engine, > > "expired last=%llx:%lld, prio=%d, hint=%d, yield?=%s\n", > > last->fence.context, > > @@ -2214,6 +2206,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) > > * we hopefully coalesce several updates into a single > > * submission. > > */ > > +check_secondary: > > if (!list_is_last(&last->sched.link, > > &engine->active.requests)) { > > Is there a tasklet_hi_schedule in here? I see set_timeslice in my checkout. That tasklet_hi_schedule() was a mistake. It just devolves into a busy-spinner as we wait for HW to send the next event, which turns out not to be as instantaneous as hoped. I recall leaving the imprint of my palm on my face when I figured out what was causing the spike in the profile. -Chris
On 24/11/2020 17:38, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2020-11-24 17:19:23) >> >> On 24/11/2020 11:42, Chris Wilson wrote: >>> Pull the repeated check for the last active request being completed to a >>> single spot, when deciding whether or not execlist preemption is >>> required. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> drivers/gpu/drm/i915/gt/intel_lrc.c | 15 ++++----------- >>> 1 file changed, 4 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c >>> index cf11cbac241b..43703efb36d1 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >>> @@ -2141,12 +2141,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine) >>> */ >>> >>> if ((last = *active)) { >>> - if (need_preempt(engine, last, rb)) { >>> - if (i915_request_completed(last)) { >>> - tasklet_hi_schedule(&execlists->tasklet); >>> - return; >>> - } >>> - >>> + if (i915_request_completed(last)) { >>> + goto check_secondary; >>> + } else if (need_preempt(engine, last, rb)) { >>> ENGINE_TRACE(engine, >>> "preempting last=%llx:%lld, prio=%d, hint=%d\n", >>> last->fence.context, >>> @@ -2174,11 +2171,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine) >>> last = NULL; >>> } else if (need_timeslice(engine, last, rb) && >>> timeslice_expired(execlists, last)) { >>> - if (i915_request_completed(last)) { >>> - tasklet_hi_schedule(&execlists->tasklet); >>> - return; >>> - } >>> - >>> ENGINE_TRACE(engine, >>> "expired last=%llx:%lld, prio=%d, hint=%d, yield?=%s\n", >>> last->fence.context, >>> @@ -2214,6 +2206,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) >>> * we hopefully coalesce several updates into a single >>> * submission. >>> */ >>> +check_secondary: >>> if (!list_is_last(&last->sched.link, >>> &engine->active.requests)) { >> >> Is there a tasklet_hi_schedule in here? I see set_timeslice in my checkout. > > That tasklet_hi_schedule() was a mistake. It just devolves into a > busy-spinner as we wait for HW to send the next event, which turns out > not to be as instantaneous as hoped. > > I recall leaving the imprint of my palm on my face when I figured out > what was causing the spike in the profile. Okay, if you mention that in the commit message you can add my rb. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index cf11cbac241b..43703efb36d1 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -2141,12 +2141,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine) */ if ((last = *active)) { - if (need_preempt(engine, last, rb)) { - if (i915_request_completed(last)) { - tasklet_hi_schedule(&execlists->tasklet); - return; - } - + if (i915_request_completed(last)) { + goto check_secondary; + } else if (need_preempt(engine, last, rb)) { ENGINE_TRACE(engine, "preempting last=%llx:%lld, prio=%d, hint=%d\n", last->fence.context, @@ -2174,11 +2171,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine) last = NULL; } else if (need_timeslice(engine, last, rb) && timeslice_expired(execlists, last)) { - if (i915_request_completed(last)) { - tasklet_hi_schedule(&execlists->tasklet); - return; - } - ENGINE_TRACE(engine, "expired last=%llx:%lld, prio=%d, hint=%d, yield?=%s\n", last->fence.context, @@ -2214,6 +2206,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine) * we hopefully coalesce several updates into a single * submission. */ +check_secondary: if (!list_is_last(&last->sched.link, &engine->active.requests)) { /*
Pull the repeated check for the last active request being completed to a single spot, when deciding whether or not execlist preemption is required. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/intel_lrc.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)