From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 2/5] drm/i915: Fix up fifo underrun tracking, take N Date: Thu, 22 May 2014 19:55:52 +0300 Message-ID: <20140522165552.GX27580@intel.com> References: <1400774195-19414-1-git-send-email-daniel.vetter@ffwll.ch> <1400774195-19414-2-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 594206E464 for ; Thu, 22 May 2014 09:55:56 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1400774195-19414-2-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, May 22, 2014 at 05:56:32PM +0200, Daniel Vetter wrote: > So apparently this is tricky. > = > We need to consider: > - We start out with all the hw enabling bits disabled, both the > individual fifo underrun interrupts and the shared display error > interrupts masked. Otherwise if the bios config is broken we'll blow > up with a NULL deref in our interrupt handler since the crtc > structures aren't set up yet at driver load time. > - On gmch we need to mask fifo underruns on the sw side, so always > need to set that in sanitize_crtc for those platforms. > - On other platforms we try to set the sw tracking so that it reflects > the real state. But since a few platforms have shared bits we must > _not_ disable fifo underrun reporting. Otherwise we'll never enable > the shared error interrupt. > = > This is the state before out patch, but unfortunately this is not good > enough. But after a suspend resume operation this is broken: > 1. We don't enable the hw interrupts since the same code runs on > resume as on driver load. > 2. The fifo underrun state adjustments we do in sanitize_crtc doesn't > fire on resume since (except for hilarious firmware) all pipes are off > at that point. But they also don't hurt since the subsequent crtc > enabling due to force_restore will enable fifo underruns. > = > Which means when we enable fifo underrun reporting we notice that the > per-crtc state is already correct and short-circuit everthing out. And > the interrupt doesn't get enabled. > = > A similar problem would happen if the bios doesn't light up anything > when the driver loads. Which is exactly what happens when we reload > the driver since our unload functions disables all outputs. > = > Now we can't just rip out the short-circuit logic and unconditionally > update the fifo underrun reporting interrupt masking: We have some > checks for shared error interrupts to catch issues that happened when > the shared error interrupt was disabled. Hmm. Do we have cases where we do enabled->enabled "transition"? Because in that case we would now clear the register without reporting if there was an underrun in the register. > = > The right fix is to push down this logic so that we can always update > the hardware state, but only check for missed fifo underruns on a real > enabled->disabled transition and ignore them when we're already > disabled. > = > On platforms with shared error interrupt the pipe CRC interrupts are > grouped together with the fifo underrun reporting this fixes pipe CRC > support after suspend and driver reloads. > = > Testcase: igt/kms_pipe_crc_basic/suspend-* > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_irq.c | 44 ++++++++++++++++++-----------------= ------ > 1 file changed, 19 insertions(+), 25 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_= irq.c > index 304f86a3162c..4d44f09eb833 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -335,7 +335,8 @@ void i9xx_check_fifo_underruns(struct drm_device *dev) > } > = > static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev, > - enum pipe pipe, bool enable) > + enum pipe pipe, > + bool enable, bool old) > { > struct drm_i915_private *dev_priv =3D dev->dev_private; > u32 reg =3D PIPESTAT(pipe); > @@ -347,7 +348,7 @@ static void i9xx_set_fifo_underrun_reporting(struct d= rm_device *dev, > I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS); > POSTING_READ(reg); > } else { > - if (pipestat & PIPE_FIFO_UNDERRUN_STATUS) > + if (old && pipestat & PIPE_FIFO_UNDERRUN_STATUS) > DRM_ERROR("pipe %c underrun\n", pipe_name(pipe)); > } > } > @@ -366,7 +367,8 @@ static void ironlake_set_fifo_underrun_reporting(stru= ct drm_device *dev, > } > = > static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev, > - enum pipe pipe, bool enable) > + enum pipe pipe, > + bool enable, bool old) > { > struct drm_i915_private *dev_priv =3D dev->dev_private; > if (enable) { > @@ -379,7 +381,8 @@ static void ivybridge_set_fifo_underrun_reporting(str= uct drm_device *dev, > } else { > ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB); > = > - if (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe)) { > + if (old && > + I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe)) { > DRM_ERROR("uncleared fifo underrun on pipe %c\n", > pipe_name(pipe)); > } > @@ -444,7 +447,7 @@ static void ibx_set_fifo_underrun_reporting(struct dr= m_device *dev, > = > static void cpt_set_fifo_underrun_reporting(struct drm_device *dev, > enum transcoder pch_transcoder, > - bool enable) > + bool enable, bool old) > { > struct drm_i915_private *dev_priv =3D dev->dev_private; > = > @@ -459,7 +462,8 @@ static void cpt_set_fifo_underrun_reporting(struct dr= m_device *dev, > } else { > ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT); > = > - if (I915_READ(SERR_INT) & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)= ) { > + if (old && I915_READ(SERR_INT) & > + SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) { > DRM_ERROR("uncleared pch fifo underrun on pch transcoder %c\n", > transcoder_name(pch_transcoder)); > } > @@ -486,28 +490,23 @@ static bool __intel_set_cpu_fifo_underrun_reporting= (struct drm_device *dev, > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct drm_crtc *crtc =3D dev_priv->pipe_to_crtc_mapping[pipe]; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > - bool ret; > + bool old; > = > assert_spin_locked(&dev_priv->irq_lock); > = > - ret =3D !intel_crtc->cpu_fifo_underrun_disabled; > - > - if (enable =3D=3D ret) > - goto done; > - > + old =3D !intel_crtc->cpu_fifo_underrun_disabled; > intel_crtc->cpu_fifo_underrun_disabled =3D !enable; > = > if (INTEL_INFO(dev)->gen < 5 || IS_VALLEYVIEW(dev)) > - i9xx_set_fifo_underrun_reporting(dev, pipe, enable); > + i9xx_set_fifo_underrun_reporting(dev, pipe, enable, old); > else 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, pipe, enable); > + ivybridge_set_fifo_underrun_reporting(dev, pipe, enable, old); > else if (IS_GEN8(dev)) > broadwell_set_fifo_underrun_reporting(dev, pipe, enable); > = > -done: > - return ret; > + return old; > } > = > bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, > @@ -556,7 +555,7 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm= _device *dev, > struct drm_crtc *crtc =3D dev_priv->pipe_to_crtc_mapping[pch_transcoder= ]; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > unsigned long flags; > - bool ret; > + bool old; > = > /* > * NOTE: Pre-LPT has a fixed cpu pipe -> pch transcoder mapping, but LPT > @@ -569,21 +568,16 @@ bool intel_set_pch_fifo_underrun_reporting(struct d= rm_device *dev, > = > spin_lock_irqsave(&dev_priv->irq_lock, flags); > = > - ret =3D !intel_crtc->pch_fifo_underrun_disabled; > - > - if (enable =3D=3D ret) > - goto done; > - > + old =3D !intel_crtc->pch_fifo_underrun_disabled; > intel_crtc->pch_fifo_underrun_disabled =3D !enable; > = > if (HAS_PCH_IBX(dev)) > ibx_set_fifo_underrun_reporting(dev, pch_transcoder, enable); > else > - cpt_set_fifo_underrun_reporting(dev, pch_transcoder, enable); > + cpt_set_fifo_underrun_reporting(dev, pch_transcoder, enable, old); > = > -done: > spin_unlock_irqrestore(&dev_priv->irq_lock, flags); > - return ret; > + return old; > } > = > = > -- = > 1.8.4.rc3 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC