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