[Mesa-dev] [PATCH 7/7] i965: Fix asynchronous mappings on !LLC platforms.

[Chris Wilson]

Quoting Kenneth Graunke (2017-07-07 06:51:49)
> On Wednesday, July 5, 2017 2:24:55 PM PDT Chris Wilson wrote:
> > Quoting Kenneth Graunke (2017-07-05 21:56:54)
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_bufmgr.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > > index 7756e2b5f6c..46696be3577 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > > @@ -56,6 +56,7 @@
> > >  #ifndef ETIME
> > >  #define ETIME ETIMEDOUT
> > >  #endif
> > > +#include "common/gen_clflush.h"
> > >  #include "common/gen_debug.h"
> > >  #include "common/gen_device_info.h"
> > >  #include "libdrm_macros.h"
> > > @@ -698,12 +699,22 @@ brw_bo_map_cpu(struct brw_context *brw, struct brw_bo *bo, unsigned flags)
> > >           VG(VALGRIND_FREELIKE_BLOCK(map, 0));
> > >           drm_munmap(map, bo->size);
> > >        }
> > > +   } else if (!bo->cache_coherent) {
> > > +      /* If we're reusing an existing CPU mapping, the CPU caches may
> > > +       * contain stale data from the last time we read from that mapping.
> > > +       * (With the BO cache, it might even be data from a previous buffer!)
> > > +       *
> > > +       * We need to invalidate those cachelines so that we see the latest
> > > +       * contents.
> > > +       */
> > > +      gen_invalidate_range(bo->map_cpu, bo->size);
> > >     }
> > 
> > This leaves us in trouble on the first invocation with MAP_ASYNC, where
> > we create the cpu mmaping but don't change any of its domains. (Not that
> > GL allows for READ | UNSYNCRONIZED if my reading of the spec was
> > correct.) However, if you use
> > 
> >       if (!(flags & MAP_ASYNC))
> >               wait_rendering(); /* teach me to use gem_wait! */
> > 
> >       if (!bo->cache_coherent)
> >               gen_invalidate_range();
> > 
> > and a set_domain(GTT, 0) on creation that should cover everything.
> 
> I think I meant to set_domain(GTT, 0) on creation in this series, and
> lost that patch somehow.  It seems like a good idea to add that.
> 
> You're correct, READ | UNSYNCHRONIZED is not allowed.  Since we disallow
> CPU maps for writes, we should never see MAP_ASYNC here.  Which means we'll
> always do gen_invalidate_range() and then set_domain(CPU).
> 
> I'm struggling to see how wait_rendering (aka set_domain(GTT)) helps us
> here...there are no CPU writes to wait for...we disallowed them...

But wait_rendering should not be set_domain! The lightweight path is to
use gem_wait() for wait_rendering (with a fallback to set_domain for
pre-2012 kernels).

> > In the meantime, s/else if (!bo->cache_coherent)/if (!bo->cache_coherent)/
> > -Chris
> 
> Oh?  I can do that.  I figured that when we asked the kernel to create a
> brand new CPU map for us, it would guarantee that the new virtual address
> range didn't have any stale data in the CPU caches.  But, if it doesn't,
> then we definitely need to clflush them out.

Oh no, it just sets up a new mapping in your vma to point to the
physical memory, it doesn't ensure anything about caches to that
physical memory (just the TLB which was cleared for that addr range
on the previous mumap).
-Chris