[Intel-gfx] [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)

[Paulo Zanoni]

2013/8/9 Chris Wilson <chris at chris-wilson.co.uk>:
> On Fri, Aug 09, 2013 at 05:10:05PM -0300, Paulo Zanoni wrote:
>> +/* Disable interrupts so we can allow Package C8+. */
>> +void hsw_pc8_disable_interrupts(struct drm_device *dev)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     unsigned long irqflags;
>> +
>> +     spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>> +
>> +     dev_priv->pc8.regsave.deimr = I915_READ(DEIMR);
>> +     dev_priv->pc8.regsave.sdeimr = I915_READ(SDEIMR);
>> +     dev_priv->pc8.regsave.gtimr = I915_READ(GTIMR);
>> +     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);
>> +     ilk_disable_gt_irq(dev_priv, 0xffffffff);
>> +     snb_disable_pm_irq(dev_priv, 0xffffffff);
>> +
>> +     dev_priv->pc8.irqs_disabled = true;
>> +
>> +     WARN(I915_READ(DEIIR), "DEIIR is not 0\n");
>> +     WARN(I915_READ(SDEIIR), "SDEIIR is not 0\n");
>> +     WARN(I915_READ(GTIIR), "GTIIR is not 0\n");
>> +     WARN(I915_READ(GEN6_PMIIR), "GEN6_PMIIR is not 0\n");
>
> I keep looking at this, because we will hit these warns. But I also
> don't think it a problem, as the interrupt handle will run as soon as we
> release the irq_lock and that will be the final time until we reenable
> interrupts. (Just kill the WARNs.)

I also thought we were going to hit these WARNs, but I don't ever hit
any of them in my current tree, even if I change the default PC8
timeout from 5s to 0.1s (which makes us enter/leave PC8+ very often),
so they never bothered me enough to be killed. Maybe the race
condition is too difficult to hit... But I can remove the WARNs.

>
> Now, why IMR and not IER?

Because on our driver, when we want to enable/disable interrupts while
everything is running we only use IMR, so our code is designed in a
way that if you grab irq_lock and update IMR with the appropriate
function you should be safe. On the other hand, some IER registers are
updated inside the irq handlers (like DEIER and SDEIER at
ironlake_irq_handler), so touching these is not as trivial. The only
advantage of using IER would be to preserve the IIR bits, but
according to Arthur the HW does not save/restore the IIR registers
when entering/leaving PC8, so no advantage.


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni