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

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3362 bytes --]

On Wed, 2014-06-11 at 15:24 -0700, Jesse Barnes wrote:
> On Wed, 11 Jun 2014 15:21:16 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Tue, 10 Jun 2014 17:26:45 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Tue, Jun 10, 2014 at 05:41:49PM +0300, Imre Deak wrote:
> > > > On Tue, 2014-06-10 at 15:57 +0200, Daniel Vetter wrote:
> > > > > On Tue, Jun 10, 2014 at 04:42:50PM +0300, Imre Deak wrote:
> > > > > > On Wed, 2014-06-04 at 13:45 -0700, Jesse Barnes wrote:
> > > > > > > This allows the system to enter the lowest power mode during system freeze.
> > > > > > > 
> > > > > > > v2: delete force wake timer at suspend (Imre)
> > > > > > > v3: add GT work suspend function (Imre)
> > > > > > > v4: use uncore forcewake reset (Daniel)
> > > > > > > 
> > > > > > > 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     |  4 ++--
> > > > > > >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> > > > > > >  drivers/gpu/drm/i915/intel_drv.h    |  1 +
> > > > > > >  drivers/gpu/drm/i915/intel_pm.c     | 20 ++++++++++++++++++++
> > > > > > >  drivers/gpu/drm/i915/intel_uncore.c |  2 +-
> > > > > > >  5 files changed, 25 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > index 66c6ffb..7148eac 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > @@ -521,7 +521,7 @@ static int i915_drm_freeze(struct drm_device *dev)
> > > > > > >  		drm_irq_uninstall(dev);
> > > > > > >  		dev_priv->enable_hotplug_processing = false;
> > > > > > >  
> > > > > > > -		intel_disable_gt_powersave(dev);
> > > > > > > +		intel_suspend_gt_powersave(dev);
> > > > > > 
> > > > > > I realized now that we actually do need to enable RC6 explicitly. If we
> > > > > > get here right after runtime resume, the deferred RC6 enabling might be
> > > > > > still pending and so RC6 might not be enabled.
> > 
> > Seems like we could just flush the enable if it was outstanding?
> 
> Oh and I see we already have the flush in v4...

Right, I was just blind and I guess also thinking in terms of timers
that don't have a similar flush API. Anyway this is fine and my comment
about it above can be ignored.

> do you think we might not actually arm the deferred enable work before
> we get here? 

I haven't thought about this. But now that you mentioned I looked into
the device and dpm core and can't see what would prevent our system
suspend handler to be called while we are still running our driver load
function. But this is a separate issue (if true at all) that exists
regardless of this patch and also not a really real life scenario. So I
suggest we investigate this as a follow-up.

> I think the runtime_resume and suspend calls should be serialized, but
> if not we'd need some locking I guess...

Yes, this is taken care of the RPM framework. Similarly system
suspend/resume handlers are serialized wrt. the runtime suspend/resume
handlers.

This patch is:
Reviewed-by: Imre Deak <imre.deak@intel.com>

Btw, now all 4 patches in the patchset are reviewed.

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx