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 <lionel.g.landwer...@intel.com>
> ---
> 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 :(
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
