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

[Lukas Wunner <lukas@wunner.de>]

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