From 314879f7fec07cedb5263681173a22d522a8ac9a Mon Sep 17 00:00:00 2001 From: Kenneth Graunke Date: Sat, 1 Jul 2017 01:55:55 -0700 Subject: [PATCH] i965: Fix asynchronous mappings on !LLC platforms. When using a read-only CPU mapping, we may encounter stale buffer contents. For example, the Piglit primitive-restart test offers the following scenario: 1. Read data via a CPU map. 2. Destroy that buffer. 3. Create a new buffer - obtaining the same one via the BO cache. 4. Call BufferSubData, which does a GTT map with MAP_WRITE | MAP_ASYNC. (We avoid set_domain for async mappings, so no flushing occurs.) 5. Read data via a CPU map. (Without explicit clflushing, this will contain data from step 1!) Otherwise, everything ought to work, keeping in mind that we never use CPU maps for writing - just read-only CPU maps. This restores the performance gains after Matt's revert in commit 71651b3139c501f50e6547c21a1cdb816b0a9dde. v2: Do the invalidate later, and even when asking for a brand new map. v3: Add more comments from Chris. Reviewed-by: Chris Wilson --- src/mesa/drivers/dri/i965/brw_bufmgr.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c index 30e4b28b9e0..26f194e1923 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" @@ -703,11 +704,25 @@ brw_bo_map_cpu(struct brw_context *brw, struct brw_bo *bo, unsigned flags) bo->map_cpu); print_flags(flags); - if (!(flags & MAP_ASYNC) || !bufmgr->has_llc) { + if (!(flags & MAP_ASYNC)) { set_domain(brw, "CPU mapping", bo, I915_GEM_DOMAIN_CPU, flags & MAP_WRITE ? I915_GEM_DOMAIN_CPU : 0); } + 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!) + * Even if it's a brand new mapping, the kernel may have zeroed the + * buffer via CPU writes. + * + * We need to invalidate those cachelines so that we see the latest + * contents, and so long as we only read from the CPU mmap we do not + * need to write those cachelines back afterwards. + */ + gen_invalidate_range(bo->map_cpu, bo->size); + } + return bo->map_cpu; } @@ -754,7 +769,7 @@ brw_bo_map_gtt(struct brw_context *brw, struct brw_bo *bo, unsigned flags) DBG("bo_map_gtt: %d (%s) -> %p, ", bo->gem_handle, bo->name, bo->map_gtt); print_flags(flags); - if (!(flags & MAP_ASYNC) || !bufmgr->has_llc) { + if (!(flags & MAP_ASYNC)) { set_domain(brw, "GTT mapping", bo, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT); } -- 2.30.2