[Intel-gfx] [Mesa-dev] [PATCH mesa] i965/gen8+: bo in state base address must be in 32-bit address range

[Ben Widawsky]

Hi. Feel free to Cc me on patches of this nature. I am far behind on mesa-dev,
and no longer read intel-gfx. I'm probably one of the sensible people to look at
this...

On Tue, Jun 23, 2015 at 01:21:27PM +0100, Michel Thierry wrote:
> Gen8+ supports 48-bit virtual addresses, but some objects must always be
> allocated inside the 32-bit address range.
> 
> In specific, any resource used with flat/heapless (0x00000000-0xfffff000)
> General State Heap (GSH) or Intruction State Heap (ISH) must be in a
> 32-bit range, because the General State Offset and Instruction State Offset
> are limited to 32-bits.

I don't think GSH, or ISH are well known terms that have every appeared
anywhere. I'd just keep the bit after the final comma (...because ...)

> 
> Set provided bo flag when the 4GB limit is not necessary, to be able to use
> the full address space.

I'm glad you got around to this. We'd been putting it off for a long time.

> 
> Cc: mesa-dev at lists.freedesktop.org
> Signed-off-by: Michel Thierry <michel.thierry at intel.com>
> ---
>  src/mesa/drivers/dri/i965/gen8_misc_state.c   | 6 +++---
>  src/mesa/drivers/dri/i965/intel_batchbuffer.h | 7 +++++++
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen8_misc_state.c b/src/mesa/drivers/dri/i965/gen8_misc_state.c
> index b20038e..26531d0 100644
> --- a/src/mesa/drivers/dri/i965/gen8_misc_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_misc_state.c
> @@ -41,17 +41,17 @@ void gen8_upload_state_base_address(struct brw_context *brw)
>     OUT_BATCH(0);
>     OUT_BATCH(mocs_wb << 16);
>     /* Surface state base address: */
> -   OUT_RELOC64(brw->batch.bo, I915_GEM_DOMAIN_SAMPLER, 0,
> +   OUT_RELOC64_32BWA(brw->batch.bo, I915_GEM_DOMAIN_SAMPLER, 0,
>                 mocs_wb << 4 | 1);
>     /* Dynamic state base address: */
> -   OUT_RELOC64(brw->batch.bo,
> +   OUT_RELOC64_32BWA(brw->batch.bo,
>                 I915_GEM_DOMAIN_RENDER | I915_GEM_DOMAIN_INSTRUCTION, 0,
>                 mocs_wb << 4 | 1);
>     /* Indirect object base address: MEDIA_OBJECT data */
>     OUT_BATCH(mocs_wb << 4 | 1);
>     OUT_BATCH(0);
>     /* Instruction base address: shader kernels (incl. SIP) */
> -   OUT_RELOC64(brw->cache.bo, I915_GEM_DOMAIN_INSTRUCTION, 0,
> +   OUT_RELOC64_32BWA(brw->cache.bo, I915_GEM_DOMAIN_INSTRUCTION, 0,
>                 mocs_wb << 4 | 1);
>  
>     /* General state buffer size */
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> index 7bdd836..5aa741e 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
> @@ -177,6 +177,13 @@ intel_batchbuffer_advance(struct brw_context *brw)
>  
>  /* Handle 48-bit address relocations for Gen8+ */
>  #define OUT_RELOC64(buf, read_domains, write_domain, delta) do { \
> +   drm_intel_bo_set_supports_48baddress(buf); \
> +   intel_batchbuffer_emit_reloc64(brw, buf, read_domains, write_domain, delta);	\
> +} while (0)
> +
> +/* Handle 48-bit address relocations for Gen8+, ask for 32-bit address */
> +#define OUT_RELOC64_32BWA(buf, read_domains, write_domain, delta) do { \
> +   drm_intel_bo_clear_supports_48baddress(buf); \
>     intel_batchbuffer_emit_reloc64(brw, buf, read_domains, write_domain, delta);	\
>  } while (0)
>  

First and least bikesheddy, you need to bump the required libdrm in the
configure.ac to support this new libdrm function (maybe you did, but I don't see
it on mesa-dev).

More bikesheddy, and forgive me here because I haven't looked at any of the
kernel interfaces or libdrm patches (you can Cc those to mesa-dev if they're
relevant fwiw).

Presumably at the end of the day it's drm_intel_bo_emit_reloc which needs to
know about these limitations. Unfortunately we don't have a flags field there.
The implementation here seems like a somewhat cumbersome workaround for that (it
looks like the context execbuf which is pretty crappy - yes, I know who the
author was). Have you already discussed adding a new emit_reloc? I suppose if
people are opposed to a new emit reloc, the only I'd like to see different is
have the functions which need the workaround just call OUT_RELOC, instead of
OUT_RELOC64 (put a comment in the call sites), and make OUT_RELOC call the
drm_intel_bo_clear_supports_48baddress() (which is obviously a nop on pre-gen8
platforms). The OUT_RELOC64 case should be left alone - we shouldn't need to
tell libdrm that I want a 64bit relocation, and it can actually be 64b...

I suspect not many other mesa devs will have an opinion here, but I'm flexible
if they disagree.