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.roman...@intel.com>

Fixes piglit's getfragdatalocation test.

Signed-off-by: Ian Romanick<ian.d.roman...@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)
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to