Attachment 115950 Details for Bug 32403 – mesa: reference built-in uniforms into gl_uniform_storage

[patch] mesa: reference built-in uniforms into gl_uniform_storage

0001-mesa-reference-built-in-uniforms-into-gl_uniform_sto.patch (text/plain), 18.33 KB, created by Martin Peres on 2015-05-21 13:36:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Peres

Created: 2015-05-21 13:36:22 UTC

Size: 18.33 KB

patch

obsolete

>From 602ec1e504a7cbe584bcdf27682660a0cc42ff09 Mon Sep 17 00:00:00 2001
>From: Martin Peres <martin.peres@linux.intel.com>
>Date: Thu, 21 May 2015 15:51:09 +0300
>Subject: [PATCH] mesa: reference built-in uniforms into gl_uniform_storage
>
>This change introduces a new field in gl_uniform_storage to
>explicitely say that a uniform is built-in. In the case where it is,
>no storage is defined to make it clear that it is read-only from the
>mesa side. I think I fixed all the places in the code that made use of
>the structure that I changed. Hopefully, any place making a wrong
>assumption and using the storage straight away will just crash.
>
>This patch seems to implement the path of least resistance towards
>listing built-in uniforms in GL_ACTIVE_UNIFORM (and other APIs).
>
>Signed-off-by: Martin Peres <martin.peres@linux.intel.com>
>---
> src/glsl/ir_uniform.h                            |  5 +++
> src/glsl/link_uniform_initializers.cpp           |  4 +-
> src/glsl/link_uniforms.cpp                       | 53 +++++++++++-------------
> src/glsl/linker.cpp                              |  6 +--
> src/glsl/standalone_scaffolding.cpp              |  2 +-
> src/glsl/tests/set_uniform_initializer_tests.cpp |  4 +-
> src/mesa/drivers/dri/i965/brw_fs.cpp             |  5 ++-
> src/mesa/drivers/dri/i965/brw_fs_nir.cpp         |  5 ++-
> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp   |  5 ++-
> src/mesa/main/mtypes.h                           |  2 +-
> src/mesa/main/shaderapi.c                        |  4 +-
> src/mesa/main/shaderobj.c                        |  4 +-
> src/mesa/main/uniform_query.cpp                  | 15 ++++++-
> src/mesa/program/ir_to_mesa.cpp                  |  9 +++-
> src/mesa/state_tracker/st_draw.c                 |  2 +-
> 15 files changed, 76 insertions(+), 49 deletions(-)
>
>diff --git a/src/glsl/ir_uniform.h b/src/glsl/ir_uniform.h
>index 21b5d05..e1b8014 100644
>--- a/src/glsl/ir_uniform.h
>+++ b/src/glsl/ir_uniform.h
>@@ -181,6 +181,11 @@ struct gl_uniform_storage {
>     * via the API.
>     */
>    bool hidden;
>+
>+   /**
>+    * This is a built-in uniform that should not be modified through any gl API.
>+    */
>+   bool builtin;
> };
> 
> #ifdef __cplusplus
>diff --git a/src/glsl/link_uniform_initializers.cpp b/src/glsl/link_uniform_initializers.cpp
>index 6907384..204acfa 100644
>--- a/src/glsl/link_uniform_initializers.cpp
>+++ b/src/glsl/link_uniform_initializers.cpp
>@@ -103,7 +103,7 @@ void
> set_sampler_binding(gl_shader_program *prog, const char *name, int binding)
> {
>    struct gl_uniform_storage *const storage =
>-      get_storage(prog->UniformStorage, prog->NumUserUniformStorage, name);
>+      get_storage(prog->UniformStorage, prog->NumUniformStorage, name);
> 
>    if (storage == NULL) {
>       assert(storage != NULL);
>@@ -193,7 +193,7 @@ set_uniform_initializer(void *mem_ctx, gl_shader_program *prog,
> 
>    struct gl_uniform_storage *const storage =
>       get_storage(prog->UniformStorage,
>-		  prog->NumUserUniformStorage,
>+                  prog->NumUniformStorage,
> 		  name);
>    if (storage == NULL) {
>       assert(storage != NULL);
>diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
>index 2c928e1..ef365b1 100644
>--- a/src/glsl/link_uniforms.cpp
>+++ b/src/glsl/link_uniforms.cpp
>@@ -589,12 +589,13 @@ private:
>       handle_samplers(base_type, &this->uniforms[id]);
>       handle_images(base_type, &this->uniforms[id]);
> 
>-      /* If there is already storage associated with this uniform, it means
>-       * that it was set while processing an earlier shader stage.  For
>-       * example, we may be processing the uniform in the fragment shader, but
>-       * the uniform was already processed in the vertex shader.
>+      /* If there is already storage associated with this uniform or if the
>+       * uniform is set as builtin, it means that it was set while processing
>+       * an earlier shader stage.  For example, we may be processing the
>+       * uniform in the fragment shader, but the uniform was already processed
>+       * in the vertex shader.
>        */
>-      if (this->uniforms[id].storage != NULL) {
>+      if (this->uniforms[id].storage != NULL || this->uniforms[id].builtin) {
>          return;
>       }
> 
>@@ -619,10 +620,15 @@ private:
>       this->uniforms[id].initialized = 0;
>       this->uniforms[id].num_driver_storage = 0;
>       this->uniforms[id].driver_storage = NULL;
>-      this->uniforms[id].storage = this->values;
>       this->uniforms[id].atomic_buffer_index = -1;
>       this->uniforms[id].hidden =
>          current_var->data.how_declared == ir_var_hidden;
>+      this->uniforms[id].builtin = is_gl_identifier(name);
>+
>+      /* Do not assign storage if the uniform is builtin */
>+      if (!this->uniforms[id].builtin)
>+         this->uniforms[id].storage = this->values;
>+
>       if (this->ubo_block_index != -1) {
> 	 this->uniforms[id].block_index = this->ubo_block_index;
> 
>@@ -894,7 +900,7 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
> {
>    ralloc_free(prog->UniformStorage);
>    prog->UniformStorage = NULL;
>-   prog->NumUserUniformStorage = 0;
>+   prog->NumUniformStorage = 0;
> 
>    if (prog->UniformHash != NULL) {
>       prog->UniformHash->clear();
>@@ -940,14 +946,6 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
> 	 if ((var == NULL) || (var->data.mode != ir_var_uniform))
> 	    continue;
> 
>-	 /* FINISHME: Update code to process built-in uniforms!
>-	  */
>-	 if (is_gl_identifier(var->name)) {
>-	    uniform_size.num_shader_uniform_components +=
>-	       var->type->component_slots();
>-	    continue;
>-	 }
>-
> 	 uniform_size.process(var);
>       }
> 
>@@ -962,16 +960,16 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
>       }
>    }
> 
>-   const unsigned num_user_uniforms = uniform_size.num_active_uniforms;
>+   const unsigned num_uniforms = uniform_size.num_active_uniforms;
>    const unsigned num_data_slots = uniform_size.num_values;
> 
>    /* On the outside chance that there were no uniforms, bail out.
>     */
>-   if (num_user_uniforms == 0)
>+   if (num_uniforms == 0)
>       return;
> 
>    struct gl_uniform_storage *uniforms =
>-      rzalloc_array(prog, struct gl_uniform_storage, num_user_uniforms);
>+      rzalloc_array(prog, struct gl_uniform_storage, num_uniforms);
>    union gl_constant_value *data =
>       rzalloc_array(uniforms, union gl_constant_value, num_data_slots);
> #ifndef NDEBUG
>@@ -992,11 +990,6 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
> 	 if ((var == NULL) || (var->data.mode != ir_var_uniform))
> 	    continue;
> 
>-	 /* FINISHME: Update code to process built-in uniforms!
>-	  */
>-	 if (is_gl_identifier(var->name))
>-	    continue;
>-
> 	 parcel.set_and_process(prog, var);
>       }
> 
>@@ -1009,10 +1002,10 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
>    }
> 
>    const unsigned hidden_uniforms =
>-      move_hidden_uniforms_to_end(prog, uniforms, num_user_uniforms);
>+      move_hidden_uniforms_to_end(prog, uniforms, num_uniforms);
> 
>    /* Reserve all the explicit locations of the active uniforms. */
>-   for (unsigned i = 0; i < num_user_uniforms; i++) {
>+   for (unsigned i = 0; i < num_uniforms; i++) {
>       if (uniforms[i].remap_location != UNMAPPED_UNIFORM_LOC) {
>          /* How many new entries for this uniform? */
>          const unsigned entries = MAX2(1, uniforms[i].array_elements);
>@@ -1028,7 +1021,11 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
>    }
> 
>    /* Reserve locations for rest of the uniforms. */
>-   for (unsigned i = 0; i < num_user_uniforms; i++) {
>+   for (unsigned i = 0; i < num_uniforms; i++) {
>+
>+      /* Built-in uniforms should not get any location. */
>+      if (uniforms[i].builtin)
>+         continue;
> 
>       /* Explicit ones have been set already. */
>       if (uniforms[i].remap_location != UNMAPPED_UNIFORM_LOC)
>@@ -1055,14 +1052,14 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
>    }
> 
> #ifndef NDEBUG
>-   for (unsigned i = 0; i < num_user_uniforms; i++) {
>+   for (unsigned i = 0; i < num_uniforms; i++) {
>       assert(uniforms[i].storage != NULL);
>    }
> 
>    assert(parcel.values == data_end);
> #endif
> 
>-   prog->NumUserUniformStorage = num_user_uniforms;
>+   prog->NumUniformStorage = num_uniforms;
>    prog->NumHiddenUniforms = hidden_uniforms;
>    prog->UniformStorage = uniforms;
> 
>diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>index ecdc025..8b92a39 100644
>--- a/src/glsl/linker.cpp
>+++ b/src/glsl/linker.cpp
>@@ -1400,8 +1400,8 @@ link_fs_input_layout_qualifiers(struct gl_shader_program *prog,
>                       "layout qualifiers for gl_FragCoord\n");
>       }
> 
>-      /* Update the linked shader state. Â Note that uses_gl_fragcoord should
>-       * accumulate the results. Â The other values should replace. Â If there
>+      /* Update the linked shader state.  Note that uses_gl_fragcoord should
>+       * accumulate the results.  The other values should replace.  If there
>        * are multiple redeclarations, all the fields except uses_gl_fragcoord
>        * are already known to be the same.
>        */
>@@ -2693,7 +2693,7 @@ build_program_resource_list(struct gl_context *ctx,
>    }
> 
>    /* Add uniforms from uniform storage. */
>-   for (unsigned i = 0; i < shProg->NumUserUniformStorage; i++) {
>+   for (unsigned i = 0; i < shProg->NumUniformStorage; i++) {
>       /* Do not add uniforms internally used by Mesa. */
>       if (shProg->UniformStorage[i].hidden)
>          continue;
>diff --git a/src/glsl/standalone_scaffolding.cpp b/src/glsl/standalone_scaffolding.cpp
>index a109c4e..00db61e 100644
>--- a/src/glsl/standalone_scaffolding.cpp
>+++ b/src/glsl/standalone_scaffolding.cpp
>@@ -89,7 +89,7 @@ _mesa_clear_shader_program_data(struct gl_shader_program *shProg)
> {
>    unsigned i;
> 
>-   shProg->NumUserUniformStorage = 0;
>+   shProg->NumUniformStorage = 0;
>    shProg->UniformStorage = NULL;
>    shProg->NumUniformRemapTable = 0;
>    shProg->UniformRemapTable = NULL;
>diff --git a/src/glsl/tests/set_uniform_initializer_tests.cpp b/src/glsl/tests/set_uniform_initializer_tests.cpp
>index d3fdeb3..91227d9 100644
>--- a/src/glsl/tests/set_uniform_initializer_tests.cpp
>+++ b/src/glsl/tests/set_uniform_initializer_tests.cpp
>@@ -110,7 +110,7 @@ establish_uniform_storage(struct gl_shader_program *prog, unsigned num_storage,
> 
>    prog->UniformStorage = rzalloc_array(prog, struct gl_uniform_storage,
> 					num_storage);
>-   prog->NumUserUniformStorage = num_storage;
>+   prog->NumUniformStorage = num_storage;
> 
>    prog->UniformStorage[index_to_set].name = (char *) name;
>    prog->UniformStorage[index_to_set].type = type;
>@@ -155,7 +155,7 @@ establish_uniform_storage(struct gl_shader_program *prog, unsigned num_storage,
> static void
> verify_initialization(struct gl_shader_program *prog, unsigned actual_index)
> {
>-   for (unsigned i = 0; i < prog->NumUserUniformStorage; i++) {
>+   for (unsigned i = 0; i < prog->NumUniformStorage; i++) {
>       if (i == actual_index) {
> 	 EXPECT_TRUE(prog->UniformStorage[actual_index].initialized);
>       } else {
>diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>index 9b3186b..37c56e9 100644
>--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>@@ -1181,9 +1181,12 @@ fs_visitor::setup_uniform_values(ir_variable *ir)
>     * with our name, or the prefix of a component that starts with our name.
>     */
>    unsigned params_before = uniforms;
>-   for (unsigned u = 0; u < shader_prog->NumUserUniformStorage; u++) {
>+   for (unsigned u = 0; u < shader_prog->NumUniformStorage; u++) {
>       struct gl_uniform_storage *storage = &shader_prog->UniformStorage[u];
> 
>+      if (storage->builtin)
>+         continue;
>+
>       if (strncmp(ir->name, storage->name, namelen) != 0 ||
>           (storage->name[namelen] != 0 &&
>            storage->name[namelen] != '.' &&
>diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>index 5dd8363..9c00b8a 100644
>--- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>+++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>@@ -218,9 +218,12 @@ fs_visitor::nir_setup_uniform(nir_variable *var)
>       * our name.
>       */
>    unsigned index = var->data.driver_location;
>-   for (unsigned u = 0; u < shader_prog->NumUserUniformStorage; u++) {
>+   for (unsigned u = 0; u < shader_prog->NumUniformStorage; u++) {
>       struct gl_uniform_storage *storage = &shader_prog->UniformStorage[u];
> 
>+      if (storage->builtin)
>+              continue;
>+
>       if (strncmp(var->name, storage->name, namelen) != 0 ||
>          (storage->name[namelen] != 0 &&
>          storage->name[namelen] != '.' &&
>diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>index 5a60fe4..6d118d2 100644
>--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>@@ -683,9 +683,12 @@ vec4_visitor::setup_uniform_values(ir_variable *ir)
>     * order we'd walk the type, so walk the list of storage and find anything
>     * with our name, or the prefix of a component that starts with our name.
>     */
>-   for (unsigned u = 0; u < shader_prog->NumUserUniformStorage; u++) {
>+   for (unsigned u = 0; u < shader_prog->NumUniformStorage; u++) {
>       struct gl_uniform_storage *storage = &shader_prog->UniformStorage[u];
> 
>+      if (storage->builtin)
>+         continue;
>+
>       if (strncmp(ir->name, storage->name, namelen) != 0 ||
>           (storage->name[namelen] != 0 &&
>            storage->name[namelen] != '.' &&
>diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>index 8342517..3d791b3 100644
>--- a/src/mesa/main/mtypes.h
>+++ b/src/mesa/main/mtypes.h
>@@ -2728,7 +2728,7 @@ struct gl_shader_program
>    } Comp;
> 
>    /* post-link info: */
>-   unsigned NumUserUniformStorage;
>+   unsigned NumUniformStorage;
>    unsigned NumHiddenUniforms;
>    struct gl_uniform_storage *UniformStorage;
> 
>diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>index a04b287..6d8e6e2 100644
>--- a/src/mesa/main/shaderapi.c
>+++ b/src/mesa/main/shaderapi.c
>@@ -569,13 +569,13 @@ get_programiv(struct gl_context *ctx, GLuint program, GLenum pname,
>       *params = _mesa_longest_attribute_name_length(shProg);
>       return;
>    case GL_ACTIVE_UNIFORMS:
>-      *params = shProg->NumUserUniformStorage - shProg->NumHiddenUniforms;
>+      *params = shProg->NumUniformStorage - shProg->NumHiddenUniforms;
>       return;
>    case GL_ACTIVE_UNIFORM_MAX_LENGTH: {
>       unsigned i;
>       GLint max_len = 0;
>       const unsigned num_uniforms =
>-         shProg->NumUserUniformStorage - shProg->NumHiddenUniforms;
>+         shProg->NumUniformStorage - shProg->NumHiddenUniforms;
> 
>       for (i = 0; i < num_uniforms; i++) {
> 	 /* Add one for the terminating NUL character for a non-array, and
>diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
>index e428960..110a18e 100644
>--- a/src/mesa/main/shaderobj.c
>+++ b/src/mesa/main/shaderobj.c
>@@ -282,10 +282,10 @@ _mesa_clear_shader_program_data(struct gl_shader_program *shProg)
>    unsigned i;
> 
>    if (shProg->UniformStorage) {
>-      for (i = 0; i < shProg->NumUserUniformStorage; ++i)
>+      for (i = 0; i < shProg->NumUniformStorage; ++i)
>          _mesa_uniform_detach_all_driver_storage(&shProg->UniformStorage[i]);
>       ralloc_free(shProg->UniformStorage);
>-      shProg->NumUserUniformStorage = 0;
>+      shProg->NumUniformStorage = 0;
>       shProg->UniformStorage = NULL;
>    }
> 
>diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
>index 728bd1b..c5c830b 100644
>--- a/src/mesa/main/uniform_query.cpp
>+++ b/src/mesa/main/uniform_query.cpp
>@@ -237,6 +237,13 @@ validate_uniform_parameters(struct gl_context *ctx,
> 
>    struct gl_uniform_storage *const uni = shProg->UniformRemapTable[location];
> 
>+   /* Even though no location is assigned to a built-in uniform and it should
>+    * already have aborted, this test makes it explicit that we are not allowing
>+    * to update the value of a built-in.
>+    */
>+   if (uni->builtin)
>+      return NULL;
>+
>    if (uni->array_elements == 0) {
>       if (count > 1) {
>          _mesa_error(ctx, GL_INVALID_OPERATION,
>@@ -1028,6 +1035,10 @@ _mesa_get_uniform_location(struct gl_shader_program *shProg,
>    if (!found)
>       return GL_INVALID_INDEX;
> 
>+   /* If the uniform is built-in, fail. */
>+   if (shProg->UniformStorage[location].builtin)
>+      return GL_INVALID_INDEX;
>+
>    /* If the uniform is an array, fail if the index is out of bounds.
>     * (A negative index is caught above.)  This also fails if the uniform
>     * is not an array, but the user is trying to index it, because
>@@ -1047,7 +1058,7 @@ _mesa_sampler_uniforms_are_valid(const struct gl_shader_program *shProg,
> 				 char *errMsg, size_t errMsgLength)
> {
>    /* Shader does not have samplers. */
>-   if (shProg->NumUserUniformStorage == 0)
>+   if (shProg->NumUniformStorage == 0)
>       return true;
> 
>    if (!shProg->SamplersValidated) {
>@@ -1087,7 +1098,7 @@ _mesa_sampler_uniforms_pipeline_are_valid(struct gl_pipeline_object *pipeline)
>       if (!shProg[idx])
>          continue;
> 
>-      for (unsigned i = 0; i < shProg[idx]->NumUserUniformStorage; i++) {
>+      for (unsigned i = 0; i < shProg[idx]->NumUniformStorage; i++) {
>          const struct gl_uniform_storage *const storage =
>             &shProg[idx]->UniformStorage[i];
>          const glsl_type *const t = (storage->type->is_array())
>diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
>index 3dcb537..4a8a8ea 100644
>--- a/src/mesa/program/ir_to_mesa.cpp
>+++ b/src/mesa/program/ir_to_mesa.cpp
>@@ -2406,9 +2406,14 @@ _mesa_associate_uniform_storage(struct gl_context *ctx,
>       if (!found)
> 	 continue;
> 
>+      struct gl_uniform_storage *storage =
>+         &shader_program->UniformStorage[location];
>+
>+      /* Do not associate any uniform storage to built-in uniforms */
>+      if (!storage->builtin)
>+         continue;
>+
>       if (location != last_location) {
>-	 struct gl_uniform_storage *storage =
>-	    &shader_program->UniformStorage[location];
> 	 enum gl_uniform_driver_format format = uniform_native;
> 
> 	 unsigned columns = 0;
>diff --git a/src/mesa/state_tracker/st_draw.c b/src/mesa/state_tracker/st_draw.c
>index 488f6ea..8b43582 100644
>--- a/src/mesa/state_tracker/st_draw.c
>+++ b/src/mesa/state_tracker/st_draw.c
>@@ -141,7 +141,7 @@ check_uniforms(struct gl_context *ctx)
>       if (shProg[j] == NULL || !shProg[j]->LinkStatus)
> 	 continue;
> 
>-      for (i = 0; i < shProg[j]->NumUserUniformStorage; i++) {
>+      for (i = 0; i < shProg[j]->NumUniformStorage; i++) {
>          const struct gl_uniform_storage *u = &shProg[j]->UniformStorage[i];
>          if (!u->initialized) {
>             _mesa_warning(ctx,
>-- 
>2.4.1
>

Actions: View | Splinter Review

Attachments on bug 32403: 41133 | 115636 | 115637 | 115950

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.