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.

Sign up for GitHub

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

Vulkan: Deal with the reformat clear better #12941

Merged
merged 2 commits into from
May 21, 2020
Merged

Vulkan: Deal with the reformat clear better #12941

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d10a0c
Remove a redundant framebuffer bind in CopyDisplayToOutput
hrydgard May 21, 2020
4aec10d
Correct an issue where reformat didn't work if no renderpass was acti…
hrydgard May 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions GPU/Vulkan/FramebufferVulkan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,14 @@ void FramebufferManagerVulkan::ReformatFramebufferFrom(VirtualFramebuffer *vfb,
// to exactly reproduce in 4444 and 8888 formats.

if (old == GE_FORMAT_565) {
// Previously started a render pass here to clear, but better to just, well, explicitly clear.
draw_->Clear(Draw::FBChannel::FB_COLOR_BIT | Draw::FBChannel::FB_STENCIL_BIT, 0, 0.0f, 0);
// We have to bind here instead of clear, since it can be that no framebuffer is bound.
// The backend can sometimes directly optimize it to a clear.
draw_->BindFramebufferAsRenderTarget(vfb->fbo, { Draw::RPAction::CLEAR, Draw::RPAction::KEEP, Draw::RPAction::CLEAR }, "ReformatFramebuffer");
Copy link
Collaborator

Choose a reason for hiding this comment

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

It was reported somewhere that there can be bugs when depthAction != stencilAction in some drivers. Not sure how worried to be.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Right, but we might want to take care of that in the RenderManager instead in that case... Hm.

// draw_->Clear(Draw::FBChannel::FB_COLOR_BIT | Draw::FBChannel::FB_STENCIL_BIT, 0, 0.0f, 0);

// Need to dirty anything that has command buffer dynamic state, in case we started a new pass above.
// Should find a way to feed that information back, maybe... Or simply correct the issue in the rendermanager.
gstate_c.Dirty(DIRTY_DEPTHSTENCIL_STATE | DIRTY_VIEWPORTSCISSOR_STATE | DIRTY_BLEND_STATE);
}
}

Expand Down
30 changes: 26 additions & 4 deletions ext/native/thin3d/VulkanRenderManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,11 +456,25 @@ VkCommandBuffer VulkanRenderManager::GetInitCmd() {

void VulkanRenderManager::BindFramebufferAsRenderTarget(VKRFramebuffer *fb, VKRRenderPassAction color, VKRRenderPassAction depth, VKRRenderPassAction stencil, uint32_t clearColor, float clearDepth, uint8_t clearStencil, const char *tag) {
assert(insideFrame_);
// Eliminate dupes.
// Eliminate dupes, instantly convert to a clear if possible.
if (!steps_.empty() && steps_.back()->stepType == VKRStepType::RENDER && steps_.back()->render.framebuffer == fb) {
if (color != VKRRenderPassAction::CLEAR && depth != VKRRenderPassAction::CLEAR && stencil != VKRRenderPassAction::CLEAR) {
// We don't move to a new step, this bind was unnecessary and we can safely skip it.
// Not sure how much this is still happening but probably worth checking for nevertheless.
u32 clearMask = 0;
if (color == VKRRenderPassAction::CLEAR) {
clearMask |= VK_IMAGE_ASPECT_COLOR_BIT;
}
if (depth == VKRRenderPassAction::CLEAR) {
clearMask |= VK_IMAGE_ASPECT_DEPTH_BIT;
}
if (stencil == VKRRenderPassAction::CLEAR) {
clearMask |= VK_IMAGE_ASPECT_STENCIL_BIT;
}

// If we need a clear and the previous step has commands already, it's best to just add a clear and keep going.
// If there's no clear needed, let's also do that.
//
// However, if we do need a clear and there are no commands in the previous pass,
// we want the queuerunner to have the opportunity to merge, so we'll go ahead and make a new renderpass.
if (clearMask == 0 || !steps_.back()->commands.empty()) {
curRenderStep_ = steps_.back();
curStepHasViewport_ = false;
curStepHasScissor_ = false;
Expand All @@ -473,6 +487,14 @@ void VulkanRenderManager::BindFramebufferAsRenderTarget(VKRFramebuffer *fb, VKRR
break;
}
}
if (clearMask != 0) {
VkRenderData data{ VKRRenderCommand::CLEAR };
data.clear.clearColor = clearColor;
data.clear.clearZ = clearDepth;
data.clear.clearStencil = clearStencil;
data.clear.clearMask = clearMask;
curRenderStep_->commands.push_back(data);
}
return;
}
}
Expand Down