2013/12/10 Daniel Vetter <daniel@xxxxxxxx>:
> On Thu, Nov 21, 2013 at 01:47:25PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
