Re: [Intel-gfx] [PATCH 12/27] drm/i915: Only report a wakeup if the waiter was truly asleep

[Chris Wilson]

On Thu, Apr 20, 2017 at 02:30:21PM +0100, Tvrtko Ursulin wrote:
> 
> 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

I forgot this patch was inbetween the series, i.e. I am not pursuing
this one at the moment.

> >References: https://bugs.freedesktop.org/show_bug.cgi?id=17
> >Signed-off-by: Chris Wilson 
> >Cc: Tvrtko Ursulin 
> >Cc: Joonas Lahtinen 
> >---
> > 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(>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.

Best one I had was to store an atomic counter in each struct intel_wait
to determine if it was inside the wait-for-breadcrumb. But that is
duplicating on-cpu (with the same advantage of not being confused by any
sleep elsewhere in the check-breadcrumb path) and fundamentally less
precise.

> Or what you meant last time by not sure it was worth it was
> referring to the above?

I think the chance that this is affecting a missed breacrumb result is
small, certainly not with the regularity of snb-2600. I had pushed it to
the end, but obviously not far enough down the list. When I looked at
the list of patches, I actually though this was a different patch
"drm/i915: Only wake the waiter from the interrupt if passed"

My apologies for the noise.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 12/27] drm/i915: Only report a wakeup if the waiter was truly asleep

[Tvrtko Ursulin]

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=17
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
 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(>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(>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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 12/27] drm/i915: Only report a wakeup if the waiter was truly asleep

[Chris Wilson]

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=17
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
 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;
}
 
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(>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);
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx