diff mbox series

[v2] drm/i915/gt: Allow clobbering gpu resets if the display is off

Message ID 20190911101959.13793-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [v2] drm/i915/gt: Allow clobbering gpu resets if the display is off | expand

Commit Message

Chris Wilson Sept. 11, 2019, 10:19 a.m. UTC
If the display is inactive, we need not worry about the gpu reset
clobbering the display! To prevent the display changing state between us
checking the active state and doing the hard reset, we introduce the
lightweight reset lock to the atomic commit for the affected (legacy)
platforms.

Testcase: igt/gem_eio/kms
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 11 +++++++++++
 drivers/gpu/drm/i915/gt/intel_reset.c        | 18 +++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

Comments

Chris Wilson Sept. 11, 2019, 10:48 a.m. UTC | #1
Quoting Chris Wilson (2019-09-11 11:19:59)
> If the display is inactive, we need not worry about the gpu reset
> clobbering the display! To prevent the display changing state between us
> checking the active state and doing the hard reset, we introduce the
> lightweight reset lock to the atomic commit for the affected (legacy)
> platforms.

> +       /* Prevent starting a GPU reset while we change global display state */
> +       srcu = -ENODEV;
> +       if (INTEL_INFO(dev_priv)->gpu_reset_clobbers_display)
> +               /* only fails if interrupted */
> +               srcu = intel_gt_reset_trylock(&dev_priv->gt);

This is overkill? Before starting the GPU reset, we serialise with the
modeset, do we not?
-Chris
Ville Syrjälä Sept. 11, 2019, 10:52 a.m. UTC | #2
On Wed, Sep 11, 2019 at 11:19:59AM +0100, Chris Wilson wrote:
> If the display is inactive, we need not worry about the gpu reset
> clobbering the display! To prevent the display changing state between us
> checking the active state and doing the hard reset, we introduce the
> lightweight reset lock to the atomic commit for the affected (legacy)
> platforms.
> 
> Testcase: igt/gem_eio/kms
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 11 +++++++++++
>  drivers/gpu/drm/i915/gt/intel_reset.c        | 18 +++++++++++++++++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 4ee750fa3ef0..a92487d8f4cf 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13986,6 +13986,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	struct intel_crtc *crtc;
>  	u64 put_domains[I915_MAX_PIPES] = {};
>  	intel_wakeref_t wakeref = 0;
> +	int srcu;
>  	int i;
>  
>  	intel_atomic_commit_fence_wait(state);
> @@ -14005,6 +14006,12 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  		}
>  	}
>  
> +	/* Prevent starting a GPU reset while we change global display state */
> +	srcu = -ENODEV;
> +	if (INTEL_INFO(dev_priv)->gpu_reset_clobbers_display)
> +		/* only fails if interrupted */
> +		srcu = intel_gt_reset_trylock(&dev_priv->gt);

Hmm. What happens if we're holding the modeset locks and there's an
ongoing reset blocked on said locks?

> +
>  	intel_commit_modeset_disables(state);
>  
>  	/* FIXME: Eventually get rid of our crtc->config pointer */
> @@ -14049,6 +14056,10 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	dev_priv->display.commit_modeset_enables(state);
>  
> +
> +	if (srcu > 0)
> +		intel_gt_reset_unlock(&dev_priv->gt, srcu);
> +
>  	if (state->modeset) {
>  		intel_encoders_update_complete(state);
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index d8b1498d7ab7..df4a86bdb6f6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -7,6 +7,7 @@
>  #include <linux/sched/mm.h>
>  #include <linux/stop_machine.h>
>  
> +#include "display/intel_display.h"
>  #include "display/intel_display_types.h"
>  #include "display/intel_overlay.h"
>  
> @@ -729,6 +730,21 @@ static void nop_submit_request(struct i915_request *request)
>  	intel_engine_queue_breadcrumbs(engine);
>  }
>  
> +static bool reset_clobbers_display(struct drm_i915_private *i915)
> +{
> +	struct intel_crtc *crtc;
> +
> +	if (!INTEL_INFO(i915)->gpu_reset_clobbers_display)
> +		return false;
> +
> +	for_each_intel_crtc(&i915->drm, crtc) {
> +		if (crtc->active)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static void __intel_gt_set_wedged(struct intel_gt *gt)
>  {
>  	struct intel_engine_cs *engine;
> @@ -755,7 +771,7 @@ static void __intel_gt_set_wedged(struct intel_gt *gt)
>  	awake = reset_prepare(gt);
>  
>  	/* Even if the GPU reset fails, it should still stop the engines */
> -	if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
> +	if (!reset_clobbers_display(gt->i915))
>  		__intel_gt_reset(gt, ALL_ENGINES);
>  
>  	for_each_engine(engine, gt->i915, id)
> -- 
> 2.23.0
Chris Wilson Sept. 11, 2019, 11:02 a.m. UTC | #3
Quoting Ville Syrjälä (2019-09-11 11:52:29)
> On Wed, Sep 11, 2019 at 11:19:59AM +0100, Chris Wilson wrote:
> > If the display is inactive, we need not worry about the gpu reset
> > clobbering the display! To prevent the display changing state between us
> > checking the active state and doing the hard reset, we introduce the
> > lightweight reset lock to the atomic commit for the affected (legacy)
> > platforms.
> > 
> > Testcase: igt/gem_eio/kms
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 11 +++++++++++
> >  drivers/gpu/drm/i915/gt/intel_reset.c        | 18 +++++++++++++++++-
> >  2 files changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 4ee750fa3ef0..a92487d8f4cf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13986,6 +13986,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> >       struct intel_crtc *crtc;
> >       u64 put_domains[I915_MAX_PIPES] = {};
> >       intel_wakeref_t wakeref = 0;
> > +     int srcu;
> >       int i;
> >  
> >       intel_atomic_commit_fence_wait(state);
> > @@ -14005,6 +14006,12 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> >               }
> >       }
> >  
> > +     /* Prevent starting a GPU reset while we change global display state */
> > +     srcu = -ENODEV;
> > +     if (INTEL_INFO(dev_priv)->gpu_reset_clobbers_display)
> > +             /* only fails if interrupted */
> > +             srcu = intel_gt_reset_trylock(&dev_priv->gt);
> 
> Hmm. What happens if we're holding the modeset locks and there's an
> ongoing reset blocked on said locks?

I was about to say it's the inner lock in both cases, but it's not.
Anyway doesn't that interaction (intel_prepare_reset) prevent
crtc->active changing as we use it from inside intel_gt_reset (which
calls intel_gt_unset_wedged)?
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 4ee750fa3ef0..a92487d8f4cf 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13986,6 +13986,7 @@  static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	struct intel_crtc *crtc;
 	u64 put_domains[I915_MAX_PIPES] = {};
 	intel_wakeref_t wakeref = 0;
+	int srcu;
 	int i;
 
 	intel_atomic_commit_fence_wait(state);
@@ -14005,6 +14006,12 @@  static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 		}
 	}
 
+	/* Prevent starting a GPU reset while we change global display state */
+	srcu = -ENODEV;
+	if (INTEL_INFO(dev_priv)->gpu_reset_clobbers_display)
+		/* only fails if interrupted */
+		srcu = intel_gt_reset_trylock(&dev_priv->gt);
+
 	intel_commit_modeset_disables(state);
 
 	/* FIXME: Eventually get rid of our crtc->config pointer */
@@ -14049,6 +14056,10 @@  static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	dev_priv->display.commit_modeset_enables(state);
 
+
+	if (srcu > 0)
+		intel_gt_reset_unlock(&dev_priv->gt, srcu);
+
 	if (state->modeset) {
 		intel_encoders_update_complete(state);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index d8b1498d7ab7..df4a86bdb6f6 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -7,6 +7,7 @@ 
 #include <linux/sched/mm.h>
 #include <linux/stop_machine.h>
 
+#include "display/intel_display.h"
 #include "display/intel_display_types.h"
 #include "display/intel_overlay.h"
 
@@ -729,6 +730,21 @@  static void nop_submit_request(struct i915_request *request)
 	intel_engine_queue_breadcrumbs(engine);
 }
 
+static bool reset_clobbers_display(struct drm_i915_private *i915)
+{
+	struct intel_crtc *crtc;
+
+	if (!INTEL_INFO(i915)->gpu_reset_clobbers_display)
+		return false;
+
+	for_each_intel_crtc(&i915->drm, crtc) {
+		if (crtc->active)
+			return true;
+	}
+
+	return false;
+}
+
 static void __intel_gt_set_wedged(struct intel_gt *gt)
 {
 	struct intel_engine_cs *engine;
@@ -755,7 +771,7 @@  static void __intel_gt_set_wedged(struct intel_gt *gt)
 	awake = reset_prepare(gt);
 
 	/* Even if the GPU reset fails, it should still stop the engines */
-	if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
+	if (!reset_clobbers_display(gt->i915))
 		__intel_gt_reset(gt, ALL_ENGINES);
 
 	for_each_engine(engine, gt->i915, id)