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

[d3d8] Ensure all references are cleared before d3d9 device reset #172

Merged
merged 3 commits into from
Jul 7, 2024

Conversation

WinterSnowfall
Copy link

@WinterSnowfall WinterSnowfall commented Jul 4, 2023

Now that we've imported the new D3D9 device reset validations we also need ensure that:

  • We clear all d3d8 cached references before calling d3d9 device reset

  • We regenerate/rebind the backbuffers and autoDS after the d3d9 device reset succeeds

  • We actually free state blocks on calls to DeleteStateBlock()

Otherwise some d3d8 games will livelock and crash, complaining they can't reset the device with bound resources (it was the default render target, depth stencil and state blocks that were bound from our end).

Fixes #174, fixes #175, partially addresses #59 (fixes the startup crash, as that seems to have been state block related).

src/d3d8/d3d8_device.cpp Outdated Show resolved Hide resolved
src/d3d8/d3d8_device.cpp Show resolved Hide resolved
@WinterSnowfall
Copy link
Author

WinterSnowfall commented Sep 17, 2023

There still seem to be some issues around bound resources (e.g. with Scrapland Remastered at times), but I don't think there's anything left to fix on d8vk end. Remaining discrepancies may just be games doing stupid things or differences in handling between d3d8 and d3d9 (let's hope not...).

Either way, this PR fixes most of the issues, but caveat emptor the saga may not be entirely over.

@WinterSnowfall
Copy link
Author

WinterSnowfall commented Sep 19, 2023

As I long feared, there's a difference between how d3d8 and d3d9 handle losable resource validation on device resets, in that d3d9 counts state blocks as losable resources while d3d8 does not. This appears to be a rare case where the docs are actually on point:

Here's d3d8:

Before calling the Reset method for a device, an application should release any explicit render targets, depth stencil surfaces, additional swap chains and D3DPOOL_DEFAULT resources associated with the device.

And d3d9:

Before calling the IDirect3DDevice9::Reset method for a device, an application should release any explicit render targets, depth stencil surfaces, additional swap chains, state blocks, and D3DPOOL_DEFAULT resources associated with the device.

We'll have to disable losable counts for state blocks in d9vk via the bridge when using it with d8vk. I'll PR something separately against dxvk to address this.

Edit: doitsujin#3665
Need to add a one-liner in the bridge constructor after this is merged and we rebase, after which is should be fine (hopefully for good).

@WinterSnowfall
Copy link
Author

Since there may be some time still until we rebase and all of this is merged, note to self that with the latest upstream code we still need to plop a call to SetD3D8CompatibilityMode as follows:

// Get the bridge interface to D3D9.
if (FAILED(GetD3D9()->QueryInterface(__uuidof(IDxvkD3D8Bridge), (void**)&m_bridge))) {
  throw DxvkError("D3D8Device: ERROR! Failed to get D3D9 Bridge. d3d9.dll might not be DXVK!");
}

m_bridge->SetAPIName("D3D8");
m_bridge->SetD3D8CompatibilityMode(true);

for everything to work properly.

@WinterSnowfall
Copy link
Author

WinterSnowfall commented Jan 15, 2024

And there I was, dear reader, chasing refs again...

D3DPOOL_DEFAULT resources are a problem now, since anything lingering on a device reset will cause games to crash. I managed to narrow down such an obscure case, by following it up to the temporary blit surfaces we were (in some cases) creating during CopyRects.

The problem there was mainly with that extra refcount that was keeping the blit surfaces around even after surface (containing object) destruction - since we're passing everything around as a public COM, the refcount for m_blitImage will stay a very constant 1 (as sent to us by d3d9). COM1 = COM2 will always keep COM2's refcount constant (increase on assignment to COM1, decrease on release of COM1), so that makes sense.

With the extra ref it was, unfortunately, always stuck at a very constant 2 and never destroyed. An easy enough fix, but it took me quite some time to figure it out.

Also took the liberty to use CreateOffscreenPlainSurface instead of CreateRenderTarget, because:
- It's now very obvious we're creating the surface in D3DPOOL_DEFAULT
- Less useless parameters passed around
- The naming better suits our end purpose, since we don't actually need a RT

Nevemind on the last bit, turns out using RTs is somewhat faster (probably because they're not lockable). I've added a note so that we don't forget we need to be extra careful when messing about with object lifecycle in this area.

@WinterSnowfall
Copy link
Author

I've included the bridge call to enable the D3D8 compatibility mode (which currently only affects state block tracking for device reset). Note however this will cause issues until a rebase is performed, since the necessary bridge code was added directly upstream.

@AlpyneDreams AlpyneDreams merged commit 1a8b65d into AlpyneDreams:main Jul 7, 2024
4 checks passed
@WinterSnowfall WinterSnowfall deleted the d8vk-resetsplit branch July 7, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Castle Strike break when alt tabbing Deer Hunter 2004 resolution change freeze
3 participants