Re: [PATCH 4/4] drm/i915: make sure PC8 is enabled on suspend and disabled on resume

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

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