[Intel-gfx] [PATCH v2] drm/i915/guc: WA to address the Ringbuffer coherency issue

[Tvrtko Ursulin]

On 26/10/2016 09:14, Chris Wilson wrote:
> On Wed, Oct 26, 2016 at 08:20:35AM +0100, Tvrtko Ursulin wrote:
>>
>> On 25/10/2016 21:03, Chris Wilson wrote:
>>> On Tue, Oct 25, 2016 at 10:05:23PM +0530, akash.goel at intel.com wrote:
>>>> From: Akash Goel <akash.goel at intel.com>
>>>>
>>>> Driver accesses the ringbuffer pages, via GMADR BAR, if the pages are
>>>> pinned in mappable aperture portion of GGTT and for ringbuffer pages
>>>> allocated from Stolen memory, access can only be done through GMADR BAR.
>>>> In case of GuC based submission, updates done in ringbuffer via GMADR
>>>> may not get committed to memory by the time the Command streamer starts
>>>> reading them, resulting in fetching of stale data.
>>>>
>>>> For Host based submission, such problem is not there as the write to Ring
>>>> Tail or ELSP register happens from the Host side prior to submission.
>>>> Access to any GFX register from CPU side goes to GTTMMADR BAR and Hw already
>>>> enforces the ordering between outstanding GMADR writes & new GTTMADR access.
>>>> MMIO writes from GuC side do not go to GTTMMADR BAR as GuC communication to
>>>> registers within GT is contained within GT, so ordering is not enforced
>>>> resulting in a race, which can manifest in form of a hang.
>>>>
>>>> To ensure the flush of in-flight GMADR writes, a POSTING READ is done to
>>>> GuC register prior to doorbell ring.
>>>> There is already a similar WA in i915_gem_object_flush_gtt_write_domain(),
>>>> which takes care of GMADR writes from User space to GEM buffers, but not the
>>>> ringbuffer writes from KMD.
>>>> This WA is needed on all recent HW.
>>>>
>>>> v2:
>>>> - Use POSTING_READ_FW instead of POSTING_READ as GuC register do not lie
>>>>  in any forcewake domain range and so the overhead of spinlock & search
>>>>  in the forcewake table is avoidable. (Chris)
>>>>
>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>>> Signed-off-by: Akash Goel <akash.goel at intel.com>
>>>
>>> Thanks for respinning, reviewed and pushed.
>>
>> No CI run with GuC enabled, or even any CI run?
>
> There were on previous versions. But yes, I am that confident that

No with the added code exercised afaict.

> this is not broken and matches the failure we have seen in BAT for other
> GTT vs physical access (on different machines on different paths).
>
> The only qualm I had was the use of is_mappable() to decide if we are
> writing through the GTT. ring->vma->obj->stolen might be better, or a
> ring->ggtt_writes flag. I wanted to fix the bug first, repaint later.

It looks fine to me as well, and I know Akash tested it extensively.

However process and all that.

Regards,

Tvrtko