Bug 76394 - [IVB/HSW] ogl-samples: gl-320-primitive-shading output errors
Summary: [IVB/HSW] ogl-samples: gl-320-primitive-shading output errors
Status: VERIFIED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: medium normal
Assignee: Ian Romanick
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-20 11:31 UTC by meng
Modified: 2015-11-27 03:09 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
render error png (18.68 KB, image/png)
2014-03-20 11:32 UTC, meng
Details
gl-320-primitive-shading-brocken.png (21.13 KB, image/png)
2014-04-17 00:57 UTC, Dieter Nützel
Details

Description meng 2014-03-20 11:31:10 UTC
System Environment:
--------------------------
Mesa:  (master)eaf9affa5ec9c5fd919e4207ab80b4677650ac67
Xserver:(master)xorg-server-1.15.99.901 Xf86_video_intel:	
Xf86_video_intel:(master)2.99.911
Kernel:	(drm-intel-nightly) git-ec45c7

Bug detailed description:
----------------------------
It’s not a regression, firstly test ogl-samples.

Pre-conditions:
 - build: https://github.com/g-truc/ogl-samples

Test-case: gl-320-primitive-shading

Expected outcome:
-----------------
https://github.com/g-truc/ogl-samples/blob/master/data/templates/reference/gl-320-primitive-shading.png

Actual outcome:
------------------------
Output is missing all colors, it's monochrome and flickers really badly. Please see error.png attached.
- "error: length called on unsized array"
- "error: unsized array index must be constant"
Comment 1 meng 2014-03-20 11:32:21 UTC
Created attachment 96095 [details]
render error png
Comment 2 Iago Toral 2014-04-04 08:16:36 UTC
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.
Comment 3 Iago Toral 2014-04-04 10:05:59 UTC
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).
Comment 4 Iago Toral 2014-04-04 13:16:01 UTC
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
Comment 5 Kenneth Graunke 2014-04-16 05:20:16 UTC
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)...
Comment 6 meng 2014-04-16 06:02:09 UTC
The issue that render error is fixed ,but its output errors still exist, so reopen the bug and change the title.
Comment 7 Dieter Nützel 2014-04-17 00:56:39 UTC
(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...
Comment 8 Dieter Nützel 2014-04-17 00:57:36 UTC
Created attachment 97486 [details]
gl-320-primitive-shading-brocken.png
Comment 9 Benjamin Bellec 2014-04-23 21:20:00 UTC
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
Comment 10 Dieter Nützel 2014-08-31 20:57:26 UTC
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
Comment 11 Iago Toral 2014-09-01 06:09:36 UTC
(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
Comment 12 Tapani Pälli 2014-09-15 10:52:54 UTC
(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).
Comment 13 Tapani Pälli 2014-09-16 06:32:07 UTC
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).
Comment 14 meng 2015-05-12 05:52:59 UTC
(In reply to Tapani Pälli from comment #13)
Verified it.
Comment 15 Dieter Nützel 2015-11-27 03:09:46 UTC
(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.


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.