[Intel-gfx] [PATCH 11/19] drm/i915: disable interrupts when enabling PC8

[Paulo Zanoni]

2013/12/10 Daniel Vetter <daniel at ffwll.ch>:
> On Thu, Nov 21, 2013 at 01:47:25PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at 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 at 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 at 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



-- 
Paulo Zanoni