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

[Imre Deak]

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.

--Imre