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

Allow skipping draws with broken pipeline variants on Vulkan #5807

Merged
merged 5 commits into from Jan 26, 2024

Conversation

gdkchan
Copy link
Member

@gdkchan gdkchan commented Oct 15, 2023

Currently we skip draws when the background compilation of the shaders on Vulkan failed. This is detected with the result code from the pipeline creation call. If an is error returned, we assume that the host failed to compile the shader that the emulator gave. While this generally works, there is an odd situation on macOS with MoltenVK where some pipeline variants may compile successfully, and others may fail. This means that the emulator can still crash in some cases due to shader compilation failures.

This PR allows draws to be skipped when any pipeline variant creation fails, not just the background one. This is only done for graphics shaders since compute generally doesn't have pipeline variants (since the only pipeline state on compute is the shader itself). This also includes minor improvements to try preventing any performance regression from additional checks. The !_program.IsLinked has been moved to CreatePipeline to avoid checking it all the time. RecreatePipelineIfNeeded has been split in two, one for graphics and other for compute. Most of what it was doing is not really necessary for compute (like updating dynamic state, vertex/index/transform feedback buffers).

Fixes #5790, although it does not fix the underlying issue (the compilation error). The only error that the compiler produces here is "Compiler encountered an internal error.", which does not really give much indication of what is wrong. I was able to track down the specific pipeline state that makes it produce that error, it was ColorBlendAttachmentState, on the background pipeline it was disabled, on the new one it is enabled. There is a single color attachment with VkFormat B10G11R11UfloatPack32.
Captura de Tela 2023-10-15 às 00 20 09
Does not render 100% due to the skipped draw, but it is better than the game crashing.

Testing is welcome, also to ensure that nothing broke on platforms other than macOS.

@github-actions github-actions bot added gpu Related to Ryujinx.Graphics graphics-backend:vulkan Graphical bugs when using the Vulkan API labels Oct 15, 2023
@ryujinx-mako ryujinx-mako bot requested review from marysaka, riperiperi and a team October 15, 2023 03:51
Copy link
Contributor

@marysaka marysaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really not a big fan of this, but it will do for now I guess...

@gdkchan
Copy link
Member Author

gdkchan commented Oct 20, 2023

Really not a big fan of this, but it will do for now I guess...

I guess the title makes it sound like I'm introducing a hack on Vulkan to work around MoltenVK/Metal issues, but this just extends something that is already being done. We already skip draws when shader compilation fails on OpenGL, and do more or less the same on Vulkan, but only when the first, background pipeline creation fails. This is just extending it to work on all pipelines. I would prefer to fix the underlying issue, but I think there is some bug on their compiler since it just reports an "internal compiler error", it might be possible to work around it, but it would require either a lot of trial and error or reversing their compiler (maybe not that hard since it's LLVM based, but still).

@psnluiz
Copy link

psnluiz commented Jan 2, 2024

This fixed Fire Emblem Three Houses crashing for me.

Copy link
Member

@riperiperi riperiperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. What was the expected extra cost of adding this? I can't seem to find anything that would cost too much more.

@gdkchan
Copy link
Member Author

gdkchan commented Jan 26, 2024

Looks good to me. What was the expected extra cost of adding this? I can't seem to find anything that would cost too much more.

There are a few extra checks needed for updating pipelines, but IIRC I tried to make up for it (for example by moving the shader IsLinked check).

@gdkchan gdkchan merged commit b8d992e into Ryujinx:master Jan 26, 2024
9 checks passed
@gdkchan gdkchan deleted the skip-broken-pipe branch January 26, 2024 16:59
amurgshere pushed a commit to amurgshere/Ryujinx that referenced this pull request Jan 28, 2024
…#5807)

* Allow skipping draws with broken pipeline variants on Vulkan

* Move IsLinked check to CreatePipeline

* Restore throw on error behaviour for background compile

* Can't remove SetAlphaTest pragmas yet

* Double new line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gpu Related to Ryujinx.Graphics graphics-backend:vulkan Graphical bugs when using the Vulkan API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fire Emblem: Three Houses Using 'Divine Pulse' ability cause 100% crash on MacOS Sonoma(14.0) M1 Max 64GB.
4 participants