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%. > 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 >>> >> > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev