[PATCH 1/6] drm/i915/breadcrumbs: Use booleans for intel_breadcrumbs_busy()

[PATCH 1/6] drm/i915/breadcrumbs: Use booleans for intel_breadcrumbs_busy()

[Chris Wilson]

Since commit 9b6586ae9f6b ("drm/i915: Keep a global seqno per-engine")
converted intel_breadcrumbs_busy() to reporting a single boolean, we
need only compute a boolean internally (and not needlessly compute the
flag).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 2a1ed6d7ad4d..3f222dee4c25 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -828,12 +828,12 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
 
 	if (b->irq_wait) {
 		wake_up_process(b->irq_wait->tsk);
-		busy |= intel_engine_flag(engine);
+		busy = true;
 	}
 
 	if (rcu_access_pointer(b->first_signal)) {
 		wake_up_process(b->signaler);
-		busy |= intel_engine_flag(engine);
+		busy = true;
 	}
 
 	spin_unlock_irq(&b->rb_lock);
-- 
2.11.0

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

[PATCH 2/6] drm/i915/breadcrumbs: Assert that irqs are disabled as we update the bottom-half

[Chris Wilson]

Check that we have disabled irqs before we take the spin_lock around
reassigned the breadcrumbs.irq_wait.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 3f222dee4c25..35529b35a276 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -301,8 +301,11 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 
+	GEM_BUG_ON(!irqs_disabled());
+
 	spin_lock(&b->irq_lock);
 	GEM_BUG_ON(!b->irq_armed);
+	GEM_BUG_ON(!b->irq_wait);
 	b->irq_wait = to_wait(next);
 	spin_unlock(&b->irq_lock);
 
@@ -395,8 +398,10 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	}
 
 	if (first) {
-		spin_lock(&b->irq_lock);
 		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
+		GEM_BUG_ON(!irqs_disabled());
+
+		spin_lock(&b->irq_lock);
 		b->irq_wait = wait;
 		/* After assigning ourselves as the new bottom-half, we must
 		 * perform a cursory check to prevent a missed interrupt.
-- 
2.11.0

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

[PATCH 3/6] drm/i915/breadcrumbs: Update bottom-half before marking as complete

[Chris Wilson]

When adding a new request to the breadcrumb rbtree, we mark all those
requests inside the rbtree that are already completed as complete. This
wakes those waiters up and allows them to skip the spinlock before
returning to userspace. If one of those is the current bottom-half, it
may then overwrite intel_wait as the interrupt handler dereferences it.

Fixes: 56299fb7d904 ("drm/i915: Signal first fence from irq handler if complete")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 34 ++++++++++++++++----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 35529b35a276..31e7c25013a4 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -381,24 +381,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	rb_link_node(&wait->node, parent, p);
 	rb_insert_color(&wait->node, &b->waiters);
 
-	if (completed) {
-		struct rb_node *next = rb_next(completed);
-
-		GEM_BUG_ON(!next && !first);
-		if (next && next != &wait->node) {
-			GEM_BUG_ON(first);
-			__intel_breadcrumbs_next(engine, next);
-		}
-
-		do {
-			struct intel_wait *crumb = to_wait(completed);
-			completed = rb_prev(completed);
-			__intel_breadcrumbs_finish(b, crumb);
-		} while (completed);
-	}
-
 	if (first) {
-		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
 		GEM_BUG_ON(!irqs_disabled());
 
 		spin_lock(&b->irq_lock);
@@ -414,6 +397,23 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 		__intel_breadcrumbs_enable_irq(b);
 		spin_unlock(&b->irq_lock);
 	}
+
+	if (completed) {
+		struct rb_node *next = rb_next(completed);
+
+		GEM_BUG_ON(!next && !first);
+		if (next && next != &wait->node) {
+			GEM_BUG_ON(first);
+			__intel_breadcrumbs_next(engine, next);
+		}
+
+		do {
+			struct intel_wait *crumb = to_wait(completed);
+			completed = rb_prev(completed);
+			__intel_breadcrumbs_finish(b, crumb);
+		} while (completed);
+	}
+
 	GEM_BUG_ON(!b->irq_wait);
 	GEM_BUG_ON(rb_first(&b->waiters) != &b->irq_wait->node);
 
-- 
2.11.0

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

[PATCH 4/6] drm/i915/breadcrumbs: Disable interrupt bottom-half first on idling

[Chris Wilson]

Before walking the rbtree of waiters (marking them as complete and waking
them), decouple the interrupt handler. This prevents a race between the
missed waiter waking up and removing its intel_wait (which skips
checking the lock) and the interrupt handler dereferencing the
intel_wait. (Though we do not expect to encounter waiters during idle!)

Fixes: e1c0c91bdaec ("drm/i915: Wake up all waiters before idling")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 31e7c25013a4..ebcb595001fc 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -179,7 +179,7 @@ void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
-	struct intel_wait *wait, *n;
+	struct intel_wait *wait, *n, *first;
 
 	if (!b->irq_armed)
 		return;
@@ -190,18 +190,19 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
 	 */
 
 	spin_lock_irq(&b->rb_lock);
+
+	spin_lock(&b->irq_lock);
+	first = fetch_and_zero(&b->irq_wait);
+	__intel_engine_disarm_breadcrumbs(engine);
+	spin_unlock(&b->irq_lock);
+
 	rbtree_postorder_for_each_entry_safe(wait, n, &b->waiters, node) {
 		RB_CLEAR_NODE(&wait->node);
-		if (wake_up_process(wait->tsk) && wait == b->irq_wait)
+		if (wake_up_process(wait->tsk) && wait == first)
 			missed_breadcrumb(engine);
 	}
 	b->waiters = RB_ROOT;
 
-	spin_lock(&b->irq_lock);
-	b->irq_wait = NULL;
-	__intel_engine_disarm_breadcrumbs(engine);
-	spin_unlock(&b->irq_lock);
-
 	spin_unlock_irq(&b->rb_lock);
 }
 
-- 
2.11.0

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

[PATCH 5/6] drm/i915/breadcrumbs: Assert that we do not shortcut the current bottom-half

[Chris Wilson]

We need to ensure that we always serialize updates to the bottom-half
using the breadcrumbs.irq_lock so that we don't race with a concurrent
interrupt handler. This is most important just prior to leaving the
waiter (when the intel_wait will be overwritten), so make sure we are
not the current bottom-half when skipping the irq locks.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index ebcb595001fc..d10148567dbc 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -287,6 +287,7 @@ static inline void __intel_breadcrumbs_finish(struct intel_breadcrumbs *b,
 					      struct intel_wait *wait)
 {
 	lockdep_assert_held(&b->rb_lock);
+	GEM_BUG_ON(b->irq_wait == wait);
 
 	/* This request is completed, so remove it from the tree, mark it as
 	 * complete, and *then* wake up the associated task.
@@ -517,8 +518,10 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
 	 * the tree by the bottom-half to avoid contention on the spinlock
 	 * by the herd.
 	 */
-	if (RB_EMPTY_NODE(&wait->node))
+	if (RB_EMPTY_NODE(&wait->node)) {
+		GEM_BUG_ON(READ_ONCE(b->irq_wait) == wait);
 		return;
+	}
 
 	spin_lock_irq(&b->rb_lock);
 	__intel_engine_remove_wait(engine, wait);
-- 
2.11.0

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

[PATCH 6/6] drm/i915: Only attempt to signal the request once from the interrupt handler

[Chris Wilson]

Check that signaled bit on the request->fence before acquiring a
reference to the request for signaling later in the interrupt handler.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 31f0d7c8992f..e646c4eba65d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1056,7 +1056,9 @@ static void notify_ring(struct intel_engine_cs *engine)
 		 * and many waiters.
 		 */
 		if (i915_seqno_passed(intel_engine_get_seqno(engine),
-				      wait->seqno))
+				      wait->seqno) &&
+		    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+			      &wait->request->fence.flags))
 			rq = i915_gem_request_get(wait->request);
 
 		wake_up_process(wait->tsk);
-- 
2.11.0

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

✓ Fi.CI.BAT: success for series starting with [1/6] drm/i915/breadcrumbs: Use booleans for intel_breadcrumbs_busy()

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/breadcrumbs: Use booleans for intel_breadcrumbs_busy()
URL   : https://patchwork.freedesktop.org/series/21289/
State : success

== Summary ==

Series 21289v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/21289/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                skip       -> PASS       (fi-snb-2600)

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 467s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 585s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 539s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 543s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 501s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 499s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 438s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 499s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 492s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 486s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 481s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 482s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 519s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 551s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 416s

25f04730646ed8f7bc59e9e9d1cc9b4c9ecbc09d drm-tip: 2017y-03m-15d-12h-56m-15s UTC integration manifest
517d981 drm/i915: Only attempt to signal the request once from the interrupt handler
9fd8a88 drm/i915/breadcrumbs: Assert that we do not shortcut the current bottom-half
d108e31 drm/i915/breadcrumbs: Disable interrupt bottom-half first on idling
12d7b43 drm/i915/breadcrumbs: Update bottom-half before marking as complete
ce21fdf drm/i915/breadcrumbs: Assert that irqs are disabled as we update the bottom-half
2d4af9c drm/i915/breadcrumbs: Use booleans for intel_breadcrumbs_busy()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4182/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915/breadcrumbs: Use booleans for intel_breadcrumbs_busy()

[Tvrtko Ursulin]

On 15/03/2017 14:01, Chris Wilson wrote:
> Since commit 9b6586ae9f6b ("drm/i915: Keep a global seqno per-engine")
> converted intel_breadcrumbs_busy() to reporting a single boolean, we
> need only compute a boolean internally (and not needlessly compute the
> flag).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 2a1ed6d7ad4d..3f222dee4c25 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -828,12 +828,12 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
>
>  	if (b->irq_wait) {
>  		wake_up_process(b->irq_wait->tsk);
> -		busy |= intel_engine_flag(engine);
> +		busy = true;
>  	}
>
>  	if (rcu_access_pointer(b->first_signal)) {
>  		wake_up_process(b->signaler);
> -		busy |= intel_engine_flag(engine);
> +		busy = true;
>  	}
>
>  	spin_unlock_irq(&b->rb_lock);
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 2/6] drm/i915/breadcrumbs: Assert that irqs are disabled as we update the bottom-half

[Tvrtko Ursulin]

On 15/03/2017 14:01, Chris Wilson wrote:
> Check that we have disabled irqs before we take the spin_lock around
> reassigned the breadcrumbs.irq_wait.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 3f222dee4c25..35529b35a276 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -301,8 +301,11 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
>  {
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>
> +	GEM_BUG_ON(!irqs_disabled());
> +
>  	spin_lock(&b->irq_lock);
>  	GEM_BUG_ON(!b->irq_armed);
> +	GEM_BUG_ON(!b->irq_wait);
>  	b->irq_wait = to_wait(next);
>  	spin_unlock(&b->irq_lock);
>
> @@ -395,8 +398,10 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  	}
>
>  	if (first) {
> -		spin_lock(&b->irq_lock);
>  		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
> +		GEM_BUG_ON(!irqs_disabled());
> +
> +		spin_lock(&b->irq_lock);
>  		b->irq_wait = wait;
>  		/* After assigning ourselves as the new bottom-half, we must
>  		 * perform a cursory check to prevent a missed interrupt.
>

A single GEM_BUG_ON(!irqs_disabled()) at the top of 
__intel_engine_add_wait might be more logical?

As a weakly related side note, there is a stale comment mentioning 
b->lock in intel_engine_enable_signalling.

Regards,

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

Re: [PATCH 2/6] drm/i915/breadcrumbs: Assert that irqs are disabled as we update the bottom-half

[Chris Wilson]

On Wed, Mar 15, 2017 at 06:20:16PM +0000, Tvrtko Ursulin wrote:
> 
> On 15/03/2017 14:01, Chris Wilson wrote:
> >Check that we have disabled irqs before we take the spin_lock around
> >reassigned the breadcrumbs.irq_wait.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> > drivers/gpu/drm/i915/intel_breadcrumbs.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >index 3f222dee4c25..35529b35a276 100644
> >--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >@@ -301,8 +301,11 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
> > {
> > 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> >
> >+	GEM_BUG_ON(!irqs_disabled());
> >+
> > 	spin_lock(&b->irq_lock);
> > 	GEM_BUG_ON(!b->irq_armed);
> >+	GEM_BUG_ON(!b->irq_wait);
> > 	b->irq_wait = to_wait(next);
> > 	spin_unlock(&b->irq_lock);
> >
> >@@ -395,8 +398,10 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
> > 	}
> >
> > 	if (first) {
> >-		spin_lock(&b->irq_lock);
> > 		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
> >+		GEM_BUG_ON(!irqs_disabled());
> >+
> >+		spin_lock(&b->irq_lock);
> > 		b->irq_wait = wait;
> > 		/* After assigning ourselves as the new bottom-half, we must
> > 		 * perform a cursory check to prevent a missed interrupt.
> >
> 
> A single GEM_BUG_ON(!irqs_disabled()) at the top of
> __intel_engine_add_wait might be more logical?

I wanted to associate it with b->irq_lock, that was my thinking.
b->rb_lock also sadly has to be irqsafe.

__intel_breadcrumbs_next() also serves remove_wait, did you mean to
remove the assert there as well?

We can safely ignore this patch, it should be catered by lockdep fairly
well, I was just being paranoid and going through the possible causes
and documenting my progress.

> As a weakly related side note, there is a stale comment mentioning
> b->lock in intel_engine_enable_signalling.

Ta.
-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: [PATCH 3/6] drm/i915/breadcrumbs: Update bottom-half before marking as complete

[Tvrtko Ursulin]

On 15/03/2017 14:01, Chris Wilson wrote:
> When adding a new request to the breadcrumb rbtree, we mark all those
> requests inside the rbtree that are already completed as complete. This
> wakes those waiters up and allows them to skip the spinlock before
> returning to userspace. If one of those is the current bottom-half, it
> may then overwrite intel_wait as the interrupt handler dereferences it.

Last sentence sounds suspicious. The interrupts are disabled when this 
runs and locking is in place. And since the fix is to move the 
"completed" block after the "first", I wonder what can get overwritten 
by who?

Oh.. __intel_breadcrumbs_finish. But how does re-ordering help? 
Shouldn't the fix be to skip the bottom-half assignment if the 
"complete" loop has processed the waiter getting added?

Regards,

Tvrtko

> Fixes: 56299fb7d904 ("drm/i915: Signal first fence from irq handler if complete")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 34 ++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 35529b35a276..31e7c25013a4 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -381,24 +381,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  	rb_link_node(&wait->node, parent, p);
>  	rb_insert_color(&wait->node, &b->waiters);
>
> -	if (completed) {
> -		struct rb_node *next = rb_next(completed);
> -
> -		GEM_BUG_ON(!next && !first);
> -		if (next && next != &wait->node) {
> -			GEM_BUG_ON(first);
> -			__intel_breadcrumbs_next(engine, next);
> -		}
> -
> -		do {
> -			struct intel_wait *crumb = to_wait(completed);
> -			completed = rb_prev(completed);
> -			__intel_breadcrumbs_finish(b, crumb);
> -		} while (completed);
> -	}
> -
>  	if (first) {
> -		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
>  		GEM_BUG_ON(!irqs_disabled());
>
>  		spin_lock(&b->irq_lock);
> @@ -414,6 +397,23 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  		__intel_breadcrumbs_enable_irq(b);
>  		spin_unlock(&b->irq_lock);
>  	}
> +
> +	if (completed) {
> +		struct rb_node *next = rb_next(completed);
> +
> +		GEM_BUG_ON(!next && !first);
> +		if (next && next != &wait->node) {
> +			GEM_BUG_ON(first);
> +			__intel_breadcrumbs_next(engine, next);
> +		}
> +
> +		do {
> +			struct intel_wait *crumb = to_wait(completed);
> +			completed = rb_prev(completed);
> +			__intel_breadcrumbs_finish(b, crumb);
> +		} while (completed);
> +	}
> +
>  	GEM_BUG_ON(!b->irq_wait);
>  	GEM_BUG_ON(rb_first(&b->waiters) != &b->irq_wait->node);
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915/breadcrumbs: Assert that irqs are disabled as we update the bottom-half

[Tvrtko Ursulin]

On 15/03/2017 18:32, Chris Wilson wrote:
> On Wed, Mar 15, 2017 at 06:20:16PM +0000, Tvrtko Ursulin wrote:
>>
>> On 15/03/2017 14:01, Chris Wilson wrote:
>>> Check that we have disabled irqs before we take the spin_lock around
>>> reassigned the breadcrumbs.irq_wait.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_breadcrumbs.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> index 3f222dee4c25..35529b35a276 100644
>>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>>> @@ -301,8 +301,11 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
>>> {
>>> 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>>>
>>> +	GEM_BUG_ON(!irqs_disabled());
>>> +
>>> 	spin_lock(&b->irq_lock);
>>> 	GEM_BUG_ON(!b->irq_armed);
>>> +	GEM_BUG_ON(!b->irq_wait);
>>> 	b->irq_wait = to_wait(next);
>>> 	spin_unlock(&b->irq_lock);
>>>
>>> @@ -395,8 +398,10 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>>> 	}
>>>
>>> 	if (first) {
>>> -		spin_lock(&b->irq_lock);
>>> 		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
>>> +		GEM_BUG_ON(!irqs_disabled());
>>> +
>>> +		spin_lock(&b->irq_lock);
>>> 		b->irq_wait = wait;
>>> 		/* After assigning ourselves as the new bottom-half, we must
>>> 		 * perform a cursory check to prevent a missed interrupt.
>>>
>>
>> A single GEM_BUG_ON(!irqs_disabled()) at the top of
>> __intel_engine_add_wait might be more logical?
>
> I wanted to associate it with b->irq_lock, that was my thinking.
> b->rb_lock also sadly has to be irqsafe.

That makes sense yes.

> __intel_breadcrumbs_next() also serves remove_wait, did you mean to
> remove the assert there as well?

Yes, I was thinking only one would do it.

> We can safely ignore this patch, it should be catered by lockdep fairly
> well, I was just being paranoid and going through the possible causes
> and documenting my progress.

This also makes sense. I think it's the best option.

Regards,

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

Re: [PATCH 3/6] drm/i915/breadcrumbs: Update bottom-half before marking as complete

[Chris Wilson]

On Wed, Mar 15, 2017 at 06:58:27PM +0000, Tvrtko Ursulin wrote:
> 
> On 15/03/2017 14:01, Chris Wilson wrote:
> >When adding a new request to the breadcrumb rbtree, we mark all those
> >requests inside the rbtree that are already completed as complete. This
> >wakes those waiters up and allows them to skip the spinlock before
> >returning to userspace. If one of those is the current bottom-half, it
> >may then overwrite intel_wait as the interrupt handler dereferences it.
> 
> Last sentence sounds suspicious. The interrupts are disabled when
> this runs and locking is in place. And since the fix is to move the
> "completed" block after the "first", I wonder what can get
> overwritten by who?
> 
> Oh.. __intel_breadcrumbs_finish. But how does re-ordering help?
> Shouldn't the fix be to skip the bottom-half assignment if the
> "complete" loop has processed the waiter getting added?

Thread A runs intel_engine_add_wait, marks an earlier waiter complete
and wakes up thread B. Thread C is processing the interrupt and grabs
the irq_wait. However, thread B sees that is is complete and exits
i915_aait_request() invalidating the irq_wait as it is being used by
thread C.

Moving the irq_wait update before we wakeup thread B, ensures that
thread C has a valid irq_wait.
-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: [PATCH 3/6] drm/i915/breadcrumbs: Update bottom-half before marking as complete

[Tvrtko Ursulin]

On 15/03/2017 19:10, Chris Wilson wrote:
> On Wed, Mar 15, 2017 at 06:58:27PM +0000, Tvrtko Ursulin wrote:
>>
>> On 15/03/2017 14:01, Chris Wilson wrote:
>>> When adding a new request to the breadcrumb rbtree, we mark all those
>>> requests inside the rbtree that are already completed as complete. This
>>> wakes those waiters up and allows them to skip the spinlock before
>>> returning to userspace. If one of those is the current bottom-half, it
>>> may then overwrite intel_wait as the interrupt handler dereferences it.
>>
>> Last sentence sounds suspicious. The interrupts are disabled when
>> this runs and locking is in place. And since the fix is to move the
>> "completed" block after the "first", I wonder what can get
>> overwritten by who?
>>
>> Oh.. __intel_breadcrumbs_finish. But how does re-ordering help?
>> Shouldn't the fix be to skip the bottom-half assignment if the
>> "complete" loop has processed the waiter getting added?
>
> Thread A runs intel_engine_add_wait, marks an earlier waiter complete
> and wakes up thread B. Thread C is processing the interrupt and grabs
> the irq_wait. However, thread B sees that is is complete and exits
> i915_aait_request() invalidating the irq_wait as it is being used by
> thread C.
>
> Moving the irq_wait update before we wakeup thread B, ensures that
> thread C has a valid irq_wait.

As discussed on the IRC, it is not that the exiting waiter explicitly 
destroys the intel_wait state, but it happens implicitly because the 
struct is on the waiter's stack. I was misled by the short-circuit in 
intel_engine_remove_wait to think commit message was incorrect so I 
suggest describing the stack situation explicitly in the commit.

With a line added to do so:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 4/6] drm/i915/breadcrumbs: Disable interrupt bottom-half first on idling

[Tvrtko Ursulin]

On 15/03/2017 14:01, Chris Wilson wrote:
> Before walking the rbtree of waiters (marking them as complete and waking
> them), decouple the interrupt handler. This prevents a race between the
> missed waiter waking up and removing its intel_wait (which skips
> checking the lock) and the interrupt handler dereferencing the
> intel_wait. (Though we do not expect to encounter waiters during idle!)
>
> Fixes: e1c0c91bdaec ("drm/i915: Wake up all waiters before idling")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 31e7c25013a4..ebcb595001fc 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -179,7 +179,7 @@ void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  {
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> -	struct intel_wait *wait, *n;
> +	struct intel_wait *wait, *n, *first;
>
>  	if (!b->irq_armed)
>  		return;
> @@ -190,18 +190,19 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
>  	 */
>
>  	spin_lock_irq(&b->rb_lock);
> +
> +	spin_lock(&b->irq_lock);
> +	first = fetch_and_zero(&b->irq_wait);
> +	__intel_engine_disarm_breadcrumbs(engine);
> +	spin_unlock(&b->irq_lock);
> +
>  	rbtree_postorder_for_each_entry_safe(wait, n, &b->waiters, node) {
>  		RB_CLEAR_NODE(&wait->node);
> -		if (wake_up_process(wait->tsk) && wait == b->irq_wait)
> +		if (wake_up_process(wait->tsk) && wait == first)
>  			missed_breadcrumb(engine);
>  	}
>  	b->waiters = RB_ROOT;
>
> -	spin_lock(&b->irq_lock);
> -	b->irq_wait = NULL;
> -	__intel_engine_disarm_breadcrumbs(engine);
> -	spin_unlock(&b->irq_lock);
> -
>  	spin_unlock_irq(&b->rb_lock);
>  }
>
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 5/6] drm/i915/breadcrumbs: Assert that we do not shortcut the current bottom-half

[Tvrtko Ursulin]

On 15/03/2017 14:01, Chris Wilson wrote:
> We need to ensure that we always serialize updates to the bottom-half
> using the breadcrumbs.irq_lock so that we don't race with a concurrent
> interrupt handler. This is most important just prior to leaving the
> waiter (when the intel_wait will be overwritten), so make sure we are
> not the current bottom-half when skipping the irq locks.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index ebcb595001fc..d10148567dbc 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -287,6 +287,7 @@ static inline void __intel_breadcrumbs_finish(struct intel_breadcrumbs *b,
>  					      struct intel_wait *wait)
>  {
>  	lockdep_assert_held(&b->rb_lock);
> +	GEM_BUG_ON(b->irq_wait == wait);
>
>  	/* This request is completed, so remove it from the tree, mark it as
>  	 * complete, and *then* wake up the associated task.
> @@ -517,8 +518,10 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
>  	 * the tree by the bottom-half to avoid contention on the spinlock
>  	 * by the herd.
>  	 */
> -	if (RB_EMPTY_NODE(&wait->node))
> +	if (RB_EMPTY_NODE(&wait->node)) {
> +		GEM_BUG_ON(READ_ONCE(b->irq_wait) == wait);
>  		return;
> +	}
>
>  	spin_lock_irq(&b->rb_lock);
>  	__intel_engine_remove_wait(engine, wait);
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 6/6] drm/i915: Only attempt to signal the request once from the interrupt handler

[Tvrtko Ursulin]

On 15/03/2017 14:01, Chris Wilson wrote:
> Check that signaled bit on the request->fence before acquiring a

             "is not set" ^

Or "Check that the request has not been signalled before.." ?

> reference to the request for signaling later in the interrupt handler.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 31f0d7c8992f..e646c4eba65d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1056,7 +1056,9 @@ static void notify_ring(struct intel_engine_cs *engine)
>  		 * and many waiters.
>  		 */
>  		if (i915_seqno_passed(intel_engine_get_seqno(engine),
> -				      wait->seqno))
> +				      wait->seqno) &&
> +		    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> +			      &wait->request->fence.flags))
>  			rq = i915_gem_request_get(wait->request);
>
>  		wake_up_process(wait->tsk);
>

Is this just to optimize things slightly? Does it happen sufficiently 
often for it to be worth it?

Regards,

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

Re: [PATCH 6/6] drm/i915: Only attempt to signal the request once from the interrupt handler

[Chris Wilson]

On Wed, Mar 15, 2017 at 07:47:20PM +0000, Tvrtko Ursulin wrote:
> 
> On 15/03/2017 14:01, Chris Wilson wrote:
> >Check that signaled bit on the request->fence before acquiring a
> 
>             "is not set" ^
> 
> Or "Check that the request has not been signalled before.." ?
> 
> >reference to the request for signaling later in the interrupt handler.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> > drivers/gpu/drm/i915/i915_irq.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >index 31f0d7c8992f..e646c4eba65d 100644
> >--- a/drivers/gpu/drm/i915/i915_irq.c
> >+++ b/drivers/gpu/drm/i915/i915_irq.c
> >@@ -1056,7 +1056,9 @@ static void notify_ring(struct intel_engine_cs *engine)
> > 		 * and many waiters.
> > 		 */
> > 		if (i915_seqno_passed(intel_engine_get_seqno(engine),
> >-				      wait->seqno))
> >+				      wait->seqno) &&
> >+		    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> >+			      &wait->request->fence.flags))
> > 			rq = i915_gem_request_get(wait->request);
> >
> > 		wake_up_process(wait->tsk);
> >
> 
> Is this just to optimize things slightly? Does it happen
> sufficiently often for it to be worth it?

Even once would be enough. An unlocked read of fence.flags is much
cheaper than the locked get/put we go through otherwise. Even if the
check is a dud, the load of the cacheline prior to us making it
exclusive is "free".

I'm mostly thinking about the situations where we have pile of bare
breadcrumbs where we may very well get interrupts faster than we can get
rid of the intel_wait. Or perhaps a mythical slow device.
-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: [PATCH 6/6] drm/i915: Only attempt to signal the request once from the interrupt handler

[Tvrtko Ursulin]

On 15/03/2017 20:05, Chris Wilson wrote:
> On Wed, Mar 15, 2017 at 07:47:20PM +0000, Tvrtko Ursulin wrote:
>>
>> On 15/03/2017 14:01, Chris Wilson wrote:
>>> Check that signaled bit on the request->fence before acquiring a
>>
>>             "is not set" ^
>>
>> Or "Check that the request has not been signalled before.." ?
>>
>>> reference to the request for signaling later in the interrupt handler.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_irq.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>> index 31f0d7c8992f..e646c4eba65d 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -1056,7 +1056,9 @@ static void notify_ring(struct intel_engine_cs *engine)
>>> 		 * and many waiters.
>>> 		 */
>>> 		if (i915_seqno_passed(intel_engine_get_seqno(engine),
>>> -				      wait->seqno))
>>> +				      wait->seqno) &&
>>> +		    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
>>> +			      &wait->request->fence.flags))
>>> 			rq = i915_gem_request_get(wait->request);
>>>
>>> 		wake_up_process(wait->tsk);
>>>
>>
>> Is this just to optimize things slightly? Does it happen
>> sufficiently often for it to be worth it?
>
> Even once would be enough. An unlocked read of fence.flags is much
> cheaper than the locked get/put we go through otherwise. Even if the
> check is a dud, the load of the cacheline prior to us making it
> exclusive is "free".
>
> I'm mostly thinking about the situations where we have pile of bare
> breadcrumbs where we may very well get interrupts faster than we can get
> rid of the intel_wait. Or perhaps a mythical slow device.

Makes sense now that you've said it. Maybe c&p into the commit as well. ;)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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