| Message ID | 1373403556-1677-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. With or without that fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > #define FPGA_DBG 0x42300 > #define FPGA_DBG_RM_NOCLAIM (1<<31) > -- > 1.8.1.4 >
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
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)) #define FPGA_DBG 0x42300 #define FPGA_DBG_RM_NOCLAIM (1<<31)
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(-)