Re: [PATCH 2/5] drm/i915: Fix up fifo underrun tracking, take N

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@ffwll.ch>
> ---
>  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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC