| Message ID | 20190812091045.29587-6-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] drm/i915/execlists: Avoid sync calls during park | expand |
Quoting Chris Wilson (2019-08-12 10:10:43) > For the guc, we need to keep the engine awake (and not parked) and not > just the gt. If we let the engine park, we disable the irq and stop > processing the tasklet, leaving state outstanding inside the tasklet. > > The downside is, of course, we now have to wait until the tasklet is run > before we consider the engine idle. Fwiw, because of this I think it may be preferable to keep to using GT pm for the tasklet; and apply Daniele's patch to keep NEEDS_BREADCRUMB_TASKLET set (which is the right thing to do anyway now that we stop switching between submission modes). -Chris
On 8/12/19 3:44 AM, Chris Wilson wrote: > Quoting Chris Wilson (2019-08-12 10:10:43) >> For the guc, we need to keep the engine awake (and not parked) and not >> just the gt. If we let the engine park, we disable the irq and stop >> processing the tasklet, leaving state outstanding inside the tasklet. >> >> The downside is, of course, we now have to wait until the tasklet is run >> before we consider the engine idle. > > Fwiw, because of this I think it may be preferable to keep to using GT > pm for the tasklet; and apply Daniele's patch to keep > NEEDS_BREADCRUMB_TASKLET set (which is the right thing to do anyway now > that we stop switching between submission modes). > -Chris > Given that the GuC submission code is about to undergo a rework I believe it'd be better to keep the fix contained to the GuC side of things for now and avoid impacting the more general request paths (i.e. patch 4 in this series, unless you still want that for other reasons). I'll clean up and send the other patch. Daniele
Quoting Daniele Ceraolo Spurio (2019-08-12 21:38:39) > > > On 8/12/19 3:44 AM, Chris Wilson wrote: > > Quoting Chris Wilson (2019-08-12 10:10:43) > >> For the guc, we need to keep the engine awake (and not parked) and not > >> just the gt. If we let the engine park, we disable the irq and stop > >> processing the tasklet, leaving state outstanding inside the tasklet. > >> > >> The downside is, of course, we now have to wait until the tasklet is run > >> before we consider the engine idle. > > > > Fwiw, because of this I think it may be preferable to keep to using GT > > pm for the tasklet; and apply Daniele's patch to keep > > NEEDS_BREADCRUMB_TASKLET set (which is the right thing to do anyway now > > that we stop switching between submission modes). > > -Chris > > > > Given that the GuC submission code is about to undergo a rework I > believe it'd be better to keep the fix contained to the GuC side of > things for now and avoid impacting the more general request paths (i.e. > patch 4 in this series, unless you still want that for other reasons). > I'll clean up and send the other patch. Oh, we need that anyway :) https://bugs.freedesktop.org/show_bug.cgi?id=111378 And it actually clarified some of the heartbeat code, so it's an eventual win. -Chris
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 5bf838223cf9..52edfe8d1c60 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -534,9 +534,10 @@ static inline int rq_prio(const struct i915_request *rq) static struct i915_request *schedule_in(struct i915_request *rq, int idx) { + GEM_BUG_ON(!intel_engine_pm_is_awake(rq->engine)); trace_i915_request_in(rq, idx); - intel_gt_pm_get(rq->engine->gt); + intel_engine_pm_get(rq->engine); return i915_request_get(rq); } @@ -544,7 +545,7 @@ static void schedule_out(struct i915_request *rq) { trace_i915_request_out(rq); - intel_gt_pm_put(rq->engine->gt); + intel_engine_pm_put(rq->engine); i915_request_put(rq); } @@ -610,8 +611,6 @@ static void guc_submission_tasklet(unsigned long data) struct i915_request **port, *rq; unsigned long flags; - spin_lock_irqsave(&engine->active.lock, flags); - for (port = execlists->inflight; (rq = *port); port++) { if (!i915_request_completed(rq)) break; @@ -624,8 +623,8 @@ static void guc_submission_tasklet(unsigned long data) memmove(execlists->inflight, port, rem * sizeof(*port)); } + spin_lock_irqsave(&engine->active.lock, flags); __guc_dequeue(engine); - spin_unlock_irqrestore(&engine->active.lock, flags); }
For the guc, we need to keep the engine awake (and not parked) and not just the gt. If we let the engine park, we disable the irq and stop processing the tasklet, leaving state outstanding inside the tasklet. The downside is, of course, we now have to wait until the tasklet is run before we consider the engine idle. Reported-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)