Warnings/Errors during i915 suspend/resume on linux 3.0-rc5
diff mbox

Message ID yund3hx5bg8.fsf@aiko.keithp.com
State New, archived
Headers show

Commit Message

Keith Packard June 29, 2011, 7:44 a.m. UTC
On Wed, 29 Jun 2011 06:27:53 +0100, Marcin Nowakowski <marcin.nowakowski.000@gmail.com> wrote:

> There was an enormous amount of such warnings in rc3/rc4, but with the
> latest fixes the number is significantly reduced - some still
> remaining though:

Thanks for finding a few more. Here's a patch which fixes these on my
system:

From 84a46e9ba3c077535c22a006c5da9988524a6b8b Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Wed, 29 Jun 2011 00:30:34 -0700
Subject: [PATCH] drm/i915: Hold struct_mutex during
 i915_save_state/i915_restore_state

Lots of register access in these functions, some of which requires the
struct mutex.

These functions now hold the struct mutex across the calls to
i915_save_display and i915_restore_display, and so the internal mutex
calls in those functions have been removed. To ensure that no-one else
was calling them (and hence violating the new required locking
invarient), those functions have been made static.

gen6_enable_rps locks the struct mutex, and so i915_restore_state
unlocks the mutex around calls to that function.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |    2 --
 drivers/gpu/drm/i915/i915_suspend.c |   19 +++++++++++++------
 2 files changed, 13 insertions(+), 8 deletions(-)

Comments

Marcin Nowakowski June 29, 2011, 6:23 p.m. UTC | #1
On Wed, Jun 29, 2011 at 8:44 AM, Keith Packard <keithp@keithp.com> wrote:
> On Wed, 29 Jun 2011 06:27:53 +0100, Marcin Nowakowski <marcin.nowakowski.000@gmail.com> wrote:
>
>> There was an enormous amount of such warnings in rc3/rc4, but with the
>> latest fixes the number is significantly reduced - some still
>> remaining though:
>
> Thanks for finding a few more. Here's a patch which fixes these on my
> system:
>
> From 84a46e9ba3c077535c22a006c5da9988524a6b8b Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp@keithp.com>
> Date: Wed, 29 Jun 2011 00:30:34 -0700
> Subject: [PATCH] drm/i915: Hold struct_mutex during
>  i915_save_state/i915_restore_state
>
> Lots of register access in these functions, some of which requires the
> struct mutex.
>
> These functions now hold the struct mutex across the calls to
> i915_save_display and i915_restore_display, and so the internal mutex
> calls in those functions have been removed. To ensure that no-one else
> was calling them (and hence violating the new required locking
> invarient), those functions have been made static.
>
> gen6_enable_rps locks the struct mutex, and so i915_restore_state
> unlocks the mutex around calls to that function.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>


All warnings now gone, thanks.

What about the *ERROR* lines I mentioned in the first email? This
patch doesn't address those - are they anything serious (otherwise
they probably wouldn't be marked as errors)?

Marcin
Keith Packard June 29, 2011, 6:43 p.m. UTC | #2
On Wed, 29 Jun 2011 19:23:06 +0100, Marcin Nowakowski <marcin.nowakowski.000@gmail.com> wrote:

> What about the *ERROR* lines I mentioned in the first email? This
> patch doesn't address those - are they anything serious (otherwise
> they probably wouldn't be marked as errors)?

That's an error in the X driver up in user space. The only call I see is
From sna; are you using that?

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index eddabf6..1c8bfb1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -997,8 +997,6 @@  extern unsigned int i915_enable_fbc;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
-extern void i915_save_display(struct drm_device *dev);
-extern void i915_restore_display(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);
 
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index e8152d2..5257cfc 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -597,7 +597,7 @@  static void i915_restore_modeset_reg(struct drm_device *dev)
 	return;
 }
 
-void i915_save_display(struct drm_device *dev)
+static void i915_save_display(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -678,7 +678,6 @@  void i915_save_display(struct drm_device *dev)
 	}
 
 	/* VGA state */
-	mutex_lock(&dev->struct_mutex);
 	dev_priv->saveVGA0 = I915_READ(VGA0);
 	dev_priv->saveVGA1 = I915_READ(VGA1);
 	dev_priv->saveVGA_PD = I915_READ(VGA_PD);
@@ -688,10 +687,9 @@  void i915_save_display(struct drm_device *dev)
 		dev_priv->saveVGACNTRL = I915_READ(VGACNTRL);
 
 	i915_save_vga(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
-void i915_restore_display(struct drm_device *dev)
+static void i915_restore_display(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -783,7 +781,6 @@  void i915_restore_display(struct drm_device *dev)
 	else
 		I915_WRITE(VGACNTRL, dev_priv->saveVGACNTRL);
 
-	mutex_lock(&dev->struct_mutex);
 	I915_WRITE(VGA0, dev_priv->saveVGA0);
 	I915_WRITE(VGA1, dev_priv->saveVGA1);
 	I915_WRITE(VGA_PD, dev_priv->saveVGA_PD);
@@ -791,7 +788,6 @@  void i915_restore_display(struct drm_device *dev)
 	udelay(150);
 
 	i915_restore_vga(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 int i915_save_state(struct drm_device *dev)
@@ -801,6 +797,8 @@  int i915_save_state(struct drm_device *dev)
 
 	pci_read_config_byte(dev->pdev, LBB, &dev_priv->saveLBB);
 
+	mutex_lock(&dev->struct_mutex);
+
 	/* Hardware status page */
 	dev_priv->saveHWS = I915_READ(HWS_PGA);
 
@@ -840,6 +838,8 @@  int i915_save_state(struct drm_device *dev)
 	for (i = 0; i < 3; i++)
 		dev_priv->saveSWF2[i] = I915_READ(SWF30 + (i << 2));
 
+	mutex_unlock(&dev->struct_mutex);
+
 	return 0;
 }
 
@@ -850,6 +850,8 @@  int i915_restore_state(struct drm_device *dev)
 
 	pci_write_config_byte(dev->pdev, LBB, dev_priv->saveLBB);
 
+	mutex_lock(&dev->struct_mutex);
+
 	/* Hardware status page */
 	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
 
@@ -867,6 +869,7 @@  int i915_restore_state(struct drm_device *dev)
 		I915_WRITE(IER, dev_priv->saveIER);
 		I915_WRITE(IMR, dev_priv->saveIMR);
 	}
+	mutex_unlock(&dev->struct_mutex);
 
 	intel_init_clock_gating(dev);
 
@@ -878,6 +881,8 @@  int i915_restore_state(struct drm_device *dev)
 	if (IS_GEN6(dev))
 		gen6_enable_rps(dev_priv);
 
+	mutex_lock(&dev->struct_mutex);
+
 	/* Cache mode state */
 	I915_WRITE (CACHE_MODE_0, dev_priv->saveCACHE_MODE_0 | 0xffff0000);
 
@@ -891,6 +896,8 @@  int i915_restore_state(struct drm_device *dev)
 	for (i = 0; i < 3; i++)
 		I915_WRITE(SWF30 + (i << 2), dev_priv->saveSWF2[i]);
 
+	mutex_unlock(&dev->struct_mutex);
+
 	intel_i2c_reset(dev);
 
 	return 0;