[Intel-gfx] [PATCH] drm/i915/fbc: disable FBC on FIFO underruns

[Pandiyan, Dhinakaran]

On Mon, 2016-08-15 at 19:36 -0300, Paulo Zanoni wrote:
> Ever since I started working on FBC I was already aware that FBC can
> really amplify the FIFO underrun symptoms. On systems where FIFO
> underruns were harmless error messages, enabling FBC would cause the
> underruns to give black screens.
> 
> We recently tried to enable FBC on Haswell and got reports of a system
> that would hang after some hours of uptime, and the first bad commit
> was the one that enabled FBC. We also observed that this system had
> FIFO underrun error messages on its dmesg. Although we don't have any
> evidence that fixing the underruns would solve the bug and make FBC
> work properly on this machine, IMHO it's better if we minimize the
> amount of possible problems by just giving up FBC whenever we detect
> an underrun.
> 
> v2: New version, different implementation and commit message.
> v3: Clarify the fact that we run from an IRQ handler (Chris).
> 
> Cc: Stefan Richter <stefanr at s5r6.in-berlin.de>
> Cc: Lyude <cpaul at redhat.com>
> Cc: stevenhoneyman at gmail.com <stevenhoneyman at gmail.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  3 ++
>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c           | 61 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_fifo_underrun.c |  2 +
>  4 files changed, 67 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 35caa9b..baa9c66 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -941,6 +941,9 @@ struct intel_fbc {
>  	bool enabled;
>  	bool active;
>  
> +	bool underrun_detected;
> +	struct work_struct underrun_work;
> +
>  	struct intel_fbc_state_cache {
>  		struct {
>  			unsigned int mode_flags;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1c700b0..50c1eb0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1494,6 +1494,7 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>  void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
>  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv);
>  
>  /* intel_hdmi.c */
>  void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index e122052..950aed5 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -750,6 +750,13 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  	struct intel_fbc *fbc = &dev_priv->fbc;
>  	struct intel_fbc_state_cache *cache = &fbc->state_cache;
>  
> +	/* We don't need to use a state cache here since this information is
> +	 * global for every CRTC. */
> +	if (fbc->underrun_detected) {
> +		fbc->no_fbc_reason = "underrun detected";
> +		return false;
> +	}
> +
>  	if (!cache->plane.visible) {
>  		fbc->no_fbc_reason = "primary plane not visible";
>  		return false;
> @@ -1193,6 +1200,59 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
>  	cancel_work_sync(&fbc->work.work);
>  }
>  
> +static void intel_fbc_underrun_work_fn(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private, fbc.underrun_work);
> +	struct intel_fbc *fbc = &dev_priv->fbc;
> +
> +	mutex_lock(&fbc->lock);
> +
> +	/* Maybe we were scheduled twice. */
> +	if (fbc->underrun_detected)
> +		goto out;

If the underrun happens very early after boot (likely during modeset),
the user might end up seeing just FBC enable and disable messages in
dmesg without knowing it (or why it) was deactivated.

For e.g.,

[ 1855.552489] [drm:intel_fbc_enable] Enabling FBC on pipe A
[ 1856.854239] [drm:__intel_fbc_disable] Disabling FBC on pipe A
[ 1857.753383] [drm:intel_fbc_alloc_cfb] Reducing the compressed
framebuffer size. This may lead to less power savings than a
non-reduced-size. Try to increase stolen memory size if available in
BIOS.
[ 1857.753384] [drm:intel_fbc_alloc_cfb] reserved 10368000 bytes of
contiguous stolen space for FBC, threshold: 4
[ 1857.753384] [drm:intel_fbc_enable] Enabling FBC on pipe A

I find this noise misleading because FBC is not going to be activated at
all.


> +
> +	DRM_DEBUG_KMS("Disabling FBC due to FIFO underrun.\n");

Print out the pipe here? Both the enabling and disabling debug messages
clarify the pipe. It may be useful to provide this information here too.

> +	fbc->underrun_detected = true;
> +
> +	intel_fbc_deactivate(dev_priv);
> +out:
> +	mutex_unlock(&fbc->lock);
> +}
> +
> +/**
> + * intel_fbc_handle_fifo_underrun_irq - disable FBC when we get a FIFO underrun
> + * @dev_priv: i915 device instance
> + *
> + * Without FBC, most underruns are harmless and don't really cause too many
> + * problems, except for an annoying message on dmesg. With FBC, underruns can
> + * become black screens or even worse, especially when paired with bad
> + * watermarks. So in order for us to be on the safe side, completely disable FBC
> + * in case we ever detect a FIFO underrun on any pipe. An underrun on any pipe
> + * already suggests that watermarks may be bad, so try to be as safe as
> + * possible.
> + *
> + * This function is called from the IRQ handler.
> + */
> +void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_fbc *fbc = &dev_priv->fbc;
> +
> +	if (!fbc_supported(dev_priv))
> +		return;
> +
> +	/* There's no guarantee that underrun_detected won't be set to true
> +	 * right after this check and before the work is scheduled, but that's
> +	 * not a problem since we'll check it again under the work function
> +	 * while FBC is locked. This check here is just to prevent us from
> +	 * unnecessarily scheduling the work, and it relies on the fact that we
> +	 * never switch underrun_detect back to false after it's true. */
> +	if (fbc->underrun_detected)
> +		return;
> +
> +	schedule_work(&fbc->underrun_work);
> +}
> +
>  /**
>   * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
>   * @dev_priv: i915 device instance
> @@ -1264,6 +1324,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  	enum pipe pipe;
>  
>  	INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
> +	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
>  	mutex_init(&fbc->lock);
>  	fbc->enabled = false;
>  	fbc->active = false;
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index 2aa7440..ebb4fed 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -372,6 +372,8 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
>  	if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false))
>  		DRM_ERROR("CPU pipe %c FIFO underrun\n",
>  			  pipe_name(pipe));
> +
> +	intel_fbc_handle_fifo_underrun_irq(dev_priv);
>  }
>  
>  /**