* [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper
@ 2016-08-08 7:28 Chris Wilson
2016-08-08 7:28 ` [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Chris Wilson @ 2016-08-08 7:28 UTC (permalink / raw)
To: intel-gfx
As we do the lockdep protected RCU lookup in a couple of places,
refactor that code to a common helper i915_gem_active_raw()..
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_gem_request.h | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 3496e28785e7..6dd01cbf4895 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -360,6 +360,21 @@ __i915_gem_active_peek(const struct i915_gem_active *active)
}
/**
+ * i915_gem_active_raw - return the active request
+ * @active - the active tracker
+ *
+ * i915_gem_active_raw() returns the current request being tracked, or NULL.
+ * It does not obtain a reference on the request for the caller, so the caller
+ * must hold struct_mutex.
+ */
+static inline struct drm_i915_gem_request *
+i915_gem_active_raw(const struct i915_gem_active *active, struct mutex *mutex)
+{
+ return rcu_dereference_protected(active->request,
+ lockdep_is_held(mutex));
+}
+
+/**
* i915_gem_active_peek - report the active request being monitored
* @active - the active tracker
*
@@ -372,8 +387,7 @@ i915_gem_active_peek(const struct i915_gem_active *active, struct mutex *mutex)
{
struct drm_i915_gem_request *request;
- request = rcu_dereference_protected(active->request,
- lockdep_is_held(mutex));
+ request = i915_gem_active_raw(active, mutex);
if (!request || i915_gem_request_completed(request))
return NULL;
@@ -635,8 +649,7 @@ i915_gem_active_retire(struct i915_gem_active *active,
struct drm_i915_gem_request *request;
int ret;
- request = rcu_dereference_protected(active->request,
- lockdep_is_held(mutex));
+ request = i915_gem_active_raw(active, mutex);
if (!request)
return 0;
--
2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang
2016-08-08 7:28 [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper Chris Wilson
@ 2016-08-08 7:28 ` Chris Wilson
2016-08-09 6:43 ` Daniel Vetter
2016-08-12 10:20 ` Joonas Lahtinen
2016-08-08 7:56 ` ✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Wrap the protected active RCU dereference in a helper Patchwork
2016-08-09 6:36 ` [PATCH 1/2] " Daniel Vetter
2 siblings, 2 replies; 8+ messages in thread
From: Chris Wilson @ 2016-08-08 7:28 UTC (permalink / raw)
To: intel-gfx
When we force the cleanup after a GPU hang, we want to retire all
requests, or else we may leak them if truly wedged (and the GPU never
advances again). Converting to the active request helpers had the issue
of doing the check against busyness before reporting the request, so if
we claim the GPU had hung but this engine hadn't we could potential skip
the request cleanup - triggering the self-check BUG.
Fixes: dcff85c8443e ("drm/i915: Enable i915_gem_wait_for_idle() ...")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f4f8eaa90f2a..cd5229301aae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2423,15 +2423,11 @@ static void i915_gem_reset_engine_cleanup(struct intel_engine_cs *engine)
struct drm_i915_gem_request *request;
struct intel_ring *ring;
- request = i915_gem_active_peek(&engine->last_request,
- &engine->i915->drm.struct_mutex);
-
/* Mark all pending requests as complete so that any concurrent
* (lockless) lookup doesn't try and wait upon the request as we
* reset it.
*/
- if (request)
- intel_engine_init_seqno(engine, request->fence.seqno);
+ intel_engine_init_seqno(engine, engine->last_submitted_seqno);
/*
* Clear the execlists queue up before freeing the requests, as those
@@ -2453,6 +2449,8 @@ static void i915_gem_reset_engine_cleanup(struct intel_engine_cs *engine)
* implicit references on things like e.g. ppgtt address spaces through
* the request.
*/
+ request = i915_gem_active_raw(&engine->last_request,
+ &engine->i915->drm.struct_mutex);
if (request)
i915_gem_request_retire_upto(request);
GEM_BUG_ON(intel_engine_is_active(engine));
--
2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* ✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Wrap the protected active RCU dereference in a helper
2016-08-08 7:28 [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper Chris Wilson
2016-08-08 7:28 ` [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang Chris Wilson
@ 2016-08-08 7:56 ` Patchwork
2016-08-09 6:36 ` [PATCH 1/2] " Daniel Vetter
2 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2016-08-08 7:56 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: Wrap the protected active RCU dereference in a helper
URL : https://patchwork.freedesktop.org/series/10777/
State : failure
== Summary ==
Series 10777v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/10777/revisions/1/mbox
Test kms_cursor_legacy:
Subgroup basic-cursor-vs-flip-varying-size:
pass -> FAIL (ro-ilk1-i5-650)
Subgroup basic-flip-vs-cursor-legacy:
pass -> FAIL (ro-bdw-i7-5600u)
pass -> FAIL (ro-skl3-i5-6260u)
fi-kbl-qkkr total:244 pass:185 dwarn:30 dfail:0 fail:2 skip:27
ro-bdw-i5-5250u total:240 pass:218 dwarn:4 dfail:0 fail:2 skip:16
ro-bdw-i7-5600u total:240 pass:206 dwarn:0 dfail:0 fail:2 skip:32
ro-bsw-n3050 total:240 pass:194 dwarn:0 dfail:0 fail:4 skip:42
ro-byt-n2820 total:240 pass:197 dwarn:0 dfail:0 fail:3 skip:40
ro-hsw-i3-4010u total:240 pass:214 dwarn:0 dfail:0 fail:0 skip:26
ro-hsw-i7-4770r total:240 pass:214 dwarn:0 dfail:0 fail:0 skip:26
ro-ilk-i7-620lm total:240 pass:173 dwarn:1 dfail:0 fail:1 skip:65
ro-ilk1-i5-650 total:235 pass:173 dwarn:0 dfail:0 fail:2 skip:60
ro-ivb-i7-3770 total:240 pass:205 dwarn:0 dfail:0 fail:0 skip:35
ro-ivb2-i7-3770 total:240 pass:209 dwarn:0 dfail:0 fail:0 skip:31
ro-skl3-i5-6260u total:240 pass:222 dwarn:0 dfail:0 fail:4 skip:14
ro-snb-i7-2620M total:240 pass:198 dwarn:0 dfail:0 fail:1 skip:41
ro-bdw-i7-5557U failed to connect after reboot
Results at /archive/results/CI_IGT_test/RO_Patchwork_1757/
b834992 drm-intel-nightly: 2016y-08m-05d-20h-40m-44s UTC integration manifest
33b90c7 drm/i915: Don't check for idleness before retiring after a GPU hang
3869338 drm/i915: Wrap the protected active RCU dereference in a helper
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper
2016-08-08 7:28 [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper Chris Wilson
2016-08-08 7:28 ` [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang Chris Wilson
2016-08-08 7:56 ` ✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Wrap the protected active RCU dereference in a helper Patchwork
@ 2016-08-09 6:36 ` Daniel Vetter
2016-08-09 7:08 ` Chris Wilson
2 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2016-08-09 6:36 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Mon, Aug 08, 2016 at 08:28:47AM +0100, Chris Wilson wrote:
> As we do the lockdep protected RCU lookup in a couple of places,
> refactor that code to a common helper i915_gem_active_raw()..
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_gem_request.h | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index 3496e28785e7..6dd01cbf4895 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -360,6 +360,21 @@ __i915_gem_active_peek(const struct i915_gem_active *active)
> }
>
> /**
> + * i915_gem_active_raw - return the active request
> + * @active - the active tracker
> + *
> + * i915_gem_active_raw() returns the current request being tracked, or NULL.
> + * It does not obtain a reference on the request for the caller, so the caller
> + * must hold struct_mutex.
> + */
> +static inline struct drm_i915_gem_request *
> +i915_gem_active_raw(const struct i915_gem_active *active, struct mutex *mutex)
There's a rcu_dereference_raw helper which doesn't bother with any lockdep
checking. A bit confusing naming, otoh I couldn't come up with something
better.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> +{
> + return rcu_dereference_protected(active->request,
> + lockdep_is_held(mutex));
> +}
> +
> +/**
> * i915_gem_active_peek - report the active request being monitored
> * @active - the active tracker
> *
> @@ -372,8 +387,7 @@ i915_gem_active_peek(const struct i915_gem_active *active, struct mutex *mutex)
> {
> struct drm_i915_gem_request *request;
>
> - request = rcu_dereference_protected(active->request,
> - lockdep_is_held(mutex));
> + request = i915_gem_active_raw(active, mutex);
> if (!request || i915_gem_request_completed(request))
> return NULL;
>
> @@ -635,8 +649,7 @@ i915_gem_active_retire(struct i915_gem_active *active,
> struct drm_i915_gem_request *request;
> int ret;
>
> - request = rcu_dereference_protected(active->request,
> - lockdep_is_held(mutex));
> + request = i915_gem_active_raw(active, mutex);
> if (!request)
> return 0;
>
> --
> 2.8.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang
2016-08-08 7:28 ` [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang Chris Wilson
@ 2016-08-09 6:43 ` Daniel Vetter
2016-08-12 10:20 ` Joonas Lahtinen
1 sibling, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2016-08-09 6:43 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Mon, Aug 08, 2016 at 08:28:48AM +0100, Chris Wilson wrote:
> When we force the cleanup after a GPU hang, we want to retire all
> requests, or else we may leak them if truly wedged (and the GPU never
> advances again). Converting to the active request helpers had the issue
> of doing the check against busyness before reporting the request, so if
> we claim the GPU had hung but this engine hadn't we could potential skip
> the request cleanup - triggering the self-check BUG.
>
> Fixes: dcff85c8443e ("drm/i915: Enable i915_gem_wait_for_idle() ...")
Was confused for a while why this is a problem du to lockless
wait_for_idle until I realized that that patch also converted the engine
reset cleanup over to the new active helpers.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f4f8eaa90f2a..cd5229301aae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2423,15 +2423,11 @@ static void i915_gem_reset_engine_cleanup(struct intel_engine_cs *engine)
> struct drm_i915_gem_request *request;
> struct intel_ring *ring;
>
> - request = i915_gem_active_peek(&engine->last_request,
> - &engine->i915->drm.struct_mutex);
> -
> /* Mark all pending requests as complete so that any concurrent
> * (lockless) lookup doesn't try and wait upon the request as we
> * reset it.
> */
> - if (request)
> - intel_engine_init_seqno(engine, request->fence.seqno);
> + intel_engine_init_seqno(engine, engine->last_submitted_seqno);
>
> /*
> * Clear the execlists queue up before freeing the requests, as those
> @@ -2453,6 +2449,8 @@ static void i915_gem_reset_engine_cleanup(struct intel_engine_cs *engine)
> * implicit references on things like e.g. ppgtt address spaces through
> * the request.
> */
> + request = i915_gem_active_raw(&engine->last_request,
> + &engine->i915->drm.struct_mutex);
> if (request)
> i915_gem_request_retire_upto(request);
> GEM_BUG_ON(intel_engine_is_active(engine));
> --
> 2.8.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper
2016-08-09 6:36 ` [PATCH 1/2] " Daniel Vetter
@ 2016-08-09 7:08 ` Chris Wilson
2016-08-10 10:21 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2016-08-09 7:08 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Tue, Aug 09, 2016 at 08:36:56AM +0200, Daniel Vetter wrote:
> On Mon, Aug 08, 2016 at 08:28:47AM +0100, Chris Wilson wrote:
> > As we do the lockdep protected RCU lookup in a couple of places,
> > refactor that code to a common helper i915_gem_active_raw()..
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/i915_gem_request.h | 21 +++++++++++++++++----
> > 1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> > index 3496e28785e7..6dd01cbf4895 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_request.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> > @@ -360,6 +360,21 @@ __i915_gem_active_peek(const struct i915_gem_active *active)
> > }
> >
> > /**
> > + * i915_gem_active_raw - return the active request
> > + * @active - the active tracker
> > + *
> > + * i915_gem_active_raw() returns the current request being tracked, or NULL.
> > + * It does not obtain a reference on the request for the caller, so the caller
> > + * must hold struct_mutex.
> > + */
> > +static inline struct drm_i915_gem_request *
> > +i915_gem_active_raw(const struct i915_gem_active *active, struct mutex *mutex)
>
> There's a rcu_dereference_raw helper which doesn't bother with any lockdep
> checking. A bit confusing naming, otoh I couldn't come up with something
> better.
__i915_gem_active_protected() ?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper
2016-08-09 7:08 ` Chris Wilson
@ 2016-08-10 10:21 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2016-08-10 10:21 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, intel-gfx
On Tue, Aug 09, 2016 at 08:08:52AM +0100, Chris Wilson wrote:
> On Tue, Aug 09, 2016 at 08:36:56AM +0200, Daniel Vetter wrote:
> > On Mon, Aug 08, 2016 at 08:28:47AM +0100, Chris Wilson wrote:
> > > As we do the lockdep protected RCU lookup in a couple of places,
> > > refactor that code to a common helper i915_gem_active_raw()..
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > > drivers/gpu/drm/i915/i915_gem_request.h | 21 +++++++++++++++++----
> > > 1 file changed, 17 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> > > index 3496e28785e7..6dd01cbf4895 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_request.h
> > > +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> > > @@ -360,6 +360,21 @@ __i915_gem_active_peek(const struct i915_gem_active *active)
> > > }
> > >
> > > /**
> > > + * i915_gem_active_raw - return the active request
> > > + * @active - the active tracker
> > > + *
> > > + * i915_gem_active_raw() returns the current request being tracked, or NULL.
> > > + * It does not obtain a reference on the request for the caller, so the caller
> > > + * must hold struct_mutex.
> > > + */
> > > +static inline struct drm_i915_gem_request *
> > > +i915_gem_active_raw(const struct i915_gem_active *active, struct mutex *mutex)
> >
> > There's a rcu_dereference_raw helper which doesn't bother with any lockdep
> > checking. A bit confusing naming, otoh I couldn't come up with something
> > better.
>
> __i915_gem_active_protected() ?
+1
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang
2016-08-08 7:28 ` [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang Chris Wilson
2016-08-09 6:43 ` Daniel Vetter
@ 2016-08-12 10:20 ` Joonas Lahtinen
1 sibling, 0 replies; 8+ messages in thread
From: Joonas Lahtinen @ 2016-08-12 10:20 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On ma, 2016-08-08 at 08:28 +0100, Chris Wilson wrote:
> When we force the cleanup after a GPU hang, we want to retire all
> requests, or else we may leak them if truly wedged (and the GPU never
> advances again). Converting to the active request helpers had the issue
> of doing the check against busyness before reporting the request, so if
> we claim the GPU had hung but this engine hadn't we could potential skip
> the request cleanup - triggering the self-check BUG.
>
Could document the splat here.
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-08-12 10:20 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-08 7:28 [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper Chris Wilson
2016-08-08 7:28 ` [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang Chris Wilson
2016-08-09 6:43 ` Daniel Vetter
2016-08-12 10:20 ` Joonas Lahtinen
2016-08-08 7:56 ` ✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Wrap the protected active RCU dereference in a helper Patchwork
2016-08-09 6:36 ` [PATCH 1/2] " Daniel Vetter
2016-08-09 7:08 ` Chris Wilson
2016-08-10 10:21 ` Daniel Vetter
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.