From 8ee426ff74916b9637411b9cd66b1994f701b25b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 8 Feb 2018 00:23:48 +0100 Subject: [PATCH 1/5] Get rid of an unused codepath (gl rendering is now always threaded) --- Windows/GPU/WindowsGLContext.cpp | 3 +- Windows/MainThread.h | 1 + ext/native/thin3d/GLRenderManager.cpp | 70 ++++++++------------------- ext/native/thin3d/GLRenderManager.h | 17 ++----- 4 files changed, 28 insertions(+), 63 deletions(-) create mode 100644 Windows/MainThread.h diff --git a/Windows/GPU/WindowsGLContext.cpp b/Windows/GPU/WindowsGLContext.cpp index ac48df35fd..7962d3a817 100644 --- a/Windows/GPU/WindowsGLContext.cpp +++ b/Windows/GPU/WindowsGLContext.cpp @@ -35,7 +35,8 @@ #include "Windows/GPU/WindowsGLContext.h" void WindowsGLContext::SwapBuffers() { - renderManager_->Swap(); + // We no longer call RenderManager::Swap here, it's handled by the render thread, which + // we're not on here. // Used during fullscreen switching to prevent rendering. if (pauseRequested) { diff --git a/Windows/MainThread.h b/Windows/MainThread.h new file mode 100644 index 0000000000..6f70f09bee --- /dev/null +++ b/Windows/MainThread.h @@ -0,0 +1 @@ +#pragma once diff --git a/ext/native/thin3d/GLRenderManager.cpp b/ext/native/thin3d/GLRenderManager.cpp index e4a4ab23ae..b6e049a1f6 100644 --- a/ext/native/thin3d/GLRenderManager.cpp +++ b/ext/native/thin3d/GLRenderManager.cpp @@ -45,11 +45,6 @@ GLRenderManager::GLRenderManager() { for (int i = 0; i < MAX_INFLIGHT_FRAMES; i++) { } - - if (!useThread_) { - // The main thread is also the render thread. - ThreadStart(); - } } GLRenderManager::~GLRenderManager() { @@ -58,12 +53,7 @@ GLRenderManager::~GLRenderManager() { } // Was anything deleted during shutdown? deleter_.Perform(); - // _assert_(deleter_.IsEmpty()); - - if (!useThread_) { - // The main thread is also the render thread. - ThreadEnd(); - } + _assert_(deleter_.IsEmpty()); } void GLRenderManager::ThreadStart() { @@ -143,7 +133,7 @@ bool GLRenderManager::ThreadFrame() { void GLRenderManager::StopThread() { // Since we don't control the thread directly, this will only pause the thread. - if (useThread_ && run_) { + if (run_) { run_ = false; for (int i = 0; i < MAX_INFLIGHT_FRAMES; i++) { auto &frameData = frameData_[i]; @@ -170,6 +160,7 @@ void GLRenderManager::StopThread() { // when we restart... for (int i = 0; i < MAX_INFLIGHT_FRAMES; i++) { auto &frameData = frameData_[i]; + std::unique_lock lock(frameData.push_mutex); if (frameData.readyForRun || frameData.steps.size() != 0) { Crash(); } @@ -181,7 +172,6 @@ void GLRenderManager::StopThread() { frameData.steps.clear(); frameData.initSteps.clear(); - std::unique_lock lock(frameData.push_mutex); while (!frameData.readyForFence) { VLOG("PUSH: Waiting for frame[%d].readyForFence = 1 (stop)", i); frameData.push_condVar.wait(lock); @@ -330,7 +320,7 @@ void GLRenderManager::BeginFrame() { FrameData &frameData = frameData_[curFrame]; // Make sure the very last command buffer from the frame before the previous has been fully executed. - if (useThread_) { + { std::unique_lock lock(frameData.push_mutex); while (!frameData.readyForFence) { VLOG("PUSH: Waiting for frame[%d].readyForFence = 1", curFrame); @@ -342,10 +332,7 @@ void GLRenderManager::BeginFrame() { VLOG("PUSH: Fencing %d", curFrame); - - // vkWaitForFences(device, 1, &frameData.fence, true, UINT64_MAX); - // vkResetFences(device, 1, &frameData.fence); - // glFenceSync(...) + // glFenceSync(&frameData.fence...) // Must be after the fence - this performs deletes. VLOG("PUSH: BeginFrame %d", curFrame); @@ -363,14 +350,7 @@ void GLRenderManager::Finish() { curRenderStep_ = nullptr; int curFrame = GetCurFrame(); FrameData &frameData = frameData_[curFrame]; - if (!useThread_) { - frameData.steps = std::move(steps_); - steps_.clear(); - frameData.initSteps = std::move(initSteps_); - initSteps_.clear(); - frameData.type = GLRRunType::END; - Run(curFrame); - } else { + { std::unique_lock lock(frameData.pull_mutex); VLOG("PUSH: Frame[%d].readyForRun = true", curFrame); frameData.steps = std::move(steps_); @@ -398,6 +378,7 @@ void GLRenderManager::BeginSubmitFrame(int frame) { } } +// Render thread void GLRenderManager::Submit(int frame, bool triggerFence) { FrameData &frameData = frameData_[frame]; @@ -409,7 +390,7 @@ void GLRenderManager::Submit(int frame, bool triggerFence) { // ideally we'd like to wait a frame or two. frameData.deleter.Perform(); - if (useThread_ && triggerFence) { + if (triggerFence) { VLOG("PULL: Frame %d.readyForFence = true", frame); std::unique_lock lock(frameData.push_mutex); @@ -420,6 +401,7 @@ void GLRenderManager::Submit(int frame, bool triggerFence) { } } +// Render thread void GLRenderManager::EndSubmitFrame(int frame) { FrameData &frameData = frameData_[frame]; frameData.hasBegun = false; @@ -435,6 +417,7 @@ void GLRenderManager::EndSubmitFrame(int frame) { } } +// Render thread void GLRenderManager::Run(int frame) { BeginSubmitFrame(frame); @@ -472,14 +455,7 @@ void GLRenderManager::FlushSync() { // TODO: Reset curRenderStep_? int curFrame = curFrame_; FrameData &frameData = frameData_[curFrame]; - if (!useThread_) { - frameData.initSteps = std::move(initSteps_); - initSteps_.clear(); - frameData.steps = std::move(steps_); - steps_.clear(); - frameData.type = GLRRunType::SYNC; - Run(curFrame); - } else { + { std::unique_lock lock(frameData.pull_mutex); VLOG("PUSH: Frame[%d].readyForRun = true (sync)", curFrame); frameData.initSteps = std::move(initSteps_); @@ -491,8 +467,7 @@ void GLRenderManager::FlushSync() { frameData.type = GLRRunType::SYNC; frameData.pull_condVar.notify_all(); } - - if (useThread_) { + { std::unique_lock lock(frameData.push_mutex); // Wait for the flush to be hit, since we're syncing. while (!frameData.readyForFence) { @@ -504,19 +479,20 @@ void GLRenderManager::FlushSync() { } } +// Render thread void GLRenderManager::EndSyncFrame(int frame) { FrameData &frameData = frameData_[frame]; Submit(frame, false); - // This is brutal! Should probably wait for a fence instead, not that it'll matter much since we'll - // still stall everything. - glFinish(); + // glFinish is not actually necessary here, and won't be until we start using + // glBufferStorage. Then we need to use fences. + // glFinish(); // At this point we can resume filling the command buffers for the current frame since // we know the device is idle - and thus all previously enqueued command buffers have been processed. // No need to switch to the next frame number. - if (useThread_) { + { std::unique_lock lock(frameData.push_mutex); frameData.readyForFence = true; frameData.readyForSubmit = true; @@ -533,11 +509,6 @@ void GLRenderManager::Wipe() { } void GLRenderManager::WaitUntilQueueIdle() { - if (!useThread_) { - // Must be idle, nothing to do. - return; - } - // Just wait for all frames to be ready. for (int i = 0; i < MAX_INFLIGHT_FRAMES; i++) { FrameData &frameData = frameData_[i]; @@ -563,16 +534,15 @@ GLPushBuffer::~GLPushBuffer() { void GLPushBuffer::Map() { assert(!writePtr_); // TODO: Even a good old glMapBuffer could actually work well here. - // VkResult res = vkMapMemory(device_, buffers_[buf_].deviceMemory, 0, size_, 0, (void **)(&writePtr_)); writePtr_ = buffers_[buf_].deviceMemory; assert(writePtr_); } void GLPushBuffer::Unmap() { assert(writePtr_); - // Here we should simply upload everything to the buffers. - // Might be worth trying with size_ instead of offset_, so the driver can replace the whole buffer. - // At least if it's close. + // Here we simply upload the data to the last buffer. + // Might be worth trying with size_ instead of offset_, so the driver can replace + // the whole buffer. At least if it's close. render_->BufferSubdata(buffers_[buf_].buffer, 0, offset_, buffers_[buf_].deviceMemory, false); writePtr_ = nullptr; } diff --git a/ext/native/thin3d/GLRenderManager.h b/ext/native/thin3d/GLRenderManager.h index 4ef204d558..b658a116cf 100644 --- a/ext/native/thin3d/GLRenderManager.h +++ b/ext/native/thin3d/GLRenderManager.h @@ -663,12 +663,6 @@ public: swapFunction_ = swapFunction; } - void Swap() { - if (!useThread_ && swapFunction_) { - swapFunction_(); - } - } - void StopThread(); bool SawOutOfMemory() { @@ -738,8 +732,6 @@ private: GLDeleter deleter_; - bool useThread_ = true; - int curFrame_ = 0; std::function swapFunction_; @@ -748,10 +740,11 @@ private: int targetHeight_ = 0; }; - -// Similar to VulkanPushBuffer but uses really stupid tactics - collect all the data in RAM then do a big -// memcpy/buffer upload at the end. This can however be optimized with glBufferStorage on chips that support that -// for massive boosts. +// Similar to VulkanPushBuffer but is currently less efficient - it collects all the data in +// RAM then does a big memcpy/buffer upload at the end of the frame. This is at least a lot +// faster than the hundreds of buffer uploads or memory array buffers we used before. +// On modern GL we could avoid the copy using glBufferStorage but not sure it's worth the +// trouble. class GLPushBuffer { public: struct BufInfo { From 481134bfe9ccb3098bbbe44e45a23a9afb3dc1d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 8 Feb 2018 00:40:55 +0100 Subject: [PATCH 2/5] Another go at the deleter problem. Still not feeling 100% good about how this works.. but it does seem to work fine. --- ext/native/thin3d/GLRenderManager.cpp | 19 +++++++++---------- ext/native/thin3d/GLRenderManager.h | 4 +--- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/ext/native/thin3d/GLRenderManager.cpp b/ext/native/thin3d/GLRenderManager.cpp index b6e049a1f6..d5ee6be8b3 100644 --- a/ext/native/thin3d/GLRenderManager.cpp +++ b/ext/native/thin3d/GLRenderManager.cpp @@ -13,7 +13,6 @@ #endif void GLDeleter::Perform() { - deleterMutex_.lock(); for (auto shader : shaders) { delete shader; } @@ -38,7 +37,6 @@ void GLDeleter::Perform() { delete framebuffer; } framebuffers.clear(); - deleterMutex_.unlock(); } GLRenderManager::GLRenderManager() { @@ -50,6 +48,7 @@ GLRenderManager::GLRenderManager() { GLRenderManager::~GLRenderManager() { for (int i = 0; i < MAX_INFLIGHT_FRAMES; i++) { _assert_(frameData_[i].deleter.IsEmpty()); + _assert_(frameData_[i].deleter_prev.IsEmpty()); } // Was anything deleted during shutdown? deleter_.Perform(); @@ -72,6 +71,7 @@ void GLRenderManager::ThreadEnd() { // Good point to run all the deleters to get rid of leftover objects. for (int i = 0; i < MAX_INFLIGHT_FRAMES; i++) { frameData_[i].deleter.Perform(); + frameData_[i].deleter_prev.Perform(); for (int j = 0; j < (int)frameData_[i].steps.size(); j++) { delete frameData_[i].steps[j]; } @@ -110,8 +110,10 @@ bool GLRenderManager::ThreadFrame() { // This means we're out of frames to render and run_ is false, so bail. return false; } - VLOG("PULL: frame[%d].readyForRun = false", threadFrame_); + VLOG("PULL: Setting frame[%d].readyForRun = false", threadFrame_); frameData.readyForRun = false; + frameData.deleter_prev.Perform(); + frameData.deleter_prev.Take(frameData.deleter); // Previously we had a quick exit here that avoided calling Run() if run_ was suddenly false, // but that created a race condition where frames could end up not finished properly on resize etc. @@ -352,17 +354,18 @@ void GLRenderManager::Finish() { FrameData &frameData = frameData_[curFrame]; { std::unique_lock lock(frameData.pull_mutex); - VLOG("PUSH: Frame[%d].readyForRun = true", curFrame); + VLOG("PUSH: Frame[%d].readyForRun = true, notifying pull", curFrame); frameData.steps = std::move(steps_); steps_.clear(); frameData.initSteps = std::move(initSteps_); initSteps_.clear(); frameData.readyForRun = true; frameData.type = GLRRunType::END; - frameData.pull_condVar.notify_all(); + frameData_[curFrame_].deleter.Take(deleter_); } - frameData_[curFrame_].deleter.Take(deleter_); + // Notify calls do not in fact need to be done with the mutex locked. + frameData.pull_condVar.notify_all(); curFrame_++; if (curFrame_ >= MAX_INFLIGHT_FRAMES) @@ -386,10 +389,6 @@ void GLRenderManager::Submit(int frame, bool triggerFence) { // When !triggerFence, we notify after syncing with Vulkan. - // Putting deletes here is safe but only because OpenGL has its own delete handling.. - // ideally we'd like to wait a frame or two. - frameData.deleter.Perform(); - if (triggerFence) { VLOG("PULL: Frame %d.readyForFence = true", frame); diff --git a/ext/native/thin3d/GLRenderManager.h b/ext/native/thin3d/GLRenderManager.h index b658a116cf..8b489c197d 100644 --- a/ext/native/thin3d/GLRenderManager.h +++ b/ext/native/thin3d/GLRenderManager.h @@ -157,7 +157,6 @@ public: return shaders.empty() && programs.empty() && buffers.empty() && textures.empty() && inputLayouts.empty() && framebuffers.empty(); } void Take(GLDeleter &other) { - deleterMutex_.lock(); _assert_msg_(G3D, IsEmpty(), "Deleter already has stuff"); shaders = std::move(other.shaders); programs = std::move(other.programs); @@ -171,7 +170,6 @@ public: other.textures.clear(); other.inputLayouts.clear(); other.framebuffers.clear(); - deleterMutex_.unlock(); } std::vector shaders; @@ -180,7 +178,6 @@ public: std::vector textures; std::vector inputLayouts; std::vector framebuffers; - std::mutex deleterMutex_; }; class GLRInputLayout { @@ -706,6 +703,7 @@ private: uint32_t curSwapchainImage = -1; GLDeleter deleter; + GLDeleter deleter_prev; std::set activePushBuffers; }; From 1eec30dac2ee5089f72e5430d4841cd93cd31841 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 8 Feb 2018 00:44:00 +0100 Subject: [PATCH 3/5] Avoid causing a gl error on startup on core contexts. --- ext/native/thin3d/GLQueueRunner.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ext/native/thin3d/GLQueueRunner.cpp b/ext/native/thin3d/GLQueueRunner.cpp index 466a6d958a..87e7ef1d53 100644 --- a/ext/native/thin3d/GLQueueRunner.cpp +++ b/ext/native/thin3d/GLQueueRunner.cpp @@ -41,7 +41,9 @@ void GLQueueRunner::CreateDeviceObjects() { populate(GL_RENDERER); populate(GL_VERSION); populate(GL_SHADING_LANGUAGE_VERSION); - populate(GL_EXTENSIONS); // TODO: Not OK to query this in core profile! + if (!gl_extensions.IsCoreContext) { // Not OK to query this in core profile! + populate(GL_EXTENSIONS); + } } void GLQueueRunner::DestroyDeviceObjects() { From 8c5a94ce01578e248206289551cdff7aa48c7dd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 8 Feb 2018 00:50:29 +0100 Subject: [PATCH 4/5] gl-render-manager: Oops, fix scissor tests after clears --- ext/native/thin3d/GLQueueRunner.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ext/native/thin3d/GLQueueRunner.cpp b/ext/native/thin3d/GLQueueRunner.cpp index 87e7ef1d53..3f5389cd6b 100644 --- a/ext/native/thin3d/GLQueueRunner.cpp +++ b/ext/native/thin3d/GLQueueRunner.cpp @@ -616,6 +616,8 @@ void GLQueueRunner::PerformRenderPass(const GLRStep &step) { glClear(c.clear.clearMask); if (c.clear.scissorW > 0) { glDisable(GL_SCISSOR_TEST); + } else { + glEnable(GL_SCISSOR_TEST); } break; case GLRRenderCommand::INVALIDATE: From 4d6c0cfb32a7ef836fee6171fa13bd058d42bc52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 8 Feb 2018 01:11:51 +0100 Subject: [PATCH 5/5] Buildfix --- SDL/SDLGLGraphicsContext.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SDL/SDLGLGraphicsContext.h b/SDL/SDLGLGraphicsContext.h index 71f130a4f7..52ea68f018 100644 --- a/SDL/SDLGLGraphicsContext.h +++ b/SDL/SDLGLGraphicsContext.h @@ -19,7 +19,7 @@ public: void ShutdownFromRenderThread() override; void SwapBuffers() override { - renderManager_->Swap(); + // Do nothing, the render thread takes care of this. } Draw::DrawContext *GetDrawContext() override {