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

Bump vello and wgpu. #179

Merged
merged 1 commit into from
Mar 2, 2024
Merged

Bump vello and wgpu. #179

merged 1 commit into from
Mar 2, 2024

Conversation

xorgy
Copy link
Member

@xorgy xorgy commented Mar 2, 2024

No description provided.

@xorgy xorgy requested a review from xStrom March 2, 2024 17:38
@Philipp-M
Copy link
Contributor

Hmm just tried it out, it freezes for me (same deadlock issue as before? (Haven't investigated that yet) Seems weird to me, since the release of wgpu 0.19.3 was just 19 hours ago?)

@xStrom
Copy link
Member

xStrom commented Mar 2, 2024

0.19.3, although recent, isn't equal to their trunk branch. So if we needed a fix from trunk to prevent the deadlock, we probably still need that.

@xorgy
Copy link
Member Author

xorgy commented Mar 2, 2024

0.19.3, although recent, isn't equal to their trunk branch. So if we needed a fix from trunk to prevent the deadlock, we probably still need that.

When I keep the override/patch in, cargo complains that it's not using it... which is interesting.

@Philipp-M
Copy link
Contributor

we probably still need that.

Yep, likely.

I haven't dug deeply, but it's freezing at the same place (get_current_texture()) as before...

@xorgy
Copy link
Member Author

xorgy commented Mar 2, 2024

we probably still need that.

Yep, likely.

I haven't dug deeply, but it's freezing at the same place (get_current_texture()) as before...

What platform are you on again? Do you have a thread for this in the Zulip?

@Philipp-M
Copy link
Contributor

Do you have a thread for this in the Zulip?

No, relevant discussion is in #176

(including platform, which is Hyprland (wayland) + nvidia beta driver (550))

@Philipp-M
Copy link
Contributor

Hmm that unfortunately doesn't help (makes sense to me since the deadlock occurs inside wgpu already, as seen in the link on #176 I've posted).

@xorgy
Copy link
Member Author

xorgy commented Mar 2, 2024

@Philipp-M Yeah I mistook it for something else that I encountered with a (very outdated) Vulkan software renderer's WSI.

@xorgy
Copy link
Member Author

xorgy commented Mar 2, 2024

@Philipp-M Does this occur with non-beta/stable NV drivers?

@xStrom
Copy link
Member

xStrom commented Mar 2, 2024

The relevant deadlock issue is wgpu#4967, which is a merged PR that fixes wgpu. Unfortunately it is a breaking change for wgpu-hal and so it won't be released as a patch for the v0.19 branch. It will arrive with wgpu v0.20 which will be released at approximately 2024-04-20.

I think we can merge this PR right now as-is, knowing that it will break Linux.

Then we will release Vello 0.1 any day now. After that is done, we could change Vello main so that it depends on the latest wgpu git rev, and then bump the Vello dep again here.

Having this fixed in Vello instead, with a git rev dep, is a more robust solution because [patch] only applies if the package that defines it is the root of the workspace. So if we use [patch] here for xilem then that won't help other Linux users of Vello, nor will it help Linux users who have xilem as a dependency. It will basically only help the Xilem examples being run from the Xilem workspace.

@Philipp-M
Copy link
Contributor

Thanks for having looked that up. Yeah sounds like a plan. (If we don't want to create immediate releases for vello in the meantime, but I guess can switch wgpu versions back then)

@xorgy
Copy link
Member Author

xorgy commented Mar 2, 2024

I think we can merge this PR right now as-is, knowing that it will break Linux.

To be clear, it works fine on Mesa on Linux, both on X11 and Wayland. It is only broken on NVIDIA's Vulkan stack, either due to a bug in their stack, or a coincidental safeguard in Mesa.

@xorgy
Copy link
Member Author

xorgy commented Mar 2, 2024

I went ahead and switched the target wgpu revision to a hash on trunk that incorporates some other interesting fixes, and which is compatible with Vello as-is.

Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

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

LGTM, and works for me.

@xorgy xorgy added this pull request to the merge queue Mar 2, 2024
@xorgy xorgy changed the title Bump vello and remove wgpu override. Bump vello and wgpu. Mar 2, 2024
Merged via the queue into linebender:main with commit 21a84d6 Mar 2, 2024
7 checks passed
@lokxii lokxii mentioned this pull request Mar 15, 2024
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.

3 participants