From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: improve GEN7_ERR_INT clearing for fifo underrun reporting Date: Wed, 10 Jul 2013 22:22:07 +0200 Message-ID: <20130710202207.GK6143@phenom.ffwll.local> References: <1372973734-7601-4-git-send-email-daniel.vetter@ffwll.ch> <1373403556-1677-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f52.google.com (mail-ee0-f52.google.com [74.125.83.52]) by gabe.freedesktop.org (Postfix) with ESMTP id 47031E5C5B for ; Wed, 10 Jul 2013 13:22:05 -0700 (PDT) Received: by mail-ee0-f52.google.com with SMTP id c50so5163947eek.11 for ; Wed, 10 Jul 2013 13:22:04 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Wed, Jul 10, 2013 at 04:47:08PM -0300, Paulo Zanoni wrote: > 2013/7/9 Daniel Vetter : > > 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 > > Signed-off-by: Daniel Vetter > > --- > > 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 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