Bug 92743

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. :)

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.

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.

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.

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.

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.

Great, thank you for handling the Mesa part of this!

(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.

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.

(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

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?

Hi Timothy,

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

/Marta

Hi Timothy,

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

BR,

Marta

Timothy is busy with other things.  Reassigning to Jordan.

Hey Jordan,

Any news on this front?

(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.

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

Just FYI interface block members are handled in link_interface_blocks.cpp interstage_member_mismatch() 

I assume this should be updated to match?