All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
@ 2018-02-06  9:46 ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-06  9:46 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson, Tvrtko Ursulin, Joonas Lahtinen, stable

When a request is preempted, it is unsubmitted from the HW queue and
removed from the active list of breadcrumbs. In the process, this
however triggers the signaler and it may see the clear rbtree with the
old, and still valid, seqno. This confuses the signaler into action and
signaling the fence.

Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.12+
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index efbc627a2a25..b955f7d7bd0f 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -588,29 +588,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
 	spin_unlock_irq(&b->rb_lock);
 }
 
-static bool signal_valid(const struct drm_i915_gem_request *request)
-{
-	return intel_wait_check_request(&request->signaling.wait, request);
-}
-
 static bool signal_complete(const struct drm_i915_gem_request *request)
 {
 	if (!request)
 		return false;
 
-	/* If another process served as the bottom-half it may have already
-	 * signalled that this wait is already completed.
-	 */
-	if (intel_wait_complete(&request->signaling.wait))
-		return signal_valid(request);
-
-	/* Carefully check if the request is complete, giving time for the
+	/*
+	 * Carefully check if the request is complete, giving time for the
 	 * seqno to be visible or if the GPU hung.
 	 */
-	if (__i915_request_irq_complete(request))
-		return true;
-
-	return false;
+	return __i915_request_irq_complete(request);
 }
 
 static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
@@ -712,6 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
 				      &request->fence.flags)) {
 				local_bh_disable();
 				dma_fence_signal(&request->fence);
+				GEM_BUG_ON(!i915_gem_request_completed(request));
 				local_bh_enable(); /* kick start the tasklets */
 			}
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
@ 2018-02-06  9:46 ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-06  9:46 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable

When a request is preempted, it is unsubmitted from the HW queue and
removed from the active list of breadcrumbs. In the process, this
however triggers the signaler and it may see the clear rbtree with the
old, and still valid, seqno. This confuses the signaler into action and
signaling the fence.

Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.12+
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index efbc627a2a25..b955f7d7bd0f 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -588,29 +588,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
 	spin_unlock_irq(&b->rb_lock);
 }
 
-static bool signal_valid(const struct drm_i915_gem_request *request)
-{
-	return intel_wait_check_request(&request->signaling.wait, request);
-}
-
 static bool signal_complete(const struct drm_i915_gem_request *request)
 {
 	if (!request)
 		return false;
 
-	/* If another process served as the bottom-half it may have already
-	 * signalled that this wait is already completed.
-	 */
-	if (intel_wait_complete(&request->signaling.wait))
-		return signal_valid(request);
-
-	/* Carefully check if the request is complete, giving time for the
+	/*
+	 * Carefully check if the request is complete, giving time for the
 	 * seqno to be visible or if the GPU hung.
 	 */
-	if (__i915_request_irq_complete(request))
-		return true;
-
-	return false;
+	return __i915_request_irq_complete(request);
 }
 
 static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
@@ -712,6 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
 				      &request->fence.flags)) {
 				local_bh_disable();
 				dma_fence_signal(&request->fence);
+				GEM_BUG_ON(!i915_gem_request_completed(request));
 				local_bh_enable(); /* kick start the tasklets */
 			}
 
-- 
2.15.1

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
  2018-02-06  9:46 ` Chris Wilson
  (?)
@ 2018-02-06  9:52 ` Chris Wilson
  -1 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-06  9:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: Tvrtko Ursulin, Joonas Lahtinen, stable

Quoting Chris Wilson (2018-02-06 09:46:33)
> When a request is preempted, it is unsubmitted from the HW queue and
> removed from the active list of breadcrumbs. In the process, this
> however triggers the signaler and it may see the clear rbtree with the
> old, and still valid, seqno. This confuses the signaler into action and
> signaling the fence.
> 
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
Testcase: igt/gem_exec_whisper/*-priority # reproducibility ~10%
-Chris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915/breadcrumbs: Ignore unsubmitted signalers
  2018-02-06  9:46 ` Chris Wilson
  (?)
  (?)
@ 2018-02-06 10:29 ` Patchwork
  -1 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-02-06 10:29 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/breadcrumbs: Ignore unsubmitted signalers
URL   : https://patchwork.freedesktop.org/series/37724/
State : success

== Summary ==

Series 37724v1 drm/i915/breadcrumbs: Ignore unsubmitted signalers
https://patchwork.freedesktop.org/api/1.0/series/37724/revisions/1/mbox/

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:423s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:421s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:374s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:490s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:287s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:484s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:488s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:470s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:459s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:563s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:575s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:281s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:516s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:390s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:404s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:414s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:460s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:416s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:457s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:493s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:502s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:593s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:428s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:515s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:527s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:484s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:415s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:427s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:529s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:402s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:472s

5883e1383ed69b4bd7a537ceafeeabbc61cdf55e drm-tip: 2018y-02m-06d-09h-04m-49s UTC integration manifest
2eb59c50f0ca drm/i915/breadcrumbs: Ignore unsubmitted signalers

== Logs ==

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* ✗ Fi.CI.IGT: warning for drm/i915/breadcrumbs: Ignore unsubmitted signalers
  2018-02-06  9:46 ` Chris Wilson
                   ` (2 preceding siblings ...)
  (?)
@ 2018-02-06 12:12 ` Patchwork
  -1 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-02-06 12:12 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/breadcrumbs: Ignore unsubmitted signalers
URL   : https://patchwork.freedesktop.org/series/37724/
State : warning

== Summary ==

Test gem_eio:
        Subgroup in-flight:
                dmesg-warn -> PASS       (shard-snb) fdo#104058
Test kms_flip:
        Subgroup modeset-vs-vblank-race:
                fail       -> PASS       (shard-apl) fdo#103060
        Subgroup flip-vs-expired-vblank:
                pass       -> FAIL       (shard-apl) fdo#102887
        Subgroup 2x-flip-vs-wf_vblank:
                fail       -> PASS       (shard-hsw) fdo#100368
Test kms_vblank:
        Subgroup pipe-b-ts-continuation-dpms-suspend:
                pass       -> SKIP       (shard-hsw)
Test kms_cursor_crc:
        Subgroup cursor-128x128-suspend:
                pass       -> SKIP       (shard-snb) fdo#103880
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047
Test kms_frontbuffer_tracking:
        Subgroup fbc-2p-scndscrn-cur-indfb-move:
                skip       -> PASS       (shard-hsw)
Test kms_atomic_interruptible:
        Subgroup legacy-setmode:
                pass       -> SKIP       (shard-snb)

fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103880 https://bugs.freedesktop.org/show_bug.cgi?id=103880
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-apl        total:3379 pass:1744 dwarn:1   dfail:0   fail:22  skip:1610 time:12444s
shard-hsw        total:3442 pass:1757 dwarn:1   dfail:0   fail:11  skip:1672 time:11707s
shard-snb        total:3442 pass:1349 dwarn:1   dfail:0   fail:10  skip:2082 time:6608s

== Logs ==

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
  2018-02-06  9:46 ` Chris Wilson
                   ` (3 preceding siblings ...)
  (?)
@ 2018-02-06 17:13 ` Chris Wilson
  -1 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-06 17:13 UTC (permalink / raw)
  To: intel-gfx; +Cc: Tvrtko Ursulin, Joonas Lahtinen, stable

Quoting Chris Wilson (2018-02-06 09:46:33)
> When a request is preempted, it is unsubmitted from the HW queue and
> removed from the active list of breadcrumbs. In the process, this
> however triggers the signaler and it may see the clear rbtree with the
> old, and still valid, seqno. This confuses the signaler into action and
> signaling the fence.
> 
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.12+

Any takers for this brown paper bug?

> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index efbc627a2a25..b955f7d7bd0f 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -588,29 +588,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
>         spin_unlock_irq(&b->rb_lock);
>  }
>  
> -static bool signal_valid(const struct drm_i915_gem_request *request)
> -{
> -       return intel_wait_check_request(&request->signaling.wait, request);
> -}
> -
>  static bool signal_complete(const struct drm_i915_gem_request *request)
>  {
>         if (!request)
>                 return false;
>  
> -       /* If another process served as the bottom-half it may have already
> -        * signalled that this wait is already completed.
> -        */
> -       if (intel_wait_complete(&request->signaling.wait))
> -               return signal_valid(request);
> -
> -       /* Carefully check if the request is complete, giving time for the
> +       /*
> +        * Carefully check if the request is complete, giving time for the
>          * seqno to be visible or if the GPU hung.
>          */
> -       if (__i915_request_irq_complete(request))
> -               return true;
> -
> -       return false;
> +       return __i915_request_irq_complete(request);
>  }
>  
>  static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
> @@ -712,6 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>                                       &request->fence.flags)) {
>                                 local_bh_disable();
>                                 dma_fence_signal(&request->fence);
> +                               GEM_BUG_ON(!i915_gem_request_completed(request));
>                                 local_bh_enable(); /* kick start the tasklets */
>                         }
>  
> -- 
> 2.15.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
  2018-02-06  9:46 ` Chris Wilson
                   ` (4 preceding siblings ...)
  (?)
@ 2018-02-07 10:40 ` Tvrtko Ursulin
  2018-02-07 11:04   ` Chris Wilson
  2018-02-07 11:10   ` Chris Wilson
  -1 siblings, 2 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2018-02-07 10:40 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Joonas Lahtinen, stable


On 06/02/2018 09:46, Chris Wilson wrote:
> When a request is preempted, it is unsubmitted from the HW queue and
> removed from the active list of breadcrumbs. In the process, this
> however triggers the signaler and it may see the clear rbtree with the
> old, and still valid, seqno. This confuses the signaler into action and
> signaling the fence.
> 
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.12+
> ---
>   drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
>   1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index efbc627a2a25..b955f7d7bd0f 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -588,29 +588,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
>   	spin_unlock_irq(&b->rb_lock);
>   }
>   
> -static bool signal_valid(const struct drm_i915_gem_request *request)
> -{
> -	return intel_wait_check_request(&request->signaling.wait, request);
> -}
> -
>   static bool signal_complete(const struct drm_i915_gem_request *request)
>   {
>   	if (!request)
>   		return false;
>   
> -	/* If another process served as the bottom-half it may have already
> -	 * signalled that this wait is already completed.
> -	 */
> -	if (intel_wait_complete(&request->signaling.wait))
> -		return signal_valid(request);

Okay so this can return true for unsubmitted requests since rb node will 
be empty and global_seqno == wait.seqno == 0.

I just panic when thinking about races and ordering, since these checks 
used to run unlocked. So even better that they are gone.

> -
> -	/* Carefully check if the request is complete, giving time for the
> +	/*
> +	 * Carefully check if the request is complete, giving time for the
>   	 * seqno to be visible or if the GPU hung.
>   	 */
> -	if (__i915_request_irq_complete(request))
> -		return true;
> -
> -	return false;
> +	return __i915_request_irq_complete(request); >   }
>   
>   static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
> @@ -712,6 +699,7 @@ static int intel_breadcrumbs_signaler(void *arg)
>   				      &request->fence.flags)) {
>   				local_bh_disable();
>   				dma_fence_signal(&request->fence);
> +				GEM_BUG_ON(!i915_gem_request_completed(request));
>   				local_bh_enable(); /* kick start the tasklets */
>   			}
>   
> 

Looks OK. But I can't say it's straightforward to understand it.

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

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
  2018-02-07 10:40 ` Tvrtko Ursulin
@ 2018-02-07 11:04   ` Chris Wilson
  2018-02-07 11:10   ` Chris Wilson
  1 sibling, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-07 11:04 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: Joonas Lahtinen, stable

Quoting Tvrtko Ursulin (2018-02-07 10:40:46)
> 
> On 06/02/2018 09:46, Chris Wilson wrote:
> > When a request is preempted, it is unsubmitted from the HW queue and
> > removed from the active list of breadcrumbs. In the process, this
> > however triggers the signaler and it may see the clear rbtree with the
> > old, and still valid, seqno. This confuses the signaler into action and
> > signaling the fence.
> > 
> > Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.12+
> > ---
> >   drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 ++++----------------
> >   1 file changed, 4 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > index efbc627a2a25..b955f7d7bd0f 100644
> > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > @@ -588,29 +588,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
> >       spin_unlock_irq(&b->rb_lock);
> >   }
> >   
> > -static bool signal_valid(const struct drm_i915_gem_request *request)
> > -{
> > -     return intel_wait_check_request(&request->signaling.wait, request);
> > -}
> > -
> >   static bool signal_complete(const struct drm_i915_gem_request *request)
> >   {
> >       if (!request)
> >               return false;
> >   
> > -     /* If another process served as the bottom-half it may have already
> > -      * signalled that this wait is already completed.
> > -      */
> > -     if (intel_wait_complete(&request->signaling.wait))
> > -             return signal_valid(request);
> 
> Okay so this can return true for unsubmitted requests since rb node will 
> be empty and global_seqno == wait.seqno == 0.

Hmm, ah, signal_valid() operated under the belief that its wait.seqno
was untouched. That makes a bit more sense now. I was having to concoct
some scary data races to try and explain how global_seqno and wait.seqno
had the same non-zero value.
-Chris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
  2018-02-07 10:40 ` Tvrtko Ursulin
  2018-02-07 11:04   ` Chris Wilson
@ 2018-02-07 11:10   ` Chris Wilson
  1 sibling, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-07 11:10 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: Joonas Lahtinen, stable

Quoting Tvrtko Ursulin (2018-02-07 10:40:46)
> 
> On 06/02/2018 09:46, Chris Wilson wrote:
> > When a request is preempted, it is unsubmitted from the HW queue and
> > removed from the active list of breadcrumbs. In the process, this
> > however triggers the signaler and it may see the clear rbtree with the
> > old, and still valid, seqno. This confuses the signaler into action and
> > signaling the fence.
> > 
> > Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.12+
> > ---
> Looks OK. But I can't say it's straightforward to understand it.

But I hope the GEM_BUG_ON() was nice icing on the cake :)
 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks, added your explanation to the changelog and pushed.
-Chris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
  2018-02-13  9:01 ` [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers Chris Wilson
@ 2018-02-14  1:00     ` Rodrigo Vivi
  0 siblings, 0 replies; 12+ messages in thread
From: Rodrigo Vivi @ 2018-02-14  1:00 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, Tvrtko Ursulin, Joonas Lahtinen, stable

On Tue, Feb 13, 2018 at 09:01:54AM +0000, Chris Wilson wrote:
> When a request is preempted, it is unsubmitted from the HW queue and
> removed from the active list of breadcrumbs. In the process, this
> however triggers the signaler and it may see the clear rbtree with the
> old, and still valid, seqno, or it may match the cleared seqno with the
> now zero rq->global_seqno. This confuses the signaler into action and
> signaling the fence.
> 
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.12+
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Link: https://patchwork.freedesktop.org/patch/msgid/20180206094633.30181-1-chris@chris-wilson.co.uk
> (cherry picked from commit fd10e2ce9905030d922e179a8047a4d50daffd8e)

applied to fixes. Thanks

> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index bd40fea16b4f..f54ddda9fdad 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -594,29 +594,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
>  	spin_unlock_irq(&b->rb_lock);
>  }
>  
> -static bool signal_valid(const struct drm_i915_gem_request *request)
> -{
> -	return intel_wait_check_request(&request->signaling.wait, request);
> -}
> -
>  static bool signal_complete(const struct drm_i915_gem_request *request)
>  {
>  	if (!request)
>  		return false;
>  
> -	/* If another process served as the bottom-half it may have already
> -	 * signalled that this wait is already completed.
> -	 */
> -	if (intel_wait_complete(&request->signaling.wait))
> -		return signal_valid(request);
> -
> -	/* Carefully check if the request is complete, giving time for the
> +	/*
> +	 * Carefully check if the request is complete, giving time for the
>  	 * seqno to be visible or if the GPU hung.
>  	 */
> -	if (__i915_request_irq_complete(request))
> -		return true;
> -
> -	return false;
> +	return __i915_request_irq_complete(request);
>  }
>  
>  static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
> @@ -659,9 +646,13 @@ static int intel_breadcrumbs_signaler(void *arg)
>  			request = i915_gem_request_get_rcu(request);
>  		rcu_read_unlock();
>  		if (signal_complete(request)) {
> -			local_bh_disable();
> -			dma_fence_signal(&request->fence);
> -			local_bh_enable(); /* kick start the tasklets */
> +			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> +				      &request->fence.flags)) {
> +				local_bh_disable();
> +				dma_fence_signal(&request->fence);
> +				GEM_BUG_ON(!i915_gem_request_completed(request));
> +				local_bh_enable(); /* kick start the tasklets */
> +			}
>  
>  			spin_lock_irq(&b->rb_lock);
>  
> -- 
> 2.16.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
@ 2018-02-14  1:00     ` Rodrigo Vivi
  0 siblings, 0 replies; 12+ messages in thread
From: Rodrigo Vivi @ 2018-02-14  1:00 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, stable

On Tue, Feb 13, 2018 at 09:01:54AM +0000, Chris Wilson wrote:
> When a request is preempted, it is unsubmitted from the HW queue and
> removed from the active list of breadcrumbs. In the process, this
> however triggers the signaler and it may see the clear rbtree with the
> old, and still valid, seqno, or it may match the cleared seqno with the
> now zero rq->global_seqno. This confuses the signaler into action and
> signaling the fence.
> 
> Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.12+
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Link: https://patchwork.freedesktop.org/patch/msgid/20180206094633.30181-1-chris@chris-wilson.co.uk
> (cherry picked from commit fd10e2ce9905030d922e179a8047a4d50daffd8e)

applied to fixes. Thanks

> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index bd40fea16b4f..f54ddda9fdad 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -594,29 +594,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
>  	spin_unlock_irq(&b->rb_lock);
>  }
>  
> -static bool signal_valid(const struct drm_i915_gem_request *request)
> -{
> -	return intel_wait_check_request(&request->signaling.wait, request);
> -}
> -
>  static bool signal_complete(const struct drm_i915_gem_request *request)
>  {
>  	if (!request)
>  		return false;
>  
> -	/* If another process served as the bottom-half it may have already
> -	 * signalled that this wait is already completed.
> -	 */
> -	if (intel_wait_complete(&request->signaling.wait))
> -		return signal_valid(request);
> -
> -	/* Carefully check if the request is complete, giving time for the
> +	/*
> +	 * Carefully check if the request is complete, giving time for the
>  	 * seqno to be visible or if the GPU hung.
>  	 */
> -	if (__i915_request_irq_complete(request))
> -		return true;
> -
> -	return false;
> +	return __i915_request_irq_complete(request);
>  }
>  
>  static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
> @@ -659,9 +646,13 @@ static int intel_breadcrumbs_signaler(void *arg)
>  			request = i915_gem_request_get_rcu(request);
>  		rcu_read_unlock();
>  		if (signal_complete(request)) {
> -			local_bh_disable();
> -			dma_fence_signal(&request->fence);
> -			local_bh_enable(); /* kick start the tasklets */
> +			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> +				      &request->fence.flags)) {
> +				local_bh_disable();
> +				dma_fence_signal(&request->fence);
> +				GEM_BUG_ON(!i915_gem_request_completed(request));
> +				local_bh_enable(); /* kick start the tasklets */
> +			}
>  
>  			spin_lock_irq(&b->rb_lock);
>  
> -- 
> 2.16.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers
  2018-02-13  7:38 patches that failed to cherry-pick on drm-intel-fixes for 4.16-rc1 Rodrigo Vivi
@ 2018-02-13  9:01 ` Chris Wilson
  2018-02-14  1:00     ` Rodrigo Vivi
  0 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2018-02-13  9:01 UTC (permalink / raw)
  To: intel-gfx
  Cc: rodrigo.vivi, Chris Wilson, Tvrtko Ursulin, Joonas Lahtinen, stable

When a request is preempted, it is unsubmitted from the HW queue and
removed from the active list of breadcrumbs. In the process, this
however triggers the signaler and it may see the clear rbtree with the
old, and still valid, seqno, or it may match the cleared seqno with the
now zero rq->global_seqno. This confuses the signaler into action and
signaling the fence.

Fixes: d6a2289d9d6b ("drm/i915: Remove the preempted request from the execution queue")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.12+
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20180206094633.30181-1-chris@chris-wilson.co.uk
(cherry picked from commit fd10e2ce9905030d922e179a8047a4d50daffd8e)
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index bd40fea16b4f..f54ddda9fdad 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -594,29 +594,16 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
 	spin_unlock_irq(&b->rb_lock);
 }
 
-static bool signal_valid(const struct drm_i915_gem_request *request)
-{
-	return intel_wait_check_request(&request->signaling.wait, request);
-}
-
 static bool signal_complete(const struct drm_i915_gem_request *request)
 {
 	if (!request)
 		return false;
 
-	/* If another process served as the bottom-half it may have already
-	 * signalled that this wait is already completed.
-	 */
-	if (intel_wait_complete(&request->signaling.wait))
-		return signal_valid(request);
-
-	/* Carefully check if the request is complete, giving time for the
+	/*
+	 * Carefully check if the request is complete, giving time for the
 	 * seqno to be visible or if the GPU hung.
 	 */
-	if (__i915_request_irq_complete(request))
-		return true;
-
-	return false;
+	return __i915_request_irq_complete(request);
 }
 
 static struct drm_i915_gem_request *to_signaler(struct rb_node *rb)
@@ -659,9 +646,13 @@ static int intel_breadcrumbs_signaler(void *arg)
 			request = i915_gem_request_get_rcu(request);
 		rcu_read_unlock();
 		if (signal_complete(request)) {
-			local_bh_disable();
-			dma_fence_signal(&request->fence);
-			local_bh_enable(); /* kick start the tasklets */
+			if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+				      &request->fence.flags)) {
+				local_bh_disable();
+				dma_fence_signal(&request->fence);
+				GEM_BUG_ON(!i915_gem_request_completed(request));
+				local_bh_enable(); /* kick start the tasklets */
+			}
 
 			spin_lock_irq(&b->rb_lock);
 
-- 
2.16.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-02-14  1:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-06  9:46 [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers Chris Wilson
2018-02-06  9:46 ` Chris Wilson
2018-02-06  9:52 ` Chris Wilson
2018-02-06 10:29 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-02-06 12:12 ` ✗ Fi.CI.IGT: warning " Patchwork
2018-02-06 17:13 ` [PATCH] " Chris Wilson
2018-02-07 10:40 ` Tvrtko Ursulin
2018-02-07 11:04   ` Chris Wilson
2018-02-07 11:10   ` Chris Wilson
2018-02-13  7:38 patches that failed to cherry-pick on drm-intel-fixes for 4.16-rc1 Rodrigo Vivi
2018-02-13  9:01 ` [PATCH] drm/i915/breadcrumbs: Ignore unsubmitted signalers Chris Wilson
2018-02-14  1:00   ` Rodrigo Vivi
2018-02-14  1:00     ` Rodrigo Vivi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.