[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=100007
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >---
> > 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.

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