On 21/01/2021 00:32, Chris Wilson wrote:
Reduce the bulk of execlists_submission_tasklet by moving the unlikely
reset function out of line.
add/remove: 1/0 grow/shrink: 0/1 up/down: 960/-935 (25)
Function old new delta
execlists_reset - 960 +960
execlists_submission_tasklet 6629 5694 -935
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
.../drm/i915/gt/intel_execlists_submission.c | 36 +++++++++----------
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 740ff05fd692..43cc85241886 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -2299,10 +2299,13 @@ static void execlists_capture(struct intel_engine_cs *engine)
kfree(cap);
}
-static void execlists_reset(struct intel_engine_cs *engine, const char *msg)
+static noinline void execlists_reset(struct intel_engine_cs *engine)
{
+ struct intel_engine_execlists *el = &engine->execlists;
const unsigned int bit = I915_RESET_ENGINE + engine->id;
unsigned long *lock = &engine->gt->reset.flags;
+ unsigned long eir = fetch_and_zero(&el->error_interrupt);
We got the locking wrong for this one. Irq handler side is under
gt->irq_lock, but there are unlocked rmw cycles in the tasklet. Not
counting this fetch_and_zero which is also unlocked.
If we nest gt->irq_lock under the engine->active.lock here, or the
opposite from the irq_handler, plus short locked sections in the tasklet
and csb handling. I *think* both options could be fine from locking order.
Regards,
Tvrtko
+ const char *msg;
if (!intel_has_reset_engine(engine->gt))
return;
@@ -2310,16 +2313,25 @@ static void execlists_reset(struct intel_engine_cs *engine, const char *msg)
if (test_and_set_bit(bit, lock))
return;
+ /* Generate the error message in priority wrt to the user! */
+ if (eir & GENMASK(15, 0))
+ msg = "CS error"; /* thrown by a user payload */
+ else if (eir & ERROR_CSB)
+ msg = "invalid CSB event";
+ else if (eir & ERROR_PREEMPT)
+ msg = "preemption time out";
+ else
+ msg = "internal error";
ENGINE_TRACE(engine, "reset for %s\n", msg);
/* Mark this tasklet as disabled to avoid waiting for it to complete */
- tasklet_disable_nosync(&engine->execlists.tasklet);
+ tasklet_disable_nosync(&el->tasklet);
ring_set_paused(engine, 1); /* Freeze the current request in place */
execlists_capture(engine);
intel_engine_reset(engine, msg);
- tasklet_enable(&engine->execlists.tasklet);
+ tasklet_enable(&el->tasklet);
clear_and_wake_up_bit(bit, lock);
}
@@ -2355,22 +2367,8 @@ static void execlists_submission_tasklet(unsigned long data)
engine->execlists.error_interrupt |= ERROR_PREEMPT;
}
- if (unlikely(READ_ONCE(engine->execlists.error_interrupt))) {
- const char *msg;
-
- /* Generate the error message in priority wrt to the user! */
- if (engine->execlists.error_interrupt & GENMASK(15, 0))
- msg = "CS error"; /* thrown by a user payload */
- else if (engine->execlists.error_interrupt & ERROR_CSB)
- msg = "invalid CSB event";
- else if (engine->execlists.error_interrupt & ERROR_PREEMPT)
- msg = "preemption time out";
- else
- msg = "internal error";
-
- engine->execlists.error_interrupt = 0;
- execlists_reset(engine, msg);
- }
+ if (unlikely(READ_ONCE(engine->execlists.error_interrupt)))
+ execlists_reset(engine);
if (!engine->execlists.pending[0]) {
execlists_dequeue_irq(engine);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
