doitsujin / dxvk

Vulkan-based implementation of D3D8, 9, 10 and 11 for Linux / Wine
zlib License
12.74k stars 818 forks source link

[d3d9] DxvkContext::updatePushConstants always uses complete layout to update push constant #3971

Closed linqun closed 4 months ago

linqun commented 4 months ago

I encounter multiple d3d9 app hang issue with AMDVLK driver after enable graphics pipeline library. and it looks all hang issues are due to incompatible pipeline layout is used in dxvk.

DXVK creates two pipeline layout independentLayout and completeLayout.

But it looks DXVK sometimes use completeLayout to update resources, then draw with pipeline which is created from independentLayout.

After checking the source code, I found DxvkContext::updatePushConstants always uses complete layout to update push constant. it looks incorrect. could you have a look? thanks!

doitsujin commented 4 months ago

The Vulkan spec states:

Pipeline Layout Compatibility Two pipeline layouts are defined to be “compatible for push constants” if they were created with identical push constant ranges.

The wording of the spec strongly suggests that VK_PIPELINE_LAYOUT_CREATE_INDEPENDENT_SETS_BIT_EXT only affects descriptor set compaibility, which we do honor, and that DXVK doesn't do anything illegal here.

doitsujin commented 4 months ago

We have a different bug with push constants in that the D3D9 front-end declares different push constant ranges for vertex and fragment shaders, maybe that's the problem? Unfortunately that is far more difficult to fix.

doitsujin commented 4 months ago

https://github.com/doitsujin/dxvk/tree/push-const-fix This branch should fix the problem.

I also bumped VkApplicationInfo::engineVersion to 2.3.2 so that the driver can disable GPL for versions ≤2.3.1.

linqun commented 4 months ago

Verified with some d3d9 apps, include GTA4, Tropico4 and Team Fortress2. It works with AMD driver. Thank you very much for the quick fix. Please let me know once the change is merged. thanks!

doitsujin commented 4 months ago

Merged, thanks for confirming.