Ping.
FWIW, when I sent this patch, I also sent a test to the piglit ml, that
got reviewed and pushed:
https://cgit.freedesktop.org/piglit/commit/?id=88a9a99794f6108b656d4142cecd373b7995863c
To run it:
./bin/shader_runner
tests/spec/arb_arrays_of_arrays/execution/ubo/fs-const-explicit-binding.shader_test
-auto -fbo
right now, that test is crashing (at least on i965 driver, but it should
crash in most drivers).
On 24/01/18 15:30, Alejandro Piñeiro wrote:
> This is already handled at link_uniform_blocks, specifically at
> process_array_leaf.
>
> Additionally, this code was not handling correctly arrays of
> arrays. When creating the name of the block to set the binding, it
> only took into account the first level, so any attempt to set a
> explicit binding on a array of array ubo would trigger an assertion.
> ---
>
> I have just send a new test to the piglit list ("arb_arrays_of_arrays:
> add ubo test with explicit binding") that crashes without this
> patch. Without this patch it reachs the "unreachable("")" at the
> method set_block_binding that is being removed.
>
> It reaches that point because set_block_binding is called with a wrong
> block_name. When it is called with an array, it uses only the first
> layer, so for arrays of arrays, it creates a wrong name (block[0]
> instead of block[0][0] for example). See below, after the "Section
> 4.4.3" comment.
>
> Initially I was trying to solve this problem by fixing how the block
> name is created, until I realized that the binding at that point was
> already correct. Then I took a look to the code, and found that
> link_uniform_blocks were already handling setting the explicit
> binding, at process_block_array_leaf. Skimming it, seemed to handle
> all the corner cases. I also sent this patch to Intel CI and I didn't
> get any regression. And in any case, if there is any case not covered
> by the tests, I think that it would be better to handle explicit
> binding for ubos in just one place.
>
>
> src/compiler/glsl/link_uniform_initializers.cpp | 58
> +------------------------
> 1 file changed, 2 insertions(+), 56 deletions(-)
>
> diff --git a/src/compiler/glsl/link_uniform_initializers.cpp
> b/src/compiler/glsl/link_uniform_initializers.cpp
> index 97796e721bf..35522f76467 100644
> --- a/src/compiler/glsl/link_uniform_initializers.cpp
> +++ b/src/compiler/glsl/link_uniform_initializers.cpp
> @@ -182,26 +182,6 @@ set_opaque_binding(void *mem_ctx, gl_shader_program
> *prog,
> }
> }
>
> -static void
> -set_block_binding(gl_shader_program *prog, const char *block_name,
> - unsigned mode, int binding)
> -{
> - unsigned num_blocks = mode == ir_var_uniform ?
> - prog->data->NumUniformBlocks :
> - prog->data->NumShaderStorageBlocks;
> - struct gl_uniform_block *blks = mode == ir_var_uniform ?
> - prog->data->UniformBlocks : prog->data->ShaderStorageBlocks;
> -
> - for (unsigned i = 0; i < num_blocks; i++) {
> - if (!strcmp(blks[i].Name, block_name)) {
> - blks[i].Binding = binding;
> - return;
> - }
> - }
> -
> - unreachable("Failed to initialize block binding");
> -}
> -
> void
> set_uniform_initializer(void *mem_ctx, gl_shader_program *prog,
> const char *name, const glsl_type *type,
> @@ -307,43 +287,9 @@ link_set_uniform_initializers(struct gl_shader_program
> *prog,
> linker::set_opaque_binding(mem_ctx, prog, var, var->type,
> var->name, &binding);
> } else if (var->is_in_buffer_block()) {
> - const glsl_type *const iface_type = var->get_interface_type();
> -
> - /* If the variable is an array and it is an interface
> instance,
> - * we need to set the binding for each array element. Just
> - * checking that the variable is an array is not sufficient.
> - * The variable could be an array element of a uniform block
> - * that lacks an instance name. For example:
> - *
> - * uniform U {
> - * float f[4];
> - * };
> - *
> - * In this case "f" would pass is_in_buffer_block (above) and
> - * type->is_array(), but it will fail is_interface_instance().
> + /* This case is handled by link_uniform_blocks (at
> + * process_block_array_leaf)
> */
> - if (var->is_interface_instance() && var->type->is_array()) {
> - for (unsigned i = 0; i < var->type->length; i++) {
> - const char *name =
> - ralloc_asprintf(mem_ctx, "%s[%u]", iface_type->name,
> i);
> -
> - /* Section 4.4.3 (Uniform Block Layout Qualifiers) of
> the
> - * GLSL 4.20 spec says:
> - *
> - * "If the binding identifier is used with a uniform
> - * block instanced as an array then the first
> element
> - * of the array takes the specified block binding
> and
> - * each subsequent element takes the next
> consecutive
> - * uniform block binding point."
> - */
> - linker::set_block_binding(prog, name, var->data.mode,
> - var->data.binding + i);
> - }
> - } else {
> - linker::set_block_binding(prog, iface_type->name,
> - var->data.mode,
> - var->data.binding);
> - }
> } else if (type->contains_atomic()) {
> /* we don't actually need to do anything. */
> } else {
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
