On 19/04/2017 10:41, Chris Wilson wrote:
If we attempt to wake up a waiter, who is currently checking the seqno
it will be in the TASK_INTERRUPTIBLE state and ttwu will report success.
However, it is actually awake and functioning -- so delay reporting the
actual wake up until it sleeps.
v2: Defend against !CONFIG_SMP
v3: Don't filter out calls to wake_up_process
References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 ++++++++++++++++--
drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ++++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 9ccbf26124c6..808d3a3cda0a 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -27,6 +27,12 @@
#include "i915_drv.h"
+#ifdef CONFIG_SMP
+#define task_asleep(tsk) (!(tsk)->on_cpu)
+#else
+#define task_asleep(tsk) ((tsk) != current)
+#endif
+
static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
{
struct intel_wait *wait;
@@ -37,8 +43,16 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
wait = b->irq_wait;
if (wait) {
result = ENGINE_WAKEUP_WAITER;
- if (wake_up_process(wait->tsk))
+
+ /* Be careful not to report a successful wakeup if the waiter
+ * is currently processing the seqno, where it will have
+ * already called set_task_state(TASK_INTERRUPTIBLE).
+ */
+ if (task_asleep(wait->tsk))
result |= ENGINE_WAKEUP_ASLEEP;
+
+ if (wake_up_process(wait->tsk))
+ result |= ENGINE_WAKEUP_SUCCESS;
The rough idea I had of atomic_inc(&b->wakeup_cnt) here with
unconditional wake_up_process, coupled with atomic_dec_and_test in the
signaler would not work? I was thinking that would avoid signaler losing
the wakeup and avoid us having to touch the low level scheduler data.
Or what you meant last time by not sure it was worth it was referring to
the above?
Regards,
Tvrtko
}
return result;
@@ -98,7 +112,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
* but we still have a waiter. Assuming all batches complete within
* DRM_I915_HANGCHECK_JIFFIES [1.5s]!
*/
- if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP) {
+ if (intel_engine_wakeup(engine) == ENGINE_WAKEUP) {
missed_breadcrumb(engine);
mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
} else {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 00d36aa4e26d..d25b88467e5e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -668,6 +668,10 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
#define ENGINE_WAKEUP_WAITER BIT(0)
#define ENGINE_WAKEUP_ASLEEP BIT(1)
+#define ENGINE_WAKEUP_SUCCESS BIT(2)
+#define ENGINE_WAKEUP (ENGINE_WAKEUP_WAITER | \
+ ENGINE_WAKEUP_ASLEEP | \
+ ENGINE_WAKEUP_SUCCESS)
void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
