| Message ID | 20170424204242.GA9933@wunner.de |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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
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
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 --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,