[Mesa-dev] [PATCH] mesa/glsl: introduce a remap table for uniform locations

[Eric Anholt]

Tapani Pälli <tapani.palli at intel.com> writes:

> Patch adds a remap table for uniforms that is used to provide a mapping
> from application specified uniform location to actual location in the
> UniformStorage. Existing UniformLocationBaseScale usage is removed as
> table can be used to set sequential values for array uniform elements.
>
> This mapping helps to implement GL_ARB_explicit_uniform_location so that
> uniforms locations can be reorganized and handled in a more easy manner.
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>

> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index d05649c..3133ea5 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2702,6 +2702,13 @@ struct gl_shader_program
>     struct gl_uniform_storage *UniformStorage;
>  
>     /**
> +    * This table is used to generate locations of uniforms
> +    * (returned by \c glGetUniformLocation).
> +    */
> +   unsigned NumUniformRemapTable;
> +   struct gl_uniform_storage **UniformRemapTable;

I think the way I'd write this comment is "Mapping from GL uniform
locations returned by glUniformLocation to UniformStorage entries.
Arrays will have multiple contiguous slots in the UniformRemapTable, all
pointing to the same UniformStorage entry."

("Remap" is vague, and I ended up reading the code to figure out what
this was mapping from.)

> diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
> index 8cc5da7..700a333 100644
> --- a/src/mesa/main/uniform_query.cpp
> +++ b/src/mesa/main/uniform_query.cpp
> @@ -246,6 +246,13 @@ validate_uniform_parameters(struct gl_context *ctx,
>        return false;
>     }
>  
> +   /* Check that the given location is in bounds of uniform remap table. */
> +   if (location >= (GLint) shProg->NumUniformRemapTable) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)",
> +		  caller, location);
> +      return false;
> +   }
> +
>     _mesa_uniform_split_location_offset(shProg, location, loc, array_index);
>  
>     if (*loc >= shProg->NumUserUniformStorage) {

Couldn't the new test just replace the *loc >=
shProg->NumUserUniformStorage test?  We're always going to have a loc
pointing to a valid value, since you looked loc up from the remaptable.

> diff --git a/src/mesa/main/uniforms.h b/src/mesa/main/uniforms.h
> index bd50fd9..d493d2d 100644
> --- a/src/mesa/main/uniforms.h
> +++ b/src/mesa/main/uniforms.h
> @@ -340,15 +340,14 @@ struct gl_builtin_uniform_desc {
>   * element.  We could insert dummy entries in the list for each array
>   * element after [0] but that causes complications elsewhere.
>   *
> - * We solve this problem by encoding two values in the location that's
> - * returned by glGetUniformLocation():
> - *  a) index into gl_uniform_list::Uniforms[] for the uniform
> - *  b) an array/field offset (0 for simple types)
> + * We solve this problem by creating multiple entries for uniform arrays
> + * in the UniformRemapTable so that their elements get sequential locations.
> + *
> + * Utility functions below offer functionality to split UniformRemapTable
> + * location in to location of the uniform in UniformStorage + offset to the
> + * array element (0 if not an array) and also merge it back again as the
> + * UniformRemapTable location.
>   *
> - * These two values are encoded in the high and low halves of a GLint.
> - * By putting the uniform number in the high part and the offset in the
> - * low part, we can support the unofficial ability to index into arrays
> - * by adding offsets to the location value.
>   */
>  /*@{*/
>  /**
> @@ -358,9 +357,8 @@ static inline GLint
>  _mesa_uniform_merge_location_offset(const struct gl_shader_program *prog,
>                                      unsigned base_location, unsigned offset)
>  {
> -   assert(prog->UniformLocationBaseScale >= 1);
> -   assert(offset < prog->UniformLocationBaseScale);
> -   return (base_location * prog->UniformLocationBaseScale) + offset;
> +   /* location in remap table + array element offset */
> +   return prog->UniformStorage[base_location].remap_location + offset;
>  }
>  
>  /**
> @@ -371,8 +369,15 @@ _mesa_uniform_split_location_offset(const struct gl_shader_program *prog,
>                                      GLint location, unsigned *base_location,
>  				    unsigned *offset)
>  {
> -   *offset = location % prog->UniformLocationBaseScale;
> -   *base_location = location / prog->UniformLocationBaseScale;
> +   *base_location = prog->UniformRemapTable[location] - prog->UniformStorage;
> +   *offset = location - prog->UniformRemapTable[location]->remap_location;
> +
> +   /**
> +    * gl_uniform_storage in in UniformStorage with the calculated base_location

Too many "in"s.

> +    * must match with the entry in remap table
> +    */
> +   assert(&prog->UniformStorage[*base_location] ==
> +          prog->UniformRemapTable[location]);
>  }

Other than these little notes,

Reviewed-by: Eric Anholt <eric at anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140307/9f9ea234/attachment-0001.pgp>