2014-03-18 17:29 GMT-03:00 Ben Widawsky <ben@xxxxxxxxxxxx>:
> On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
