[3/3] intel/blorp: Add a simpler interface for softpin-only drivers.

Submitted by Kenneth Graunke on Nov. 29, 2018, 8:24 a.m.

Details

Message ID 20181129082440.2397-3-kenneth@whitecape.org
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Kenneth Graunke Nov. 29, 2018, 8:24 a.m.
Drivers using softpin do not need (or have) relocations.  The old
relocation interface is somewhat awkward and limiting.

In particular, brw_surface_reloc assumes that all SURFACE_STATEs will
exist in a single buffer, and only provides ss_offset.  The driver is
supposed to implicitly know about this buffer, in order to offset from
a CPU map of that buffer to write the value.  With softpin, we can put
SURFACE_STATEs in any buffer we like, as long as it's within 4GB of
Surface State Base Address.  So, this model breaks down.

Drivers can now #define BLORP_USE_SOFTPIN and define a simpler
blorp_use_pinned_bo() helper, which adds the buffer to the validation
list for the current batch, and returns the (statically assigned,
unchanging) address for the buffer.

The upcoming Iris driver will use this interface.
---
 src/intel/blorp/blorp_genX_exec.h | 36 +++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/blorp/blorp_genX_exec.h b/src/intel/blorp/blorp_genX_exec.h
index 20f30c7116d..f18de3dc6ce 100644
--- a/src/intel/blorp/blorp_genX_exec.h
+++ b/src/intel/blorp/blorp_genX_exec.h
@@ -47,10 +47,27 @@ 
 static void *
 blorp_emit_dwords(struct blorp_batch *batch, unsigned n);
 
+#ifdef BLORP_USE_SOFTPIN
+static uint64_t
+blorp_use_pinned_bo(struct blorp_batch *batch, struct blorp_address addr);
+
+/* Wrappers to avoid #ifdefs everywhere */
+#define blorp_emit_reloc _blorp_combine_address
+static inline void
+blorp_surface_reloc(struct blorp_batch *batch, uint32_t ss_offset,
+                    struct blorp_address address, uint32_t delta)
+{
+}
+#else
 static uint64_t
 blorp_emit_reloc(struct blorp_batch *batch,
                  void *location, struct blorp_address address, uint32_t delta);
 
+static void
+blorp_surface_reloc(struct blorp_batch *batch, uint32_t ss_offset,
+                    struct blorp_address address, uint32_t delta);
+#endif
+
 static void *
 blorp_alloc_dynamic_state(struct blorp_batch *batch,
                           uint32_t size,
@@ -78,10 +95,6 @@  blorp_alloc_binding_table(struct blorp_batch *batch, unsigned num_entries,
 static void
 blorp_flush_range(struct blorp_batch *batch, void *start, size_t size);
 
-static void
-blorp_surface_reloc(struct blorp_batch *batch, uint32_t ss_offset,
-                    struct blorp_address address, uint32_t delta);
-
 #if GEN_GEN >= 7 && GEN_GEN < 10
 static struct blorp_address
 blorp_get_surface_base_address(struct blorp_batch *batch);
@@ -104,15 +117,23 @@  _blorp_combine_address(struct blorp_batch *batch, void *location,
    if (address.buffer == NULL) {
       return address.offset + delta;
    } else {
+#ifdef BLORP_USE_SOFTPIN
+      return blorp_use_pinned_bo(batch, address) + delta;
+#else
       return blorp_emit_reloc(batch, location, address, delta);
+#endif
    }
 }
 
 static uint64_t
 KSP(struct blorp_batch *batch, struct blorp_address address)
 {
+#ifdef BLORP_USE_SOFTPIN
+   return blorp_use_pinned_bo(batch, address);
+#else
    assert(address.buffer == NULL);
    return address.offset;
+#endif
 }
 
 #define __gen_address_type struct blorp_address
@@ -1370,6 +1391,13 @@  blorp_emit_surface_state(struct blorp_batch *batch,
    isl_surf_fill_state(batch->blorp->isl_dev, state,
                        .surf = &surf, .view = &surface->view,
                        .aux_surf = &surface->aux_surf, .aux_usage = aux_usage,
+#ifdef BLORP_USE_SOFTPIN
+                       .address = blorp_use_pinned_bo(batch, surface->addr),
+                       .aux_address = aux_usage != ISL_AUX_USAGE_NONE ?
+                          blorp_use_pinned_bo(batch, surface->aux_addr) : 0,
+                       .clear_address = !use_clear_address ? 0 :
+                          blorp_use_pinned_bo(batch, surface->clear_color_addr),
+#endif
                        .mocs = surface->addr.mocs,
                        .clear_color = surface->clear_color,
                        .use_clear_address = use_clear_address,

Comments

I must say, I don't really like this patch.  It seems like it's just trying
to let you avoid writing two reloc functions which should, by and large, be
no-ops for iris.    That said, the way blorp does surface relocs is really
annoying because it forces the reloc function to write in the value.
Unfortunately, untangling this would require passing a reloc function
pointer into isl_surf_fill_state.  Maybe it's time we did that?  On the
other hand, future drivers are going to be softpin-only so I'm a bit
hesitant to add more interface for relocations.  Bah!

In the end, I do think I agree with Jordan in not liking the #ifdef.  Maybe
we can just force all drivers to define all three functions and anv/i965
can make blorp_use_address() function a no-op.  Would that be reasonable?

--Jason

On Thu, Nov 29, 2018 at 2:24 AM Kenneth Graunke <kenneth@whitecape.org>
wrote:

> Drivers using softpin do not need (or have) relocations.  The old
> relocation interface is somewhat awkward and limiting.
>
> In particular, brw_surface_reloc assumes that all SURFACE_STATEs will
> exist in a single buffer, and only provides ss_offset.  The driver is
> supposed to implicitly know about this buffer, in order to offset from
> a CPU map of that buffer to write the value.  With softpin, we can put
> SURFACE_STATEs in any buffer we like, as long as it's within 4GB of
> Surface State Base Address.  So, this model breaks down.
>
> Drivers can now #define BLORP_USE_SOFTPIN and define a simpler
> blorp_use_pinned_bo() helper, which adds the buffer to the validation
> list for the current batch, and returns the (statically assigned,
> unchanging) address for the buffer.
>
> The upcoming Iris driver will use this interface.
> ---
>  src/intel/blorp/blorp_genX_exec.h | 36 +++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/src/intel/blorp/blorp_genX_exec.h
> b/src/intel/blorp/blorp_genX_exec.h
> index 20f30c7116d..f18de3dc6ce 100644
> --- a/src/intel/blorp/blorp_genX_exec.h
> +++ b/src/intel/blorp/blorp_genX_exec.h
> @@ -47,10 +47,27 @@
>  static void *
>  blorp_emit_dwords(struct blorp_batch *batch, unsigned n);
>
> +#ifdef BLORP_USE_SOFTPIN
> +static uint64_t
> +blorp_use_pinned_bo(struct blorp_batch *batch, struct blorp_address addr);
> +
> +/* Wrappers to avoid #ifdefs everywhere */
> +#define blorp_emit_reloc _blorp_combine_address
> +static inline void
> +blorp_surface_reloc(struct blorp_batch *batch, uint32_t ss_offset,
> +                    struct blorp_address address, uint32_t delta)
> +{
> +}
> +#else
>  static uint64_t
>  blorp_emit_reloc(struct blorp_batch *batch,
>                   void *location, struct blorp_address address, uint32_t
> delta);
>
> +static void
> +blorp_surface_reloc(struct blorp_batch *batch, uint32_t ss_offset,
> +                    struct blorp_address address, uint32_t delta);
> +#endif
> +
>  static void *
>  blorp_alloc_dynamic_state(struct blorp_batch *batch,
>                            uint32_t size,
> @@ -78,10 +95,6 @@ blorp_alloc_binding_table(struct blorp_batch *batch,
> unsigned num_entries,
>  static void
>  blorp_flush_range(struct blorp_batch *batch, void *start, size_t size);
>
> -static void
> -blorp_surface_reloc(struct blorp_batch *batch, uint32_t ss_offset,
> -                    struct blorp_address address, uint32_t delta);
> -
>  #if GEN_GEN >= 7 && GEN_GEN < 10
>  static struct blorp_address
>  blorp_get_surface_base_address(struct blorp_batch *batch);
> @@ -104,15 +117,23 @@ _blorp_combine_address(struct blorp_batch *batch,
> void *location,
>     if (address.buffer == NULL) {
>        return address.offset + delta;
>     } else {
> +#ifdef BLORP_USE_SOFTPIN
> +      return blorp_use_pinned_bo(batch, address) + delta;
> +#else
>        return blorp_emit_reloc(batch, location, address, delta);
> +#endif
>     }
>  }
>
>  static uint64_t
>  KSP(struct blorp_batch *batch, struct blorp_address address)
>  {
> +#ifdef BLORP_USE_SOFTPIN
> +   return blorp_use_pinned_bo(batch, address);
> +#else
>     assert(address.buffer == NULL);
>     return address.offset;
> +#endif
>  }
>
>  #define __gen_address_type struct blorp_address
> @@ -1370,6 +1391,13 @@ blorp_emit_surface_state(struct blorp_batch *batch,
>     isl_surf_fill_state(batch->blorp->isl_dev, state,
>                         .surf = &surf, .view = &surface->view,
>                         .aux_surf = &surface->aux_surf, .aux_usage =
> aux_usage,
> +#ifdef BLORP_USE_SOFTPIN
> +                       .address = blorp_use_pinned_bo(batch,
> surface->addr),
> +                       .aux_address = aux_usage != ISL_AUX_USAGE_NONE ?
> +                          blorp_use_pinned_bo(batch, surface->aux_addr) :
> 0,
> +                       .clear_address = !use_clear_address ? 0 :
> +                          blorp_use_pinned_bo(batch,
> surface->clear_color_addr),
> +#endif
>                         .mocs = surface->addr.mocs,
>                         .clear_color = surface->clear_color,
>                         .use_clear_address = use_clear_address,
> --
> 2.19.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>