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

Copy color to depth, defer depth buffer copies #15858

Merged
merged 7 commits into from
Aug 20, 2022
Merged

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Aug 17, 2022

Goes on top of #15857 (currently includes all of it, but will be rebased once that's merged).

This supersedes #15777 , and includes all of it. Anyway, it:

  • Removes the color-to-depth drawing mode, in favor of color-to-depth copies on bind
  • Makes the depth copy flow more logical, and a template for the upcoming color copy flow that will take care of Better ways to deal with overlapping render targets #15728
  • In case of missing depth texture support, binds a null texture instead of going to RAM.
  • Cleans up and refactors a bit

This has one known regression, which we can live with temporarily (the fix might shake out of future changes):

@hrydgard hrydgard force-pushed the copy-color-to-depth branch from 35366ab to 58a3da6 Compare August 18, 2022 05:15
@hrydgard hrydgard added the GE emulation Backend-independent GPU issues label Aug 18, 2022
@hrydgard hrydgard added this to the v1.14.0 milestone Aug 18, 2022
@hrydgard hrydgard marked this pull request as ready for review August 18, 2022 05:21
@hrydgard
Copy link
Owner Author

hrydgard commented Aug 18, 2022

I was hoping that this would fix #15491 (ratchet & clank) but the stumbling block there is how it uses a raw 565 copy (sampling as 565, writing 565) to stretch the depth buffer. Our matching logic rejects that.

It'll require something I've planned as one of the next steps, which is that if we texture-sample the "wrong" format, we create a clone of the framebuffer at the same address but with the desired format, and then let the planned logic to copy to color from overlapping buffers handle it (call CopyToColorFromOverlappingFramebuffers on it before texturing from it). Alternatively, it could be handled like depal, but without the CLUT. Might actually be the most straightforward path, though least generic. Not sure...

EDIT: This turned into #15859 (chose the depal path for now), though there are issues.

I think this is ready to go in as-is now.

@hrydgard hrydgard force-pushed the copy-color-to-depth branch from f8ab610 to 12db0e5 Compare August 20, 2022 06:36
@hrydgard
Copy link
Owner Author

I'm gonna get this in soon, it seems to work well now, with the lone exception of Colin McRae. I do plan to fix that one again, when I go look at the other problems it has (like its sun effect).

@@ -1685,6 +1685,16 @@ void GPUCommon::Execute_Prim(u32 op, u32 diff) {
return;
}

if (!gstate_c.usingDepth) {
bool isClearingDepth = gstate.isModeClear() && gstate.isClearModeDepthMask();;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: ;;.

-[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.

this was later removed.

} else if (source.channel == RASTER_COLOR) {
VirtualFramebuffer *src = source.vfb;
// Copying color to depth.
BlitUsingRaster(src->fbo, 0.0f, 0.0f, src->renderWidth, src->renderHeight, dest->fbo, 0.0f, 0.0f, dest->renderWidth, dest->renderHeight, false, DRAW2D_565_TO_DEPTH, "565_to_depth");
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Maybe log drawnFormat if not 565? Maybe related to the Colin thing?

-[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.

I'm planning to remove drawnFormat soon, and instead have duplicate VirtualFramebuffer, and automatic reinterpret copies between them on-demand. But yeah, can certainly add such logging in the meantime.

GPU/Common/FramebufferManagerCommon.cpp Show resolved Hide resolved
@@ -1957,7 +1959,7 @@ void FramebufferManagerCommon::UpdateFramebufUsage(VirtualFramebuffer *vfb) {

checkFlag(FB_USAGE_DISPLAYED_FRAMEBUFFER, vfb->last_frame_displayed);
checkFlag(FB_USAGE_TEXTURE, vfb->last_frame_used);
checkFlag(FB_USAGE_RENDERTARGET, vfb->last_frame_render);
checkFlag(FB_USAGE_RENDER_COLOR, vfb->last_frame_render);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, what if it's only rendering depth? I could see it being useful to know that... hm. Maybe for render-to-texture selection too. Can't remember if I've seen games render a scene with RGB and stencil all masked out, just to establish depth... feel like I have...

-[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.

Yeah, this tracking isn't perfect yet. I don't think I have seen such a case though, but I also don't think it'll hurt very much if we get that wrong..

hrydgard added a commit that referenced this pull request Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GE emulation Backend-independent GPU issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants