On Thu, May 22, 2014 at 07:55:52PM +0300, Ville Syrjälä wrote:
> 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.
We definitely have disabled->disabled transtions, which is what we need to
filter out. Since it means we have either untrusted watermarks or hit and
underrun.
For enabled->enabled I think that can happen in crtc_enable - we
unconditionally enable underrun reporting againg to clear out old fail (or
firmware setups). But we don't always disable it when disabling the crtc
since some platforms/ports don't underrun when disabled.
This also only matters when we've had an underrun with a shared error
interrupt somewhere. Which is the case where the old code is broken
anyway, so I'm not sure how much we should care really.
Also the current code only checks for missed fifo underruns on disabling,
so even if there is a notch of a leak here it's definitely not something
this patch will regress.
-Daniel
>
> >
> > 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 <daniel.vetter@xxxxxxxx>
> > ---
> > 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 = dev->dev_private;
> > u32 reg = PIPESTAT(pipe);
> > @@ -347,7 +348,7 @@ static void i9xx_set_fifo_underrun_reporting(struct drm_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(struct 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 = dev->dev_private;
> > if (enable) {
> > @@ -379,7 +381,8 @@ static void ivybridge_set_fifo_underrun_reporting(struct 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 drm_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 = dev->dev_private;
> >
> > @@ -459,7 +462,8 @@ static void cpt_set_fifo_underrun_reporting(struct drm_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 = dev->dev_private;
> > struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > - bool ret;
> > + bool old;
> >
> > assert_spin_locked(&dev_priv->irq_lock);
> >
> > - ret = !intel_crtc->cpu_fifo_underrun_disabled;
> > -
> > - if (enable == ret)
> > - goto done;
> > -
> > + old = !intel_crtc->cpu_fifo_underrun_disabled;
> > intel_crtc->cpu_fifo_underrun_disabled = !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 = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
> > struct intel_crtc *intel_crtc = 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 drm_device *dev,
> >
> > spin_lock_irqsave(&dev_priv->irq_lock, flags);
> >
> > - ret = !intel_crtc->pch_fifo_underrun_disabled;
> > -
> > - if (enable == ret)
> > - goto done;
> > -
> > + old = !intel_crtc->pch_fifo_underrun_disabled;
> > intel_crtc->pch_fifo_underrun_disabled = !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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
