On 10/07/2011 11:20 AM, Stéphane Marchesin wrote:
2011/10/7 Ian Romanick<i...@freedesktop.org>:
On 10/06/2011 06:54 PM, Stéphane Marchesin wrote:
Hi Ian,
This regresses Chrome GPU acceleration for all GPUs (I tested i915g,
llvmpipe, i965).
See also bugzilla #41499 and #41508. I tried to debug this a little
yesterday, but I couldn't see what was going wrong. None of our piglit or
GLES2 tests regress, so that's annoying.
Looking at the misrendered images, it looks like the attributes are just
getting sent to the wrong slots. All that I can figure is that the failing
programs link the program, set the attribute bindings, and then re-link the
program. Somehow in that process the bindings set by the linker in the
first link are used instead of the ones set by the application (and the
second link). As far as I can tell, that should work, but I haven't had a
chance to put together a test case.
Isn't it enough to run Chrome? It reproduces 100%.
I guess I should have said "minimal test case." :) I can't really put
Chrome in piglit. However, it looks like the existing
glsl-bindattriblocation tests should hit the case I was worrying about.
Do you have any other insights?
I didn't really look into it, I got the same graphical result as
41508, and bisected to that commit.
Stéphane
Stéphane
On Fri, Sep 30, 2011 at 16:44, Ian Romanick<i...@freedesktop.org> wrote:
From: Ian Romanick<ian.d.roman...@intel.com>
Signed-off-by: Ian Romanick<ian.d.roman...@intel.com>
---
src/glsl/linker.cpp | 138
+++++++++++++++++++++++---------------------------
1 files changed, 64 insertions(+), 74 deletions(-)
diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
index 6d40dd2..9463f53 100644
--- a/src/glsl/linker.cpp
+++ b/src/glsl/linker.cpp
@@ -1298,71 +1298,6 @@
assign_attribute_or_color_locations(gl_shader_program *prog,
invalidate_variable_locations(sh, direction, generic_base);
- if ((target_index == MESA_SHADER_VERTEX)&& (prog->Attributes !=
NULL)) {
- for (unsigned i = 0; i< prog->Attributes->NumParameters; i++) {
- ir_variable *const var =
-
sh->symbols->get_variable(prog->Attributes->Parameters[i].Name);
-
- /* Note: attributes that occupy multiple slots, such as arrays
or
- * matrices, may appear in the attrib array multiple times.
- */
- if ((var == NULL) || (var->location != -1))
- continue;
-
- /* From page 61 of the OpenGL 4.0 spec:
- *
- * "LinkProgram will fail if the attribute bindings assigned
by
- * BindAttribLocation do not leave not enough space to
assign a
- * location for an active matrix attribute or an active
attribute
- * array, both of which require multiple contiguous generic
- * attributes."
- *
- * Previous versions of the spec contain similar language but
omit the
- * bit about attribute arrays.
- *
- * Page 61 of the OpenGL 4.0 spec also says:
- *
- * "It is possible for an application to bind more than one
- * attribute name to the same location. This is referred to
as
- * aliasing. This will only work if only one of the aliased
- * attributes is active in the executable program, or if no
path
- * through the shader consumes more than one attribute of a
set
- * of attributes aliased to the same location. A link error
can
- * occur if the linker determines that every path through
the
- * shader consumes multiple aliased attributes, but
- * implementations are not required to generate an error in
this
- * case."
- *
- * These two paragraphs are either somewhat contradictory, or I
don't
- * fully understand one or both of them.
- */
- /* FINISHME: The code as currently written does not support
attribute
- * FINISHME: location aliasing (see comment above).
- */
- const int attr =
prog->Attributes->Parameters[i].StateIndexes[0];
- const unsigned slots = count_attribute_slots(var->type);
-
- /* Mask representing the contiguous slots that will be used by
this
- * attribute.
- */
- const unsigned use_mask = (1<< slots) - 1;
-
- /* Generate a link error if the set of bits requested for this
- * attribute overlaps any previously allocated bits.
- */
- if ((~(use_mask<< attr)& used_locations) != used_locations) {
- linker_error(prog,
- "insufficient contiguous attribute locations "
- "available for vertex shader input `%s'",
- var->name);
- return false;
- }
-
- var->location = VERT_ATTRIB_GENERIC0 + attr;
- used_locations |= (use_mask<< attr);
- }
- }
-
/* Temporary storage for the set of attributes that need locations
assigned.
*/
struct temp_attr {
@@ -1389,28 +1324,83 @@
assign_attribute_or_color_locations(gl_shader_program *prog,
continue;
if (var->explicit_location) {
- const unsigned slots = count_attribute_slots(var->type);
- const unsigned use_mask = (1<< slots) - 1;
- const int attr = var->location - generic_base;
-
if ((var->location>= (int)(max_index + generic_base))
|| (var->location< 0)) {
linker_error(prog,
"invalid explicit location %d specified for
`%s'\n",
- (var->location< 0) ? var->location : attr,
+ (var->location< 0)
+ ? var->location : var->location - generic_base,
var->name);
return false;
- } else if (var->location>= generic_base) {
- used_locations |= (use_mask<< attr);
+ }
+ } else if (target_index == MESA_SHADER_VERTEX) {
+ unsigned binding;
+
+ if (prog->AttributeBindings->get(binding, var->name)) {
+ assert(binding>= VERT_ATTRIB_GENERIC0);
+ var->location = binding;
}
}
/* The location was explicitly assigned, nothing to do here.
*/
- if (var->location != -1)
+ const unsigned slots = count_attribute_slots(var->type);
+ if (var->location != -1) {
+ if (var->location>= generic_base) {
+ /* From page 61 of the OpenGL 4.0 spec:
+ *
+ * "LinkProgram will fail if the attribute bindings
assigned
+ * by BindAttribLocation do not leave not enough space to
+ * assign a location for an active matrix attribute or an
+ * active attribute array, both of which require multiple
+ * contiguous generic attributes."
+ *
+ * Previous versions of the spec contain similar language but
omit
+ * the bit about attribute arrays.
+ *
+ * Page 61 of the OpenGL 4.0 spec also says:
+ *
+ * "It is possible for an application to bind more than
one
+ * attribute name to the same location. This is referred
to as
+ * aliasing. This will only work if only one of the
aliased
+ * attributes is active in the executable program, or if
no
+ * path through the shader consumes more than one
attribute of
+ * a set of attributes aliased to the same location. A
link
+ * error can occur if the linker determines that every
path
+ * through the shader consumes multiple aliased
attributes,
+ * but implementations are not required to generate an
error
+ * in this case."
+ *
+ * These two paragraphs are either somewhat contradictory, or
I
+ * don't fully understand one or both of them.
+ */
+ /* FINISHME: The code as currently written does not support
+ * FINISHME: attribute location aliasing (see comment above).
+ */
+ /* Mask representing the contiguous slots that will be used
by
+ * this attribute.
+ */
+ const unsigned attr = var->location - generic_base;
+ const unsigned use_mask = (1<< slots) - 1;
+
+ /* Generate a link error if the set of bits requested for
this
+ * attribute overlaps any previously allocated bits.
+ */
+ if ((~(use_mask<< attr)& used_locations) != used_locations)
{
+ linker_error(prog,
+ "insufficient contiguous attribute locations
"
+ "available for vertex shader input `%s'",
+ var->name);
+ return false;
+ }
+
+ used_locations |= (use_mask<< attr);
+ }
+
continue;
+ }
- to_assign[num_attr].slots = count_attribute_slots(var->type);
+ to_assign[num_attr].slots = slots;
to_assign[num_attr].var = var;
num_attr++;
}
--
1.7.6
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
