Bug 42032

Summary:	Commit 29361e14 causes white textures in Civ V under wine
Product:	Mesa	Reporter:	Christopher James Halse Rogers <chalserogers>
Component:	Drivers/DRI/i965	Assignee:	Kenneth Graunke <kenneth>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	medium	CC:	eric, eugeni, idr, kenneth, mike
Version:	git
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:

Description Christopher James Halse Rogers 2011-10-20 00:12:53 UTC

Civ V under wine (mostly) works with mesa 7.11 when floating point textures are enabled, but with 7.12 git (up to 35600279, Oct 20 checked) the world textures are rendered all white.  I've bisected this to find that the following commit is what broke it:

commit 29361e14df8e5e92df747d52303da2c454e2cacc
Author: Eric Anholt <eric@anholt.net>
Date:   Thu Sep 1 16:43:38 2011 -0700

    i965/vs: Allow copy propagation on GRFs.
    
    Further reduces instruction count by 4.0% in 40.7% of the vertex
    shaders.

diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
index 7862d78..c46735a 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
@@ -187,6 +187,7 @@ try_copy_propagation(struct intel_context *intel,
    value.swizzle = BRW_SWIZZLE4(s[0], s[1], s[2], s[3]);
 
    if (value.file != UNIFORM &&
+       value.file != GRF &&
        value.file != ATTR)
       return false;
 
@@ -204,6 +205,10 @@ try_copy_propagation(struct intel_context *intel,
    if (intel->gen >= 6 && inst->is_math())
       return false;
 
+   /* Don't report progress if this is a noop. */
+   if (value.equals(&inst->src[arg]))
+      return false;
+
    inst->src[arg] = value;
    return true;
 }
@@ -307,7 +312,7 @@ vec4_visitor::opt_copy_propagation()
                      cur_value[i][j] &&
                      cur_value[i][j]->file == GRF &&
                      cur_value[i][j]->reg == inst->dst.reg &&
-                     cur_value[i][j]->reg == inst->dst.reg) {
+                     cur_value[i][j]->reg_offset == inst->dst.reg_offset) {
                     cur_value[i][j] = NULL;
                  }
               }


The Civ V world textures use floating point buffers; I'd guess this is why it's not been picked up yet.  Reverting 29361e14df8e5e92df747d52303da2c454e2cacc on git resolves this problem.

Comment 1 Kenneth Graunke 2011-12-23 20:57:50 UTC

Proposed patch series sent to mailing list and to the reporter.  I've tested it with another application that exhibits similar issues and works with the same patch reverted, but I haven't tested Civilization V itself.

Comment 2 Kenneth Graunke 2011-12-27 14:33:51 UTC

Thanks for the report and for testing the proposed patches!  This is now fixed in master:

commit 07ee9f374f2946f852896e9264c7fa83eafc3f16
Author: Kenneth Graunke <kenneth@whitecape.org>
Date:   Fri Dec 23 20:24:46 2011 -0800

    i965/vs: Properly clear cur_value when propagating direct copies.
    
    Consider the following code:
    
    MOV A.x, B.x
    MOV B.x, C.x
    
    After the first line, cur_value[A][0] == B, indicating that A.x's
    current value came from register B.
    
    When processing the second line, we update cur_value[B][0] to C.
    However, for drect copies, we fail to reset cur_value[A][0] to NULL.
    This is necessary because the value of A is no longer the value of B.
    
    Fixes Counter-Strike: Source in Wine (where the menu rendered completely
    black in DX9 mode), completely white textures in Civilization V, and the
    new Piglit test glsl-vs-copy-propagation-1.shader_test.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42032
    Tested-by: Matt Turner <mattst88@gmail.com>
    Tested-by: Christopher James Halse Rogers <christopher.halse.rogers@canonica
    Reviewed-by: Eric Anholt <eric@anholt.net>
    Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>

commit 443c8d1ab7ddad9392046e041e4e9a4fda7cd6e7
Author: Kenneth Graunke <kenneth@whitecape.org>
Date:   Fri Dec 23 19:57:08 2011 -0800

    i965/vs: Fix incorrect subscript when resetting copy propagation records.
    
    In this code, 'i' loops over the number of virtual GRFs, while 'j' loops
    over the number of vector components (0 <= j <= 3).
    
    It can't possibly be correct to see if bit 'i' is set in the destination
    writemask, as it will have values much larger than 3.  Clearly this is
    supposed to be 'j'.
    
    Found by inspection.
    
    Tested-by: Matt Turner <mattst88@gmail.com>
    Tested-by: Christopher James Halse Rogers <christopher.halse.rogers@canonica
    Reviewed-by: Eric Anholt <eric@anholt.net>
    Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>

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.