diff mbox

[v2,1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

Message ID 20170424204242.GA9933@wunner.de (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Lukas Wunner April 24, 2017, 8:42 p.m. UTC
On Mon, Apr 24, 2017 at 10:02:30PM +0200, Lukas Wunner wrote:
> On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> > Some drivers - like i915 - may not support the system suspend direct
> > complete optimization due to differences in their runtime and system
> > suspend sequence. Add a flag that when set resumes the device before
> > calling the driver's system suspend handlers which effectively disables
> > the optimization.
> 
> FWIW, there are at least two alternative solutions to this problem which
> do not require changes to the PCI core:
> 
> (1) Add a ->prepare hook to i915_pm_ops which calls pm_runtime_get_sync()
>     and a ->complete hook which calls pm_runtime_put().

Thinking a bit more about this, it's even simpler:  The PM core acquires
a runtime PM ref in device_prepare() and releases it in device_complete(),
so it's sufficient to just call pm_runtime_resume() in a ->prepare hook
that's newly added to i915.  No ->complete hook necessary.  Tentative
patch below, based on drm-intel-fixes, would replace both of your patches.

Thanks,

Lukas

-- >8 --

Comments

Rafael J. Wysocki April 24, 2017, 8:56 p.m. UTC | #1
On Monday, April 24, 2017 10:42:42 PM Lukas Wunner wrote:
> On Mon, Apr 24, 2017 at 10:02:30PM +0200, Lukas Wunner wrote:
> > On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> > > Some drivers - like i915 - may not support the system suspend direct
> > > complete optimization due to differences in their runtime and system
> > > suspend sequence. Add a flag that when set resumes the device before
> > > calling the driver's system suspend handlers which effectively disables
> > > the optimization.
> > 
> > FWIW, there are at least two alternative solutions to this problem which
> > do not require changes to the PCI core:
> > 
> > (1) Add a ->prepare hook to i915_pm_ops which calls pm_runtime_get_sync()
> >     and a ->complete hook which calls pm_runtime_put().
> 
> Thinking a bit more about this, it's even simpler:  The PM core acquires
> a runtime PM ref in device_prepare() and releases it in device_complete(),
> so it's sufficient to just call pm_runtime_resume() in a ->prepare hook
> that's newly added to i915.  No ->complete hook necessary.  Tentative
> patch below, based on drm-intel-fixes, would replace both of your patches.

Calling it in ->prepare() means that everybody is now waiting for you to resume.

Not quite optimal IMO.

Thanks,
Rafael
Lukas Wunner April 25, 2017, 6:21 a.m. UTC | #2
On Mon, Apr 24, 2017 at 10:56:40PM +0200, Rafael J. Wysocki wrote:
> On Monday, April 24, 2017 10:42:42 PM Lukas Wunner wrote:
> > On Mon, Apr 24, 2017 at 10:02:30PM +0200, Lukas Wunner wrote:
> > > On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> > > > Some drivers - like i915 - may not support the system suspend direct
> > > > complete optimization due to differences in their runtime and system
> > > > suspend sequence. Add a flag that when set resumes the device before
> > > > calling the driver's system suspend handlers which effectively disables
> > > > the optimization.
> > > 
> > > FWIW, there are at least two alternative solutions to this problem which
> > > do not require changes to the PCI core:
> > > 
> > > (1) Add a ->prepare hook to i915_pm_ops which calls pm_runtime_get_sync()
> > >     and a ->complete hook which calls pm_runtime_put().
> > 
> > Thinking a bit more about this, it's even simpler:  The PM core acquires
> > a runtime PM ref in device_prepare() and releases it in device_complete(),
> > so it's sufficient to just call pm_runtime_resume() in a ->prepare hook
> > that's newly added to i915.  No ->complete hook necessary.  Tentative
> > patch below, based on drm-intel-fixes, would replace both of your patches.
> 
> Calling it in ->prepare() means that everybody is now waiting for you to
> resume.
> 
> Not quite optimal IMO.

Okay, understood.

However in the absence of a device_link from the HDA device (consumer)
to the GPU (supplier), there's no guarantee that the GPU is resumed
when the HDA device needs it due to the asynchronous invocation of
the ->suspend hooks.  This is assuming that the HDA device already
needs the GPU during ->suspend phase.

Thanks,

Lukas
Imre Deak April 25, 2017, 8:55 a.m. UTC | #3
On Tue, Apr 25, 2017 at 08:21:49AM +0200, Lukas Wunner wrote:
> On Mon, Apr 24, 2017 at 10:56:40PM +0200, Rafael J. Wysocki wrote:
> > On Monday, April 24, 2017 10:42:42 PM Lukas Wunner wrote:
> > > On Mon, Apr 24, 2017 at 10:02:30PM +0200, Lukas Wunner wrote:
> > > > On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> > > > > Some drivers - like i915 - may not support the system suspend direct
> > > > > complete optimization due to differences in their runtime and system
> > > > > suspend sequence. Add a flag that when set resumes the device before
> > > > > calling the driver's system suspend handlers which effectively disables
> > > > > the optimization.
> > > > 
> > > > FWIW, there are at least two alternative solutions to this problem which
> > > > do not require changes to the PCI core:
> > > > 
> > > > (1) Add a ->prepare hook to i915_pm_ops which calls pm_runtime_get_sync()
> > > >     and a ->complete hook which calls pm_runtime_put().
> > > 
> > > Thinking a bit more about this, it's even simpler:  The PM core acquires
> > > a runtime PM ref in device_prepare() and releases it in device_complete(),
> > > so it's sufficient to just call pm_runtime_resume() in a ->prepare hook
> > > that's newly added to i915.  No ->complete hook necessary.  Tentative
> > > patch below, based on drm-intel-fixes, would replace both of your patches.
> > 
> > Calling it in ->prepare() means that everybody is now waiting for you to
> > resume.
> > 
> > Not quite optimal IMO.
> 
> Okay, understood.
> 
> However in the absence of a device_link from the HDA device (consumer)
> to the GPU (supplier), there's no guarantee that the GPU is resumed
> when the HDA device needs it due to the asynchronous invocation of
> the ->suspend hooks.  This is assuming that the HDA device already
> needs the GPU during ->suspend phase.

i915 has ->resume_early and ->suspend_late hooks that provides the above
guarantee.

--Imre
Lukas Wunner April 25, 2017, 9:46 a.m. UTC | #4
On Tue, Apr 25, 2017 at 11:55:07AM +0300, Imre Deak wrote:
> On Tue, Apr 25, 2017 at 08:21:49AM +0200, Lukas Wunner wrote:
> > On Mon, Apr 24, 2017 at 10:56:40PM +0200, Rafael J. Wysocki wrote:
> > > On Monday, April 24, 2017 10:42:42 PM Lukas Wunner wrote:
> > > > On Mon, Apr 24, 2017 at 10:02:30PM +0200, Lukas Wunner wrote:
> > > > > On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> > > > > > Some drivers - like i915 - may not support the system suspend direct
> > > > > > complete optimization due to differences in their runtime and system
> > > > > > suspend sequence. Add a flag that when set resumes the device before
> > > > > > calling the driver's system suspend handlers which effectively disables
> > > > > > the optimization.
> > > > > 
> > > > > FWIW, there are at least two alternative solutions to this problem which
> > > > > do not require changes to the PCI core:
> > > > > 
> > > > > (1) Add a ->prepare hook to i915_pm_ops which calls pm_runtime_get_sync()
> > > > >     and a ->complete hook which calls pm_runtime_put().
> > > > 
> > > > Thinking a bit more about this, it's even simpler:  The PM core acquires
> > > > a runtime PM ref in device_prepare() and releases it in device_complete(),
> > > > so it's sufficient to just call pm_runtime_resume() in a ->prepare hook
> > > > that's newly added to i915.  No ->complete hook necessary.  Tentative
> > > > patch below, based on drm-intel-fixes, would replace both of your patches.
> > > 
> > > Calling it in ->prepare() means that everybody is now waiting for you to
> > > resume.
> > > 
> > > Not quite optimal IMO.
> > 
> > Okay, understood.
> > 
> > However in the absence of a device_link from the HDA device (consumer)
> > to the GPU (supplier), there's no guarantee that the GPU is resumed
> > when the HDA device needs it due to the asynchronous invocation of
> > the ->suspend hooks.  This is assuming that the HDA device already
> > needs the GPU during ->suspend phase.
> 
> i915 has ->resume_early and ->suspend_late hooks that provides the above
> guarantee.

Employing device links is preferable IMO:

* Less code.  Just find the HDA device in your ->probe hook and call
  device_link_add().  Alternatively do it from the HDA driver.  Even
  adding the link from both drivers should be fine.  I can provide
  you with an evaluation patch if you'd like.

* Avoidance of doing things behind the PM core's back.

* Takes care of more stuff than just system sleep (e.g. shutdown ordering).

Thanks,

Lukas
Imre Deak April 25, 2017, 10:22 a.m. UTC | #5
On Tue, Apr 25, 2017 at 11:46:42AM +0200, Lukas Wunner wrote:
> On Tue, Apr 25, 2017 at 11:55:07AM +0300, Imre Deak wrote:
> > On Tue, Apr 25, 2017 at 08:21:49AM +0200, Lukas Wunner wrote:
> > > On Mon, Apr 24, 2017 at 10:56:40PM +0200, Rafael J. Wysocki wrote:
> > > > On Monday, April 24, 2017 10:42:42 PM Lukas Wunner wrote:
> > > > > On Mon, Apr 24, 2017 at 10:02:30PM +0200, Lukas Wunner wrote:
> > > > > > On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> > > > > > > Some drivers - like i915 - may not support the system suspend direct
> > > > > > > complete optimization due to differences in their runtime and system
> > > > > > > suspend sequence. Add a flag that when set resumes the device before
> > > > > > > calling the driver's system suspend handlers which effectively disables
> > > > > > > the optimization.
> > > > > > 
> > > > > > FWIW, there are at least two alternative solutions to this problem which
> > > > > > do not require changes to the PCI core:
> > > > > > 
> > > > > > (1) Add a ->prepare hook to i915_pm_ops which calls pm_runtime_get_sync()
> > > > > >     and a ->complete hook which calls pm_runtime_put().
> > > > > 
> > > > > Thinking a bit more about this, it's even simpler:  The PM core acquires
> > > > > a runtime PM ref in device_prepare() and releases it in device_complete(),
> > > > > so it's sufficient to just call pm_runtime_resume() in a ->prepare hook
> > > > > that's newly added to i915.  No ->complete hook necessary.  Tentative
> > > > > patch below, based on drm-intel-fixes, would replace both of your patches.
> > > > 
> > > > Calling it in ->prepare() means that everybody is now waiting for you to
> > > > resume.
> > > > 
> > > > Not quite optimal IMO.
> > > 
> > > Okay, understood.
> > > 
> > > However in the absence of a device_link from the HDA device (consumer)
> > > to the GPU (supplier), there's no guarantee that the GPU is resumed
> > > when the HDA device needs it due to the asynchronous invocation of
> > > the ->suspend hooks.  This is assuming that the HDA device already
> > > needs the GPU during ->suspend phase.
> > 
> > i915 has ->resume_early and ->suspend_late hooks that provides the above
> > guarantee.
> 
> Employing device links is preferable IMO:
> 
> * Less code.  Just find the HDA device in your ->probe hook and call
>   device_link_add().  Alternatively do it from the HDA driver.  Even
>   adding the link from both drivers should be fine.  I can provide
>   you with an evaluation patch if you'd like.
> 
> * Avoidance of doing things behind the PM core's back.
> 
> * Takes care of more stuff than just system sleep (e.g. shutdown ordering).

Yes, device_links is something to consider for the future, however not
for this fix which needs to be backported and which also addresses the
other issue I mentioned in the commit log.

--Imre
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5c089b3..6ef156b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1820,6 +1820,11 @@  void i915_reset(struct drm_i915_private *dev_priv)
 	goto wakeup;
 }
 
+static int i915_pm_prepare(struct device *kdev)
+{
+	pm_runtime_resume(kdev);
+}
+
 static int i915_pm_suspend(struct device *kdev)
 {
 	struct pci_dev *pdev = to_pci_dev(kdev);
@@ -2451,6 +2456,7 @@  static int intel_runtime_resume(struct device *kdev)
 	 * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND,
 	 * PMSG_RESUME]
 	 */
+	.prepare = i915_pm_prepare,
 	.suspend = i915_pm_suspend,
 	.suspend_late = i915_pm_suspend_late,
 	.resume_early = i915_pm_resume_early,