Bug 103098 - [OpenGL CTS] KHR-GL45.enhanced_layouts.varying_structure_locations fails
Summary: [OpenGL CTS] KHR-GL45.enhanced_layouts.varying_structure_locations fails
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Intel 3D Bugs Mailing List
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 102590
  Show dependency treegraph
 
Reported: 2017-10-04 10:40 UTC by Juan A. Suarez
Modified: 2017-11-09 00:53 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Juan A. Suarez 2017-10-04 10:40:08 UTC
This test is failing in Skylake/Broadwell:

"FAILURE. Test case: double. Inspection of separable draw program interface failed:
Failed to query program for varying: single. Reason: GetProgramResourceiv: glGetError() returned GL_INVALID_VALUE at gl4cEnhancedLayoutsTest
s.cpp:3062"

and

"Attribute: array - invalid location: 0 expected: 1"
Comment 1 Kenneth Graunke 2017-10-07 20:49:39 UTC
Ilia has a partial fix, but no more time to work on it.  Someone could pick it up and finish it:
https://github.com/imirkin/mesa/commit/4d9d4c44eb38f3f5573e33043e1fed608ce910a9
Comment 2 Juan A. Suarez 2017-10-19 08:31:10 UTC
There's an updated version of the patch, that fixes the full test.


https://github.com/imirkin/mesa/commit/2e47b59970bc05e69973b1429fe6cc0890e607c1
Comment 3 Juan A. Suarez 2017-10-19 08:32:48 UTC
Ilia, any chance to submit this patch for review?

It is fixing the full test in Intel. I know that still fails on Nouveau, and you plan to refactor part of it.

But wondering if it is possible to submit it and continue with the work in a new patch.
Comment 4 Ilia Mirkin 2017-10-19 13:05:15 UTC
Hmmm... well, I had left a TODO of refactoring get_varying_type, which I copied from somewhere.

Also, TBH, I have some recollection of it breaking something else.

Oh right -- so with like a TCS input, "vec4 foo[32]", are we supposed to just have a "foo" resource or a "foo[0]" resource (and/or a "foo[1]" resource? I think that tripped me up last (and some tests were failing as a result).

Did you run that (second) patch through a full test run that includes dEQP?

Either way, feel free to send it out with fixes or whatever. Like Ken mentioned, I'm out of time to work on this.
Comment 5 Juan A. Suarez 2017-10-23 08:10:03 UTC
(In reply to Ilia Mirkin from comment #4)
> 
> Did you run that (second) patch through a full test run that includes dEQP?
> 

No, I didn't run full tests. But I'll do.
Comment 6 Juan A. Suarez 2017-10-27 14:08:45 UTC
I've submitted patch https://patchwork.freedesktop.org/series/32757/ to fix this issue.


It is mostly Ilia patch, without the get_varying_type() function.

I've checked and it doesn't cause any regression in piglit, dEQP or GL-CTS.


Also, this varying_structure_locations test has a bug when checking the varyings. So besides this Mesa patch, I also submitted https://gerrit.khronos.org/#/c/1889/ to fix it.


With both patches this test should pass in Mesa/Intel.
Comment 7 Juan A. Suarez 2017-10-27 14:49:27 UTC
(In reply to Ilia Mirkin from comment #4)
>
> Oh right -- so with like a TCS input, "vec4 foo[32]", are we supposed to
> just have a "foo" resource or a "foo[0]" resource (and/or a "foo[1]"
> resource? I think that tripped me up last (and some tests were failing as a
> result).
> 
>

According to spec:

"For an active variable declared as an array of basic types, a single
entry will be generated, with its name string formed by concatenating
the name of the array and the string "[0]". "


I understand this means we would have "foo[0]" only.
Comment 8 Ilia Mirkin 2017-10-27 14:55:09 UTC
(In reply to Juan A. Suarez from comment #7)
> (In reply to Ilia Mirkin from comment #4)
> >
> > Oh right -- so with like a TCS input, "vec4 foo[32]", are we supposed to
> > just have a "foo" resource or a "foo[0]" resource (and/or a "foo[1]"
> > resource? I think that tripped me up last (and some tests were failing as a
> > result).
> > 
> >
> 
> According to spec:
> 
> "For an active variable declared as an array of basic types, a single
> entry will be generated, with its name string formed by concatenating
> the name of the array and the string "[0]". "
> 
> 
> I understand this means we would have "foo[0]" only.

But is it really considered to be an array, given that TCS & co imply a level of arrayness for everything? Maybe :) I just don't know.
Comment 9 Juan A. Suarez 2017-11-08 09:24:43 UTC
Pushed

commit d5a641106baae2122cc3f09b4a755077d902ee88
Author: Juan A. Suarez Romero <jasuarez@igalia.com>
Date:   Tue Oct 31 17:39:17 2017 +0000

    glsl: add varying resources for arrays of complex types
    
    This patch is mostly a patch done by Ilia Mirkin.
    
    It fixes KHR-GL45.enhanced_layouts.varying_structure_locations.
    
    v2: fix locations for TCS/TES/GS inputs and outputs (Ilia)
    
    CC: Ilia Mirkin <imirkin@alum.mit.edu>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103098
    Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
    Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

 src/compiler/glsl/linker.cpp | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 4 deletions(-)


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.