Bug 85376

Here's a discussion thread on the dolphin forums:

https://forums.dolphin-emu.org/Thread-everything-is-the-wrong-color-on-linux-with-open-source-amd-drivers?pid=340104#pid340104

And the change in dolphin which probably caused the behaviour to change: https://github.com/dolphin-emu/dolphin/commit/a9a8c730748b8c8a2e83feb4c38626361cf9daa1

Created attachment 109230 [details]
Apitrace of the faulty fifo

Created attachment 109231 [details]
Result of  R600_DEBUG=ps,fs apitrace replay dolphin-emu.2.trace

The affected shader seems to be the #22 one, but it's hard to guess what's wrong with the shader http://pastie.org/private/ccwhydvtgzw0rc1t8xdeoq

As abstract of this dolphin commit, it's basicly a rewrite of our fragment shaders with integer math, so I doubt it's useful to debug in this direction :/

About this shader, I see two uncommon features: We write to gl_FragDepth and we use a bitshift with an offset from an integer ubo. Maybe I'm lucky ...

About the issue itself, it only happens on *some* GPUs, so maybe a kind of shader compilation issue?

Created attachment 109239 [details]
apitrace with MESA_EXTENSION_OVERRIDE=-GL_ARB_buffer_storage

Created attachment 109245 [details]
rsps_edit6

Created attachment 109247 [details]
psfs4

Created attachment 109289 [details]
psfs results of edit7

Created attachment 109303 [details]
psfs results of edit9

Some observations (from IRC, edited):

In shader 21 (from attachment 109303 [details]), we see the following:

<imirkin_> MULLO_INT          R5.x,  [0x00000046 9.80909e-44].x, PV.x
<imirkin_> MULLO_INT          R5.y,  [0x00000078 1.68156e-43].x, PV.x
<imirkin_> that seems wrong
<imirkin_> the second PV.x uses the newly computed R5.x, no?
<imirkin_> instead of the previous R5.x before the mul happened
<imirkin_> so the 2nd and 3rd components get the compounded multiplication factor of .x and the respective .y and .z factor
<tstellar> Starting with the R5.x value returned by the ADD_INT, the caluclation is: R5.y = ((R5.x *46) * 78); R5.z = (R5.x * 46) * 0xc8
<imirkin_>   6: UMUL TEMP[1].xyz, IMM[0].yzww, TEMP[1].xxxx
<imirkin_> i'm guessing the thing that splits it up into 3 instructions forgets that it's overwriting the source
<imirkin_> cayman_mul_int_instr
<imirkin_> cayman-specific, and doesn't do the tmp register dance

vs the "regular" umul implementation (op2_trans) which will use a temp register if the dst mask has multiple dests. I wonder if all the cayman_* emit functions need this treatment.

Created attachment 109308 [details]
Proposed fix

Does the attached patch fix the issue?

Created attachment 109371 [details]
psfs output on edit9 with glennks patch

The attached patch did not help :(

Created attachment 109650 [details] [review]
patch to hopefully fix

Not sure if this is fixed or not, but I was able to isolate the GLSL where things go sideways.

This is broken:

    prev.rgb = (prev.rgb * (256 - ifog) + cfogcolor.rgb * ifog) >> 8;

The above apparently should have the same effect as the following, which works correctly:

    prev.r = (prev.r * (256 - ifog) + cfogcolor.r * ifog) >> 8;
    prev.g = (prev.g * (256 - ifog) + cfogcolor.g * ifog) >> 8;
    prev.b = (prev.b * (256 - ifog) + cfogcolor.b * ifog) >> 8;

So something is going sideways distributing the above operations across vector components. Seems reasonable to suspect code generation.

I tried splitting just the bitshift step with no effect, so my guess is that at least multiplying an int3 by an int is doing something wrong.

After some patches, dolphin is no segfaulting instead of showing the wong colors. I'm uploading a gdb-log and an apitrace.

Created attachment 109838 [details]
Apitrace of dolphin crashing while running the fifo posted above

Created attachment 109839 [details]
gdb output from crash in dolphin

The crash was due to bad build options on my part. Fix seems to be good!

Resolving per comment #19 (you can do this yourself BTW :).