VulkanRenderManager - big refactoring of the Vulkan code #10033
Conversation
hrydgard
changed the title
…game still broken.
…e while stuck on condvar)
… need to run the swap chain from the thread that created it?
…ighten up some image transitions.
… Add a cap so we can try to unify BlitFramebufferDepth later.
…on of tesselation (should really use separate big desc layouts for them)
hrydgard
force-pushed
the
vulkan-queue-manager
branch
from
e07472f to
7312576
Compare
hrydgard
changed the title
| vp.TopLeftX = (float)x; | ||
| vp.TopLeftY = (float)y; | ||
| vp.Width = (float)w; | ||
| vp.Height = (float)h; | ||
| // Since we're about to override it. |
There was a problem hiding this comment.
This comment is probably no longer relevant.
| @@ -126,7 +126,8 @@ void DrawEngineVulkan::InitDeviceObjects() { | |||
| bindings[4].descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC; | |||
| bindings[4].stageFlags = VK_SHADER_STAGE_VERTEX_BIT; | |||
| bindings[4].binding = DRAW_BINDING_DYNUBO_BONE; | |||
| // Hardware tessellation | |||
| // Hardware tessellation. TODO: Don't allocate these unless actually drawing splines. | |||
| // Will require additional | |||
There was a problem hiding this comment.
I think you may have forgot to
-[Unknown]
| @@ -809,7 +809,7 @@ void FramebufferManagerCommon::SetViewport2D(int x, int y, int w, int h) { | |||
| } | |||
|
|
|||
| void FramebufferManagerCommon::CopyDisplayToOutput() { | |||
| DownloadFramebufferOnSwitch(currentRenderVfb_); | |||
| // DownloadFramebufferOnSwitch(currentRenderVfb_); | |||
There was a problem hiding this comment.
Can we restore this?
-[Unknown]
| step->copy.dst = dst; | ||
| step->copy.dstPos = dstPos; | ||
|
|
||
| std::unique_lock<std::mutex> lock(mutex_); |
There was a problem hiding this comment.
Hmm, not all pushes to steps_ have locks and I think this mutex_ is no longer used... just noting.
-[Unknown]
| res = vkBeginCommandBuffer(cmd, &begin); | ||
| assert(res == VK_SUCCESS); | ||
|
|
||
| // TODO: Deal with the VK_SUBOPTIMAL_KHR and VK_ERROR_OUT_OF_DATE_KHR |
There was a problem hiding this comment.
This TODO has journeyed a little ways from what it once referenced, I think? Also now there is an extra assert here.
-[Unknown]
| break; | ||
| } | ||
| default: | ||
| ELOG("Unimpl queue command"); |
There was a problem hiding this comment.
Maybe an assert?
-[Unknown]
| } | ||
|
|
||
| #if 0 | ||
| // This part is based on faulty old thinking. |
There was a problem hiding this comment.
Should we just delete?
I'm missing what's faulty - although this is definitely suboptimal, and if we're (hopefully) optimizing out clears, it shouldn't be needed as often?
-[Unknown]
| void VulkanRenderManager::BindFramebufferAsRenderTarget(VKRFramebuffer *fb, VKRRenderPassAction color, VKRRenderPassAction depth, uint32_t clearColor, float clearDepth, uint8_t clearStencil) { | ||
| // Eliminate dupes. | ||
| if (steps_.size() && steps_.back()->stepType == VKRStepType::RENDER && steps_.back()->render.framebuffer == fb) { | ||
| if (color != VKRRenderPassAction::CLEAR && depth != VKRRenderPassAction::CLEAR) { |
There was a problem hiding this comment.
I wonder if we sometimes set clear unnecessarily and create dupes (e.g. that both clear.)
Since our GL code skips dup binds, which improved perf a little... but probably not common, guess we'll see later.
-[Unknown]
| renderManager_.CreateBackbuffers(); | ||
| break; | ||
| default: | ||
| break; |
There was a problem hiding this comment.
Hmm, why not an assert? I'd rather be explicit, it's not uncommon we'd add an event and forget to update D3D11 or somewhere.
-[Unknown]
|
|
||
| renderManager_.BindPipeline(curPipeline_->vkpipeline); | ||
| // TODO: blend constants, stencil, viewports should be here, after bindpipeline.. | ||
| renderManager_.Draw(pipelineLayout_, descSet, 1, &ubo_offset, vulkanVbuf, (int)vbBindOffset, vertexCount); | ||
| } | ||
|
|
||
| // TODO: We should avoid this function as much as possible, instead use renderpass on-load clearing. |
There was a problem hiding this comment.
In theory this is no longer true and it's optimized even if we use this func, right? I mean, we should still do this but it's no longer a TODO anyway...
-[Unknown]
There was a problem hiding this comment.
Right, removing comment.
This defers the command buffer generation until the end of the frame. This lets us deal with things like transitions and clears in a much better way, and will allow for some pretty cool optimizations in the future. Also, the Vulkan sync framebuffer readback code will later simply flush the queue, wait for completion, perform the copy and start a new "frame" without swapping backbuffers. That's not implemented yet though.
Essentially this reimplements parts of a traditional API driver on top of Vulkan since we can't reorganize games to be more Vulkan-friendly - so we'll have to do it as a post process.
This also contains initial code to do the command buffer generation on another thread while starting the next frame, similarly to what nVidia's D3D11 drivers do in the background. This is not quite working yet, but would be very beneficial on mobile where we have plenty of spare cores and command buffer generation is more expensive than on PC (not sure why that is the case but it is).
Performance impact seems negligible. Possibly because it's beneficial to the instruction cache do a lot of vulkan commands in a row...
This fixes a few games that were broken in Vulkan before like Ridge Racer.