[Mesa-dev] [PATCH] mesa: reference built-in uniforms into gl_uniform_storage
Jose Fonseca
jfonseca at vmware.com
Thu Jun 4 02:53:13 PDT 2015
On 31/05/15 08:10, Tapani wrote:
>
> I've read this a couple of times now and cannot spot any users of
> storage that would be making a wrong assumption, you've fixed these and
> I trust Jenkins was OK for i915?
>
> Everything is ok if you remove 'I think' and 'Hopefully' from commit
> message :)
Martin's fears were legitimate, as this change just caused +2729 tests
to fail with piglit. :)
So there must be some code that still needs to be updated somewhere
(maybe in the gallium specific paths). Question is then: Where's Wally?
Jose
>
> Reviewed-by: Tapani Pälli <tapani.palli at intel.com>
>
> On 05/26/2015 03:57 PM, Martin Peres wrote:
>> 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 at linux.intel.com>
>> ---
>> src/glsl/ir_uniform.h | 5 +++
>> src/glsl/link_uniform_initializers.cpp | 4 +-
>> src/glsl/link_uniforms.cpp | 55
>> +++++++++++-------------
>> 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, 77 insertions(+), 50 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..11ae06f 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++) {
>> - assert(uniforms[i].storage != NULL);
>> + for (unsigned i = 0; i < num_uniforms; i++) {
>> + assert(uniforms[i].storage != NULL || uniforms[i].builtin);
>> }
>> 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 99e0a38..9978380 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 42a0d78..5cd1813 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 56a2278..5d3501c 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 59a73a9..c6431f7 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..cab5083 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 this
>> + * function should already have returned NULL, 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,
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list