[PATCH 0/6] drm/i915/pm: Clean up the hibernate vs. PCI D3 quirk

[PATCH 0/6] drm/i915/pm: Clean up the hibernate vs. PCI D3 quirk

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Attempt to make i915 rely more on the standard pci pm
code instead of hand rolling a bunch of
pci_save_state()+pci_set_power_state() stuff in the
driver.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-pci@vger.kernel.org

Ville Syrjälä (6):
  PCI/PM: Respect pci_dev->skip_bus_pm in the .poweroff() path
  drm/i915/pm: Hoist pci_save_state()+pci_set_power_state() to the end
    of pm _late() hook
  drm/i915/pm: Simplify pm hook documentation
  drm/i915/pm: Move the hibernate+D3 quirk stuff into noirq() pm hooks
  drm/i915/pm: Do pci_restore_state() in switcheroo resume hook
  drm/i915/pm: Use pci_dev->skip_bus_pm for hibernate vs. D3 workaround

 drivers/gpu/drm/i915/i915_driver.c | 121 +++++++++++++++++++----------
 drivers/pci/pci-driver.c           |  16 +++-
 2 files changed, 94 insertions(+), 43 deletions(-)

-- 
2.44.2

[PATCH 1/6] PCI/PM: Respect pci_dev->skip_bus_pm in the .poweroff() path

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On some older laptops i915 needs to leave the GPU in
D0 when hibernating the system, or else the BIOS
hangs somewhere. Currently that is achieved by calling
pci_save_state() ahead of time, which then skips the
whole pci_prepare_to_sleep() stuff.

It feels to me that this approach could lead to unintended
side effects as it causes the pci code to deviate from the
standard path in various ways. In order to keep i915
behaviour more standard it seems preferrable to use
pci_dev->skip_bus_pm here. Duplicate the relevant logic
from pci_pm_suspend_noirq() in pci_pm_poweroff_noirq().

It also looks like the current code is may put the parent
bridge into D3 despite leaving the device in D0. Though
perhaps the host bridge (which is where the integrated
GPU lives) always has subordinates, which would make
this a non-issue for i915. But maybe this could be a
problem for other devices. Utilizing skip_bus_pm will
make the behaviour of leaving the bridge in D0 a bit
more explicit if nothing else.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-pci@vger.kernel.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/pci/pci-driver.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index f412ef73a6e4..ef436895939c 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1142,6 +1142,8 @@ static int pci_pm_poweroff(struct device *dev)
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
+	pci_dev->skip_bus_pm = false;
+
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
 
@@ -1206,9 +1208,21 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 			return error;
 	}
 
-	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
+	if (!pci_dev->state_saved && !pci_dev->skip_bus_pm &&
+	    !pci_has_subordinate(pci_dev))
 		pci_prepare_to_sleep(pci_dev);
 
+	if (pci_dev->current_state == PCI_D0) {
+		pci_dev->skip_bus_pm = true;
+		/*
+		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
+		 * downstream device is in D0, so avoid changing the power state
+		 * of the parent bridge by setting the skip_bus_pm flag for it.
+		 */
+		if (pci_dev->bus->self)
+			pci_dev->bus->self->skip_bus_pm = true;
+	}
+
 	/*
 	 * The reason for doing this here is the same as for the analogous code
 	 * in pci_pm_suspend_noirq().
-- 
2.44.2

[PATCH 2/6] drm/i915/pm: Hoist pci_save_state()+pci_set_power_state() to the end of pm _late() hook

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

driver/pci does the pci_save_state()+pci_set_power_state() from the
_noirq() pm hooks. Move our manual calls (needed for the hibernate+D3
workaround with buggy BIOSes) towards that same point. We currently
have no _noirq() hooks, so end of _late() hooks is the best we can
do right now.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-pci@vger.kernel.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 6dc0104a3e36..9d557ff8adf5 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1015,7 +1015,6 @@ static int i915_drm_suspend(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_display *display = &dev_priv->display;
-	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	pci_power_t opregion_target_state;
 
 	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
@@ -1029,8 +1028,6 @@ static int i915_drm_suspend(struct drm_device *dev)
 		intel_display_driver_disable_user_access(dev_priv);
 	}
 
-	pci_save_state(pdev);
-
 	intel_display_driver_suspend(dev_priv);
 
 	intel_dp_mst_suspend(dev_priv);
@@ -1090,10 +1087,16 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 		drm_err(&dev_priv->drm, "Suspend complete failed: %d\n", ret);
 		intel_power_domains_resume(dev_priv);
 
-		goto out;
+		goto fail;
 	}
 
+	enable_rpm_wakeref_asserts(rpm);
+
+	if (!dev_priv->uncore.user_forcewake_count)
+		intel_runtime_pm_driver_release(rpm);
+
 	pci_disable_device(pdev);
+
 	/*
 	 * During hibernation on some platforms the BIOS may try to access
 	 * the device even though it's already in D3 and hang the machine. So
@@ -1105,11 +1108,17 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
 	 * Fujitsu FSC S7110
 	 * Acer Aspire 1830T
+	 *
+	 * pci_save_state() is needed to prevent driver/pci from
+	 * automagically putting the device into D3.
 	 */
+	pci_save_state(pdev);
 	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
 		pci_set_power_state(pdev, PCI_D3hot);
 
-out:
+	return 0;
+
+fail:
 	enable_rpm_wakeref_asserts(rpm);
 	if (!dev_priv->uncore.user_forcewake_count)
 		intel_runtime_pm_driver_release(rpm);
-- 
2.44.2

[PATCH 3/6] drm/i915/pm: Simplify pm hook documentation

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Stop spelling out each variant of the hook ("" vs. "_late" vs.
"_early") and just say eg. "@thaw*" to indicate all of them.
Avoids having to update the docs whenever we start/stop using
one of the variants.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-pci@vger.kernel.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 9d557ff8adf5..1e5abf72dfc4 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1644,18 +1644,18 @@ const struct dev_pm_ops i915_pm_ops = {
 
 	/*
 	 * S4 event handlers
-	 * @freeze, @freeze_late    : called (1) before creating the
-	 *                            hibernation image [PMSG_FREEZE] and
-	 *                            (2) after rebooting, before restoring
-	 *                            the image [PMSG_QUIESCE]
-	 * @thaw, @thaw_early       : called (1) after creating the hibernation
-	 *                            image, before writing it [PMSG_THAW]
-	 *                            and (2) after failing to create or
-	 *                            restore the image [PMSG_RECOVER]
-	 * @poweroff, @poweroff_late: called after writing the hibernation
-	 *                            image, before rebooting [PMSG_HIBERNATE]
-	 * @restore, @restore_early : called after rebooting and restoring the
-	 *                            hibernation image [PMSG_RESTORE]
+	 * @freeze*   : called (1) before creating the
+	 *              hibernation image [PMSG_FREEZE] and
+	 *              (2) after rebooting, before restoring
+	 *              the image [PMSG_QUIESCE]
+	 * @thaw*     : called (1) after creating the hibernation
+	 *              image, before writing it [PMSG_THAW]
+	 *              and (2) after failing to create or
+	 *              restore the image [PMSG_RECOVER]
+	 * @poweroff* : called after writing the hibernation
+	 *              image, before rebooting [PMSG_HIBERNATE]
+	 * @restore*  : called after rebooting and restoring the
+	 *              hibernation image [PMSG_RESTORE]
 	 */
 	.freeze = i915_pm_freeze,
 	.freeze_late = i915_pm_freeze_late,
-- 
2.44.2

[PATCH 4/6] drm/i915/pm: Move the hibernate+D3 quirk stuff into noirq() pm hooks

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If the driver doesn't call pci_save_state() driver/pci will
normally save+power manage the device from the _noirq() pm
hooks.

We can't let that happen as some old BIOSes fail to hibernate
when the device is in D3. However, we can get a bit closer to
the standard behaviour by doing our explicit pci_save_state()
and pci_set_power_state() stuff from driver provided _noirq()
hooks as well.

This results in a change of behaviur where we no longer go
into D3 at the end of .freeze_late(), so when it comes time
to .thaw() we'll already be in D0, and thus we can drop the
explicit pci_set_power_state(D0) call.

Presumable swictcheroo suspend will want to go into D3 so
call the _noirq() stuff from the switcheroo suspend hook,
and since we dropped the pci_set_power_state(D0) from
.resume_early() we'll need to add one back into the
swtcheroo resume hook.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-pci@vger.kernel.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 76 ++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 1e5abf72dfc4..fe7c34045794 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1097,6 +1097,21 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 
 	pci_disable_device(pdev);
 
+	return 0;
+
+fail:
+	enable_rpm_wakeref_asserts(rpm);
+	if (!dev_priv->uncore.user_forcewake_count)
+		intel_runtime_pm_driver_release(rpm);
+
+	return ret;
+}
+
+static int i915_drm_suspend_noirq(struct drm_device *dev, bool hibernation)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
+
 	/*
 	 * During hibernation on some platforms the BIOS may try to access
 	 * the device even though it's already in D3 and hang the machine. So
@@ -1117,13 +1132,6 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 		pci_set_power_state(pdev, PCI_D3hot);
 
 	return 0;
-
-fail:
-	enable_rpm_wakeref_asserts(rpm);
-	if (!dev_priv->uncore.user_forcewake_count)
-		intel_runtime_pm_driver_release(rpm);
-
-	return ret;
 }
 
 int i915_driver_suspend_switcheroo(struct drm_i915_private *i915,
@@ -1142,7 +1150,15 @@ int i915_driver_suspend_switcheroo(struct drm_i915_private *i915,
 	if (error)
 		return error;
 
-	return i915_drm_suspend_late(&i915->drm, false);
+	error = i915_drm_suspend_late(&i915->drm, false);
+	if (error)
+		return error;
+
+	error = i915_drm_suspend_noirq(&i915->drm, false);
+	if (error)
+		return error;
+
+	return 0;
 }
 
 static int i915_drm_resume(struct drm_device *dev)
@@ -1246,23 +1262,6 @@ static int i915_drm_resume_early(struct drm_device *dev)
 	 * similar so that power domains can be employed.
 	 */
 
-	/*
-	 * Note that we need to set the power state explicitly, since we
-	 * powered off the device during freeze and the PCI core won't power
-	 * it back up for us during thaw. Powering off the device during
-	 * freeze is not a hard requirement though, and during the
-	 * suspend/resume phases the PCI core makes sure we get here with the
-	 * device powered on. So in case we change our freeze logic and keep
-	 * the device powered we can also remove the following set power state
-	 * call.
-	 */
-	ret = pci_set_power_state(pdev, PCI_D0);
-	if (ret) {
-		drm_err(&dev_priv->drm,
-			"failed to set PCI D0 power state (%d)\n", ret);
-		return ret;
-	}
-
 	/*
 	 * Note that pci_enable_device() first enables any parent bridge
 	 * device and only then sets the power state for this device. The
@@ -1302,11 +1301,16 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
 int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
 {
+	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
 	int ret;
 
 	if (i915->drm.switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
+	ret = pci_set_power_state(pdev, PCI_D0);
+	if (ret)
+		return ret;
+
 	ret = i915_drm_resume_early(&i915->drm);
 	if (ret)
 		return ret;
@@ -1363,6 +1367,16 @@ static int i915_pm_suspend_late(struct device *kdev)
 	return i915_drm_suspend_late(&i915->drm, false);
 }
 
+static int i915_pm_suspend_noirq(struct device *kdev)
+{
+	struct drm_i915_private *i915 = kdev_to_i915(kdev);
+
+	if (i915->drm.switch_power_state == DRM_SWITCH_POWER_OFF)
+		return 0;
+
+	return i915_drm_suspend_noirq(&i915->drm, false);
+}
+
 static int i915_pm_poweroff_late(struct device *kdev)
 {
 	struct drm_i915_private *i915 = kdev_to_i915(kdev);
@@ -1373,6 +1387,16 @@ static int i915_pm_poweroff_late(struct device *kdev)
 	return i915_drm_suspend_late(&i915->drm, true);
 }
 
+static int i915_pm_poweroff_noirq(struct device *kdev)
+{
+	struct drm_i915_private *i915 = kdev_to_i915(kdev);
+
+	if (i915->drm.switch_power_state == DRM_SWITCH_POWER_OFF)
+		return 0;
+
+	return i915_drm_suspend_noirq(&i915->drm, true);
+}
+
 static int i915_pm_resume_early(struct device *kdev)
 {
 	struct drm_i915_private *i915 = kdev_to_i915(kdev);
@@ -1638,6 +1662,7 @@ const struct dev_pm_ops i915_pm_ops = {
 	.prepare = i915_pm_prepare,
 	.suspend = i915_pm_suspend,
 	.suspend_late = i915_pm_suspend_late,
+	.suspend_noirq = i915_pm_suspend_noirq,
 	.resume_early = i915_pm_resume_early,
 	.resume = i915_pm_resume,
 	.complete = i915_pm_complete,
@@ -1663,6 +1688,7 @@ const struct dev_pm_ops i915_pm_ops = {
 	.thaw = i915_pm_thaw,
 	.poweroff = i915_pm_suspend,
 	.poweroff_late = i915_pm_poweroff_late,
+	.poweroff_noirq = i915_pm_poweroff_noirq,
 	.restore_early = i915_pm_restore_early,
 	.restore = i915_pm_restore,
 
-- 
2.44.2

[PATCH 5/6] drm/i915/pm: Do pci_restore_state() in switcheroo resume hook

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Since this switcheroo stuff bypasses all the core pm we
have to manually manage the pci state. To that end add the
missing pci_restore_state() to the switcheroo resume hook.
We already have the pci_save_state() counterpart on the
suspend side.

I suppose this might not matter in practice as the
integrated GPU probably won't lose any state in D3,
and I presume there are no machines where this code
would come into play with an Intel discrete GPU.

Arguably none of this code should exist in the driver
in the first place, and instead the entire switcheroo
mechanism should be rewritten and properly integrated into
core pm code...

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-pci@vger.kernel.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index fe7c34045794..c3e7225ea1ba 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1311,6 +1311,8 @@ int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
 	if (ret)
 		return ret;
 
+	pci_restore_state(pdev);
+
 	ret = i915_drm_resume_early(&i915->drm);
 	if (ret)
 		return ret;
-- 
2.44.2

[PATCH 6/6] drm/i915/pm: Use pci_dev->skip_bus_pm for hibernate vs. D3 workaround

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On some older laptops we have to leave the device in D0
during hibernation, or else the BIOS just hangs and never
finishes the hibernation.

Currently we are achieving that by skipping the
pci_set_power_state(D3). However we also need to call
pci_save_state() ahead of time, or else
pci_pm_suspend_noirq() will do the pci_set_power_state(D3)
anyway.

This is all rather ugly, and might cause us to deviate from
standard pci pm behaviour in unknown ways since we always
call pci_save_state() for any kind of suspend operation.

Stop calling pci_save_state()+pci_set_power_state() entirely
(apart from the switcheroo paths) and instead set
pci_dev->skip_bus_pm=true to prevent the D3 during hibernation
on old machines. Apart from that we'll just let the normal
pci pm code take care of everything for us.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-pci@vger.kernel.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index c3e7225ea1ba..05948d00a874 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1123,13 +1123,9 @@ static int i915_drm_suspend_noirq(struct drm_device *dev, bool hibernation)
 	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
 	 * Fujitsu FSC S7110
 	 * Acer Aspire 1830T
-	 *
-	 * pci_save_state() is needed to prevent driver/pci from
-	 * automagically putting the device into D3.
 	 */
-	pci_save_state(pdev);
-	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
-		pci_set_power_state(pdev, PCI_D3hot);
+	if (hibernation && GRAPHICS_VER(dev_priv) < 6)
+		pdev->skip_bus_pm = true;
 
 	return 0;
 }
@@ -1137,6 +1133,7 @@ static int i915_drm_suspend_noirq(struct drm_device *dev, bool hibernation)
 int i915_driver_suspend_switcheroo(struct drm_i915_private *i915,
 				   pm_message_t state)
 {
+	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
 	int error;
 
 	if (drm_WARN_ON_ONCE(&i915->drm, state.event != PM_EVENT_SUSPEND &&
@@ -1158,6 +1155,9 @@ int i915_driver_suspend_switcheroo(struct drm_i915_private *i915,
 	if (error)
 		return error;
 
+	pci_save_state(pdev);
+	pci_set_power_state(pdev, PCI_D3hot);
+
 	return 0;
 }
 
-- 
2.44.2

Re: [PATCH 1/6] PCI/PM: Respect pci_dev->skip_bus_pm in the .poweroff() path

[Bjorn Helgaas]

On Wed, Sep 25, 2024 at 05:45:21PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On some older laptops i915 needs to leave the GPU in
> D0 when hibernating the system, or else the BIOS
> hangs somewhere. Currently that is achieved by calling
> pci_save_state() ahead of time, which then skips the
> whole pci_prepare_to_sleep() stuff.

IIUC this refers to pci_pm_suspend_noirq(), which has this:

  if (!pci_dev->state_saved) {
    pci_save_state(pci_dev);
    if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
      pci_prepare_to_sleep(pci_dev);
  }

Would be good if the commit log included the name of the function
where pci_prepare_to_sleep() is skipped.

If there's a general requirement to leave all devices in D0 when
hibernating, it would be nice to have have some documentation like an
ACPI spec reference.

Or if this is some i915-specific thing, maybe a pointer to history
like a lore or bugzilla reference.

> It feels to me that this approach could lead to unintended
> side effects as it causes the pci code to deviate from the
> standard path in various ways. In order to keep i915
> behaviour more standard it seems preferrable to use
> pci_dev->skip_bus_pm here. Duplicate the relevant logic
> from pci_pm_suspend_noirq() in pci_pm_poweroff_noirq().
> 
> It also looks like the current code is may put the parent
> bridge into D3 despite leaving the device in D0. Though
> perhaps the host bridge (which is where the integrated
> GPU lives) always has subordinates, which would make
> this a non-issue for i915. But maybe this could be a
> problem for other devices. Utilizing skip_bus_pm will
> make the behaviour of leaving the bridge in D0 a bit
> more explicit if nothing else.

s/is may/may/

Rewrap to fill 75 columns.  Could apply to all patches in the series.

Will need an ack from Rafael, author of:

  d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
  3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-idle")

which added .skip_bus_pm and its use in pci_pm_suspend_noirq().

IIUC this is a cleanup that doesn't fix any known problem?  The
overall diffstat doesn't make it look like a simplification, although
it might certainly be cleaner somehow:

> drivers/gpu/drm/i915/i915_driver.c | 121 +++++++++++++++++++----------
> drivers/pci/pci-driver.c           |  16 +++-
> 2 files changed, 94 insertions(+), 43 deletions(-)

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: linux-pci@vger.kernel.org
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/pci/pci-driver.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index f412ef73a6e4..ef436895939c 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1142,6 +1142,8 @@ static int pci_pm_poweroff(struct device *dev)
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  
> +	pci_dev->skip_bus_pm = false;
> +
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
>  
> @@ -1206,9 +1208,21 @@ static int pci_pm_poweroff_noirq(struct device *dev)
>  			return error;
>  	}
>  
> -	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
> +	if (!pci_dev->state_saved && !pci_dev->skip_bus_pm &&
> +	    !pci_has_subordinate(pci_dev))
>  		pci_prepare_to_sleep(pci_dev);
>  
> +	if (pci_dev->current_state == PCI_D0) {
> +		pci_dev->skip_bus_pm = true;
> +		/*
> +		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
> +		 * downstream device is in D0, so avoid changing the power state
> +		 * of the parent bridge by setting the skip_bus_pm flag for it.
> +		 */
> +		if (pci_dev->bus->self)
> +			pci_dev->bus->self->skip_bus_pm = true;
> +	}
> +
>  	/*
>  	 * The reason for doing this here is the same as for the analogous code
>  	 * in pci_pm_suspend_noirq().
> -- 
> 2.44.2
> 

Re: [PATCH 4/6] drm/i915/pm: Move the hibernate+D3 quirk stuff into noirq() pm hooks

[Bjorn Helgaas]

On Wed, Sep 25, 2024 at 05:45:24PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If the driver doesn't call pci_save_state() driver/pci will
> normally save+power manage the device from the _noirq() pm
> hooks.

drivers/pci.  Or maybe just mention the PM hook names specifically.

> We can't let that happen as some old BIOSes fail to hibernate
> when the device is in D3. However, we can get a bit closer to
> the standard behaviour by doing our explicit pci_save_state()
> and pci_set_power_state() stuff from driver provided _noirq()
> hooks as well.
> 
> This results in a change of behaviur where we no longer go
> into D3 at the end of .freeze_late(), so when it comes time
> to .thaw() we'll already be in D0, and thus we can drop the
> explicit pci_set_power_state(D0) call.

s/behaviur/behaviour/

> Presumable swictcheroo suspend will want to go into D3 so
> call the _noirq() stuff from the switcheroo suspend hook,
> and since we dropped the pci_set_power_state(D0) from
> .resume_early() we'll need to add one back into the
> swtcheroo resume hook.

s/swictcheroo/switcheroo/
s/swtcheroo/switcheroo/

Or maybe just use the actual function names here too.  That saves
time for me, at least, because it points me at exactly where to look.

Bjorn

Re: [PATCH 6/6] drm/i915/pm: Use pci_dev->skip_bus_pm for hibernate vs. D3 workaround

[Bjorn Helgaas]

On Wed, Sep 25, 2024 at 05:45:26PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On some older laptops we have to leave the device in D0
> during hibernation, or else the BIOS just hangs and never
> finishes the hibernation.
> 
> Currently we are achieving that by skipping the
> pci_set_power_state(D3). However we also need to call
> pci_save_state() ahead of time, or else
> pci_pm_suspend_noirq() will do the pci_set_power_state(D3)
> anyway.
> 
> This is all rather ugly, and might cause us to deviate from
> standard pci pm behaviour in unknown ways since we always
> call pci_save_state() for any kind of suspend operation.
> 
> Stop calling pci_save_state()+pci_set_power_state() entirely
> (apart from the switcheroo paths) and instead set
> pci_dev->skip_bus_pm=true to prevent the D3 during hibernation
> on old machines. Apart from that we'll just let the normal
> pci pm code take care of everything for us.
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: linux-pci@vger.kernel.org
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index c3e7225ea1ba..05948d00a874 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1123,13 +1123,9 @@ static int i915_drm_suspend_noirq(struct drm_device *dev, bool hibernation)
>  	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
>  	 * Fujitsu FSC S7110
>  	 * Acer Aspire 1830T
> -	 *
> -	 * pci_save_state() is needed to prevent driver/pci from
> -	 * automagically putting the device into D3.
>  	 */
> -	pci_save_state(pdev);
> -	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
> -		pci_set_power_state(pdev, PCI_D3hot);
> +	if (hibernation && GRAPHICS_VER(dev_priv) < 6)
> +		pdev->skip_bus_pm = true;

.skip_bus_pm was previously strictly internal to
drivers/pci/pci-driver.c.  Not sure about using it outside
drivers/pci/; would want Rafael to chime in.

>  	return 0;
>  }
> @@ -1137,6 +1133,7 @@ static int i915_drm_suspend_noirq(struct drm_device *dev, bool hibernation)
>  int i915_driver_suspend_switcheroo(struct drm_i915_private *i915,
>  				   pm_message_t state)
>  {
> +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
>  	int error;
>  
>  	if (drm_WARN_ON_ONCE(&i915->drm, state.event != PM_EVENT_SUSPEND &&
> @@ -1158,6 +1155,9 @@ int i915_driver_suspend_switcheroo(struct drm_i915_private *i915,
>  	if (error)
>  		return error;
>  
> +	pci_save_state(pdev);
> +	pci_set_power_state(pdev, PCI_D3hot);
> +
>  	return 0;
>  }
>  
> -- 
> 2.44.2
> 

Re: [PATCH 2/6] drm/i915/pm: Hoist pci_save_state()+pci_set_power_state() to the end of pm _late() hook

[Rodrigo Vivi]

On Wed, Sep 25, 2024 at 05:45:22PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> driver/pci does the pci_save_state()+pci_set_power_state() from the
> _noirq() pm hooks. Move our manual calls (needed for the hibernate+D3
> workaround with buggy BIOSes) towards that same point. We currently
> have no _noirq() hooks, so end of _late() hooks is the best we can
> do right now.
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: linux-pci@vger.kernel.org
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 6dc0104a3e36..9d557ff8adf5 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1015,7 +1015,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_display *display = &dev_priv->display;
> -	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  	pci_power_t opregion_target_state;
>  
>  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> @@ -1029,8 +1028,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  		intel_display_driver_disable_user_access(dev_priv);
>  	}
>  
> -	pci_save_state(pdev);
> -
>  	intel_display_driver_suspend(dev_priv);
>  
>  	intel_dp_mst_suspend(dev_priv);
> @@ -1090,10 +1087,16 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  		drm_err(&dev_priv->drm, "Suspend complete failed: %d\n", ret);
>  		intel_power_domains_resume(dev_priv);
>  
> -		goto out;
> +		goto fail;
>  	}
>  
> +	enable_rpm_wakeref_asserts(rpm);
> +
> +	if (!dev_priv->uncore.user_forcewake_count)
> +		intel_runtime_pm_driver_release(rpm);
> +

why do we need this?
probably deserves a separate patch?

>  	pci_disable_device(pdev);
> +
>  	/*
>  	 * During hibernation on some platforms the BIOS may try to access
>  	 * the device even though it's already in D3 and hang the machine. So
> @@ -1105,11 +1108,17 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
>  	 * Fujitsu FSC S7110
>  	 * Acer Aspire 1830T
> +	 *
> +	 * pci_save_state() is needed to prevent driver/pci from
> +	 * automagically putting the device into D3.
>  	 */

I'm still not convinced that this would automagically prevent the D3,
specially in this part of the code.

I would prefer to simply remove this call, or keep it and move it
here to be consistent with other drivers, but also add the restore
portion of it for consistency and alignment...

> +	pci_save_state(pdev);
>  	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
>  		pci_set_power_state(pdev, PCI_D3hot);
>  
> -out:
> +	return 0;
> +
> +fail:
>  	enable_rpm_wakeref_asserts(rpm);
>  	if (!dev_priv->uncore.user_forcewake_count)
>  		intel_runtime_pm_driver_release(rpm);
> -- 
> 2.44.2
> 

Re: [PATCH 3/6] drm/i915/pm: Simplify pm hook documentation

[Rodrigo Vivi]

On Wed, Sep 25, 2024 at 05:45:23PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Stop spelling out each variant of the hook ("" vs. "_late" vs.
> "_early") and just say eg. "@thaw*" to indicate all of them.
> Avoids having to update the docs whenever we start/stop using
> one of the variants.

That or simply remove them all and refer only to the pm documentation?
"Entering Hibernation" of Documentation/driver-api/pm/devices.rst

> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: linux-pci@vger.kernel.org
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 9d557ff8adf5..1e5abf72dfc4 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1644,18 +1644,18 @@ const struct dev_pm_ops i915_pm_ops = {
>  
>  	/*
>  	 * S4 event handlers
> -	 * @freeze, @freeze_late    : called (1) before creating the
> -	 *                            hibernation image [PMSG_FREEZE] and
> -	 *                            (2) after rebooting, before restoring
> -	 *                            the image [PMSG_QUIESCE]
> -	 * @thaw, @thaw_early       : called (1) after creating the hibernation
> -	 *                            image, before writing it [PMSG_THAW]
> -	 *                            and (2) after failing to create or
> -	 *                            restore the image [PMSG_RECOVER]
> -	 * @poweroff, @poweroff_late: called after writing the hibernation
> -	 *                            image, before rebooting [PMSG_HIBERNATE]
> -	 * @restore, @restore_early : called after rebooting and restoring the
> -	 *                            hibernation image [PMSG_RESTORE]
> +	 * @freeze*   : called (1) before creating the
> +	 *              hibernation image [PMSG_FREEZE] and
> +	 *              (2) after rebooting, before restoring
> +	 *              the image [PMSG_QUIESCE]
> +	 * @thaw*     : called (1) after creating the hibernation
> +	 *              image, before writing it [PMSG_THAW]
> +	 *              and (2) after failing to create or
> +	 *              restore the image [PMSG_RECOVER]
> +	 * @poweroff* : called after writing the hibernation
> +	 *              image, before rebooting [PMSG_HIBERNATE]
> +	 * @restore*  : called after rebooting and restoring the
> +	 *              hibernation image [PMSG_RESTORE]
>  	 */
>  	.freeze = i915_pm_freeze,
>  	.freeze_late = i915_pm_freeze_late,
> -- 
> 2.44.2
> 

Re: [PATCH 5/6] drm/i915/pm: Do pci_restore_state() in switcheroo resume hook

[Rodrigo Vivi]

On Wed, Sep 25, 2024 at 05:45:25PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Since this switcheroo stuff bypasses all the core pm we
> have to manually manage the pci state. To that end add the
> missing pci_restore_state() to the switcheroo resume hook.
> We already have the pci_save_state() counterpart on the
> suspend side.
> 
> I suppose this might not matter in practice as the
> integrated GPU probably won't lose any state in D3,
> and I presume there are no machines where this code
> would come into play with an Intel discrete GPU.
> 
> Arguably none of this code should exist in the driver
> in the first place, and instead the entire switcheroo
> mechanism should be rewritten and properly integrated into
> core pm code...
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: linux-pci@vger.kernel.org
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index fe7c34045794..c3e7225ea1ba 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1311,6 +1311,8 @@ int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
>  	if (ret)
>  		return ret;
>  
> +	pci_restore_state(pdev);

then why not simply call that inside the resume, for a better alignment
with the save counterpart?

> +
>  	ret = i915_drm_resume_early(&i915->drm);
>  	if (ret)
>  		return ret;
> -- 
> 2.44.2
> 

Re: [PATCH 5/6] drm/i915/pm: Do pci_restore_state() in switcheroo resume hook

[Ville Syrjälä]

On Thu, Sep 26, 2024 at 10:48:41AM -0400, Rodrigo Vivi wrote:
> On Wed, Sep 25, 2024 at 05:45:25PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Since this switcheroo stuff bypasses all the core pm we
> > have to manually manage the pci state. To that end add the
> > missing pci_restore_state() to the switcheroo resume hook.
> > We already have the pci_save_state() counterpart on the
> > suspend side.
> > 
> > I suppose this might not matter in practice as the
> > integrated GPU probably won't lose any state in D3,
> > and I presume there are no machines where this code
> > would come into play with an Intel discrete GPU.
> > 
> > Arguably none of this code should exist in the driver
> > in the first place, and instead the entire switcheroo
> > mechanism should be rewritten and properly integrated into
> > core pm code...
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index fe7c34045794..c3e7225ea1ba 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1311,6 +1311,8 @@ int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
> >  	if (ret)
> >  		return ret;
> >  
> > +	pci_restore_state(pdev);
> 
> then why not simply call that inside the resume, for a better alignment
> with the save counterpart?

This is switcheroo resume. And the counterpart is in switcheroo suspend.

For the core pm hooks I'm getting rid of both save and restore.

> 
> > +
> >  	ret = i915_drm_resume_early(&i915->drm);
> >  	if (ret)
> >  		return ret;
> > -- 
> > 2.44.2
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/6] drm/i915/pm: Simplify pm hook documentation

[Ville Syrjälä]

On Thu, Sep 26, 2024 at 10:45:44AM -0400, Rodrigo Vivi wrote:
> On Wed, Sep 25, 2024 at 05:45:23PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Stop spelling out each variant of the hook ("" vs. "_late" vs.
> > "_early") and just say eg. "@thaw*" to indicate all of them.
> > Avoids having to update the docs whenever we start/stop using
> > one of the variants.
> 
> That or simply remove them all and refer only to the pm documentation?
> "Entering Hibernation" of Documentation/driver-api/pm/devices.rst

That's not very succinct. Having a better quick overview
of the whole situation might still be nice. 

> 
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index 9d557ff8adf5..1e5abf72dfc4 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1644,18 +1644,18 @@ const struct dev_pm_ops i915_pm_ops = {
> >  
> >  	/*
> >  	 * S4 event handlers
> > -	 * @freeze, @freeze_late    : called (1) before creating the
> > -	 *                            hibernation image [PMSG_FREEZE] and
> > -	 *                            (2) after rebooting, before restoring
> > -	 *                            the image [PMSG_QUIESCE]
> > -	 * @thaw, @thaw_early       : called (1) after creating the hibernation
> > -	 *                            image, before writing it [PMSG_THAW]
> > -	 *                            and (2) after failing to create or
> > -	 *                            restore the image [PMSG_RECOVER]
> > -	 * @poweroff, @poweroff_late: called after writing the hibernation
> > -	 *                            image, before rebooting [PMSG_HIBERNATE]
> > -	 * @restore, @restore_early : called after rebooting and restoring the
> > -	 *                            hibernation image [PMSG_RESTORE]
> > +	 * @freeze*   : called (1) before creating the
> > +	 *              hibernation image [PMSG_FREEZE] and
> > +	 *              (2) after rebooting, before restoring
> > +	 *              the image [PMSG_QUIESCE]
> > +	 * @thaw*     : called (1) after creating the hibernation
> > +	 *              image, before writing it [PMSG_THAW]
> > +	 *              and (2) after failing to create or
> > +	 *              restore the image [PMSG_RECOVER]
> > +	 * @poweroff* : called after writing the hibernation
> > +	 *              image, before rebooting [PMSG_HIBERNATE]
> > +	 * @restore*  : called after rebooting and restoring the
> > +	 *              hibernation image [PMSG_RESTORE]
> >  	 */
> >  	.freeze = i915_pm_freeze,
> >  	.freeze_late = i915_pm_freeze_late,
> > -- 
> > 2.44.2
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/6] drm/i915/pm: Hoist pci_save_state()+pci_set_power_state() to the end of pm _late() hook

[Ville Syrjälä]

On Thu, Sep 26, 2024 at 10:43:21AM -0400, Rodrigo Vivi wrote:
> On Wed, Sep 25, 2024 at 05:45:22PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > driver/pci does the pci_save_state()+pci_set_power_state() from the
> > _noirq() pm hooks. Move our manual calls (needed for the hibernate+D3
> > workaround with buggy BIOSes) towards that same point. We currently
> > have no _noirq() hooks, so end of _late() hooks is the best we can
> > do right now.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index 6dc0104a3e36..9d557ff8adf5 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1015,7 +1015,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_display *display = &dev_priv->display;
> > -	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> >  	pci_power_t opregion_target_state;
> >  
> >  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > @@ -1029,8 +1028,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  		intel_display_driver_disable_user_access(dev_priv);
> >  	}
> >  
> > -	pci_save_state(pdev);
> > -
> >  	intel_display_driver_suspend(dev_priv);
> >  
> >  	intel_dp_mst_suspend(dev_priv);
> > @@ -1090,10 +1087,16 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> >  		drm_err(&dev_priv->drm, "Suspend complete failed: %d\n", ret);
> >  		intel_power_domains_resume(dev_priv);
> >  
> > -		goto out;
> > +		goto fail;
> >  	}
> >  
> > +	enable_rpm_wakeref_asserts(rpm);
> > +
> > +	if (!dev_priv->uncore.user_forcewake_count)
> > +		intel_runtime_pm_driver_release(rpm);
> > +
> 
> why do we need this?
> probably deserves a separate patch?

It was there already.

> 
> >  	pci_disable_device(pdev);
> > +
> >  	/*
> >  	 * During hibernation on some platforms the BIOS may try to access
> >  	 * the device even though it's already in D3 and hang the machine. So
> > @@ -1105,11 +1108,17 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> >  	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
> >  	 * Fujitsu FSC S7110
> >  	 * Acer Aspire 1830T
> > +	 *
> > +	 * pci_save_state() is needed to prevent driver/pci from
> > +	 * automagically putting the device into D3.
> >  	 */
> 
> I'm still not convinced that this would automagically prevent the D3,
> specially in this part of the code.

You need to read pci_pm_poweroff_noirq()

> 
> I would prefer to simply remove this call, or keep it and move it
> here to be consistent with other drivers, but also add the restore
> portion of it for consistency and alignment...
> 
> > +	pci_save_state(pdev);
> >  	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
> >  		pci_set_power_state(pdev, PCI_D3hot);
> >  
> > -out:
> > +	return 0;
> > +
> > +fail:
> >  	enable_rpm_wakeref_asserts(rpm);
> >  	if (!dev_priv->uncore.user_forcewake_count)
> >  		intel_runtime_pm_driver_release(rpm);
> > -- 
> > 2.44.2
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/6] PCI/PM: Respect pci_dev->skip_bus_pm in the .poweroff() path

[Ville Syrjälä]

On Wed, Sep 25, 2024 at 02:28:42PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 25, 2024 at 05:45:21PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On some older laptops i915 needs to leave the GPU in
> > D0 when hibernating the system, or else the BIOS
> > hangs somewhere. Currently that is achieved by calling
> > pci_save_state() ahead of time, which then skips the
> > whole pci_prepare_to_sleep() stuff.
> 
> IIUC this refers to pci_pm_suspend_noirq(), which has this:
> 
>   if (!pci_dev->state_saved) {
>     pci_save_state(pci_dev);
>     if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
>       pci_prepare_to_sleep(pci_dev);
>   }
> 
> Would be good if the commit log included the name of the function
> where pci_prepare_to_sleep() is skipped.

Sure, I can amend the commit msg.

> 
> If there's a general requirement to leave all devices in D0 when
> hibernating, it would be nice to have have some documentation like an
> ACPI spec reference.

No, IIRC the ACPI spec even says that you must (or at least
should) put devices into D3. But the buggy BIOS on some old
laptops keels over when you do that. Hence we need this quirk.

> Or if this is some i915-specific thing, maybe a pointer to history
> like a lore or bugzilla reference.

The two relevant commits I can find are:

commit 54875571bbfd ("drm/i915: apply the PCI_D0/D3 hibernation
workaround everywhere on pre GEN6")
commit ab3be73fa7b4 ("drm/i915: gen4: work around hang during
hibernation")

> 
> > It feels to me that this approach could lead to unintended
> > side effects as it causes the pci code to deviate from the
> > standard path in various ways. In order to keep i915
> > behaviour more standard it seems preferrable to use
> > pci_dev->skip_bus_pm here. Duplicate the relevant logic
> > from pci_pm_suspend_noirq() in pci_pm_poweroff_noirq().
> > 
> > It also looks like the current code is may put the parent
> > bridge into D3 despite leaving the device in D0. Though
> > perhaps the host bridge (which is where the integrated
> > GPU lives) always has subordinates, which would make
> > this a non-issue for i915. But maybe this could be a
> > problem for other devices. Utilizing skip_bus_pm will
> > make the behaviour of leaving the bridge in D0 a bit
> > more explicit if nothing else.
> 
> s/is may/may/
> 
> Rewrap to fill 75 columns.  Could apply to all patches in the series.
> 
> Will need an ack from Rafael, author of:
> 
>   d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
>   3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-idle")
> 
> which added .skip_bus_pm and its use in pci_pm_suspend_noirq().
> 
> IIUC this is a cleanup that doesn't fix any known problem?  The
> overall diffstat doesn't make it look like a simplification, although
> it might certainly be cleaner somehow:

My main concern is that using pci_save_state() might cause the pci
code to deviate from the normal path in more ways than just skipping
the D0->D3 transition. And then we might end up constantly chasing
after driver/pci changes in order to match its behaviour.

Not to mention that having the pci_save_state() in the driver code
is clearly confusing a bunch of our developers.

> 
> > drivers/gpu/drm/i915/i915_driver.c | 121 +++++++++++++++++++----------
> > drivers/pci/pci-driver.c           |  16 +++-
> > 2 files changed, 94 insertions(+), 43 deletions(-)
> 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/pci/pci-driver.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index f412ef73a6e4..ef436895939c 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1142,6 +1142,8 @@ static int pci_pm_poweroff(struct device *dev)
> >  	struct pci_dev *pci_dev = to_pci_dev(dev);
> >  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> >  
> > +	pci_dev->skip_bus_pm = false;
> > +
> >  	if (pci_has_legacy_pm_support(pci_dev))
> >  		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
> >  
> > @@ -1206,9 +1208,21 @@ static int pci_pm_poweroff_noirq(struct device *dev)
> >  			return error;
> >  	}
> >  
> > -	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
> > +	if (!pci_dev->state_saved && !pci_dev->skip_bus_pm &&
> > +	    !pci_has_subordinate(pci_dev))
> >  		pci_prepare_to_sleep(pci_dev);
> >  
> > +	if (pci_dev->current_state == PCI_D0) {
> > +		pci_dev->skip_bus_pm = true;
> > +		/*
> > +		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
> > +		 * downstream device is in D0, so avoid changing the power state
> > +		 * of the parent bridge by setting the skip_bus_pm flag for it.
> > +		 */
> > +		if (pci_dev->bus->self)
> > +			pci_dev->bus->self->skip_bus_pm = true;
> > +	}
> > +
> >  	/*
> >  	 * The reason for doing this here is the same as for the analogous code
> >  	 * in pci_pm_suspend_noirq().
> > -- 
> > 2.44.2
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/6] drm/i915/pm: Simplify pm hook documentation

[Rodrigo Vivi]

On Thu, Sep 26, 2024 at 06:38:56PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 26, 2024 at 10:45:44AM -0400, Rodrigo Vivi wrote:
> > On Wed, Sep 25, 2024 at 05:45:23PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Stop spelling out each variant of the hook ("" vs. "_late" vs.
> > > "_early") and just say eg. "@thaw*" to indicate all of them.
> > > Avoids having to update the docs whenever we start/stop using
> > > one of the variants.
> > 
> > That or simply remove them all and refer only to the pm documentation?
> > "Entering Hibernation" of Documentation/driver-api/pm/devices.rst
> 
> That's not very succinct. Having a better quick overview
> of the whole situation might still be nice. 

Fair enough.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> > 
> > > 
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: linux-pci@vger.kernel.org
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_driver.c | 24 ++++++++++++------------
> > >  1 file changed, 12 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > index 9d557ff8adf5..1e5abf72dfc4 100644
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -1644,18 +1644,18 @@ const struct dev_pm_ops i915_pm_ops = {
> > >  
> > >  	/*
> > >  	 * S4 event handlers
> > > -	 * @freeze, @freeze_late    : called (1) before creating the
> > > -	 *                            hibernation image [PMSG_FREEZE] and
> > > -	 *                            (2) after rebooting, before restoring
> > > -	 *                            the image [PMSG_QUIESCE]
> > > -	 * @thaw, @thaw_early       : called (1) after creating the hibernation
> > > -	 *                            image, before writing it [PMSG_THAW]
> > > -	 *                            and (2) after failing to create or
> > > -	 *                            restore the image [PMSG_RECOVER]
> > > -	 * @poweroff, @poweroff_late: called after writing the hibernation
> > > -	 *                            image, before rebooting [PMSG_HIBERNATE]
> > > -	 * @restore, @restore_early : called after rebooting and restoring the
> > > -	 *                            hibernation image [PMSG_RESTORE]
> > > +	 * @freeze*   : called (1) before creating the
> > > +	 *              hibernation image [PMSG_FREEZE] and
> > > +	 *              (2) after rebooting, before restoring
> > > +	 *              the image [PMSG_QUIESCE]
> > > +	 * @thaw*     : called (1) after creating the hibernation
> > > +	 *              image, before writing it [PMSG_THAW]
> > > +	 *              and (2) after failing to create or
> > > +	 *              restore the image [PMSG_RECOVER]
> > > +	 * @poweroff* : called after writing the hibernation
> > > +	 *              image, before rebooting [PMSG_HIBERNATE]
> > > +	 * @restore*  : called after rebooting and restoring the
> > > +	 *              hibernation image [PMSG_RESTORE]
> > >  	 */
> > >  	.freeze = i915_pm_freeze,
> > >  	.freeze_late = i915_pm_freeze_late,
> > > -- 
> > > 2.44.2
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 5/6] drm/i915/pm: Do pci_restore_state() in switcheroo resume hook

[Rodrigo Vivi]

On Thu, Sep 26, 2024 at 06:36:13PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 26, 2024 at 10:48:41AM -0400, Rodrigo Vivi wrote:
> > On Wed, Sep 25, 2024 at 05:45:25PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Since this switcheroo stuff bypasses all the core pm we
> > > have to manually manage the pci state. To that end add the
> > > missing pci_restore_state() to the switcheroo resume hook.
> > > We already have the pci_save_state() counterpart on the
> > > suspend side.
> > > 
> > > I suppose this might not matter in practice as the
> > > integrated GPU probably won't lose any state in D3,
> > > and I presume there are no machines where this code
> > > would come into play with an Intel discrete GPU.
> > > 
> > > Arguably none of this code should exist in the driver
> > > in the first place, and instead the entire switcheroo
> > > mechanism should be rewritten and properly integrated into
> > > core pm code...
> > > 
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: linux-pci@vger.kernel.org
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_driver.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > index fe7c34045794..c3e7225ea1ba 100644
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -1311,6 +1311,8 @@ int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	pci_restore_state(pdev);
> > 
> > then why not simply call that inside the resume, for a better alignment
> > with the save counterpart?
> 
> This is switcheroo resume. And the counterpart is in switcheroo suspend.
> 
> For the core pm hooks I'm getting rid of both save and restore.

With this I totally agree. I probably missed something when just
reading the patches... I had to apply them all to see the final version.

So,

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> > 
> > > +
> > >  	ret = i915_drm_resume_early(&i915->drm);
> > >  	if (ret)
> > >  		return ret;
> > > -- 
> > > 2.44.2
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 2/6] drm/i915/pm: Hoist pci_save_state()+pci_set_power_state() to the end of pm _late() hook

[Rodrigo Vivi]

On Thu, Sep 26, 2024 at 06:41:17PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 26, 2024 at 10:43:21AM -0400, Rodrigo Vivi wrote:
> > On Wed, Sep 25, 2024 at 05:45:22PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > driver/pci does the pci_save_state()+pci_set_power_state() from the
> > > _noirq() pm hooks. Move our manual calls (needed for the hibernate+D3
> > > workaround with buggy BIOSes) towards that same point. We currently
> > > have no _noirq() hooks, so end of _late() hooks is the best we can
> > > do right now.
> > > 
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: linux-pci@vger.kernel.org
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_driver.c | 19 ++++++++++++++-----
> > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > index 6dc0104a3e36..9d557ff8adf5 100644
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -1015,7 +1015,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	struct intel_display *display = &dev_priv->display;
> > > -	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> > >  	pci_power_t opregion_target_state;
> > >  
> > >  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> > > @@ -1029,8 +1028,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> > >  		intel_display_driver_disable_user_access(dev_priv);
> > >  	}
> > >  
> > > -	pci_save_state(pdev);
> > > -
> > >  	intel_display_driver_suspend(dev_priv);
> > >  
> > >  	intel_dp_mst_suspend(dev_priv);
> > > @@ -1090,10 +1087,16 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> > >  		drm_err(&dev_priv->drm, "Suspend complete failed: %d\n", ret);
> > >  		intel_power_domains_resume(dev_priv);
> > >  
> > > -		goto out;
> > > +		goto fail;
> > >  	}
> > >  
> > > +	enable_rpm_wakeref_asserts(rpm);
> > > +
> > > +	if (!dev_priv->uncore.user_forcewake_count)
> > > +		intel_runtime_pm_driver_release(rpm);
> > > +
> > 
> > why do we need this?
> > probably deserves a separate patch?
> 
> It was there already.

yes, but it was done in a later stage and now it is called earlier
in the regular path. I believe it deserves a 'why' that is not clear
in this commit message.

> 
> > 
> > >  	pci_disable_device(pdev);
> > > +
> > >  	/*
> > >  	 * During hibernation on some platforms the BIOS may try to access
> > >  	 * the device even though it's already in D3 and hang the machine. So
> > > @@ -1105,11 +1108,17 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> > >  	 * Lenovo Thinkpad X301, X61s, X60, T60, X41
> > >  	 * Fujitsu FSC S7110
> > >  	 * Acer Aspire 1830T
> > > +	 *
> > > +	 * pci_save_state() is needed to prevent driver/pci from
> > > +	 * automagically putting the device into D3.
> > >  	 */
> > 
> > I'm still not convinced that this would automagically prevent the D3,
> > specially in this part of the code.
> 
> You need to read pci_pm_poweroff_noirq()
> 
> > 
> > I would prefer to simply remove this call, or keep it and move it
> > here to be consistent with other drivers, but also add the restore
> > portion of it for consistency and alignment...
> > 
> > > +	pci_save_state(pdev);
> > >  	if (!(hibernation && GRAPHICS_VER(dev_priv) < 6))
> > >  		pci_set_power_state(pdev, PCI_D3hot);
> > >  
> > > -out:
> > > +	return 0;
> > > +
> > > +fail:
> > >  	enable_rpm_wakeref_asserts(rpm);
> > >  	if (!dev_priv->uncore.user_forcewake_count)
> > >  		intel_runtime_pm_driver_release(rpm);
> > > -- 
> > > 2.44.2
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 1/6] PCI/PM: Respect pci_dev->skip_bus_pm in the .poweroff() path

[Bjorn Helgaas]

On Thu, Sep 26, 2024 at 07:03:16PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 25, 2024 at 02:28:42PM -0500, Bjorn Helgaas wrote:
> > On Wed, Sep 25, 2024 at 05:45:21PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > On some older laptops i915 needs to leave the GPU in
> > > D0 when hibernating the system, or else the BIOS
> > > hangs somewhere. Currently that is achieved by calling
> > > pci_save_state() ahead of time, which then skips the
> > > whole pci_prepare_to_sleep() stuff.

> > If there's a general requirement to leave all devices in D0 when
> > hibernating, it would be nice to have have some documentation like an
> > ACPI spec reference.
> 
> No, IIRC the ACPI spec even says that you must (or at least
> should) put devices into D3. But the buggy BIOS on some old
> laptops keels over when you do that. Hence we need this quirk.

Can we include a reference to this part of the ACPI spec and some
details on which laptops have this issue?

I'm a little bit wary of changing the PCI core in a generic-looking
way on the basis of some unspecified buggy old BIOS.  That feels like
something we're likely to break in the future.

> > Or if this is some i915-specific thing, maybe a pointer to history
> > like a lore or bugzilla reference.
> 
> The two relevant commits I can find are:
> 
> commit 54875571bbfd ("drm/i915: apply the PCI_D0/D3 hibernation
> workaround everywhere on pre GEN6")
> commit ab3be73fa7b4 ("drm/i915: gen4: work around hang during
> hibernation")

Thanks, this feels like important history to include somewhere.

> > IIUC this is a cleanup that doesn't fix any known problem?  The
> > overall diffstat doesn't make it look like a simplification, although
> > it might certainly be cleaner somehow:
> 
> My main concern is that using pci_save_state() might cause the pci
> code to deviate from the normal path in more ways than just skipping
> the D0->D3 transition. And then we might end up constantly chasing
> after driver/pci changes in order to match its behaviour.
> 
> Not to mention that having the pci_save_state() in the driver code
> is clearly confusing a bunch of our developers.

I'm all in favor of removing pci_save_state() from drivers when
possible.  I take it that this doesn't fix a functional issue.

Bjorn

Re: [PATCH 1/6] PCI/PM: Respect pci_dev->skip_bus_pm in the .poweroff() path

[Ville Syrjälä]

On Mon, Sep 30, 2024 at 02:50:09PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 26, 2024 at 07:03:16PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 25, 2024 at 02:28:42PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 25, 2024 at 05:45:21PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > On some older laptops i915 needs to leave the GPU in
> > > > D0 when hibernating the system, or else the BIOS
> > > > hangs somewhere. Currently that is achieved by calling
> > > > pci_save_state() ahead of time, which then skips the
> > > > whole pci_prepare_to_sleep() stuff.
> 
> > > If there's a general requirement to leave all devices in D0 when
> > > hibernating, it would be nice to have have some documentation like an
> > > ACPI spec reference.
> > 
> > No, IIRC the ACPI spec even says that you must (or at least
> > should) put devices into D3. But the buggy BIOS on some old
> > laptops keels over when you do that. Hence we need this quirk.
> 
> Can we include a reference to this part of the ACPI spec

It's been years since I looked at that, but a quick trawl of the
ACPI 6.3 spec (what I had at hand) landed me this:

"7.4.2.5 System \_S4 State
 ...
 - Devices states are compatible with the current Power Resource states. In
   other words, all devices are in the D3 state when the system state is S4."

"16.1.6 Transitioning from the Working to the Sleeping State
 ...
 4. OSPM places all device drivers into their respective Dx state. If the
    device is enabled for wake, it enters the Dx state associated with the
    wake capability. If the device is not enabled to wake the system, it
    enters the D3 state."

> and some
> details on which laptops have this issue?

The known models are listed in a comment in i915 code (added
in the two mentioned commits), though I suspect there are
probably more because we couldn't find any obvious pattern
why these known models are affected.

> 
> I'm a little bit wary of changing the PCI core in a generic-looking
> way on the basis of some unspecified buggy old BIOS.  That feels like
> something we're likely to break in the future.
> 
> > > Or if this is some i915-specific thing, maybe a pointer to history
> > > like a lore or bugzilla reference.
> > 
> > The two relevant commits I can find are:
> > 
> > commit 54875571bbfd ("drm/i915: apply the PCI_D0/D3 hibernation
> > workaround everywhere on pre GEN6")
> > commit ab3be73fa7b4 ("drm/i915: gen4: work around hang during
> > hibernation")
> 
> Thanks, this feels like important history to include somewhere.
> 
> > > IIUC this is a cleanup that doesn't fix any known problem?  The
> > > overall diffstat doesn't make it look like a simplification, although
> > > it might certainly be cleaner somehow:
> > 
> > My main concern is that using pci_save_state() might cause the pci
> > code to deviate from the normal path in more ways than just skipping
> > the D0->D3 transition. And then we might end up constantly chasing
> > after driver/pci changes in order to match its behaviour.
> > 
> > Not to mention that having the pci_save_state() in the driver code
> > is clearly confusing a bunch of our developers.
> 
> I'm all in favor of removing pci_save_state() from drivers when
> possible.  I take it that this doesn't fix a functional issue.

No known issue so far.

But we are probably going to add eg. PME support at some point,
and the fact that pci_save_state() also skips pci_enable_wake()
makes me think we'd have to hand roll a lot more stuff in the
driver code if we keep using the pci_save_state(). Though I 
suppose we could do the pci_save_state() only on those
old systems which won't have PME anyway. 

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/6] PCI/PM: Respect pci_dev->skip_bus_pm in the .poweroff() path

[Ville Syrjälä]

On Wed, Sep 25, 2024 at 02:28:42PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 25, 2024 at 05:45:21PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On some older laptops i915 needs to leave the GPU in
> > D0 when hibernating the system, or else the BIOS
> > hangs somewhere. Currently that is achieved by calling
> > pci_save_state() ahead of time, which then skips the
> > whole pci_prepare_to_sleep() stuff.
> 
> IIUC this refers to pci_pm_suspend_noirq(), which has this:
> 
>   if (!pci_dev->state_saved) {
>     pci_save_state(pci_dev);
>     if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev))
>       pci_prepare_to_sleep(pci_dev);
>   }
> 
> Would be good if the commit log included the name of the function
> where pci_prepare_to_sleep() is skipped.
> 
> If there's a general requirement to leave all devices in D0 when
> hibernating, it would be nice to have have some documentation like an
> ACPI spec reference.
> 
> Or if this is some i915-specific thing, maybe a pointer to history
> like a lore or bugzilla reference.
> 
> > It feels to me that this approach could lead to unintended
> > side effects as it causes the pci code to deviate from the
> > standard path in various ways. In order to keep i915
> > behaviour more standard it seems preferrable to use
> > pci_dev->skip_bus_pm here. Duplicate the relevant logic
> > from pci_pm_suspend_noirq() in pci_pm_poweroff_noirq().
> > 
> > It also looks like the current code is may put the parent
> > bridge into D3 despite leaving the device in D0. Though
> > perhaps the host bridge (which is where the integrated
> > GPU lives) always has subordinates, which would make
> > this a non-issue for i915. But maybe this could be a
> > problem for other devices. Utilizing skip_bus_pm will
> > make the behaviour of leaving the bridge in D0 a bit
> > more explicit if nothing else.
> 
> s/is may/may/
> 
> Rewrap to fill 75 columns.  Could apply to all patches in the series.
> 
> Will need an ack from Rafael, author of:
> 
>   d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
>   3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-idle")
> 
> which added .skip_bus_pm and its use in pci_pm_suspend_noirq().

Rafael, any thoughts on this stuff?

> 
> IIUC this is a cleanup that doesn't fix any known problem?  The
> overall diffstat doesn't make it look like a simplification, although
> it might certainly be cleaner somehow:
> 
> > drivers/gpu/drm/i915/i915_driver.c | 121 +++++++++++++++++++----------
> > drivers/pci/pci-driver.c           |  16 +++-
> > 2 files changed, 94 insertions(+), 43 deletions(-)
> 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/pci/pci-driver.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index f412ef73a6e4..ef436895939c 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1142,6 +1142,8 @@ static int pci_pm_poweroff(struct device *dev)
> >  	struct pci_dev *pci_dev = to_pci_dev(dev);
> >  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> >  
> > +	pci_dev->skip_bus_pm = false;
> > +
> >  	if (pci_has_legacy_pm_support(pci_dev))
> >  		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
> >  
> > @@ -1206,9 +1208,21 @@ static int pci_pm_poweroff_noirq(struct device *dev)
> >  			return error;
> >  	}
> >  
> > -	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
> > +	if (!pci_dev->state_saved && !pci_dev->skip_bus_pm &&
> > +	    !pci_has_subordinate(pci_dev))
> >  		pci_prepare_to_sleep(pci_dev);
> >  
> > +	if (pci_dev->current_state == PCI_D0) {
> > +		pci_dev->skip_bus_pm = true;
> > +		/*
> > +		 * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
> > +		 * downstream device is in D0, so avoid changing the power state
> > +		 * of the parent bridge by setting the skip_bus_pm flag for it.
> > +		 */
> > +		if (pci_dev->bus->self)
> > +			pci_dev->bus->self->skip_bus_pm = true;
> > +	}
> > +
> >  	/*
> >  	 * The reason for doing this here is the same as for the analogous code
> >  	 * in pci_pm_suspend_noirq().
> > -- 
> > 2.44.2
> > 

-- 
Ville Syrjälä
Intel