On 15/01/2018 21:24, Chris Wilson wrote:
Rather than have multiple locked instructions inside the notify_ring()
irq handler, move them inside the spinlock and reduce their intrinsic
locking.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem_request.c | 4 ++--
drivers/gpu/drm/i915/i915_irq.c | 6 +++---
drivers/gpu/drm/i915/intel_breadcrumbs.c | 13 ++++++++-----
drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +-
4 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 836db90ef81b..08bbd56277e5 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -1128,7 +1128,7 @@ static bool __i915_spin_request(const struct drm_i915_gem_request *req,
* takes to sleep on a request, on the order of a microsecond.
*/
- irq = atomic_read(&engine->irq_count);
+ irq = READ_ONCE(engine->breadcrumbs.irq_count);
timeout_us += local_clock_us(&cpu);
do {
if (i915_seqno_passed(intel_engine_get_seqno(engine), seqno))
@@ -1139,7 +1139,7 @@ static bool __i915_spin_request(const struct drm_i915_gem_request *req,
* assume we won't see one in the near future but require
* the engine->seqno_barrier() to fixup coherency.
*/
- if (atomic_read(&engine->irq_count) != irq)
+ if (READ_ONCE(engine->breadcrumbs.irq_count) != irq)
break;
if (signal_pending_state(state, current))
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0b272501b738..e5f76d580010 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1073,9 +1073,6 @@ static void notify_ring(struct intel_engine_cs *engine)
if (unlikely(!engine->breadcrumbs.irq_armed))
return;
- atomic_inc(&engine->irq_count);
- set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
-
rcu_read_lock();
spin_lock(&engine->breadcrumbs.irq_lock);
@@ -1107,6 +1104,9 @@ static void notify_ring(struct intel_engine_cs *engine)
i915_seqno_passed(seqno, wait->seqno - 1))
tsk = wait->tsk;
}
+
+ engine->breadcrumbs.irq_count++;
+ __set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
I'm nervous about moving the ENGINE_IRQ_BREADCRUMB setting to be
conditional. __i915_request_irq_complete documents the ordering of these
things is crucial so I worry we don't miss a wakeup. Once bitten twice
shy? Don't know..
irq_count change looks safe, so can I, once again, suggest to split into
two patches? :/
Regards,
Tvrtko
} else {
if (engine->breadcrumbs.irq_armed)
__intel_engine_disarm_breadcrumbs(engine);
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index e3667dc1e96d..7c82cfe23922 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -98,12 +98,14 @@ static void intel_breadcrumbs_hangcheck(struct timer_list *t)
struct intel_engine_cs *engine =
from_timer(engine, t, breadcrumbs.hangcheck);
struct intel_breadcrumbs *b = &engine->breadcrumbs;
+ unsigned int irq_count;
if (!b->irq_armed)
return;
- if (b->hangcheck_interrupts != atomic_read(&engine->irq_count)) {
- b->hangcheck_interrupts = atomic_read(&engine->irq_count);
+ irq_count = READ_ONCE(b->irq_count);
+ if (b->hangcheck_interrupts != irq_count) {
+ b->hangcheck_interrupts = irq_count;
mod_timer(&b->hangcheck, wait_timeout());
return;
}
@@ -176,7 +178,7 @@ static void irq_enable(struct intel_engine_cs *engine)
* we still need to force the barrier before reading the seqno,
* just in case.
*/
- set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
+ __set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
/* Caller disables interrupts */
spin_lock(&engine->i915->irq_lock);
@@ -270,13 +272,14 @@ static bool use_fake_irq(const struct intel_breadcrumbs *b)
if (!test_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings))
return false;
- /* Only start with the heavy weight fake irq timer if we have not
+ /*
+ * Only start with the heavy weight fake irq timer if we have not
* seen any interrupts since enabling it the first time. If the
* interrupts are still arriving, it means we made a mistake in our
* engine->seqno_barrier(), a timing error that should be transient
* and unlikely to reoccur.
*/
- return atomic_read(&engine->irq_count) == b->hangcheck_interrupts;
+ return READ_ONCE(b->irq_count) == b->hangcheck_interrupts;
}
static void enable_fake_irq(struct intel_breadcrumbs *b)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c5ff203e42d6..f406d0ff4612 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -305,7 +305,6 @@ struct intel_engine_cs {
struct drm_i915_gem_object *default_state;
- atomic_t irq_count;
unsigned long irq_posted;
#define ENGINE_IRQ_BREADCRUMB 0
#define ENGINE_IRQ_EXECLIST 1
@@ -340,6 +339,7 @@ struct intel_engine_cs {
unsigned int hangcheck_interrupts;
unsigned int irq_enabled;
+ unsigned int irq_count;
bool irq_armed : 1;
I915_SELFTEST_DECLARE(bool mock : 1);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
