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

[Imre Deak]

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