AlpyneDreams / d8vk

Direct3D 8 to Vulkan translation for DXVK! Merged into dxvk: https://github.com/doitsujin/dxvk
zlib License
485 stars 8 forks source link

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

Closed WinterSnowfall closed 4 months ago

WinterSnowfall commented 1 year ago

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

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

WinterSnowfall commented 1 year ago

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 commented 1 year ago

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: https://github.com/doitsujin/dxvk/pull/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 commented 11 months ago

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 commented 10 months ago

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 commented 5 months ago

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.