-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
WIP: Virtual readbacks #11531
WIP: Virtual readbacks #11531
Conversation
bdae73f
to
98924d0
Compare
GPU/Common/FramebufferCommon.cpp
Outdated
vfb->fb_address = dstBasePtr; // NOTE - not necessarily in VRAM! | ||
vfb->fb_stride = dstStride; | ||
vfb->z_address = 0; | ||
vfb->z_stride =0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Spacing. Maybe we should verify that other code would fill these values in if the framebuf was drawn to later...
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good point. This can be quite dangerous otherwise...
GPU/Common/FramebufferCommon.cpp
Outdated
vfbs_.push_back(vfb); | ||
dstBuffer = vfb; | ||
} | ||
dstBuffer->last_frame_used = gpuStats.numFlips; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be inside the if, or else be inside a nullcheck.
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At that point, dstBuffer is guaranteed not to be null. Plus, these targets that we don't otherwise render to but texture from do need their last_frame_used updated, otherwise they'll quickly be decimated. It's kind of a bug that we didn't update this before on copy targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When BlockTransferAllowCreateFB
is false, ifdstBuffer
cannot be null - why do we even need the !dstBuffer &&
check above, then?
-[Unknown]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um.. right. Oops, sorry was too hasty :)
Reportedly the speedup is massive in Digimon Adventures and the toon borders are sharp, with this. So seems promising. I really want to rethink and unify textures, framebuffers and CLUTs long term, but some ideas I have would require at least ES 3.0 level features and preferably higher. I really wish we could just forget about ES 2.0 and DX9 but we can't. So we're probably stuck with our current pile of hacks for some time longer, and this is just an addition to it. I will improve this further before merging of course, at least to fix the MotoGP case too so we can delete one hack while adding this one. |
Prerequisite for #11531, virtual readbacks.
98924d0
to
999bdbb
Compare
Prerequisite for #11531, virtual readbacks.
Rebased on #11553 and then master, added support in the detected-memcpy path as well and killed the old MotoGP hack. This can actually also be used to avoid the sun flare readbacks in Burnout, the sun flare is working but it feels slightly different somehow, maybe because higher resolution. Not sure if I want to enable that - it does end up creating a ton of tiny framebuffers since it varies the size of its readback. The only important thing left is to make it play nice if a game starts to render to one of these buffers. This can only happen if it's in VRAM, but Digimon is a case where that applies (though I don't know if it actually ever tries to render to one of these buffers). |
Prerequisite for #11531, virtual readbacks.
…b from readback destination). Does not yet work outside VRAM but should fix Digimon Adventure.
dd88d49
to
32cd6df
Compare
That last part is done, and it's re-rebased on framebuffers-outside-vram. |
I wonder if we want to apply "lower resolution for effects" to this or anything... -[Unknown] |
Hm, that's an interesting point.. Definitely not for Digimon Adventure outlines, but maybe for Burnout's sun. |
Burnout's sun is still same |
I know, I didn't enable this for Burnout yet. |
As outlined in #11528
Fixes performance in Digimon Adventures and lets us remove the old MotoGP hack.
In addition there are multiple other games this could help but we'll add them later.