[Mesa-dev] [PATCH 5/7] linker: Set block bindings based on UniformBlocks rather than UniformStorage

[Ian Romanick]

On 04/09/2014 08:09 AM, Kenneth Graunke wrote:
> On 04/04/2014 02:01 PM, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> For blocks, gl_shader_program::UniformStorage isn't very useful.  The
>> names stored there are the names of the elements of the block, so
>> finding blocks with an instance name is hard.  There is also only one
>> entry in ::UniformStorage for each element of a block array, and that is
>> a deal breaker.
>>
>> Using ::UniformBlocks is what _mesa_GetUniformBlockIndex does.  I
>> contemplated sharing code between set_block_binding and
>> _mesa_GetUniformBlockIndex, but building the stand-alone compiler and
>> the unit tests make this hard.  I plan to return to this effort shortly.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76323
>> Cc: "10.1" <mesa-stable at lists.freedesktop.org>
>> Cc: github at socker.lepus.uberspace.de
>> ---
>>  src/glsl/link_uniform_initializers.cpp | 32 +++++++++++++++++++++-----------
>>  1 file changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/glsl/link_uniform_initializers.cpp b/src/glsl/link_uniform_initializers.cpp
>> index c633850..491eb69 100644
>> --- a/src/glsl/link_uniform_initializers.cpp
>> +++ b/src/glsl/link_uniform_initializers.cpp
>> @@ -46,6 +46,18 @@ get_storage(gl_uniform_storage *storage, unsigned num_storage,
>>     return NULL;
>>  }
>>  
>> +static unsigned
>> +get_uniform_block_index(const gl_shader_program *shProg,
>> +                        const char *uniformBlockName)
>> +{
>> +   for (unsigned i = 0; i < shProg->NumUniformBlocks; i++) {
>> +      if (!strcmp(shProg->UniformBlocks[i].Name, uniformBlockName))
>> +	 return i;
>> +   }
>> +
>> +   return GL_INVALID_INDEX;
>> +}
>> +
>>  void
>>  copy_constant_to_storage(union gl_constant_value *storage,
>>  			 const ir_constant *val,
>> @@ -123,29 +135,24 @@ set_sampler_binding(gl_shader_program *prog, const char *name, int binding)
>>  }
>>  
>>  void
>> -set_block_binding(gl_shader_program *prog, const char *name, int binding)
>> +set_block_binding(gl_shader_program *prog, const char *block_name, int binding)
>>  {
>> -   struct gl_uniform_storage *const storage =
>> -      get_storage(prog->UniformStorage, prog->NumUserUniformStorage, name);
>> +   const unsigned block_index = get_uniform_block_index(prog, block_name);
>>  
>> -   if (storage == NULL) {
>> -      assert(storage != NULL);
>> +   if (block_index == GL_INVALID_INDEX) {
>> +      assert(block_index != GL_INVALID_INDEX);
>>        return;
>>     }
>>  
>> -   if (storage->block_index != -1) {
>>        /* This is a field of a UBO.  val is the binding index. */
>>        for (int i = 0; i < MESA_SHADER_STAGES; i++) {
>> -         int stage_index = prog->UniformBlockStageIndex[i][storage->block_index];
>> +         int stage_index = prog->UniformBlockStageIndex[i][block_index];
>>  
>>           if (stage_index != -1) {
>>              struct gl_shader *sh = prog->_LinkedShaders[i];
>>              sh->UniformBlocks[stage_index].Binding = binding;
>>           }
>>        }
>> -   }
>> -
>> -   storage->initialized = true;
> 
> Why is it not necessary to set storage->initialized = true?  It goes
> away here and never seems to come back.

The meaning of gl_uniform_storage::initialized is: Has this uniform ever
been given a value?  Setting the binding of a block doesn't give
anything a value, but it does for samplers.  I hadn't fully worked
through the logic when I wrote the patch, and I forgot to come back to
it to document it.

>>  }
>>  
>>  void
>> @@ -253,7 +260,10 @@ link_set_uniform_initializers(struct gl_shader_program *prog)
>>                  || (type->is_array() && type->fields.array->is_sampler())) {
>>                 linker::set_sampler_binding(prog, var->name, var->data.binding);
>>              } else if (var->is_in_uniform_block()) {
>> -               linker::set_block_binding(prog, var->name, var->data.binding);
>> +               const glsl_type *const iface_type = var->get_interface_type();
>> +
>> +               linker::set_block_binding(prog, iface_type->name,
>> +                                         var->data.binding);
>>              } else {
>>                 assert(!"Explicit binding not on a sampler or UBO.");
>>              }
>>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140410/25cc7449/attachment-0001.sig>