[Intel-gfx] [PATCH] drm/i915: Prevent the system suspend complete optimization

[Imre Deak]

On Thu, Apr 13, 2017 at 12:10:49PM +0300, Imre Deak wrote:
> On Thu, Apr 13, 2017 at 04:29:41AM +0200, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > First off, sorry for introducing the problem and thanks for taking care of
> > it!
> > 
> > On 4/11/2017 7:09 PM, Imre Deak wrote:
> > >On Tue, Apr 11, 2017 at 05:54:07PM +0100, Chris Wilson wrote:
> > >>On Tue, Apr 11, 2017 at 07:12:35PM +0300, Imre Deak wrote:
> > >>>+static int i915_pm_prepare(struct device *kdev)
> > >>>+{
> > >>>+	/*
> > >>>+	 * Get a reference to disable the direct complete optimization. This
> > >>>+	 * is needed, since we have a suspend sequence specific to system
> > >>>+	 * suspend (that is different from runtime suspend) and because we
> > >>>+	 * need to provide power to the sound driver while its system suspend
> > >>>+	 * handler is running. This is not possible with the optimization in
> > >>>+	 * effect, when the i915 runtime PM is disabled for the whole duration
> > >>>+	 * of the suspend sequence if the device was already runtime
> > >>>+	 * suspended at the beginning of the sequence. In this case the i915
> > >>>+	 * suspend/resume hooks would be also skipped (besides its prepare and
> > >>>+	 * complete hooks).
> > >>>+	 */
> > >>>+	intel_runtime_pm_get(kdev_to_i915(kdev));
> > >>>+
> > >>>+	return 0;
> > >>>+}
> > >>>+
> > >>>+static void i915_pm_complete(struct device *kdev)
> > >>>+{
> > >>>+	/* Put the ref taken in the prepare step. */
> > >>>+	intel_runtime_pm_put(kdev_to_i915(kdev));
> > >>Do we always call i915_pm_complete() if any of the post-prepare suspend
> > >>steps fail? Otherwise, it looks very sensible from our pov.
> > >Yes, it's called even in the failure case (for S3 for example
> > >suspend_devices_and_enter()->Recover_platform:->Resume_devices:->
> > >dpm_resume_end()->dpm_complete()).
> > 
> > ->prepare and ->complete are not the most friendly places to do these
> > things, though, because then the whole kernel needs to wait for them to
> > return and if they take time, it goes ugly.
> > 
> > Have you considered adding a need_resume flag to struct pci_dev, setting it
> > for i915 and checking it along with platform_pci_need_resume() in
> > pci_dev_keep_suspended()?
> 
> Haven't considered it, can do that instead.
> 
> Note that it doesn't need to be resumed in all cases, although that's
> what's happening now. During suspend-to-idle, depending on the HDA
> driver's requirements, it could stay suspended. Calling
> pm_runtime_get_noresume() in ->prepare() and pm_runtime_put() in
> ->complete() would be more inline with that without the overhead of
> actually resuming. Although pci_pm_suspend() will resume the device even
> then.

Ah, just realized that get_noresume() is not enough to prevent the
optimization, the device needs to be actually resumed for that. So nvm
about the above, I'll add the new flag as you suggested.

--Imre