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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-01-02 13:19:52)
>> Chris Wilson <chris@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@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.

Ok. And the GEN5+ it will initialized to false.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx