[Intel-gfx] [PATCH 2/8] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Paulo Zanoni]

2013/7/30 Chris Wilson <chris at chris-wilson.co.uk>:
> On Mon, Jul 29, 2013 at 05:48:21PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> If the error interrupts are already disabled, don't disable and
>> reenable them. This is going to be needed when we're in PC8+, where
>> all the interrupts are disabled so we won't risk re-enabling
>> DE_ERR_INT_IVB.
>
>>       if (IS_HASWELL(dev)) {
>>               spin_lock(&dev_priv->irq_lock);
>> -             ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> +             if (!(I915_READ(DEIMR) & DE_ERR_INT_IVB)) {
>> +                     ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> +                     err_int_reenable = true;
>> +             }
>
> Or just make ironlake_disable_display_irq() return a bool.

Will do.

> So this then
> raises the question: please justify why the deimr state tracker is out
> of sync with the register.

It shouldn't be.

> If it is possible that we write to this
> register whilst under pc8 and then restore a different value, that is
> scary. I would rather have a big BUG_ON(!pc8) inside
> ironlake_disable_display_irq() and move the pc8 handling logic there
> (i.e. if we get a request for something whilst under pc8, modify the
> saved bits rather than the actual register).

Your concerns are valid and some of the WARNs I have are exactly to
catch these things, but we catch them *after* we disallow/disable PC8.
On my new series I'll port GTIMR, GEN6_PMIMR and SDEIMR to the same
model as ironlake_enable/disable_display_irq and then add some more
WARNs in case we're calling them with PC8 enabled. Maybe the safest
thing to do would actually disable PC8 in case that happens (and also
print an error message so we can fix these cases).

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



-- 
Paulo Zanoni