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@xxxxxxxxxxxxxxxxxx>
> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
