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 <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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
