Bug 106452 - radv: Early discard in fragment shader breaks derivatives
Summary: radv: Early discard in fragment shader breaks derivatives
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Vulkan/radeon (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-09 09:13 UTC by Philip Rebohle
Modified: 2018-12-04 09:20 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
GLSL fragment shader (9.08 KB, text/plain)
2018-05-09 09:13 UTC, Philip Rebohle
Details
Incorrect rendering (5.71 MB, image/png)
2018-05-09 09:17 UTC, Philip Rebohle
Details
Expected rendering (5.94 MB, image/png)
2018-05-09 09:17 UTC, Philip Rebohle
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rebohle 2018-05-09 09:13:48 UTC
Created attachment 139440 [details]
GLSL fragment shader

Hello,

as mentioned in the title, a 'discard' in a fragment shader can break derivatives used for subsequent texture sample operations, which leads to an incorrect mip level being selected. This causes visual issues in The Witcher 3 with DXVK during LOD transitions, and potentially other games which use a similar LOD system.

Here's a Renderdoc capture that shows the issue (recorded on an RX 480, requires a recent build of the renderdoc v1.x git branch):

https://mega.nz/#!1exzXYaT!jl-JsS7g52enndH6twBwPMSKPReXArkqyFXyoTjvun4

The EIDs of interest are 8645 and 8648. The game is in the process of performing a smooth LOD transition of a mesh by discarding fragments when rendering either version of the mesh.

The attached shader is a modified GLSL version of the fragment shader which produces correct results, it can be used to replace the original shader in Renderdoc. The only change I made is to defer the 'discard' to the end of the shader as documented in the code.
Comment 1 Philip Rebohle 2018-05-09 09:17:02 UTC
Created attachment 139441 [details]
Incorrect rendering
Comment 2 Philip Rebohle 2018-05-09 09:17:41 UTC
Created attachment 139442 [details]
Expected rendering
Comment 3 Philip Rebohle 2018-05-09 09:21:15 UTC
To avoid any confusion: The screenshots show the expected vs. actual contents of the first color attachment at EID 8648 in the Renderdoc capture linked above.
Comment 4 Alex Smith 2018-05-09 09:56:41 UTC
Derivatives are undefined after a discard in non-uniform control flow. SPIR-V's OpKill says "all subsequent control flow is non-uniform", and derivatives are only defined in uniform control flow.

Unfortunately differs from the D3D behaviour. radeonsi has a drirc option "glsl_correct_derivatives_after_discard" to match the D3D behaviour. Would be nice if somehow that behaviour could be exposed on RADV (an extension?) - in some cases the workaround of delaying the discard to the end of the shader can cause a significant perf hit.
Comment 5 Samuel Pitoiset 2018-05-09 10:17:07 UTC
Yes, I can implement a similar behaviour on RADV which can be enabled based on the executable name (and using a debug option).
Comment 6 Philip Rebohle 2018-05-09 10:37:20 UTC
Hm, I wasn't aware of that.

In general I'd prefer a general workaround on my end since other drivers are probably affectes as well, but i'm not sure if that's viable.

Alex Smith: Another issue with moving the discard to the end, besides performance, is that shaders which have side effects through SSBO/Image stores and atomics would require extra care as well.

Samuel Pitoiset: If you choose to implement a workaround, would it be feasible to abuse the info from the VkApplicationInfo structure for that? DXVK always sets pEngineName to "DXVK", and I believe we're hitting this issue in multiple games.
Comment 7 Alex Smith 2018-05-09 10:49:06 UTC
Yup, have to conditionalize any later side effects as well.

A workaround activated based on app name isn't so nice. I don't like the idea of us shipping a game assuming that the driver is going to apply a workaround - it'd be much better if we could explicitly opt-in to the changed behaviour if available (and use the less optimal workaround on our side otherwise), hence my suggestion of an extension.
Comment 8 Samuel Pitoiset 2018-05-09 11:41:10 UTC
I do agree with Alex. Though, as a temporary solution we could force that behaviour in radv when dxvk is detected until we found a better option?
Comment 9 Samuel Pitoiset 2018-12-04 09:20:56 UTC
This has been fixed in DXVK a while ago. Discarding at end of shaders isn't good for performance but we are working on a better solution. Closing.


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.