| 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 |
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
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
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 --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)
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(-)