drm/i915: optionally check GTT checksum across suspend/resume

Commit Message

Jesse Barnes Sept. 12, 2012, 8:27 p.m. UTC

This adds a module parameter, gtt_suspend_verify, that will enable a simple
GTT checksum across suspend and resume.

We currently spend a good amount of time (>20ms) clearing all the GTT
PTEs at resume time, even though this may not be necessary on some
machines.  This debug feature is intended to help determine whether
newer machines don't need the unconditional GTT clear on resume.

Signed-off-by: Jesse Barnes <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.c |   33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |    2 ++
 2 files changed, 35 insertions(+), 0 deletions(-)

Comments

Ben Widawsky Sept. 13, 2012, 6:02 a.m. UTC | #1

On Wed, 12 Sep 2012 13:27:10 -0700
Jesse Barnes <[e-mail]> wrote:

> This adds a module parameter, gtt_suspend_verify, that will enable a
> simple GTT checksum across suspend and resume.
> 
> We currently spend a good amount of time (>20ms) clearing all the GTT
> PTEs at resume time, even though this may not be necessary on some
> machines.  This debug feature is intended to help determine whether
> newer machines don't need the unconditional GTT clear on resume.
> 
> Signed-off-by: Jesse Barnes <[e-mail]>


It's a cool idea, but I'd vote against adding a module parameter to
verify it. Daniel's trees should be well enough tested that
you could just disable it on IVB+ or whatever and let it go through QA
and developer testing. Aside from that, a debugfs flag to toggle it
would probably be a little better.

Also, as a bikeshed you could probably get a much better detection with
a CRC or something similar.

I dunno, just a thought,

Keith Packard Sept. 13, 2012, 2:48 p.m. UTC | #2

Ben Widawsky <[e-mail]> writes:

> Also, as a bikeshed you could probably get a much better detection with
> a CRC or something similar.

Wonder if you could use the existing MD5 code in the kernel; would avoid
all sorts of unfortunate false-positives.

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0d4f37e..338c178 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -119,6 +119,12 @@  module_param_named(i915_enable_ppgtt, i915_enable_ppgtt, int, 0600);
 MODULE_PARM_DESC(i915_enable_ppgtt,
 		"Enable PPGTT (default: true)");
 
+int i915_gtt_suspend_verify __read_mostly = 0;
+module_param_named(gtt_suspend_verify, i915_gtt_suspend_verify, int, 0600);
+MODULE_PARM_DESC(gtt_suspend_verify,
+		 "Verify GTT doesn't change across suspend/resume "
+		 "(default: false)");
+
 static struct drm_driver driver;
 extern int intel_agp_enabled;
 
@@ -454,6 +460,22 @@  bool i915_semaphore_is_enabled(struct drm_device *dev)
 	return 1;
 }
 
+static unsigned long intel_gtt_checksum(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long checksum = 0;
+	u32 *entries;
+	int i;
+
+	entries = ioremap(dev_priv->mm.gtt_base_addr,
+			  dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT);
+	for (i = 0; i < dev_priv->mm.gtt->gtt_mappable_entries; i++)
+		checksum += entries[i];
+	iounmap(entries);
+
+	return checksum;
+}
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -487,6 +509,9 @@  static int i915_drm_freeze(struct drm_device *dev)
 	intel_fbdev_set_suspend(dev, 1);
 	console_unlock();
 
+	if (i915_gtt_suspend_verify)
+		dev_priv->gtt_checksum = intel_gtt_checksum(dev);
+
 	return 0;
 }
 
@@ -525,6 +550,14 @@  static int i915_drm_thaw(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int error = 0;
 
+	if (i915_gtt_suspend_verify) {
+		unsigned long checksum;
+
+		checksum = intel_gtt_checksum(dev);
+		WARN(checksum != dev_priv->gtt_checksum,
+		     "GTT checksum mismatch (pre-suspend: 0x%08x vs current: 0x%08x)\n", dev_priv->gtt_checksum, checksum);
+	}
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		mutex_lock(&dev->struct_mutex);
 		i915_gem_restore_gtt_mappings(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9fce782..65cb051 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -524,6 +524,8 @@  typedef struct drm_i915_private {
 
 	unsigned long quirks;
 
+	unsigned long gtt_checksum;
+
 	/* Register state */
 	bool modeset_on_lid;
 	u8 saveLBB;