[Mesa-dev] [PATCH] i965: Allow 48-bit addressing on Gen8+.

[Chris Wilson]

Quoting Kenneth Graunke (2018-02-27 00:05:46)
> +static bool
> +gem_supports_48b_addresses(int fd)
> +{
> +   struct drm_i915_gem_exec_object2 obj = {
> +      .flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS,
> +   };
> +
> +   struct drm_i915_gem_execbuffer2 execbuf = {
> +      .buffers_ptr = (uintptr_t)&obj,
> +      .buffer_count = 1,
> +      .rsvd1 = 0xffffffu,
> +   };
> +
> +   int ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf);
> +
> +   return ret == -1 && errno == ENOENT;

Note that this reports ENOENT due to the unknown ctx id which is
evaluated before the object now. But since you are only looking for the
flag, that's ok as the behaviour changed after the flag was introduced.

> +}
> +
>  /**
>   * Initializes the GEM buffer manager, which uses the kernel to allocate, map,
>   * and manage map buffer objections.
> @@ -1418,6 +1439,8 @@ brw_bufmgr_init(struct gen_device_info *devinfo, int fd)
>  
>     bufmgr->has_llc = devinfo->has_llc;
>     bufmgr->has_mmap_wc = gem_param(fd, I915_PARAM_MMAP_VERSION) > 0;
> +   bufmgr->supports_48b_addresses =
> +      devinfo->gen >= 8 && gem_supports_48b_addresses(fd);
>  
>     init_cache_buckets(bufmgr);
>  
> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c b/src/mesa/drivers/dri/i965/brw_misc_state.c
> index c4ef6812bff..29d74876c27 100644
> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> @@ -634,6 +634,12 @@ brw_upload_state_base_address(struct brw_context *brw)
>     }
>  
>     if (devinfo->gen >= 8) {
> +      /* STATE_BASE_ADDRESS has issues with 48-bit address spaces.  If the
> +       * address + size as seen by STATE_BASE_ADDRESS overflows 48 bits,
> +       * the GPU appears to treat all accesses to the buffer as being out
> +       * of bounds and returns zero.  To work around this, we pin all SBAs
> +       * to the bottom 4GB.
> +       */

We could do with a quick explanation as to why we can't program the max
size. Aiui, it's because of dynamic resize and so you cannot program
here the size of object as reported to the kernel.

> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c > index 26718e0d1a2..0a8d3a80b64 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -1093,6 +1093,21 @@ emit_reloc(struct intel_batchbuffer *batch,
>     unsigned int index = add_exec_bo(batch, target);
>     struct drm_i915_gem_exec_object2 *entry = &batch->validation_list[index];
>  
> +   if (reloc_flags & RELOC_32BIT) {
> +      /* Restrict this buffer to the low 32 bits of the address space.
> +       *
> +       * Altering the validation list flags restricts it for this batch,
> +       * but we also alter the BO's kflags to restrict it permanently
> +       * (until the BO is destroyed and put back in the cache).  Buffers
> +       * may stay bound across batches, and we want keep it constrained.
> +       */

Hmm, I think the intent is more along the lines of the buffer may be
reused for non-r32b targets between batches, but we want to avoid
ping-pong migrations into r32b (as moving an active buffer will stall,
hmm, can fix if needs must), so once we go r32b we never go back.

> +      target->kflags &= ~EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> +      entry->flags &= ~EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> +
> +      /* RELOC_32BIT is not an EXEC_OBJECT_* flag, so get rid of it. */
> +      reloc_flags &= ~RELOC_32BIT;
> +   }
> +
>     if (reloc_flags)
>        entry->flags |= reloc_flags & batch->valid_reloc_flags;

> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> index fb180289a0c..2e54adb3ed2 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> @@ -119,6 +119,7 @@ struct brw_bufmgr {
>     bool has_llc:1;
>     bool has_mmap_wc:1;
>     bool bo_reuse:1;
> +   bool supports_48b_addresses:1;
>  };
>  
>  static int bo_set_tiling_internal(struct brw_bo *bo, uint32_t tiling_mode,
> @@ -409,6 +410,8 @@ retry:
>     bo->reusable = true;
>     bo->cache_coherent = bufmgr->has_llc;
>     bo->index = -1;
> +   if (bufmgr->supports_48b_addresses)
> +      bo->kflags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS;

bo->kflags = bufmgr->supports_48b_addresses << 3 ?
(hidden behind a bit of syntactic sugar)
-Chris