[Mesa-dev] [PATCH 2/3] intel/blorp: Make KSP a blorp

Discussion:

[Mesa-dev] [PATCH 2/3] intel/blorp: Make KSP a blorp_address instead of an offset.

Kenneth Graunke

2018-11-29 08:24:39 UTC

In i965, shader programs live in a single buffer, and every batch emits
a STATE_BASE_ADDRESS packet pointing to that buffer. This takes care of
pinning the buffer for the batch; from then on, we can use an offset.

In the upcoming Iris driver, shader programs can live in multiple
buffers, and those buffers need to be pinned when shader assembly is
referenced. To aid in this, we turn KSP into a blorp_address rather
than a 32-bit offset. This lets us also pass a buffer to pin.

For now, we simply assert the BO is NULL and return the offset, as
we did before - nothing should behave differently.
---
src/intel/blorp/blorp.h | 7 ++++--
src/intel/blorp/blorp_genX_exec.h | 27 +++++++++++++--------
src/intel/blorp/blorp_priv.h | 6 ++---
src/intel/vulkan/anv_blorp.c | 10 +++++---
src/mesa/drivers/dri/i965/brw_blorp.c | 18 ++++++++++----
src/mesa/drivers/dri/i965/gen4_blorp_exec.h | 12 ++++-----
6 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/src/intel/blorp/blorp.h b/src/intel/blorp/blorp.h
index 1e22712602d..da0c9ac205c 100644
--- a/src/intel/blorp/blorp.h
+++ b/src/intel/blorp/blorp.h
@@ -35,6 +35,7 @@ struct brw_stage_prog_data;
extern "C" {
#endif

+struct blorp_address;
struct blorp_batch;
struct blorp_params;

@@ -47,13 +48,15 @@ struct blorp_context {

bool (*lookup_shader)(struct blorp_context *blorp,
const void *key, uint32_t key_size,
- uint32_t *kernel_out, void *prog_data_out);
+ struct blorp_address *kernel_out,
+ void *prog_data_out);
bool (*upload_shader)(struct blorp_context *blorp,
const void *key, uint32_t key_size,
const void *kernel, uint32_t kernel_size,
const struct brw_stage_prog_data *prog_data,
uint32_t prog_data_size,
- uint32_t *kernel_out, void *prog_data_out);
+ struct blorp_address *kernel_out,
+ void *prog_data_out);
void (*exec)(struct blorp_batch *batch, const struct blorp_params *params);
};

diff --git a/src/intel/blorp/blorp_genX_exec.h b/src/intel/blorp/blorp_genX_exec.h
index 065980616ec..20f30c7116d 100644
--- a/src/intel/blorp/blorp_genX_exec.h
+++ b/src/intel/blorp/blorp_genX_exec.h
@@ -108,6 +108,13 @@ _blorp_combine_address(struct blorp_batch *batch, void *location,
}
}

+static uint64_t
+KSP(struct blorp_batch *batch, struct blorp_address address)
+{
+ assert(address.buffer == NULL);
+ return address.offset;
+}
+
#define __gen_address_type struct blorp_address
#define __gen_user_data struct blorp_batch
#define __gen_combine_address _blorp_combine_address
@@ -615,7 +622,7 @@ blorp_emit_vs_config(struct blorp_batch *batch,
if (vs_prog_data) {
vs.Enable = true;

- vs.KernelStartPointer = params->vs_prog_kernel;
+ vs.KernelStartPointer = KSP(batch, params->vs_prog_kernel);

vs.DispatchGRFStartRegisterForURBData =
vs_prog_data->base.base.dispatch_grf_start_reg;
@@ -795,11 +802,11 @@ blorp_emit_ps_config(struct blorp_batch *batch,
ps.DispatchGRFStartRegisterForConstantSetupData2 =
brw_wm_prog_data_dispatch_grf_start_reg(prog_data, ps, 2);

- ps.KernelStartPointer0 = params->wm_prog_kernel +
+ ps.KernelStartPointer0 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 0);
- ps.KernelStartPointer1 = params->wm_prog_kernel +
+ ps.KernelStartPointer1 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 1);
- ps.KernelStartPointer2 = params->wm_prog_kernel +
+ ps.KernelStartPointer2 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 2);
}

@@ -905,11 +912,11 @@ blorp_emit_ps_config(struct blorp_batch *batch,
ps.DispatchGRFStartRegisterForConstantSetupData2 =
brw_wm_prog_data_dispatch_grf_start_reg(prog_data, ps, 2);

- ps.KernelStartPointer0 = params->wm_prog_kernel +
+ ps.KernelStartPointer0 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 0);
- ps.KernelStartPointer1 = params->wm_prog_kernel +
+ ps.KernelStartPointer1 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 1);
- ps.KernelStartPointer2 = params->wm_prog_kernel +
+ ps.KernelStartPointer2 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 2);

ps.AttributeEnable = prog_data->num_varying_inputs > 0;
@@ -973,11 +980,11 @@ blorp_emit_ps_config(struct blorp_batch *batch,
wm.DispatchGRFStartRegisterForConstantSetupData2 =
brw_wm_prog_data_dispatch_grf_start_reg(prog_data, wm, 2);

- wm.KernelStartPointer0 = params->wm_prog_kernel +
+ wm.KernelStartPointer0 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 0);
- wm.KernelStartPointer1 = params->wm_prog_kernel +
+ wm.KernelStartPointer1 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 1);
- wm.KernelStartPointer2 = params->wm_prog_kernel +
+ wm.KernelStartPointer2 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 2);

wm.NumberofSFOutputAttributes = prog_data->num_varying_inputs;
diff --git a/src/intel/blorp/blorp_priv.h b/src/intel/blorp/blorp_priv.h
index a6aa2aa4151..b60505ea06a 100644
--- a/src/intel/blorp/blorp_priv.h
+++ b/src/intel/blorp/blorp_priv.h
@@ -203,11 +203,11 @@ struct blorp_params
unsigned num_samples;
unsigned num_draw_buffers;
unsigned num_layers;
- uint32_t vs_prog_kernel;
+ struct blorp_address vs_prog_kernel;
struct brw_vs_prog_data *vs_prog_data;
- uint32_t sf_prog_kernel;
+ struct blorp_address sf_prog_kernel;
struct brw_sf_prog_data *sf_prog_data;
- uint32_t wm_prog_kernel;
+ struct blorp_address wm_prog_kernel;
struct brw_wm_prog_data *wm_prog_data;

bool use_pre_baked_binding_table;
diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
index 478b8e7a3db..e6bcf4b0bc1 100644
--- a/src/intel/vulkan/anv_blorp.c
+++ b/src/intel/vulkan/anv_blorp.c
@@ -26,7 +26,8 @@
static bool
lookup_blorp_shader(struct blorp_context *blorp,
const void *key, uint32_t key_size,
- uint32_t *kernel_out, void *prog_data_out)
+ struct blorp_address *kernel_out,
+ void *prog_data_out)
{
struct anv_device *device = blorp->driver_ctx;

@@ -43,7 +44,7 @@ lookup_blorp_shader(struct blorp_context *blorp,
*/
anv_shader_bin_unref(device, bin);

- *kernel_out = bin->kernel.offset;
+ *kernel_out = (struct blorp_address) { .offset = bin->kernel.offset };
*(const struct brw_stage_prog_data **)prog_data_out = bin->prog_data;

return true;
@@ -55,7 +56,8 @@ upload_blorp_shader(struct blorp_context *blorp,
const void *kernel, uint32_t kernel_size,
const struct brw_stage_prog_data *prog_data,
uint32_t prog_data_size,
- uint32_t *kernel_out, void *prog_data_out)
+ struct blorp_address *kernel_out,
+ void *prog_data_out)
{
struct anv_device *device = blorp->driver_ctx;

@@ -81,7 +83,7 @@ upload_blorp_shader(struct blorp_context *blorp,
*/
anv_shader_bin_unref(device, bin);

- *kernel_out = bin->kernel.offset;
+ *kernel_out = (struct blorp_address) { .offset = bin->kernel.offset };
*(const struct brw_stage_prog_data **)prog_data_out = bin->prog_data;

return true;
diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c b/src/mesa/drivers/dri/i965/brw_blorp.c
index b286b231537..d021e2906fe 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp.c
+++ b/src/mesa/drivers/dri/i965/brw_blorp.c
@@ -45,11 +45,16 @@
static bool
brw_blorp_lookup_shader(struct blorp_context *blorp,
const void *key, uint32_t key_size,
- uint32_t *kernel_out, void *prog_data_out)
+ struct blorp_address *kernel_out,
+ void *prog_data_out)
{
struct brw_context *brw = blorp->driver_ctx;
- return brw_search_cache(&brw->cache, BRW_CACHE_BLORP_PROG, key, key_size,
- kernel_out, prog_data_out, true);
+ uint32_t offset;
+ bool found =
+ brw_search_cache(&brw->cache, BRW_CACHE_BLORP_PROG, key, key_size,
+ &offset, prog_data_out, true);
+ *kernel_out = (struct blorp_address) { .offset = offset };
+ return found;
}

static bool
@@ -58,12 +63,15 @@ brw_blorp_upload_shader(struct blorp_context *blorp,
const void *kernel, uint32_t kernel_size,
const struct brw_stage_prog_data *prog_data,
uint32_t prog_data_size,
- uint32_t *kernel_out, void *prog_data_out)
+ struct blorp_address *kernel_out,
+ void *prog_data_out)
{
struct brw_context *brw = blorp->driver_ctx;
+ uint32_t offset;
brw_upload_cache(&brw->cache, BRW_CACHE_BLORP_PROG, key, key_size,
kernel, kernel_size, prog_data, prog_data_size,
- kernel_out, prog_data_out);
+ &offset, prog_data_out);
+ *kernel_out = (struct blorp_address) { .offset = offset };
return true;
}

diff --git a/src/mesa/drivers/dri/i965/gen4_blorp_exec.h b/src/mesa/drivers/dri/i965/gen4_blorp_exec.h
index 0edc518fa35..3b0f1ea0979 100644
--- a/src/mesa/drivers/dri/i965/gen4_blorp_exec.h
+++ b/src/mesa/drivers/dri/i965/gen4_blorp_exec.h
@@ -77,9 +77,9 @@ blorp_emit_sf_state(struct blorp_batch *batch,
blorp_emit_dynamic(batch, GENX(SF_STATE), sf, 64, &offset) {
#if GEN_GEN == 4
sf.KernelStartPointer =
- instruction_state_address(batch, params->sf_prog_kernel);
+ instruction_state_address(batch, KSP(batch, params->sf_prog_kernel));
#else
- sf.KernelStartPointer = params->sf_prog_kernel;
+ sf.KernelStartPointer = KSP(batch, params->sf_prog_kernel);
#endif
sf.GRFRegisterCount = DIV_ROUND_UP(prog_data->total_grf, 16) - 1;
sf.VertexURBEntryReadLength = prog_data->urb_read_length;
@@ -136,14 +136,14 @@ blorp_emit_wm_state(struct blorp_batch *batch,

#if GEN_GEN == 4
wm.KernelStartPointer0 =
- instruction_state_address(batch, params->wm_prog_kernel);
+ instruction_state_address(batch, KSP(batch, params->wm_prog_kernel));
wm.GRFRegisterCount0 = brw_wm_prog_data_reg_blocks(prog_data, wm, 0);
#else
- wm.KernelStartPointer0 = params->wm_prog_kernel +
+ wm.KernelStartPointer0 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 0);
- wm.KernelStartPointer1 = params->wm_prog_kernel +
+ wm.KernelStartPointer1 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 1);
- wm.KernelStartPointer2 = params->wm_prog_kernel +
+ wm.KernelStartPointer2 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 2);
wm.GRFRegisterCount0 = brw_wm_prog_data_reg_blocks(prog_data, wm, 0);
wm.GRFRegisterCount1 = brw_wm_prog_data_reg_blocks(prog_data, wm, 1);

--
2.19.1

Kenneth Graunke

2018-11-29 08:24:40 UTC

Permalink

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

Jason Ekstrand

2018-12-07 22:26:51 UTC

Permalink

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

Post by Kenneth Graunke
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,
+ 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
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Jason Ekstrand

2018-12-07 21:47:42 UTC

Permalink

Down with 32-bit offsets!

In the softpin world, surface state base address may be a fixed 64-bit
address (with no associated BO). It makes sense to store this in the
offset field. But it needs to be the full size.
We also update the clear color address to be consistently uint64_t
everywhere so we can continue passing intel_miptree_get_clear_color
a pointer to the blorp_address's offset field without type mismatches.
---
src/intel/blorp/blorp.h | 2 +-
src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 2 +-
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 2 +-
src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/intel/blorp/blorp.h b/src/intel/blorp/blorp.h
index ee343a4a6bb..1e22712602d 100644
--- a/src/intel/blorp/blorp.h
+++ b/src/intel/blorp/blorp.h
@@ -91,8 +91,8 @@ void blorp_batch_finish(struct blorp_batch *batch);
struct blorp_address {
void *buffer;
+ uint64_t offset;
unsigned reloc_flags;
- uint32_t offset;
uint32_t mocs;
};
diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
index 4daa0e2add1..b067a174056 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -156,7 +156,7 @@ brw_emit_surface_state(struct brw_context *brw,
struct isl_surf *aux_surf = NULL;
uint64_t aux_offset = 0;
struct brw_bo *clear_bo = NULL;
- uint32_t clear_offset = 0;
+ uint64_t clear_offset = 0;
if (aux_usage != ISL_AUX_USAGE_NONE) {
aux_surf = &mt->aux_buf->surf;
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 8e50aabb3b4..a679ddf3e48 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -3865,7 +3865,7 @@ intel_miptree_get_clear_color(const struct
gen_device_info *devinfo,
const struct intel_mipmap_tree *mt,
enum isl_format view_format, bool sampling,
struct brw_bo **clear_color_bo,
- uint32_t *clear_color_offset)
+ uint64_t *clear_color_offset)
{
assert(mt->aux_buf);
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
index b0333655ad5..21beeded92a 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
@@ -714,7 +714,7 @@ intel_miptree_get_clear_color(const struct
gen_device_info *devinfo,
const struct intel_mipmap_tree *mt,
enum isl_format view_format, bool sampling,
struct brw_bo **clear_color_bo,
- uint32_t *clear_color_offset);
+ uint64_t *clear_color_offset);
static inline int
--
2.19.1
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Jason Ekstrand

2018-12-07 22:13:27 UTC

Permalink

I kind-of wonder if we want to allow for relocations and not surface state
base address in which case you'd want to do something "real" with offset.
Making this an address and then ignoring the buffer part entirely seems
like it promises a bit too much. Then again, I don't think it's really
hurting anything. Meh; this is probably fine.

Post by Kenneth Graunke
In i965, shader programs live in a single buffer, and every batch emits
a STATE_BASE_ADDRESS packet pointing to that buffer. This takes care of
pinning the buffer for the batch; from then on, we can use an offset.
In the upcoming Iris driver, shader programs can live in multiple
buffers, and those buffers need to be pinned when shader assembly is
referenced. To aid in this, we turn KSP into a blorp_address rather
than a 32-bit offset. This lets us also pass a buffer to pin.
For now, we simply assert the BO is NULL and return the offset, as
we did before - nothing should behave differently.
---
src/intel/blorp/blorp.h | 7 ++++--
src/intel/blorp/blorp_genX_exec.h | 27 +++++++++++++--------
src/intel/blorp/blorp_priv.h | 6 ++---
src/intel/vulkan/anv_blorp.c | 10 +++++---
src/mesa/drivers/dri/i965/brw_blorp.c | 18 ++++++++++----
src/mesa/drivers/dri/i965/gen4_blorp_exec.h | 12 ++++-----
6 files changed, 50 insertions(+), 30 deletions(-)
diff --git a/src/intel/blorp/blorp.h b/src/intel/blorp/blorp.h
index 1e22712602d..da0c9ac205c 100644
--- a/src/intel/blorp/blorp.h
+++ b/src/intel/blorp/blorp.h
@@ -35,6 +35,7 @@ struct brw_stage_prog_data;
extern "C" {
#endif
+struct blorp_address;
struct blorp_batch;
struct blorp_params;
@@ -47,13 +48,15 @@ struct blorp_context {
bool (*lookup_shader)(struct blorp_context *blorp,
const void *key, uint32_t key_size,
- uint32_t *kernel_out, void *prog_data_out);
+ struct blorp_address *kernel_out,
+ void *prog_data_out);
bool (*upload_shader)(struct blorp_context *blorp,
const void *key, uint32_t key_size,
const void *kernel, uint32_t kernel_size,
const struct brw_stage_prog_data *prog_data,
uint32_t prog_data_size,
- uint32_t *kernel_out, void *prog_data_out);
+ struct blorp_address *kernel_out,
+ void *prog_data_out);
void (*exec)(struct blorp_batch *batch, const struct blorp_params *params);
};
diff --git a/src/intel/blorp/blorp_genX_exec.h
b/src/intel/blorp/blorp_genX_exec.h
index 065980616ec..20f30c7116d 100644
--- a/src/intel/blorp/blorp_genX_exec.h
+++ b/src/intel/blorp/blorp_genX_exec.h
@@ -108,6 +108,13 @@ _blorp_combine_address(struct blorp_batch *batch, void *location,
}
}
+static uint64_t
+KSP(struct blorp_batch *batch, struct blorp_address address)
+{
+ assert(address.buffer == NULL);
+ return address.offset;
+}
+
#define __gen_address_type struct blorp_address
#define __gen_user_data struct blorp_batch
#define __gen_combine_address _blorp_combine_address
@@ -615,7 +622,7 @@ blorp_emit_vs_config(struct blorp_batch *batch,
if (vs_prog_data) {
vs.Enable = true;
- vs.KernelStartPointer = params->vs_prog_kernel;
+ vs.KernelStartPointer = KSP(batch, params->vs_prog_kernel);
vs.DispatchGRFStartRegisterForURBData =
vs_prog_data->base.base.dispatch_grf_start_reg;
@@ -795,11 +802,11 @@ blorp_emit_ps_config(struct blorp_batch *batch,
ps.DispatchGRFStartRegisterForConstantSetupData2 =
brw_wm_prog_data_dispatch_grf_start_reg(prog_data, ps, 2);
- ps.KernelStartPointer0 = params->wm_prog_kernel +
+ ps.KernelStartPointer0 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 0);
- ps.KernelStartPointer1 = params->wm_prog_kernel +
+ ps.KernelStartPointer1 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 1);
- ps.KernelStartPointer2 = params->wm_prog_kernel +
+ ps.KernelStartPointer2 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 2);
}
@@ -905,11 +912,11 @@ blorp_emit_ps_config(struct blorp_batch *batch,
ps.DispatchGRFStartRegisterForConstantSetupData2 =
brw_wm_prog_data_dispatch_grf_start_reg(prog_data, ps, 2);
- ps.KernelStartPointer0 = params->wm_prog_kernel +
+ ps.KernelStartPointer0 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 0);
- ps.KernelStartPointer1 = params->wm_prog_kernel +
+ ps.KernelStartPointer1 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 1);
- ps.KernelStartPointer2 = params->wm_prog_kernel +
+ ps.KernelStartPointer2 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, ps, 2);
ps.AttributeEnable = prog_data->num_varying_inputs > 0;
@@ -973,11 +980,11 @@ blorp_emit_ps_config(struct blorp_batch *batch,
wm.DispatchGRFStartRegisterForConstantSetupData2 =
brw_wm_prog_data_dispatch_grf_start_reg(prog_data, wm, 2);
- wm.KernelStartPointer0 = params->wm_prog_kernel +
+ wm.KernelStartPointer0 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 0);
- wm.KernelStartPointer1 = params->wm_prog_kernel +
+ wm.KernelStartPointer1 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 1);
- wm.KernelStartPointer2 = params->wm_prog_kernel +
+ wm.KernelStartPointer2 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 2);
wm.NumberofSFOutputAttributes = prog_data->num_varying_inputs;
diff --git a/src/intel/blorp/blorp_priv.h b/src/intel/blorp/blorp_priv.h
index a6aa2aa4151..b60505ea06a 100644
--- a/src/intel/blorp/blorp_priv.h
+++ b/src/intel/blorp/blorp_priv.h
@@ -203,11 +203,11 @@ struct blorp_params
unsigned num_samples;
unsigned num_draw_buffers;
unsigned num_layers;
- uint32_t vs_prog_kernel;
+ struct blorp_address vs_prog_kernel;
struct brw_vs_prog_data *vs_prog_data;
- uint32_t sf_prog_kernel;
+ struct blorp_address sf_prog_kernel;
struct brw_sf_prog_data *sf_prog_data;
- uint32_t wm_prog_kernel;
+ struct blorp_address wm_prog_kernel;
struct brw_wm_prog_data *wm_prog_data;
bool use_pre_baked_binding_table;
diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
index 478b8e7a3db..e6bcf4b0bc1 100644
--- a/src/intel/vulkan/anv_blorp.c
+++ b/src/intel/vulkan/anv_blorp.c
@@ -26,7 +26,8 @@
static bool
lookup_blorp_shader(struct blorp_context *blorp,
const void *key, uint32_t key_size,
- uint32_t *kernel_out, void *prog_data_out)
+ struct blorp_address *kernel_out,
+ void *prog_data_out)
{
struct anv_device *device = blorp->driver_ctx;
@@ -43,7 +44,7 @@ lookup_blorp_shader(struct blorp_context *blorp,
*/
anv_shader_bin_unref(device, bin);
- *kernel_out = bin->kernel.offset;
+ *kernel_out = (struct blorp_address) { .offset = bin->kernel.offset };
*(const struct brw_stage_prog_data **)prog_data_out = bin->prog_data;
return true;
@@ -55,7 +56,8 @@ upload_blorp_shader(struct blorp_context *blorp,
const void *kernel, uint32_t kernel_size,
const struct brw_stage_prog_data *prog_data,
uint32_t prog_data_size,
- uint32_t *kernel_out, void *prog_data_out)
+ struct blorp_address *kernel_out,
+ void *prog_data_out)
{
struct anv_device *device = blorp->driver_ctx;
@@ -81,7 +83,7 @@ upload_blorp_shader(struct blorp_context *blorp,
*/
anv_shader_bin_unref(device, bin);
- *kernel_out = bin->kernel.offset;
+ *kernel_out = (struct blorp_address) { .offset = bin->kernel.offset };
*(const struct brw_stage_prog_data **)prog_data_out = bin->prog_data;
return true;
diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
b/src/mesa/drivers/dri/i965/brw_blorp.c
index b286b231537..d021e2906fe 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp.c
+++ b/src/mesa/drivers/dri/i965/brw_blorp.c
@@ -45,11 +45,16 @@
static bool
brw_blorp_lookup_shader(struct blorp_context *blorp,
const void *key, uint32_t key_size,
- uint32_t *kernel_out, void *prog_data_out)
+ struct blorp_address *kernel_out,
+ void *prog_data_out)
{
struct brw_context *brw = blorp->driver_ctx;
- return brw_search_cache(&brw->cache, BRW_CACHE_BLORP_PROG, key, key_size,
- kernel_out, prog_data_out, true);
+ uint32_t offset;
+ bool found =
+ brw_search_cache(&brw->cache, BRW_CACHE_BLORP_PROG, key, key_size,
+ &offset, prog_data_out, true);
+ *kernel_out = (struct blorp_address) { .offset = offset };
+ return found;
}
static bool
@@ -58,12 +63,15 @@ brw_blorp_upload_shader(struct blorp_context *blorp,
const void *kernel, uint32_t kernel_size,
const struct brw_stage_prog_data *prog_data,
uint32_t prog_data_size,
- uint32_t *kernel_out, void *prog_data_out)
+ struct blorp_address *kernel_out,
+ void *prog_data_out)
{
struct brw_context *brw = blorp->driver_ctx;
+ uint32_t offset;
brw_upload_cache(&brw->cache, BRW_CACHE_BLORP_PROG, key, key_size,
kernel, kernel_size, prog_data, prog_data_size,
- kernel_out, prog_data_out);
+ &offset, prog_data_out);
+ *kernel_out = (struct blorp_address) { .offset = offset };
return true;
}
diff --git a/src/mesa/drivers/dri/i965/gen4_blorp_exec.h
b/src/mesa/drivers/dri/i965/gen4_blorp_exec.h
index 0edc518fa35..3b0f1ea0979 100644
--- a/src/mesa/drivers/dri/i965/gen4_blorp_exec.h
+++ b/src/mesa/drivers/dri/i965/gen4_blorp_exec.h
@@ -77,9 +77,9 @@ blorp_emit_sf_state(struct blorp_batch *batch,
blorp_emit_dynamic(batch, GENX(SF_STATE), sf, 64, &offset) {
#if GEN_GEN == 4
sf.KernelStartPointer =
- instruction_state_address(batch, params->sf_prog_kernel);
+ instruction_state_address(batch, KSP(batch,
params->sf_prog_kernel));
#else
- sf.KernelStartPointer = params->sf_prog_kernel;
+ sf.KernelStartPointer = KSP(batch, params->sf_prog_kernel);
#endif
sf.GRFRegisterCount = DIV_ROUND_UP(prog_data->total_grf, 16) - 1;
sf.VertexURBEntryReadLength = prog_data->urb_read_length;
@@ -136,14 +136,14 @@ blorp_emit_wm_state(struct blorp_batch *batch,
#if GEN_GEN == 4
wm.KernelStartPointer0 =
- instruction_state_address(batch, params->wm_prog_kernel);
+ instruction_state_address(batch, KSP(batch,
params->wm_prog_kernel));
wm.GRFRegisterCount0 = brw_wm_prog_data_reg_blocks(prog_data, wm, 0);
#else
- wm.KernelStartPointer0 = params->wm_prog_kernel +
+ wm.KernelStartPointer0 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 0);
- wm.KernelStartPointer1 = params->wm_prog_kernel +
+ wm.KernelStartPointer1 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 1);
- wm.KernelStartPointer2 = params->wm_prog_kernel +
+ wm.KernelStartPointer2 = KSP(batch, params->wm_prog_kernel) +
brw_wm_prog_data_prog_offset(prog_data, wm, 2);
wm.GRFRegisterCount0 = brw_wm_prog_data_reg_blocks(prog_data, wm, 0);
wm.GRFRegisterCount1 = brw_wm_prog_data_reg_blocks(prog_data, wm, 1);
--
2.19.1
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Jason Ekstrand

2018-12-07 22:38:28 UTC

Permalink

I somewhat recant my statements below. Even in a driver that puts the full
address in the offset field, having the BO pointer may still be useful for
the purpose of adding it to a residency list somewhere.

Post by Jason Ekstrand
I kind-of wonder if we want to allow for relocations and not surface state
base address in which case you'd want to do something "real" with offset.
Making this an address and then ignoring the buffer part entirely seems
like it promises a bit too much. Then again, I don't think it's really
hurting anything. Meh; this is probably fine.