[Mesa-dev] [PATCH 4/4] mesa: Implement glGetFragDataLocation

[Ian Romanick]

On 11/08/2011 08:16 PM, Kenneth Graunke wrote:
> On 11/08/2011 08:10 PM, Kenneth Graunke wrote:
>> On 11/04/2011 04:41 PM, Ian Romanick wrote:
>>> From: Ian Romanick<ian.d.romanick at intel.com>
>>>
>>> Fixes piglit's getfragdatalocation test.
>>>
>>> Signed-off-by: Ian Romanick<ian.d.romanick at intel.com>
>>> ---
>>>   src/mesa/main/shader_query.cpp |   56 ++++++++++++++++++++++++++++++++++++++++
>>>   src/mesa/main/shaderapi.c      |   20 --------------
>>>   2 files changed, 56 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
>>> index 23667a1..644916c 100644
>>> --- a/src/mesa/main/shader_query.cpp
>>> +++ b/src/mesa/main/shader_query.cpp
>>> @@ -274,3 +274,59 @@ _mesa_BindFragDataLocation(GLuint program, GLuint colorNumber,
>>>       * glLinkProgram is called again.
>>>       */
>>>   }
>>> +
>>> +GLint GLAPIENTRY
>>> +_mesa_GetFragDataLocation(GLuint program, const GLchar *name)
>>> +{
>>> +   GET_CURRENT_CONTEXT(ctx);
>>> +   struct gl_shader_program *const shProg =
>>> +      _mesa_lookup_shader_program_err(ctx, program, "glGetFragDataLocation");
>>> +
>>> +   if (!shProg) {
>>> +      return -1;
>>> +   }
>>> +
>>> +   if (!shProg->LinkStatus) {
>>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>>> +                  "glGetFragDataLocation(program not linked)");
>>> +      return -1;
>>> +   }
>>> +
>>> +   if (!name)
>>> +      return -1;
>>> +
>>> +   if (strncmp(name, "gl_", 3) == 0) {
>>> +      _mesa_error(ctx, GL_INVALID_OPERATION,
>>> +                  "glGetFragDataLocation(illegal name)");
>>> +      return -1;
>>> +   }
>>> +
>>> +   /* Not having a fragment shader is not an error.
>>> +    */
>>> +   if (shProg->_LinkedShaders[MESA_SHADER_FRAGMENT] == NULL)
>>> +      return -1;
>>> +
>>> +   exec_list *ir = shProg->_LinkedShaders[MESA_SHADER_FRAGMENT]->ir;
>>> +   foreach_list(node, ir) {
>>> +      const ir_variable *const var = ((ir_instruction *) node)->as_variable();
>>> +
>>> +      /* The extra check against VERT_ATTRIB_GENERIC0 is because
>>
>> Ditto Eric's comment...FRAG_RESULT_DATA0.
>>
>>> +       * glGetFragDataLocation cannot be used on "conventional" attributes.
>>> +       *
>>> +       * From page 95 of the OpenGL 3.0 spec:
>>> +       *
>>> +       *     "If name is not an active attribute, if name is a conventional
>>> +       *     attribute, or if an error occurs, -1 will be returned."
>>> +       */
>>
>> I don't think this is quite sufficient: according to the man page, it's
>> only legal to call glGetFragDataLocation on user-defined FS outputs.
>> But gl_FragData has location>= FRAG_RESULT_DATA0, doesn't it?
>>
>> Maybe you need to strncmp(var->name, "gl_", 3) == 0 instead?
>
> Durr...I can read.  You already checked that above.
>
> I assert that all FS outputs with var->location<  FRAG_RESULT_DATA0 have
> names that begin with "gl_".  So, this check is just redundant, and you
> can drop it.  Right?
>
> After all, the only "conventional" attributes are gl_FragColor,
> gl_FragData, and gl_FragDepth.

You mean, drop the 'var->location < FRAG_RESULT_DATA0' from the 
if-statement below?  That is probably true.  It was a copy-and-paste 
from GetAttributeLocation.  The check *is* necessary there because there 
is no 'strncmp("gl_", name)' in that function.  GetAttributeLocation can 
get away with that because there is no shared locations between built-in 
and user-defined.  In the GetFragDataLocation, gl_FragData[] and 
user-defined variables overlap.

How would you feel about a follow-on patch that makes the two functions 
consistent with each other by removing the check from both loops and 
adding the strcmp outside the loop in GetAttributeLocation?

>>> +      if (var == NULL
>>> +	  || var->mode != ir_var_out
>>> +	  || var->location == -1
>>> +	  || var->location<  FRAG_RESULT_DATA0)
>>> +	 continue;
>>> +
>>> +      if (strcmp(var->name, name) == 0)
>>> +	 return var->location - FRAG_RESULT_DATA0;
>>> +   }
>>> +
>>> +   return -1;
>>> +}
>>> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>>> index 078f534..c766bd4 100644
>>> --- a/src/mesa/main/shaderapi.c
>>> +++ b/src/mesa/main/shaderapi.c
>>> @@ -496,16 +496,6 @@ get_attached_shaders(struct gl_context *ctx, GLuint program, GLsizei maxCount,
>>>   }
>>>
>>>
>>> -static GLint
>>> -get_frag_data_location(struct gl_context *ctx, GLuint program,
>>> -                       const GLchar *name)
>>> -{
>>> -   _mesa_problem(ctx, "get_frag_data_location() not implemented yet");
>>> -   return -1;
>>> -}
>>> -
>>> -
>>> -
>>>   /**
>>>    * glGetHandleARB() - return ID/name of currently bound shader program.
>>>    */
>>> @@ -1167,16 +1157,6 @@ _mesa_GetAttachedShaders(GLuint program, GLsizei maxCount,
>>>   }
>>>
>>>
>>> -/* GL_EXT_gpu_shader4, GL3 */
>>> -GLint GLAPIENTRY
>>> -_mesa_GetFragDataLocation(GLuint program, const GLchar *name)
>>> -{
>>> -   GET_CURRENT_CONTEXT(ctx);
>>> -   return get_frag_data_location(ctx, program, name);
>>> -}
>>> -
>>> -
>>> -
>>>   void GLAPIENTRY
>>>   _mesa_GetInfoLogARB(GLhandleARB object, GLsizei maxLength, GLsizei * length,
>>>                       GLcharARB * infoLog)