Bug 96358 - SSO: wrong interface validation between GS and VS (regresion due to latest gles 3.1)
Summary: SSO: wrong interface validation between GS and VS (regresion due to latest gl...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Ian Romanick
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-03 19:28 UTC by gregory.hainaut
Modified: 2016-07-07 06:18 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Strip arrayness from interface block names in some IO validation (2.76 KB, patch)
2016-06-10 15:05 UTC, Ian Romanick
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description gregory.hainaut 2016-06-03 19:28:54 UTC
Hello,

My program generates pipeline invalidation error but it used to be fine. It is related to bd3f15cffdbbec6d1ea5b7db2fcddaf8b7ae4524.

Here the Vertex Shader output

out SHADER
{
    vec4 t_float;
    vec4 t_int;
    vec4 c;
    flat vec4 fc;
} VSout;

Here the Geometry Shader input. Please notice the array.

in SHADER
{
    vec4 t_float;
    vec4 t_int;
    vec4 c;
    flat vec4 fc;
} GSin[];

In src/mesa/main/shader_query.cpp in validate_io function:

The error detected is this one:

      /* Section 7.4.1 (Shader Interface Matching) of the OpenGL ES 3.1 spec
       * says:
       *
       *    - An output variable is considered to match an input variable in
       *      the subsequent shader if:
       *
       *      - the two variables match in name, type, and qualification; or
       *
       *      - the two variables are declared with the same location
       *        qualifier and match in type and qualification.
       */
      if (producer_var == NULL) {
         valid = false;
         goto out;
      }


Here the consumer_var info
(gdb) p *consumer_var
$17 = {
  type = 0x8acc940, 
  interface_type = 0x8b39b50, 
  outermost_struct_type = 0x0, 
  name = 0x8ae8e68 "SHADER[2].c", 
  location = -1, 
  component = 0, 
  index = 0, 
  patch = 0, 
  mode = 4, 
  interpolation = 2, 
  explicit_location = 0, 
  precision = 0
}

And the first output
gdb) p *outputs[0 
$24 = {
  type = 0xf4b9f4f0 <glsl_type::_vec4_type>, 
  interface_type = 0x8b382e8, 
  outermost_struct_type = 0x0, 
  name = 0x8b27b80 "SHADER.c", 
  location = -1, 
  component = 0, 
  index = 0, 
  patch = 0, 
  mode = 5, 
  interpolation = 0, 
  explicit_location = 0, 
  precision = 0
}

As you can the name of the GS interface got an extra "[2]" likely the size of the array. So the previous strcmp isn't possible
<<<<
  if (!var->explicit_location &&
                strcmp(consumer_var->name, var->name) == 0) {
>>>>

Good luck :)
Comment 1 gregory.hainaut 2016-06-03 19:35:04 UTC
Note I just took the commit from a git blame. It seems the code replace an older function. But I don't know if the older function was broken too.
Comment 2 Ilia Mirkin 2016-06-03 19:41:13 UTC
(In reply to gregory.hainaut from comment #1)
> Note I just took the commit from a git blame. It seems the code replace an
> older function. But I don't know if the older function was broken too.

The previous code was pretty busted. However it was never run on desktop GL, while with the new logic, it's run on desktop GL for debug contexts. However it hasn't had a lot of testing on desktop GL, only the dEQP (ES) tests.
Comment 3 Ian Romanick 2016-06-05 00:24:43 UTC
I'm surprised that nothing in GLES 3.2 CTS or dEQP caught this. The problem is that in this variable-in-an-interface case the arrayness isn't properly checked. That should be easy enough to fix for this release.  In the longer term, I think a bunch of the program_interface_query code should just be rewritten.
Comment 4 Ian Romanick 2016-06-10 15:05:15 UTC
Created attachment 124453 [details] [review]
Strip arrayness from interface block names in some IO validation

This patch is a bit of a hack, but I think it should fix the problem.  If it works, I'll improve it & send to the list for review.

If this doesn't fix the problem, I think we should just remove IO validation from desktop GL altogether.

diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
index 5a46cfe..10a9544 100644
--- a/src/mesa/main/pipelineobj.c
+++ b/src/mesa/main/pipelineobj.c
@@ -928,8 +928,7 @@ _mesa_validate_program_pipeline(struct gl_context* ctx,
     * Based on this, only perform the most-strict checking on ES or when the
     * application has created a debug context.
     */
-   if ((_mesa_is_gles(ctx) || (ctx->Const.ContextFlags & GL_CONTEXT_FLAG_DEBUG_BIT)) &&
-       !_mesa_validate_pipeline_io(pipe))
+   if (_mesa_is_gles(ctx) && !_mesa_validate_pipeline_io(pipe))
       return GL_FALSE;
 
    pipe->Validated = GL_TRUE;
Comment 5 gregory.hainaut 2016-06-10 16:44:39 UTC
I think there is a typo.

(gdb) p consumer_name
$1 = 0x8ae8b14 "SHADER[2].c"

(gdb) p consumer_name
$2 = 0x2bdf75b8 "SHADERc"

(gdb) p *outputs[0]
$4 = {
  type = 0xf4c0e5e0 <glsl_type::_vec4_type>, 
  interface_type = 0x8ae871c, 
  outermost_struct_type = 0x0, 
  name = 0x8a84e9c "SHADER.c", 
  location = -1, 
  component = 0, 
  index = 0, 
  patch = 0, 
  mode = 5, 
  interpolation = 0, 
  explicit_location = 0, 
  precision = 0
}
Comment 6 gregory.hainaut 2016-06-10 17:12:22 UTC
Using "strcpy(s, t + 1)" is better. But another check is failling below.

      if (producer_var->type != consumer_var->type ||
          producer_var->interpolation != consumer_var->interpolation ||
          producer_var->precision != consumer_var->precision) {
         valid = false;
         goto out;
      }

Here some debug info. So you can't compare type pointer due to arrayness. And I don't know what interpolation is different it ought to be the same. Likely Mesa set the FLAT status to some IO.

(gdb) p *producer_var 
$18 = {
  type = 0xf4c0d5e0 <glsl_type::_vec4_type>, 
  interface_type = 0x8b08d38, 
  outermost_struct_type = 0x0, 
  name = 0x8b02028 "SHADER.c", 
  location = -1, 
  component = 0, 
  index = 0, 
  patch = 0, 
  mode = 5, 
  interpolation = 0, 
  explicit_location = 0, 
  precision = 0
}

(gdb) p *consumer_var
$19 = {
  type = 0x8ac14e8, 
  interface_type = 0x8b0fb80, 
  outermost_struct_type = 0x0, 
  name = 0x8ad36a0 "SHADER[2].c", 
  location = -1, 
  component = 0, 
  index = 0, 
  patch = 0, 
  mode = 4, 
  interpolation = 2, 
  explicit_location = 0, 
  precision = 0
}


(gdb) p *consumer_var->type
$25 = {
  gl_type = 35666, 
  base_type = GLSL_TYPE_ARRAY, 
  sampler_dimensionality = 0, 
  sampler_shadow = 0, 
  sampler_array = 0, 
  sampled_type = 0, 
  interface_packing = 0, 
  vector_elements = 0 '\000', 
  matrix_columns = 0 '\000', 
  length = 2, 
  name = 0x8ac1520 "vec4[2]", 
  fields = {
    array = 0xf4c0d5e0 <glsl_type::_vec4_type>, 
    parameters = 0xf4c0d5e0 <glsl_type::_vec4_type>, 
    structure = 0xf4c0d5e0 <glsl_type::_vec4_type>
  }, 

(gdb) p *producer_var->type
$27 = {
  gl_type = 35666, 
  base_type = GLSL_TYPE_FLOAT, 
  sampler_dimensionality = 0, 
  sampler_shadow = 0, 
  sampler_array = 0, 
  sampled_type = 0, 
  interface_packing = 0, 
  vector_elements = 4 '\004', 
  matrix_columns = 1 '\001', 
  length = 0, 
  name = 0x87932c8 "vec4", 
  fields = {
    array = 0x0, 
    parameters = 0x0, 
    structure = 0x0
  },
Comment 7 gregory.hainaut 2016-06-10 17:15:00 UTC
Forgot to say that interface_type won't match neither again due to arrayness.
Comment 8 gregory.hainaut 2016-06-18 08:55:30 UTC
I did a quick of latest Ian's patches. They are working fine for me. Bug could be closed once there are merged.
Comment 9 Arek Ruśniak 2016-07-05 08:05:31 UTC
Hi Gregory, I have a problem with building mesa with:
https://cgit.freedesktop.org/mesa/mesa/commit/?id=6a524c76f502fe15bb3612065a23ece693aed237

Is that commit related to this bug? If not just tell me then I open new raport. 


mesa building time error:

main/uniform_query.cpp: In function 'bool _mesa_sampler_uniforms_pipeline_are_valid(gl_pipeline_object*)':
main/uniform_query.cpp:1086:47: error: cannot convert 'gl_linked_shader* const' to 'gl_shader*' in assignment
       shader = shProg[idx]->_LinkedShaders[idx];
                                               ^
main/uniform_query.cpp:1087:31: error: 'struct gl_shader' has no member named 'Program'
       if (!shader || !shader->Program)
                               ^~~~~~~
main/uniform_query.cpp:1090:22: error: 'struct gl_shader' has no member named 'Program'
       mask = shader->Program->SamplersUsed;
                      ^~~~~~~
main/uniform_query.cpp:1093:32: error: 'struct gl_shader' has no member named 'SamplerUnits'
          GLuint unit = shader->SamplerUnits[s];
                                ^~~~~~~~~~~~
main/uniform_query.cpp:1094:31: error: 'struct gl_shader' has no member named 'SamplerTargets'
          GLuint tgt = shader->SamplerTargets[s];
                               ^~~~~~~~~~~~~~
main/uniform_query.cpp:1115:34: error: 'struct gl_shader' has no member named 'num_samplers'
       active_samplers += shader->num_samplers;
                                  ^~~~~~~~~~~~
make[5]: *** [Makefile:2971: main/uniform_query.lo] Error 1
Comment 10 Timothy Arceri 2016-07-05 08:39:03 UTC
(In reply to Arek Ruśniak from comment #9)
> Hi Gregory, I have a problem with building mesa with:
> https://cgit.freedesktop.org/mesa/mesa/commit/
> ?id=6a524c76f502fe15bb3612065a23ece693aed237
> 
> Is that commit related to this bug? If not just tell me then I open new
> raport. 
> 

Sorry this was my bad. I'll push a fix in a moment.
Comment 11 Arek Ruśniak 2016-07-05 10:41:31 UTC
Np, thx for quick fix.
Comment 12 Timothy Arceri 2016-07-07 06:18:17 UTC
commit	73a6a4ce4975016d4f86d644b31d30bb6d3a38f8

mesa: Strip arrayness from interface block names in some IO validation
Outputs from the vertex shader need to be able to match
per-vertex-arrayed inputs of later stages.  Acomplish this by stripping
one level of arrayness from the names and types of outputs going to a
per-vertex-arrayed stage.

v2: Add missing checks for TESS_EVAL->GEOMETRY.  Noticed by Timothy
Arceri.

v3: Use a slightly simpler stage check suggested by Ilia.

Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96358
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Cc: "12.0" <mesa-stable@lists.freedesktop.org>
Cc: Gregory Hainaut <gregory.hainaut@gmail.com>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>


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