[Intel-gfx] [PATCH] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Zanoni, Paulo R]

Em Sex, 2016-06-17 às 17:39 +0100, Chris Wilson escreveu:
> Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are
> Enabled
> 
> "Display flickering may occur when both FBC (Frame Buffer
> Compression)
> and VT - d (Intel® Virtualization Technology for Directed I/O) are
> enabled
> and in use by the display controller."

Ouch...

> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index fddba1eed5ad..e47785467220 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1230,6 +1230,18 @@ void intel_fbc_init_pipe_state(struct
> drm_i915_private *dev_priv)
>  			dev_priv->fbc.visible_pipes_mask |= (1 <<
> crtc->pipe);
>  }
>  
> +static bool need_vtd_wa(struct drm_i915_private *dev_priv)

Notice we have a function with the exact same name in intel_display.c.

> +{
> +#ifdef CONFIG_INTEL_IOMMU
> +	if (!intel_iommu_gfx_mapped)
> +		return false;
> +
> +	if (INTEL_GEN(dev_priv) == 9)
> +		return true;
> +#endif
> +	return false;
> +}
> +
>  /**
>   * intel_fbc_init - Initialize FBC
>   * @dev_priv: the i915 device
> @@ -1247,6 +1259,12 @@ void intel_fbc_init(struct drm_i915_private
> *dev_priv)
>  	fbc->active = false;
>  	fbc->work.scheduled = false;
>  
> +	if (need_vtd_wa(dev_priv)) {
> +		struct intel_device_info *info =
> +			(struct intel_device_info *)&dev_priv->info;
> +		info->has_fbc = false;
> +	}

I just find this piece above a little not-so-beautiful and possibly
confusing for people debugging failures and/or IGT. Alternatives:

1 - Move the check to intel_fbc_can_choose(), so we can give a nice
no_fbc_reason. Problem: this would not be as efficient as what you
wrote.

2 - Move the check to intel_sanitize_fbc_option(), which is reviewed
but not merged yet. Problem: same as 1.

3 - Patch fbc_supported() and make the line below call fbc_supported()
instead of HAS_FBC(). Problem: we call it many times.

4 - Create dev_priv->fbc.is_supported (or some other meaningful name),
set it at init time, and make fbc_supported() use it (or just replace
fbc_supported() calls with the variable check).

All solutions above would probably require some adjustments to debugfs
and/or IGT (which relies on debugfs), but at least they wouldn't
surprise users or IGT runners with "why does it say SKL is not
supported by FBC?".

> +
>  	if (!HAS_FBC(dev_priv)) {
>  		fbc->no_fbc_reason = "unsupported by this chipset";
>  		return;