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

Use i32::MAX as the max buffer size for Dx12 #4020

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

nical
Copy link
Contributor

@nical nical commented Aug 9, 2023

This is also the limit used in Dawn.

Checklist

  • Run cargo clippy.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description

Orthogonal but somewhat related to #4019 . We currently expose a max buffer size of u64::MAX in Dx12 which is not realistic.

I propose using i32::MAX has the advantage of being more bad-driver-proof as some implementation may use signed integers to store sizes. We've already run into this situation with vulkan and were able to diagnose it thanks to the driver in question being open-source but it's trickier to investigate this type of things with closed source windows drivers.

Dawn uses a similar limit for its dx12 backend.

@teoxoy
Copy link
Member

teoxoy commented Aug 10, 2023

I propose using i32::MAX has the advantage of being more bad-driver-proof as some implementation may use signed integers to store sizes. We've already run into this situation with vulkan and were able to diagnose it thanks to the driver in question being open-source but it's trickier to investigate this type of things with closed source windows drivers.

I can't find any reports where a dx12 driver behaves improperly with a buffer size > i32::MAX. CreatePlacedResource will return an OOM error if the allocation failed (and so will we).

We are also still using u64::MAX on Vulkan unless we are on linux/android and the gpu is not from nvidia. Ideally we'd use VkPhysicalDeviceMaintenance3Properties::maxMemoryAllocationSize (#3326) but since it's optional, we still need a default.

Specific driver bugs aside, my impression is that it's ok to treat this limit as if it didn't exist (i.e. u64::MAX) on backends that don't have a corresponding limit.

@nical
Copy link
Contributor Author

nical commented Aug 10, 2023

You won't see reports because people don't usually do silly things like that in their apps (and expect things to work).
In the browser we have to be careful we expose people's computer to arbitrary scripts so I naturally err on the side of better safe that sorry. It's certainly less harmful for wgpu to take risks outside of the browser and we do have stricter limits in Gecko on top so I won't press with one if it's controversial.

@cwfitzgerald
Copy link
Member

Will this MR be superceeded by #4053?

@nical
Copy link
Contributor Author

nical commented Aug 16, 2023

It can be as far as I'm concerned although it's not unreasonable to also have a more realistic default here if it is something we want.

@cwfitzgerald
Copy link
Member

SGTM

@cwfitzgerald cwfitzgerald merged commit 7b07f42 into gfx-rs:trunk Aug 16, 2023
@nical nical deleted the dx12-buf-limit branch August 17, 2023 13:36
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