[PATCH 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers

[PATCH 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers

[Imre Deak]

The first part of the patchset (1-6) fixes an S4 bug on VLV introduced
recently. The rest unifies the various S3/S4 handlers, which are in
practice the same. The only real difference - besides some unused code -
is that during S3 suspend we disable the PCI device whereas across an S4
freeze/thaw we keep it enabled. Given that we disable everything else
anyway, we can just as well disable the PCI device there too. Doing so
allows us to handle S3 suspend/resume and S4 freeze/thaw/restore/
power-off the same way, simplifying/clarifying things quite a bit.

I smoke tested this on BDW, HSW, VLV (BYT-M), IVB, GEN45.

Imre Deak (16):
  drm/i915: vlv: fix gunit HW state corruption during S4 suspend
  drm/i915: remove dead code from legacy suspend handler
  drm/i915: factor out i915_drm_suspend_late
  drm/i915: unify legacy S3 suspend and S4 freeze handlers
  drm/i915: propagate error from legacy resume handler
  drm/i915: vlv: fix switcheroo/legacy suspend/resume
  drm/i915: fix S4 suspend while switcheroo state is off
  drm/i915: remove unused restore_gtt_mappings optimization during
    suspend
  drm/i915: check for GT faults during S3 resume and S4 restore too
  drm/i915: enable output polling during S4 thaw
  drm/i915: disable/re-enable PCI device around S4 freeze/thaw
  drm/i915: unify S3 and S4 suspend/resume handlers
  drm/i915: sanitize suspend/resume helper function names
  drm/i915: add poweroff_late handler
  drm/i915: unify switcheroo and legacy suspend/resume handlers
  drm/i915: add comments on what stage a given PM handler is called

 drivers/gpu/drm/i915/i915_dma.c |   4 +-
 drivers/gpu/drm/i915/i915_drv.c | 185 +++++++++++++++-------------------------
 drivers/gpu/drm/i915/i915_drv.h |   4 +-
 3 files changed, 74 insertions(+), 119 deletions(-)

-- 
1.8.4

[PATCH 01/16] drm/i915: vlv: fix gunit HW state corruption during S4 suspend

[Imre Deak]

During S4 freeze we don't call intel_suspend_complete(), which would
save the gunit HW state, but during S4 thaw/restore events we call
intel_resume_prepare() which restores it, thus ending up in a corrupted
HW state.

Fix this by calling intel_suspend_complete() from the corresponding
freeze_late event handler.

The issue was introduced in
commit 016970beb05da6285c2f3ed2bee1c676cb75972e
Author: Sagar Kamble <sagar.a.kamble@intel.com>
Date:   Wed Aug 13 23:07:06 2014 +0530

CC: Sagar Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b8bd008..2365875 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -987,6 +987,15 @@ static int i915_pm_freeze(struct device *dev)
 	return i915_drm_freeze(drm_dev);
 }
 
+static int i915_pm_freeze_late(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct drm_i915_private *dev_priv = drm_dev->dev_private;
+
+	return intel_suspend_complete(dev_priv);
+}
+
 static int i915_pm_thaw_early(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
@@ -1571,6 +1580,7 @@ static const struct dev_pm_ops i915_pm_ops = {
 	.resume_early = i915_pm_resume_early,
 	.resume = i915_pm_resume,
 	.freeze = i915_pm_freeze,
+	.freeze_late = i915_pm_freeze_late,
 	.thaw_early = i915_pm_thaw_early,
 	.thaw = i915_pm_thaw,
 	.poweroff = i915_pm_poweroff,
-- 
1.8.4

[PATCH 02/16] drm/i915: remove dead code from legacy suspend handler

[Imre Deak]

The legacy DRM suspend logic (effective in UMS) doesn't handle any S4 thaw
events so we don't need to care about it either. Only S3 suspend and S4
freeze events are handled. Leave an assert behind to be sure.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2365875..f36e2d2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -618,9 +618,9 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
 		return -ENODEV;
 	}
 
-	if (state.event == PM_EVENT_PRETHAW)
-		return 0;
-
+	if (WARN_ON_ONCE(state.event != PM_EVENT_SUSPEND &&
+			 state.event != PM_EVENT_FREEZE))
+		return -EINVAL;
 
 	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
-- 
1.8.4

[PATCH 03/16] drm/i915: factor out i915_drm_suspend_late

[Imre Deak]

This is needed by an upcoming patch fixing the switcheroo/legacy suspend
paths.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f36e2d2..7fa34bd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -608,6 +608,25 @@ static int i915_drm_freeze(struct drm_device *dev)
 	return 0;
 }
 
+static int i915_drm_suspend_late(struct drm_device *drm_dev)
+{
+	struct drm_i915_private *dev_priv = drm_dev->dev_private;
+	int ret;
+
+	ret = intel_suspend_complete(dev_priv);
+
+	if (ret) {
+		DRM_ERROR("Suspend complete failed: %d\n", ret);
+
+		return ret;
+	}
+
+	pci_disable_device(drm_dev->pdev);
+	pci_set_power_state(drm_dev->pdev, PCI_D3hot);
+
+	return 0;
+}
+
 int i915_suspend(struct drm_device *dev, pm_message_t state)
 {
 	int error;
@@ -931,8 +950,6 @@ static int i915_pm_suspend_late(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
-	struct drm_i915_private *dev_priv = drm_dev->dev_private;
-	int ret;
 
 	/*
 	 * We have a suspedn ordering issue with the snd-hda driver also
@@ -946,16 +963,7 @@ static int i915_pm_suspend_late(struct device *dev)
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
-	ret = intel_suspend_complete(dev_priv);
-
-	if (ret)
-		DRM_ERROR("Suspend complete failed: %d\n", ret);
-	else {
-		pci_disable_device(pdev);
-		pci_set_power_state(pdev, PCI_D3hot);
-	}
-
-	return ret;
+	return i915_drm_suspend_late(drm_dev);
 }
 
 static int i915_pm_resume_early(struct device *dev)
-- 
1.8.4

[PATCH 04/16] drm/i915: unify legacy S3 suspend and S4 freeze handlers

[Imre Deak]

i915_suspend() is called from the DRM legacy S3 suspend/S4 freeze paths
and the switcheroo suspend path. For switcheroo we only ever need to
perform a full suspend (PM_EVENT_SUSPEND) and for the DRM legacy path
we can handle the S4 freeze (PM_EVENT_FREEZE) the same way as S3
suspend. The only difference atm between suspend and freeze is that
during freeze we don't disable the PCI device, but there is no reason
why we can't do so. So unify the two cases to reduce complexity.

Note that for the DRM legacy case the thaw event is not handled, so
we disable the display before creating the hibernation image and it
won't get re-enabled until reboot. We could fix this leaving the
display enabled for the image creation/writing (if we care enough
about UMS), but this can be done as a follow-up.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7fa34bd..bf4ff5e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -648,11 +648,9 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
 	if (error)
 		return error;
 
-	if (state.event == PM_EVENT_SUSPEND) {
-		/* Shut down the device */
-		pci_disable_device(dev->pdev);
-		pci_set_power_state(dev->pdev, PCI_D3hot);
-	}
+	/* Shut down the device */
+	pci_disable_device(dev->pdev);
+	pci_set_power_state(dev->pdev, PCI_D3hot);
 
 	return 0;
 }
-- 
1.8.4

[PATCH 05/16] drm/i915: propagate error from legacy resume handler

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index bf4ff5e..89b63fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -789,10 +789,13 @@ int i915_resume(struct drm_device *dev)
 
 static int i915_resume_legacy(struct drm_device *dev)
 {
-	i915_resume_early(dev);
-	i915_resume(dev);
+	int ret;
 
-	return 0;
+	ret = i915_resume_early(dev);
+	if (ret)
+		return ret;
+
+	return i915_resume(dev);
 }
 
 /**
-- 
1.8.4

[PATCH 06/16] drm/i915: vlv: fix switcheroo/legacy suspend/resume

[Imre Deak]

During switcheroo/legacy suspend we don't call the suspend_late handler but
when resuming afterwards we call resume_early. This happened to work so far,
since suspend_late only disabled the PCI device. This changed in

commit 016970beb05da6285c2f3ed2bee1c676cb75972e
Author: Sagar Kamble <sagar.a.kamble@intel.com>
Date:   Wed Aug 13 23:07:06 2014 +0530

    drm/i915: Sharing platform specific sequence between runtime and system susp

after which we also saved/restored the VLV Gunit HW state in
suspend_late/resume_early. So now since we don't save the state during
suspend a following resume will restore a corrupted state.

Fix this by calling the suspend_late handler during both switcheroo and
legacy suspend.

CC: Sagar Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 89b63fc..ca74d6d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -648,11 +648,7 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
 	if (error)
 		return error;
 
-	/* Shut down the device */
-	pci_disable_device(dev->pdev);
-	pci_set_power_state(dev->pdev, PCI_D3hot);
-
-	return 0;
+	return i915_drm_suspend_late(dev);
 }
 
 static int i915_drm_thaw_early(struct drm_device *dev)
@@ -769,7 +765,7 @@ static int i915_resume_early(struct drm_device *dev)
 	return i915_drm_thaw_early(dev);
 }
 
-int i915_resume(struct drm_device *dev)
+static int i915_drm_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
@@ -795,7 +791,12 @@ static int i915_resume_legacy(struct drm_device *dev)
 	if (ret)
 		return ret;
 
-	return i915_resume(dev);
+	return i915_drm_resume(dev);
+}
+
+int i915_resume(struct drm_device *dev)
+{
+	return i915_resume_legacy(dev);
 }
 
 /**
@@ -980,7 +981,7 @@ static int i915_pm_resume(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
 
-	return i915_resume(drm_dev);
+	return i915_drm_resume(drm_dev);
 }
 
 static int i915_pm_freeze(struct device *dev)
-- 
1.8.4

[PATCH 07/16] drm/i915: fix S4 suspend while switcheroo state is off

[Imre Deak]

If the device is suspended already through the switcheroo interface we
shouldn't suspend it again. We have the corresponding check for S3
suspend already, add it for S4 freeze and poweroff too.

Note that there is still the problem that the resume path should also
check for the switcheroo-off state and keep the device disabled or in
case of S4 disable it. That is a separate issue, which is not addressed
in this patchset.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 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 ca74d6d..a3addc2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -994,6 +994,9 @@ static int i915_pm_freeze(struct device *dev)
 		return -ENODEV;
 	}
 
+	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+		return 0;
+
 	return i915_drm_freeze(drm_dev);
 }
 
@@ -1003,6 +1006,9 @@ static int i915_pm_freeze_late(struct device *dev)
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
 	struct drm_i915_private *dev_priv = drm_dev->dev_private;
 
+	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+		return 0;
+
 	return intel_suspend_complete(dev_priv);
 }
 
@@ -1027,6 +1033,9 @@ static int i915_pm_poweroff(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
 
+	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+		return 0;
+
 	return i915_drm_freeze(drm_dev);
 }
 
-- 
1.8.4

[PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend

[Imre Deak]

The logic to skip restoring GTT mappings was added to speed up
suspend/resume, but not on old GENs where not restoring them caused
problems. The check for old GENs is based on the existance of OpRegion,
but this doesn't work since opregion is initialized only after
the check. So we end up always restoring the mappings.

On my BYT - which has OpRegion - skipping restoring the mappings during
suspend doesn't work, I get a GPU hang after resume. Also the logic of
when to allow the optimization during S4 is reversed: we should allow it
during S4 thaw but not during S4 restore, but atm we have it the other
way around in the code.

Since correctness wins over optimal code and since the optimization
wasn't used anyway I decided not to try to fix it at this point, but
just remove it. This allows us to unify the S3 and S4 handlers in the
following patches.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a3addc2..5e25283 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -667,12 +667,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)
 	return ret;
 }
 
-static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
+static int __i915_drm_thaw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
-	    restore_gtt_mappings) {
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		mutex_lock(&dev->struct_mutex);
 		i915_gem_restore_gtt_mappings(dev);
 		mutex_unlock(&dev->struct_mutex);
@@ -740,7 +739,7 @@ static int i915_drm_thaw(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		i915_check_and_clear_faults(dev);
 
-	return __i915_drm_thaw(dev, true);
+	return __i915_drm_thaw(dev);
 }
 
 static int i915_resume_early(struct drm_device *dev)
@@ -767,15 +766,9 @@ static int i915_resume_early(struct drm_device *dev)
 
 static int i915_drm_resume(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	/*
-	 * Platforms with opregion should have sane BIOS, older ones (gen3 and
-	 * earlier) need to restore the GTT mappings since the BIOS might clear
-	 * all our scratch PTEs.
-	 */
-	ret = __i915_drm_thaw(dev, !dev_priv->opregion.header);
+	ret = __i915_drm_thaw(dev);
 	if (ret)
 		return ret;
 
-- 
1.8.4

[PATCH 09/16] drm/i915: check for GT faults during S3 resume and S4 restore too

[Imre Deak]

Checking for GT faults is not specific in any way to S4 thaw, so do it
also during S3 resume and S4 restore. This allows us to unify the
handlers for these events in an upcoming patch.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5e25283..d0fee60 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -672,6 +672,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		i915_check_and_clear_faults(dev);
+
 		mutex_lock(&dev->struct_mutex);
 		i915_gem_restore_gtt_mappings(dev);
 		mutex_unlock(&dev->struct_mutex);
@@ -736,9 +738,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 
 static int i915_drm_thaw(struct drm_device *dev)
 {
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		i915_check_and_clear_faults(dev);
-
 	return __i915_drm_thaw(dev);
 }
 
-- 
1.8.4

[PATCH 10/16] drm/i915: enable output polling during S4 thaw

[Imre Deak]

To avoid processing hotplug events we disable connector polling for the
duration of S3 suspend. We also disable it for S4 freeze, and keep it
disabled after S4 thaw. This won't prevent though hotplug processing,
since we re-enable interrupts anyway. There is also no need to prevent
it at that time, since we reinitialize everything during thaw, so the
device is in a consistent state. So to simplify things enable polling
during thaw, which will allow us to handle S4 thaw the same way as S3
resume in an upcoming patch.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d0fee60..ea8dda7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -733,6 +733,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
 
 	intel_opregion_notify_adapter(dev, PCI_D0);
 
+	drm_kms_helper_poll_enable(dev);
+
 	return 0;
 }
 
@@ -765,14 +767,7 @@ static int i915_resume_early(struct drm_device *dev)
 
 static int i915_drm_resume(struct drm_device *dev)
 {
-	int ret;
-
-	ret = __i915_drm_thaw(dev);
-	if (ret)
-		return ret;
-
-	drm_kms_helper_poll_enable(dev);
-	return 0;
+	return __i915_drm_thaw(dev);
 }
 
 static int i915_resume_legacy(struct drm_device *dev)
-- 
1.8.4

[PATCH 11/16] drm/i915: disable/re-enable PCI device around S4 freeze/thaw

[Imre Deak]

We already disable everything during S4 freeze, except the PCI device
itself. There is no reason why we couldn't disable that too and doing
so allows us to unify these handlers in the next patch with the
corresponding S3 suspend/resume handlers.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ea8dda7..0ff5e92 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -991,12 +991,11 @@ static int i915_pm_freeze_late(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
-	struct drm_i915_private *dev_priv = drm_dev->dev_private;
 
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
-	return intel_suspend_complete(dev_priv);
+	return i915_drm_suspend_late(drm_dev);
 }
 
 static int i915_pm_thaw_early(struct device *dev)
@@ -1004,7 +1003,7 @@ static int i915_pm_thaw_early(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
 
-	return i915_drm_thaw_early(drm_dev);
+	return i915_resume_early(drm_dev);
 }
 
 static int i915_pm_thaw(struct device *dev)
-- 
1.8.4

[PATCH 12/16] drm/i915: unify S3 and S4 suspend/resume handlers

[Imre Deak]

The S3 and S4 events are now handled the same way internally, there is no
need to keep separate wrapper functions around them. Simply reuse the
suspend/resume versions everywhere.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 98 +++++++----------------------------------
 1 file changed, 17 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0ff5e92..44c1c03 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -651,22 +651,6 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
 	return i915_drm_suspend_late(dev);
 }
 
-static int i915_drm_thaw_early(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
-
-	ret = intel_resume_prepare(dev_priv, false);
-	if (ret)
-		DRM_ERROR("Resume prepare failed: %d,Continuing resume\n", ret);
-
-	intel_uncore_early_sanitize(dev, true);
-	intel_uncore_sanitize(dev);
-	intel_power_domains_init_hw(dev_priv);
-
-	return ret;
-}
-
 static int __i915_drm_thaw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -738,13 +722,11 @@ static int __i915_drm_thaw(struct drm_device *dev)
 	return 0;
 }
 
-static int i915_drm_thaw(struct drm_device *dev)
-{
-	return __i915_drm_thaw(dev);
-}
-
 static int i915_resume_early(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
 	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
@@ -762,7 +744,15 @@ static int i915_resume_early(struct drm_device *dev)
 
 	pci_set_master(dev->pdev);
 
-	return i915_drm_thaw_early(dev);
+	ret = intel_resume_prepare(dev_priv, false);
+	if (ret)
+		DRM_ERROR("Resume prepare failed: %d,Continuing resume\n", ret);
+
+	intel_uncore_early_sanitize(dev, true);
+	intel_uncore_sanitize(dev);
+	intel_power_domains_init_hw(dev_priv);
+
+	return ret;
 }
 
 static int i915_drm_resume(struct drm_device *dev)
@@ -971,60 +961,6 @@ static int i915_pm_resume(struct device *dev)
 	return i915_drm_resume(drm_dev);
 }
 
-static int i915_pm_freeze(struct device *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
-	if (!drm_dev || !drm_dev->dev_private) {
-		dev_err(dev, "DRM not initialized, aborting suspend.\n");
-		return -ENODEV;
-	}
-
-	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
-		return 0;
-
-	return i915_drm_freeze(drm_dev);
-}
-
-static int i915_pm_freeze_late(struct device *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
-	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
-		return 0;
-
-	return i915_drm_suspend_late(drm_dev);
-}
-
-static int i915_pm_thaw_early(struct device *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
-	return i915_resume_early(drm_dev);
-}
-
-static int i915_pm_thaw(struct device *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
-	return i915_drm_thaw(drm_dev);
-}
-
-static int i915_pm_poweroff(struct device *dev)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
-	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
-		return 0;
-
-	return i915_drm_freeze(drm_dev);
-}
-
 static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	hsw_enable_pc8(dev_priv);
@@ -1584,11 +1520,11 @@ static const struct dev_pm_ops i915_pm_ops = {
 	.suspend_late = i915_pm_suspend_late,
 	.resume_early = i915_pm_resume_early,
 	.resume = i915_pm_resume,
-	.freeze = i915_pm_freeze,
-	.freeze_late = i915_pm_freeze_late,
-	.thaw_early = i915_pm_thaw_early,
-	.thaw = i915_pm_thaw,
-	.poweroff = i915_pm_poweroff,
+	.freeze = i915_pm_suspend,
+	.freeze_late = i915_pm_suspend_late,
+	.thaw_early = i915_pm_resume_early,
+	.thaw = i915_pm_resume,
+	.poweroff = i915_pm_suspend,
 	.restore_early = i915_pm_resume_early,
 	.restore = i915_pm_resume,
 	.runtime_suspend = intel_runtime_suspend,
-- 
1.8.4

[PATCH 13/16] drm/i915: sanitize suspend/resume helper function names

[Imre Deak]

By now the S4 freeze/thaw and S3 suspend/resume events are handled the
same way, so we can rename the freeze/thaw internal helpers to
suspend/resume accordingly to make clearer what the helpers do. Also
rename i915_resume_early to i915_drm_resume_early aligning it with the
rest of the helper names.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44c1c03..f6157a1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -532,7 +532,7 @@ static int intel_suspend_complete(struct drm_i915_private *dev_priv);
 static int intel_resume_prepare(struct drm_i915_private *dev_priv,
 				bool rpm_resume);
 
-static int i915_drm_freeze(struct drm_device *dev)
+static int i915_drm_suspend(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
@@ -644,14 +644,14 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
 	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
-	error = i915_drm_freeze(dev);
+	error = i915_drm_suspend(dev);
 	if (error)
 		return error;
 
 	return i915_drm_suspend_late(dev);
 }
 
-static int __i915_drm_thaw(struct drm_device *dev)
+static int i915_drm_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -722,7 +722,7 @@ static int __i915_drm_thaw(struct drm_device *dev)
 	return 0;
 }
 
-static int i915_resume_early(struct drm_device *dev)
+static int i915_drm_resume_early(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
@@ -755,16 +755,11 @@ static int i915_resume_early(struct drm_device *dev)
 	return ret;
 }
 
-static int i915_drm_resume(struct drm_device *dev)
-{
-	return __i915_drm_thaw(dev);
-}
-
 static int i915_resume_legacy(struct drm_device *dev)
 {
 	int ret;
 
-	ret = i915_resume_early(dev);
+	ret = i915_drm_resume_early(dev);
 	if (ret)
 		return ret;
 
@@ -922,7 +917,7 @@ static int i915_pm_suspend(struct device *dev)
 	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
 		return 0;
 
-	return i915_drm_freeze(drm_dev);
+	return i915_drm_suspend(drm_dev);
 }
 
 static int i915_pm_suspend_late(struct device *dev)
@@ -950,7 +945,7 @@ static int i915_pm_resume_early(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
 
-	return i915_resume_early(drm_dev);
+	return i915_drm_resume_early(drm_dev);
 }
 
 static int i915_pm_resume(struct device *dev)
-- 
1.8.4

[PATCH 14/16] drm/i915: add poweroff_late handler

[Imre Deak]

The suspend_late handler saves some registers and powers off the device,
so it doesn't have a big overhead. Calling it at S4 poweroff_late time
makes the power off handling identical to the S3 suspend and S4 freeze
handling, so do this for consistency.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f6157a1..907a027 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1520,6 +1520,7 @@ static const struct dev_pm_ops i915_pm_ops = {
 	.thaw_early = i915_pm_resume_early,
 	.thaw = i915_pm_resume,
 	.poweroff = i915_pm_suspend,
+	.poweroff_late = i915_pm_suspend_late,
 	.restore_early = i915_pm_resume_early,
 	.restore = i915_pm_resume,
 	.runtime_suspend = intel_runtime_suspend,
-- 
1.8.4

[PATCH 15/16] drm/i915: unify switcheroo and legacy suspend/resume handlers

[Imre Deak]

By now we handle switcheroo and legacy suspend/resume the same way, so
no need to keep separate functions for them.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c |  4 ++--
 drivers/gpu/drm/i915/i915_drv.c | 11 +++--------
 drivers/gpu/drm/i915/i915_drv.h |  4 ++--
 3 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 09dc0d0..6c86e88 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1274,12 +1274,12 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_
 		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
 		/* i915 resume handler doesn't set to D0 */
 		pci_set_power_state(dev->pdev, PCI_D0);
-		i915_resume(dev);
+		i915_resume_legacy(dev);
 		dev->switch_power_state = DRM_SWITCH_POWER_ON;
 	} else {
 		pr_err("switched off\n");
 		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
-		i915_suspend(dev, pmm);
+		i915_suspend_legacy(dev, pmm);
 		dev->switch_power_state = DRM_SWITCH_POWER_OFF;
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 907a027..55b04e6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -627,7 +627,7 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev)
 	return 0;
 }
 
-int i915_suspend(struct drm_device *dev, pm_message_t state)
+int i915_suspend_legacy(struct drm_device *dev, pm_message_t state)
 {
 	int error;
 
@@ -755,7 +755,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
 	return ret;
 }
 
-static int i915_resume_legacy(struct drm_device *dev)
+int i915_resume_legacy(struct drm_device *dev)
 {
 	int ret;
 
@@ -766,11 +766,6 @@ static int i915_resume_legacy(struct drm_device *dev)
 	return i915_drm_resume(dev);
 }
 
-int i915_resume(struct drm_device *dev)
-{
-	return i915_resume_legacy(dev);
-}
-
 /**
  * i915_reset - reset chip after a hang
  * @dev: drm device to reset
@@ -1564,7 +1559,7 @@ static struct drm_driver driver = {
 	.set_busid = drm_pci_set_busid,
 
 	/* Used in place of i915_pm_ops for non-DRIVER_MODESET */
-	.suspend = i915_suspend,
+	.suspend = i915_suspend_legacy,
 	.resume = i915_resume_legacy,
 
 	.device_is_agp = i915_driver_device_is_agp,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e3ca8df..1396005 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2205,8 +2205,8 @@ struct drm_i915_cmd_table {
 extern const struct drm_ioctl_desc i915_ioctls[];
 extern int i915_max_ioctl;
 
-extern int i915_suspend(struct drm_device *dev, pm_message_t state);
-extern int i915_resume(struct drm_device *dev);
+extern int i915_suspend_legacy(struct drm_device *dev, pm_message_t state);
+extern int i915_resume_legacy(struct drm_device *dev);
 extern int i915_master_create(struct drm_device *dev, struct drm_master *master);
 extern void i915_master_destroy(struct drm_device *dev, struct drm_master *master);
 
-- 
1.8.4

[PATCH 16/16] drm/i915: add comments on what stage a given PM handler is called

[Imre Deak]

This will hopefully make it easier to navigate the code without the need
to consult the full PM documentation.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 55b04e6..44c9317 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1506,10 +1506,21 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
 }
 
 static const struct dev_pm_ops i915_pm_ops = {
+	/* S0ix, S3 event handlers */
 	.suspend = i915_pm_suspend,
 	.suspend_late = i915_pm_suspend_late,
 	.resume_early = i915_pm_resume_early,
 	.resume = i915_pm_resume,
+
+	/*
+	 * S4 event handlers
+	 * @freeze, @freeze_late    : called before creating hibernation image
+	 * @thaw, @thaw_early       : called after creating hibernation image,
+	 *                            before writing it
+	 * @poweroff, @poweroff_late: called after writing hibernation image,
+	 *                            before rebooting
+	 * @restore, @restore_early : called after rebooting
+	 */
 	.freeze = i915_pm_suspend,
 	.freeze_late = i915_pm_suspend_late,
 	.thaw_early = i915_pm_resume_early,
@@ -1518,6 +1529,8 @@ static const struct dev_pm_ops i915_pm_ops = {
 	.poweroff_late = i915_pm_suspend_late,
 	.restore_early = i915_pm_resume_early,
 	.restore = i915_pm_resume,
+
+	/* D0<->D3 (runtime PM) event handlers */
 	.runtime_suspend = intel_runtime_suspend,
 	.runtime_resume = intel_runtime_resume,
 };
-- 
1.8.4

Re: [PATCH 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers

[Daniel Vetter]

On Wed, Sep 10, 2014 at 06:16:53PM +0300, Imre Deak wrote:
> The first part of the patchset (1-6) fixes an S4 bug on VLV introduced
> recently. The rest unifies the various S3/S4 handlers, which are in
> practice the same. The only real difference - besides some unused code -
> is that during S3 suspend we disable the PCI device whereas across an S4
> freeze/thaw we keep it enabled. Given that we disable everything else
> anyway, we can just as well disable the PCI device there too. Doing so
> allows us to handle S3 suspend/resume and S4 freeze/thaw/restore/
> power-off the same way, simplifying/clarifying things quite a bit.

Hm, this might explain why we've seen random corruption on S4 on recent
platforms.

https://bugzilla.kernel.org/show_bug.cgi?id=59321

Can you please ask for retesting from reporters?

Thanks, Daniel
> 
> I smoke tested this on BDW, HSW, VLV (BYT-M), IVB, GEN45.
> 
> Imre Deak (16):
>   drm/i915: vlv: fix gunit HW state corruption during S4 suspend
>   drm/i915: remove dead code from legacy suspend handler
>   drm/i915: factor out i915_drm_suspend_late
>   drm/i915: unify legacy S3 suspend and S4 freeze handlers
>   drm/i915: propagate error from legacy resume handler
>   drm/i915: vlv: fix switcheroo/legacy suspend/resume
>   drm/i915: fix S4 suspend while switcheroo state is off
>   drm/i915: remove unused restore_gtt_mappings optimization during
>     suspend
>   drm/i915: check for GT faults during S3 resume and S4 restore too
>   drm/i915: enable output polling during S4 thaw
>   drm/i915: disable/re-enable PCI device around S4 freeze/thaw
>   drm/i915: unify S3 and S4 suspend/resume handlers
>   drm/i915: sanitize suspend/resume helper function names
>   drm/i915: add poweroff_late handler
>   drm/i915: unify switcheroo and legacy suspend/resume handlers
>   drm/i915: add comments on what stage a given PM handler is called
> 
>  drivers/gpu/drm/i915/i915_dma.c |   4 +-
>  drivers/gpu/drm/i915/i915_drv.c | 185 +++++++++++++++-------------------------
>  drivers/gpu/drm/i915/i915_drv.h |   4 +-
>  3 files changed, 74 insertions(+), 119 deletions(-)
> 
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers

[Imre Deak]

On Wed, 2014-09-10 at 17:52 +0200, Daniel Vetter wrote:
> On Wed, Sep 10, 2014 at 06:16:53PM +0300, Imre Deak wrote:
> > The first part of the patchset (1-6) fixes an S4 bug on VLV introduced
> > recently. The rest unifies the various S3/S4 handlers, which are in
> > practice the same. The only real difference - besides some unused code -
> > is that during S3 suspend we disable the PCI device whereas across an S4
> > freeze/thaw we keep it enabled. Given that we disable everything else
> > anyway, we can just as well disable the PCI device there too. Doing so
> > allows us to handle S3 suspend/resume and S4 freeze/thaw/restore/
> > power-off the same way, simplifying/clarifying things quite a bit.
> 
> Hm, this might explain why we've seen random corruption on S4 on recent
> platforms.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=59321
> 
> Can you please ask for retesting from reporters?

Ok, can do, I also forgot to add

https://bugs.freedesktop.org/show_bug.cgi?id=82842

which it fixes. I can't see immediately how platforms other than VLV
would be fixed with these, but maybe I missed something.

--Imre

> Thanks, Daniel
> > 
> > I smoke tested this on BDW, HSW, VLV (BYT-M), IVB, GEN45.
> > 
> > Imre Deak (16):
> >   drm/i915: vlv: fix gunit HW state corruption during S4 suspend
> >   drm/i915: remove dead code from legacy suspend handler
> >   drm/i915: factor out i915_drm_suspend_late
> >   drm/i915: unify legacy S3 suspend and S4 freeze handlers
> >   drm/i915: propagate error from legacy resume handler
> >   drm/i915: vlv: fix switcheroo/legacy suspend/resume
> >   drm/i915: fix S4 suspend while switcheroo state is off
> >   drm/i915: remove unused restore_gtt_mappings optimization during
> >     suspend
> >   drm/i915: check for GT faults during S3 resume and S4 restore too
> >   drm/i915: enable output polling during S4 thaw
> >   drm/i915: disable/re-enable PCI device around S4 freeze/thaw
> >   drm/i915: unify S3 and S4 suspend/resume handlers
> >   drm/i915: sanitize suspend/resume helper function names
> >   drm/i915: add poweroff_late handler
> >   drm/i915: unify switcheroo and legacy suspend/resume handlers
> >   drm/i915: add comments on what stage a given PM handler is called
> > 
> >  drivers/gpu/drm/i915/i915_dma.c |   4 +-
> >  drivers/gpu/drm/i915/i915_drv.c | 185 +++++++++++++++-------------------------
> >  drivers/gpu/drm/i915/i915_drv.h |   4 +-
> >  3 files changed, 74 insertions(+), 119 deletions(-)
> > 
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 09/16] drm/i915: check for GT faults during S3 resume and S4 restore too

[Chris Wilson]

On Wed, Sep 10, 2014 at 06:17:02PM +0300, Imre Deak wrote:
> Checking for GT faults is not specific in any way to S4 thaw, so do it
> also during S3 resume and S4 restore. This allows us to unify the
> handlers for these events in an upcoming patch.

So why not just move i915_check_and_clear_faults() to
intel_uncore_early_santize()? That would seem to be the right BIOS
scrubbing function that should also be doing this task.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend

[Chris Wilson]

On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote:
> Since correctness wins over optimal code and since the optimization

Optimal code is also correct ;-) s/optimal/just plain broken/
-Chris
-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend

[Daniel Vetter]

On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote:
> The logic to skip restoring GTT mappings was added to speed up
> suspend/resume, but not on old GENs where not restoring them caused
> problems. The check for old GENs is based on the existance of OpRegion,
> but this doesn't work since opregion is initialized only after
> the check. So we end up always restoring the mappings.
> 
> On my BYT - which has OpRegion - skipping restoring the mappings during
> suspend doesn't work, I get a GPU hang after resume. Also the logic of
> when to allow the optimization during S4 is reversed: we should allow it
> during S4 thaw but not during S4 restore, but atm we have it the other
> way around in the code.
> 
> Since correctness wins over optimal code and since the optimization
> wasn't used anyway I decided not to try to fix it at this point, but
> just remove it. This allows us to unify the S3 and S4 handlers in the
> following patches.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Adding Jesse. Also he claimed that it actually helped back in

commit 1abd02e2dd7e0bd577000301fb2fd47780637387
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Fri Nov 2 11:14:02 2012 -0700

    drm/i915: don't rewrite the GTT on resume v4

dunno where exactly this broke.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index a3addc2..5e25283 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -667,12 +667,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)
>  	return ret;
>  }
>  
> -static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> +static int __i915_drm_thaw(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
> -	    restore_gtt_mappings) {
> +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		mutex_lock(&dev->struct_mutex);
>  		i915_gem_restore_gtt_mappings(dev);
>  		mutex_unlock(&dev->struct_mutex);
> @@ -740,7 +739,7 @@ static int i915_drm_thaw(struct drm_device *dev)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		i915_check_and_clear_faults(dev);
>  
> -	return __i915_drm_thaw(dev, true);
> +	return __i915_drm_thaw(dev);
>  }
>  
>  static int i915_resume_early(struct drm_device *dev)
> @@ -767,15 +766,9 @@ static int i915_resume_early(struct drm_device *dev)
>  
>  static int i915_drm_resume(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> -	/*
> -	 * Platforms with opregion should have sane BIOS, older ones (gen3 and
> -	 * earlier) need to restore the GTT mappings since the BIOS might clear
> -	 * all our scratch PTEs.
> -	 */
> -	ret = __i915_drm_thaw(dev, !dev_priv->opregion.header);
> +	ret = __i915_drm_thaw(dev);
>  	if (ret)
>  		return ret;
>  
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/16] drm/i915: unify legacy S3 suspend and S4 freeze handlers

[Daniel Vetter]

On Wed, Sep 10, 2014 at 06:16:57PM +0300, Imre Deak wrote:
> i915_suspend() is called from the DRM legacy S3 suspend/S4 freeze paths
> and the switcheroo suspend path. For switcheroo we only ever need to
> perform a full suspend (PM_EVENT_SUSPEND) and for the DRM legacy path
> we can handle the S4 freeze (PM_EVENT_FREEZE) the same way as S3
> suspend. The only difference atm between suspend and freeze is that
> during freeze we don't disable the PCI device, but there is no reason
> why we can't do so. So unify the two cases to reduce complexity.
> 
> Note that for the DRM legacy case the thaw event is not handled, so
> we disable the display before creating the hibernation image and it
> won't get re-enabled until reboot. We could fix this leaving the
> display enabled for the image creation/writing (if we care enough
> about UMS), but this can be done as a follow-up.

UMS doesn't even get compiled any more, please don't try to fix it ;-)

My plan is to garbage-collect it all around when 3.17 is out the door.
-Daniel

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 7fa34bd..bf4ff5e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -648,11 +648,9 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
>  	if (error)
>  		return error;
>  
> -	if (state.event == PM_EVENT_SUSPEND) {
> -		/* Shut down the device */
> -		pci_disable_device(dev->pdev);
> -		pci_set_power_state(dev->pdev, PCI_D3hot);
> -	}
> +	/* Shut down the device */
> +	pci_disable_device(dev->pdev);
> +	pci_set_power_state(dev->pdev, PCI_D3hot);
>  
>  	return 0;
>  }
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers

[Daniel Vetter]

On Wed, Sep 10, 2014 at 09:38:50PM +0300, Imre Deak wrote:
> On Wed, 2014-09-10 at 17:52 +0200, Daniel Vetter wrote:
> > On Wed, Sep 10, 2014 at 06:16:53PM +0300, Imre Deak wrote:
> > > The first part of the patchset (1-6) fixes an S4 bug on VLV introduced
> > > recently. The rest unifies the various S3/S4 handlers, which are in
> > > practice the same. The only real difference - besides some unused code -
> > > is that during S3 suspend we disable the PCI device whereas across an S4
> > > freeze/thaw we keep it enabled. Given that we disable everything else
> > > anyway, we can just as well disable the PCI device there too. Doing so
> > > allows us to handle S3 suspend/resume and S4 freeze/thaw/restore/
> > > power-off the same way, simplifying/clarifying things quite a bit.
> > 
> > Hm, this might explain why we've seen random corruption on S4 on recent
> > platforms.
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=59321
> > 
> > Can you please ask for retesting from reporters?
> 
> Ok, can do, I also forgot to add
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=82842
> 
> which it fixes. I can't see immediately how platforms other than VLV
> would be fixed with these, but maybe I missed something.

drm/i915: disable/re-enable PCI device around S4 freeze/thaw

looks rather generic and not vlv specific, and could very well fix the
kernel bz I've pasted. Or am I horribly blind?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 09/16] drm/i915: check for GT faults during S3 resume and S4 restore too

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 582 bytes --]

On Thu, 2014-09-11 at 08:47 +0100, Chris Wilson wrote:
> On Wed, Sep 10, 2014 at 06:17:02PM +0300, Imre Deak wrote:
> > Checking for GT faults is not specific in any way to S4 thaw, so do it
> > also during S3 resume and S4 restore. This allows us to unify the
> > handlers for these events in an upcoming patch.
> 
> So why not just move i915_check_and_clear_faults() to
> intel_uncore_early_santize()? That would seem to be the right BIOS
> scrubbing function that should also be doing this task.

Ok, haven't consider moving the check, but it makes sense.

--Imre


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 556 bytes --]

On Thu, 2014-09-11 at 08:49 +0100, Chris Wilson wrote:
> On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote:
> > Since correctness wins over optimal code and since the optimization
> 
> Optimal code is also correct ;-) s/optimal/just plain broken/

Yes, bad wording. To clarify, since the optimization is now always off
anyway (and it's also broken), I would hope that we could remove it for
now and concentrate on fixing the existing s/r issues. Once we find that
things are stable enough we could add back this optimization.

--Imre


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/16] drm/i915: unify legacy S3 suspend and S4 freeze handlers

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 2511 bytes --]

On Thu, 2014-09-11 at 11:00 +0200, Daniel Vetter wrote:
> On Wed, Sep 10, 2014 at 06:16:57PM +0300, Imre Deak wrote:
> > i915_suspend() is called from the DRM legacy S3 suspend/S4 freeze paths
> > and the switcheroo suspend path. For switcheroo we only ever need to
> > perform a full suspend (PM_EVENT_SUSPEND) and for the DRM legacy path
> > we can handle the S4 freeze (PM_EVENT_FREEZE) the same way as S3
> > suspend. The only difference atm between suspend and freeze is that
> > during freeze we don't disable the PCI device, but there is no reason
> > why we can't do so. So unify the two cases to reduce complexity.
> > 
> > Note that for the DRM legacy case the thaw event is not handled, so
> > we disable the display before creating the hibernation image and it
> > won't get re-enabled until reboot. We could fix this leaving the
> > display enabled for the image creation/writing (if we care enough
> > about UMS), but this can be done as a follow-up.
> 
> UMS doesn't even get compiled any more, please don't try to fix it ;-)

Ok. That reference above to UMS was only for a future possible fix, but
then we can ignore it. There are some UMS (aka legacy s/r) related
changes/fixes in this patchset too, but they are done alongside the
switcheroo fixes, since in the end the two things are handled the same
way.

--Imre

> My plan is to garbage-collect it all around when 3.17 is out the door.
> -Daniel
> 
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 7fa34bd..bf4ff5e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -648,11 +648,9 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
> >  	if (error)
> >  		return error;
> >  
> > -	if (state.event == PM_EVENT_SUSPEND) {
> > -		/* Shut down the device */
> > -		pci_disable_device(dev->pdev);
> > -		pci_set_power_state(dev->pdev, PCI_D3hot);
> > -	}
> > +	/* Shut down the device */
> > +	pci_disable_device(dev->pdev);
> > +	pci_set_power_state(dev->pdev, PCI_D3hot);
> >  
> >  	return 0;
> >  }
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1968 bytes --]

On Thu, 2014-09-11 at 11:02 +0200, Daniel Vetter wrote:
> On Wed, Sep 10, 2014 at 09:38:50PM +0300, Imre Deak wrote:
> > On Wed, 2014-09-10 at 17:52 +0200, Daniel Vetter wrote:
> > > On Wed, Sep 10, 2014 at 06:16:53PM +0300, Imre Deak wrote:
> > > > The first part of the patchset (1-6) fixes an S4 bug on VLV introduced
> > > > recently. The rest unifies the various S3/S4 handlers, which are in
> > > > practice the same. The only real difference - besides some unused code -
> > > > is that during S3 suspend we disable the PCI device whereas across an S4
> > > > freeze/thaw we keep it enabled. Given that we disable everything else
> > > > anyway, we can just as well disable the PCI device there too. Doing so
> > > > allows us to handle S3 suspend/resume and S4 freeze/thaw/restore/
> > > > power-off the same way, simplifying/clarifying things quite a bit.
> > > 
> > > Hm, this might explain why we've seen random corruption on S4 on recent
> > > platforms.
> > > 
> > > https://bugzilla.kernel.org/show_bug.cgi?id=59321
> > > 
> > > Can you please ask for retesting from reporters?
> > 
> > Ok, can do, I also forgot to add
> > 
> > https://bugs.freedesktop.org/show_bug.cgi?id=82842
> > 
> > which it fixes. I can't see immediately how platforms other than VLV
> > would be fixed with these, but maybe I missed something.
> 
> drm/i915: disable/re-enable PCI device around S4 freeze/thaw
> 
> looks rather generic and not vlv specific, and could very well fix the
> kernel bz I've pasted. Or am I horribly blind?

Yea it's generic, so possibly fixes something. Although by the time we
disable the PCI device in freeze everything should be idle, so if simply
disabling/re-enabling makes a difference then we failed to idle
something. Or we depend on a HW reset (implicit in the
disable/re-enable) before reinitializing things in thaw. Anyway we can
clarify this more once we get feedback from the retesting.

--Imre


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend

[Jesse Barnes]

On Thu, 11 Sep 2014 14:59:35 +0300
Imre Deak <imre.deak@intel.com> wrote:

> On Thu, 2014-09-11 at 08:49 +0100, Chris Wilson wrote:
> > On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote:
> > > Since correctness wins over optimal code and since the optimization
> > 
> > Optimal code is also correct ;-) s/optimal/just plain broken/
> 
> Yes, bad wording. To clarify, since the optimization is now always off
> anyway (and it's also broken), I would hope that we could remove it for
> now and concentrate on fixing the existing s/r issues. Once we find that
> things are stable enough we could add back this optimization.

Arg, I guess we didn't test after moving to the opregion test?  Or maybe
it was working when it landed for S3 and not for S4?  Or broke sometime
after it landed?

Anyway this is a really valuable optimization for resume time on some
platforms, and really we shouldn't have other agents clobbering our GTT
on resume, so I'd really like to fix/re-add it asap.

Jesse

[PATCH v2 09/16] drm/i915: check for GT faults in all resume handlers and driver load time

[Imre Deak]

Checking for GT faults is not specific in any way to S4 thaw, so do it
also during S3 resume, S4 restore and driver load time. This allows us to
unify the Sx handlers in an upcoming patch.

v2:
- move the check to intel_uncore_early_sanitize(), so we check at driver
  load time too (Chris)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |  3 ---
 drivers/gpu/drm/i915/intel_uncore.c | 13 +++++++++++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5e25283..3090a94 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -736,9 +736,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 
 static int i915_drm_thaw(struct drm_device *dev)
 {
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		i915_check_and_clear_faults(dev);
-
 	return __i915_drm_thaw(dev);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 918b761..cc46ac5 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -363,7 +363,8 @@ void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
+static void __intel_uncore_early_sanitize(struct drm_device *dev,
+					  bool restore_forcewake)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -389,6 +390,12 @@ void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
 	intel_uncore_forcewake_reset(dev, restore_forcewake);
 }
 
+void intel_uncore_early_sanitize(struct drm_device *dev, bool restore_forcewake)
+{
+	__intel_uncore_early_sanitize(dev, restore_forcewake);
+	i915_check_and_clear_faults(dev);
+}
+
 void intel_uncore_sanitize(struct drm_device *dev)
 {
 	/* BIOS often leaves RC6 enabled, but disable it for hw init */
@@ -833,7 +840,7 @@ void intel_uncore_init(struct drm_device *dev)
 	setup_timer(&dev_priv->uncore.force_wake_timer,
 		    gen6_force_wake_timer, (unsigned long)dev_priv);
 
-	intel_uncore_early_sanitize(dev, false);
+	__intel_uncore_early_sanitize(dev, false);
 
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
@@ -951,6 +958,8 @@ void intel_uncore_init(struct drm_device *dev)
 		dev_priv->uncore.funcs.mmio_readq  = gen4_read64;
 		break;
 	}
+
+	i915_check_and_clear_faults(dev);
 }
 
 void intel_uncore_fini(struct drm_device *dev)
-- 
1.8.4

[PATCH v2 16/16] drm/i915: add comments on what stage a given PM handler is called

[Imre Deak]

This will hopefully make it easier to navigate the code without the need
to consult the full PM documentation.

v2:
- add a comment that the freeze handler is also called after rebooting

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b5508ae..538a658 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1504,10 +1504,24 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
 }
 
 static const struct dev_pm_ops i915_pm_ops = {
+	/* S0ix, S3 event handlers */
 	.suspend = i915_pm_suspend,
 	.suspend_late = i915_pm_suspend_late,
 	.resume_early = i915_pm_resume_early,
 	.resume = i915_pm_resume,
+
+	/*
+	 * S4 event handlers
+	 * @freeze, @freeze_late    : called (1) before creating hibernation
+	 *                            image and (2) after rebooting, before
+	 *                            restoring the image
+	 * @thaw, @thaw_early       : called after creating hibernation image,
+	 *                            before writing it
+	 * @poweroff, @poweroff_late: called after writing hibernation image,
+	 *                            before rebooting
+	 * @restore, @restore_early : called after rebooting and restoring the
+	 *                            image
+	 */
 	.freeze = i915_pm_suspend,
 	.freeze_late = i915_pm_suspend_late,
 	.thaw_early = i915_pm_resume_early,
@@ -1516,6 +1530,8 @@ static const struct dev_pm_ops i915_pm_ops = {
 	.poweroff_late = i915_pm_suspend_late,
 	.restore_early = i915_pm_resume_early,
 	.restore = i915_pm_resume,
+
+	/* D0<->D3 (runtime PM) event handlers */
 	.runtime_suspend = intel_runtime_suspend,
 	.runtime_resume = intel_runtime_resume,
 };
-- 
1.8.4

Re: [PATCH v2 09/16] drm/i915: check for GT faults in all resume handlers and driver load time

[Chris Wilson]

On Mon, Sep 15, 2014 at 06:21:56PM +0300, Imre Deak wrote:
> Checking for GT faults is not specific in any way to S4 thaw, so do it
> also during S3 resume, S4 restore and driver load time. This allows us to
> unify the Sx handlers in an upcoming patch.
> 
> v2:
> - move the check to intel_uncore_early_sanitize(), so we check at driver
>   load time too (Chris)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

I stared at early_sanitize and sanitize and decided that what you did
here was correct,

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 01/16] drm/i915: vlv: fix gunit HW state corruption during S4 suspend

[Sagar Arun Kamble]

>From DPM documentation, thaw_early should undo actions by freeze_late.
Can we move following snippet from thaw_early to thaw to comply with
this?

	intel_uncore_early_sanitize(dev, true);
        intel_uncore_sanitize(dev);
        intel_power_domains_init_hw(dev_priv);

On Wed, 2014-09-10 at 18:16 +0300, Imre Deak wrote:
> During S4 freeze we don't call intel_suspend_complete(), which would
> save the gunit HW state, but during S4 thaw/restore events we call
> intel_resume_prepare() which restores it, thus ending up in a corrupted
> HW state.
> 
> Fix this by calling intel_suspend_complete() from the corresponding
> freeze_late event handler.
> 
> The issue was introduced in
> commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> Author: Sagar Kamble <sagar.a.kamble@intel.com>
> Date:   Wed Aug 13 23:07:06 2014 +0530
> 
> CC: Sagar Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b8bd008..2365875 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -987,6 +987,15 @@ static int i915_pm_freeze(struct device *dev)
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> +static int i915_pm_freeze_late(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> +
> +	return intel_suspend_complete(dev_priv);
> +}
> +
>  static int i915_pm_thaw_early(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -1571,6 +1580,7 @@ static const struct dev_pm_ops i915_pm_ops = {
>  	.resume_early = i915_pm_resume_early,
>  	.resume = i915_pm_resume,
>  	.freeze = i915_pm_freeze,
> +	.freeze_late = i915_pm_freeze_late,
>  	.thaw_early = i915_pm_thaw_early,
>  	.thaw = i915_pm_thaw,
>  	.poweroff = i915_pm_poweroff,

Re: [PATCH 01/16] drm/i915: vlv: fix gunit HW state corruption during S4 suspend

[Imre Deak]

The point of doing these in thaw_early is to work around an ordering
issue wrt. to the hda driver, see the comment in i915_resume_early(). I
don't see a problem of having them in thaw_early; if you meant that they
lack the corresponding cleanup calls in freeze_late, it's just because
they don't have any.

On Mon, 2014-09-15 at 22:56 +0530, Sagar Arun Kamble wrote:
> From DPM documentation, thaw_early should undo actions by freeze_late.
> Can we move following snippet from thaw_early to thaw to comply with
> this?
> 
> 	intel_uncore_early_sanitize(dev, true);
>         intel_uncore_sanitize(dev);
>         intel_power_domains_init_hw(dev_priv);
> 
> On Wed, 2014-09-10 at 18:16 +0300, Imre Deak wrote:
> > During S4 freeze we don't call intel_suspend_complete(), which would
> > save the gunit HW state, but during S4 thaw/restore events we call
> > intel_resume_prepare() which restores it, thus ending up in a corrupted
> > HW state.
> > 
> > Fix this by calling intel_suspend_complete() from the corresponding
> > freeze_late event handler.
> > 
> > The issue was introduced in
> > commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> > Author: Sagar Kamble <sagar.a.kamble@intel.com>
> > Date:   Wed Aug 13 23:07:06 2014 +0530
> > 
> > CC: Sagar Kamble <sagar.a.kamble@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index b8bd008..2365875 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -987,6 +987,15 @@ static int i915_pm_freeze(struct device *dev)
> >  	return i915_drm_freeze(drm_dev);
> >  }
> >  
> > +static int i915_pm_freeze_late(struct device *dev)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > +
> > +	return intel_suspend_complete(dev_priv);
> > +}
> > +
> >  static int i915_pm_thaw_early(struct device *dev)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> > @@ -1571,6 +1580,7 @@ static const struct dev_pm_ops i915_pm_ops = {
> >  	.resume_early = i915_pm_resume_early,
> >  	.resume = i915_pm_resume,
> >  	.freeze = i915_pm_freeze,
> > +	.freeze_late = i915_pm_freeze_late,
> >  	.thaw_early = i915_pm_thaw_early,
> >  	.thaw = i915_pm_thaw,
> >  	.poweroff = i915_pm_poweroff,
> 
> 

Re: [PATCH 01/16] drm/i915: vlv: fix gunit HW state corruption during S4 suspend

[Sagar Arun Kamble]

Thanks Imre.

Reviewed-by: Sagar Kamble <sagar.a.kamble@intel.com>
On Mon, 2014-09-15 at 20:42 +0300, Imre Deak wrote:
> The point of doing these in thaw_early is to work around an ordering
> issue wrt. to the hda driver, see the comment in i915_resume_early(). I
> don't see a problem of having them in thaw_early; if you meant that they
> lack the corresponding cleanup calls in freeze_late, it's just because
> they don't have any.
> 
> On Mon, 2014-09-15 at 22:56 +0530, Sagar Arun Kamble wrote:
> > From DPM documentation, thaw_early should undo actions by freeze_late.
> > Can we move following snippet from thaw_early to thaw to comply with
> > this?
> > 
> > 	intel_uncore_early_sanitize(dev, true);
> >         intel_uncore_sanitize(dev);
> >         intel_power_domains_init_hw(dev_priv);
> > 
> > On Wed, 2014-09-10 at 18:16 +0300, Imre Deak wrote:
> > > During S4 freeze we don't call intel_suspend_complete(), which would
> > > save the gunit HW state, but during S4 thaw/restore events we call
> > > intel_resume_prepare() which restores it, thus ending up in a corrupted
> > > HW state.
> > > 
> > > Fix this by calling intel_suspend_complete() from the corresponding
> > > freeze_late event handler.
> > > 
> > > The issue was introduced in
> > > commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> > > Author: Sagar Kamble <sagar.a.kamble@intel.com>
> > > Date:   Wed Aug 13 23:07:06 2014 +0530
> > > 
> > > CC: Sagar Kamble <sagar.a.kamble@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index b8bd008..2365875 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -987,6 +987,15 @@ static int i915_pm_freeze(struct device *dev)
> > >  	return i915_drm_freeze(drm_dev);
> > >  }
> > >  
> > > +static int i915_pm_freeze_late(struct device *dev)
> > > +{
> > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > > +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > > +
> > > +	return intel_suspend_complete(dev_priv);
> > > +}
> > > +
> > >  static int i915_pm_thaw_early(struct device *dev)
> > >  {
> > >  	struct pci_dev *pdev = to_pci_dev(dev);
> > > @@ -1571,6 +1580,7 @@ static const struct dev_pm_ops i915_pm_ops = {
> > >  	.resume_early = i915_pm_resume_early,
> > >  	.resume = i915_pm_resume,
> > >  	.freeze = i915_pm_freeze,
> > > +	.freeze_late = i915_pm_freeze_late,
> > >  	.thaw_early = i915_pm_thaw_early,
> > >  	.thaw = i915_pm_thaw,
> > >  	.poweroff = i915_pm_poweroff,
> > 
> > 
> 
> 

Re: [PATCH 06/16] drm/i915: vlv: fix switcheroo/legacy suspend/resume

[Sagar Arun Kamble]

Reviewed-by: Sagar Kamble <sagar.a.kamble@intel.com>

On Wed, 2014-09-10 at 18:16 +0300, Imre Deak wrote:
> During switcheroo/legacy suspend we don't call the suspend_late handler but
> when resuming afterwards we call resume_early. This happened to work so far,
> since suspend_late only disabled the PCI device. This changed in
> 
> commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> Author: Sagar Kamble <sagar.a.kamble@intel.com>
> Date:   Wed Aug 13 23:07:06 2014 +0530
> 
>     drm/i915: Sharing platform specific sequence between runtime and system susp
> 
> after which we also saved/restored the VLV Gunit HW state in
> suspend_late/resume_early. So now since we don't save the state during
> suspend a following resume will restore a corrupted state.
> 
> Fix this by calling the suspend_late handler during both switcheroo and
> legacy suspend.
> 
> CC: Sagar Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 89b63fc..ca74d6d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -648,11 +648,7 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
>  	if (error)
>  		return error;
>  
> -	/* Shut down the device */
> -	pci_disable_device(dev->pdev);
> -	pci_set_power_state(dev->pdev, PCI_D3hot);
> -
> -	return 0;
> +	return i915_drm_suspend_late(dev);
>  }
>  
>  static int i915_drm_thaw_early(struct drm_device *dev)
> @@ -769,7 +765,7 @@ static int i915_resume_early(struct drm_device *dev)
>  	return i915_drm_thaw_early(dev);
>  }
>  
> -int i915_resume(struct drm_device *dev)
> +static int i915_drm_resume(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
> @@ -795,7 +791,12 @@ static int i915_resume_legacy(struct drm_device *dev)
>  	if (ret)
>  		return ret;
>  
> -	return i915_resume(dev);
> +	return i915_drm_resume(dev);
> +}
> +
> +int i915_resume(struct drm_device *dev)
> +{
> +	return i915_resume_legacy(dev);
>  }
>  
>  /**
> @@ -980,7 +981,7 @@ static int i915_pm_resume(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
>  
> -	return i915_resume(drm_dev);
> +	return i915_drm_resume(drm_dev);
>  }
>  
>  static int i915_pm_freeze(struct device *dev)

Re: [PATCH 02/16] drm/i915: remove dead code from legacy suspend handler

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 06:16:55PM +0300, Imre Deak wrote:
> The legacy DRM suspend logic (effective in UMS) doesn't handle any S4 thaw
> events so we don't need to care about it either. Only S3 suspend and S4
> freeze events are handled. Leave an assert behind to be sure.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Looks correct.

Although it the lack of .thaw() means there's no error recovery when
things go south. But this is UMS so who cares.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2365875..f36e2d2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -618,9 +618,9 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
>  		return -ENODEV;
>  	}
>  
> -	if (state.event == PM_EVENT_PRETHAW)
> -		return 0;
> -
> +	if (WARN_ON_ONCE(state.event != PM_EVENT_SUSPEND &&
> +			 state.event != PM_EVENT_FREEZE))
> +		return -EINVAL;
>  
>  	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 02/16] drm/i915: remove dead code from legacy suspend handler

[Daniel Vetter]

On Tue, Oct 21, 2014 at 02:56:46PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 10, 2014 at 06:16:55PM +0300, Imre Deak wrote:
> > The legacy DRM suspend logic (effective in UMS) doesn't handle any S4 thaw
> > events so we don't need to care about it either. Only S3 suspend and S4
> > freeze events are handled. Leave an assert behind to be sure.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Looks correct.
> 
> Although it the lack of .thaw() means there's no error recovery when
> things go south. But this is UMS so who cares.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 01/16] drm/i915: vlv: fix gunit HW state corruption during S4 suspend

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 06:16:54PM +0300, Imre Deak wrote:
> During S4 freeze we don't call intel_suspend_complete(), which would
> save the gunit HW state, but during S4 thaw/restore events we call
> intel_resume_prepare() which restores it, thus ending up in a corrupted
> HW state.
> 
> Fix this by calling intel_suspend_complete() from the corresponding
> freeze_late event handler.
> 
> The issue was introduced in
> commit 016970beb05da6285c2f3ed2bee1c676cb75972e
> Author: Sagar Kamble <sagar.a.kamble@intel.com>
> Date:   Wed Aug 13 23:07:06 2014 +0530
> 
> CC: Sagar Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Yep, pairs up with .thaw_early()/.restore_early(), which should take
care of all the valid combinations of pm events.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b8bd008..2365875 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -987,6 +987,15 @@ static int i915_pm_freeze(struct device *dev)
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> +static int i915_pm_freeze_late(struct device *dev)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> +
> +	return intel_suspend_complete(dev_priv);
> +}
> +
>  static int i915_pm_thaw_early(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> @@ -1571,6 +1580,7 @@ static const struct dev_pm_ops i915_pm_ops = {
>  	.resume_early = i915_pm_resume_early,
>  	.resume = i915_pm_resume,
>  	.freeze = i915_pm_freeze,
> +	.freeze_late = i915_pm_freeze_late,
>  	.thaw_early = i915_pm_thaw_early,
>  	.thaw = i915_pm_thaw,
>  	.poweroff = i915_pm_poweroff,
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 07/16] drm/i915: fix S4 suspend while switcheroo state is off

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 06:17:00PM +0300, Imre Deak wrote:
> If the device is suspended already through the switcheroo interface we
> shouldn't suspend it again. We have the corresponding check for S3
> suspend already, add it for S4 freeze and poweroff too.
> 
> Note that there is still the problem that the resume path should also
> check for the switcheroo-off state and keep the device disabled or in
> case of S4 disable it. That is a separate issue, which is not addressed
> in this patchset.

That's about RESUME/RESTORE I take it.

But what about .thaw()? I think simply adding the same check to .thaw()
would work out just fine since it's always called after .freeze() for
THAW/RECOVER.

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  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 ca74d6d..a3addc2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -994,6 +994,9 @@ static int i915_pm_freeze(struct device *dev)
>  		return -ENODEV;
>  	}
>  
> +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> +		return 0;
> +
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> @@ -1003,6 +1006,9 @@ static int i915_pm_freeze_late(struct device *dev)
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
>  	struct drm_i915_private *dev_priv = drm_dev->dev_private;
>  
> +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> +		return 0;
> +
>  	return intel_suspend_complete(dev_priv);
>  }
>  
> @@ -1027,6 +1033,9 @@ static int i915_pm_poweroff(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
>  
> +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> +		return 0;
> +
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 07/16] drm/i915: fix S4 suspend while switcheroo state is off

[Imre Deak]

On Tue, 2014-10-21 at 15:41 +0300, Ville Syrjälä wrote:
> On Wed, Sep 10, 2014 at 06:17:00PM +0300, Imre Deak wrote:
> > If the device is suspended already through the switcheroo interface we
> > shouldn't suspend it again. We have the corresponding check for S3
> > suspend already, add it for S4 freeze and poweroff too.
> > 
> > Note that there is still the problem that the resume path should also
> > check for the switcheroo-off state and keep the device disabled or in
> > case of S4 disable it. That is a separate issue, which is not addressed
> > in this patchset.
> 
> That's about RESUME/RESTORE I take it.
> 
> But what about .thaw()? I think simply adding the same check to .thaw()
> would work out just fine since it's always called after .freeze() for
> THAW/RECOVER.

Yes, the check is missing from there too (actually by the end of the
patchset THAW/RESUME/RESTORE are handled the same way). I didn't want to
fix up that part in this patchset, since I thought avoiding RESTORE in
case DRM_SWITCH_POWER_OFF is set is not enough during S4, we have to
actually disable the device if it was enabled. But thinking about it
again that seems to be not true, since before RESTORE FREEZE is called
in the loader kernel, which leaves the device in a disabled state.

So I can put the check to .thaw too (.suspend by the end of the
patchset), but it'd be easier to do it on top of this patchset if that's
ok.

--Imre

> 
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  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 ca74d6d..a3addc2 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -994,6 +994,9 @@ static int i915_pm_freeze(struct device *dev)
> >  		return -ENODEV;
> >  	}
> >  
> > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> > +		return 0;
> > +
> >  	return i915_drm_freeze(drm_dev);
> >  }
> >  
> > @@ -1003,6 +1006,9 @@ static int i915_pm_freeze_late(struct device *dev)
> >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> >  	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> >  
> > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> > +		return 0;
> > +
> >  	return intel_suspend_complete(dev_priv);
> >  }
> >  
> > @@ -1027,6 +1033,9 @@ static int i915_pm_poweroff(struct device *dev)
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> >  
> > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> > +		return 0;
> > +
> >  	return i915_drm_freeze(drm_dev);
> >  }
> >  
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/16] drm/i915: disable/re-enable PCI device around S4 freeze/thaw

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 06:17:04PM +0300, Imre Deak wrote:
> We already disable everything during S4 freeze, except the PCI device
> itself. There is no reason why we couldn't disable that too and doing
> so allows us to unify these handlers in the next patch with the
> corresponding S3 suspend/resume handlers.

First I was a bit WTF because the PCI core doesn't seem to explicitly
set the device back to D0 in .thaw() whereas it does for
.restore()/.resume(). But then I realized pci_enable_device() does that
anyway, so this looks fine.

To properly optimize .freeze()/.thaw() we should basically just quiesce
the device but leave everything powered on. But such optimizations are
for the future, and in the meantime sharing the same codepaths should
mean we have the same bugs for both, instead of slightly different bugs
for each.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ea8dda7..0ff5e92 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -991,12 +991,11 @@ static int i915_pm_freeze_late(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> -	struct drm_i915_private *dev_priv = drm_dev->dev_private;
>  
>  	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
>  		return 0;
>  
> -	return intel_suspend_complete(dev_priv);
> +	return i915_drm_suspend_late(drm_dev);
>  }
>  
>  static int i915_pm_thaw_early(struct device *dev)
> @@ -1004,7 +1003,7 @@ static int i915_pm_thaw_early(struct device *dev)
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
>  
> -	return i915_drm_thaw_early(drm_dev);
> +	return i915_resume_early(drm_dev);
>  }
>  
>  static int i915_pm_thaw(struct device *dev)
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 07/16] drm/i915: fix S4 suspend while switcheroo state is off

[Ville Syrjälä]

On Tue, Oct 21, 2014 at 04:08:21PM +0300, Imre Deak wrote:
> On Tue, 2014-10-21 at 15:41 +0300, Ville Syrjälä wrote:
> > On Wed, Sep 10, 2014 at 06:17:00PM +0300, Imre Deak wrote:
> > > If the device is suspended already through the switcheroo interface we
> > > shouldn't suspend it again. We have the corresponding check for S3
> > > suspend already, add it for S4 freeze and poweroff too.
> > > 
> > > Note that there is still the problem that the resume path should also
> > > check for the switcheroo-off state and keep the device disabled or in
> > > case of S4 disable it. That is a separate issue, which is not addressed
> > > in this patchset.
> > 
> > That's about RESUME/RESTORE I take it.
> > 
> > But what about .thaw()? I think simply adding the same check to .thaw()
> > would work out just fine since it's always called after .freeze() for
> > THAW/RECOVER.
> 
> Yes, the check is missing from there too (actually by the end of the
> patchset THAW/RESUME/RESTORE are handled the same way). I didn't want to
> fix up that part in this patchset, since I thought avoiding RESTORE in
> case DRM_SWITCH_POWER_OFF is set is not enough during S4, we have to
> actually disable the device if it was enabled. But thinking about it
> again that seems to be not true, since before RESTORE FREEZE is called
> in the loader kernel, which leaves the device in a disabled state.

I was thinking just about fixing THAW/RECOVER since those get executed
from the same kernel as FREEZE/QUIESCE, but I guess you're correct that
we can just handle RESUME/RESTORE the same way since SUSPEND/QUIESCE in
the pre-resume kernel should have disabled the device anyway in case it
was enabled, which means the state will match the switcheroo disabled
state in the resumed kernel. And if switcheroo says the device should
be enabled we enable it normally when resuming. So yeah, sounds like it
should just work (tm).

> 
> So I can put the check to .thaw too (.suspend by the end of the
> patchset), but it'd be easier to do it on top of this patchset if that's
> ok.

Sure. 

> 
> --Imre
> 
> > 
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  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 ca74d6d..a3addc2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -994,6 +994,9 @@ static int i915_pm_freeze(struct device *dev)
> > >  		return -ENODEV;
> > >  	}
> > >  
> > > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> > > +		return 0;
> > > +
> > >  	return i915_drm_freeze(drm_dev);
> > >  }
> > >  
> > > @@ -1003,6 +1006,9 @@ static int i915_pm_freeze_late(struct device *dev)
> > >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > >  	struct drm_i915_private *dev_priv = drm_dev->dev_private;
> > >  
> > > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> > > +		return 0;
> > > +
> > >  	return intel_suspend_complete(dev_priv);
> > >  }
> > >  
> > > @@ -1027,6 +1033,9 @@ static int i915_pm_poweroff(struct device *dev)
> > >  	struct pci_dev *pdev = to_pci_dev(dev);
> > >  	struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > >  
> > > +	if (drm_dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> > > +		return 0;
> > > +
> > >  	return i915_drm_freeze(drm_dev);
> > >  }
> > >  
> > > -- 
> > > 1.8.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 14/16] drm/i915: add poweroff_late handler

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 06:17:07PM +0300, Imre Deak wrote:
> The suspend_late handler saves some registers and powers off the device,
> so it doesn't have a big overhead. Calling it at S4 poweroff_late time
> makes the power off handling identical to the S3 suspend and S4 freeze
> handling, so do this for consistency.

Heh. I was wondering where .poweroff_late() was when reading the earlier
patches. I like consistency.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f6157a1..907a027 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1520,6 +1520,7 @@ static const struct dev_pm_ops i915_pm_ops = {
>  	.thaw_early = i915_pm_resume_early,
>  	.thaw = i915_pm_resume,
>  	.poweroff = i915_pm_suspend,
> +	.poweroff_late = i915_pm_suspend_late,
>  	.restore_early = i915_pm_resume_early,
>  	.restore = i915_pm_resume,
>  	.runtime_suspend = intel_runtime_suspend,
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 16/16] drm/i915: add comments on what stage a given PM handler is called

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 06:17:09PM +0300, Imre Deak wrote:
> This will hopefully make it easier to navigate the code without the need
> to consult the full PM documentation.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 55b04e6..44c9317 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1506,10 +1506,21 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
>  }
>  
>  static const struct dev_pm_ops i915_pm_ops = {
> +	/* S0ix, S3 event handlers */

Well, S0ix can occur in due to runtime PM as well.

>  	.suspend = i915_pm_suspend,
>  	.suspend_late = i915_pm_suspend_late,
>  	.resume_early = i915_pm_resume_early,
>  	.resume = i915_pm_resume,
> +
> +	/*
> +	 * S4 event handlers
> +	 * @freeze, @freeze_late    : called before creating hibernation image

That's FREEZE. Also called before restoring the hibenation image (QUIESCE).

> +	 * @thaw, @thaw_early       : called after creating hibernation image,
> +	 *                            before writing it

That's THAW. Also called after failing the snapshot or restore (RECOVER).

> +	 * @poweroff, @poweroff_late: called after writing hibernation image,
> +	 *                            before rebooting
> +	 * @restore, @restore_early : called after rebooting

"after restoring the hibernation image" is what I'd say. That's RESTORE.

> +	 */
>  	.freeze = i915_pm_suspend,
>  	.freeze_late = i915_pm_suspend_late,
>  	.thaw_early = i915_pm_resume_early,
> @@ -1518,6 +1529,8 @@ static const struct dev_pm_ops i915_pm_ops = {
>  	.poweroff_late = i915_pm_suspend_late,
>  	.restore_early = i915_pm_resume_early,
>  	.restore = i915_pm_resume,
> +
> +	/* D0<->D3 (runtime PM) event handlers */
>  	.runtime_suspend = intel_runtime_suspend,
>  	.runtime_resume = intel_runtime_resume,
>  };
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend

[Ville Syrjälä]

On Thu, Sep 11, 2014 at 07:15:27AM -0700, Jesse Barnes wrote:
> On Thu, 11 Sep 2014 14:59:35 +0300
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > On Thu, 2014-09-11 at 08:49 +0100, Chris Wilson wrote:
> > > On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote:
> > > > Since correctness wins over optimal code and since the optimization
> > > 
> > > Optimal code is also correct ;-) s/optimal/just plain broken/
> > 
> > Yes, bad wording. To clarify, since the optimization is now always off
> > anyway (and it's also broken), I would hope that we could remove it for
> > now and concentrate on fixing the existing s/r issues. Once we find that
> > things are stable enough we could add back this optimization.
> 
> Arg, I guess we didn't test after moving to the opregion test?  Or maybe
> it was working when it landed for S3 and not for S4?  Or broke sometime
> after it landed?
> 
> Anyway this is a really valuable optimization for resume time on some
> platforms, and really we shouldn't have other agents clobbering our GTT
> on resume, so I'd really like to fix/re-add it asap.

If/when we add this back I would suggest that we also add a sanity check
that can be enabled with drm.debug which would verify the GTT mapping
are what we expect. That way at least most developer would have it
enabled and we'd catch problems earlier, and also bug reporters would
be forced to enable it when we ask for dmesg w/ debugs.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 16/16] drm/i915: add comments on what stage a given PM handler is called

[Imre Deak]

On Tue, 2014-10-21 at 16:42 +0300, Ville Syrjälä wrote:
> On Wed, Sep 10, 2014 at 06:17:09PM +0300, Imre Deak wrote:
> > This will hopefully make it easier to navigate the code without the need
> > to consult the full PM documentation.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 55b04e6..44c9317 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1506,10 +1506,21 @@ static int intel_resume_prepare(struct drm_i915_private *dev_priv,
> >  }
> >  
> >  static const struct dev_pm_ops i915_pm_ops = {
> > +	/* S0ix, S3 event handlers */
> 
> Well, S0ix can occur in due to runtime PM as well.

Ok, I'll add a note about this. 

> 
> >  	.suspend = i915_pm_suspend,
> >  	.suspend_late = i915_pm_suspend_late,
> >  	.resume_early = i915_pm_resume_early,
> >  	.resume = i915_pm_resume,
> > +
> > +	/*
> > +	 * S4 event handlers
> > +	 * @freeze, @freeze_late    : called before creating hibernation image
> 
> That's FREEZE. Also called before restoring the hibenation image (QUIESCE).

Yes, I noticed this and sent a v2 with that added.

> 
> > +	 * @thaw, @thaw_early       : called after creating hibernation image,
> > +	 *                            before writing it
> 
> That's THAW. Also called after failing the snapshot or restore (RECOVER).

Ok, will add this.

> 
> > +	 * @poweroff, @poweroff_late: called after writing hibernation image,
> > +	 *                            before rebooting
> > +	 * @restore, @restore_early : called after rebooting
> 
> "after restoring the hibernation image" is what I'd say. That's RESTORE.

This is also clarified in v2.

> 
> > +	 */
> >  	.freeze = i915_pm_suspend,
> >  	.freeze_late = i915_pm_suspend_late,
> >  	.thaw_early = i915_pm_resume_early,
> > @@ -1518,6 +1529,8 @@ static const struct dev_pm_ops i915_pm_ops = {
> >  	.poweroff_late = i915_pm_suspend_late,
> >  	.restore_early = i915_pm_resume_early,
> >  	.restore = i915_pm_resume,
> > +
> > +	/* D0<->D3 (runtime PM) event handlers */
> >  	.runtime_suspend = intel_runtime_suspend,
> >  	.runtime_resume = intel_runtime_resume,
> >  };
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 06:16:53PM +0300, Imre Deak wrote:
> The first part of the patchset (1-6) fixes an S4 bug on VLV introduced
> recently. The rest unifies the various S3/S4 handlers, which are in
> practice the same. The only real difference - besides some unused code -
> is that during S3 suspend we disable the PCI device whereas across an S4
> freeze/thaw we keep it enabled. Given that we disable everything else
> anyway, we can just as well disable the PCI device there too. Doing so
> allows us to handle S3 suspend/resume and S4 freeze/thaw/restore/
> power-off the same way, simplifying/clarifying things quite a bit.
> 
> I smoke tested this on BDW, HSW, VLV (BYT-M), IVB, GEN45.
> 
> Imre Deak (16):
>   drm/i915: vlv: fix gunit HW state corruption during S4 suspend
>   drm/i915: remove dead code from legacy suspend handler
>   drm/i915: factor out i915_drm_suspend_late
>   drm/i915: unify legacy S3 suspend and S4 freeze handlers
>   drm/i915: propagate error from legacy resume handler
>   drm/i915: vlv: fix switcheroo/legacy suspend/resume
>   drm/i915: fix S4 suspend while switcheroo state is off
>   drm/i915: remove unused restore_gtt_mappings optimization during
>     suspend
>   drm/i915: check for GT faults during S3 resume and S4 restore too
>   drm/i915: enable output polling during S4 thaw
>   drm/i915: disable/re-enable PCI device around S4 freeze/thaw
>   drm/i915: unify S3 and S4 suspend/resume handlers
>   drm/i915: sanitize suspend/resume helper function names
>   drm/i915: add poweroff_late handler
>   drm/i915: unify switcheroo and legacy suspend/resume handlers
>   drm/i915: add comments on what stage a given PM handler is called

It all looks pretty decent to me. You can slap

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

onto everything except 09/16. I was a bit lazy and didn't look at that
one since Chris has already checked it.

> 
>  drivers/gpu/drm/i915/i915_dma.c |   4 +-
>  drivers/gpu/drm/i915/i915_drv.c | 185 +++++++++++++++-------------------------
>  drivers/gpu/drm/i915/i915_drv.h |   4 +-
>  3 files changed, 74 insertions(+), 119 deletions(-)
> 
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend

[Jesse Barnes]

On Tue, 21 Oct 2014 16:50:12 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Thu, Sep 11, 2014 at 07:15:27AM -0700, Jesse Barnes wrote:
> > On Thu, 11 Sep 2014 14:59:35 +0300
> > Imre Deak <imre.deak@intel.com> wrote:
> > 
> > > On Thu, 2014-09-11 at 08:49 +0100, Chris Wilson wrote:
> > > > On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote:
> > > > > Since correctness wins over optimal code and since the
> > > > > optimization
> > > > 
> > > > Optimal code is also correct ;-) s/optimal/just plain broken/
> > > 
> > > Yes, bad wording. To clarify, since the optimization is now
> > > always off anyway (and it's also broken), I would hope that we
> > > could remove it for now and concentrate on fixing the existing
> > > s/r issues. Once we find that things are stable enough we could
> > > add back this optimization.
> > 
> > Arg, I guess we didn't test after moving to the opregion test?  Or
> > maybe it was working when it landed for S3 and not for S4?  Or
> > broke sometime after it landed?
> > 
> > Anyway this is a really valuable optimization for resume time on
> > some platforms, and really we shouldn't have other agents
> > clobbering our GTT on resume, so I'd really like to fix/re-add it
> > asap.
> 
> If/when we add this back I would suggest that we also add a sanity
> check that can be enabled with drm.debug which would verify the GTT
> mapping are what we expect. That way at least most developer would
> have it enabled and we'd catch problems earlier, and also bug
> reporters would be forced to enable it when we ask for dmesg w/
> debugs.

Yeah we had that awhile back iirc, but I think it got bikeshedded to
death.  I'd be happy if someone resurrected it; I'd give it an r-b
without suggesting more sophisticated checksum algorithms. :)

http://lists.freedesktop.org/archives/intel-gfx/2012-September/020305.html

Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx