Re: [Intel-gfx] [PATCH] drm/i915: Don't mask EI UP interrupt on IVB|SNB

2013-08-23 Thread Azad, Vinit 
Thanks Mika. It looks good to me.

Reviewed-by: Vinit Azad 
-Original Message-
From: Mika Kuoppala [mailto:mika.kuopp...@linux.intel.com] 
Sent: Thursday, August 22, 2013 11:09
To: intel-gfx@lists.freedesktop.org
Cc: Mika Kuoppala; Azad, Vinit
Subject: [PATCH] drm/i915: Don't mask EI UP interrupt on IVB|SNB

Submitting a batchbuffer which simulates a gpu hang by doing 
MI_BATCH_BUFFER_START into itself, to test hangcheck, started to hard hang the 
whole box (IVB). Bisecting lead to this commit:

commit 664b422c2966cd39b8f67e8d53a566ea8c877cd6
Author: Vinit Azad 
Date:   Wed Aug 14 13:34:33 2013 -0700

drm/i915: Only unmask required PM interrupts

Experimenting with the mask register showed that unmasking EI UP will prevent 
the hard hang in IVB and SNB.
HSW doesn't hang with EI UP masked.

Considering we are just disabling interrupts that aren't even delivered to 
driver, this change is more likely to paper over some weirdness in gpu's 
internal state machine. But until better explanation can be found, let's trade 
little bit of power for stability on these architectures.

v2: - Unmask EI_EXPIRED directly in I915_WRITE (Vinit)
v3: - Only unmask on SNB and IVB

Cc: Vinit Azad 
Signed-off-by: Mika Kuoppala 
---
 drivers/gpu/drm/i915/intel_pm.c |   12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c 
index cbab95dc..e6a124e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3447,14 +3447,24 @@ int intel_enable_rc6(const struct drm_device *dev)  
static void gen6_enable_rps_interrupts(struct drm_device *dev)  {
struct drm_i915_private *dev_priv = dev->dev_private;
+   u32 enabled_intrs;
 
spin_lock_irq(&dev_priv->irq_lock);
WARN_ON(dev_priv->rps.pm_iir);
snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS);
I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
spin_unlock_irq(&dev_priv->irq_lock);
+
/* only unmask PM interrupts we need. Mask all others. */
-   I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS);
+   enabled_intrs = GEN6_PM_RPS_EVENTS;
+
+   /* IVB and SNB hard hangs on looping batchbuffer
+* if GEN6_PM_UP_EI_EXPIRED is masked.
+*/
+   if (INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev))
+   enabled_intrs |= GEN6_PM_RP_UP_EI_EXPIRED;
+
+   I915_WRITE(GEN6_PMINTRMSK, ~enabled_intrs);
 }
 
 static void gen6_enable_rps(struct drm_device *dev)
--
1.7.9.5

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

Re: [Intel-gfx] [PATCH] drm/i915: Only unmask required PM interrupts

2013-08-14 Thread Azad, Vinit 
Our interrupt handler isn't being fired since we do set the IER bits properly 
(IIR bits aren't set). The overhead isn't because our driver is reacting to 
these interrupts, but because hardware keeps generating internal messages when 
PMINTRMSK doesn't mask out the up/down EI interrupts (which happen 
periodically).

Unfortunately, the default value of PMINTRMSK register is 0 instead of 
0x as it is for other IMR, so we have to mask out the interrupts 
manually.

-Original Message-
From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of 
Daniel Vetter
Sent: Wednesday, August 14, 2013 13:47
To: Azad, Vinit
Cc: intel-gfx
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Only unmask required PM interrupts

On Wed, Aug 14, 2013 at 10:34 PM, Vinit Azad  wrote:
> Un-masking all PM interrupts causes hardware to generate interrupts 
> regardless of whether the interrupts are enabled on the DE side. Since 
> turbo only need up/down threshold and
> rc6 timeout interrupt, mask all other interrupts bits to avoid 
> unnecessary overhead/wake up.

Just to clarify since I can't really believe this yet: Even though we disable 
all other interrupt sources in PMIER and mask them in PMIMR hw still manages to 
fire off our interrupt handler? Do those interrupts end up setting PMIIR?

Thanks, Daniel
>
> Change-Id: I6c947df6fd5f60584d39b9e8b8c89faa51a5e827
> Signed-off-by: Vinit Azad 
> ---
>  drivers/gpu/drm/i915/intel_pm.c |8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
> b/drivers/gpu/drm/i915/intel_pm.c index 8e9ce07..17a0dae 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3328,8 +3328,8 @@ static void gen6_enable_rps(struct drm_device *dev)
> I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> spin_unlock_irq(&dev_priv->rps.lock);
> -   /* unmask all PM interrupts */
> -   I915_WRITE(GEN6_PMINTRMSK, 0);
> +   /* only unmask PM interrupts we need. Mask all others. */
> +   I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS);
>
> rc6vids = 0;
> ret = sandybridge_pcode_read(dev_priv, 
> GEN6_PCODE_READ_RC6VIDS, &rc6vids); @@ -3596,8 +3596,8 @@ static void 
> valleyview_enable_rps(struct drm_device *dev)
> WARN_ON(dev_priv->rps.pm_iir != 0);
> I915_WRITE(GEN6_PMIMR, 0);
> spin_unlock_irq(&dev_priv->rps.lock);
> -   /* enable all PM interrupts */
> -   I915_WRITE(GEN6_PMINTRMSK, 0);
> +   /* enable only the PM interrupts we need. Mask everything else */
> +   I915_WRITE(GEN6_PMINTRMSK, ~GEN6_PM_RPS_EVENTS);
>
> gen6_gt_force_wake_put(dev_priv);  }
> --
> 1.7.9.5
>
> ___
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx