[PATCH 1/4] drm/i915: Check signaled state after enabling signaling

[PATCH 1/4] drm/i915: Check signaled state after enabling signaling

[Chris Wilson]

Settting up the irq to signal the request completion takes a finite
amount of time, during which it is possible that the request already
completed. Check afterwards, just in case, so that we can respond
immediately.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 0d1e0d8873ef..46d869e26b4d 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -62,7 +62,7 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
 		return false;
 
 	intel_engine_enable_signaling(to_request(fence), true);
-	return true;
+	return !i915_fence_signaled(fence);
 }
 
 static signed long i915_fence_wait(struct dma_fence *fence,
-- 
2.11.0

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

[PATCH 2/4] drm/i915: Report back whether the irq was armed when adding the waiter

[Chris Wilson]

The important condition that we need to check after enabling the
interrupt for signaling is whether the request completed in the process
(and so we missed that interrupt). A large cost in enabling the
signaling (rather than waiters) is in waking up the auxiliary signaling
thread, but we only need to do so to catch that missed interrupt. If we
know we didn't miss any interrupts (because we didn't arm the interrupt)
then we can skip waking the auxiliary thread.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 183afcb036aa..dbcad9f6b2d5 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -329,7 +329,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 	struct rb_node **p, *parent, *completed;
-	bool first;
+	bool first, irq_armed;
 	u32 seqno;
 
 	/* Insert the request into the retirement ordered list
@@ -344,6 +344,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	 * removing stale elements in the tree, we may be able to reduce the
 	 * ping-pong between the old bottom-half and ourselves as first-waiter.
 	 */
+	irq_armed = false;
 	first = true;
 	parent = NULL;
 	completed = NULL;
@@ -390,6 +391,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 
 	if (first) {
 		spin_lock(&b->irq_lock);
+		irq_armed = !b->irq_armed;
 		b->irq_wait = wait;
 		/* After assigning ourselves as the new bottom-half, we must
 		 * perform a cursory check to prevent a missed interrupt.
@@ -426,20 +428,24 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	GEM_BUG_ON(!b->irq_armed);
 	GEM_BUG_ON(rb_first(&b->waiters) != &b->irq_wait->node);
 
-	return first;
+	return irq_armed;
 }
 
 bool intel_engine_add_wait(struct intel_engine_cs *engine,
 			   struct intel_wait *wait)
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
-	bool first;
+	bool irq_armed;
 
 	spin_lock_irq(&b->rb_lock);
-	first = __intel_engine_add_wait(engine, wait);
+	irq_armed = __intel_engine_add_wait(engine, wait);
 	spin_unlock_irq(&b->rb_lock);
+	if (irq_armed)
+		return irq_armed;
 
-	return first;
+	/* Make the caller recheck if its request has already started. */
+	return i915_seqno_passed(intel_engine_get_seqno(engine),
+				 wait->seqno - 1);
 }
 
 static inline bool chain_wakeup(struct rb_node *rb, int priority)
-- 
2.11.0

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

[PATCH 3/4] drm/i915: Skip adding the request to the signal tree is complete

[Chris Wilson]

Enabling the interrupt for the signaler takes a finite amount of time (a
few microseconds) during which it is possible for the request to
complete. Check afterwards and skip adding the request to the signal
rbtree if it complete.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 44 ++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index dbcad9f6b2d5..a855771ec1a9 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -714,27 +714,33 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request,
 	 */
 	wakeup &= __intel_engine_add_wait(engine, &request->signaling.wait);
 
-	/* Now insert ourselves into the retirement ordered list of signals
-	 * on this engine. We track the oldest seqno as that will be the
-	 * first signal to complete.
-	 */
-	parent = NULL;
-	first = true;
-	p = &b->signals.rb_node;
-	while (*p) {
-		parent = *p;
-		if (i915_seqno_passed(seqno,
-				      to_signaler(parent)->signaling.wait.seqno)) {
-			p = &parent->rb_right;
-			first = false;
-		} else {
-			p = &parent->rb_left;
+	if (!__i915_gem_request_completed(request, seqno)) {
+		/* Now insert ourselves into the retirement ordered list of
+		 * signals on this engine. We track the oldest seqno as that
+		 * will be the first signal to complete.
+		 */
+		parent = NULL;
+		first = true;
+		p = &b->signals.rb_node;
+		while (*p) {
+			parent = *p;
+			if (i915_seqno_passed(seqno,
+						to_signaler(parent)->signaling.wait.seqno)) {
+				p = &parent->rb_right;
+				first = false;
+			} else {
+				p = &parent->rb_left;
+			}
 		}
+		rb_link_node(&request->signaling.node, parent, p);
+		rb_insert_color(&request->signaling.node, &b->signals);
+		if (first)
+			rcu_assign_pointer(b->first_signal, request);
+	} else {
+		__intel_engine_remove_wait(engine, &request->signaling.wait);
+		i915_gem_request_put(request);
+		wakeup = false;
 	}
-	rb_link_node(&request->signaling.node, parent, p);
-	rb_insert_color(&request->signaling.node, &b->signals);
-	if (first)
-		rcu_assign_pointer(b->first_signal, request);
 
 	spin_unlock(&b->rb_lock);
 
-- 
2.11.0

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

[PATCH 4/4] drm/i915: Remove the spin-request during execbuf await_request

[Chris Wilson]

Originally we would enable and disable the breadcrumb interrupt
immediately on demand. This was slow enough to have a large impact
(>30%) on tasks that hopped between engines. However, by using a shadow
to keep the irq alive for an extra interrupt (see commit 67b807a89230
("drm/i915: Delay disabling the user interrupt for breadcrumbs")) and
by recently reducing the cost in adding ourselves to the signal tree, we
no longer need to spin-request during await_request to avoid delays in
throughput tests. Without the earlier patches to stop the wakeup when
signaling if the irq was already active, we saw no improvement in
execbuf overhead (and corresponding contention in other clients) despite
the removal of the spinner in a simple test like glxgears. This means
that will be scenarios where now we spend longer enabling the interrupt
than we would have spent spinning, but these are not likely to have as
noticeable an impact as the high frequency test cases (where there
should not be any regression).

Ulterior motive: generalising the engine->sync_to to handle different
types of semaphores and non-semaphores.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 46d869e26b4d..8c59c79cbd8b 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -683,7 +683,6 @@ static int
 i915_gem_request_await_request(struct drm_i915_gem_request *to,
 			       struct drm_i915_gem_request *from)
 {
-	u32 seqno;
 	int ret;
 
 	GEM_BUG_ON(to == from);
@@ -707,18 +706,14 @@ i915_gem_request_await_request(struct drm_i915_gem_request *to,
 		return ret < 0 ? ret : 0;
 	}
 
-	seqno = i915_gem_request_global_seqno(from);
-	if (!seqno)
-		goto await_dma_fence;
+	if (to->engine->semaphore.sync_to) {
+		u32 seqno;
 
-	if (!to->engine->semaphore.sync_to) {
-		if (!__i915_gem_request_started(from, seqno))
-			goto await_dma_fence;
+		GEM_BUG_ON(!from->engine->semaphore.signal);
 
-		if (!__i915_spin_request(from, seqno, TASK_INTERRUPTIBLE, 2))
+		seqno = i915_gem_request_global_seqno(from);
+		if (!seqno)
 			goto await_dma_fence;
-	} else {
-		GEM_BUG_ON(!from->engine->semaphore.signal);
 
 		if (seqno <= to->timeline->global_sync[from->engine->id])
 			return 0;
@@ -729,10 +724,9 @@ i915_gem_request_await_request(struct drm_i915_gem_request *to,
 			return ret;
 
 		to->timeline->global_sync[from->engine->id] = seqno;
+		return 0;
 	}
 
-	return 0;
-
 await_dma_fence:
 	ret = i915_sw_fence_await_dma_fence(&to->submit,
 					    &from->fence, 0,
-- 
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/4] drm/i915: Check signaled state after enabling signaling

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Check signaled state after enabling signaling
URL   : https://patchwork.freedesktop.org/series/25283/
State : success

== Summary ==

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

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#101022
Test kms_busy:
        Subgroup basic-flip-default-a:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144 +2

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

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:445s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:438s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:581s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:507s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:495s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:488s
fi-glk-2a        total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:594s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:437s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:410s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:414s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:494s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:466s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:472s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:568s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:457s
fi-skl-6700hq    total:278  pass:229  dwarn:2   dfail:0   fail:25  skip:22  time:406s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:466s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:503s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:438s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:535s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:406s

d90972f79041a05fe10d62ee29e2d1c345dda772 drm-tip: 2017y-06m-05d-10h-01m-30s UTC integration manifest
68f4b8f drm/i915: Remove the spin-request during execbuf await_request
2660f73 drm/i915: Skip adding the request to the signal tree is complete
0155e73 drm/i915: Report back whether the irq was armed when adding the waiter
3a73b7a drm/i915: Check signaled state after enabling signaling

== Logs ==

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

Re: [PATCH 1/4] drm/i915: Check signaled state after enabling signaling

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Settting up the irq to signal the request completion takes a finite
      ^-t

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> amount of time, during which it is possible that the request already
> completed. Check afterwards, just in case, so that we can respond
> immediately.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_request.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 0d1e0d8873ef..46d869e26b4d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -62,7 +62,7 @@ static bool i915_fence_enable_signaling(struct dma_fence *fence)
>  		return false;
>  
>  	intel_engine_enable_signaling(to_request(fence), true);
> -	return true;
> +	return !i915_fence_signaled(fence);
>  }
>  
>  static signed long i915_fence_wait(struct dma_fence *fence,
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Remove the spin-request during execbuf await_request

[Tvrtko Ursulin]

On 05/06/2017 11:26, Chris Wilson wrote:
> Originally we would enable and disable the breadcrumb interrupt
> immediately on demand. This was slow enough to have a large impact
> (>30%) on tasks that hopped between engines. However, by using a shadow
> to keep the irq alive for an extra interrupt (see commit 67b807a89230
> ("drm/i915: Delay disabling the user interrupt for breadcrumbs")) and
> by recently reducing the cost in adding ourselves to the signal tree, we
> no longer need to spin-request during await_request to avoid delays in
> throughput tests. Without the earlier patches to stop the wakeup when
> signaling if the irq was already active, we saw no improvement in
> execbuf overhead (and corresponding contention in other clients) despite
> the removal of the spinner in a simple test like glxgears. This means
> that will be scenarios where now we spend longer enabling the interrupt

"There will be" I guess?

> than we would have spent spinning, but these are not likely to have as
> noticeable an impact as the high frequency test cases (where there
> should not be any regression).
> 
> Ulterior motive: generalising the engine->sync_to to handle different
> types of semaphores and non-semaphores.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_request.c | 18 ++++++------------
>   1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 46d869e26b4d..8c59c79cbd8b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -683,7 +683,6 @@ static int
>   i915_gem_request_await_request(struct drm_i915_gem_request *to,
>   			       struct drm_i915_gem_request *from)
>   {
> -	u32 seqno;
>   	int ret;
>   
>   	GEM_BUG_ON(to == from);
> @@ -707,18 +706,14 @@ i915_gem_request_await_request(struct drm_i915_gem_request *to,
>   		return ret < 0 ? ret : 0;
>   	}
>   
> -	seqno = i915_gem_request_global_seqno(from);
> -	if (!seqno)
> -		goto await_dma_fence;
> +	if (to->engine->semaphore.sync_to) {
> +		u32 seqno;
>   
> -	if (!to->engine->semaphore.sync_to) {
> -		if (!__i915_gem_request_started(from, seqno))
> -			goto await_dma_fence;
> +		GEM_BUG_ON(!from->engine->semaphore.signal);
>   
> -		if (!__i915_spin_request(from, seqno, TASK_INTERRUPTIBLE, 2))
> +		seqno = i915_gem_request_global_seqno(from);
> +		if (!seqno)
>   			goto await_dma_fence;
> -	} else {
> -		GEM_BUG_ON(!from->engine->semaphore.signal);
>   
>   		if (seqno <= to->timeline->global_sync[from->engine->id])
>   			return 0;
> @@ -729,10 +724,9 @@ i915_gem_request_await_request(struct drm_i915_gem_request *to,
>   			return ret;
>   
>   		to->timeline->global_sync[from->engine->id] = seqno;
> +		return 0;
>   	}
>   
> -	return 0;
> -
>   await_dma_fence:
>   	ret = i915_sw_fence_await_dma_fence(&to->submit,
>   					    &from->fence, 0,
> 

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 1/4] drm/i915: Check signaled state after enabling signaling

[Joonas Lahtinen]

On ma, 2017-06-05 at 11:26 +0100, Chris Wilson wrote:
> Settting up the irq to signal the request completion takes a finite
> amount of time, during which it is possible that the request already
> completed. Check afterwards, just in case, so that we can respond
> immediately.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Skip adding the request to the signal tree is complete

[Joonas Lahtinen]

On ma, 2017-06-05 at 11:26 +0100, Chris Wilson wrote:
> Enabling the interrupt for the signaler takes a finite amount of time (a
> few microseconds) during which it is possible for the request to
> complete. Check afterwards and skip adding the request to the signal
> rbtree if it complete.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -714,27 +714,33 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request,
> +	if (!__i915_gem_request_completed(request, seqno)) {
> +		/* Now insert ourselves into the retirement ordered list of
> +		 * signals on this engine. We track the oldest seqno as that
> +		 * will be the first signal to complete.
> +		 */
> +		parent = NULL;
> +		first = true;
> +		p = &b->signals.rb_node;

These variables could be more tightly scoped now.

> +		while (*p) {
> +			parent = *p;
> +			if (i915_seqno_passed(seqno,
> +						to_signaler(parent)->signaling.wait.seqno)) {

Alignment incorrect.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Remove the spin-request during execbuf await_request

[Joonas Lahtinen]

On ma, 2017-06-05 at 11:26 +0100, Chris Wilson wrote:
> Originally we would enable and disable the breadcrumb interrupt
> immediately on demand. This was slow enough to have a large impact
> (>30%) on tasks that hopped between engines. However, by using a shadow
> to keep the irq alive for an extra interrupt (see commit 67b807a89230
> ("drm/i915: Delay disabling the user interrupt for breadcrumbs")) and
> by recently reducing the cost in adding ourselves to the signal tree, we
> no longer need to spin-request during await_request to avoid delays in
> throughput tests. Without the earlier patches to stop the wakeup when
> signaling if the irq was already active, we saw no improvement in
> execbuf overhead (and corresponding contention in other clients) despite
> the removal of the spinner in a simple test like glxgears. This means
> that will be scenarios where now we spend longer enabling the interrupt

      ^ there ?          "now where we" ? 

> than we would have spent spinning, but these are not likely to have as
> noticeable an impact as the high frequency test cases (where there
> should not be any regression).
> 
> Ulterior motive: generalising the engine->sync_to to handle different
> types of semaphores and non-semaphores.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>

Does what is described, so code itself is:

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Some Testcase:'s would be cool, without those it's bit handwavy. Maybe
an Ack from Tvrtko.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Remove the spin-request during execbuf await_request

[Chris Wilson]

Quoting Joonas Lahtinen (2017-06-08 11:02:40)
> On ma, 2017-06-05 at 11:26 +0100, Chris Wilson wrote:
> > Originally we would enable and disable the breadcrumb interrupt
> > immediately on demand. This was slow enough to have a large impact
> > (>30%) on tasks that hopped between engines. However, by using a shadow
> > to keep the irq alive for an extra interrupt (see commit 67b807a89230
> > ("drm/i915: Delay disabling the user interrupt for breadcrumbs")) and
> > by recently reducing the cost in adding ourselves to the signal tree, we
> > no longer need to spin-request during await_request to avoid delays in
> > throughput tests. Without the earlier patches to stop the wakeup when
> > signaling if the irq was already active, we saw no improvement in
> > execbuf overhead (and corresponding contention in other clients) despite
> > the removal of the spinner in a simple test like glxgears. This means
> > that will be scenarios where now we spend longer enabling the interrupt
> 
>       ^ there ?          "now where we" ? 
> 
> > than we would have spent spinning, but these are not likely to have as
> > noticeable an impact as the high frequency test cases (where there
> > should not be any regression).
> > 
> > Ulterior motive: generalising the engine->sync_to to handle different
> > types of semaphores and non-semaphores.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Oscar Mateo <oscar.mateo@intel.com>
> 
> Does what is described, so code itself is:
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> Some Testcase:'s would be cool, without those it's bit handwavy. Maybe
> an Ack from Tvrtko.

Testcase for what though? It doesn't fix or improve anything, it just
moves the overhead of the breadcrumb from one column to another.

And we are sorely lacking performance tests in igt. Correctness wise
there are many that cover this await_request.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Report back whether the irq was armed when adding the waiter

[Joonas Lahtinen]

On ma, 2017-06-05 at 11:26 +0100, Chris Wilson wrote:
> The important condition that we need to check after enabling the
> interrupt for signaling is whether the request completed in the process
> (and so we missed that interrupt). A large cost in enabling the
> signaling (rather than waiters) is in waking up the auxiliary signaling
> thread, but we only need to do so to catch that missed interrupt. If we
> know we didn't miss any interrupts (because we didn't arm the interrupt)
> then we can skip waking the auxiliary thread.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

<SNIP>

> @@ -390,6 +391,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  
>  	if (first) {
>  		spin_lock(&b->irq_lock);
> +		irq_armed = !b->irq_armed;

This line here is quite confusing, "armed_irq" for local variable would
be *much* clearer. Could be further amended if I'm the only with such
mindset.

With a less confusing local variable name;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas

>  		b->irq_wait = wait;
>  		/* After assigning ourselves as the new bottom-half, we must
>  		 * perform a cursory check to prevent a missed interrupt.
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Report back whether the irq was armed when adding the waiter

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> The important condition that we need to check after enabling the
> interrupt for signaling is whether the request completed in the process
> (and so we missed that interrupt). A large cost in enabling the
> signaling (rather than waiters) is in waking up the auxiliary signaling
> thread, but we only need to do so to catch that missed interrupt. If we
> know we didn't miss any interrupts (because we didn't arm the interrupt)
> then we can skip waking the auxiliary thread.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 183afcb036aa..dbcad9f6b2d5 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -329,7 +329,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  {
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>  	struct rb_node **p, *parent, *completed;
> -	bool first;
> +	bool first, irq_armed;
>  	u32 seqno;
>  
>  	/* Insert the request into the retirement ordered list
> @@ -344,6 +344,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  	 * removing stale elements in the tree, we may be able to reduce the
>  	 * ping-pong between the old bottom-half and ourselves as first-waiter.
>  	 */
> +	irq_armed = false;
>  	first = true;
>  	parent = NULL;
>  	completed = NULL;
> @@ -390,6 +391,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  
>  	if (first) {
>  		spin_lock(&b->irq_lock);
> +		irq_armed = !b->irq_armed;

This could use a comment.

>  		b->irq_wait = wait;
>  		/* After assigning ourselves as the new bottom-half, we must
>  		 * perform a cursory check to prevent a missed interrupt.
> @@ -426,20 +428,24 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  	GEM_BUG_ON(!b->irq_armed);
>  	GEM_BUG_ON(rb_first(&b->waiters) != &b->irq_wait->node);
>  
> -	return first;
> +	return irq_armed;
>  }
>  
>  bool intel_engine_add_wait(struct intel_engine_cs *engine,
>  			   struct intel_wait *wait)
>  {
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> -	bool first;
> +	bool irq_armed;
>  
>  	spin_lock_irq(&b->rb_lock);
> -	first = __intel_engine_add_wait(engine, wait);
> +	irq_armed = __intel_engine_add_wait(engine, wait);
>  	spin_unlock_irq(&b->rb_lock);
> +	if (irq_armed)
> +		return irq_armed;
>  
> -	return first;
> +	/* Make the caller recheck if its request has already
> started. */

This could be lifted to the function documentation to describe
what the return value actually means. But I am not insisting.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> +	return i915_seqno_passed(intel_engine_get_seqno(engine),
> +				 wait->seqno - 1);
>  }
>  
>  static inline bool chain_wakeup(struct rb_node *rb, int priority)
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx