On 05/09/2018 18:01, Kenneth Graunke wrote:
On Tuesday, September 4, 2018 3:30:42 AM PDT Lionel Landwerlin wrote:
Both brw_bo_map_cpu() & brw_bo_map_wc() assert if mapping the
underlying BO fails. Failing back to brw_bo_map_gtt() doesn't seem to
make any sense for that reason.

We also only call brw_bo_map_gtt() for tiled buffers which as far as
we know are never mapped coherent (coherent is a parameter reserved
for buffer objects which are always allocated linear). So explicitly
assert if we ever run into this case.

This makes checking the kernel about whether GTT maps are actually
coherent unnecessary.

v2: Add some explanation (Chris/Lionel)

Signed-off-by: Lionel Landwerlin <[email protected]>
---
  src/mesa/drivers/dri/i965/brw_bufmgr.c | 34 ++++++++++++--------------
  1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c 
b/src/mesa/drivers/dri/i965/brw_bufmgr.c
index f1675b191c1..95c5439a521 100644
--- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
+++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
@@ -1095,6 +1095,21 @@ brw_bo_map_gtt(struct brw_context *brw, struct brw_bo 
*bo, unsigned flags)
  {
     struct brw_bufmgr *bufmgr = bo->bufmgr;
+ /* It was recently discovered that some platforms cannot do coherent GTT
+    * mappings. The kernel API previously assumed that they were. To deal with
+    * what is effectively a change in ABI, the kernel introduced a
+    * I915_PARAM_MMAP_GTT_COHERENT getparam to let userspace know whether the
+    * underlying platform is capable of coherent GTT mappings.
+    *
+    * Some digging in the i965 codebase revealed that we don't actually do
+    * coherent GTT mappings. Coherent mappings are only used for buffer
+    * objects which are allocated linear and we only call this function for
+    * non linear buffers. The couple of asserts below are a reminder of the
+    * cases where we should use GTT maps.
+    */
+   assert(bo->tiling_mode != I915_TILING_NONE);
+   assert((flags & MAP_COHERENT) == 0);
+
     /* Get a mapping of the buffer if we haven't before. */
     if (bo->map_gtt == NULL) {
        DBG("bo_map_gtt: mmap %d (%s)\n", bo->gem_handle, bo->name);
@@ -1186,25 +1201,6 @@ brw_bo_map(struct brw_context *brw, struct brw_bo *bo, 
unsigned flags)
     else
        map = brw_bo_map_wc(brw, bo, flags);
- /* Allow the attempt to fail by falling back to the GTT where necessary.
-    *
-    * Not every buffer can be mmaped directly using the CPU (or WC), for
-    * example buffers that wrap stolen memory or are imported from other
-    * devices. For those, we have little choice but to use a GTT mmapping.
-    * However, if we use a slow GTT mmapping for reads where we expected fast
-    * access, that order of magnitude difference in throughput will be clearly
-    * expressed by angry users.
-    *
-    * We skip MAP_RAW because we want to avoid map_gtt's fence detiling.
-    */
-   if (!map && !(flags & MAP_RAW)) {
-      if (brw) {
-         perf_debug("Fallback GTT mapping for %s with access flags %x\n",
-                    bo->name, flags);
-      }
-      map = brw_bo_map_gtt(brw, bo, flags);
-   }
-
     return map;
  }
NAK, unless I've missed something this will still break Cherryview
Chromebooks using ancient kernels that don't support WC mappings.

The assert !COHERENT is fine.  The rest is only valid if
bufmgr->has_mmap_wc, sadly :(
I think the !COHERENT is enough to fulfill the change that happened in the kernel. We just want to avoid coherent GTT maps, so if I leave the fallback and just add the assert() in map_gtt() would that be good with you?

Thanks,
-
Lionel
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to