2014-10-07 16:58 GMT-03:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>: > On Tue, Oct 07, 2014 at 04:11:10PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> >> 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@xxxxxxxxxxxxxxx >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC -- Paulo Zanoni -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html