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

[Jason Ekstrand]

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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181207/063ad692/attachment.html>