On 06/04/2017 18:40, Tvrtko Ursulin wrote:
On 06/04/2017 10:30, 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
It really bothers to fish into this low level info which probably isn't
intended to be used from the outside.
+
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;
And this still has the problem of not being atomic between reporting the
two flags. So the reported status can be false which also bothers me.
I will need to take some more time thinking about this.
Warning, the idea below is potentially unrefined! :)
How about a scheme where on wake_up we would atomic_inc our own wakeup
counter, and then the signaller keeps atomic_dec_and_test one item at a
time until it has consumed all the wakeups? The code in
intel_breadcrumbs_hangcheck only declares a missed breadcrumb if the
wakeup counter is one, meaning this was the first wakeup?
Regards,
Tvrtko
+
+ if (wake_up_process(wait->tsk))
+ result |= ENGINE_WAKEUP_SUCCESS;
}
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 cbe61d3f31da..974a5928bec9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -663,6 +663,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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
