Skip to content

Commit

Permalink
Vulkan: Also check for empty clearing renderpasses later in the frame.
Browse files Browse the repository at this point in the history
  • Loading branch information
@hrydgard
hrydgard committed Mar 16, 2018
1 parent 17eca3e commit a08464e
Showing 1 changed file with 30 additions and 23 deletions.
53 changes: 30 additions & 23 deletions ext/native/thin3d/VulkanQueueRunner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,30 +260,37 @@ void VulkanQueueRunner::RunSteps(VkCommandBuffer cmd, const std::vector<VKRStep

// Push down empty "Clear/Store" renderpasses, and merge them with the first "Load/Store" to the same framebuffer.
// Actually let's just bother with the first one for now. This affects Wipeout Pure.
if (steps.size() > 1 && steps[0]->stepType == VKRStepType::RENDER &&
steps[0]->render.numDraws == 0 &&
steps[0]->render.color == VKRRenderPassAction::CLEAR &&
steps[0]->render.stencil == VKRRenderPassAction::CLEAR &&
steps[0]->render.depth == VKRRenderPassAction::CLEAR) {
// Drop the first step, and merge it into the next step that touches the same framebuffer.
for (size_t i = 1; i < steps.size(); i++) {
if (steps[i]->stepType == VKRStepType::RENDER &&
steps[i]->render.framebuffer == steps[0]->render.framebuffer) {
if (steps[i]->render.color != VKRRenderPassAction::CLEAR) {
steps[i]->render.color = VKRRenderPassAction::CLEAR;
steps[i]->render.clearColor = steps[0]->render.clearColor;
for (int j = 0; j < (int)steps.size() - 1; j++) {
if (steps.size() > 1 && steps[j]->stepType == VKRStepType::RENDER &&
steps[j]->render.numDraws == 0 &&
steps[j]->render.color == VKRRenderPassAction::CLEAR &&
steps[j]->render.stencil == VKRRenderPassAction::CLEAR &&
steps[j]->render.depth == VKRRenderPassAction::CLEAR) {

//if (j != 0)
// __debugbreak();

// Drop the first step, and merge it into the next step that touches the same framebuffer.
for (size_t i = j + 1; i < steps.size(); i++) {

This comment has been minimized.

Sign in to view

Copy link
@unknownbrackets

unknownbrackets Mar 17, 2018

Collaborator

Hm. This could be unsafe if there's a dependency. For example:

RT1: Clear
if (false) {
RT1: Render
}
RT2: Render from RT1

In that case, we would now render from garbage I think?

-[Unknown]

This comment has been minimized.

Sign in to view

Copy link
@hrydgard

hrydgard Mar 17, 2018 •
edited
Loading

Author Owner

What's with the "if (false) {" ? That doesn't really happen, right?

But hm. The one case I can see a danger is:

Rt1: Clear
Rt2: Render, texturing from Rt1 (which is cleared, so no good reason to do this)
Rt1: Render

Which gets collapsed to

(disabled pass, previously "Rt1: Clear")
Rt2: Render: Texturing from Rt1
Rt1: Clear, render

To be safe, we need to keep track of the framebuffers that we bind as textures in each render pass, but as far as I can tell, this is only an issue if texturing from a known cleared target and I don't think games do that a lot..

This comment has been minimized.

Sign in to view

Copy link
@unknownbrackets

unknownbrackets Mar 17, 2018

Collaborator

I meant, consider a situation where the game doesn't always render to RT1 (possibly because of BJUMP or whatever.) So if (someGameCodeThatReturnsFalse()) {, if that's clearer.

-[Unknown]

This comment has been minimized.

Sign in to view

Copy link
@hrydgard

hrydgard Mar 17, 2018 •
edited
Loading

Author Owner

I see. Yeah, in that case the driver bug glitch returns (the clear doesn't get eliminated any more since the search will not find a match in the frame, we're looking at things way past BJUMPs), and turns out it does exactly this in a loading screen of Ridge Racer. Oh well...

This comment has been minimized.

Sign in to view

Copy link
@unknownbrackets

unknownbrackets Mar 17, 2018

Collaborator

Sure, I was speaking from the game's perspective in an obviously failed attempt at clarity (since we're in control of what happens from game -> render manager, we don't just have to take our lumps - as we do for what the game sends.) I'm not under some delusion that we pass BJUMPs to the render manager.

BindFramebufferAsTexture already checks the dependency, so naively (not thinking about shadows), we could just add the dependency to a vector in that code. Another thing to think about is non-render steps, like copy, which I think this current code also gets wrong. Example:

  1. RENDER - RT1: Draw
  2. RENDER - RT2: Clear
  3. COPY - RT2: Copy RT1 to RT2
  4. RENDER - RT2: Draw

In this case, we'd move forward step 2 (the clear) to step 4 (the first draw) and we'd silently erase the copy.

Obviously, it's silly for games to clear buffers and then immediately copy over them, but I'm pretty sure I've seen this (possibly with a smaller copy than clear or something.)

-[Unknown]

This comment has been minimized.

Sign in to view

Copy link
@hrydgard

hrydgard Mar 17, 2018

Author Owner

You're right. Maybe should back this out and reintroduce the LAYOUT_GENERAL hack, and think this through a bit more before restoring it.
if (steps[i]->stepType == VKRStepType::RENDER &&
steps[i]->render.framebuffer == steps[j]->render.framebuffer) {
if (steps[i]->render.color != VKRRenderPassAction::CLEAR) {
steps[i]->render.color = VKRRenderPassAction::CLEAR;
steps[i]->render.clearColor = steps[j]->render.clearColor;
}
if (steps[i]->render.depth != VKRRenderPassAction::CLEAR) {
steps[i]->render.depth = VKRRenderPassAction::CLEAR;
steps[i]->render.clearDepth = steps[j]->render.clearDepth;
}
if (steps[i]->render.stencil != VKRRenderPassAction::CLEAR) {
steps[i]->render.stencil = VKRRenderPassAction::CLEAR;
steps[i]->render.clearStencil = steps[j]->render.clearStencil;
}
// Cheaply skip the first step.
steps[j]->stepType = VKRStepType::RENDER_SKIP;
ILOG("Avoided a lone clear at step %d, merged it with step %d", (int)j, (int)i);
break;
}
if (steps[i]->render.depth != VKRRenderPassAction::CLEAR) {
steps[i]->render.depth = VKRRenderPassAction::CLEAR;
steps[i]->render.clearDepth = steps[0]->render.clearDepth;
}
if (steps[i]->render.stencil != VKRRenderPassAction::CLEAR) {
steps[i]->render.stencil = VKRRenderPassAction::CLEAR;
steps[i]->render.clearStencil = steps[0]->render.clearStencil;
}
// Cheaply skip the first step.
steps[0]->stepType = VKRStepType::RENDER_SKIP;
break;
}
}
}
Expand Down

0 comments on commit a08464e

Please sign in to comment.