Re: [PATCH] drm/i915: Restore user forcewake domains across suspend

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Tvrtko Ursulin (2018-08-09 12:46:05)
> 
> On 08/08/2018 22:08, Chris Wilson wrote:
> > On suspend, we cancel the automatic forcewake and clear all other sources
> > of forcewake so the machine can sleep before we do suspend. However, we
> > expose the forcewake to userspace (only via debugfs, but nevertheless we
> > do) and want to restore that upon resume or else our accounting will be
> > off and we may not acquire the forcewake before we use it. So record
> > which domains we cleared on suspend and reacquire them early on resume.
> > 
> > v2: Hold the spinlock to appease our sanitychecks
> > 
> > Reported-by: Imre Deak <imre.deak@xxxxxxxxxxxxxxx>
> > Fixes: b8473050805f ("drm/i915: Fix forcewake active domain tracking")
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> > Cc: Imre Deak <imre.deak@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/intel_uncore.c           | 44 ++++++++++---------
> >   drivers/gpu/drm/i915/intel_uncore.h           |  1 +
> >   drivers/gpu/drm/i915/selftests/intel_uncore.c |  2 +-
> >   3 files changed, 26 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 284be151f645..cf40361fe181 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -369,8 +369,8 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
> >   }
> >   
> >   /* Note callers must have acquired the PUNIT->PMIC bus, before calling this. */
> > -static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
> > -                                      bool restore)
> > +static unsigned int
> > +intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv)
> >   {
> >       unsigned long irqflags;
> >       struct intel_uncore_forcewake_domain *domain;
> > @@ -422,20 +422,11 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
> >               dev_priv->uncore.funcs.force_wake_put(dev_priv, fw);
> >   
> >       fw_domains_reset(dev_priv, dev_priv->uncore.fw_domains);
> > -
> > -     if (restore) { /* If reset with a user forcewake, try to restore */
> > -             if (fw)
> > -                     dev_priv->uncore.funcs.force_wake_get(dev_priv, fw);
> > -
> > -             if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv))
> > -                     dev_priv->uncore.fifo_count =
> > -                             fifo_free_entries(dev_priv);
> > -     }
> > -
> > -     if (!restore)
> > -             assert_forcewakes_inactive(dev_priv);
> > +     assert_forcewakes_inactive(dev_priv);
> >   
> >       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > +
> > +     return fw; /* track the lost user forcewake domains */
> >   }
> >   
> >   static u64 gen9_edram_size(struct drm_i915_private *dev_priv)
> > @@ -544,7 +535,7 @@ check_for_unclaimed_mmio(struct drm_i915_private *dev_priv)
> >   }
> >   
> >   static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
> > -                                       bool restore_forcewake)
> > +                                       unsigned int restore_forcewake)
> >   {
> >       /* clear out unclaimed reg detection bit */
> >       if (check_for_unclaimed_mmio(dev_priv))
> > @@ -559,7 +550,17 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
> >       }
> >   
> >       iosf_mbi_punit_acquire();
> > -     intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
> > +     intel_uncore_forcewake_reset(dev_priv);
> > +     if (restore_forcewake) {
> > +             spin_lock_irq(&dev_priv->uncore.lock);
> > +             dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > +                                                   restore_forcewake);
> > +
> > +             if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv))
> > +                     dev_priv->uncore.fifo_count =
> > +                             fifo_free_entries(dev_priv);
> > +             spin_unlock_irq(&dev_priv->uncore.lock);
> > +     }
> >       iosf_mbi_punit_release();
> >   }
> >   
> > @@ -568,13 +569,16 @@ void intel_uncore_suspend(struct drm_i915_private *dev_priv)
> >       iosf_mbi_punit_acquire();
> >       iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
> >               &dev_priv->uncore.pmic_bus_access_nb);
> > -     intel_uncore_forcewake_reset(dev_priv, false);
> > +     dev_priv->uncore.fw_domains_user =
> > +             intel_uncore_forcewake_reset(dev_priv);
> >       iosf_mbi_punit_release();
> >   }
> >   
> >   void intel_uncore_resume_early(struct drm_i915_private *dev_priv)
> >   {
> > -     __intel_uncore_early_sanitize(dev_priv, true);
> > +     __intel_uncore_early_sanitize(dev_priv,
> > +                                   dev_priv->uncore.fw_domains_user);
> > +
> >       iosf_mbi_register_pmic_bus_access_notifier(
> >               &dev_priv->uncore.pmic_bus_access_nb);
> >       i915_check_and_clear_faults(dev_priv);
> > @@ -1555,7 +1559,7 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
> >   
> >       intel_uncore_edram_detect(dev_priv);
> >       intel_uncore_fw_domains_init(dev_priv);
> > -     __intel_uncore_early_sanitize(dev_priv, false);
> > +     __intel_uncore_early_sanitize(dev_priv, 0);
> 
> I wonder if a tweak to not have a parameter but function acts on 
> uncore.fw_domains_user on it's own, and consumes it after restoring form 
> it. But it is just a different tweak on the same theme so never mind.
> 
> >   
> >       dev_priv->uncore.unclaimed_mmio_check = 1;
> >       dev_priv->uncore.pmic_bus_access_nb.notifier_call =
> > @@ -1642,7 +1646,7 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv)
> >       iosf_mbi_punit_acquire();
> >       iosf_mbi_unregister_pmic_bus_access_notifier_unlocked(
> >               &dev_priv->uncore.pmic_bus_access_nb);
> > -     intel_uncore_forcewake_reset(dev_priv, false);
> > +     intel_uncore_forcewake_reset(dev_priv);
> >       iosf_mbi_punit_release();
> >   }
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> > index 2fbe93178fb2..137d8ac856fe 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.h
> > +++ b/drivers/gpu/drm/i915/intel_uncore.h
> > @@ -104,6 +104,7 @@ struct intel_uncore {
> >   
> >       enum forcewake_domains fw_domains;
> >       enum forcewake_domains fw_domains_active;
> > +     enum forcewake_domains fw_domains_user;
> 
> Maybe call it fw_domains_saved so it is obvious it is special since user 
> domains are not always tracked in it.

Tweaked s/user/saved/ and gave it a short comment.

Thanks for reporting the issue Imre, and for both of you in reviewing
it.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux