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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx