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

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

[Imre Deak]

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.

Needed by the next patch fixing suspend/resume on i915.

Suggested by Rafael.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/pci/pci.c   | 3 ++-
 include/linux/pci.h | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7904d02..020f02d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2141,7 +2141,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
 
 	if (!pm_runtime_suspended(dev)
 	    || pci_target_state(pci_dev) != pci_dev->current_state
-	    || platform_pci_need_resume(pci_dev))
+	    || platform_pci_need_resume(pci_dev)
+	    || pci_dev->needs_resume)
 		return false;
 
 	/*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e2d1a12..3fe00a6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -316,6 +316,9 @@ struct pci_dev {
 	unsigned int	hotplug_user_indicators:1; /* SlotCtl indicators
 						      controlled exclusively by
 						      user sysfs */
+	unsigned int	needs_resume:1;	/* Resume before calling the driver's
+					   system suspend hooks, disabling the
+					   direct_complete optimization. */
 	unsigned int	d3_delay;	/* D3->D0 transition time in ms */
 	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
 
@@ -1113,6 +1116,10 @@ bool pci_check_pme_status(struct pci_dev *dev);
 void pci_pme_wakeup_bus(struct pci_bus *bus);
 void pci_d3cold_enable(struct pci_dev *dev);
 void pci_d3cold_disable(struct pci_dev *dev);
+static inline void pci_resume_before_suspend(struct pci_dev *dev, bool enable)
+{
+	dev->needs_resume = enable;
+}
 
 static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 				  bool enable)
-- 
2.5.0

[PATCH v2 2/2] drm/i915: Prevent the system suspend complete optimization

[Imre Deak]

Since

commit bac2a909a096c9110525c18cbb8ce73c660d5f71
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Wed Jan 21 02:17:42 2015 +0100

    PCI / PM: Avoid resuming PCI devices during system suspend

PCI devices will default to allowing the system suspend complete
optimization where devices are not woken up during system suspend if
they were already runtime suspended. This however breaks the i915/HDA
drivers for two reasons:

- The i915 driver has system suspend specific steps that it needs to
  run, that bring the device to a different state than its runtime
  suspended state.

- The HDA driver's suspend handler requires power that it will request
  from the i915 driver's power domain handler. This in turn requires the
  i915 driver to runtime resume itself, but this won't be possible if the
  suspend complete optimization is in effect: in this case the i915
  runtime PM is disabled and trying to get an RPM reference returns
  -EACCESS.

Solve this by requiring the PCI/PM core to resume the device during
system suspend which in effect disables the suspend complete optimization.

One possibility for this bug staying hidden for so long is that the
optimization for a device is disabled if it's disabled for any of its
children devices. i915 may have a backlight device as its child which
doesn't support runtime PM and so doesn't allow the optimization either.
So if this backlight device got registered the bug stayed hidden.

Credits to Marta, Tomi and David who enabled pstore logging,
that caught one instance of this issue across a suspend/
resume-to-ram and Ville who rememberd that the optimization was enabled
for some devices at one point.

The first WARN triggered by the problem:

[ 6250.746445] WARNING: CPU: 2 PID: 17384 at drivers/gpu/drm/i915/intel_runtime_pm.c:2846 intel_runtime_pm_get+0x6b/0xd0 [i915]
[ 6250.746448] pm_runtime_get_sync() failed: -13
[ 6250.746451] Modules linked in: snd_hda_intel i915 vgem snd_hda_codec_hdmi x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul
snd_hda_codec_realtek snd_hda_codec_generic ghash_clmulni_intel e1000e snd_hda_codec snd_hwdep snd_hda_core ptp mei_me pps_core snd_pcm lpc_ich mei prime_
numbers i2c_hid i2c_designware_platform i2c_designware_core [last unloaded: i915]
[ 6250.746512] CPU: 2 PID: 17384 Comm: kworker/u8:0 Tainted: G     U  W       4.11.0-rc5-CI-CI_DRM_334+ #1
[ 6250.746515] Hardware name:                  /NUC5i5RYB, BIOS RYBDWi35.86A.0362.2017.0118.0940 01/18/2017
[ 6250.746521] Workqueue: events_unbound async_run_entry_fn
[ 6250.746525] Call Trace:
[ 6250.746530]  dump_stack+0x67/0x92
[ 6250.746536]  __warn+0xc6/0xe0
[ 6250.746542]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746546]  warn_slowpath_fmt+0x46/0x50
[ 6250.746553]  ? __pm_runtime_resume+0x56/0x80
[ 6250.746584]  intel_runtime_pm_get+0x6b/0xd0 [i915]
[ 6250.746610]  intel_display_power_get+0x1b/0x40 [i915]
[ 6250.746646]  i915_audio_component_get_power+0x15/0x20 [i915]
[ 6250.746654]  snd_hdac_display_power+0xc8/0x110 [snd_hda_core]
[ 6250.746661]  azx_runtime_resume+0x218/0x280 [snd_hda_intel]
[ 6250.746667]  pci_pm_runtime_resume+0x76/0xa0
[ 6250.746672]  __rpm_callback+0xb4/0x1f0
[ 6250.746677]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746682]  rpm_callback+0x1f/0x80
[ 6250.746686]  ? pci_restore_standard_config+0x40/0x40
[ 6250.746690]  rpm_resume+0x4ba/0x740
[ 6250.746698]  __pm_runtime_resume+0x49/0x80
[ 6250.746703]  pci_pm_suspend+0x57/0x140
[ 6250.746709]  dpm_run_callback+0x6f/0x330
[ 6250.746713]  ? pci_pm_freeze+0xe0/0xe0
[ 6250.746718]  __device_suspend+0xf9/0x370
[ 6250.746724]  ? dpm_watchdog_set+0x60/0x60
[ 6250.746730]  async_suspend+0x1a/0x90
[ 6250.746735]  async_run_entry_fn+0x34/0x160
[ 6250.746741]  process_one_work+0x1f2/0x6d0
[ 6250.746749]  worker_thread+0x49/0x4a0
[ 6250.746755]  kthread+0x107/0x140
[ 6250.746759]  ? process_one_work+0x6d0/0x6d0
[ 6250.746763]  ? kthread_create_on_node+0x40/0x40
[ 6250.746768]  ret_from_fork+0x2e/0x40
[ 6250.746778] ---[ end trace 102a62fd2160f5e6 ]---

v2:
- Use the new pci_dev->needs_resume flag, to avoid any overhead during
  the ->pm_prepare hook. (Rafael)

Fixes: bac2a909a096 ("PCI / PM: Avoid resuming PCI devices during system suspend")
References: https://bugs.freedesktop.org/show_bug.cgi?id=100378
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Marta Lofstedt <marta.lofstedt@intel.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
---
 drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8be958f..dd7ce69 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1207,6 +1207,15 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_free_priv;
 
 	pci_set_drvdata(pdev, &dev_priv->drm);
+	/*
+	 * Disable the system suspend direct complete optimization, which can
+	 * leave the device suspended skipping the driver's suspend handlers
+	 * if the device was already runtime suspended. This is needed due to
+	 * the difference in our runtime and system suspend sequence and
+	 * becaue the HDA driver may require us to enable the audio power
+	 * domain during system suspend.
+	 */
+	pci_resume_before_suspend(pdev, true);
 
 	ret = i915_driver_init_early(dev_priv, ent);
 	if (ret < 0)
-- 
2.5.0

Re: [PATCH v2 2/2] drm/i915: Prevent the system suspend complete optimization

[Lukas Wunner]

On Mon, Apr 24, 2017 at 05:27:43PM +0300, Imre Deak wrote:
> Since
> 
> commit bac2a909a096c9110525c18cbb8ce73c660d5f71
> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Date:   Wed Jan 21 02:17:42 2015 +0100
> 
>     PCI / PM: Avoid resuming PCI devices during system suspend

This is not the commit you are looking for. :-)  See below.


> PCI devices will default to allowing the system suspend complete
> optimization where devices are not woken up during system suspend if
> they were already runtime suspended. This however breaks the i915/HDA
> drivers for two reasons:
> 
> - The i915 driver has system suspend specific steps that it needs to
>   run, that bring the device to a different state than its runtime
>   suspended state.
> 
> - The HDA driver's suspend handler requires power that it will request
>   from the i915 driver's power domain handler. This in turn requires the
>   i915 driver to runtime resume itself, but this won't be possible if the
>   suspend complete optimization is in effect: in this case the i915
>   runtime PM is disabled and trying to get an RPM reference returns
>   -EACCESS.

Hm, sounds like something that needs to be solved with device_links.


> 
> Solve this by requiring the PCI/PM core to resume the device during
> system suspend which in effect disables the suspend complete optimization.
> 
> One possibility for this bug staying hidden for so long is that the
> optimization for a device is disabled if it's disabled for any of its
> children devices. i915 may have a backlight device as its child which
> doesn't support runtime PM and so doesn't allow the optimization either.
> So if this backlight device got registered the bug stayed hidden.

No, the reason this hasn't popped up earlier is because direct_complete
has only been enabled for DRM devices for a few months now, to be specific
since

    commit d14d2a8453d650bea32a1c5271af1458cd283a0f
    Author: Lukas Wunner <lukas@wunner.de>
    Date:   Wed Jun 8 12:49:29 2016 +0200

    drm: Remove dev_pm_ops from drm_class

which landed in v4.8.

(Sorry for not raising my voice earlier, this patch appeared on my radar
just now.)

Kind regards,

Lukas

> 
> Credits to Marta, Tomi and David who enabled pstore logging,
> that caught one instance of this issue across a suspend/
> resume-to-ram and Ville who rememberd that the optimization was enabled
> for some devices at one point.
> 
> The first WARN triggered by the problem:
> 
> [ 6250.746445] WARNING: CPU: 2 PID: 17384 at drivers/gpu/drm/i915/intel_runtime_pm.c:2846 intel_runtime_pm_get+0x6b/0xd0 [i915]
> [ 6250.746448] pm_runtime_get_sync() failed: -13
> [ 6250.746451] Modules linked in: snd_hda_intel i915 vgem snd_hda_codec_hdmi x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul
> snd_hda_codec_realtek snd_hda_codec_generic ghash_clmulni_intel e1000e snd_hda_codec snd_hwdep snd_hda_core ptp mei_me pps_core snd_pcm lpc_ich mei prime_
> numbers i2c_hid i2c_designware_platform i2c_designware_core [last unloaded: i915]
> [ 6250.746512] CPU: 2 PID: 17384 Comm: kworker/u8:0 Tainted: G     U  W       4.11.0-rc5-CI-CI_DRM_334+ #1
> [ 6250.746515] Hardware name:                  /NUC5i5RYB, BIOS RYBDWi35.86A.0362.2017.0118.0940 01/18/2017
> [ 6250.746521] Workqueue: events_unbound async_run_entry_fn
> [ 6250.746525] Call Trace:
> [ 6250.746530]  dump_stack+0x67/0x92
> [ 6250.746536]  __warn+0xc6/0xe0
> [ 6250.746542]  ? pci_restore_standard_config+0x40/0x40
> [ 6250.746546]  warn_slowpath_fmt+0x46/0x50
> [ 6250.746553]  ? __pm_runtime_resume+0x56/0x80
> [ 6250.746584]  intel_runtime_pm_get+0x6b/0xd0 [i915]
> [ 6250.746610]  intel_display_power_get+0x1b/0x40 [i915]
> [ 6250.746646]  i915_audio_component_get_power+0x15/0x20 [i915]
> [ 6250.746654]  snd_hdac_display_power+0xc8/0x110 [snd_hda_core]
> [ 6250.746661]  azx_runtime_resume+0x218/0x280 [snd_hda_intel]
> [ 6250.746667]  pci_pm_runtime_resume+0x76/0xa0
> [ 6250.746672]  __rpm_callback+0xb4/0x1f0
> [ 6250.746677]  ? pci_restore_standard_config+0x40/0x40
> [ 6250.746682]  rpm_callback+0x1f/0x80
> [ 6250.746686]  ? pci_restore_standard_config+0x40/0x40
> [ 6250.746690]  rpm_resume+0x4ba/0x740
> [ 6250.746698]  __pm_runtime_resume+0x49/0x80
> [ 6250.746703]  pci_pm_suspend+0x57/0x140
> [ 6250.746709]  dpm_run_callback+0x6f/0x330
> [ 6250.746713]  ? pci_pm_freeze+0xe0/0xe0
> [ 6250.746718]  __device_suspend+0xf9/0x370
> [ 6250.746724]  ? dpm_watchdog_set+0x60/0x60
> [ 6250.746730]  async_suspend+0x1a/0x90
> [ 6250.746735]  async_run_entry_fn+0x34/0x160
> [ 6250.746741]  process_one_work+0x1f2/0x6d0
> [ 6250.746749]  worker_thread+0x49/0x4a0
> [ 6250.746755]  kthread+0x107/0x140
> [ 6250.746759]  ? process_one_work+0x6d0/0x6d0
> [ 6250.746763]  ? kthread_create_on_node+0x40/0x40
> [ 6250.746768]  ret_from_fork+0x2e/0x40
> [ 6250.746778] ---[ end trace 102a62fd2160f5e6 ]---
> 
> v2:
> - Use the new pci_dev->needs_resume flag, to avoid any overhead during
>   the ->pm_prepare hook. (Rafael)
> 
> Fixes: bac2a909a096 ("PCI / PM: Avoid resuming PCI devices during system suspend")
> References: https://bugs.freedesktop.org/show_bug.cgi?id=100378
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 8be958f..dd7ce69 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1207,6 +1207,15 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto out_free_priv;
>  
>  	pci_set_drvdata(pdev, &dev_priv->drm);
> +	/*
> +	 * Disable the system suspend direct complete optimization, which can
> +	 * leave the device suspended skipping the driver's suspend handlers
> +	 * if the device was already runtime suspended. This is needed due to
> +	 * the difference in our runtime and system suspend sequence and
> +	 * becaue the HDA driver may require us to enable the audio power
> +	 * domain during system suspend.
> +	 */
> +	pci_resume_before_suspend(pdev, true);
>  
>  	ret = i915_driver_init_early(dev_priv, ent);
>  	if (ret < 0)
> -- 
> 2.5.0
> 

Re: [PATCH v2 2/2] drm/i915: Prevent the system suspend complete optimization

[Imre Deak]

On Mon, Apr 24, 2017 at 10:16:38PM +0200, Lukas Wunner wrote:
> On Mon, Apr 24, 2017 at 05:27:43PM +0300, Imre Deak wrote:
> > Since
> > 
> > commit bac2a909a096c9110525c18cbb8ce73c660d5f71
> > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Date:   Wed Jan 21 02:17:42 2015 +0100
> > 
> >     PCI / PM: Avoid resuming PCI devices during system suspend
> 
> This is not the commit you are looking for. :-)  See below.
> 
> 
> > PCI devices will default to allowing the system suspend complete
> > optimization where devices are not woken up during system suspend if
> > they were already runtime suspended. This however breaks the i915/HDA
> > drivers for two reasons:
> > 
> > - The i915 driver has system suspend specific steps that it needs to
> >   run, that bring the device to a different state than its runtime
> >   suspended state.
> > 
> > - The HDA driver's suspend handler requires power that it will request
> >   from the i915 driver's power domain handler. This in turn requires the
> >   i915 driver to runtime resume itself, but this won't be possible if the
> >   suspend complete optimization is in effect: in this case the i915
> >   runtime PM is disabled and trying to get an RPM reference returns
> >   -EACCESS.
> 
> Hm, sounds like something that needs to be solved with device_links.
> 
> 
> > 
> > Solve this by requiring the PCI/PM core to resume the device during
> > system suspend which in effect disables the suspend complete optimization.
> > 
> > One possibility for this bug staying hidden for so long is that the
> > optimization for a device is disabled if it's disabled for any of its
> > children devices. i915 may have a backlight device as its child which
> > doesn't support runtime PM and so doesn't allow the optimization either.
> > So if this backlight device got registered the bug stayed hidden.
> 
> No, the reason this hasn't popped up earlier is because direct_complete
> has only been enabled for DRM devices for a few months now, to be specific
> since
> 
>     commit d14d2a8453d650bea32a1c5271af1458cd283a0f
>     Author: Lukas Wunner <lukas@wunner.de>
>     Date:   Wed Jun 8 12:49:29 2016 +0200
> 
>     drm: Remove dev_pm_ops from drm_class
> 
> which landed in v4.8.

Right, this kept the optimization disabled even after bac2a909a096c91.
It did stay disabled on platforms with a backlight driver registered as
described above.

--Imre

> 
> (Sorry for not raising my voice earlier, this patch appeared on my radar
> just now.)
> 
> Kind regards,
> 
> Lukas
> 
> > 
> > Credits to Marta, Tomi and David who enabled pstore logging,
> > that caught one instance of this issue across a suspend/
> > resume-to-ram and Ville who rememberd that the optimization was enabled
> > for some devices at one point.
> > 
> > The first WARN triggered by the problem:
> > 
> > [ 6250.746445] WARNING: CPU: 2 PID: 17384 at drivers/gpu/drm/i915/intel_runtime_pm.c:2846 intel_runtime_pm_get+0x6b/0xd0 [i915]
> > [ 6250.746448] pm_runtime_get_sync() failed: -13
> > [ 6250.746451] Modules linked in: snd_hda_intel i915 vgem snd_hda_codec_hdmi x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul
> > snd_hda_codec_realtek snd_hda_codec_generic ghash_clmulni_intel e1000e snd_hda_codec snd_hwdep snd_hda_core ptp mei_me pps_core snd_pcm lpc_ich mei prime_
> > numbers i2c_hid i2c_designware_platform i2c_designware_core [last unloaded: i915]
> > [ 6250.746512] CPU: 2 PID: 17384 Comm: kworker/u8:0 Tainted: G     U  W       4.11.0-rc5-CI-CI_DRM_334+ #1
> > [ 6250.746515] Hardware name:                  /NUC5i5RYB, BIOS RYBDWi35.86A.0362.2017.0118.0940 01/18/2017
> > [ 6250.746521] Workqueue: events_unbound async_run_entry_fn
> > [ 6250.746525] Call Trace:
> > [ 6250.746530]  dump_stack+0x67/0x92
> > [ 6250.746536]  __warn+0xc6/0xe0
> > [ 6250.746542]  ? pci_restore_standard_config+0x40/0x40
> > [ 6250.746546]  warn_slowpath_fmt+0x46/0x50
> > [ 6250.746553]  ? __pm_runtime_resume+0x56/0x80
> > [ 6250.746584]  intel_runtime_pm_get+0x6b/0xd0 [i915]
> > [ 6250.746610]  intel_display_power_get+0x1b/0x40 [i915]
> > [ 6250.746646]  i915_audio_component_get_power+0x15/0x20 [i915]
> > [ 6250.746654]  snd_hdac_display_power+0xc8/0x110 [snd_hda_core]
> > [ 6250.746661]  azx_runtime_resume+0x218/0x280 [snd_hda_intel]
> > [ 6250.746667]  pci_pm_runtime_resume+0x76/0xa0
> > [ 6250.746672]  __rpm_callback+0xb4/0x1f0
> > [ 6250.746677]  ? pci_restore_standard_config+0x40/0x40
> > [ 6250.746682]  rpm_callback+0x1f/0x80
> > [ 6250.746686]  ? pci_restore_standard_config+0x40/0x40
> > [ 6250.746690]  rpm_resume+0x4ba/0x740
> > [ 6250.746698]  __pm_runtime_resume+0x49/0x80
> > [ 6250.746703]  pci_pm_suspend+0x57/0x140
> > [ 6250.746709]  dpm_run_callback+0x6f/0x330
> > [ 6250.746713]  ? pci_pm_freeze+0xe0/0xe0
> > [ 6250.746718]  __device_suspend+0xf9/0x370
> > [ 6250.746724]  ? dpm_watchdog_set+0x60/0x60
> > [ 6250.746730]  async_suspend+0x1a/0x90
> > [ 6250.746735]  async_run_entry_fn+0x34/0x160
> > [ 6250.746741]  process_one_work+0x1f2/0x6d0
> > [ 6250.746749]  worker_thread+0x49/0x4a0
> > [ 6250.746755]  kthread+0x107/0x140
> > [ 6250.746759]  ? process_one_work+0x6d0/0x6d0
> > [ 6250.746763]  ? kthread_create_on_node+0x40/0x40
> > [ 6250.746768]  ret_from_fork+0x2e/0x40
> > [ 6250.746778] ---[ end trace 102a62fd2160f5e6 ]---
> > 
> > v2:
> > - Use the new pci_dev->needs_resume flag, to avoid any overhead during
> >   the ->pm_prepare hook. (Rafael)
> > 
> > Fixes: bac2a909a096 ("PCI / PM: Avoid resuming PCI devices during system suspend")
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=100378
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 8be958f..dd7ce69 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1207,6 +1207,15 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  		goto out_free_priv;
> >  
> >  	pci_set_drvdata(pdev, &dev_priv->drm);
> > +	/*
> > +	 * Disable the system suspend direct complete optimization, which can
> > +	 * leave the device suspended skipping the driver's suspend handlers
> > +	 * if the device was already runtime suspended. This is needed due to
> > +	 * the difference in our runtime and system suspend sequence and
> > +	 * becaue the HDA driver may require us to enable the audio power
> > +	 * domain during system suspend.
> > +	 */
> > +	pci_resume_before_suspend(pdev, true);
> >  
> >  	ret = i915_driver_init_early(dev_priv, ent);
> >  	if (ret < 0)
> > -- 
> > 2.5.0
> > 

RE: [PATCH v2 2/2] drm/i915: Prevent the system suspend complete optimization

[Lofstedt, Marta]

Thanks, Imre

I have tested this and I confirm that it solves the pm_runtime_get_sync() failed: -13 and the issues that follow after.
This is also the root-cause in freedesktop bug 100770, which will be solved by your patch.


BR,
Marta
> -----Original Message-----
> From: Deak, Imre
> Sent: Tuesday, April 25, 2017 1:29 PM
> To: Lukas Wunner <lukas@wunner.de>
> Cc: intel-gfx@lists.freedesktop.org; Wysocki, Rafael J
> <rafael.j.wysocki@intel.com>; Lofstedt, Marta <marta.lofstedt@intel.com>;
> David Weinehall <david.weinehall@linux.intel.com>; Sarvela, Tomi P
> <tomi.p.sarvela@intel.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>;
> Kuoppala, Mika <mika.kuoppala@intel.com>; Chris Wilson <chris@chris-
> wilson.co.uk>; Takashi Iwai <tiwai@suse.de>; Bjorn Helgaas
> <bhelgaas@google.com>; linux-pci@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] drm/i915: Prevent the system suspend complete
> optimization
> 
> On Mon, Apr 24, 2017 at 10:16:38PM +0200, Lukas Wunner wrote:
> > On Mon, Apr 24, 2017 at 05:27:43PM +0300, Imre Deak wrote:
> > > Since
> > >
> > > commit bac2a909a096c9110525c18cbb8ce73c660d5f71
> > > Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Date:   Wed Jan 21 02:17:42 2015 +0100
> > >
> > >     PCI / PM: Avoid resuming PCI devices during system suspend
> >
> > This is not the commit you are looking for. :-)  See below.
> >
> >
> > > PCI devices will default to allowing the system suspend complete
> > > optimization where devices are not woken up during system suspend if
> > > they were already runtime suspended. This however breaks the
> > > i915/HDA drivers for two reasons:
> > >
> > > - The i915 driver has system suspend specific steps that it needs to
> > >   run, that bring the device to a different state than its runtime
> > >   suspended state.
> > >
> > > - The HDA driver's suspend handler requires power that it will request
> > >   from the i915 driver's power domain handler. This in turn requires the
> > >   i915 driver to runtime resume itself, but this won't be possible if the
> > >   suspend complete optimization is in effect: in this case the i915
> > >   runtime PM is disabled and trying to get an RPM reference returns
> > >   -EACCESS.
> >
> > Hm, sounds like something that needs to be solved with device_links.
> >
> >
> > >
> > > Solve this by requiring the PCI/PM core to resume the device during
> > > system suspend which in effect disables the suspend complete
> optimization.
> > >
> > > One possibility for this bug staying hidden for so long is that the
> > > optimization for a device is disabled if it's disabled for any of
> > > its children devices. i915 may have a backlight device as its child
> > > which doesn't support runtime PM and so doesn't allow the optimization
> either.
> > > So if this backlight device got registered the bug stayed hidden.
> >
> > No, the reason this hasn't popped up earlier is because
> > direct_complete has only been enabled for DRM devices for a few months
> > now, to be specific since
> >
> >     commit d14d2a8453d650bea32a1c5271af1458cd283a0f
> >     Author: Lukas Wunner <lukas@wunner.de>
> >     Date:   Wed Jun 8 12:49:29 2016 +0200
> >
> >     drm: Remove dev_pm_ops from drm_class
> >
> > which landed in v4.8.
> 
> Right, this kept the optimization disabled even after bac2a909a096c91.
> It did stay disabled on platforms with a backlight driver registered as
> described above.
> 
> --Imre
> 
> >
> > (Sorry for not raising my voice earlier, this patch appeared on my
> > radar just now.)
> >
> > Kind regards,
> >
> > Lukas
> >
> > >
> > > Credits to Marta, Tomi and David who enabled pstore logging, that
> > > caught one instance of this issue across a suspend/ resume-to-ram
> > > and Ville who rememberd that the optimization was enabled for some
> > > devices at one point.
> > >
> > > The first WARN triggered by the problem:
> > >
> > > [ 6250.746445] WARNING: CPU: 2 PID: 17384 at
> > > drivers/gpu/drm/i915/intel_runtime_pm.c:2846
> > > intel_runtime_pm_get+0x6b/0xd0 [i915] [ 6250.746448]
> > > pm_runtime_get_sync() failed: -13 [ 6250.746451] Modules linked in:
> > > snd_hda_intel i915 vgem snd_hda_codec_hdmi x86_pkg_temp_thermal
> intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul
> snd_hda_codec_realtek snd_hda_codec_generic ghash_clmulni_intel
> e1000e snd_hda_codec snd_hwdep snd_hda_core ptp mei_me pps_core
> snd_pcm lpc_ich mei prime_ numbers i2c_hid i2c_designware_platform
> i2c_designware_core [last unloaded: i915]
> > > [ 6250.746512] CPU: 2 PID: 17384 Comm: kworker/u8:0 Tainted: G     U  W
> 4.11.0-rc5-CI-CI_DRM_334+ #1
> > > [ 6250.746515] Hardware name:                  /NUC5i5RYB, BIOS
> RYBDWi35.86A.0362.2017.0118.0940 01/18/2017
> > > [ 6250.746521] Workqueue: events_unbound async_run_entry_fn [
> > > 6250.746525] Call Trace:
> > > [ 6250.746530]  dump_stack+0x67/0x92 [ 6250.746536]
> > > __warn+0xc6/0xe0 [ 6250.746542]  ?
> > > pci_restore_standard_config+0x40/0x40
> > > [ 6250.746546]  warn_slowpath_fmt+0x46/0x50 [ 6250.746553]  ?
> > > __pm_runtime_resume+0x56/0x80 [ 6250.746584]
> > > intel_runtime_pm_get+0x6b/0xd0 [i915] [ 6250.746610]
> > > intel_display_power_get+0x1b/0x40 [i915] [ 6250.746646]
> > > i915_audio_component_get_power+0x15/0x20 [i915] [ 6250.746654]
> > > snd_hdac_display_power+0xc8/0x110 [snd_hda_core] [ 6250.746661]
> > > azx_runtime_resume+0x218/0x280 [snd_hda_intel] [ 6250.746667]
> > > pci_pm_runtime_resume+0x76/0xa0 [ 6250.746672]
> > > __rpm_callback+0xb4/0x1f0 [ 6250.746677]  ?
> > > pci_restore_standard_config+0x40/0x40
> > > [ 6250.746682]  rpm_callback+0x1f/0x80 [ 6250.746686]  ?
> > > pci_restore_standard_config+0x40/0x40
> > > [ 6250.746690]  rpm_resume+0x4ba/0x740 [ 6250.746698]
> > > __pm_runtime_resume+0x49/0x80 [ 6250.746703]
> > > pci_pm_suspend+0x57/0x140 [ 6250.746709]
> > > dpm_run_callback+0x6f/0x330 [ 6250.746713]  ?
> > > pci_pm_freeze+0xe0/0xe0 [ 6250.746718]
> __device_suspend+0xf9/0x370
> > > [ 6250.746724]  ? dpm_watchdog_set+0x60/0x60 [ 6250.746730]
> > > async_suspend+0x1a/0x90 [ 6250.746735]
> > > async_run_entry_fn+0x34/0x160 [ 6250.746741]
> > > process_one_work+0x1f2/0x6d0 [ 6250.746749]
> > > worker_thread+0x49/0x4a0 [ 6250.746755]  kthread+0x107/0x140 [
> > > 6250.746759]  ? process_one_work+0x6d0/0x6d0 [ 6250.746763]  ?
> > > kthread_create_on_node+0x40/0x40 [ 6250.746768]
> > > ret_from_fork+0x2e/0x40 [ 6250.746778] ---[ end trace
> > > 102a62fd2160f5e6 ]---
> > >
> > > v2:
> > > - Use the new pci_dev->needs_resume flag, to avoid any overhead
> during
> > >   the ->pm_prepare hook. (Rafael)
> > >
> > > Fixes: bac2a909a096 ("PCI / PM: Avoid resuming PCI devices during
> > > system suspend")
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=100378
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: Marta Lofstedt <marta.lofstedt@intel.com>
> > > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > > Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Takashi Iwai <tiwai@suse.de>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: linux-pci@vger.kernel.org
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c index 8be958f..dd7ce69 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1207,6 +1207,15 @@ int i915_driver_load(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> > >  		goto out_free_priv;
> > >
> > >  	pci_set_drvdata(pdev, &dev_priv->drm);
> > > +	/*
> > > +	 * Disable the system suspend direct complete optimization,
> which can
> > > +	 * leave the device suspended skipping the driver's suspend
> handlers
> > > +	 * if the device was already runtime suspended. This is needed
> due to
> > > +	 * the difference in our runtime and system suspend sequence
> and
> > > +	 * becaue the HDA driver may require us to enable the audio
> power
> > > +	 * domain during system suspend.
> > > +	 */
> > > +	pci_resume_before_suspend(pdev, true);
> > >
> > >  	ret = i915_driver_init_early(dev_priv, ent);
> > >  	if (ret < 0)
> > > --
> > > 2.5.0
> > >

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

[Lukas Wunner]

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().

(2) Assign a struct dev_pm_domain to the GPU which overrides the PCI
    ->prepare hook to return 0 unconditionally.  See here for an example:
    http://lxr.free-electrons.com/source/drivers/gpu/vga/vga_switcheroo.c#L1056

It may be worth considering approach (1) (which is the simpler of the two)
in favor of this patch.  Because I'm not sure there are many drivers that
will make use of this change to the PCI core.

It's unfortunate that the return value of PCI drivers' ->prepare hooks
has totally different semantics than that of bus types, thereby
necessitating such workarounds. :-(

Best regards,

Lukas

> 
> Needed by the next patch fixing suspend/resume on i915.
> 
> Suggested by Rafael.
> 
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/pci/pci.c   | 3 ++-
>  include/linux/pci.h | 7 +++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7904d02..020f02d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2141,7 +2141,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
>  
>  	if (!pm_runtime_suspended(dev)
>  	    || pci_target_state(pci_dev) != pci_dev->current_state
> -	    || platform_pci_need_resume(pci_dev))
> +	    || platform_pci_need_resume(pci_dev)
> +	    || pci_dev->needs_resume)
>  		return false;
>  
>  	/*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e2d1a12..3fe00a6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -316,6 +316,9 @@ struct pci_dev {
>  	unsigned int	hotplug_user_indicators:1; /* SlotCtl indicators
>  						      controlled exclusively by
>  						      user sysfs */
> +	unsigned int	needs_resume:1;	/* Resume before calling the driver's
> +					   system suspend hooks, disabling the
> +					   direct_complete optimization. */
>  	unsigned int	d3_delay;	/* D3->D0 transition time in ms */
>  	unsigned int	d3cold_delay;	/* D3cold->D0 transition time in ms */
>  
> @@ -1113,6 +1116,10 @@ bool pci_check_pme_status(struct pci_dev *dev);
>  void pci_pme_wakeup_bus(struct pci_bus *bus);
>  void pci_d3cold_enable(struct pci_dev *dev);
>  void pci_d3cold_disable(struct pci_dev *dev);
> +static inline void pci_resume_before_suspend(struct pci_dev *dev, bool enable)
> +{
> +	dev->needs_resume = enable;
> +}
>  
>  static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
>  				  bool enable)
> -- 
> 2.5.0
> 

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

[Lukas Wunner]

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.

Thanks,

Lukas

-- >8 --
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5c089b3..6ef156b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1820,6 +1820,11 @@ void i915_reset(struct drm_i915_private *dev_priv)
 	goto wakeup;
 }
 
+static int i915_pm_prepare(struct device *kdev)
+{
+	pm_runtime_resume(kdev);
+}
+
 static int i915_pm_suspend(struct device *kdev)
 {
 	struct pci_dev *pdev = to_pci_dev(kdev);
@@ -2451,6 +2456,7 @@ static int intel_runtime_resume(struct device *kdev)
 	 * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND,
 	 * PMSG_RESUME]
 	 */
+	.prepare = i915_pm_prepare,
 	.suspend = i915_pm_suspend,
 	.suspend_late = i915_pm_suspend_late,
 	.resume_early = i915_pm_resume_early,

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

[Rafael J. Wysocki]

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.

Thanks,
Rafael

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

[Lukas Wunner]

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.

Thanks,

Lukas

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

[Imre Deak]

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.

--Imre

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

[Lukas Wunner]

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).

Thanks,

Lukas

Re: [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