| Message ID | 1401397897-4655-2-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote: > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > This allows the system to enter the lowest power mode during system freeze. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.c | 3 --- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 66c6ffb..433bdfa 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev) > drm_irq_uninstall(dev); > dev_priv->enable_hotplug_processing = false; > > - intel_disable_gt_powersave(dev); > - I wonder what was the reason for this call. One possibility is that i915_save_state() depends on it to save the correct registers, but it would be good to clarify this. It also cancels some deferred works which we do need here. But we could also add that to intel_enable_gt_powersave_sync() in this patch. > /* > * Disable CRTCs directly since we want to preserve sw state > * for _thaw. > @@ -543,7 +541,6 @@ static int i915_drm_freeze(struct drm_device *dev) > i915_save_state(dev); > > intel_opregion_fini(dev); > - intel_uncore_fini(dev); Some stuff in the above call is unrelated to this patch, like intel_uncore_forcewake_reset(). At least canceling force_wake_timer there seems to be needed here. In any case it would be better to have the above two changes in a separate patch. With that fixed this patch looks ok to me. The original patch was from me, so fwiw: Reviewed-by: Imre Deak <imre.deak@intel.com> > console_lock(); > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index c597b0d..bf90e7d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -956,6 +956,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv); > 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_enable_gt_powersave_sync(struct drm_device *dev); > void intel_disable_gt_powersave(struct drm_device *dev); > void intel_reset_gt_powersave(struct drm_device *dev); > void ironlake_teardown_rc6(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 1840d15..8d9e036 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4891,12 +4891,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) > } > } > > -static void intel_gen6_powersave_work(struct work_struct *work) > +void intel_enable_gt_powersave_sync(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = > - container_of(work, struct drm_i915_private, > - rps.delayed_resume_work.work); > - struct drm_device *dev = dev_priv->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > > mutex_lock(&dev_priv->rps.hw_lock); > > @@ -4917,6 +4914,15 @@ static void intel_gen6_powersave_work(struct work_struct *work) > intel_runtime_pm_put(dev_priv); > } > > +static void intel_gen6_powersave_work(struct work_struct *work) > +{ > + struct drm_i915_private *dev_priv = > + container_of(work, struct drm_i915_private, > + rps.delayed_resume_work.work); > + > + intel_enable_gt_powersave_sync(dev_priv->dev); > +} > + > void intel_enable_gt_powersave(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private;
On Fri, 30 May 2014 15:54:37 +0300 Imre Deak <imre.deak@intel.com> wrote: > On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote: > > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > This allows the system to enter the lowest power mode during system freeze. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 3 --- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 66c6ffb..433bdfa 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev) > > drm_irq_uninstall(dev); > > dev_priv->enable_hotplug_processing = false; > > > > - intel_disable_gt_powersave(dev); > > - > > I wonder what was the reason for this call. One possibility is that > i915_save_state() depends on it to save the correct registers, but it > would be good to clarify this. > > It also cancels some deferred works which we do need here. But we could > also add that to intel_enable_gt_powersave_sync() in this patch. Yeah I was worried about that too, but then we do the reset on resume anyway, and I didn't see anything in my logs in testing... But I can split that out if there's a reason to. Seems like we do a bit too much teardown at suspend these days (like tearing down opregion state), I'd like to trim it back if possible and share between runtime and system suspend/freeze. I'll look into the forcewake bits. Thanks,
On Fri, May 30, 2014 at 08:32:20AM -0700, Jesse Barnes wrote: > But I can split that out if there's a reason to. Seems like we do a > bit too much teardown at suspend these days (like tearing down opregion > state), I'd like to trim it back if possible and share between runtime > and system suspend/freeze. I guess the answer is to move all the paranoid bits from suspend into hibernate-resume. -Chris
On Fri, 30 May 2014 16:40:27 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, May 30, 2014 at 08:32:20AM -0700, Jesse Barnes wrote: > > But I can split that out if there's a reason to. Seems like we do a > > bit too much teardown at suspend these days (like tearing down opregion > > state), I'd like to trim it back if possible and share between runtime > > and system suspend/freeze. > > I guess the answer is to move all the paranoid bits from suspend into > hibernate-resume. Well and probably move a bunch of stuff out of _freeze and into a shared _suspend function for use at runtime suspend, suspend, and hibernate. Separate patches for all of that though so all the breakage is easily bisected. :)
On Thu, 29 May 2014 14:11:35 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > From: Kristen Carlson Accardi <kristen@linux.intel.com> Imre is the author. > > This allows the system to enter the lowest power mode during system freeze. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.c | 3 --- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 66c6ffb..433bdfa 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev) > drm_irq_uninstall(dev); > dev_priv->enable_hotplug_processing = false;Linux-3.16 > > - intel_disable_gt_powersave(dev); > - > /* > * Disable CRTCs directly since we want to preserve sw state > * for _thaw. > @@ -543,7 +541,6 @@ static int i915_drm_freeze(struct drm_device *dev) > i915_save_state(dev); > > 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/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index c597b0d..bf90e7d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -956,6 +956,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv); > 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_enable_gt_powersave_sync(struct drm_device *dev); > void intel_disable_gt_powersave(struct drm_device *dev); > void intel_reset_gt_powersave(struct drm_device *dev); > void ironlake_teardown_rc6(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 1840d15..8d9e036 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4891,12 +4891,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) > } > } > > -static void intel_gen6_powersave_work(struct work_struct *work) > +void intel_enable_gt_powersave_sync(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = > - container_of(work, struct drm_i915_private, > - rps.delayed_resume_work.work); > - struct drm_device *dev = dev_priv->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > > mutex_lock(&dev_priv->rps.hw_lock); > > @@ -4917,6 +4914,15 @@ static void intel_gen6_powersave_work(struct work_struct *work) > intel_runtime_pm_put(dev_priv); > } > > +static void intel_gen6_powersave_work(struct work_struct *work) > +{ > + struct drm_i915_private *dev_priv = > + container_of(work, struct drm_i915_private, > + rps.delayed_resume_work.work); > + > + intel_enable_gt_powersave_sync(dev_priv->dev); > +} > + > void intel_enable_gt_powersave(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private;
On Fri, May 30, 2014 at 08:32:20AM -0700, Jesse Barnes wrote: > On Fri, 30 May 2014 15:54:37 +0300 > Imre Deak <imre.deak@intel.com> wrote: > > > On Thu, 2014-05-29 at 14:11 -0700, Jesse Barnes wrote: > > > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > > > This allows the system to enter the lowest power mode during system freeze. > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 3 --- > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_pm.c | 16 +++++++++++----- > > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index 66c6ffb..433bdfa 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev) > > > drm_irq_uninstall(dev); > > > dev_priv->enable_hotplug_processing = false; > > > > > > - intel_disable_gt_powersave(dev); > > > - > > > > I wonder what was the reason for this call. One possibility is that > > i915_save_state() depends on it to save the correct registers, but it > > would be good to clarify this. save_state needs to die. Pretty much because it's fragile like you've just pointed out. > > It also cancels some deferred works which we do need here. But we could > > also add that to intel_enable_gt_powersave_sync() in this patch. > > Yeah I was worried about that too, but then we do the reset on resume > anyway, and I didn't see anything in my logs in testing... > > But I can split that out if there's a reason to. Seems like we do a > bit too much teardown at suspend these days (like tearing down opregion > state), I'd like to trim it back if possible and share between runtime > and system suspend/freeze. Yeah, that's the direction I'm pushing towards, too - we should only stop timers, work, interrupts and stuff like that, but never tear down structures. So if you can use this opportunity to fix a few of the offenders (like opregion) I'd be very happy. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 66c6ffb..433bdfa 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -521,8 +521,6 @@ static int i915_drm_freeze(struct drm_device *dev) drm_irq_uninstall(dev); dev_priv->enable_hotplug_processing = false; - intel_disable_gt_powersave(dev); - /* * Disable CRTCs directly since we want to preserve sw state * for _thaw. @@ -543,7 +541,6 @@ static int i915_drm_freeze(struct drm_device *dev) i915_save_state(dev); 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/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c597b0d..bf90e7d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -956,6 +956,7 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv); 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_enable_gt_powersave_sync(struct drm_device *dev); void intel_disable_gt_powersave(struct drm_device *dev); void intel_reset_gt_powersave(struct drm_device *dev); void ironlake_teardown_rc6(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 1840d15..8d9e036 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4891,12 +4891,9 @@ void intel_disable_gt_powersave(struct drm_device *dev) } } -static void intel_gen6_powersave_work(struct work_struct *work) +void intel_enable_gt_powersave_sync(struct drm_device *dev) { - struct drm_i915_private *dev_priv = - container_of(work, struct drm_i915_private, - rps.delayed_resume_work.work); - struct drm_device *dev = dev_priv->dev; + struct drm_i915_private *dev_priv = dev->dev_private; mutex_lock(&dev_priv->rps.hw_lock); @@ -4917,6 +4914,15 @@ static void intel_gen6_powersave_work(struct work_struct *work) intel_runtime_pm_put(dev_priv); } +static void intel_gen6_powersave_work(struct work_struct *work) +{ + struct drm_i915_private *dev_priv = + container_of(work, struct drm_i915_private, + rps.delayed_resume_work.work); + + intel_enable_gt_powersave_sync(dev_priv->dev); +} + void intel_enable_gt_powersave(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;