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

[Imre Deak]

On ke, 2015-09-16 at 09:17 +0100, Chris Wilson wrote:
> On Tue, Sep 15, 2015 at 09:30:20PM +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 CPU cache after any change
> > from the CPU to the context object. Note that this also includes any
> > changes done by the VM core as opposed to the driver, when
> > reading from backing store/bzeroing the pages.
> > 
> > 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.
> > 
> > Note that I also considered using set_pages_array_uc/wc on the context
> > object but this wouldn't work with kmap_atomic which always returns a WB
> > mapping, at least on HIGHMEM. The alternative would be keeping a UC/WC
> > kernel mapping around whenever the context object is pinned, but this
> > would be a bigger change. Since I'm not sure if there would be any
> > benefit in using set_pages_array, I chose the simpler clflush method.
> 
> Nope. Fix execlists to use correct GEM domain management.

Not sure what you mean. In both execlist and ringbuffer mode the context
object is already put to the CPU domain, so there is no difference
there. We don't touch the context in ringbuffer mode, so there is
nothing else to do there. In execlist mode we do and so - after this
change - call i915_gem_clflush_object() which is what is done for all
other GEM objects too. This one in turn will do the proper thing based
on the given platform and the object's cache level, which is also
correct after patch 1/2.

> From experience the whole context object needs to be flushed if no longer
> coherent.

We know it's no longer coherent when we first pin it in
intel_lr_context_do_pin() so we need to flush there the whole object.
(This is after we initialized it in populate_lr_context() and/or
possibly read it back from backing storage.) Afterwards we know it won't
get accessed from the CPU until we submit it (execlist_update_context),
where we only update the ring tail/start and PDP values in it, so there
it's enough to flush these particular addresses.

Btw, I noticed now that I missed the GUC path, where there is a similar
update before submission in lr_context_update().

> Are you absolutely sure that you want to enable snooping on those pages
> since that historically would be bogus? I would expect some strong
> bspec reference saying that it is legal.

Well I don't want it, but this is what we do already anyway. On CHV and
BXT the GTT PTE PAT index is ignored and so all entries map to PAT index
0. We set up this PAT index as snooped, since we also have the HWSP in
GTT. We could go around this by mapping the HWSP also non-snooped and do
uncached seqno reads or as Ville pointed out, moving the HWSP to PPGTT.
In both cases we could set PAT index 0 as not snooped.

In either case I think the above is a separate issue that could be
addressed as a follow-up. Even if we map the context object unsnooped
we'll need the logic to flush it.

--Imre