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

[chris at chris-wilson.co.uk]

On Fri, Jun 17, 2016 at 07:02:57PM +0000, Zanoni, Paulo R wrote:
> 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.

It's amazing how often we need to write w/a for vtd. I wasn't feeling
imaginative and the uniformity may be useful?

> > +{
> > +#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:

Honestly, I thought you were going to suggest something more like:

#define mkwrite_intel_info(ptr) ((struct intel_device_info *)&(ptr)->info)

if (need_vta_wa(dev_priv))
	mkwrite_intel_info(dev_priv)->has_fbc = false;

We have a few other places that require such a beast.

> 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?".

Or, we use DRM_INFO() and explain the quirk, which is what I should have
done (since that is the mo for applying quirks).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre