Bug 89224

Summary: Incorrect rendering of Unigine Valley running in VM on VMware Workstation
Product: Mesa Reporter: Andrey Sudnik <andrey.sudnik>
Component: Drivers/DRI/i965Assignee: Matt Turner <mattst88>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: medium CC: abdiel.janulgue, andrey.sudnik, idr
Version: 10.4   
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
i915 platform: i915 features:
Attachments: Unigine Valley Mesa-10.4
Unigine Valley Mesa-10.4
Valley patch for mesa-10.4

Description Andrey Sudnik 2015-02-19 10:48:21 UTC
Created attachment 113657 [details]
Unigine Valley Mesa-10.4

System Environment:
 chipset: HSW
 system architecture: x86-64
 xf86-video-intel: 2.99.914-1
 xserver: 1.16.0-1
 mesa: 10.4-devel, git id: 7841a246b93d8d8aeb65df5805c0e9d05567c57e
 libdrm: 2.4.56-1
 kernel: 3.16.0
 OS: Ubuntu Linux 14.10

Bug detailed description:
I run Unigine Valley benchmark in Windows 7 VM that works on Linux Workstation 11.0.0.
After shifting to Mesa 10.4 Unigine Valley image become corrupted.

I’ve found guilty commit, it is 7841a246b93d8d8aeb65df5805c0e9d05567c57e:
2014-08-31 7841a24 i965/vec4: Allow propagation of instructions with saturate flag to sel, Abdiel Janulgue <abdiel.janulgue@linux.intel.com>

Mesa built from previous commit 40aeb558ce8a7ffaaa6f81be16419b9b238c16d8 renders good picture.
Comment 1 Andrey Sudnik 2015-02-19 10:57:25 UTC
Created attachment 113658 [details]
Unigine Valley Mesa-10.4

Attached JPEG image with Unigine Valley incorrect rendering.
Comment 2 Andrey Sudnik 2015-02-19 11:08:24 UTC
Checked with Mesa-10.5rc1. The issue is reproducible.
Comment 3 Andrey Sudnik 2015-02-19 12:56:08 UTC
I found quick fix, but not sure it is completely correct:

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 6769310..2ed9ce1 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
@@ -47,6 +47,7 @@ is_direct_copy(vec4_instruction *inst)
    return (inst->opcode == BRW_OPCODE_MOV &&
           !inst->predicate &&
           inst->dst.file == GRF &&
+           !inst->saturate &&
           !inst->dst.reladdr &&
           !inst->src[0].reladdr &&
           inst->dst.type == inst->src[0].type);
Comment 4 Ian Romanick 2015-02-19 19:05:21 UTC
Abdiel, can you look at this?  It would be good if a fix for this could land in time for the 10.5 release.
Comment 5 Andrey Sudnik 2015-02-20 07:03:58 UTC
Created attachment 113683 [details] [review]
Valley patch for mesa-10.4

This patch looks more accurately.
Comment 6 Matt Turner 2015-02-26 18:29:26 UTC
I'll look into this.
Comment 7 Matt Turner 2015-03-05 19:42:18 UTC
(In reply to Andrey Sudnik from comment #5)
> Created attachment 113683 [details] [review] [review]
> Valley patch for mesa-10.4
> This patch looks more accurately.

I wrote a small vertex shader to reproduce this:

in vec4 vertex;
out vec4 color;

uniform vec4 a, b;

void main() {
	gl_Position = vertex;
	color.xyz = clamp(a.xyz, 0.0, 1.0);
	color.w = a.w;

Without your patch, we simply drop the saturate in copy propagation. I need to come up with a way of testing this other than inspecting the disassembly.

I've sent your patch to the mailing list for review. Thanks!
Comment 8 Matt Turner 2015-03-05 23:48:34 UTC
Thanks for the patch. I've committed it as

commit 0dfec59a2785cf7a87ee5128889ecebe810b611b
Author: Andrey Sudnik <andrey.sudnik@intel.com>
Date:   Thu Mar 5 11:16:49 2015 -0800

    i965/vec4: Don't lose the saturate modifier in copy propagation.
    Cc: 10.4, 10.5 <mesa-stable@lists.freedesktop.org>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89224
    Reviewed-by: Matt Turner <mattst88@gmail.com>
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

It should end up in the 10.4.6 and 10.5.0 releases.
Comment 9 Andrey Sudnik 2015-03-06 04:32:22 UTC
Thank you Matt!

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.