Bug 89726 - [Bisected] dEQP-GLES3: uniform linking logic in the presence of structs
Summary: [Bisected] dEQP-GLES3: uniform linking logic in the presence of structs
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-23 08:33 UTC by Samuel Iglesias Gonsálvez
Modified: 2015-03-24 14:33 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
patch to fix the issue (512 bytes, patch)
2015-03-23 10:44 UTC, Tapani Pälli
Details | Splinter Review

Description Samuel Iglesias Gonsálvez 2015-03-23 08:33:34 UTC
Failed tests:

      * dEQP-GLES3.functional.uniform_api.info_query.active_uniform.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.info_query.active_uniform.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.info_query.active_uniform.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.info_query.active_uniform.nested_structs_arrays.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.info_query.active_uniform.nested_structs_arrays.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.info_query.active_uniform.nested_structs_arrays.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.info_query.active_uniform.unused_uniforms.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.info_query.active_uniform.unused_uniforms.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.info_query.active_uniform.unused_uniforms.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.info_query.indices_active_uniformsiv.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.info_query.indices_active_uniformsiv.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.info_query.indices_active_uniformsiv.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.info_query.indices_active_uniformsiv.nested_structs_arrays.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.info_query.indices_active_uniformsiv.nested_structs_arrays.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.info_query.indices_active_uniformsiv.nested_structs_arrays.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.info_query.indices_active_uniformsiv.unused_uniforms.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.info_query.indices_active_uniformsiv.unused_uniforms.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.info_query.indices_active_uniformsiv.unused_uniforms.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.info_query.consistency.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.info_query.consistency.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.info_query.consistency.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.info_query.consistency.nested_structs_arrays.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.info_query.consistency.nested_structs_arrays.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.info_query.consistency.nested_structs_arrays.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.info_query.consistency.unused_uniforms.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.info_query.consistency.unused_uniforms.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.info_query.consistency.unused_uniforms.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.initial.get_uniform.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.initial.get_uniform.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.initial.get_uniform.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.initial.get_uniform.nested_structs_arrays.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.initial.get_uniform.nested_structs_arrays.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.initial.get_uniform.nested_structs_arrays.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.get_uniform.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.get_uniform.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.get_uniform.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.get_uniform.nested_structs_arrays.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.get_uniform.nested_structs_arrays.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.get_uniform.nested_structs_arrays.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.render.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.render.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.render.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.render.nested_structs_arrays.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.render.nested_structs_arrays.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_pointer.render.nested_structs_arrays.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.get_uniform.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.get_uniform.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.get_uniform.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.get_uniform.nested_structs_arrays.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.get_uniform.nested_structs_arrays.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.get_uniform.nested_structs_arrays.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.render.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.render.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.render.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.render.nested_structs_arrays.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.render.nested_structs_arrays.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.by_value.render.nested_structs_arrays.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.basic_array_assign_full.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.basic_array_assign_full.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.basic_array_assign_full.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.basic_array_assign_partial.array_in_struct.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.basic_array_assign_partial.array_in_struct.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.basic_array_assign_partial.array_in_struct.sampler2D_samplerCube_both
      * dEQP-GLES3.functional.uniform_api.value.assigned.unused_uniforms.sampler2D_samplerCube_vertex
      * dEQP-GLES3.functional.uniform_api.value.assigned.unused_uniforms.sampler2D_samplerCube_fragment
      * dEQP-GLES3.functional.uniform_api.value.assigned.unused_uniforms.sampler2D_samplerCube_both


Bisected:

commit 53bf7c8fd2e11a6c64d6ff3a98b56d64d2242505
Author: Ilia Mirkin <imirkin@alum.mit.edu>
Date:   Tue Feb 10 03:02:09 2015 -0500

    glsl: fix uniform linking logic in the presence of structs
    
    Add a enter/leave record callback so that the offset may be aligned to
    the proper value. Otherwise only leaf fields are called, and the first
    field needs to be aligned to the outer struct's base alignment while the
    last field needs to be aligned to the inner struct's base alignment.
    
    This removes most usage of the last field/record type values passed into
    visit_field.
    
    Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
    Reviewed-by: Dave Airlie <airlied@redhat.com>


How to reproduce it:

$ cd modules/gles3/
$ ./deqp-gles3 -n dEQP-GLES3.functional.uniform_api.info_query.active_uniform.array_in_struct.sampler2D_samplerCube_vertex

Error message:

$ ./deqp-gles3 -n dEQP-GLES3.functional.uniform_api.info_query.active_uniform.array_in_struct.sampler2D_samplerCube_vertex
dEQP Core 2014.x (0xcafebabe) starting..
  target implementation = 'Default'

Test case 'dEQP-GLES3.functional.uniform_api.info_query.active_uniform.array_in_struct.sampler2D_samplerCube_vertex'..
deqp-gles3: glsl_types.cpp:1019: unsigned int glsl_type::std140_base_alignment(bool) const: Assertion `this->fields.array->is_record()' failed.
Aborted (core dumped)

Tested on HSW with Mesa master HEAD: 2fd21d8a84bd28461c964e2ab350389a55b7f001
Comment 1 Ilia Mirkin 2015-03-23 10:10:14 UTC
Where might one find a copy of this test to see what it's doing?
Comment 2 Ilia Mirkin 2015-03-23 10:19:18 UTC
Nevermind, reproduced, will send a piglit test shortly. (And perhaps a fix, too...)
Comment 3 Tapani Pälli 2015-03-23 10:44:33 UTC
Created attachment 114547 [details] [review]
patch to fix the issue

I took a look earlier, this might be the fix.
Comment 4 Ilia Mirkin 2015-03-23 10:51:54 UTC
Yeah.... IMO that's the wrong fix though. I think my earlier code was just bonkers (which I knew at the time, but was hoping I could get away with). This is what I'm testing right now:

diff --git a/src/glsl/glsl_types.cpp b/src/glsl/glsl_types.cpp
index 38b37a6..be87b0a 100644
--- a/src/glsl/glsl_types.cpp
+++ b/src/glsl/glsl_types.cpp
@@ -1077,15 +1077,6 @@ glsl_type::std140_base_alignment(bool row_major) const
       return base_alignment;
    }
 
-   /* A sampler may never occur in a UBO (without bindless of some sort),
-    * however it is convenient to use this alignment function even with
-    * regular uniforms. This allows use of this function on uniform structs
-    * that contain samplers.
-    */
-   if (this->is_sampler()) {
-      return 0;
-   }
-
    assert(!"not reached");
    return -1;
 }
diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
index 799c74b..59adc29 100644
--- a/src/glsl/link_uniforms.cpp
+++ b/src/glsl/link_uniforms.cpp
@@ -547,6 +547,8 @@ private:
    virtual void enter_record(const glsl_type *type, const char *name,
                              bool row_major) {
       assert(type->is_record());
+      if (this->ubo_block_index == -1)
+         return;
       this->ubo_byte_offset = glsl_align(
             this->ubo_byte_offset, type->std140_base_alignment(row_major));
    }
@@ -554,6 +556,8 @@ private:
    virtual void leave_record(const glsl_type *type, const char *name,
                              bool row_major) {
       assert(type->is_record());
+      if (this->ubo_block_index == -1)
+         return;
       this->ubo_byte_offset = glsl_align(
             this->ubo_byte_offset, type->std140_base_alignment(row_major));
    }
Comment 5 Tapani Pälli 2015-03-23 11:05:24 UTC
(In reply to Ilia Mirkin from comment #4)
> Yeah.... IMO that's the wrong fix though. I think my earlier code was just
> bonkers (which I knew at the time, but was hoping I could get away with).

I think 1ec715ce was fine as then you don't need to make special cases here and there in the already quite complicated code.

> This is what I'm testing right now:
> 
> diff --git a/src/glsl/glsl_types.cpp b/src/glsl/glsl_types.cpp
> index 38b37a6..be87b0a 100644
> --- a/src/glsl/glsl_types.cpp
> +++ b/src/glsl/glsl_types.cpp
> @@ -1077,15 +1077,6 @@ glsl_type::std140_base_alignment(bool row_major) const
>        return base_alignment;
>     }
>  
> -   /* A sampler may never occur in a UBO (without bindless of some sort),
> -    * however it is convenient to use this alignment function even with
> -    * regular uniforms. This allows use of this function on uniform structs
> -    * that contain samplers.
> -    */
> -   if (this->is_sampler()) {
> -      return 0;
> -   }
> -
>     assert(!"not reached");
>     return -1;
>  }
> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> index 799c74b..59adc29 100644
> --- a/src/glsl/link_uniforms.cpp
> +++ b/src/glsl/link_uniforms.cpp
> @@ -547,6 +547,8 @@ private:
>     virtual void enter_record(const glsl_type *type, const char *name,
>                               bool row_major) {
>        assert(type->is_record());
> +      if (this->ubo_block_index == -1)
> +         return;
>        this->ubo_byte_offset = glsl_align(
>              this->ubo_byte_offset, type->std140_base_alignment(row_major));
>     }
> @@ -554,6 +556,8 @@ private:
>     virtual void leave_record(const glsl_type *type, const char *name,
>                               bool row_major) {
>        assert(type->is_record());
> +      if (this->ubo_block_index == -1)
> +         return;
>        this->ubo_byte_offset = glsl_align(
>              this->ubo_byte_offset, type->std140_base_alignment(row_major));
>     }

Yep, that should do it (if nothing else depends on 1ec715ce).
Comment 6 Ilia Mirkin 2015-03-24 14:33:26 UTC
Pushed as baa22c8825133a69fd0657f09d2a027236233eb1 upstream. Sorry for the breakage!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.