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@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. 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
