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

[Tvrtko Ursulin]

On 20/07/2018 16:21, Chris Wilson wrote:
> 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.

Hope the oddity is not hiding something but we'll see.

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

Ah yes.

>>> +/*
>>> + * 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.

I'll take the view that get_param is tiny enough for this to be fine.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko