[Intel-gfx] [libdrm PATCH] intel: Make unsynchronized GTT mappings work on systems with snooping.

Sun Mar 12 22:07:14 UTC 2017

On Sunday, March 12, 2017 6:21:12 AM PDT Chris Wilson wrote:
> On Fri, Mar 10, 2017 at 05:14:32PM -0800, Kenneth Graunke wrote:
> > On systems without LLC, drm_intel_gem_bo_map_unsynchronized() has
> > had the surprising behavior of doing a synchronized GTT mapping.
> > This is obviously not what the user of the API wanted.
> > 
> > Eric left a comment indicating a valid concern: if the CPU and GPU
> > caches are incoherent, we don't keep track of where the user last
> > mapped the buffer, and what caches might contain relevant data.
> 
> Note this is an issue in libdrm_intel not tracking the cache domain
> transitions. Even just a switch between cpu and coherent would solve the
> majority of that - the caveat being shared bo where the tracking is
> incomplete.

Hmm.  Given the API we have today, I suppose we could make libdrm track
that - it just doesn't currently...

>  
> > Modern Atom systems still don't have LLC, but they do offer snooping,
> > which effectively makes the caches coherent.  The kernel appears to
> > set up the PTE/PPAT to enable snooping for everything where the cache
> > level is not I915_CACHE_NONE.  As far as I know, only scanout buffers
> > are marked as uncached.
> 
> Byt, bsw beg to differ. I don't have a bxt to know the results of the
> igt/kernel tests.

They do?  I was reading i915_gem_gtt.c, and see in byt_pte_encode:

        if (level != I915_CACHE_NONE)
                pte |= BYT_PTE_SNOOPED_BY_CPU_CACHES;

and also in chv_setup_private_ppat, a whole lot of CHV_PPAT_SNOOP.
It sure looked to me that we were setting up snooping for all non-UC
buffers.

i915_gem_object_is_coherent() also appears to indicate that LLC or
cache_level != I915_CACHE_NONE guarantees coherency.

Am I missing something?

> > Any buffers used by scanout should be flagged as non-reusable with
> > drm_intel_bo_disable_reuse(), prime export, or flink.  So, we can
> > assume that any reusable buffer should be snooped.
> 
> Not really, there is no reason why scanout buffers can't be reused.

Yes, we could make them reusable.  However, as far as I can tell, libdrm
doesn't consider the cacheability settings and just adds all reusable
BOs back into the cache.

As I understand it, the kernel marks a BO uncached when it's used for
display, and that BO remains uncached forever.  So that would mean that
libdrm's buffer cache would have random uncached buffers in it, which
would be really undesirable.  So I believe libdrm (and its users) mark
all scanout buffers non-reusable.

It would probably be better to check I915_GEM_GET_CACHING !=
I915_CACHING_NONE, but I was hoping to avoid the extra ioctl.

> > This patch enables unsynchronized mappings for reusable buffers
> > on all Gen6+ hardware (which have either LLC or snooping).
> > 
> > On Broxton, this improves the performance of Unigine Valley 1.0
> > on Low settings at 1280x720 by about 45%, and Unigine Heaven 4.0
> > (same settings) by about 53%.
> 
> Does anyone have figures for gtt performance on bxt - does it cover over
> the same performance penalty from earler atoms? Basically why bother to
> enable this over wc mapping (no stalls for a contended, limited
> resource) + detiling. (Just note that for detiling Y to WC you need to
> use a temporary cacheable page, or rearrange the code to make sure the
> reads/writes are in 64 byte chunks.) 

I'd like to use WC mappings eventually, but to me, fixing the
lack-of-async seems kind of orthogonal to changing GTT vs. WC.
Even if we want to use WC, why not fix this too?

> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: mesa-dev at lists.freedesktop.org
> > ---
> >  intel/intel_bufmgr_gem.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > It looks like Mesa and Beignet are the only callers of this function
> > (SNA and Anvil don't use libdrm, UXA and vaapi don't use this function.)
> > 
> > This passed our full barrage of Piglit/dEQP/GLCTS/GLESCTS testing.
> > gnome-shell still works, as does Unigine, and GLBenchmark.
> > 
> > I haven't tested any OpenCL workloads.
> > 
> > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> > index e260f2dc..f53f1fcc 100644
> > --- a/intel/intel_bufmgr_gem.c
> > +++ b/intel/intel_bufmgr_gem.c
> > @@ -1630,9 +1630,7 @@ int
> >  drm_intel_gem_bo_map_unsynchronized(drm_intel_bo *bo)
> >  {
> >  	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
> > -#ifdef HAVE_VALGRIND
> >  	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
> > -#endif
> >  	int ret;
> >  
> >  	/* If the CPU cache isn't coherent with the GTT, then use a
> > @@ -1641,8 +1639,12 @@ drm_intel_gem_bo_map_unsynchronized(drm_intel_bo *bo)
> >  	 * terms of drm_intel_bo_map vs drm_intel_gem_bo_map_gtt, so
> >  	 * we would potentially corrupt the buffer even when the user
> >  	 * does reasonable things.
> > +	 *
> > +	 * The caches are coherent on LLC platforms or snooping is enabled
> > +	 * for the BO.  The kernel enables snooping for non-scanout (reusable)
> > +	 * buffers on modern non-LLC systems.
> 
> gen >= 9; the snoop was reinvented

Then I'd really like to know what those BYT and CHV PTE/PPAT bits
mean...

> Not enabled by default on !llc. By default object and PTE are set to
> uncached, and mocs default is to follow PTE.
> 
> I would just do the unsync map and leave it to the caller to ensure it
> is used correctly. Or just use the raw interfaces that leave the domain
> tracking to the caller.

So, drop this altogether and let the caller sort it out?  I suspect that
would be fine, too.  It seems like we want to deprecate (or at least
stop using) these APIs in the long run, in favor of the raw interfaces,
so in the meantime, as long as any changes do the right thing for
current users, it's probably fine.

> >  	 */
> > -	if (!bufmgr_gem->has_llc)
> > +	if (bufmgr_gem->gen < 6 || !bo_gem->reusable)
> >  		return drm_intel_gem_bo_map_gtt(bo);
> >  
> >  	pthread_mutex_lock(&bufmgr_gem->lock);
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20170312/51919c3d/attachment.sig>