On 25/06/2018 10:48, Chris Wilson wrote:
In the following patch, we will process the CSB events under the
timeline.lock and not serailiased by the tasklet. This also means that we
Typo in serialised.
will need to protect access to common variables such as
execlists->csb_head with the timeline.lock during reset.
v2: Move sync_irq to avoid deadlocks between taking timeline.lock from
our interrupt handler.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/intel_lrc.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b5c809201c7a..2cbb293fb409 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -871,7 +871,6 @@ static void reset_irq(struct intel_engine_cs *engine)
{
/* Mark all CS interrupts as complete */
smp_store_mb(engine->execlists.active, 0);
- synchronize_hardirq(engine->i915->drm.irq);
clear_gtiir(engine);
@@ -912,6 +911,8 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
/* Cancel the requests on the HW and clear the ELSP tracker. */
execlists_cancel_port_requests(execlists);
+
+ synchronize_hardirq(engine->i915->drm.irq);
This is why I hate trickery. It used to be smp_store_mb then
synchronize_hardirq, and now it would be the opposite. I have no idea
what's broken before, and what's after, or if all is just pointless.
Hmmm... even funnier, it seems that in the current code we already have
synchronize_hardirq from the irqdisabled section. Should that be fixed
with an explicit patch first?
reset_irq(engine);
spin_lock(&engine->timeline.lock);
@@ -1969,8 +1970,8 @@ static void execlists_reset(struct intel_engine_cs *engine,
engine->name, request ? request->global_seqno : 0,
intel_engine_get_seqno(engine));
- /* See execlists_cancel_requests() for the irq/spinlock split. */
- local_irq_save(flags);
+ synchronize_hardirq(engine->i915->drm.irq);
+ spin_lock_irqsave(&engine->timeline.lock, flags);
What is the point of synchronize_hardirq here? If it was running the
spinlock would wait for it, if it was pending the opposite or nothing,
but if we wait for it before the spinlock what says new one cannot
become pending between synchronize_hardirq and the spinlock?
/*
* Catch up with any missed context-switch interrupts.
@@ -1985,14 +1986,12 @@ static void execlists_reset(struct intel_engine_cs *engine,
reset_irq(engine);
/* Push back any incomplete requests for replay after the reset. */
- spin_lock(&engine->timeline.lock);
__unwind_incomplete_requests(engine);
- spin_unlock(&engine->timeline.lock);
/* Following the reset, we need to reload the CSB read/write pointers */
engine->execlists.csb_head = GEN8_CSB_ENTRIES - 1;
- local_irq_restore(flags);
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
/*
* If the request was innocent, we leave the request in the ELSP
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
