On Thursday, September 6, 2018 3:30:58 AM PDT Lionel Landwerlin wrote: > 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?
No, that's not workable. The point is that if you have an old kernel
(!bufmgr->has_mmap_wc), even linear mappings may not be CPU mappable
(incoherent atoms), not WC mappable (no kernel support), so they get
GTT mapped. Which means you may have MAP_COHERENT, linear, GTT maps.
The only solutions are:
1. Mandate WC support. Then we could apply this patch. It would
require the ChromeOS guys to backport WC maps to cros-3.18 for CHV.
2. Stop exposing GL_{ARB,EXT}_buffer_storage if !has_llc and !has_wc.
This would eliminate MAP_COHERENT as an API feature.
3. Leave the bug as is - issue a warning instead of an assert.
4. Try and put in a kludge to workaround the bug.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
