Re: [PATCH] drm/i915: improve GEN7_ERR_INT clearing for fifo underrun reporting

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 10, 2013 at 04:47:08PM -0300, Paulo Zanoni wrote:
> 2013/7/9 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Same treatment as for SERR_INT: If we clear only the bit for the pipe
> > we're enabling (but unconditionally) then we can always check for
> > possible underruns after having disabled the interrupt. That way pipe
> > underruns won't be lost, but at worst only get reported in a delayed
> > fashion.
> >
> > v2: The same logic bug as in the SERR handling change also existed
> > here. The same bugfix of only reporting missed underruns when the
> > error interrupt was masked applies, too.
> >
> > v3: Do the same fixes as for the SERR handling that Paulo suggested in
> > his review:
> > - s/%i/%c/ fix in the debug output
> > - move the DE_ERR_INT_IVB read into the respective if block
> >
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++++++-------
> >  drivers/gpu/drm/i915/i915_reg.h |  1 +
> >  2 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index dd9d999..76e977b 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -154,21 +154,27 @@ static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
> >  }
> >
> >  static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
> > -                                                 bool enable)
> > +                                                 enum pipe pipe, bool enable)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> >         if (enable) {
> > +               I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN(pipe));
> > +
> >                 if (!ivb_can_enable_err_int(dev))
> >                         return;
> >
> > -               I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN_A |
> > -                                        ERR_INT_FIFO_UNDERRUN_B |
> > -                                        ERR_INT_FIFO_UNDERRUN_C);
> > -
> >                 ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
> >         } else {
> > +               bool was_enabled = !(I915_READ(DEIMR) & DE_ERR_INT_IVB);
> > +
> > +               /* Change the state _after_ we've read out the current one. */
> >                 ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> > +
> > +               if (!was_enabled &&
> > +                   (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
> > +                       DRM_DEBUG_KMS("uncleared fifo underrun on pipe %c\n",
> > +                                     pipe_name(pipe));
> > +               }
> >         }
> >  }
> >
> > @@ -274,7 +280,7 @@ bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> >         if (IS_GEN5(dev) || IS_GEN6(dev))
> >                 ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
> >         else if (IS_GEN7(dev))
> > -               ivybridge_set_fifo_underrun_reporting(dev, enable);
> > +               ivybridge_set_fifo_underrun_reporting(dev, pipe, enable);
> >
> >  done:
> >         spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 7e2684f..43e81c1 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -681,6 +681,7 @@
> >  #define   ERR_INT_FIFO_UNDERRUN_C      (1<<6)
> >  #define   ERR_INT_FIFO_UNDERRUN_B      (1<<3)
> >  #define   ERR_INT_FIFO_UNDERRUN_A      (1<<0)
> > +#define   ERR_INT_FIFO_UNDERRUN(pipe)  (1<< (pipe*3))
> 
> Checkpatch complains about inconsistent error spacing here.

Oops, I've missed to apply this change from your review on the previous
patch. Will bikeshed when I apply the first part of this series tomorrow.
> 
> With or without that fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks for your review,
Daniel

> 
> >
> >  #define FPGA_DBG               0x42300
> >  #define   FPGA_DBG_RM_NOCLAIM  (1<<31)
> > --
> > 1.8.1.4
> >
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch