Re: [PATCH] drm/i915: use correct runtime get/put calls at init/teardown

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

On Friday, September 18, 2015 03:01:50 AM Rafael J. Wysocki wrote:
> On Thursday, September 17, 2015 02:23:32 PM Jesse Barnes wrote:
> > According to the PCI docs and Rafael, we need to be using
> > pm_runtime_put_noidle() and pm_runtime_get_noresume() in our init and
> > teardown routines, rather than using a direct enable/disable pair (and
> > we didn't even have the enable side, so never autosuspended after an
> > unload).
> > 
> > This fixes one failure of the basic-pci-d3-state test on my BYT.  I'm
> > still debugging why the device never autosuspends.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 85c35fd..1addb8a 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1822,7 +1822,7 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
> >  
> >  	/* Make sure we're not suspended first. */
> >  	pm_runtime_get_sync(device);
> > -	pm_runtime_disable(device);
> 
> That is correct IMO.  If you ensure that the usage counter is always above 0
> and the device is "on", you don't need to disable runtime PM in addition to that.
> 
> > +	pm_runtime_get_noresume(device);
> 
> I'm not sure that this is needed.  You've already bumped up the usage counter.
> 
> Are you going to drop it going forward?
>
> 
> >  }
> >  
> >  /**
> > @@ -2114,8 +2114,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
> >  	if (!HAS_RUNTIME_PM(dev))
> >  		return;
> >  
> > -	pm_runtime_set_active(device);
> > -

This change looks OK to me.

You're called from local_pci_probe() that does pm_runtime_get_sync(), so the
device should be active at this point if I'm not missing anything.

The only kind of gray area is that the device may be physically off at probe
time (after boot) and we're thinking it is on. Is that possible even?

> >  	/*
> >  	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
> >  	 * requirement.
> > @@ -2130,5 +2128,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
> >  	pm_runtime_use_autosuspend(device);
> >  
> >  	pm_runtime_put_autosuspend(device);
> > +	pm_runtime_put_noidle(device);
> 
> That shouldn't be necessary.  The "put" is already done in pm_runtime_put_autosuspend().
> 
> >  }
> >  
> > 

Thanks,
Rafael

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