On 11/19/2015 01:35 AM, Chris Wilson wrote: > On Thu, Nov 19, 2015 at 10:14:08AM +0100, Daniel Vetter wrote: >> 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. > > Both myself, old QA (when they finally got around to running some of the > coherency tests), new QA and VPG have reported coherency issues with > access through the GGTT. I can believe it; it would be good to find the root cause the hw issue though. Obviously we're not understanding something fully... Jesse _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
