Bug 70009

First apology for accidentally pushing Enter too early.

Now to the bug... Wine is known to produce some invalid shaders on old cards. (see bug 42514 or http://bugs.winehq.org/show_bug.cgi?id=29146)  This is because wine need some uniform components for internal fixups and when the shader already uses all of them, it generates incorrect shaders. On my old RV530 with r300g driver this usually resulted in using dummy shader instead of the failing one (and hence missing some game object or effects), however except from the few missing things, the apps were mostly working fine.

However, starting with 6a2baf3a06125405aa648e208af782e53f1312c0 , there is major random color corruption in affected apps (specifically this was tested with EVE online).

6a2baf3a06125405aa648e208af782e53f1312c0 is the first bad commit
commit 6a2baf3a06125405aa648e208af782e53f1312c0
Author: Paul Berry <stereotype441@gmail.com>
Date:   Mon Jun 10 14:01:45 2013 -0700

    glsl/linker: Make update_array_sizes apply to just uniforms.
    
    Commit 586b4b5 (glsl: Also update implicit sizes of varyings at link
    time) extended update_array_sizes() to apply to both uniforms and
    shader ins/outs.  However, doing creates problems for geometry
    shaders, because update_array_sizes() assumes that variables with
    matching names in different parts of the pipeline should have the same
    sizes.  With the addition of geometry shaders, this is no longer true
    (e.g. both vertex and geometry shaders have a gl_ClipDistance output
    variable, but there's no reason these variables should have the same
    sizes).
    
    The original reason for commit 586b4b5 (avoid problems with
    gl_TexCoord being 0 length) has since been addressed by commit 6f53921
    (linker: Ensure that unsized arrays have a size after linking).  So go
    ahead and switch update_array_sizes() back to only acting on uniforms.
    
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>


After commit 98d2498404ba69a3efc1c765b1a1885d151181ed it changed to mostly black

98d2498404ba69a3efc1c765b1a1885d151181ed is the first bad commit
commit 98d2498404ba69a3efc1c765b1a1885d151181ed
Author: Paul Berry <stereotype441@gmail.com>
Date:   Fri Aug 9 07:58:43 2013 -0700

    glsl: Fix incorrect pattern matching in ir_set_program_inouts
    
    In commit 8fc41df (glsl: Modify ir_set_program_inouts to handle
    geometry shaders), when attempting to pattern match the "foo" part of
    expressions such as:
    
       foo[i][j]
       foo[i]
    
    I incorrectly called as_dereference_variable() on the subexpression
    foo[i] instead of foo.  As a result, the pattern never matched, so
    ir_set_program_inouts would fall back on marking the entire variable
    as used, rather than just the portion indexed by the array.
    
    This didn't result in incorrect behaviour, but it could have resulted
    in inefficiency by causing the back-end to allocate resources for
    unused parts of an input or output array.
    
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

It would be nice if we could go back to the status before, eg. "ignoring the bad shaders" and rendering everything else.

So is seems, that when I record a trace using r300g driver (RV530) and replay it using either llvmpipe or i965, it works fine. Probably this is just some issue in the r300g driver, uncovered by core mesa changes...
EVE online apitrace uploaded here: http://pavel.ondracka.cz/EVE.trace

(In reply to comment #2)
> So is seems, that when I record a trace using r300g driver (RV530) and
> replay it using either llvmpipe or i965, it works fine. Probably this is
> just some issue in the r300g driver, uncovered by core mesa changes...
> EVE online apitrace uploaded here: http://pavel.ondracka.cz/EVE.trace

llvmpipe and i965 support more temps than r300 class hw so they don't cross the limit.

(In reply to comment #3)
> (In reply to comment #2)
> > So is seems, that when I record a trace using r300g driver (RV530) and
> > replay it using either llvmpipe or i965, it works fine. Probably this is
> > just some issue in the r300g driver, uncovered by core mesa changes...
> > EVE online apitrace uploaded here: http://pavel.ondracka.cz/EVE.trace
> 
> llvmpipe and i965 support more temps than r300 class hw so they don't cross
> the limit.

Yeah, thats why they show no errors and promblems when launching EVE directly, because wine detects this and generate valid shaders...

However the main problem is a little bit different. Citing one sample shader from bug 42514:
The shader declares the array VC to have 245 elements:
uniform vec4 VC[245];
then it tries to access the 246th element of the array:
R0.xyz = (VC[245].xyz);


Also citing from wine bug 29146:
Yeah, we should be better about this. The reason we don't already do this is
that a shader like this may still work if it doesn't actually use the constants
near the end that we remove. We should probably print a message mentioning
something along those lines when such a shader fails to compile or link.

So when i generate the apitrace with r300g in already contains the invalid shaders, however llvmpipe and softpipe still handle things correctly (e.g. display majority of stuff) and also r300g did so before the mentioned bad commit.

Do particular shaders fail to compile or is there just incorrect rendering?

Created attachment 87014 [details]
EVE online Wine terminal output

(In reply to comment #5)
> Do particular shaders fail to compile or is there just incorrect rendering?

Yeah, there is a particular failing shader, however it fails to compile and link even before the patch, its just that the patch introduced also a bad rendering (but only for r300g). 

I've attached the wine log, which shows the failing shader, however I'm starting to believe, that it may not be the root cause here.

Created attachment 87143 [details] [review]
possible fix

Could you please try the attached patch?

(In reply to comment #7)
> Created attachment 87143 [details] [review] [review]
> possible fix
> 
> Could you please try the attached patch?

The attached patch doesn't help.

Sorry I'm out of ideas. Can the regression be reproduced by some existing piglit test maybe?

(In reply to comment #9)
> Sorry I'm out of ideas. Can the regression be reproduced by some existing
> piglit test maybe?

I'll run a piglit before and after the change... BTW you can't reproduce this with the http://pavel.ondracka.cz/EVE.trace trace?

(In reply to comment #9)
> Sorry I'm out of ideas. Can the regression be reproduced by some existing
> piglit test maybe?

Sadly there are no changes in quick piglit test, except for glsl-fs-discard-02 and  glx-copy-sub-buffer samples=4 and they both pass when I run the manually.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/357.