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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 0 additions & 7 deletions GPU/Common/FramebufferCommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -894,13 +894,6 @@ void FramebufferManagerCommon::CopyDisplayToOutput(bool reallyDirty) {
if (Memory::IsValidAddress(fbaddr)) {
// The game is displaying something directly from RAM. In GTA, it's decoded video.
if (!vfb) {
if (useBufferedRendering_) {
// Bind and clear the backbuffer. This should be the first time during the frame that it's bound.
draw_->BindFramebufferAsRenderTarget(nullptr, { Draw::RPAction::CLEAR, Draw::RPAction::CLEAR, Draw::RPAction::CLEAR }, "CopyDisplayToOutput_Backbuffer");
}
// Just a pointer to plain memory to draw. We should create a framebuffer, then draw to it.
SetViewport2D(0, 0, pixelWidth_, pixelHeight_);
draw_->SetScissorRect(0, 0, pixelWidth_, pixelHeight_);
DrawFramebufferToOutput(Memory::GetPointer(fbaddr), displayFormat_, displayStride_);
gstate_c.Dirty(DIRTY_BLEND_STATE);
return;
Expand Down
10 changes: 8 additions & 2 deletions GPU/Vulkan/FramebufferVulkan.cpp
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
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