| Message ID | 1401397897-4655-4-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 matches the runtime suspend paths and allows the system to enter > the lowest power mode at freeze time. > > 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 | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index b6211d7..24dc856 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev) > > intel_display_set_init_power(dev_priv, false); > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > + hsw_enable_pc8(dev_priv); > + > return 0; > } > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > + hsw_disable_pc8(dev_priv); I would put this before we access any of the HW regs in thaw_early() and correspondingly the above call to hsw_enable_pc8() to suspend_late() before we call pci_disable_device(). With that change this is: Reviewed-by: Imre Deak <imre.deak@intel.com> > + > if (drm_core_check_feature(dev, DRIVER_MODESET) && > restore_gtt_mappings) { > mutex_lock(&dev->struct_mutex);
On Fri, 30 May 2014 16:37:53 +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 matches the runtime suspend paths and allows the system to enter > > the lowest power mode at freeze time. > > > > 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 | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index b6211d7..24dc856 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > intel_display_set_init_power(dev_priv, false); > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > + hsw_enable_pc8(dev_priv); > > + > > return 0; > > } > > > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > + hsw_disable_pc8(dev_priv); > > I would put this before we access any of the HW regs in thaw_early() and > correspondingly the above call to hsw_enable_pc8() to suspend_late() > before we call pci_disable_device(). > > With that change this is: > Reviewed-by: Imre Deak <imre.deak@intel.com> For the thaw side I think that makes sense. But for the freeze side, putting it in suspend_late won't get us the freeze behavior we want. I think Rafael and Kristen are thinking of re-using the freeze infrastructure for some kind of connected standby feature, where most stuff is frozen, but the system isn't in S3 or S4, so we need the enable_pc8 call in the _freeze path as well. Rafael, is that correct? I'll add a late_freeze and put it there instead, so it doesn't pollute the S3 suspend path. Thanks,
On Fri, 30 May 2014 23:12:45 +0200 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Friday, May 30, 2014 11:29:15 AM Jesse Barnes wrote: > > On Fri, 30 May 2014 16:37:53 +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 matches the runtime suspend paths and allows the system to enter > > > > the lowest power mode at freeze time. > > > > > > > > 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 | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index b6211d7..24dc856 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > > > + hsw_enable_pc8(dev_priv); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > > > { > > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > > > + hsw_disable_pc8(dev_priv); > > > > > > I would put this before we access any of the HW regs in thaw_early() and > > > correspondingly the above call to hsw_enable_pc8() to suspend_late() > > > before we call pci_disable_device(). > > > > > > With that change this is: > > > Reviewed-by: Imre Deak <imre.deak@intel.com> > > > > For the thaw side I think that makes sense. > > > > But for the freeze side, putting it in suspend_late won't get us the > > freeze behavior we want. I think Rafael and Kristen are thinking of > > re-using the freeze infrastructure for some kind of connected standby > > feature, where most stuff is frozen, but the system isn't in S3 or S4, > > so we need the enable_pc8 call in the _freeze path as well. > > > > Rafael, is that correct? > > No, it isn't. The .freeze()/.thaw() callbacks are hibernation-specific. > There are no plans for using this in PM beyond hibernation. > > What we're going to use are .suspend/_late/_noirq() and the corresponding > resume callbacks and runtime PM. > > > I'll add a late_freeze and put it there instead, so it doesn't pollute > > the S3 suspend path. > > The freeze/thaw stuff need not do any PM BTW, it just needs to quiesce the > device to prevent it from doing DMA etc and then bring it back to life. Ok thanks. Kristen corrected me on IRC too. The latest patch I sent should do what we want then, now that I've removed the freeze_late function and put our PC8 enable in suspend_late like Imre suggested initially.
On Friday, May 30, 2014 11:29:15 AM Jesse Barnes wrote: > On Fri, 30 May 2014 16:37:53 +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 matches the runtime suspend paths and allows the system to enter > > > the lowest power mode at freeze time. > > > > > > 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 | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index b6211d7..24dc856 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > > > intel_display_set_init_power(dev_priv, false); > > > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > > + hsw_enable_pc8(dev_priv); > > > + > > > return 0; > > > } > > > > > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > > + hsw_disable_pc8(dev_priv); > > > > I would put this before we access any of the HW regs in thaw_early() and > > correspondingly the above call to hsw_enable_pc8() to suspend_late() > > before we call pci_disable_device(). > > > > With that change this is: > > Reviewed-by: Imre Deak <imre.deak@intel.com> > > For the thaw side I think that makes sense. > > But for the freeze side, putting it in suspend_late won't get us the > freeze behavior we want. I think Rafael and Kristen are thinking of > re-using the freeze infrastructure for some kind of connected standby > feature, where most stuff is frozen, but the system isn't in S3 or S4, > so we need the enable_pc8 call in the _freeze path as well. > > Rafael, is that correct? No, it isn't. The .freeze()/.thaw() callbacks are hibernation-specific. There are no plans for using this in PM beyond hibernation. What we're going to use are .suspend/_late/_noirq() and the corresponding resume callbacks and runtime PM. > I'll add a late_freeze and put it there instead, so it doesn't pollute > the S3 suspend path. The freeze/thaw stuff need not do any PM BTW, it just needs to quiesce the device to prevent it from doing DMA etc and then bring it back to life. Rafael
On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote: > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > This matches the runtime suspend paths and allows the system to enter > the lowest power mode at freeze time. > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> pc8 is fully subsumed into runtime pm by now. Do we _really_ still need this? -Daniel > --- > drivers/gpu/drm/i915/i915_drv.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index b6211d7..24dc856 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev) > > intel_display_set_init_power(dev_priv, false); > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > + hsw_enable_pc8(dev_priv); > + > return 0; > } > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > + hsw_disable_pc8(dev_priv); > + > if (drm_core_check_feature(dev, DRIVER_MODESET) && > restore_gtt_mappings) { > mutex_lock(&dev->struct_mutex); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote: > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote: > > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > This matches the runtime suspend paths and allows the system to enter > > the lowest power mode at freeze time. > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need > this? Yes, since the system suspend/resume handlers are called with an RPM ref held and thus PC8 disabled. --Imre > -Daniel > > > --- > > drivers/gpu/drm/i915/i915_drv.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index b6211d7..24dc856 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev) > > > > intel_display_set_init_power(dev_priv, false); > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > + hsw_enable_pc8(dev_priv); > > + > > return 0; > > } > > > > @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > + hsw_disable_pc8(dev_priv); > > + > > if (drm_core_check_feature(dev, DRIVER_MODESET) && > > restore_gtt_mappings) { > > mutex_lock(&dev->struct_mutex); > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Jun 02, 2014 at 02:37:35PM +0300, Imre Deak wrote: > On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote: > > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote: > > > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > > > This matches the runtime suspend paths and allows the system to enter > > > the lowest power mode at freeze time. > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need > > this? > > Yes, since the system suspend/resume handlers are called with an RPM ref > held and thus PC8 disabled. But doesn't patch 1 try to fix that? Imo we should have this here but instead go through highl-level the runtime pm functions to shut off the chip on all platforms. -Daniel
On Mon, 2014-06-02 at 17:32 +0200, Daniel Vetter wrote: > On Mon, Jun 02, 2014 at 02:37:35PM +0300, Imre Deak wrote: > > On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote: > > > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote: > > > > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > > > > > This matches the runtime suspend paths and allows the system to enter > > > > the lowest power mode at freeze time. > > > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > > > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need > > > this? > > > > Yes, since the system suspend/resume handlers are called with an RPM ref > > held and thus PC8 disabled. > > But doesn't patch 1 try to fix that? That only disables the display side, but we won't disable PC8 until the RPM suspend handler is called. And that won't happen because the last RPM ref is held by the DPM framework for the duration of system suspend/resume handlers. > Imo we should have this here but instead go through highl-level the runtime > pm functions to shut off the chip on all platforms. After the planned refactoring we could have a low-level function that we can call both from here and the runtime PM path, but until that happens we need to do this here directly. --Imre
On Mon, Jun 02, 2014 at 06:57:18PM +0300, Imre Deak wrote: > On Mon, 2014-06-02 at 17:32 +0200, Daniel Vetter wrote: > > On Mon, Jun 02, 2014 at 02:37:35PM +0300, Imre Deak wrote: > > > On Mon, 2014-06-02 at 10:45 +0200, Daniel Vetter wrote: > > > > On Thu, May 29, 2014 at 02:11:37PM -0700, Jesse Barnes wrote: > > > > > From: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > > > > > > > This matches the runtime suspend paths and allows the system to enter > > > > > the lowest power mode at freeze time. > > > > > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > > > > > pc8 is fully subsumed into runtime pm by now. Do we _really_ still need > > > > this? > > > > > > Yes, since the system suspend/resume handlers are called with an RPM ref > > > held and thus PC8 disabled. > > > > But doesn't patch 1 try to fix that? > > That only disables the display side, but we won't disable PC8 until the > RPM suspend handler is called. And that won't happen because the last > RPM ref is held by the DPM framework for the duration of system > suspend/resume handlers. Yeah, there's discussion going on to make system suspend be optionally based upon runtime pm in the core. Atm that's not possible since the system wakes up everyone to suspend them. > > Imo we should have this here but instead go through highl-level the runtime > > pm functions to shut off the chip on all platforms. > > After the planned refactoring we could have a low-level function that we > can call both from here and the runtime PM path, but until that happens > we need to do this here directly. Yeah, that's what I'm actually aiming for - we should be able to call the runtime pm suspend code from the system suspend code and share pretty much all the code. The sequence I'm thinking of would be for system suspend: 1. Stop everything we need to stop (gpu, display, rps, ...) to be able to enter runtime pm. 2. Check for leaked references. This might be tricky because audio. 3. Call runtime pm suspend hooks. Resume would be the same, but inverted. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index b6211d7..24dc856 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev) intel_display_set_init_power(dev_priv, false); + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) + hsw_enable_pc8(dev_priv); + return 0; } @@ -618,6 +621,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) { struct drm_i915_private *dev_priv = dev->dev_private; + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) + hsw_disable_pc8(dev_priv); + if (drm_core_check_feature(dev, DRIVER_MODESET) && restore_gtt_mappings) { mutex_lock(&dev->struct_mutex);