Re: [RFC 2/3] drm/i915: do not disable handler after underrun

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jan 26, 2016 at 11:36:56AM -0800, Joe Konno wrote:
> From: Joe Konno <joe.konno@intel.com>
> 
> Previously, cpu and pch underrun reporting was done on a "report once
> and disable" basis to prevent interrupt/dmesg floods.
> 
> With this change, do not disable the underrun interrupt handlers.
> Instead, use throttled (DRM_ERROR_RATELIMITED) dmesg output if dmesg
> underrun reporting enabled.
> 
> Signed-off-by: Joe Konno <joe.konno@intel.com>

This is too simple unfortunately. In many cases the underrun is a one-off
right when scan-out starts and then stops again, until something changes
again. This is the typical failure mode for non-atomic wm update and
similar stuff.

But it's also possible that the pipe wedges entirely and the underrun is
stuck. Then you'll end up doing nothing else than serving fifo underruns,
with no cpu time left for anything else. This kills your box.

So if you want to keep track of a count of underruns the right way is to
disable it when it happens, but then schedule re-enabling for the next
vblank. Also I think we shouldn't spam dmesg more than we do already, so
instead of ratelimiting I'd just keep a counter after the first one
happened, before the next modeset.

Another really annoying thing with underruns: On some platforms the irq
control bits are shared between them all, which means you can't disable
them individually. If you underrun on one pipe you also have to disable it
for all the others.

In short: This is all a pretty big mess :(
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_fifo_underrun.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index 9516bd416226..4a1a2781fc5e 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -393,26 +393,23 @@ bool intel_get_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
>   * @pipe: (CPU) pipe to set state for
>   *
>   * This handles a CPU fifo underrun interrupt, generating an underrun warning
> - * into dmesg if underrun reporting is enabled and then disables the underrun
> - * interrupt to avoid an irq storm.
> + * into dmesg if underrun reporting is enabled.
>   */
>  void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  					 enum pipe pipe)
>  {
>  	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +	bool report;
>  
>  	/* We may be called too early in init, thanks BIOS! */
>  	if (crtc == NULL)
>  		return;
>  
> -	/* GMCH can't disable fifo underruns, filter them. */
> -	if (HAS_GMCH_DISPLAY(dev_priv->dev) &&
> -	    to_intel_crtc(crtc)->cpu_fifo_underrun_disabled)
> -		return;
> +	report = intel_get_cpu_fifo_underrun_reporting(dev_priv, pipe);
>  
> -	if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
> -		DRM_ERROR("CPU pipe %c FIFO underrun\n",
> -			  pipe_name(pipe));
> +	if (report)
> +		DRM_ERROR_RATELIMITED("CPU pipe %c FIFO underrun\n",
> +				      pipe_name(pipe));
>  }
>  
>  /**
> @@ -421,16 +418,18 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>   * @pch_transcoder: the PCH transcoder (same as pipe on IVB and older)
>   *
>   * This handles a PCH fifo underrun interrupt, generating an underrun warning
> - * into dmesg if underrun reporting is enabled and then disables the underrun
> - * interrupt to avoid an irq storm.
> + * into dmesg if underrun reporting is enabled.
>   */
>  void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  					 enum transcoder pch_transcoder)
>  {
> -	if (intel_set_pch_fifo_underrun_reporting(dev_priv, pch_transcoder,
> -						  false))
> -		DRM_ERROR("PCH transcoder %c FIFO underrun\n",
> -			  transcoder_name(pch_transcoder));
> +	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
> +	bool report = intel_get_pch_fifo_underrun_reporting(dev_priv,
> +							    pch_transcoder);
> +
> +	if (report)
> +		DRM_ERROR_RATELIMITED("PCH transcoder %c FIFO underrun\n",
> +				      transcoder_name(pch_transcoder));
>  }
>  
>  /**
> -- 
> 2.6.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx