Re: [PATCH 2/4] drm/i915: leave rc6 enabled at suspend time

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch