Vulkan threading tweaks and minor #10049

unknownbrackets · 2017-11-05T05:29:39Z

Messed with this a bit - didn't test many games afterward (and it's still off by default), but it seems to work in the few I tested and improve performance (including ones using readback, I think.)

Also fixes an issue with switching backends with the debugger attached (quick backend switch mode.)

Oh, forgot to note - I moved FlushSync to the thread because I didn't want to worry about previous pending frames (this was causing issues it seemed like.)

Open to better names than VKRRunType. A bit tired, not sure what's better.

-[Unknown]

This at least allows us to detect that the backend failed to init. Happens when switching backends with debugger attached (probably driver bug?)

We end up with a second thread start at frame 1, so the thread needs to start at frame 1 too.

hrydgard · 2017-11-05T09:02:24Z

Cool. I'll look at this in detail later and try it on mobile, too.

hrydgard · 2017-11-05T11:41:02Z

From some quick testing this seems very promising. Good job finding my bugs :)

There are some things that we'll need to fix, for example, when running in debug mode I run into this validation error on startup:

ERROR: [THREADING] Queue Code 1 : THREADING ERROR : object of type VkQueue is simultaneously used in thread 11900 and thread 56504

This is because the main thread calls g_Vulkan->WaitUntilQueueIdle() from WindowsVulkanContext::Resize , while the submit-thread is submitting stuff to the queue. We need to delegate that to the thread as well somehow.

God of War asserts:

assert(insideFrame_);

for some reason, needs looking into.

Anyway, very cool. Will try on mobile soon where this should really help a lot.

hrydgard · 2017-11-05T12:12:36Z

Works on Nexus 5X, very unscientific quick testing seems to indicate something like 20-30% speed boost!

Time to rip out the old multithreading code... and maybe should try something like this for GL too, not sure I can be bothered though.

Regarding the old multithreading code, I'm thinking to rip out the option and the synchronization and the event queue stuff in the GPU, but keep the code to spin up a separate CPU thread because that might have to be the way to go for any sort of GL deferred drawing - must draw on the thread we spin up from Java, while Vulkan is entirely thread-agnostic in a much nicer way.

We don't want a dependency on the thread state, of course.

unknownbrackets · 2017-11-05T15:34:01Z

We'll need to refcount framebuffers or have them deleted on the render side - GoW is hitting that. Opinion on using RefCountedObject or? Part of the issue is the Draw::Framebuffer needs deleting, so either that needs to happen in the queue runner (and it's a thin3d object not VKR) or the VKRFramebuffer needs its own refcount.

I'm thinking we might as well just mutex WaitUntilQueueIdle? It only happens on resize and shutdown, although it'll be a cost on submit. The alternative is another run type, like sync.

Yeah, the problem with the old option is that at the time I didn't realize games could rely on drawsync timing so heavily, and wanted to move off the overhead of list decoding if possible. Agreed on removing, except the actual thread separation, although that'll need changes to target a queue runner rather than the GPU.

Removing it might also improve performance a very tiny bit since there's some overhead in places for the threadsync in the GPU...

-[Unknown]

hrydgard · 2017-11-05T15:44:01Z

Refcount seems fine, but we'll have to make sure to use atomic increments and decrements. Or we could add a deleter queue to the thin3d VulkanContext. So many abstractions...

I'm fine with mutexing WaitUntilQueueIdle.

Yup, I didn't realize that either. And DL decoding is a cost but turns out that on mobile, queueing up GL or Vulkan operations is more expensive so that's the part we want on a thread the most anyway.

I don't expect much improvements from getting rid of the overhead, but I guess every bit helps...

Fixes crash in GoW when using a thread.

Needed when the last rendered FB needs to be downloaded.

Otherwise it's only done after destroying Draw, so no need to mutex.

hrydgard · 2017-11-05T16:51:43Z

ext/native/thin3d/VulkanRenderManager.cpp

@@ -218,6 +218,8 @@ void VulkanRenderManager::DestroyBackbuffers() {
 		}
 		thread_.join();
 	}
+	vulkan_->WaitUntilQueueIdle();


Yeah, this should be a better place for this.

hrydgard · 2017-11-05T17:04:48Z

ext/native/thin3d/VulkanRenderManager.cpp

+			auto &frameData = frameData_[i];
+			frameData.readyForRun = false;
+			if (!shutdown && !frameData.readyForFence) {
+				vkDestroyFence(vulkan_->GetDevice(), frameData.fence, nullptr);


What about vkResetFences? Unless we zap the whole device, fences should be reusable.

(Hm, I made this comment before but it got lost?)

EDIT: No wait, we want them signalled.. The comment above should be something like "Pre-signal the fences for next time around." in that case.

Oops, yeah. Typed that when I had only moved the flags so far...

-[Unknown]

When resizing or similar, we may end up with frames we never ran. This also happens on startup. We need them signaled at start so we can wait on them, or we may deadlock.

Fixes hrydgard#10048.

hrydgard · 2017-11-05T17:30:19Z

Now there's just the matter of whether this should be an option, and whether it should be on by default perhaps...

hrydgard · 2017-11-05T20:32:43Z

@unknownbrackets Having a strange issue now where in Release mode, it hangs right on startup, while it works in debug. Do you see that too?

EDIT: It's some race when recreating the backbuffers due to the initial Resize event. Looking into it.

hrydgard · 2017-11-05T21:21:45Z

Fixed it, I think.

unknownbrackets · 2017-11-05T22:34:40Z

Cool, yeah, never hit that race - I only hit the fence one intermittently, so maybe I didn't catch it after fixing the fence.

-[Unknown]

unknownbrackets added 5 commits November 4, 2017 20:45

Vulkan: Fix segfault on swapchain fail.

8edc6ea

This at least allows us to detect that the backend failed to init. Happens when switching backends with debugger attached (probably driver bug?)

Vulkan: Trigger condvar only at frame end.

ec0f640

Vulkan: Fix out-of-sync frames on threading.

8b55940

We end up with a second thread start at frame 1, so the thread needs to start at frame 1 too.

Vulkan: Allow sync when using threading.

97fa0a7

Vulkan: Fix a comment.

dc200a4

Vulkan: Account inside frame from push side.

ab9aee9

We don't want a dependency on the thread state, of course.

unknownbrackets added 3 commits November 5, 2017 08:44

Vulkan: Refcount framebuffer deletes.

2ad9eb0

Fixes crash in GoW when using a thread.

GPU: Restore initial readback on output.

efbba31

Needed when the last rendered FB needs to be downloaded.

Vulkan: Wait for queue idle in DestroyBackbuffers.

ebe9dca

Otherwise it's only done after destroying Draw, so no need to mutex.

unknownbrackets force-pushed the vulkan-minor branch from df11f34 to 5802747 Compare November 5, 2017 16:44

hrydgard reviewed Nov 5, 2017

View reviewed changes

unknownbrackets added 2 commits November 5, 2017 09:22

Vulkan: Resignal unexecuted fences on thread stop.

56d3440

When resizing or similar, we may end up with frames we never ran. This also happens on startup. We need them signaled at start so we can wait on them, or we may deadlock.

Vulkan: Make sure backbuffer is bound for UI.

ead4c5f

Fixes hrydgard#10048.

unknownbrackets force-pushed the vulkan-minor branch from 49109c1 to ead4c5f Compare November 5, 2017 17:22

hrydgard approved these changes Nov 5, 2017

View reviewed changes

hrydgard merged commit 2f305f9 into hrydgard:master Nov 5, 2017

unknownbrackets deleted the vulkan-minor branch November 5, 2017 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vulkan threading tweaks and minor #10049

Vulkan threading tweaks and minor #10049

unknownbrackets commented Nov 5, 2017 •

edited

hrydgard commented Nov 5, 2017

hrydgard commented Nov 5, 2017

hrydgard commented Nov 5, 2017 •

edited

unknownbrackets commented Nov 5, 2017

hrydgard commented Nov 5, 2017

hrydgard Nov 5, 2017

hrydgard Nov 5, 2017 •

edited

unknownbrackets Nov 5, 2017

hrydgard commented Nov 5, 2017

hrydgard commented Nov 5, 2017 •

edited

hrydgard commented Nov 5, 2017

unknownbrackets commented Nov 5, 2017

Vulkan threading tweaks and minor #10049

Vulkan threading tweaks and minor #10049

Conversation

unknownbrackets commented Nov 5, 2017 • edited

hrydgard commented Nov 5, 2017

hrydgard commented Nov 5, 2017

hrydgard commented Nov 5, 2017 • edited

unknownbrackets commented Nov 5, 2017

hrydgard commented Nov 5, 2017

hrydgard Nov 5, 2017

Choose a reason for hiding this comment

hrydgard Nov 5, 2017 • edited

Choose a reason for hiding this comment

unknownbrackets Nov 5, 2017

Choose a reason for hiding this comment

hrydgard commented Nov 5, 2017

hrydgard commented Nov 5, 2017 • edited

hrydgard commented Nov 5, 2017

unknownbrackets commented Nov 5, 2017

unknownbrackets commented Nov 5, 2017 •

edited

hrydgard commented Nov 5, 2017 •

edited

hrydgard Nov 5, 2017 •

edited

hrydgard commented Nov 5, 2017 •

edited