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

Mutable self for memory mapping #33

Closed
kvark opened this issue Jan 3, 2021 · 9 comments · Fixed by #34
Closed

Mutable self for memory mapping #33

kvark opened this issue Jan 3, 2021 · 9 comments · Fixed by #34

Comments

@kvark
Copy link
Contributor

kvark commented Jan 3, 2021

It used to be the case that mapping memory worked with &self. This changed with gfx-rs/gfx#3551, according to Vulkan's synchronization semantics. Now, working with gpu-alloc is more difficult. Perhaps, the MemoryDevice trait here can be changed to work with &mut Memory instead when mapping/unmapping?

@kvark
Copy link
Contributor Author

kvark commented Jan 3, 2021

Actually, just realized this wouldn't help us, since we have Arc<Memory> anyway 🤔

@kvark
Copy link
Contributor Author

kvark commented Jan 3, 2021

The problem still stands though. Gpu-alloc should work well with Vulkan semantics, which gfx-hal now reflects more precisely, and currently it would require something like Arc<RwLock<Memory>>, which is sub-optimal. A better solution is needed.

@zakarumych
Copy link
Owner

Perhaps Arc<UnsafeCell<Memory>> would allow sound implementation without overhead.

@zakarumych
Copy link
Owner

Mapping related operations are already synchronized by MemoryBlock. Freeing Memory require dropping all MemoryBlocks created from it, so it is synchronized with mapping operations too.

@kvark
Copy link
Contributor Author

kvark commented Jan 4, 2021

I believe what "explicit synchronization" means in Vulkan is not "all calls to this function using the same value of X must be serialized" but rather a more stronger "all access to value X must be serialized around calling this function". This means, calling vkBindBufferMemory in parallel with vkMapMemory is UB, and that means Arc<UnsafeCell<>> is not a proper solution.

@zakarumych
Copy link
Owner

zakarumych commented Jan 4, 2021

vkBindBufferMemory does not specify that host access to memory object must be externally serialized.
So it is unclear to me if vkMapMemory and vkBindBufferMemory can or can't occur in parallel.

@kvark
Copy link
Contributor Author

kvark commented Jan 4, 2021

vkBindBufferMemory does not specify that host access to memory object must be externally serialized.

I think that basically means &Memory in Rust sense. So you can use it at the same time as you use other non-externally-synchronized methods. But it's still "host access"! So it still has to take into account other host accesses for parallel use.

@kvark
Copy link
Contributor Author

kvark commented Jan 4, 2021

@Ralith pointed out to the relevant spec text:

A parameter declared as externally synchronized may have its contents updated at any time during the host execution of the command. If two commands operate on the same object and at least one of the commands declares the object to be externally synchronized, then the caller must guarantee not only that the commands do not execute simultaneously, but also that the two commands are separated by an appropriate memory barrier (if needed).

bors bot added a commit to gfx-rs/wgpu that referenced this issue Jan 9, 2021
1136: Changed Arc<B::Memory> to Arc<Mutex<B::Memory>> in GpuAllocator r=kvark a=ElArtista

Work around for blocker zakarumych/gpu-alloc#33, makes wgpu compatible with latest gfx

Co-authored-by: TheArtist <[email protected]>
@kvark
Copy link
Contributor Author

kvark commented Jan 12, 2021

@Wumpf has landed a workaround in wgpu-core, I hope it's temporary!
We'll need gpu-alloc and gpu-descriptor crates released soon for wgpu. Just finishing with the low-level stuff now - gfx-rs/gfx#3582
Would it be possible to find a solution to this issue before the release? Is there anything else you think is necessary before releasing gpu-alloc?

zakarumych added a commit that referenced this issue Jan 15, 2021
Now those methods require &mut M
e.g. externally synchronized memory object

Fixes #33
zakarumych added a commit that referenced this issue Jan 19, 2021
Now those methods require &mut M
e.g. externally synchronized memory object

Fixes #33
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 a pull request may close this issue.

2 participants