[Intel-gfx] [PATCH 14/20] drm/i915: enable SDEIER later

[Paulo Zanoni]

2014-03-18 17:29 GMT-03:00 Ben Widawsky <ben at bwidawsk.net>:
> On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> On the preinstall stage we should just disable all the interrupts, but
>> we currently enable all the south display interrupts due to the way we
>> touch SDEIER at the IRQ handlers (note: they are still masked and our
>> IRQ handler is disabled).
>
> I think this statement is false. The interrupt is enabled right after
> preinstall(). For the nomodeset case, this actually seems to make some
> difference. It still looks fine to me though.

Could you please clarify this paragraph?

We currently enable SDEIER at ibx_irq_preinstall, but the reason why
we do this is because we change SDEIER at the IRQ handler. So if we
move that code from preinstall to postinstall, but at a point where no
interrupts are enabled yet, we should be safe, and this is what the
patch does.

>
>> Instead of doing that, let's make the
>> preinstall stage just disable all the south interrupts, and do the
>> proper interrupt dance/ordering at the postinstall stage, including an
>> assert to check if everything is behaving as expected.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++------
>>  1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 95f535b..4479e29 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -2814,13 +2814,24 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>>
>>       if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
>>               I915_WRITE(SERR_INT, 0xffffffff);
>> +}
>>
>> -     /*
>> -      * SDEIER is also touched by the interrupt handler to work around missed
>> -      * PCH interrupts. Hence we can't update it after the interrupt handler
>> -      * is enabled - instead we unconditionally enable all PCH interrupt
>> -      * sources here, but then only unmask them as needed with SDEIMR.
>> -      */
>> +/*
>> + * SDEIER is also touched by the interrupt handler to work around missed PCH
>> + * interrupts. Hence we can't update it after the interrupt handler is enabled -
>> + * instead we unconditionally enable all PCH interrupt sources here, but then
>> + * only unmask them as needed with SDEIMR.
>> + *
>> + * This function needs to be called before interrupts are enabled.
>> + */
>> +static void ibx_irq_pre_postinstall(struct drm_device *dev)
>
> sde_irq_postinstall()?

I agree the current name is bad, but we use the IBX naming for
everything on the PCH at i915_irq.c, and ironlake_irq_postinstall()
already calls a function named ibx_irq_postinstall(), so I needed a
name that means "call this just before the postinstall() steps". If we
rename it to sde_irq_postinstall we may confuse users due to the fact
that we also have ibx_irq_postinstall. So with the current patch, we
have this:

void ironlake_irq_postinstall()
{
    /* We have to call this before enabling the interrupts */
    ibx_irq_pre_postinstall();
    /* Enable the interrupts */
    GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
    /* Now do the necessary postinstall adjustments to the PCH stuff */
    ibx_irq_postinstall();
}

But I'm still open to new naming suggestions.

>
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     if (HAS_PCH_NOP(dev))
>> +             return;
>> +
>> +     WARN_ON(I915_READ(SDEIER) != 0);
>>       I915_WRITE(SDEIER, 0xffffffff);
>>       POSTING_READ(SDEIER);
>>  }
>> @@ -3026,6 +3037,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>>
>>       dev_priv->irq_mask = ~display_mask;
>>
>> +     ibx_irq_pre_postinstall(dev);
>> +
>>       GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
>>
>>       gen5_gt_irq_postinstall(dev);
>> @@ -3217,6 +3230,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> +     ibx_irq_pre_postinstall(dev);
>> +
>>       gen8_gt_irq_postinstall(dev_priv);
>>       gen8_de_irq_postinstall(dev_priv);
>>
>> --
>> 1.8.5.3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Paulo Zanoni