[Mesa-dev] [PATCH 05/20] i965: Store gather table information in the program data
Francisco Jerez
currojerez at riseup.net
Fri Oct 23 10:02:43 PDT 2015
Abdiel Janulgue <abdiel.janulgue at linux.intel.com> writes:
> The resource streamer is able to gather and pack sparsely-located
> constant data from any buffer object by a referring to a gather table
> This patch adds support for keeping track of these constant data
> fetches into a gather table.
>
> The gather table is generated from two sources. Ordinary uniform fetches
> are stored first. These are then combined with a separate table containing
> UBO entries. The separate entry for UBOs is needed to make it easier to
> generate the gather mask when combining and packing the constant data.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> ---
> src/mesa/drivers/dri/i965/brw_context.h | 11 +++++++++++
> src/mesa/drivers/dri/i965/brw_gs.c | 5 +++++
> src/mesa/drivers/dri/i965/brw_program.c | 5 +++++
> src/mesa/drivers/dri/i965/brw_shader.cpp | 5 ++++-
> src/mesa/drivers/dri/i965/brw_shader.h | 11 +++++++++++
> src/mesa/drivers/dri/i965/brw_vs.c | 6 ++++++
> src/mesa/drivers/dri/i965/brw_wm.c | 5 +++++
> 7 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index 33c49b7..de0db5a 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -364,9 +364,12 @@ struct brw_stage_prog_data {
> GLuint nr_params; /**< number of float params/constants */
> GLuint nr_pull_params;
> unsigned nr_image_params;
> + unsigned nr_ubo_params;
You already used this field in an earlier patch of this series, please
include the declaration in the other patch to avoid breaking the build
and make bisecting easier.
> + unsigned nr_gather_table;
'nr_gather_table' sounds like you may have more than one gather table ;).
How about 'nr_gather_constants'? (And maybe rename 'gather_table' to
'gather_constants' if you want to keep the names of both fields
consistent.)
>
> unsigned curb_read_length;
> unsigned total_scratch;
> + unsigned max_ubo_const_block;
Because gen7_submit_gather_table() is already O(n) on the number of
gather table entries and you could probably calculate it easily there, I
doubt you gain much from having this value precomputed in
brw_stage_prog_data, but if you insist I guess some other name like
"max_gather_buffer_index" would be more descriptive.
>
> /**
> * Register where the thread expects to find input data from the URB
> @@ -385,6 +388,14 @@ struct brw_stage_prog_data {
> const gl_constant_value **param;
> const gl_constant_value **pull_param;
>
> + /** Combined gather table containing uniform and UBO entries */
> + struct {
> + int reg;
What's reg supposed to do?
> + unsigned channel_mask;
> + unsigned const_block;
> + unsigned const_offset;
> + } *gather_table;
> +
> /**
> * Image metadata passed to the shader as uniforms. This is deliberately
> * ignored by brw_stage_prog_data_compare() because its contents don't have
> diff --git a/src/mesa/drivers/dri/i965/brw_gs.c b/src/mesa/drivers/dri/i965/brw_gs.c
> index 16ea684..17e87b8 100644
> --- a/src/mesa/drivers/dri/i965/brw_gs.c
> +++ b/src/mesa/drivers/dri/i965/brw_gs.c
> @@ -72,6 +72,11 @@ brw_codegen_gs_prog(struct brw_context *brw,
> rzalloc_array(NULL, struct brw_image_param, gs->NumImages);
> c.prog_data.base.base.nr_params = param_count;
> c.prog_data.base.base.nr_image_params = gs->NumImages;
> + c.prog_data.base.base.nr_gather_table = 0;
> + c.prog_data.base.base.gather_table =
> + rzalloc_size(NULL, sizeof(*c.prog_data.base.base.gather_table) *
> + (c.prog_data.base.base.nr_params +
> + c.prog_data.base.base.nr_ubo_params));
>
If you named the struct that represents a gather table entry you could
use rzalloc_array here.
> if (brw->gen >= 7) {
> if (gp->program.OutputType == GL_POINTS) {
> diff --git a/src/mesa/drivers/dri/i965/brw_program.c b/src/mesa/drivers/dri/i965/brw_program.c
> index 1ac0ed2..aa805be 100644
> --- a/src/mesa/drivers/dri/i965/brw_program.c
> +++ b/src/mesa/drivers/dri/i965/brw_program.c
> @@ -558,6 +558,10 @@ brw_stage_prog_data_compare(const struct brw_stage_prog_data *a,
> if (memcmp(a->pull_param, b->pull_param, a->nr_pull_params * sizeof(void *)))
> return false;
>
> + if (memcmp(a->gather_table, b->gather_table,
> + a->nr_gather_table * sizeof(*a->gather_table)))
> + return false;
> +
Because the gather table is a function of the program code and not the
other way around this memcmp() shouldn't be necessary.
> return true;
> }
>
> @@ -568,6 +572,7 @@ brw_stage_prog_data_free(const void *p)
>
> ralloc_free(prog_data->param);
> ralloc_free(prog_data->pull_param);
> + ralloc_free(prog_data->gather_table);
> }
>
> void
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
> index de1a7fe..9d45cfe 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> @@ -917,7 +917,10 @@ backend_shader::backend_shader(const struct brw_compiler *compiler,
> stage_prog_data(stage_prog_data),
> mem_ctx(mem_ctx),
> cfg(NULL),
> - stage(stage)
> + stage(stage),
> + use_gather_constants(false),
Please add an argument to the constructor in order to pass the value of
this compiler option -- Seems kind of dodgy to initialize the variable
to a value you don't intend to use only to change it later on after
construction. Another alternative would be to move this option into
brw_compiler, although I guess that would mean that the setting would
have to be global for all stages instead of having per-stage enables --
A global enable might be good enough though AFAICT.
> + nr_ubo_gather_table(0),
> + ubo_gather_table(NULL)
> {
> debug_enabled = INTEL_DEBUG & intel_debug_flag_for_shader_stage(stage);
> stage_name = _mesa_shader_stage_to_string(stage);
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
> index ccccf4d..f0afce5 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.h
> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
> @@ -275,6 +275,17 @@ public:
> unsigned n) = 0;
> void setup_image_uniform_values(unsigned param_offset,
> const gl_uniform_storage *storage);
> + bool use_gather_constants;
> + unsigned nr_ubo_gather_table;
> +
> + /** Gather table for UBO entries only */
> + struct gather_table {
> + int reg;
> + unsigned channel_mask;
> + unsigned const_block;
> + unsigned const_offset;
> + };
Any reason you don't declare this struct at the top level and re-use the
same type for brw_stage_prog_data::gather_table too? I suggest you
rename it to 'brw_gather_constant_entry' or something similar so it's
clear that this is just one entry and not the whole gather table.
> + gather_table *ubo_gather_table;
> };
>
> uint32_t brw_texture_offset(int *offsets, unsigned num_components);
> diff --git a/src/mesa/drivers/dri/i965/brw_vs.c b/src/mesa/drivers/dri/i965/brw_vs.c
> index 4e0d34f..8501796 100644
> --- a/src/mesa/drivers/dri/i965/brw_vs.c
> +++ b/src/mesa/drivers/dri/i965/brw_vs.c
> @@ -141,6 +141,12 @@ brw_codegen_vs_prog(struct brw_context *brw,
> stage_prog_data->nr_image_params);
> stage_prog_data->nr_params = param_count;
>
> + stage_prog_data->nr_gather_table = 0;
> + stage_prog_data->gather_table =
> + rzalloc_size(NULL, sizeof(*stage_prog_data->gather_table) *
> + (stage_prog_data->nr_params +
> + stage_prog_data->nr_ubo_params));
> +
Use rzalloc_array().
> GLbitfield64 outputs_written = vp->program.Base.OutputsWritten;
> prog_data.inputs_read = vp->program.Base.InputsRead;
>
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c
> index 6ee9284..204baa6 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> @@ -208,6 +208,11 @@ brw_codegen_wm_prog(struct brw_context *brw,
> prog_data.base.nr_image_params);
> prog_data.base.nr_params = param_count;
>
> + prog_data.base.nr_gather_table = 0;
> + prog_data.base.gather_table =
> + rzalloc_size(NULL, sizeof(*prog_data.base.gather_table) *
> + (prog_data.base.nr_params + prog_data.base.nr_ubo_params));
> +
Same here, and I guess you're missing the same change in
brw_codegen_cs_prog().
> prog_data.barycentric_interp_modes =
> brw_compute_barycentric_interp_modes(brw, key->flat_shade,
> key->persample_shading,
> --
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151023/77318a76/attachment.sig>
More information about the mesa-dev
mailing list