[PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper

[PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper

[Chris Wilson]

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

[PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang

[Chris Wilson]

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

Re: [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang

[Daniel Vetter]

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

Re: [PATCH 2/2] drm/i915: Don't check for idleness before retiring after a GPU hang

[Joonas Lahtinen]

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

✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Wrap the protected active RCU dereference in a helper

[Patchwork]

== 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

Re: [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper

[Daniel Vetter]

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

Re: [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper

[Chris Wilson]

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

Re: [PATCH 1/2] drm/i915: Wrap the protected active RCU dereference in a helper

[Daniel Vetter]

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