[Intel-gfx] [PATCH v2 2/2] drm/i915/bxt: work around HW context corruption due to coherency problem

[Chris Wilson]

On Wed, Sep 23, 2015 at 03:58:37PM +0200, Daniel Vetter wrote:
> On Wed, Sep 23, 2015 at 02:39:13PM +0100, Chris Wilson wrote:
> > On Wed, Sep 23, 2015 at 03:35:59PM +0200, Daniel Vetter wrote:
> > > On Fri, Sep 18, 2015 at 10:02:24AM +0100, Chris Wilson wrote:
> > > > On Thu, Sep 17, 2015 at 07:17:44PM +0300, Imre Deak wrote:
> > > > > The execlist context object is mapped with a CPU/GPU coherent mapping
> > > > > everywhere, but on BXT A stepping due to a HW issue the coherency is not
> > > > > guaranteed. To work around this flush the context object after pinning
> > > > > it (to flush cache lines left by the context initialization/read-back
> > > > > from backing storage) and mark it as uncached so later updates during
> > > > > context switching will be coherent.
> > > > > 
> > > > > I noticed this problem via a GPU hang, where IPEHR pointed to an invalid
> > > > > opcode value. I couldn't find this value on the ring but looking at the
> > > > > contents of the active context object it turned out to be a parameter
> > > > > dword of a bigger command there. The original command opcode itself
> > > > > was zeroed out, based on the above I assume due to a CPU writeback of
> > > > > the corresponding cacheline. When restoring the context the GPU would
> > > > > jump over the zeroed out opcode and hang when trying to execute the
> > > > > above parameter dword.
> > > > > 
> > > > > I could easily reproduce this by running igt/gem_render_copy_redux and
> > > > > gem_tiled_blits/basic in parallel, but I guess it could be triggered by
> > > > > anything involving frequent switches between two separate contexts. With
> > > > > this workaround I couldn't reproduce the problem.
> > > > > 
> > > > > v2:
> > > > > - instead of clflushing after updating the tail and PDP values during
> > > > >   context switching, map the corresponding page as uncached to avoid a
> > > > >   race between CPU and GPU, both updating the same cacheline at the same
> > > > >   time (Ville)
> > > > 
> > > > No. Changing PAT involves a stop_machine() and is severely detrimental
> > > > to performance (context creation overhead does impact userspace).
> > > > Mapping it as uncached doesn't remove the race anyway.
> > > 
> > > Yeah it's not pretty, but otoh it's for A stepping and we'll kill it again
> > > once bxt is shipping. I think with a big "IXME: dont ever dare to copy
> > > this" comment this is acceptable. It's not really the worst "my gpu
> > > crawls" workaround we've seend for early hw ...
> > 
> > Thinking about this, an incoherent TAIL write cannot cause IPEHR !=
> > *ACTHD. The flush is just papering over the absence of a flush elsewhere
> > and the root cause remains unfixed.
> 
> I thought the incoherence happens in the ring itself, not the TAIL update.
> That one might just cause additional fun.

Mostly as a reminder to myself, ringbuffer writes are through the GTT
(even for llc execlists platforms currently, though there have been
patches to relax that...) The only way we could get incoherence directly
would be a missing wmb() between the ring writes and the TAIL/ELSP
commit, or missing wmb() between PTE updates.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre