[PATCH] drm/i915: Order two completing nop_submit_request

[PATCH] drm/i915: Order two completing nop_submit_request

[Chris Wilson]

If two nop's (requests in-flight following a wedged device) complete at
the same time, the global_seqno value written to the HWSP is undefined
as the two threads are not serialized.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ab8c6946fea4..2779bcaff82e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3016,8 +3016,11 @@ static void nop_submit_request(struct drm_i915_gem_request *request)
 {
 	GEM_BUG_ON(!i915_terminally_wedged(&request->i915->gpu_error));
 	dma_fence_set_error(&request->fence, -EIO);
-	i915_gem_request_submit(request);
+
+	spin_lock_irq(&request->engine->timeline->lock);
+	__i915_gem_request_submit(request);
 	intel_engine_init_global_seqno(request->engine, request->global_seqno);
+	spin_unlock_irq(&request->engine->timeline->lock);
 }
 
 static void engine_set_wedged(struct intel_engine_cs *engine)
-- 
2.14.2

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

Re: [PATCH] drm/i915: Order two completing nop_submit_request

[Tvrtko Ursulin]

On 06/10/2017 12:22, Chris Wilson wrote:
> If two nop's (requests in-flight following a wedged device) complete at
> the same time, the global_seqno value written to the HWSP is undefined
> as the two threads are not serialized.
> 
> Fixes: ce1135c7de64 ("drm/i915: Complete requests in nop_submit_request")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ab8c6946fea4..2779bcaff82e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3016,8 +3016,11 @@ static void nop_submit_request(struct drm_i915_gem_request *request)
>   {
>   	GEM_BUG_ON(!i915_terminally_wedged(&request->i915->gpu_error));
>   	dma_fence_set_error(&request->fence, -EIO);
> -	i915_gem_request_submit(request);
> +
> +	spin_lock_irq(&request->engine->timeline->lock);
> +	__i915_gem_request_submit(request);
>   	intel_engine_init_global_seqno(request->engine, request->global_seqno);
> +	spin_unlock_irq(&request->engine->timeline->lock);
>   }
>   
>   static void engine_set_wedged(struct intel_engine_cs *engine)
> 

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] drm/i915: Order two completing nop_submit_request

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-10-06 12:42:34)
> 
> On 06/10/2017 12:22, Chris Wilson wrote:
> > If two nop's (requests in-flight following a wedged device) complete at
> > the same time, the global_seqno value written to the HWSP is undefined
> > as the two threads are not serialized.
> > 
> > Fixes: ce1135c7de64 ("drm/i915: Complete requests in nop_submit_request")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index ab8c6946fea4..2779bcaff82e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3016,8 +3016,11 @@ static void nop_submit_request(struct drm_i915_gem_request *request)
> >   {
> >       GEM_BUG_ON(!i915_terminally_wedged(&request->i915->gpu_error));
> >       dma_fence_set_error(&request->fence, -EIO);
> > -     i915_gem_request_submit(request);
> > +
> > +     spin_lock_irq(&request->engine->timeline->lock);
> > +     __i915_gem_request_submit(request);
> >       intel_engine_init_global_seqno(request->engine, request->global_seqno);
> > +     spin_unlock_irq(&request->engine->timeline->lock);
> >   }
> >   
> >   static void engine_set_wedged(struct intel_engine_cs *engine)
> > 
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Hmm, I better make that irqsave/irqrestore. I guess that will make you
happier as well :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Order two completing nop_submit_request

[Chris Wilson]

If two nop's (requests in-flight following a wedged device) complete at
the same time, the global_seqno value written to the HWSP is undefined
as the two threads are not serialized.

v2: Use irqsafe spinlock. We expect the callback may be called from
inside another irq spinlock, so we can't unconditionally restore irqs.

Fixes: ce1135c7de64 ("drm/i915: Complete requests in nop_submit_request")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v1
---
 drivers/gpu/drm/i915/i915_gem.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ab8c6946fea4..6a6974ed8f74 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3014,10 +3014,15 @@ void i915_gem_reset_finish(struct drm_i915_private *dev_priv)
 
 static void nop_submit_request(struct drm_i915_gem_request *request)
 {
+	unsigned long flags;
+
 	GEM_BUG_ON(!i915_terminally_wedged(&request->i915->gpu_error));
 	dma_fence_set_error(&request->fence, -EIO);
-	i915_gem_request_submit(request);
+
+	spin_lock_irqsave(&request->engine->timeline->lock, flags);
+	__i915_gem_request_submit(request);
 	intel_engine_init_global_seqno(request->engine, request->global_seqno);
+	spin_unlock_irqrestore(&request->engine->timeline->lock, flags);
 }
 
 static void engine_set_wedged(struct intel_engine_cs *engine)
-- 
2.14.2

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

Re: [PATCH v2] drm/i915: Order two completing nop_submit_request

[Tvrtko Ursulin]

On 06/10/2017 12:56, Chris Wilson wrote:
> If two nop's (requests in-flight following a wedged device) complete at
> the same time, the global_seqno value written to the HWSP is undefined
> as the two threads are not serialized.
> 
> v2: Use irqsafe spinlock. We expect the callback may be called from
> inside another irq spinlock, so we can't unconditionally restore irqs.
> 
> Fixes: ce1135c7de64 ("drm/i915: Complete requests in nop_submit_request")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v1
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ab8c6946fea4..6a6974ed8f74 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3014,10 +3014,15 @@ void i915_gem_reset_finish(struct drm_i915_private *dev_priv)
>   
>   static void nop_submit_request(struct drm_i915_gem_request *request)
>   {
> +	unsigned long flags;
> +
>   	GEM_BUG_ON(!i915_terminally_wedged(&request->i915->gpu_error));
>   	dma_fence_set_error(&request->fence, -EIO);
> -	i915_gem_request_submit(request);
> +
> +	spin_lock_irqsave(&request->engine->timeline->lock, flags);
> +	__i915_gem_request_submit(request);
>   	intel_engine_init_global_seqno(request->engine, request->global_seqno);
> +	spin_unlock_irqrestore(&request->engine->timeline->lock, flags);
>   }
>   
>   static void engine_set_wedged(struct intel_engine_cs *engine)
> 

Ooops..

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

✓ Fi.CI.BAT: success for drm/i915: Order two completing nop_submit_request (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Order two completing nop_submit_request (rev2)
URL   : https://patchwork.freedesktop.org/series/31486/
State : success

== Summary ==

Series 31486v2 drm/i915: Order two completing nop_submit_request
https://patchwork.freedesktop.org/api/1.0/series/31486/revisions/2/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-n2820) fdo#101705
Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> INCOMPLETE (fi-cfl-s) fdo#103022

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

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:460s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:471s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:395s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:574s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:290s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:526s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:530s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:547s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:522s
fi-cfl-s         total:288  pass:255  dwarn:1   dfail:0   fail:0   skip:31 
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:621s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:434s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:600s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:436s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:428s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:511s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:476s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:495s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:580s
fi-kbl-7567u     total:289  pass:265  dwarn:4   dfail:0   fail:0   skip:20  time:492s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:601s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:660s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:481s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:661s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:532s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:510s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:473s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:583s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:436s

7dacd1f2e70cb3202e2b153d76b05b601d099082 drm-tip: 2017y-10m-06d-12h-29m-28s UTC integration manifest
a89ae6600867 drm/i915: Order two completing nop_submit_request

== Logs ==

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

✗ Fi.CI.IGT: warning for drm/i915: Order two completing nop_submit_request (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Order two completing nop_submit_request (rev2)
URL   : https://patchwork.freedesktop.org/series/31486/
State : warning

== Summary ==

Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-B-planes:
                pass       -> SKIP       (shard-hsw)

shard-hsw        total:2446 pass:1327 dwarn:6   dfail:0   fail:9   skip:1104 time:10071s

== Logs ==

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

Re: [PATCH v2] drm/i915: Order two completing nop_submit_request

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-10-06 13:23:03)
> 
> On 06/10/2017 12:56, Chris Wilson wrote:
> > If two nop's (requests in-flight following a wedged device) complete at
> > the same time, the global_seqno value written to the HWSP is undefined
> > as the two threads are not serialized.
> > 
> > v2: Use irqsafe spinlock. We expect the callback may be called from
> > inside another irq spinlock, so we can't unconditionally restore irqs.
> > 
> > Fixes: ce1135c7de64 ("drm/i915: Complete requests in nop_submit_request")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v1
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index ab8c6946fea4..6a6974ed8f74 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3014,10 +3014,15 @@ void i915_gem_reset_finish(struct drm_i915_private *dev_priv)
> >   
> >   static void nop_submit_request(struct drm_i915_gem_request *request)
> >   {
> > +     unsigned long flags;
> > +
> >       GEM_BUG_ON(!i915_terminally_wedged(&request->i915->gpu_error));
> >       dma_fence_set_error(&request->fence, -EIO);
> > -     i915_gem_request_submit(request);
> > +
> > +     spin_lock_irqsave(&request->engine->timeline->lock, flags);
> > +     __i915_gem_request_submit(request);
> >       intel_engine_init_global_seqno(request->engine, request->global_seqno);
> > +     spin_unlock_irqrestore(&request->engine->timeline->lock, flags);
> >   }
> >   
> >   static void engine_set_wedged(struct intel_engine_cs *engine)
> > 
> 
> Ooops..
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks for asking the question that lead to the discovery of the race and
then reviewing the results! Pushed,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx