Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulkan: Workaround Adreno discard bug #11197

Merged
merged 1 commit into from
Jun 20, 2018

Conversation

unknownbrackets
Copy link
Collaborator

Like #11192, but applied to regular fragment shaders too. However, I did some more testing: it still works with depth_unchanged (ExecutionModeDepthUnchanged) specified. I think that should prevent any performance impact over and above already using discard.

I updated stencil to match, although it shouldn't matter since it doesn't use depth. Just for consistency.

At first, I planned to do this for Adreno only, but I found a MESA bug that was exactly the same. Now I'm expecting other mobile drivers did the wrong thing too. We can easily restrict it gl_extensions.bugs if we discover performance impact.

On the topic of bad drivers, the spec says (emphasis mine):

Vulkan - 25.4
The depth bounds test, stencil test, depth test, and occlusion query sample counting are performed before fragment shading if and only if early fragment tests are enabled by the fragment shader (see Early Fragment Tests).

I realize this probably only specifies apparent behavior, but I figured it can't hurt to assert early_fragment_tests since we're detecting the opposite anyway.

-[Unknown]

This also explicitly enables early fragment tests when possible.  Using
conversative depth still works on Adreno and should allow some depth
optimizations.
@hrydgard
Copy link
Owner

hrydgard commented Jun 20, 2018

Wow, that's an interesting find that the bug is so widespread.

Hopefully those shader compilers won't ever decide that gl_FragDepth = gl_FragCoord.z is a noop and take it out before the other checks they do :)

@hrydgard hrydgard merged commit 06340bf into hrydgard:master Jun 20, 2018
@unknownbrackets
Copy link
Collaborator Author

Hopefully, but at least with Vulkan we control the compiler... it will output ExecutionModeDepthReplacing in addition to ExecutionModeDepthUnchanged, afaiu.

-[Unknown]

@unknownbrackets unknownbrackets deleted the vulkan-earlyfrag branch June 20, 2018 07:39
@unknownbrackets
Copy link
Collaborator Author

Oh, and just to clarify: it seems the MESA bug is fixed now, it just existed in a previous version. Still, it suggests it's a common mistake indeed.

-[Unknown]

@hrydgard
Copy link
Owner

Oh, good that the decision of the execution mode is encoded in the SPIR-V. Because technically we of course only control the frontend, after SPIR-V there'll always be another compiler or two before the code hits the shader execution units of the GPU :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants