[Intel-gfx] [PATCH 1/2] drm/i915: properly reenable gen8 pipe IRQs

[Paulo Zanoni]

2014-10-07 16:58 GMT-03:00 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> On Tue, Oct 07, 2014 at 04:11:10PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> We were missing the pipe B/C vblank bits! Take a look at
>> gen8_de_irq_postinstall for a comparison.
>>
>> This should fix a bunch of IGT tests.
>>
>> There are a few more things we could improve on this code, but this
>> should be the minimal fix to unblock us.
>>
>> Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=83640
>> Testcase: igt/*
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index b12c4c4..3bbdb9c 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3166,11 +3166,13 @@ static void gen8_irq_reset(struct drm_device *dev)
>>
>>  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv)
>>  {
>> +     uint32_t extra_iir = GEN8_PIPE_VBLANK | GEN8_PIPE_FIFO_UNDERRUN;
>
> extra_iir seems a bit weird as a name when this is about IER. Maybe

In my head, this was supposed to be named extra_ier... Why would I
name it extra_iir?

>
> uint32_t de_pipe_enables = ~dev_priv->de_irq_mask | ...;

The problem is that since we have de_irq_mask for both pipes B and C,
I'd have to set de_pipe_enables twice. Even if they're supposed to be
the same thing today, I prefer to not depend on this fact.

I'll send a new version with another variable name.

>
> to better match gen8_de_irq_postinstall()?
>
>
> In any case now those reported vblank timeout failures at least
> make some sense, so even w/o that change:
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> But IIRC we had some vblank timeout reports on other platforms too
> (PNV maybe?). Wouldn't it be cute if this patch cured those too :)

For the eventual readers unfamiliar with the hardware: he knows this
patch won't fix PNV.

>
> And I'll end with the rant of the day:
> Boy do I hate that stupid ~ we have to sprinkle around everywhere. Almost
> makes me wish someone would go and hide that in some special IMR register
> access macro so we wouldn't have to invert bits all the time.

Yeah, it is confusing...

>
>> +
>>       spin_lock_irq(&dev_priv->irq_lock);
>>       GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_B, dev_priv->de_irq_mask[PIPE_B],
>> -                       ~dev_priv->de_irq_mask[PIPE_B]);
>> +                       ~dev_priv->de_irq_mask[PIPE_B] | extra_iir);
>>       GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_C, dev_priv->de_irq_mask[PIPE_C],
>> -                       ~dev_priv->de_irq_mask[PIPE_C]);
>> +                       ~dev_priv->de_irq_mask[PIPE_C] | extra_iir);
>>       spin_unlock_irq(&dev_priv->irq_lock);
>>  }
>>
>> --
>> 2.1.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni