| Message ID | 1442525012-1845-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-09-17 18:23 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: > 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. */ So is the comment above still valid? As far as I remember, we explicitly wake up the hardware after unload because our driver is (was) not prepared to be loaded on a hardware that is suspended. Did you try module reloading after this change? Also, basic-pci-d3-state should not be unloading/reloading the driver, so it's not clear to me how this change helps passing that test. > pm_runtime_get_sync(device); > - pm_runtime_disable(device); > + pm_runtime_get_noresume(device); > } > > /** > @@ -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); > - > /* > * 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); > } > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 09/17/2015 02:40 PM, Paulo Zanoni wrote: > 2015-09-17 18:23 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: >> 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. */ > > So is the comment above still valid? > > As far as I remember, we explicitly wake up the hardware after unload > because our driver is (was) not prepared to be loaded on a hardware > that is suspended. Did you try module reloading after this change? I'm still debugging it; in fact I think this change may be wrong. > Also, basic-pci-d3-state should not be unloading/reloading the driver, > so it's not clear to me how this change helps passing that test. The BAT tests just happen to run the module reload test first. That's how I caught this in the first place... Jesse
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); > - > /* > * 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(). > } > > The rule is that .probe() should do one extra decrementation of the usage counter and .remove() should do one extra incrementation of it. Enable/disable should never be necessary. Thanks, Rafael
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
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); + pm_runtime_get_noresume(device); } /** @@ -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); - /* * 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); }
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(-)