Fix crash on Linux Nvidia 550 driver #12542

eero-lehtinen · 2024-03-17T19:51:57Z

Objective

Fix crashing on Linux with latest stable Nvidia 550 driver when resizing. The crash happens at startup with some setups.

Fixes #12199

I think this would be nice to get into 0.13.1

Solution

Ignore wgpu::SurfaceError::Outdated always on this platform+driver.

It looks like Nvidia considered the previous behaviour of not returning this error a bug:
"Fixed a bug where vkAcquireNextImageKHR() was not returning VK_ERROR_OUT_OF_DATE_KHR when it should with WSI X11 swapchains" (https://www.nvidia.com/Download/driverResults.aspx/218826/en-us/)

What I gather from this is that the surface was outdated on previous drivers too, but they just didn't report it as an error. So behaviour shouldn't change.

In the issue conversation we experimented with calling continue when this error happens, but I found that it results in some small issues like bevy_egui scale not updating with the window sometimes. Just doing nothing seems to work better.

Changelog

Fixed crashing on Linux with Nvidia 550 driver when resizing the window

Migration Guide

github-actions · 2024-03-17T19:52:34Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

eero-lehtinen · 2024-03-17T20:10:21Z

Looks like I'm also undoing a change from #12055, where configure_surface was removed from being called on resize. I don't have an iPhone to test this change with.

crates/bevy_render/src/view/window/mod.rs

eero-lehtinen · 2024-03-17T21:00:58Z

CI timed out but logs say that test was successful. Is this a real error?

WarrenHood · 2024-03-18T00:12:37Z

Why not just always ignore surface outdated errors? Rather than only for linux with nvidia 550 drivers

eero-lehtinen · 2024-03-18T07:50:48Z

Why not just always ignore surface outdated errors? Rather than only for linux with nvidia 550 drivers

The error is not expected anywhere else. But yeah always ignoring it would reduce the amount of code. I'm not opposed to doing it that way.

eero-lehtinen · 2024-03-18T08:10:20Z

CI timed out but logs say that test was successful. Is this a real error?

The error was real but now fixed.

notverymoe · 2024-03-19T03:00:50Z

I'm concerned about specifically checking for the 550 driver. This seems to be a fix by Nvidia, so 560, 570 etc. will contain this behaviour. And given that the error was effectively ignored in versions prior to 550, I think this should be safe to just ignore on Nvidia Linux as a whole?

eero-lehtinen · 2024-03-19T10:31:21Z

I'm concerned about specifically checked for the 550 driver. This seems to be a fix by nvidia, so 560, 570 etc. will contain this behaviour. And given that the error was effectively ignored in versions prior to 550, I think this should be safe to just ignore on Nvidia Linux as a whole?

Yeah it was a panic before so it shouldn't be too bad to ignore it. I'll do that.

torsteingrindvik

Not a windowing expert, but cherry-picking these commits solved various crashes I get since I'm on linux + new Nvidia drivers so I'm very happy with this.

IceSentry · 2024-03-22T20:13:13Z

Why did you remove the not_already_configured related code? It seems unrelated to me?

eero-lehtinen · 2024-03-23T01:31:44Z

Why did you remove the not_already_configured related code? It seems unrelated to me?

After #12055 both branches do the same thing because configure_surface was removed. I didn't want to copy paste my check for both branches, so I removed the first. But I guess it's not this PRs job to fix, so I can also make my changes as minimal as possible.

IQuick143

I can't say anything about the changes themselves as I don't understand windowing code, but testing on my machine I can confirm this does resolve the issue. (Void Linux, X.Org, nvidia-550.54.14_1 driver)

james7132

Silently absorbing the error seems like a future issue waiting to happen. Can we downgrade this to an error or warning log?

eero-lehtinen · 2024-03-24T00:19:03Z

Silently absorbing the error seems like a future issue waiting to happen. Can we downgrade this to an error or warning log?

I added a debug level log because it happens pretty often and there is no way to fix it.

james7132 · 2024-03-24T00:39:36Z

I added a debug level log because it happens pretty often and there is no way to fix it.

An error_once or warn_once should log it once and then black hole the rest.

IceSentry · 2024-03-24T19:13:20Z

crates/bevy_render/src/view/window/mod.rs

+            render_instance
+                .enumerate_adapters(wgpu::Backends::VULKAN)
+                .iter()
+                .any(|adapter| adapter.get_info().name.starts_with("NVIDIA"))


I'm not sure if starts_with() is good enough here. We've had a bug somewhere else because the vendor name wasn't at the start.

In addition to the name field there seems to be driver that just says "NVIDIA" and nothing else. Would that be better? I don't have other cards to test with.

Not sure how quirky these AdapterInfo strings can get but maybe just changing it to name.to_uppercase().contains("NVIDIA") could work. You could also add a fallback check of the driver string to account for cases in which the name would not match on an NVIDIA device, no idea if this would be necessary though. This is out of the scope of this PR but the same starts_with() method is currently used in defining may_erroneously_timeout in the lines above.

I wouldn't add random extra checks if we don't know what edge case we are handling. This has worked fine for everyone who tested it.

Checking for vendor id would probably be the best for reliability.

I can confirm with a little test system that this spams true:
(EDIT: I have an nvidia card of course..)

fn vendor_info_hex(adapter: Res<RenderAdapter>) { let nvidia = 0x10de; let vendor = adapter.get_info().vendor; info!("Is nvidia? {}", vendor == nvidia); }

found the ID value here: https://envytools.readthedocs.io/en/latest/hw/pciid.html#introduction

According to wgpu there might be other bytes non-zero: https://docs.rs/wgpu/latest/wgpu/struct.AdapterInfo.html#structfield.vendor

in the ID, so might need to mask off only the 16 bits we care about

let vendor = adapter.get_info().vendor & 0xFFFF;

Thanks for investigating. I made this change. I just compare to a constant, but an enum based abstraction for the vendor check would be better in the future and should be used everywhere instead of strings.

crates/bevy_render/src/view/window/mod.rs

# Objective Fix crashing on Linux with latest stable Nvidia 550 driver when resizing. The crash happens at startup with some setups. Fixes #12199 I think this would be nice to get into 0.13.1 ## Solution Ignore `wgpu::SurfaceError::Outdated` always on this platform+driver. It looks like Nvidia considered the previous behaviour of not returning this error a bug: "Fixed a bug where vkAcquireNextImageKHR() was not returning VK_ERROR_OUT_OF_DATE_KHR when it should with WSI X11 swapchains" (https://www.nvidia.com/Download/driverResults.aspx/218826/en-us/) What I gather from this is that the surface was outdated on previous drivers too, but they just didn't report it as an error. So behaviour shouldn't change. In the issue conversation we experimented with calling `continue` when this error happens, but I found that it results in some small issues like bevy_egui scale not updating with the window sometimes. Just doing nothing seems to work better. ## Changelog - Fixed crashing on Linux with Nvidia 550 driver when resizing the window ## Migration Guide --------- Co-authored-by: James Liu <contact@jamessliu.com>

mockersf reviewed Mar 17, 2024

View reviewed changes

crates/bevy_render/src/view/window/mod.rs Outdated Show resolved Hide resolved

SolarLiner added A-Rendering Drawing game state to the screen O-Linux Specific to the Linux desktop operating system labels Mar 17, 2024

eero-lehtinen force-pushed the fix-nvidia-550-crash branch from 1d385a7 to 05e1444 Compare March 19, 2024 10:47

torsteingrindvik approved these changes Mar 19, 2024

View reviewed changes

eero-lehtinen force-pushed the fix-nvidia-550-crash branch 2 times, most recently from 584f230 to 3eef57d Compare March 23, 2024 01:45

james7132 requested a review from mockersf March 23, 2024 22:20

alice-i-cecile added this to the 0.13.2 milestone Mar 23, 2024

IQuick143 approved these changes Mar 23, 2024

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 23, 2024

james7132 reviewed Mar 23, 2024

View reviewed changes

alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 23, 2024

eero-lehtinen force-pushed the fix-nvidia-550-crash branch from f712ea6 to ec10fe7 Compare March 24, 2024 01:00

james7132 approved these changes Mar 24, 2024

View reviewed changes

eero-lehtinen force-pushed the fix-nvidia-550-crash branch from ec10fe7 to c891c01 Compare March 24, 2024 01:02

IceSentry reviewed Mar 24, 2024

View reviewed changes

james7132 mentioned this pull request Mar 25, 2024

App::run crashes with default plugins #12699

Closed

matiqo15 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 28, 2024

eero-lehtinen added 4 commits March 30, 2024 23:20

Fix crashing on outdated error with Nvidia 550

7e79e9a

Add logging for the ignored error

d2dbf53

Warn once instead of debug

8a0eada

Use vendor id to check for nvidia

8c940ac

eero-lehtinen force-pushed the fix-nvidia-550-crash branch from c891c01 to 8c940ac Compare March 30, 2024 21:22

james7132 reviewed Mar 30, 2024

View reviewed changes

crates/bevy_render/src/view/window/mod.rs Outdated Show resolved Hide resolved

Generalize warning text.

93ea153

james7132 enabled auto-merge March 30, 2024 23:02

IceSentry approved these changes Mar 30, 2024

View reviewed changes

james7132 added this pull request to the merge queue Mar 30, 2024

Merged via the queue into bevyengine:main with commit 70c69cd Mar 30, 2024
27 checks passed

eero-lehtinen mentioned this pull request Apr 13, 2024

Off screen window crashes App #12887

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash on Linux Nvidia 550 driver #12542

Fix crash on Linux Nvidia 550 driver #12542

eero-lehtinen commented Mar 17, 2024 •

edited

github-actions bot commented Mar 17, 2024

eero-lehtinen commented Mar 17, 2024 •

edited

eero-lehtinen commented Mar 17, 2024

WarrenHood commented Mar 18, 2024 •

edited

eero-lehtinen commented Mar 18, 2024

eero-lehtinen commented Mar 18, 2024

notverymoe commented Mar 19, 2024 •

edited

eero-lehtinen commented Mar 19, 2024

torsteingrindvik left a comment

IceSentry commented Mar 22, 2024

eero-lehtinen commented Mar 23, 2024 •

edited

IQuick143 left a comment

james7132 left a comment

eero-lehtinen commented Mar 24, 2024 •

edited

james7132 commented Mar 24, 2024

IceSentry Mar 24, 2024

eero-lehtinen Mar 24, 2024

tbvanderwoude Mar 25, 2024

eero-lehtinen Mar 28, 2024

eero-lehtinen Mar 28, 2024

torsteingrindvik Mar 30, 2024 •

edited

torsteingrindvik Mar 30, 2024

eero-lehtinen Mar 30, 2024 •

edited

Fix crash on Linux Nvidia 550 driver #12542

Fix crash on Linux Nvidia 550 driver #12542

Conversation

eero-lehtinen commented Mar 17, 2024 • edited

Objective

Solution

Changelog

Migration Guide

github-actions bot commented Mar 17, 2024

eero-lehtinen commented Mar 17, 2024 • edited

eero-lehtinen commented Mar 17, 2024

WarrenHood commented Mar 18, 2024 • edited

eero-lehtinen commented Mar 18, 2024

eero-lehtinen commented Mar 18, 2024

notverymoe commented Mar 19, 2024 • edited

eero-lehtinen commented Mar 19, 2024

torsteingrindvik left a comment

Choose a reason for hiding this comment

IceSentry commented Mar 22, 2024

eero-lehtinen commented Mar 23, 2024 • edited

IQuick143 left a comment

Choose a reason for hiding this comment

james7132 left a comment

Choose a reason for hiding this comment

eero-lehtinen commented Mar 24, 2024 • edited

james7132 commented Mar 24, 2024

IceSentry Mar 24, 2024

Choose a reason for hiding this comment

eero-lehtinen Mar 24, 2024

Choose a reason for hiding this comment

tbvanderwoude Mar 25, 2024

Choose a reason for hiding this comment

eero-lehtinen Mar 28, 2024

Choose a reason for hiding this comment

eero-lehtinen Mar 28, 2024

Choose a reason for hiding this comment

torsteingrindvik Mar 30, 2024 • edited

Choose a reason for hiding this comment

torsteingrindvik Mar 30, 2024

Choose a reason for hiding this comment

eero-lehtinen Mar 30, 2024 • edited

Choose a reason for hiding this comment

eero-lehtinen commented Mar 17, 2024 •

edited

eero-lehtinen commented Mar 17, 2024 •

edited

WarrenHood commented Mar 18, 2024 •

edited

notverymoe commented Mar 19, 2024 •

edited

eero-lehtinen commented Mar 23, 2024 •

edited

eero-lehtinen commented Mar 24, 2024 •

edited

torsteingrindvik Mar 30, 2024 •

edited

eero-lehtinen Mar 30, 2024 •

edited