diff mbox

[11/19] drm/i915: disable interrupts when enabling PC8

Message ID 1385048853-1579-12-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Nov. 21, 2013, 3:47 p.m. UTC 
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The plan is to merge PC8 and D3 into a single feature, and when we're
in D3 we won't get any hotplug interrupt anyway, so leaving them
enable doesn't make sense, and it also brings us a problem. The
problem is that we get a hotplug interrupt right when we we wake up
from D3, when we're still waking up everything. If we fully disable
interrupts we won't get this hotplug interrupt, so we won't have
problems.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

Comments

Rodrigo Vivi Dec. 2, 2013, 1:33 p.m. UTC | #1 
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Thu, Nov 21, 2013 at 1:47 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> The plan is to merge PC8 and D3 into a single feature, and when we're
> in D3 we won't get any hotplug interrupt anyway, so leaving them
> enable doesn't make sense, and it also brings us a problem. The
> problem is that we get a hotplug interrupt right when we we wake up
> from D3, when we're still waking up everything. If we fully disable
> interrupts we won't get this hotplug interrupt, so we won't have
> problems.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 70c4cef..d0f4e61 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3902,8 +3902,8 @@ void hsw_pc8_disable_interrupts(struct drm_device *dev)
>         dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
>         dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
>
> -       ironlake_disable_display_irq(dev_priv, ~DE_PCH_EVENT_IVB);
> -       ibx_disable_display_interrupt(dev_priv, ~SDE_HOTPLUG_MASK_CPT);
> +       ironlake_disable_display_irq(dev_priv, 0xffffffff);
> +       ibx_disable_display_interrupt(dev_priv, 0xffffffff);
>         ilk_disable_gt_irq(dev_priv, 0xffffffff);
>         snb_disable_pm_irq(dev_priv, 0xffffffff);
>
> @@ -3917,34 +3917,26 @@ void hsw_pc8_restore_interrupts(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         unsigned long irqflags;
> -       uint32_t val, expected;
> +       uint32_t val;
>
>         spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>
>         val = I915_READ(DEIMR);
> -       expected = ~DE_PCH_EVENT_IVB;
> -       WARN(val != expected, "DEIMR is 0x%08x, not 0x%08x\n", val, expected);
> +       WARN(val != 0xffffffff, "DEIMR is 0x%08x\n", val);
>
> -       val = I915_READ(SDEIMR) & ~SDE_HOTPLUG_MASK_CPT;
> -       expected = ~SDE_HOTPLUG_MASK_CPT;
> -       WARN(val != expected, "SDEIMR non-HPD bits are 0x%08x, not 0x%08x\n",
> -            val, expected);
> +       val = I915_READ(SDEIMR);
> +       WARN(val != 0xffffffff, "SDEIMR is 0x%08x\n", val);
>
>         val = I915_READ(GTIMR);
> -       expected = 0xffffffff;
> -       WARN(val != expected, "GTIMR is 0x%08x, not 0x%08x\n", val, expected);
> +       WARN(val != 0xffffffff, "GTIMR is 0x%08x\n", val);
>
>         val = I915_READ(GEN6_PMIMR);
> -       expected = 0xffffffff;
> -       WARN(val != expected, "GEN6_PMIMR is 0x%08x, not 0x%08x\n", val,
> -            expected);
> +       WARN(val != 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val);
>
>         dev_priv->pc8.irqs_disabled = false;
>
>         ironlake_enable_display_irq(dev_priv, ~dev_priv->pc8.regsave.deimr);
> -       ibx_enable_display_interrupt(dev_priv,
> -                                    ~dev_priv->pc8.regsave.sdeimr &
> -                                    ~SDE_HOTPLUG_MASK_CPT);
> +       ibx_enable_display_interrupt(dev_priv, ~dev_priv->pc8.regsave.sdeimr);
>         ilk_enable_gt_irq(dev_priv, ~dev_priv->pc8.regsave.gtimr);
>         snb_enable_pm_irq(dev_priv, ~dev_priv->pc8.regsave.gen6_pmimr);
>         I915_WRITE(GTIER, dev_priv->pc8.regsave.gtier);
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Dec. 10, 2013, 9:59 p.m. UTC | #2 
On Thu, Nov 21, 2013 at 01:47:25PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The plan is to merge PC8 and D3 into a single feature, and when we're
> in D3 we won't get any hotplug interrupt anyway, so leaving them
> enable doesn't make sense, and it also brings us a problem. The
> problem is that we get a hotplug interrupt right when we we wake up
> from D3, when we're still waking up everything. If we fully disable
> interrupts we won't get this hotplug interrupt, so we won't have
> problems.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Now that we forgo the partial interrupt enabling of pc8 there's imo no
need any more for the pc8 interrup reg save/restore dance. Instead it'd be
much better to just disable the interrup by disabling the interrupt
handler and then when reenabling things to use our core interrupt enabling
functions.

This way the runtime d3 path uses the same code as resume and driver load.
Furthermore the D3 code will be a bit more generic, which helps with
enabling platforms. But this can (should) be done in a follow-up.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 70c4cef..d0f4e61 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3902,8 +3902,8 @@ void hsw_pc8_disable_interrupts(struct drm_device *dev)
>  	dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
>  	dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
>  
> -	ironlake_disable_display_irq(dev_priv, ~DE_PCH_EVENT_IVB);
> -	ibx_disable_display_interrupt(dev_priv, ~SDE_HOTPLUG_MASK_CPT);
> +	ironlake_disable_display_irq(dev_priv, 0xffffffff);
> +	ibx_disable_display_interrupt(dev_priv, 0xffffffff);
>  	ilk_disable_gt_irq(dev_priv, 0xffffffff);
>  	snb_disable_pm_irq(dev_priv, 0xffffffff);
>  
> @@ -3917,34 +3917,26 @@ void hsw_pc8_restore_interrupts(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
> -	uint32_t val, expected;
> +	uint32_t val;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  
>  	val = I915_READ(DEIMR);
> -	expected = ~DE_PCH_EVENT_IVB;
> -	WARN(val != expected, "DEIMR is 0x%08x, not 0x%08x\n", val, expected);
> +	WARN(val != 0xffffffff, "DEIMR is 0x%08x\n", val);
>  
> -	val = I915_READ(SDEIMR) & ~SDE_HOTPLUG_MASK_CPT;
> -	expected = ~SDE_HOTPLUG_MASK_CPT;
> -	WARN(val != expected, "SDEIMR non-HPD bits are 0x%08x, not 0x%08x\n",
> -	     val, expected);
> +	val = I915_READ(SDEIMR);
> +	WARN(val != 0xffffffff, "SDEIMR is 0x%08x\n", val);
>  
>  	val = I915_READ(GTIMR);
> -	expected = 0xffffffff;
> -	WARN(val != expected, "GTIMR is 0x%08x, not 0x%08x\n", val, expected);
> +	WARN(val != 0xffffffff, "GTIMR is 0x%08x\n", val);
>  
>  	val = I915_READ(GEN6_PMIMR);
> -	expected = 0xffffffff;
> -	WARN(val != expected, "GEN6_PMIMR is 0x%08x, not 0x%08x\n", val,
> -	     expected);
> +	WARN(val != 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val);
>  
>  	dev_priv->pc8.irqs_disabled = false;
>  
>  	ironlake_enable_display_irq(dev_priv, ~dev_priv->pc8.regsave.deimr);
> -	ibx_enable_display_interrupt(dev_priv,
> -				     ~dev_priv->pc8.regsave.sdeimr &
> -				     ~SDE_HOTPLUG_MASK_CPT);
> +	ibx_enable_display_interrupt(dev_priv, ~dev_priv->pc8.regsave.sdeimr);
>  	ilk_enable_gt_irq(dev_priv, ~dev_priv->pc8.regsave.gtimr);
>  	snb_enable_pm_irq(dev_priv, ~dev_priv->pc8.regsave.gen6_pmimr);
>  	I915_WRITE(GTIER, dev_priv->pc8.regsave.gtier);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Paulo Zanoni Dec. 11, 2013, 9:33 p.m. UTC | #3 
2013/12/10 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Nov 21, 2013 at 01:47:25PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> The plan is to merge PC8 and D3 into a single feature, and when we're
>> in D3 we won't get any hotplug interrupt anyway, so leaving them
>> enable doesn't make sense, and it also brings us a problem. The
>> problem is that we get a hotplug interrupt right when we we wake up
>> from D3, when we're still waking up everything. If we fully disable
>> interrupts we won't get this hotplug interrupt, so we won't have
>> problems.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Now that we forgo the partial interrupt enabling of pc8 there's imo no
> need any more for the pc8 interrup reg save/restore dance. Instead it'd be
> much better to just disable the interrup by disabling the interrupt
> handler and then when reenabling things to use our core interrupt enabling
> functions.
>

I tried to do this in August, in one of the early PC8 implementations.
I showed you my implementation, we discussed and then concluded the
current approach was better. Of course, at that time I was trying to
keep the hotplug interrupts alive, so the code will look a little
better now, but I don't see too much benefit.


> This way the runtime d3 path uses the same code as resume and driver load.

Actually I recently had this crazy idea of doing the opposite: make
the suspend/resume code use the runtime PM code. But that's not
something I investigated deeply.


> Furthermore the D3 code will be a bit more generic, which helps with
> enabling platforms. But this can (should) be done in a follow-up.

I'll add it to the list.

> -Daniel
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 26 +++++++++-----------------
>>  1 file changed, 9 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 70c4cef..d0f4e61 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3902,8 +3902,8 @@ void hsw_pc8_disable_interrupts(struct drm_device *dev)
>>       dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
>>       dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
>>
>> -     ironlake_disable_display_irq(dev_priv, ~DE_PCH_EVENT_IVB);
>> -     ibx_disable_display_interrupt(dev_priv, ~SDE_HOTPLUG_MASK_CPT);
>> +     ironlake_disable_display_irq(dev_priv, 0xffffffff);
>> +     ibx_disable_display_interrupt(dev_priv, 0xffffffff);
>>       ilk_disable_gt_irq(dev_priv, 0xffffffff);
>>       snb_disable_pm_irq(dev_priv, 0xffffffff);
>>
>> @@ -3917,34 +3917,26 @@ void hsw_pc8_restore_interrupts(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       unsigned long irqflags;
>> -     uint32_t val, expected;
>> +     uint32_t val;
>>
>>       spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>>
>>       val = I915_READ(DEIMR);
>> -     expected = ~DE_PCH_EVENT_IVB;
>> -     WARN(val != expected, "DEIMR is 0x%08x, not 0x%08x\n", val, expected);
>> +     WARN(val != 0xffffffff, "DEIMR is 0x%08x\n", val);
>>
>> -     val = I915_READ(SDEIMR) & ~SDE_HOTPLUG_MASK_CPT;
>> -     expected = ~SDE_HOTPLUG_MASK_CPT;
>> -     WARN(val != expected, "SDEIMR non-HPD bits are 0x%08x, not 0x%08x\n",
>> -          val, expected);
>> +     val = I915_READ(SDEIMR);
>> +     WARN(val != 0xffffffff, "SDEIMR is 0x%08x\n", val);
>>
>>       val = I915_READ(GTIMR);
>> -     expected = 0xffffffff;
>> -     WARN(val != expected, "GTIMR is 0x%08x, not 0x%08x\n", val, expected);
>> +     WARN(val != 0xffffffff, "GTIMR is 0x%08x\n", val);
>>
>>       val = I915_READ(GEN6_PMIMR);
>> -     expected = 0xffffffff;
>> -     WARN(val != expected, "GEN6_PMIMR is 0x%08x, not 0x%08x\n", val,
>> -          expected);
>> +     WARN(val != 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val);
>>
>>       dev_priv->pc8.irqs_disabled = false;
>>
>>       ironlake_enable_display_irq(dev_priv, ~dev_priv->pc8.regsave.deimr);
>> -     ibx_enable_display_interrupt(dev_priv,
>> -                                  ~dev_priv->pc8.regsave.sdeimr &
>> -                                  ~SDE_HOTPLUG_MASK_CPT);
>> +     ibx_enable_display_interrupt(dev_priv, ~dev_priv->pc8.regsave.sdeimr);
>>       ilk_enable_gt_irq(dev_priv, ~dev_priv->pc8.regsave.gtimr);
>>       snb_enable_pm_irq(dev_priv, ~dev_priv->pc8.regsave.gen6_pmimr);
>>       I915_WRITE(GTIER, dev_priv->pc8.regsave.gtier);
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> 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
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 70c4cef..d0f4e61 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3902,8 +3902,8 @@  void hsw_pc8_disable_interrupts(struct drm_device *dev)
 	dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
 	dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
 
-	ironlake_disable_display_irq(dev_priv, ~DE_PCH_EVENT_IVB);
-	ibx_disable_display_interrupt(dev_priv, ~SDE_HOTPLUG_MASK_CPT);
+	ironlake_disable_display_irq(dev_priv, 0xffffffff);
+	ibx_disable_display_interrupt(dev_priv, 0xffffffff);
 	ilk_disable_gt_irq(dev_priv, 0xffffffff);
 	snb_disable_pm_irq(dev_priv, 0xffffffff);
 
@@ -3917,34 +3917,26 @@  void hsw_pc8_restore_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
-	uint32_t val, expected;
+	uint32_t val;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
 	val = I915_READ(DEIMR);
-	expected = ~DE_PCH_EVENT_IVB;
-	WARN(val != expected, "DEIMR is 0x%08x, not 0x%08x\n", val, expected);
+	WARN(val != 0xffffffff, "DEIMR is 0x%08x\n", val);
 
-	val = I915_READ(SDEIMR) & ~SDE_HOTPLUG_MASK_CPT;
-	expected = ~SDE_HOTPLUG_MASK_CPT;
-	WARN(val != expected, "SDEIMR non-HPD bits are 0x%08x, not 0x%08x\n",
-	     val, expected);
+	val = I915_READ(SDEIMR);
+	WARN(val != 0xffffffff, "SDEIMR is 0x%08x\n", val);
 
 	val = I915_READ(GTIMR);
-	expected = 0xffffffff;
-	WARN(val != expected, "GTIMR is 0x%08x, not 0x%08x\n", val, expected);
+	WARN(val != 0xffffffff, "GTIMR is 0x%08x\n", val);
 
 	val = I915_READ(GEN6_PMIMR);
-	expected = 0xffffffff;
-	WARN(val != expected, "GEN6_PMIMR is 0x%08x, not 0x%08x\n", val,
-	     expected);
+	WARN(val != 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val);
 
 	dev_priv->pc8.irqs_disabled = false;
 
 	ironlake_enable_display_irq(dev_priv, ~dev_priv->pc8.regsave.deimr);
-	ibx_enable_display_interrupt(dev_priv,
-				     ~dev_priv->pc8.regsave.sdeimr &
-				     ~SDE_HOTPLUG_MASK_CPT);
+	ibx_enable_display_interrupt(dev_priv, ~dev_priv->pc8.regsave.sdeimr);
 	ilk_enable_gt_irq(dev_priv, ~dev_priv->pc8.regsave.gtimr);
 	snb_enable_pm_irq(dev_priv, ~dev_priv->pc8.regsave.gen6_pmimr);
 	I915_WRITE(GTIER, dev_priv->pc8.regsave.gtier);