-
Notifications
You must be signed in to change notification settings - Fork 855
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
Upload per-draw data for dynamic systemmem buffers #3765
Conversation
@ Josh If you have any ideas how to make this a bit less ugly, I'm all ears. |
a824f79
to
10658eb
Compare
BaseVertexIndex is signed in Vulkan, D3D11 & D3D9.
2517b6d
to
2a87024
Compare
Nine turns SYSMEM + DYNAMIC into PIPE_STREAM buffers, I assume the underlying driver will then do something similar as this PR. |
I changed the PR so it does the dynamic upload for either vertex buffers or the index buffer even if the other one doesn't qualify for dynamic uploads. Clickteam Fusion (used by Flammable Freddy) hits this case with a Usage=0 index buffer and a large Usage=Dynamic vertex buffer. |
During testing it has been found that the PR causes BloodRayne: Terminal Cut to only render black. The game itself is d3d8 but this edition ships with a dll that translates it to 9. Apitrace that reproduces the issue. |
src/d3d9/d3d9_device.cpp
Outdated
@@ -2615,7 +2615,22 @@ namespace dxvk { | |||
if (unlikely(!PrimitiveCount)) | |||
return S_OK; | |||
|
|||
PrepareDraw(PrimitiveType); | |||
bool dynamicSysmemVBOs = true; |
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.
If every vbo is nullptr, this should be false to handle the case of Draws with 0 vertex buffers bound, right?
src/d3d9/d3d9_device.cpp
Outdated
@@ -2652,7 +2667,34 @@ namespace dxvk { | |||
if (unlikely(!PrimitiveCount)) | |||
return S_OK; | |||
|
|||
PrepareDraw(PrimitiveType); | |||
bool dynamicSysmemVBOs = true; | |||
for (uint32_t i = 0; i < caps::MaxStreams && dynamicSysmemVBOs; i++) { |
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.
Can we de-dupe this code between the two places?
src/d3d9/d3d9_device.cpp
Outdated
|
||
PrepareDraw(PrimitiveType, !dynamicSysmemVBOs, !dynamicSysmemIBO); | ||
|
||
if (unlikely(dynamicSysmemVBOs || dynamicSysmemIBO)) { |
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.
I don't really like this open-coded here, can we move it to PrepareDraw?
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.
Feel free to gate it behind some constexpr thing for indexed vs not.
858513e
to
6478ffb
Compare
@Joshua-Ashton Updated it, pls give some feedback on the changes. |
Fixes #3755
Fixes #2375
Fixes #1828
Every frame Shank 2 maps multiple >1MB buffers with SizeToLock = 0.
That means we cannot just upload a subset of the buffer but have to upload the entire buffer multiple times per frame.
Copying that much data around is slow and on top of that it exceeds our 16MB staging buffer budget which makes us stall until the GPU is done with the earlier staging data.
This is actually the first time I've seen a game blow that budget outside of loading screens.
This PR changes it so that draws that only use buffers with D3DPOOL_SYSTEMMEM + D3DUSAGE_DYNAMIC upload all required data before each draw similar to DrawPrimitiveUp/DrawIndexedPrimitiveUp.
This reduces the amount of data copied each frame in Shank 2 from >30MB to <1MB.
I assume D3D9 drivers (or the runtime) do something similar given that applications need to actually provide information about the accessed vertices for indexed draws (https://learn.microsoft.com/en-us/windows/win32/api/d3d9/nf-d3d9-idirect3ddevice9-drawindexedprimitive).