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 threading bug in Vulkan rendering device #78794

Merged
merged 1 commit into from
Jul 8, 2023

Conversation

joemarshall
Copy link
Contributor

@joemarshall joemarshall commented Jun 28, 2023

This is a fix for the following bug:

#78786

The bug was that:
draw_list_switch_to_next_pass was not holding the lock, and was calling
_draw_list_free(&viewport);
and draw_list_allocate with stuff in between, all mid a single multiple pass draw-list.

The free and allocate functions release and acquire the lock. So in between the two calls, other stuff was happening (other draw lists being started and ended), which meant bad things happened because the vulkan state goes bad (segfaults in graphics drivers on my machine).

This is I think a pretty trivial bugfix - deffo for 4.2, would be great for 4.1 if poss?

@joemarshall joemarshall requested a review from a team as a code owner June 28, 2023 12:55
@AThousandShips AThousandShips added this to the 4.2 milestone Jun 28, 2023
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jun 28, 2023
@akien-mga akien-mga changed the title fix threading bug in vulkan rendering device Fix threading bug in Vulkan rendering device Jun 28, 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.

Looks good. From reviewing related code, it seems like everything called during a draw list needs to be made thread safe.

@joemarshall
Copy link
Contributor Author

Looks good. From reviewing related code, it seems like everything called during a draw list needs to be made thread safe.

The draw_list_* calls mostly don't need thread safety adding because the lock is taken during draw_list_begin, and released on draw_list_end. It makes sense because there is only one draw list tracked, so it makes no sense for drawing calls from another thread to get put in during creation of a draw list in one thread.

Incidentally I don't think holding this lock is a major issue, because it's vulkan, everything is deferred, the actual draw_list calls are super fast. The only downside of it is that it means if people call draw_list_begin without a matching draw_list_end, or try to add things to the same draw_list across multiple threads, bad things will happen. But really there isn't any non-stupid use case for that until or unless we add support for multiple draw lists on one renderingdevice.

This bug was the only thing I caught when I went through it, I think I was quite thorough - I stepped through things in debugger also.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

If this fixes threading bugs, it's fine I guess. In any case, we have to reassess the whole thread safety topic in RD to determine and enforce specific design goals.

@akien-mga akien-mga merged commit 98358b2 into godotengine:master Jul 8, 2023
@akien-mga
Copy link
Member

akien-mga commented Jul 8, 2023

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 10, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.1.

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

Successfully merging this pull request may close these issues.

Game quits after rendering calls on main rendering device if multithreaded rendering is enabled
6 participants