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

Fix Vulkan multithreaded compute list and GPU particle processing #79849

Merged

Conversation

bitsawer
Copy link
Member

Possibly helps with some other existing issues too, anything that uses compute shaders and threads can be affected.

Best test project to test this is probably this one: #70607 (comment) The project has a nice, big button that seems to trigger this bug every time.

The issue is that under heavy load, the multithreaded scene culling sometimes ends up calling RenderingDeviceVulkan::compute_list_begin() quickly in multiple threads when dealing with particles etc. This should be fine, but the actual bug is that the lock (_THREAD_SAFE_LOCK_) was taken too late, only after the error checks. This causes some threads processing particles to simply fail instead of simply waiting their turn before doing those error checks. This PR adds _THREAD_SAFE_METHOD_ to make sure the other threads wait their turn properly before proceeding.

Usually, the code fails here: https://github.com/godotengine/godot/blob/6588a4a29af1621086feac0117d5d4d37af957fd/servers/rendering/renderer_rd/storage_rd/particles_storage.cpp#L1210C3-L1210C3 as the processing code is run in a multiple WorkerThreadPool threads at the same time.

This continues the somewhat piecemeal fashion of adding thread thread safety into the Vulkan rendering device similar to #79526, not sure if these should also ported to the Direct3D 12 PR eventually.

@bitsawer bitsawer requested a review from a team as a code owner July 24, 2023 10:11
@bitsawer bitsawer added this to the 4.2 milestone Jul 24, 2023
@bitsawer bitsawer added cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 24, 2023
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

One more mutex for the pile...

Let's merge this for now. It will help us identify problematic functions when we overhaul the multithreading functionality

@YuriSizov YuriSizov merged commit 21524e2 into godotengine:master Jul 24, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment