On Wed, Nov 18, 2015 at 03:08:47PM -0800, Jesse Barnes wrote: > On 11/17/2015 08:37 AM, Daniel Vetter wrote: > > On Fri, Oct 30, 2015 at 04:58:41PM +0000, Chris Wilson wrote: > >> On Fri, Oct 30, 2015 at 05:14:21PM +0100, Daniel Vetter wrote: > >>> On Fri, Oct 23, 2015 at 06:43:32PM +0100, Chris Wilson wrote: > >>>> When accessing through the GTT from one CPU whilst concurrently updating > >>>> the GGTT PTEs in another thread, the hardware likes to return random > >>>> data. As we have strong serialisation prevent us from modifying the PTE > >>>> of an active GTT mmapping, we have to conclude that it whilst modifying > >>>> other PTE's that error occurs. (I have not looked for any pattern such > >>>> as modifying PTE within the same page or cacheline as active PTE - > >>>> though checking whether revoking neighbouring objects should be enough > >>>> to test that theory.) The corruption also seems restricted to Braswell > >>>> and disappears with maxcpus=0. This patch stops all access through the > >>>> GTT by other CPUs when we update any PTE by stopping the machine around > >>>> the GGTT update. > >>>> > >>>> Testcase: igt/gem_concurrent_blit > >>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89079 > >>>> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > >>> > >>> Wild guess, since it wouldn't be the first time hw engineers screwed this > >>> up. > >>> > >>> Cheers, Daniel > >>> > >>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>> index d1c5cf89fe77..de983c8e6e54 100644 > >>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>> @@ -2337,12 +2337,8 @@ int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj) > >>> > >>> static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte) > >>> { > >>> -#ifdef writeq > >>> - writeq(pte, addr); > >>> -#else > >>> iowrite32((u32)pte, addr); > >>> iowrite32(pte >> 32, addr + 4); > >>> -#endif > >> > >> Tried: > >> static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte) > >> { > >> -#ifdef writeq > >> - writeq(pte, addr); > >> -#else > >> - iowrite32((u32)pte, addr); > >> - iowrite32(pte >> 32, addr + 4); > >> -#endif > >> + iowrite32(0, addr); > >> + wmb(); > >> + iowrite32(upper_32_bits(pte), addr + 4); > >> + iowrite32(lower_32_bits(pte), addr); > >> + wmb(); > >> } > >> > >> and just the plain iowrite(lower), iowrite(upper), neither helps. > > > > Added a note about this and applied to dinq. Yay for awesome hw. > > I thought Ville explained how this wasn't necessary? Ville can't repro, Chris claims it fixes something, I don't have a system. We probably should dig into this more, but since I didn't see anything going on I figured I can just pull it in for now. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
