[PATCH 0/2] forcewake warning fixes

[PATCH 0/2] forcewake warning fixes

[Ben Widawsky]

I'm having some trouble testing these currently. Hopefully someone else can
verify them.

Ben Widawsky (2):
  drm/i915: more forcewake lock error info
  drm/i915: save/resume forcewake lock fixes

 drivers/gpu/drm/i915/i915_drv.c     |   12 ++++++++----
 drivers/gpu/drm/i915/i915_drv.h     |    8 +++++---
 drivers/gpu/drm/i915/i915_suspend.c |    5 +++++
 3 files changed, 18 insertions(+), 7 deletions(-)

-- 
1.7.5.2

[PATCH 1/2] drm/i915: more forcewake lock error info

[Ben Widawsky]

Get some more useful info for debugging backtraces on forcewake
problems. Appropriate debug flags will be required for the drm module.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   12 ++++++++----
 drivers/gpu/drm/i915/i915_drv.h |    8 +++++---
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0defd42..af59811 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -317,13 +317,15 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
  * be called at the beginning of the sequence followed by a call to
  * gen6_gt_force_wake_put() at the end of the sequence.
  */
-void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
+int gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 {
-	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+	int ret = WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
 
 	/* Forcewake is atomic in case we get in here without the lock */
 	if (atomic_add_return(1, &dev_priv->forcewake_count) == 1)
 		__gen6_gt_force_wake_get(dev_priv);
+
+	return ret;
 }
 
 static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
@@ -335,12 +337,14 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 /*
  * see gen6_gt_force_wake_get()
  */
-void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
+int gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
-	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+	int ret = WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
 
 	if (atomic_dec_and_test(&dev_priv->forcewake_count))
 		__gen6_gt_force_wake_put(dev_priv);
+
+	return ret;
 }
 
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8a9fd91..7421eb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1362,8 +1362,8 @@ extern void intel_display_print_error_state(struct seq_file *m,
  * must be set to prevent GT core from power down and stale values being
  * returned.
  */
-void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
-void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
+int gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
+int gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
 
 /* We give fast paths for the really cool registers */
@@ -1376,7 +1376,9 @@ void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
 static inline u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
 	u##x val = 0; \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		gen6_gt_force_wake_get(dev_priv); \
+		if (gen6_gt_force_wake_get(dev_priv)) { \
+			DRM_DEBUG_DRIVER("unlocked forcewake for 0x%08x\n", reg); \
+		} \
 		val = read##y(dev_priv->regs + reg); \
 		gen6_gt_force_wake_put(dev_priv); \
 	} else { \
-- 
1.7.5.2

[PATCH 2/2] drm/i915: save/resume forcewake lock fixes

[Ben Widawsky]

The lock must be held for the saving and restoring of VGA state.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
CC: Alexander Zhaunerchyk <alex.vizor@gmail.com>
CC: Andrey Rahmatullin <wrar@wrar.name>
---
 drivers/gpu/drm/i915/i915_suspend.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 60a94d2..e8152d2 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -678,6 +678,7 @@ 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);
@@ -687,6 +688,7 @@ 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)
@@ -780,6 +782,8 @@ void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(CPU_VGACNTRL, dev_priv->saveVGACNTRL);
 	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);
@@ -787,6 +791,7 @@ 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)
-- 
1.7.5.2

Re: [PATCH 1/2] drm/i915: more forcewake lock error info

[Chris Wilson]

On Wed, 22 Jun 2011 09:55:00 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Get some more useful info for debugging backtraces on forcewake
> problems. Appropriate debug flags will be required for the drm module.

Wouldn't it be neater to pass the reg down to
gen6_gt_force_wake_get() so that we only have a single debug message and
not an additional one for every inlined read/write?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: save/resume forcewake lock fixes

[Chris Wilson]

On Wed, 22 Jun 2011 09:55:01 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> The lock must be held for the saving and restoring of VGA state.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> CC: Alexander Zhaunerchyk <alex.vizor@gmail.com>
> CC: Andrey Rahmatullin <wrar@wrar.name>

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: more forcewake lock error info

[Ben Widawsky]

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

On Wed, Jun 22, 2011 at 10:00 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote:

> On Wed, 22 Jun 2011 09:55:00 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > Get some more useful info for debugging backtraces on forcewake
> > problems. Appropriate debug flags will be required for the drm module.
>
> Wouldn't it be neater to pass the reg down to
> gen6_gt_force_wake_get() so that we only have a single debug message and
> not an additional one for every inlined read/write?
> -Chris
>

Urg... having some trouble with my email right now, so you have to deal with
my gmail/webmail...

1. Yes, but I was lazy.
2. People call force_wake_get() without actually specifying a register, so I
didn't want to enforce people entering bogus info on those calls.


>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 1749 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 1/2] drm/i915: more forcewake lock error info

[Chris Wilson]

On Wed, 22 Jun 2011 10:07:50 -0700, Ben Widawsky <bwidawsk@gmail.com> wrote:
> On Wed, Jun 22, 2011 at 10:00 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote:
> 
> > On Wed, 22 Jun 2011 09:55:00 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > Get some more useful info for debugging backtraces on forcewake
> > > problems. Appropriate debug flags will be required for the drm module.
> >
> > Wouldn't it be neater to pass the reg down to
> > gen6_gt_force_wake_get() so that we only have a single debug message and
> > not an additional one for every inlined read/write?
> > -Chris
> >
> 
> Urg... having some trouble with my email right now, so you have to deal with
> my gmail/webmail...
> 
> 1. Yes, but I was lazy.
> 2. People call force_wake_get() without actually specifying a register, so I
> didn't want to enforce people entering bogus info on those calls.

We could always go for the traditional file:line automagic macros...
But we already have that from the stacktrace if people are good enough to
provide symbols.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: more forcewake lock error info

[Ben Widawsky]

On Wed, Jun 22, 2011 at 06:15:40PM +0100, Chris Wilson wrote:
> On Wed, 22 Jun 2011 10:07:50 -0700, Ben Widawsky <bwidawsk@gmail.com> wrote:
> > On Wed, Jun 22, 2011 at 10:00 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote:
> > 
> > > On Wed, 22 Jun 2011 09:55:00 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > > Get some more useful info for debugging backtraces on forcewake
> > > > problems. Appropriate debug flags will be required for the drm module.
> > >
> > > Wouldn't it be neater to pass the reg down to
> > > gen6_gt_force_wake_get() so that we only have a single debug message and
> > > not an additional one for every inlined read/write?
> > > -Chris
> > >
> > 
> > Urg... having some trouble with my email right now, so you have to deal with
> > my gmail/webmail...
> > 
> > 1. Yes, but I was lazy.
> > 2. People call force_wake_get() without actually specifying a register, so I
> > didn't want to enforce people entering bogus info on those calls.
> 
> We could always go for the traditional file:line automagic macros...
> But we already have that from the stacktrace if people are good enough to
> provide symbols.

Yeah, I figured this would be easiest as it somewhat removes variance
between commits, I can just search for the unprotected register access
on the branch du jour (especially with you guys refactoring the code all
time time...  JBARNES).

I'm fine if the consensus is to throw this one out. Seemed helpful at
the time, so I thought I'd post it.

> -Chris

Ben

Re: [PATCH 2/2] drm/i915: save/resume forcewake lock fixes

[Keith Packard]

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

On Wed, 22 Jun 2011 18:04:46 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

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

I've added this to drm-intel-fixes

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 2/2] drm/i915: save/resume forcewake lock fixes

[Andrey Rahmatullin]

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

Tested-by: Andrey Rahmatullin <wrar@wrar.name>

-- 
WBR, wRAR

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 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