[Intel-gfx] [PATCH 06/39] drm/i915: Always try to reset the GPU on takeover

[Chris Wilson]

Quoting Mika Kuoppala (2019-01-02 13:19:52)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > When we first introduced the reset to sanitize the GPU on taking over
> > from the BIOS and before returning control to third parties (the BIOS!),
> > we restricted it to only systems utilizing HW contexts as we were
> > uncertain of how stable our reset mechanism truly was. We now have
> > reasonable coverage across all machines that expose a GPU reset method,
> > and so we should be safe to sanitize the GPU state everywhere.
> >
> > v2: We _have_ to skip the reset if it would clobber the display.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c           |  2 +-
> >  drivers/gpu/drm/i915/i915_gem.c           | 11 ++---------
> >  drivers/gpu/drm/i915/i915_pci.c           |  5 +++++
> >  drivers/gpu/drm/i915/intel_device_info.h  |  1 +
> >  drivers/gpu/drm/i915/intel_display.c      |  4 ++--
> >  drivers/gpu/drm/i915/intel_engine_cs.c    | 14 +++++++++++++-
> >  drivers/gpu/drm/i915/intel_ringbuffer.h   |  2 +-
> >  drivers/gpu/drm/i915/selftests/i915_gem.c |  2 +-
> >  8 files changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index dcb935338c63..a96169acdb07 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -2174,7 +2174,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
> >  
> >       intel_power_domains_resume(dev_priv);
> >  
> > -     intel_engines_sanitize(dev_priv);
> > +     intel_engines_sanitize(dev_priv, true);
> >  
> >       enable_rpm_wakeref_asserts(dev_priv);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index e46a25747ab7..0ebde13620cb 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3432,8 +3432,7 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
> >       i915_retire_requests(i915);
> >       GEM_BUG_ON(i915->gt.active_requests);
> >  
> > -     if (!intel_gpu_reset(i915, ALL_ENGINES))
> > -             intel_engines_sanitize(i915);
> > +     intel_engines_sanitize(i915, false);
> >  
> >       /*
> >        * Undo nop_submit_request. We prevent all new i915 requests from
> > @@ -5037,8 +5036,6 @@ void __i915_gem_object_release_unless_active(struct drm_i915_gem_object *obj)
> >  
> >  void i915_gem_sanitize(struct drm_i915_private *i915)
> >  {
> > -     int err;
> > -
> >       GEM_TRACE("\n");
> >  
> >       mutex_lock(&i915->drm.struct_mutex);
> > @@ -5063,11 +5060,7 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
> >        * it may impact the display and we are uncertain about the stability
> >        * of the reset, so this could be applied to even earlier gen.
> >        */
> > -     err = -ENODEV;
> > -     if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
> > -             err = WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));
> > -     if (!err)
> > -             intel_engines_sanitize(i915);
> > +     intel_engines_sanitize(i915, false);
> >  
> >       intel_uncore_forcewake_put(i915, FORCEWAKE_ALL);
> >       intel_runtime_pm_put(i915);
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index 0d342f2b44a5..dd4aff2b256e 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -82,6 +82,7 @@
> >       .display.has_overlay = 1, \
> >       .display.overlay_needs_physical = 1, \
> >       .display.has_gmch_display = 1, \
> > +     .gpu_reset_clobbers_display = true, \
> >       .hws_needs_physical = 1, \
> >       .unfenced_needs_alignment = 1, \
> >       .ring_mask = RENDER_RING, \
> > @@ -122,6 +123,7 @@ static const struct intel_device_info intel_i865g_info = {
> >       GEN(3), \
> >       .num_pipes = 2, \
> >       .display.has_gmch_display = 1, \
> > +     .gpu_reset_clobbers_display = true, \
> >       .ring_mask = RENDER_RING, \
> >       .has_snoop = true, \
> >       .has_coherent_ggtt = true, \
> > @@ -198,6 +200,7 @@ static const struct intel_device_info intel_pineview_info = {
> >       .num_pipes = 2, \
> >       .display.has_hotplug = 1, \
> >       .display.has_gmch_display = 1, \
> > +     .gpu_reset_clobbers_display = true, \
> >       .ring_mask = RENDER_RING, \
> >       .has_snoop = true, \
> >       .has_coherent_ggtt = true, \
> > @@ -228,6 +231,7 @@ static const struct intel_device_info intel_g45_info = {
> >       GEN4_FEATURES,
> >       PLATFORM(INTEL_G45),
> >       .ring_mask = RENDER_RING | BSD_RING,
> > +     .gpu_reset_clobbers_display = false,
> >  };
> >  
> >  static const struct intel_device_info intel_gm45_info = {
> > @@ -237,6 +241,7 @@ static const struct intel_device_info intel_gm45_info = {
> >       .display.has_fbc = 1,
> >       .display.supports_tv = 1,
> >       .ring_mask = RENDER_RING | BSD_RING,
> > +     .gpu_reset_clobbers_display = false,
> 
> Why not explicitly set this on rest of gen >= 5?

This was to override the .gpu_reset_clobbers_display = true pulled in
from GEN4_FEATURES. Despite appearances to the alternative, we are
setting GEN2_FEATURES, GEN3_FEATURES and GEN4_FEATURES.
-Chris