[Intel-gfx] [PATCH 2/2] drm/i915: Serialise updates to GGTT with access through GGTT on Braswell

[Chris Wilson]

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 at chris-wilson.co.uk>
> > >>>
> > >>> 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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre