| Message ID | 1412709071-1886-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 07, 2014 at 04:11:10PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@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@vger.kernel.org > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 uint32_t de_pipe_enables = ~dev_priv->de_irq_mask | ...; 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@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 :) 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. > + > 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2014-10-07 16:58 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > On Tue, Oct 07, 2014 at 04:11:10PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@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@vger.kernel.org >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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@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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
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; + 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); }