| Message ID | 1401914722-17532-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 04, 2014 at 01:45:22PM -0700, Jesse Barnes wrote: > This allows the system to enter the lowest power mode during system freeze. > > v2: delete force wake timer at suspend (Imre) > v3: add GT work suspend function (Imre) > v4: use uncore forcewake reset (Daniel) > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Someone please look at https://bugs.freedesktop.org/show_bug.cgi?id=78059 Yes, I'm starting to block semi-related feature work on regressions because we suck that much. -Daniel > --- > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > 5 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 66c6ffb..7148eac 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev) > drm_irq_uninstall(dev); > dev_priv->enable_hotplug_processing = false; > > - intel_disable_gt_powersave(dev); > + intel_suspend_gt_powersave(dev); > > /* > * Disable CRTCs directly since we want to preserve sw state > @@ -542,8 +542,8 @@ static int i915_drm_freeze(struct drm_device *dev) > > i915_save_state(dev); > > + intel_uncore_forcewake_reset(dev, false); > intel_opregion_fini(dev); > - intel_uncore_fini(dev); > > console_lock(); > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index bea9ab40..89d6b47 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2084,6 +2084,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev); > extern void intel_uncore_init(struct drm_device *dev); > extern void intel_uncore_check_errors(struct drm_device *dev); > extern void intel_uncore_fini(struct drm_device *dev); > +extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore); > > void > i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index c597b0d..74fbe4d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -957,6 +957,7 @@ void intel_init_gt_powersave(struct drm_device *dev); > void intel_cleanup_gt_powersave(struct drm_device *dev); > void intel_enable_gt_powersave(struct drm_device *dev); > void intel_disable_gt_powersave(struct drm_device *dev); > +void intel_suspend_gt_powersave(struct drm_device *dev); > void intel_reset_gt_powersave(struct drm_device *dev); > void ironlake_teardown_rc6(struct drm_device *dev); > void gen6_update_ring_freq(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 1840d15..139eebe 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4864,6 +4864,26 @@ void intel_cleanup_gt_powersave(struct drm_device *dev) > valleyview_cleanup_gt_powersave(dev); > } > > +/** > + * intel_suspend_gt_powersave - suspend PM work and helper threads > + * @dev: drm device > + * > + * We don't want to disable RC6 or other features here, we just want > + * to make sure any work we've queued has finished and won't bother > + * us while we're suspended. > + */ > +void intel_suspend_gt_powersave(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + /* Interrupts should be disabled already to avoid re-arming. */ > + WARN_ON(dev->irq_enabled); > + > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > + cancel_work_sync(&dev_priv->rps.work); > +} > + > void intel_disable_gt_powersave(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 871c284..741a4e3 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -316,7 +316,7 @@ static void gen6_force_wake_timer(unsigned long arg) > intel_runtime_pm_put(dev_priv); > } > > -static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > +void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > { > struct drm_i915_private *dev_priv = dev->dev_private; > unsigned long irqflags; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 5 Jun 2014 11:21:02 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jun 04, 2014 at 01:45:22PM -0700, Jesse Barnes wrote: > > This allows the system to enter the lowest power mode during system freeze. > > > > v2: delete force wake timer at suspend (Imre) > > v3: add GT work suspend function (Imre) > > v4: use uncore forcewake reset (Daniel) > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > Someone please look at > > https://bugs.freedesktop.org/show_bug.cgi?id=78059 > > Yes, I'm starting to block semi-related feature work on regressions > because we suck that much. Chris said a fix was already available...
On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote: > This allows the system to enter the lowest power mode during system freeze. > > v2: delete force wake timer at suspend (Imre) > v3: add GT work suspend function (Imre) > v4: use uncore forcewake reset (Daniel) > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > 5 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 66c6ffb..7148eac 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev) > drm_irq_uninstall(dev); > dev_priv->enable_hotplug_processing = false; > > - intel_disable_gt_powersave(dev); > + intel_suspend_gt_powersave(dev); I realized now that we actually do need to enable RC6 explicitly. If we get here right after runtime resume, the deferred RC6 enabling might be still pending and so RC6 might not be enabled. --Imre > > /* > * Disable CRTCs directly since we want to preserve sw state > @@ -542,8 +542,8 @@ static int i915_drm_freeze(struct drm_device *dev) > > i915_save_state(dev); > > + intel_uncore_forcewake_reset(dev, false); > intel_opregion_fini(dev); > - intel_uncore_fini(dev); > > console_lock(); > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index bea9ab40..89d6b47 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2084,6 +2084,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev); > extern void intel_uncore_init(struct drm_device *dev); > extern void intel_uncore_check_errors(struct drm_device *dev); > extern void intel_uncore_fini(struct drm_device *dev); > +extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore); > > void > i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index c597b0d..74fbe4d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -957,6 +957,7 @@ void intel_init_gt_powersave(struct drm_device *dev); > void intel_cleanup_gt_powersave(struct drm_device *dev); > void intel_enable_gt_powersave(struct drm_device *dev); > void intel_disable_gt_powersave(struct drm_device *dev); > +void intel_suspend_gt_powersave(struct drm_device *dev); > void intel_reset_gt_powersave(struct drm_device *dev); > void ironlake_teardown_rc6(struct drm_device *dev); > void gen6_update_ring_freq(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 1840d15..139eebe 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4864,6 +4864,26 @@ void intel_cleanup_gt_powersave(struct drm_device *dev) > valleyview_cleanup_gt_powersave(dev); > } > > +/** > + * intel_suspend_gt_powersave - suspend PM work and helper threads > + * @dev: drm device > + * > + * We don't want to disable RC6 or other features here, we just want > + * to make sure any work we've queued has finished and won't bother > + * us while we're suspended. > + */ > +void intel_suspend_gt_powersave(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + /* Interrupts should be disabled already to avoid re-arming. */ > + WARN_ON(dev->irq_enabled); > + > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > + > + cancel_work_sync(&dev_priv->rps.work); > +} > + > void intel_disable_gt_powersave(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 871c284..741a4e3 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -316,7 +316,7 @@ static void gen6_force_wake_timer(unsigned long arg) > intel_runtime_pm_put(dev_priv); > } > > -static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > +void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > { > struct drm_i915_private *dev_priv = dev->dev_private; > unsigned long irqflags;
On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote: > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote: > > This allows the system to enter the lowest power mode during system freeze. > > > > v2: delete force wake timer at suspend (Imre) > > v3: add GT work suspend function (Imre) > > v4: use uncore forcewake reset (Daniel) > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 66c6ffb..7148eac 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev) > > drm_irq_uninstall(dev); > > dev_priv->enable_hotplug_processing = false; > > > > - intel_disable_gt_powersave(dev); > > + intel_suspend_gt_powersave(dev); > > I realized now that we actually do need to enable RC6 explicitly. If we > get here right after runtime resume, the deferred RC6 enabling might be > still pending and so RC6 might not be enabled. Hm, for runtime pm we might schedule the delayed rps work with a 0 delay, just to get the gpu going faster. Just an aside. Also some runtime pm vs system suspend tests in igt would be good if we don't have them yet. And if we have them some analysis why this didn't blow up in testing (and something to address that gap if feasible). -Daniel > > --Imre > > > > > /* > > * Disable CRTCs directly since we want to preserve sw state > > @@ -542,8 +542,8 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > i915_save_state(dev); > > > > + intel_uncore_forcewake_reset(dev, false); > > intel_opregion_fini(dev); > > - intel_uncore_fini(dev); > > > > console_lock(); > > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index bea9ab40..89d6b47 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2084,6 +2084,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev); > > extern void intel_uncore_init(struct drm_device *dev); > > extern void intel_uncore_check_errors(struct drm_device *dev); > > extern void intel_uncore_fini(struct drm_device *dev); > > +extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore); > > > > void > > i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe, > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index c597b0d..74fbe4d 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -957,6 +957,7 @@ void intel_init_gt_powersave(struct drm_device *dev); > > void intel_cleanup_gt_powersave(struct drm_device *dev); > > void intel_enable_gt_powersave(struct drm_device *dev); > > void intel_disable_gt_powersave(struct drm_device *dev); > > +void intel_suspend_gt_powersave(struct drm_device *dev); > > void intel_reset_gt_powersave(struct drm_device *dev); > > void ironlake_teardown_rc6(struct drm_device *dev); > > void gen6_update_ring_freq(struct drm_device *dev); > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 1840d15..139eebe 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4864,6 +4864,26 @@ void intel_cleanup_gt_powersave(struct drm_device *dev) > > valleyview_cleanup_gt_powersave(dev); > > } > > > > +/** > > + * intel_suspend_gt_powersave - suspend PM work and helper threads > > + * @dev: drm device > > + * > > + * We don't want to disable RC6 or other features here, we just want > > + * to make sure any work we've queued has finished and won't bother > > + * us while we're suspended. > > + */ > > +void intel_suspend_gt_powersave(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + > > + /* Interrupts should be disabled already to avoid re-arming. */ > > + WARN_ON(dev->irq_enabled); > > + > > + flush_delayed_work(&dev_priv->rps.delayed_resume_work); > > + > > + cancel_work_sync(&dev_priv->rps.work); > > +} > > + > > void intel_disable_gt_powersave(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > index 871c284..741a4e3 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -316,7 +316,7 @@ static void gen6_force_wake_timer(unsigned long arg) > > intel_runtime_pm_put(dev_priv); > > } > > > > -static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > > +void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > unsigned long irqflags; > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote: > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote: > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote: > > > This allows the system to enter the lowest power mode during system freeze. > > > > > > v2: delete force wake timer at suspend (Imre) > > > v3: add GT work suspend function (Imre) > > > v4: use uncore forcewake reset (Daniel) > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index 66c6ffb..7148eac 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev) > > > drm_irq_uninstall(dev); > > > dev_priv->enable_hotplug_processing = false; > > > > > > - intel_disable_gt_powersave(dev); > > > + intel_suspend_gt_powersave(dev); > > > > I realized now that we actually do need to enable RC6 explicitly. If we > > get here right after runtime resume, the deferred RC6 enabling might be > > still pending and so RC6 might not be enabled. > > Hm, for runtime pm we might schedule the delayed rps work with a 0 delay, > just to get the gpu going faster. Just an aside. > > Also some runtime pm vs system suspend tests in igt would be good if we > don't have them yet. And if we have them some analysis why this didn't > blow up in testing (and something to address that gap if feasible). To clarify the above is about the new functionality to enable deeper system wide power states. Atm, we don't have a bug related to the above runtime resume->system suspend sequence, since we explicitly disable gt power saving/RC6. For deeper power states we have to do the opposite and leave RC6 enabled and that's what I commented on. --Imre
On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote: > On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote: > > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote: > > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote: > > > > This allows the system to enter the lowest power mode during system freeze. > > > > > > > > v2: delete force wake timer at suspend (Imre) > > > > v3: add GT work suspend function (Imre) > > > > v4: use uncore forcewake reset (Daniel) > > > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++++ > > > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index 66c6ffb..7148eac 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > drm_irq_uninstall(dev); > > > > dev_priv->enable_hotplug_processing = false; > > > > > > > > - intel_disable_gt_powersave(dev); > > > > + intel_suspend_gt_powersave(dev); > > > > > > I realized now that we actually do need to enable RC6 explicitly. If we > > > get here right after runtime resume, the deferred RC6 enabling might be > > > still pending and so RC6 might not be enabled. > > > > Hm, for runtime pm we might schedule the delayed rps work with a 0 delay, > > just to get the gpu going faster. Just an aside. > > > > Also some runtime pm vs system suspend tests in igt would be good if we > > don't have them yet. And if we have them some analysis why this didn't > > blow up in testing (and something to address that gap if feasible). > > To clarify the above is about the new functionality to enable deeper > system wide power states. Atm, we don't have a bug related to the above > runtime resume->system suspend sequence, since we explicitly disable gt > power saving/RC6. For deeper power states we have to do the opposite and > leave RC6 enabled and that's what I commented on. Well I just want to make sure that we have test coverage. If Jesse has run all the rpm tests with piglit run -t rpm and it didn't blow up we need to fix this gap. -Daniel
On Tue, 10 Jun 2014 17:26:45 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote: > > On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote: > > > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote: > > > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote: > > > > > This allows the system to enter the lowest power mode during system freeze. > > > > > > > > > > v2: delete force wake timer at suspend (Imre) > > > > > v3: add GT work suspend function (Imre) > > > > > v4: use uncore forcewake reset (Daniel) > > > > > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++++ > > > > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > > > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > > index 66c6ffb..7148eac 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > drm_irq_uninstall(dev); > > > > > dev_priv->enable_hotplug_processing = false; > > > > > > > > > > - intel_disable_gt_powersave(dev); > > > > > + intel_suspend_gt_powersave(dev); > > > > > > > > I realized now that we actually do need to enable RC6 explicitly. If we > > > > get here right after runtime resume, the deferred RC6 enabling might be > > > > still pending and so RC6 might not be enabled. Seems like we could just flush the enable if it was outstanding? > > > Hm, for runtime pm we might schedule the delayed rps work with a 0 delay, > > > just to get the gpu going faster. Just an aside. > > > > > > Also some runtime pm vs system suspend tests in igt would be good if we > > > don't have them yet. And if we have them some analysis why this didn't > > > blow up in testing (and something to address that gap if feasible). > > > > To clarify the above is about the new functionality to enable deeper > > system wide power states. Atm, we don't have a bug related to the above > > runtime resume->system suspend sequence, since we explicitly disable gt > > power saving/RC6. For deeper power states we have to do the opposite and > > leave RC6 enabled and that's what I commented on. > > Well I just want to make sure that we have test coverage. If Jesse has run > all the rpm tests with piglit run -t rpm and it didn't blow up we need to > fix this gap. I have not... I'm not sure how to test this though, as I think we'd need to measure c states or power when we do a suspend. If we checked for RC6 enable at resume, we'd probably race with the enable work queue. Either way, we should add some asserts to the suspend path to make sure all the conditions we need for minimum power are met.
On Wed, 11 Jun 2014 15:21:16 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Tue, 10 Jun 2014 17:26:45 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote: > > > On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote: > > > > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote: > > > > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote: > > > > > > This allows the system to enter the lowest power mode during system freeze. > > > > > > > > > > > > v2: delete force wake timer at suspend (Imre) > > > > > > v3: add GT work suspend function (Imre) > > > > > > v4: use uncore forcewake reset (Daniel) > > > > > > > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > > > --- > > > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > > > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++++ > > > > > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > > > > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > > > index 66c6ffb..7148eac 100644 > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > drm_irq_uninstall(dev); > > > > > > dev_priv->enable_hotplug_processing = false; > > > > > > > > > > > > - intel_disable_gt_powersave(dev); > > > > > > + intel_suspend_gt_powersave(dev); > > > > > > > > > > I realized now that we actually do need to enable RC6 explicitly. If we > > > > > get here right after runtime resume, the deferred RC6 enabling might be > > > > > still pending and so RC6 might not be enabled. > > Seems like we could just flush the enable if it was outstanding? Oh and I see we already have the flush in v4... do you think we might not actually arm the deferred enable work before we get here? I think the runtime_resume and suspend calls should be serialized, but if not we'd need some locking I guess... Jesse
On Wed, 2014-06-11 at 15:24 -0700, Jesse Barnes wrote: > On Wed, 11 Jun 2014 15:21:16 -0700 > Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > On Tue, 10 Jun 2014 17:26:45 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote: > > > > On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote: > > > > > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote: > > > > > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote: > > > > > > > This allows the system to enter the lowest power mode during system freeze. > > > > > > > > > > > > > > v2: delete force wake timer at suspend (Imre) > > > > > > > v3: add GT work suspend function (Imre) > > > > > > > v4: use uncore forcewake reset (Daniel) > > > > > > > > > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++-- > > > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++++++ > > > > > > > drivers/gpu/drm/i915/intel_uncore.c | 2 +- > > > > > > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > > > > index 66c6ffb..7148eac 100644 > > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > drm_irq_uninstall(dev); > > > > > > > dev_priv->enable_hotplug_processing = false; > > > > > > > > > > > > > > - intel_disable_gt_powersave(dev); > > > > > > > + intel_suspend_gt_powersave(dev); > > > > > > > > > > > > I realized now that we actually do need to enable RC6 explicitly. If we > > > > > > get here right after runtime resume, the deferred RC6 enabling might be > > > > > > still pending and so RC6 might not be enabled. > > > > Seems like we could just flush the enable if it was outstanding? > > Oh and I see we already have the flush in v4... Right, I was just blind and I guess also thinking in terms of timers that don't have a similar flush API. Anyway this is fine and my comment about it above can be ignored. > do you think we might not actually arm the deferred enable work before > we get here? I haven't thought about this. But now that you mentioned I looked into the device and dpm core and can't see what would prevent our system suspend handler to be called while we are still running our driver load function. But this is a separate issue (if true at all) that exists regardless of this patch and also not a really real life scenario. So I suggest we investigate this as a follow-up. > I think the runtime_resume and suspend calls should be serialized, but > if not we'd need some locking I guess... Yes, this is taken care of the RPM framework. Similarly system suspend/resume handlers are serialized wrt. the runtime suspend/resume handlers. This patch is: Reviewed-by: Imre Deak <imre.deak@intel.com> Btw, now all 4 patches in the patchset are reviewed.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 66c6ffb..7148eac 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev) drm_irq_uninstall(dev); dev_priv->enable_hotplug_processing = false; - intel_disable_gt_powersave(dev); + intel_suspend_gt_powersave(dev); /* * Disable CRTCs directly since we want to preserve sw state @@ -542,8 +542,8 @@ static int i915_drm_freeze(struct drm_device *dev) i915_save_state(dev); + intel_uncore_forcewake_reset(dev, false); intel_opregion_fini(dev); - intel_uncore_fini(dev); console_lock(); intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index bea9ab40..89d6b47 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2084,6 +2084,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev); extern void intel_uncore_init(struct drm_device *dev); extern void intel_uncore_check_errors(struct drm_device *dev); extern void intel_uncore_fini(struct drm_device *dev); +extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore); void i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c597b0d..74fbe4d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -957,6 +957,7 @@ void intel_init_gt_powersave(struct drm_device *dev); void intel_cleanup_gt_powersave(struct drm_device *dev); void intel_enable_gt_powersave(struct drm_device *dev); void intel_disable_gt_powersave(struct drm_device *dev); +void intel_suspend_gt_powersave(struct drm_device *dev); void intel_reset_gt_powersave(struct drm_device *dev); void ironlake_teardown_rc6(struct drm_device *dev); void gen6_update_ring_freq(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 1840d15..139eebe 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4864,6 +4864,26 @@ void intel_cleanup_gt_powersave(struct drm_device *dev) valleyview_cleanup_gt_powersave(dev); } +/** + * intel_suspend_gt_powersave - suspend PM work and helper threads + * @dev: drm device + * + * We don't want to disable RC6 or other features here, we just want + * to make sure any work we've queued has finished and won't bother + * us while we're suspended. + */ +void intel_suspend_gt_powersave(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + /* Interrupts should be disabled already to avoid re-arming. */ + WARN_ON(dev->irq_enabled); + + flush_delayed_work(&dev_priv->rps.delayed_resume_work); + + cancel_work_sync(&dev_priv->rps.work); +} + void intel_disable_gt_powersave(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 871c284..741a4e3 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -316,7 +316,7 @@ static void gen6_force_wake_timer(unsigned long arg) intel_runtime_pm_put(dev_priv); } -static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) +void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) { struct drm_i915_private *dev_priv = dev->dev_private; unsigned long irqflags;