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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center