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

[Paulo Zanoni]

2013/8/20 Daniel Vetter <daniel at ffwll.ch>:
> On Tue, Aug 20, 2013 at 11:43:46AM -0300, Paulo Zanoni wrote:
>> 2013/8/20 Daniel Vetter <daniel at ffwll.ch>:
>> > On Tue, Aug 06, 2013 at 06:57:16PM -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.
>> >>
>> >> v2: Use dev_priv->irq_mask (Chris)
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>> >>  1 file changed, 5 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> >> index d96bd1b..5e7e6f6 100644
>> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> >> @@ -1373,6 +1373,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>> >>       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>> >>       u32 de_iir, gt_iir, de_ier, sde_ier = 0;
>> >>       irqreturn_t ret = IRQ_NONE;
>> >> +     bool err_int_reenable = false;
>> >>
>> >>       atomic_inc(&dev_priv->irq_received);
>> >>
>> >> @@ -1401,7 +1402,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>> >>        * handler. */
>> >>       if (IS_HASWELL(dev)) {
>> >>               spin_lock(&dev_priv->irq_lock);
>> >> -             ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> >> +             err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB;
>> >> +             if (err_int_reenable)
>> >> +                     ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> >
>> > Hm, that reminds me that this entire logic here is racy wrt concurrent
>> > interrupt enabling on a different cpu core (e.g. due to a modeset now
>> > again allowing display error interrupts). Do we still need this or could
>> > we just ditch this entire complexity?
>>
>> Can you please explain more? We still check ivb_can_enable_err_int
>> before reenabling.
>
> Yeah, but in-between someone could sneak in and enable the display error
> interrupt (since modeset doesn't block it any more), but while the
> interrupt is still running. I.e.
>
> CPU 0                   CPU 1
>                         disable DERR due to modeset
>
> start interrupt handler,
> check that DERRR is off,
> do nothing
>
>                         reanable DERR due to modeset done
>
> -> interrupt handler still running, but DERR is enabled
>
> end interrupt handler

Oh, I see. The reason we disable ERR_INT at the irq handler is only
because if we ever hit "unclaimed register"s during interrupts we
won't keep getting new interrupts all over again. So your concerns are
valid, but this all is just a problem we find a way to trigger ERR_INT
interrupts from inside the irq handler.

>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni