Bug 92743 - Centroid shouldn't have to match between the FS and the VS
Summary: Centroid shouldn't have to match between the FS and the VS
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: 10.5
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Jordan Justen
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: i965-deqp 93185
  Show dependency treegraph
 
Reported: 2015-10-30 19:15 UTC by Corentin Wallez
Modified: 2016-04-03 07:29 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
patch with relaxed interpolation matching (2.54 KB, text/plain)
2015-11-03 12:23 UTC, Marta Löfstedt
Details
relax only centroid requirements patch (1.38 KB, text/plain)
2015-11-03 14:00 UTC, Marta Löfstedt
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Corentin Wallez 2015-10-30 19:15:12 UTC
While running the GLES3 dEQP tests on ANGLE there was a failure that seemed related to a misinterpretation of an underspecified part of the GLSL spec.

This was running on an Ubuntu variant with the followin:
OpenGL renderer string: Mesa DRI Intel(R) HD Graphics 5500 (Broadwell GT2) 
OpenGL core profile version string: 3.3 (Core Profile) Mesa 10.5.9
OpenGL core profile shading language version string: 3.30

The problem is that glLinkProgram enforces varyings to have the same centroid qualifier in the VS and FS. The GLSL 330 spec isn't clear on the this but the GLSL ES 3.1 spec explicitly says centroid doesn't have to match. Also starting from the GLSL 420 the list of interpolation qualifier is explicitly given and centroid is not one of them. To finish there was a Khronos bug about that issue which resolution was that centroid didn't have to match.

The relevant dEQP test is dEQP-GLES3.functional.shaders.linkage.varying.rules.differing_interpolation_2, you can find it by searching for "differing_interpolation" in https://android.googlesource.com/platform/external/deqp/+/master/data/gles3/shaders/linkage.test
Comment 1 Ian Romanick 2015-10-30 20:33:48 UTC
That is correct.  I remember the Khronos bug.  I was going to investigate our implementation, but I never got around to it.  Look like you and dEQP beat me to it. :)
Comment 2 Marta Löfstedt 2015-11-03 12:19:53 UTC
So, I removed the check for centroid matching, but the test still does not pass. The new reason is that we now have smooth as output in vertex, but no interpolation qualifier in frag.
So, is the relaxed requirement for centroid also to be applied to all interpolation qualifiers?

For example: GLSL ES 3.0 standard, in section 4.3.9 say:
"The type and presence of the interpolation qualifiers and storage qualifiers and invariant qualifiers of
variables with the same name declared in all linked shaders must match, otherwise the link command will
fail."

Also, in GLSL ES 3.10 we have a nice table in 9.2.1 where we see that centroid does not have to match, but smooth does.
Comment 3 Marta Löfstedt 2015-11-03 12:23:36 UTC
Created attachment 119374 [details]
patch with relaxed interpolation matching

The attached patch removes the tests for interpolation qualifier matching between stages. And will make the test pass.
Comment 4 Marta Löfstedt 2015-11-03 14:00:39 UTC
Created attachment 119379 [details]
relax only centroid requirements patch

To clarify, I also add the patch that only relaxes the requirement for centroid. With this patch the test does not pass.
Comment 5 Corentin Wallez 2015-11-03 19:15:43 UTC
Here is a copy-paste of a bug I filed against dEQP that explains the situation. I'm not sure what the right step forward is yet, we might have to file a Khronos bug to have a resolution. In the meantime the centroid-only patch seems safe to merge, but not the other one.

The GLES3.functional.shaders.linkage.varying.rules.differing_interpolation_2 test tests that these two varyings correctly link together.
 - smooth out mediump float var;
 - centroid in mediump float var;
The differences between the variables is two-fold, no-centroid vs. centroid and smooth interpolation vs. default interpolation. The spec says that centroid and no-centroid is ok, see longer explanation in the first comment of https://bugs.freedesktop.org/show_bug.cgi?id=92743
However the spec unclear on the smooth vs. default being allowed. The GLSL ES 3.0.4 and GLSL 330 spec say in the interpolation section that:
    The type and presence of the interpolation qualifiers and storage qualifiers and invariant qualifiers of variables with the same name declared in all linked shaders must match, otherwise the link command will fail. (end of section 4.3.9 of the GLSL ES 3.0.4 spec)
Which seems to indicate that default vs. smooth is not valid since default is the absence of qualifier.

However in bug https://cvs.khronos.org/bugzilla/show_bug.cgi?id=7819 a block of text was added to the spec starting from GLSL 430 stating that the interpolation qualifiers do not have to link, and that the fragment shader's one is the one that gets used. So starting from that point on, the spec contradicts itself.

Now the question is whether the edit to GLSL 430 spec shows what the intent was all along, in which case we should ignore all the previous spec text on the presence having to match. Or it is something that should start to take effect only with the GLSL 430 spec onward in which case there is a spec bug for these versions.
Comment 6 Marta Löfstedt 2015-11-04 08:35:52 UTC
Great, that you already filed a bug against the dEQP. 

I will sent up a cleanup version of the centroid only patch to the Mesa list and we will see how the discussion pans out.
Comment 7 Corentin Wallez 2015-11-04 18:40:17 UTC
Great, thank you for handling the Mesa part of this!
Comment 8 Tapani Pälli 2015-11-06 12:44:32 UTC
(In reply to Corentin Wallez from comment #5)
> Here is a copy-paste of a bug I filed against dEQP that explains the
> situation. I'm not sure what the right step forward is yet, we might have to
> file a Khronos bug to have a resolution. In the meantime the centroid-only
> patch seems safe to merge, but not the other one.
> 
> The GLES3.functional.shaders.linkage.varying.rules.differing_interpolation_2
> test tests that these two varyings correctly link together.
>  - smooth out mediump float var;
>  - centroid in mediump float var;
> The differences between the variables is two-fold, no-centroid vs. centroid
> and smooth interpolation vs. default interpolation. The spec says that
> centroid and no-centroid is ok, see longer explanation in the first comment
> of https://bugs.freedesktop.org/show_bug.cgi?id=92743
> However the spec unclear on the smooth vs. default being allowed. The GLSL
> ES 3.0.4 and GLSL 330 spec say in the interpolation section that:
>     The type and presence of the interpolation qualifiers and storage
> qualifiers and invariant qualifiers of variables with the same name declared
> in all linked shaders must match, otherwise the link command will fail. (end
> of section 4.3.9 of the GLSL ES 3.0.4 spec)
> Which seems to indicate that default vs. smooth is not valid since default
> is the absence of qualifier.
> 
> However in bug https://cvs.khronos.org/bugzilla/show_bug.cgi?id=7819 a block
> of text was added to the spec starting from GLSL 430 stating that the
> interpolation qualifiers do not have to link, and that the fragment shader's
> one is the one that gets used. So starting from that point on, the spec
> contradicts itself.
> 
> Now the question is whether the edit to GLSL 430 spec shows what the intent
> was all along, in which case we should ignore all the previous spec text on
> the presence having to match. Or it is something that should start to take
> effect only with the GLSL 430 spec onward in which case there is a spec bug
> for these versions.

GLSL 440 spec removed the contradictory statement:

"Bug 10990: Remove old contradictory text requiring interpolation qualifiers to match cross stage; they must only match within a stage." This is how Mesa works currently, interpolation is checked against only if GLSL version is < 440.
Comment 9 Marta Löfstedt 2015-11-06 13:24:33 UTC
Yes Tapani, 

The idea is to remove the centroid requirement from mesa. I have sent up a patch for this:
https://patchwork.freedesktop.org/patch/63641/

and then have Google update the dEQP test according to GLES standard, i.e. the shader should not link for GLES (GLSL version is < 440), when smooth differs over shader stages.
Comment 10 Timothy Arceri 2016-02-15 07:42:16 UTC
(In reply to Marta Löfstedt from comment #9)
> Yes Tapani, 
> 
> The idea is to remove the centroid requirement from mesa. I have sent up a
> patch for this:
> https://patchwork.freedesktop.org/patch/63641/
> 
> and then have Google update the dEQP test according to GLES standard, i.e.
> the shader should not link for GLES (GLSL version is < 440), when smooth
> differs over shader stages.

I think Google is already doing the right thing. From the ES GLSL spec:

"When no interpolation qualifier is present, smooth interpolation is used."

So there should be no problem having smooth in one stage and nothing in the next as the default is smooth.

I looked at this problem last year after seeing issues in other deqp tests and had a fix but I wasn't sure at the time the impact on desktop GL. Looking at it again it seems its always smooth by default too unless there is some compat builtin stuff going on but that shouldn't be an issue.

I've sent a patch for this half of the issue to the mailing list.

https://lists.freedesktop.org/archives/mesa-dev/2016-February/107692.html
Comment 11 Timothy Arceri 2016-02-16 21:46:48 UTC
Smooth matching fix pushed:

commit	07e6a37332d2d3908ee1f1160ee25ed9c127b770
glsl: set user defined varyings to smooth by default in ES

This is usually handled by the backends in order to handle the
various interactions with the gl_*Color built-ins.

The problem is this means linking will fail if one side on the
interface adds the smooth qualifier to the varying and the other
side just uses the default even though they match.

This fixes various deqp tests. The spec is not clear what to for
desktop GL so leave it as is for now.

Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92743

Marta are you still working on the other fix for this bug?
Comment 12 Marta Löfstedt 2016-02-17 07:57:39 UTC
Hi Timothy,

Unfortunately I am currently not allowed to work on this issues.
If you want it you can take it.

/Marta
Comment 13 Marta Löfstedt 2016-02-18 09:29:39 UTC
Hi Timothy,

I hope it is OK that I assign this to you. I am currently buzy with other stuff. 

BR,

Marta
Comment 14 Kenneth Graunke 2016-03-11 01:48:30 UTC
Timothy is busy with other things.  Reassigning to Jordan.
Comment 15 Kenneth Graunke 2016-03-15 07:55:30 UTC
Hey Jordan,

Any news on this front?
Comment 16 Jordan Justen 2016-03-15 17:47:49 UTC
(Assuming we want to follow what the specs say...)

I looked at the specs, and it seems as if the test is
incorrect.

My reading of the spec says that for ES3, the error should
be generated. (So, we are currently doing the right thing.)

In the spec, it looks like we should not generate the error
ES31.

We currently aren't doing the correct thing for ES31.

I have 2 deqp patches, and 2 mesa patches.
Comment 17 Jordan Justen 2016-04-03 06:40:14 UTC
We decided to simply remove the checking for this
rather than strictly following the spec wording:

commit ef1b397b0770ddc24240462a1426f6c3fd4952bb (master)                                                                                                                                                             
                                                                                                                                                                                                                     
    glsl: Don't require matching centroid qualifiers
Comment 18 Timothy Arceri 2016-04-03 07:29:36 UTC
Just FYI interface block members are handled in link_interface_blocks.cpp interstage_member_mismatch() 

I assume this should be updated to match?


bug/show.html.tmpl processed on Jan 16, 2017 at 17:21:31.
(provided by the Example extension).