[Mesa-dev] [PATCH] mesa: fix program resource queries for builtin variables

[Martin Peres]

On 02/06/15 13:30, Tapani Pälli wrote:
> Patch fixes special cases with gl_VertexID and sets all builtin
> variables locations as '-1' as specified by the extension spec.
>
> Fixes ES 3.1 conformance test failure:
> 	ES31-CTS.program_interface_query.input-built-in
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
>   src/mesa/main/shader_query.cpp | 22 +++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index 3445f89..7d7cb73 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -479,12 +479,19 @@ _mesa_GetFragDataLocation(GLuint program, const GLchar *name)
>   const char*
>   _mesa_program_resource_name(struct gl_program_resource *res)
>   {
> +   const ir_variable *var;
>      switch (res->Type) {
>      case GL_UNIFORM_BLOCK:
>         return RESOURCE_UBO(res)->Name;
>      case GL_TRANSFORM_FEEDBACK_VARYING:
>         return RESOURCE_XFB(res)->Name;
>      case GL_PROGRAM_INPUT:
> +      var = RESOURCE_VAR(res);
> +      if (var->data.mode == ir_var_system_value &&
> +          var->data.location == SYSTEM_VALUE_VERTEX_ID_ZERO_BASE) {
> +         return "gl_VertexID";
> +      }

This would warrant a comment like the following:

/* Special case gl_VertexIDMESA -> gl_VertexID. */


> +   /* fallthrough */
>      case GL_PROGRAM_OUTPUT:
>         return RESOURCE_VAR(res)->name;
>      case GL_UNIFORM:
> @@ -539,6 +546,15 @@ struct gl_program_resource *
>   _mesa_program_resource_find_name(struct gl_shader_program *shProg,
>                                    GLenum programInterface, const char *name)
>   {
> +   GET_CURRENT_CONTEXT(ctx);
> +   const char *full_name = name;
> +
> +   /* Special case gl_VertexID -> gl_VertexIDMESA. */
> +   if (name && ctx->Const.VertexID_is_zero_based) {
> +      if (strcmp(name, "gl_VertexID") == 0)
> +         full_name = "gl_VertexIDMESA";
> +   }
> +

Can you add a comment to explain why we need to have gl_VertexIDMESA and 
not just gl_VertexID?
>      struct gl_program_resource *res = shProg->ProgramResourceList;
>      for (unsigned i = 0; i < shProg->NumProgramResourceList; i++, res++) {
>         if (res->Type != programInterface)
> @@ -563,7 +579,7 @@ _mesa_program_resource_find_name(struct gl_shader_program *shProg,
>            break;
>         case GL_PROGRAM_INPUT:
>         case GL_PROGRAM_OUTPUT:
> -         if (array_index_of_resource(res, name) >= 0)
> +         if (array_index_of_resource(res, full_name) >= 0)
>               return res;
>            break;
>         default:
> @@ -728,6 +744,10 @@ program_resource_location(struct gl_shader_program *shProg,
>            return -1;
>      }
>   
> +   /* Built-in locations should report GL_INVALID_INDEX. */
> +   if (strncmp(name, "gl_", 3) == 0)
There is a function that already does this job. You should use it!

is_gl_identifier(name)

> +      return GL_INVALID_INDEX;
It would be nicer if this patch was split in two (one to fix VertexID 
and one for the general case) but meh!

> +
>      /* VERT_ATTRIB_GENERIC0 and FRAG_RESULT_DATA0 are decremented as these
>       * offsets are used internally to differentiate between built-in attributes
>       * and user-defined attributes.

With those fixed.

Reviewed-by: Martin Peres <martin.peres at linux.intel.com>