Bug 76394

Created attachment 96095 [details]
render error png

The problem seems to be that there is a conflict with the names of in/out variables in the geometry shader that the compiler is not handling properly, even when they are in different interface blocks.

Inputs and outputs in the geometry shader are defined like this:

in block
{
	vec4 Color;
} In[];

out block
{
       vec4 Color;
} Out;

Simply giving the output a different name (and editing the vertex/frag shaders accordingly) fixes the problem. For example:

out block
{
	vec4 ColorF;
} Out;

Note: It will still write the error messages mentioned in the bug report after this change. These errors are produced with every access to gl_in in the geometry shader.

Debugging the code in src/glsl/ir_set_program_inouts.cpp reveals the conflict happens because the in Color variable in the geometry shader is incorrectly considered an output variable.

In this instruction:

Out.Color = In[i].Color;

In[i].Color is not identified as 'ir_var_shader_in' but as 'ir_var_shader_out' instead, which then leads to follow a different (incorrect) code path in ir_set_program_inouts_visitor::visit_enter(ir_dereference_array *ir).

The problem is that the glsl compiler generates flat names for the fields in these blocks that do not consider the instance name, so it produces a name like block.Color for boths fields, leading to clashes.

Patch sent to the mailing list for review:
http://lists.freedesktop.org/archives/mesa-dev/2014-April/057202.html

Committed as:

commit 6d0e30c6a332de9ea7ab00e1fd303df2fb337c64
Author: Iago Toral Quiroga <itoral@igalia.com>
Date:   Fri Apr 4 15:11:15 2014 +0200

    glsl: Properly handle blocks that define the same field name.
    
    Currently we can have name space collisions between blocks that define the same
    fields. For example:
    
    in block
    {
        vec4 Color;
    } In[];
    
    out block
    {
        vec4 Color;
    } Out;
    
    These two blocks will assign the same interface name (block.Color) to the Color
    field in flatten_named_interface_blocks_declarations.cpp, leading to havoc.
    This was breaking badly the gl-320-primitive-shading test from ogl-samples.
    
    The patch uses the block instance name to avoid collisions, producing names
    like block.In.Color and block.Out.Color to avoid the name clash.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76394
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Marking fixed.  But...I'm not sure if those errors are expected.  If not, we should probably reopen this (and change the title)...

The issue that render error is fixed ,but its output errors still exist, so reopen the bug and change the title.

(In reply to comment #5)
> Committed as:
> 
> commit 6d0e30c6a332de9ea7ab00e1fd303df2fb337c64
> Author: Iago Toral Quiroga <itoral@igalia.com>
> Date:   Fri Apr 4 15:11:15 2014 +0200
> 
>     glsl: Properly handle blocks that define the same field name.
>     
>     Currently we can have name space collisions between blocks that define
> the same
>     fields. For example:
>     
>     in block
>     {
>         vec4 Color;
>     } In[];
>     
>     out block
>     {
>         vec4 Color;
>     } Out;
>     
>     These two blocks will assign the same interface name (block.Color) to
> the Color
>     field in flatten_named_interface_blocks_declarations.cpp, leading to
> havoc.
>     This was breaking badly the gl-320-primitive-shading test from
> ogl-samples.
>     
>     The patch uses the block instance name to avoid collisions, producing
> names
>     like block.In.Color and block.Out.Color to avoid the name clash.
>     
>     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76394
>     Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Your nice patch fixed gl-320-primitive-shading test mostly, here.

Before, I only got an empty black window.
Now, it shows only little partial damage which changes during mouse rotation (right button). See attchment:
gl-320-primitive-shading-brocken.png

Related bug was:
https://bugs.freedesktop.org/show_bug.cgi?id=74717

> Marking fixed.  But...I'm not sure if those errors are expected.  If not, we
> should probably reopen this (and change the title)...

Oh,...
But I'm on r600g (RV730 AGP), so...

Created attachment 97486 [details]
gl-320-primitive-shading-brocken.png

With current master the test doesn't crash for me and is correctly displayed but I still have some error messages:

0:22(21): error: length called on unsized array
0:24(17): error: unsized array index must be constant
0:25(15): error: unsized array index must be constant


Config:
OpenGL renderer string: Gallium 0.4 on AMD CYPRESS
OpenGL core profile version string: 3.3 (Core Profile) Mesa 10.2.0-devel (git-fd92346)
OpenGL core profile shading language version string: 3.30

Build:
./autogen.sh --with-gallium-drivers=r600 --with-dri-drivers= --enable-texture-float --disable-dri3 --disable-r600-llvm-compiler --disable-gallium-llvm --enable-64-bit CFLAGS="-O2 -m64" CXXFLAGS="-O2 -m64" --libdir=/usr/lib64

Ping!

[r600g] --- Should we change the 'Component'?

GL_RENDERER   = Gallium 0.4 on AMD RV730
GL_VERSION    = 3.0 Mesa 10.4.0-devel (git-5598458)
GL_VENDOR     = X.Org

Same broken image as before.

Thanks,
  Dieter

(In reply to comment #10)
> Ping!
> 
> [r600g] --- Should we change the 'Component'?

The output for Intel is correct but there are still some error messages in the console so there is still something wrong somewhere. I'd suggest to keep this in i965 and maybe create a new bug for r600g.

> GL_RENDERER   = Gallium 0.4 on AMD RV730
> GL_VERSION    = 3.0 Mesa 10.4.0-devel (git-5598458)
> GL_VENDOR     = X.Org
> 
> Same broken image as before.
> 
> Thanks,
>   Dieter

(In reply to comment #11)
> (In reply to comment #10)
> > Ping!
> > 
> > [r600g] --- Should we change the 'Component'?
> 
> The output for Intel is correct but there are still some error messages in
> the console so there is still something wrong somewhere. I'd suggest to keep
> this in i965 and maybe create a new bug for r600g.

Yep, these are separate issues. This one could continue living as a mesa glsl compiler bug, array size is checked here before it is known (array size being a special case here with gl_in).

With some closer inspection to "./tests/gl-320-primitive-shading.cpp" it can be seen that the error output is expected, run consists of both negative and positive tests (this is true with some of the other tests as well, very poorly documented though). The compilation resulting in errors is supposed to fail, therefore resolving as fixed (for intel).

(In reply to Tapani Pälli from comment #13)
Verified it.

(In reply to Dieter Nützel from comment #8)
> Created attachment 97486 [details]
> gl-320-primitive-shading-brocken.png

This one WORKS with Mesa 11.0.6 (and git) on RV730 (AGP), now.
Was a driver bug?!

Could be CLOSED.