Bug 97232 - Line rendering broken in Dolphin when using gl_ClipDistance
Summary: Line rendering broken in Dolphin when using gl_ClipDistance
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Kenneth Graunke
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-07 00:18 UTC by Jules Blok
Modified: 2016-12-06 20:39 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
apitrace file (228.56 KB, application/x-7z-compressed)
2016-08-07 00:18 UTC, Jules Blok
Details
apitrace file llvmpipe (314.75 KB, application/x-7z-compressed)
2016-08-10 15:32 UTC, Jules Blok
Details
apitrace file i965 (197.06 KB, application/x-7z-compressed)
2016-08-11 19:07 UTC, Jules Blok
Details
apitrace file i965 (149.39 KB, application/x-xz)
2016-08-11 20:39 UTC, Jules Blok
Details
apitrace file i965 (149.39 KB, application/x-xz)
2016-08-11 20:46 UTC, Jules Blok
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jules Blok 2016-08-07 00:18:01 UTC
Created attachment 125581 [details]
apitrace file

We've added a user-defined clipping plane (gl_ClipDistance) to our vertex shader, unfortunately this breaks our geometry shaders on the Intel drivers. It seems that the software rasterizer (llvm) does handle this case correctly.

I've attached an apitrace to reproduce the problem.

Expected output: https://fifoci.dolphin-emu.org/media/results/eae81c0cc9421e4b98a35024ee1a9a471cf1e7ca.png

Actual output: https://fifoci.dolphin-emu.org/media/results/44bba96a45cf665df718f81ea48f867e174999da.png

You can also take a look at this issue on our automated testing system: 
https://fifoci.dolphin-emu.org/compare/1931322-1924567/

And here is the relevant PR where we are running into this issue: https://github.com/dolphin-emu/dolphin/pull/4085
Comment 1 Ilia Mirkin 2016-08-08 04:49:21 UTC
I played around with this on gen6. Unfortunately the attached trace is unusable there due to the #version 400 selection, however building dolphin from the referenced PR was able to reproduce the issue.

Not sure if it's related, but I'm seeing a read-out-of-bounds when running valgrind on a trace generated on gen6:

==18850== Memcheck, a memory error detector
==18850== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==18850== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==18850== Command: glretrace dolphin-emu-nogui.trace
==18850== 
==18850== Invalid read of size 4
==18850==    at 0xA687DEA: brw::vec4_visitor::var_range_end(unsigned int, unsigned int) const (brw_vec4_live_variables.cpp:335)
==18850==    by 0xA67B77C: brw::vec4_visitor::opt_cse_local(bblock_t*) (brw_vec4_cse.cpp:249)
==18850==    by 0xA67BD7A: brw::vec4_visitor::opt_cse() (brw_vec4_cse.cpp:274)
==18850==    by 0xA6779A5: brw::vec4_visitor::run() [clone .part.22] (brw_vec4.cpp:1988)
==18850==    by 0xA678677: brw_compile_vs (brw_vec4.cpp:2173)
==18850==    by 0xA5A85B5: brw_codegen_vs_prog (brw_vs.c:194)
==18850==    by 0xA5A8BDD: brw_vs_precompile (brw_vs.c:405)
==18850==    by 0xA5909B9: brw_shader_precompile (brw_link.cpp:65)
==18850==    by 0xA5909B9: brw_link_shader (brw_link.cpp:283)
==18850==    by 0xA41F715: _mesa_glsl_link_shader (ir_to_mesa.cpp:3070)
==18850==    by 0xA31F9AD: _mesa_link_program.part.22 (shaderapi.c:1093)
==18850==    by 0x58163D: retrace_glLinkProgram(trace::Call&) (in /usr/bin/glretrace)
==18850==    by 0x40CD18: retrace::retraceCall(trace::Call*) (in /usr/bin/glretrace)
==18850==  Address 0xd190280 is 0 bytes after a block of size 320 alloc'd
==18850==    at 0x4C2C070: calloc (vg_replace_malloc.c:623)
==18850==    by 0xA4C6E61: rzalloc_size (ralloc.c:125)
==18850==    by 0xA6878EC: brw::vec4_visitor::calculate_live_intervals() (brw_vec4_live_variables.cpp:242)
==18850==    by 0xA67BD5D: brw::vec4_visitor::opt_cse() (brw_vec4_cse.cpp:271)
==18850==    by 0xA6779A5: brw::vec4_visitor::run() [clone .part.22] (brw_vec4.cpp:1988)
==18850==    by 0xA678677: brw_compile_vs (brw_vec4.cpp:2173)
==18850==    by 0xA5A85B5: brw_codegen_vs_prog (brw_vs.c:194)
==18850==    by 0xA5A8BDD: brw_vs_precompile (brw_vs.c:405)
==18850==    by 0xA5909B9: brw_shader_precompile (brw_link.cpp:65)
==18850==    by 0xA5909B9: brw_link_shader (brw_link.cpp:283)
==18850==    by 0xA41F715: _mesa_glsl_link_shader (ir_to_mesa.cpp:3070)
==18850==    by 0xA31F9AD: _mesa_link_program.part.22 (shaderapi.c:1093)
==18850==    by 0x58163D: retrace_glLinkProgram(trace::Call&) (in /usr/bin/glretrace)

However commenting out opt_cse does not resolve the (underlying) issue.

A separate thought is that only gl_ClipDistance[0] is written, and there is some indication that if any of the clip distances are NaN, the whole primitive gets rejected. However also writing distances 1,2,3 didn't help, nor did writing 4..7. (Although it did appear to stop flickering a wrong primitive for one frame in that situation... all black now.)
Comment 2 Jules Blok 2016-08-08 18:25:36 UTC
(In reply to Ilia Mirkin from comment #1)
> A separate thought is that only gl_ClipDistance[0] is written, and there is
> some indication that if any of the clip distances are NaN, the whole
> primitive gets rejected. However also writing distances 1,2,3 didn't help,
> nor did writing 4..7. (Although it did appear to stop flickering a wrong
> primitive for one frame in that situation... all black now.)

I've only enabled the first clipping plane. According to the specs: https://www.opengl.org/sdk/docs/man/html/gl_ClipDistance.xhtml

    Values written into gl_ClipDistance planes that are not enabled have no effect.

So I highly doubt this is affecting anything, unless the spec is not implemented correctly.
Comment 3 Ilia Mirkin 2016-08-09 01:28:11 UTC
(In reply to Jules Blok from comment #2)
> So I highly doubt this is affecting anything, unless the spec is not
> implemented correctly.

It's been known to happen :)
Comment 4 Ilia Mirkin 2016-08-10 02:31:10 UTC
Taking a closer look, it appears that you write gl_ClipDistance[0] in the vertex shader but not in the geometry shader. I don't think that's supposed to work -- you have to write the clip distance in the last pre-rast stage in order for it to have any effect.
Comment 5 Ilia Mirkin 2016-08-10 03:29:17 UTC
Curiously, with the below patch on top of what's in the dolphin pr, that still fails on i965/snb, and still passes with llvmpipe.

diff --git a/Source/Core/VideoCommon/GeometryShaderGen.cpp b/Source/Core/VideoCommon/GeometryShaderGen.cpp
index 4d90e6f..3cbe9ce 100644
--- a/Source/Core/VideoCommon/GeometryShaderGen.cpp
+++ b/Source/Core/VideoCommon/GeometryShaderGen.cpp
@@ -311,6 +311,9 @@ static void EmitVertex(ShaderCode& out, const geometry_shader_uid_data* uid_data
 
   if (ApiType == APIType::OpenGL)
   {
+    if (g_ActiveConfig.backend_info.bSupportsDepthClamp)
+      out.Write("\tgl_ClipDistance[0] = %s.clipDist;\n", vertex);
+
     out.Write("\tgl_Position = %s.pos;\n", vertex);
     AssignVSOutputMembers(out, "ps", vertex, uid_data->numTexGens, uid_data->pixel_lighting);
   }
Comment 6 Jules Blok 2016-08-10 14:02:13 UTC
(In reply to Ilia Mirkin from comment #5)
> Curiously, with the below patch on top of what's in the dolphin pr, that
> still fails on i965/snb, and still passes with llvmpipe.

I should've mentioned that I've already tried that in the past, it didn't help.
Comment 7 Jules Blok 2016-08-10 15:32:42 UTC
Created attachment 125672 [details]
apitrace file llvmpipe

I've attached a new trace file captured with llvmpipe instead of the nvidia proprietary driver.
Comment 8 Jules Blok 2016-08-10 15:35:50 UTC
In fact, the first capture was done on Windows, not the nvidia driver on Linux.
Comment 9 Ilia Mirkin 2016-08-11 14:46:31 UTC
(In reply to Jules Blok from comment #6)
> (In reply to Ilia Mirkin from comment #5)
> > Curiously, with the below patch on top of what's in the dolphin pr, that
> > still fails on i965/snb, and still passes with llvmpipe.
> 
> I should've mentioned that I've already tried that in the past, it didn't
> help.

Irrespective of that, the current logic in the PR in question is strictly wrong. The fact that it renders anything anywhere when a GS is active is relying on undefined behavior (since clip distance 0 is enabled, and nothing is written to it from the last stage).
Comment 10 Jules Blok 2016-08-11 15:06:32 UTC
I wasn't sure whether I had to pass through the clipping distance, the spec wasn't very clear on that. I've changed the shader according to your patch to conform to that.
Comment 11 Ilia Mirkin 2016-08-11 15:42:05 UTC
(In reply to Ilia Mirkin from comment #9)
> Irrespective of that, the current logic in the PR in question is strictly
> wrong. The fact that it renders anything anywhere when a GS is active is
> relying on undefined behavior (since clip distance 0 is enabled, and nothing
> is written to it from the last stage).

FTR, looks like the most recent version of the PR has fixed that.
Comment 12 Ilia Mirkin 2016-08-11 15:43:13 UTC
(In reply to Jules Blok from comment #10)
> I wasn't sure whether I had to pass through the clipping distance, the spec
> wasn't very clear on that. I've changed the shader according to your patch
> to conform to that.

Well, what would the clip distance value be if you didn't? You're generating any number of fresh vertices, and the clip distance is a per-vertex attrib. There's no real other way for it to work.
Comment 13 Jules Blok 2016-08-11 16:41:40 UTC
(In reply to Ilia Mirkin from comment #12)
> Well, what would the clip distance value be if you didn't? You're generating
> any number of fresh vertices, and the clip distance is a per-vertex attrib.
> There's no real other way for it to work.

I assumed that, since I didn't actually change gl_ClipDistance in the geom shader, it would just continue to use the value I had already set on the input vertex. And it looks like most drivers do handle it like that, but as you said that is likely undefined behavior.
Comment 14 Jules Blok 2016-08-11 16:44:45 UTC
In fact now that I read the reference again, that is explicitly undefined behaviour:

> It is also undefined after the geometry processing stage if the geometry shader executable calls EmitVertex without having written gl_ClipDistance since the last call to EmitVertex (or hasn't written it at all).
Comment 15 Jules Blok 2016-08-11 19:07:30 UTC
Created attachment 125713 [details]
apitrace file i965

Ilia Mirkin found some application bugs, these have been fixed and a new trace has been recorded on i965. It does not fix the issue however.

Through some testing we've found that merely writing to gl_ClipDistance will break the geometry shader. Even when GL_CLIP_DISTANCE0 has been disabled and such writes should be ignored according to the spec.
Comment 16 Jules Blok 2016-08-11 20:39:06 UTC
Created attachment 125715 [details]
apitrace file i965

This trace file was captured with the latest apitrace version.
Comment 17 Jules Blok 2016-08-11 20:46:54 UTC
Created attachment 125716 [details]
apitrace file i965

Looks like the previous file got corrupted somehow.
Comment 18 Ilia Mirkin 2016-08-17 18:25:04 UTC
A curious observation:

tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test

passes on intel. With the following diff:

diff --git a/tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test b/tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test
index 26ea096..46b09be 100644
--- a/tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test
+++ b/tests/spec/glsl-1.50/execution/geometry/clip-distance-out-values.shader_test
@@ -11,8 +11,12 @@ GLSL >= 1.50
 [vertex shader]
 #version 150
 
+out vec4 c;
+
 void main()
 {
+  //gl_ClipDistance[0] = -1;
+  c = vec4(0,1,0,1);
 }
 
 [geometry shader]
@@ -20,7 +24,10 @@ void main()
 
 layout(points) in;
 layout(triangle_strip, max_vertices = 4) out;
-float gl_ClipDistance[8];
+out float gl_ClipDistance[8];
+
+in vec4 c[];
+out vec4 color;
 
 struct vertex_data {
   vec4 position;
@@ -43,16 +50,20 @@ void main()
   for (int i = 0; i < 4; i++) {
     gl_Position = vdata[i].position;
     gl_ClipDistance = vdata[i].clip_distance;
+    color = c[0];
     EmitVertex();
   }
+  EndPrimitive();
 }
 
 [fragment shader]
 #version 150
 
+in vec4 color;
+
 void main()
 {
-  gl_FragColor = vec4(1.0);
+  gl_FragColor = color;
 }
 
 [test]


it still passes. (Well, it fails, because I wrote in a different color, but that's a side issue.)

However uncommenting the gl_ClipDistance[] in the vertex shader makes it render all black. Using gl_ClipDistance[0] = 1 makes it render red. So the VUE map somehow gets messed up when the vertex shader is emitting a clip distance, but that clip distance isn't used due to there being a TES or GS stage in the pipeline.
Comment 19 Jules Blok 2016-08-17 18:56:22 UTC
> but that clip distance isn't used due to there being a TES or GS stage in the pipeline.

So a possible workaround for this issue is to not write to the clip distance in the vertex shader when a GS stage is active?
Comment 20 Ilia Mirkin 2016-08-17 19:16:46 UTC
VS Output VUE map (4 slots, non-SSO)
  [0] VARYING_SLOT_PSIZ
  [1] VARYING_SLOT_POS
  [2] VARYING_SLOT_CLIP_DIST0
  [3] VARYING_SLOT_VAR0

GS Input VUE map (3 slots, non-SSO)
  [0] VARYING_SLOT_PSIZ
  [1] VARYING_SLOT_POS
  [2] VARYING_SLOT_VAR0

Probably not a great sign. [And yes, avoiding setting gl_ClipDistance in VS when there's a GS present would probably work around this bug.]
Comment 21 Kenneth Graunke 2016-08-17 23:26:30 UTC
Thanks for tracking this down, Ilia - the problem became really obvious once you pointed out that it's a VS -> GS varying problem.  I have some fixes locally - I'm testing them out now.
Comment 22 Ilia Mirkin 2016-09-18 19:30:06 UTC
(In reply to Kenneth Graunke from comment #21)
> Thanks for tracking this down, Ilia - the problem became really obvious once
> you pointed out that it's a VS -> GS varying problem.  I have some fixes
> locally - I'm testing them out now.

I'm guessing that the latest iteration of those is at

https://cgit.freedesktop.org/~kwg/mesa/log/?h=vuemap-fixes

Do they need more work, or did this issue just fall off your radar?
Comment 23 Kenneth Graunke 2016-12-05 09:10:11 UTC
Yeah, those patches are broken.

I took another stab at this tonight, and came up with a different approach I think should work better.  Hopefully...
Comment 24 Kenneth Graunke 2016-12-06 20:39:22 UTC
Fixed by:

commit 44fd85d8eb1fba68829917c0cf5ce052095964ee
Author: Kenneth Graunke <kenneth@whitecape.org>
Date:   Sun Dec 4 23:54:48 2016 -0800

    i965: Unify shader interfaces explicitly.


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.