[Intel-gfx] [PATCH] drm/i915: Only force GGTT coherency w/a on required chipsets

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-07-20 16:13:14)
> 
> On 20/07/2018 11:19, Chris Wilson wrote:
> > Not all chipsets have an internal buffer delaying the visibility of
> > writes via the GGTT being visible by other physical paths, but we use a
> > very heavy workaround for all. We only need to apply that workarounds to
> > the chipsets we know suffer from the delay and the resulting coherency
> > issue.
> > 
> > Similarly, the same inconsistent coherency fouls up our ABI promise that
> > a write into a mmap_gtt is immediately visible to others. Since the HW
> > has made that a lie, let userspace know when that contract is broken.
> > (Not that userspace would want to use mmap_gtt on those chipsets for
> > other performance reasons...)
> > 
> > Testcase: igt/drv_selftest/live_coherency
> > Testcase: igt/gem_mmap_gtt/coherency
> 
> The list of platforms to mark with false was compiled from the test results?

Yes. Coverage of older chipsets is less than ideal, as we have fewer of
them being tested and the gmch wasn't tied to any processor. So whereas
my PIIIm + i915gm passes, CI's P4 + i915g fails. That is odd.
 
> But then before this patch workaround was applied everywhere - so if the 
> test was failing even then - that means workaround wasn't sufficient?

The test is being run in userspace; bypassing any domain control in the
kernel, assuming the coherency model built into the ABI.

> > +/*
> > + * Once upon a time we supposed that writes through the GGTT would be
> > + * immediately in physical memory (once flushed out of the CPU path). However,
> > + * on a few different processors and chipsets, this is not necessarily the case
> > + * as the writes appear to be buffered internally. Thus a read of the backing
> > + * storage (physical memory) via a different path (with different physical tags
> > + * to the indirect write via the GGTT) will see stale values from before
> > + * the GGTT write. Inside the kernel, we can for the most part keep track of
> > + * the different read/write domains in use (e.g. set-domain), but the assumption
> > + * of coherency is baked into the ABI, hence reporting its true state in this
> > + * parameter.
> > + *
> > + * If set to true, writes via mmap_gtt are immediately visible following an
> > + * lfence to flush the WCB.
> > + *
> > + * If set to false, writes via mmap_gtt are indeterminatnly delayed in an in
> > + * internal buffer and are _not_ immediately visible to third parties accessing
> > + * directly via mmap_cpu/mmap_wc. Use of mmap_gtt as part of an IPC
> > + * communications channel when set to false is strongly disadvised.
> 
> I would perhaps change the language from "set to true/false" to 
> "reported as true/false".
> 
> > + */
> > +#define I915_PARAM_MMAP_GTT_COHERENT 52
> > +
> >   typedef struct drm_i915_getparam {
> >       __s32 param;
> >       /*
> > 
> 
> Looks fine to me in principle. Is there some userspace ready to start 
> consuming it?

My only plan is for igt to know when the tests are expected to fail.
Real userspace should not be using GTT, it is slow (even slower than
intended ;) and constrained, so subject to aperture thrashing, fencing
is only usable for two out of the many tiling modes, etc, etc, etc.
-Chris