[PATCH 1/2] drm/i915: Only wake the waiter from the interrupt if passed

[PATCH 1/2] drm/i915: Only wake the waiter from the interrupt if passed

[Chris Wilson]

As we now check if the seqno is complete in order to signal the fence,
we can also decide not to wake up the first_waiter until it is ready
(since it is waiting on the same seqno). The only caveat is that if we
need the engine->irq_seqno_barrier to enforce some coherency between an
interrupt and the seqno read, we have to always wake the waiter in order
to perform that heavyweight barrier.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4d0e8f76ed1a..bb69c5b0efc4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1038,6 +1038,8 @@ static void notify_ring(struct intel_engine_cs *engine)
 	spin_lock(&engine->breadcrumbs.irq_lock);
 	wait = engine->breadcrumbs.irq_wait;
 	if (wait) {
+		bool wakeup = engine->irq_seqno_barrier;
+
 		/* We use a callback from the dma-fence to submit
 		 * requests after waiting on our own requests. To
 		 * ensure minimum delay in queuing the next request to
@@ -1050,12 +1052,15 @@ static void notify_ring(struct intel_engine_cs *engine)
 		 * and many waiters.
 		 */
 		if (i915_seqno_passed(intel_engine_get_seqno(engine),
-				      wait->seqno) &&
-		    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
-			      &wait->request->fence.flags))
-			rq = i915_gem_request_get(wait->request);
+				      wait->seqno)) {
+			wakeup = true;
+			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+				      &wait->request->fence.flags))
+				rq = i915_gem_request_get(wait->request);
+		}
 
-		wake_up_process(wait->tsk);
+		if (wakeup)
+			wake_up_process(wait->tsk);
 	} else {
 		__intel_engine_disarm_breadcrumbs(engine);
 	}
-- 
2.14.1

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

[PATCH 2/2] drm/i915: Check waiter->seqno carefully in case of preemption

[Chris Wilson]

If preemption occurs at precisely the right moment, we may decide that
the wait is complete even though the wait's request is no longer
executing (having been preempted). We handle this situation by double
checking that request following deciding whether the wait is complete.

Reported-by: Michał Winiarski <michal.winiarski@intel.com
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bb69c5b0efc4..7a53d94b7e61 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1053,10 +1053,13 @@ static void notify_ring(struct intel_engine_cs *engine)
 		 */
 		if (i915_seqno_passed(intel_engine_get_seqno(engine),
 				      wait->seqno)) {
+			struct drm_i915_gem_request *waiter = wait->request;
+
 			wakeup = true;
 			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
-				      &wait->request->fence.flags))
-				rq = i915_gem_request_get(wait->request);
+				      &waiter->fence.flags) &&
+			    intel_wait_check_request(wait, waiter))
+				rq = i915_gem_request_get(waiter);
 		}
 
 		if (wakeup)
-- 
2.14.1

_______________________________________________
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/2] drm/i915: Only wake the waiter from the interrupt if passed

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Only wake the waiter from the interrupt if passed
URL   : https://patchwork.freedesktop.org/series/30540/
State : success

== Summary ==

Series 30540v1 series starting with [1/2] drm/i915: Only wake the waiter from the interrupt if passed
https://patchwork.freedesktop.org/api/1.0/series/30540/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-cfl-s) fdo#102294

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:445s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:481s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:419s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:527s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:280s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:504s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:494s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:500s
fi-cfl-s         total:289  pass:222  dwarn:35  dfail:0   fail:0   skip:32  time:548s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:412s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:601s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:425s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:406s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:429s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:494s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:462s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:480s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:582s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:589s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:540s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:451s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:757s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:494s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:471s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:567s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:427s

dee3ba3a8c8bc9b11aca9cddc4e47a17b15db7eb drm-tip: 2017y-09m-18d-15h-59m-43s UTC integration manifest
e88a91f72fcc drm/i915: Check waiter->seqno carefully in case of preemption
88e33463d026 drm/i915: Only wake the waiter from the interrupt if passed

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5736/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for series starting with [1/2] drm/i915: Only wake the waiter from the interrupt if passed

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Only wake the waiter from the interrupt if passed
URL   : https://patchwork.freedesktop.org/series/30540/
State : warning

== Summary ==

Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252
Test gem_flink_race:
        Subgroup flink_close:
                pass       -> FAIL       (shard-hsw) fdo#102655
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-A-planes:
                pass       -> SKIP       (shard-hsw)
Test kms_cursor_legacy:
        Subgroup short-flip-after-cursor-atomic-transitions:
                pass       -> SKIP       (shard-hsw)
Test kms_chv_cursor_fail:
        Subgroup pipe-A-256x256-right-edge:
                pass       -> SKIP       (shard-hsw)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> SKIP       (shard-hsw)
Test drv_module_reload:
        Subgroup basic-reload-inject:
                dmesg-warn -> PASS       (shard-hsw) fdo#102707
Test kms_flip:
        Subgroup plain-flip-fb-recreate:
                fail       -> PASS       (shard-hsw) fdo#102504

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102655 https://bugs.freedesktop.org/show_bug.cgi?id=102655
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#102504 https://bugs.freedesktop.org/show_bug.cgi?id=102504

shard-hsw        total:2313 pass:1241 dwarn:0   dfail:0   fail:13  skip:1059 time:9555s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5736/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Only wake the waiter from the interrupt if passed

[Tvrtko Ursulin]

On 18/09/2017 17:27, Chris Wilson wrote:
> As we now check if the seqno is complete in order to signal the fence,
> we can also decide not to wake up the first_waiter until it is ready
> (since it is waiting on the same seqno). The only caveat is that if we
> need the engine->irq_seqno_barrier to enforce some coherency between an
> interrupt and the seqno read, we have to always wake the waiter in order
> to perform that heavyweight barrier.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_irq.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4d0e8f76ed1a..bb69c5b0efc4 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1038,6 +1038,8 @@ static void notify_ring(struct intel_engine_cs *engine)
>   	spin_lock(&engine->breadcrumbs.irq_lock);
>   	wait = engine->breadcrumbs.irq_wait;
>   	if (wait) {
> +		bool wakeup = engine->irq_seqno_barrier;
> +
>   		/* We use a callback from the dma-fence to submit
>   		 * requests after waiting on our own requests. To
>   		 * ensure minimum delay in queuing the next request to
> @@ -1050,12 +1052,15 @@ static void notify_ring(struct intel_engine_cs *engine)
>   		 * and many waiters.
>   		 */
>   		if (i915_seqno_passed(intel_engine_get_seqno(engine),
> -				      wait->seqno) &&
> -		    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> -			      &wait->request->fence.flags))
> -			rq = i915_gem_request_get(wait->request);
> +				      wait->seqno)) {
> +			wakeup = true;
> +			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> +				      &wait->request->fence.flags))
> +				rq = i915_gem_request_get(wait->request);
> +		}
>   
> -		wake_up_process(wait->tsk);
> +		if (wakeup)
> +			wake_up_process(wait->tsk);
>   	} else {
>   		__intel_engine_disarm_breadcrumbs(engine);
>   	}
> 

Looks straightforward enough.

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/2] drm/i915: Check waiter->seqno carefully in case of preemption

[Tvrtko Ursulin]

On 18/09/2017 17:27, Chris Wilson wrote:
> If preemption occurs at precisely the right moment, we may decide that
> the wait is complete even though the wait's request is no longer
> executing (having been preempted). We handle this situation by double
> checking that request following deciding whether the wait is complete.
> 
> Reported-by: Michał Winiarski <michal.winiarski@intel.com
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bb69c5b0efc4..7a53d94b7e61 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1053,10 +1053,13 @@ static void notify_ring(struct intel_engine_cs *engine)
>   		 */
>   		if (i915_seqno_passed(intel_engine_get_seqno(engine),
>   				      wait->seqno)) {
> +			struct drm_i915_gem_request *waiter = wait->request;
> +
>   			wakeup = true;
>   			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> -				      &wait->request->fence.flags))
> -				rq = i915_gem_request_get(wait->request);
> +				      &waiter->fence.flags) &&
> +			    intel_wait_check_request(wait, waiter))
> +				rq = i915_gem_request_get(waiter);
>   		}
>   
>   		if (wakeup)
> 

Hm but as the user interrupt is nor serialized to exelists, what 
prevents the preemption to happen after the intel_wait_check_request and 
before dma_fence_signal?

Regards,

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

Re: [PATCH 2/2] drm/i915: Check waiter->seqno carefully in case of preemption

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-09-19 10:28:42)
> 
> On 18/09/2017 17:27, Chris Wilson wrote:
> > If preemption occurs at precisely the right moment, we may decide that
> > the wait is complete even though the wait's request is no longer
> > executing (having been preempted). We handle this situation by double
> > checking that request following deciding whether the wait is complete.
> > 
> > Reported-by: Michał Winiarski <michal.winiarski@intel.com
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index bb69c5b0efc4..7a53d94b7e61 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1053,10 +1053,13 @@ static void notify_ring(struct intel_engine_cs *engine)
> >                */
> >               if (i915_seqno_passed(intel_engine_get_seqno(engine),
> >                                     wait->seqno)) {
> > +                     struct drm_i915_gem_request *waiter = wait->request;
> > +
> >                       wakeup = true;
> >                       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > -                                   &wait->request->fence.flags))
> > -                             rq = i915_gem_request_get(wait->request);
> > +                                   &waiter->fence.flags) &&
> > +                         intel_wait_check_request(wait, waiter))
> > +                             rq = i915_gem_request_get(waiter);
> >               }
> >   
> >               if (wakeup)
> > 
> 
> Hm but as the user interrupt is nor serialized to exelists, what 
> prevents the preemption to happen after the intel_wait_check_request and 
> before dma_fence_signal?

The preemption is serialized to the breadcrumb, so that our
understanding of i915_gem_request_completed() is accurate. It would be
a nasty bug if we flagged DMA_FENCE_FLAG_SIGNALED_BIT prior to
i915_gem_request_completed().

Here, wait_check_request just confirms that the wait->seqno still
matches the rq->global_seqno. So as we have already found that the
breadcrumb has passed wait->seqno, that means that the request is ready
to wake up. We only do the wake avoidance if we believe that we can
trust the order of MI_USER_INTERRUPT with the breadcrumb write, so that
then we know that if the request is not yet ready, we will receive
another interrupt in the future. (On the other side, if the wait queue
is manipulated, it ensures that the new waiter is woken to do the
breadcrumb check to avoid missed interrupts.)

So... If the preemption happens after intel_wait_check_request, it does
not affect the prior completed request.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Check waiter->seqno carefully in case of preemption

[Michał Winiarski]

On Tue, Sep 19, 2017 at 11:07:43AM +0100, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-09-19 10:28:42)
> > 
> > On 18/09/2017 17:27, Chris Wilson wrote:
> > > If preemption occurs at precisely the right moment, we may decide that
> > > the wait is complete even though the wait's request is no longer
> > > executing (having been preempted). We handle this situation by double
> > > checking that request following deciding whether the wait is complete.
> > > 
> > > Reported-by: Michał Winiarski <michal.winiarski@intel.com
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index bb69c5b0efc4..7a53d94b7e61 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -1053,10 +1053,13 @@ static void notify_ring(struct intel_engine_cs *engine)
> > >                */
> > >               if (i915_seqno_passed(intel_engine_get_seqno(engine),
> > >                                     wait->seqno)) {
> > > +                     struct drm_i915_gem_request *waiter = wait->request;
> > > +
> > >                       wakeup = true;
> > >                       if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > > -                                   &wait->request->fence.flags))
> > > -                             rq = i915_gem_request_get(wait->request);
> > > +                                   &waiter->fence.flags) &&
> > > +                         intel_wait_check_request(wait, waiter))
> > > +                             rq = i915_gem_request_get(waiter);
> > >               }
> > >   
> > >               if (wakeup)
> > > 
> > 
> > Hm but as the user interrupt is nor serialized to exelists, what 
> > prevents the preemption to happen after the intel_wait_check_request and 
> > before dma_fence_signal?
> 
> The preemption is serialized to the breadcrumb, so that our
> understanding of i915_gem_request_completed() is accurate. It would be
> a nasty bug if we flagged DMA_FENCE_FLAG_SIGNALED_BIT prior to
> i915_gem_request_completed().
> 
> Here, wait_check_request just confirms that the wait->seqno still
> matches the rq->global_seqno. So as we have already found that the
> breadcrumb has passed wait->seqno, that means that the request is ready
> to wake up. We only do the wake avoidance if we believe that we can
> trust the order of MI_USER_INTERRUPT with the breadcrumb write, so that
> then we know that if the request is not yet ready, we will receive
> another interrupt in the future. (On the other side, if the wait queue
> is manipulated, it ensures that the new waiter is woken to do the
> breadcrumb check to avoid missed interrupts.)
> 
> So... If the preemption happens after intel_wait_check_request, it does
> not affect the prior completed request.

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> -Chris
> _______________________________________________
> 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