godot
godot copied to clipboard
godotengine
Direct 3D 12: Implement proper fallback for format casting
In short, this removes the ugly aliasing hack used when relaxed casting is not available and adds the proper reinterpret-copy or copy-via-buffer approach.
In other words, this PR fixes SDFGI for those drivers not implementing relaxed casting and where the current hack was not working properly.
Includes #87872.
UPDATE: I've updated the code disabling relaxed casting for now. (See https://github.com/godotengine/godot/pull/88027#issuecomment-1934199704.)
The PR kind of works as expected and doesn't. In its current state, it did not fix the issue at all for me, but apparently it's because whenever this is called...
D3D12_FEATURE_DATA_D3D12_OPTIONS12 options12 = {};
res = device->CheckFeatureSupport(D3D12_FEATURE_D3D12_OPTIONS12, &options12, sizeof(options12));
if (SUCCEEDED(res)) {
format_capabilities.relaxed_casting_supported = options12.RelaxedFormatCastingSupported;
}
My system in particular (NVIDIA RTX 3090 Ti with latest driver, Windows 11 with the latest Windows SDK) will return that this feature is supported. However, as I mentioned to @RandomShaper before, it seems like a bit of a false positive as the view creation expecting relaxed format casting to work will fail and result in a crash afterwards. That poses a bit of a problem, as it seems either the driver or the SDK being used to build it is causing this weird issue (I tested with different SDKs and could not get the result to be different). It seems that whatever driver stack I'm stuck with will incorrectly report that the feature is supported when it's not.
Force-disabling the setting however works as expected and finally makes SDFGI work in D3D12, so I can confirm that the path does its job as long as the format capability is detected properly. I'm not sure the PR is at fault here as the format capability checking is a pretty standard implementation. But it seems likely we can't quite rely on the format casting flag to identify if it truly works or not in some particular scenarios. Perhaps we can rely on creating something and checking if it fails instead? Or address the IHVs who report this feature as true into checking what the problem is?
EDIT: The fallback might not be that viable as the error is pretty fatal when it occurs.
D3D12 ERROR: ID3D12Device::CreateShaderResourceView: The Format (0x43, R9G9B9E5_SHAREDEXP) is invalid, when creating a View; it is not a fully qualified Format castable from the Format of the Resource (0x27, R32_TYPELESS). [ STATE_CREATION ERROR #28: CREATESHADERRESOURCEVIEW_INVALIDFORMAT]
D3D12: Removing Device.
D3D12 ERROR: ID3D12Device::RemoveDevice: Device removal has been triggered for the following reason (DXGI_ERROR_INVALID_CALL: There is strong evidence that the application has performed an illegal or undefined operation, and such a condition could not be returned to the application cleanly through a return code). [ EXECUTION ERROR #232: DEVICE_REMOVAL_PROCESS_AT_FAULT]
An option would be to disable the use of relaxed casting even if advertised as supported depending on OS and GPU vendor, but I'd rather do that in a separate PR or, in any case, consider it a separate issue that should indeed be interesting to report to Nvidia.
An option would be to disable the use of relaxed casting even if advertised as supported depending on OS and GPU vendor, but I'd rather do that in a separate PR or, in any case, consider it a separate issue that should indeed be interesting to report to Nvidia.
It might be the safest option to indeed just not check for support until we have a solid confirmation that the feature is widely available and supported on more hardware.
Are you sure sure that a driver update won't do the trick for you?
I don't have more driver updates to do. 😞
I've added temporary code to disable it. See the updated PR description. (Warning: the PR description asks to check your comment above. If one then reads the next comment, that is, this one, they will enter an infinite loop.)
Can be rebased now that the first commit was merged.
Done. I was just now wondering why GitHub didn't just subsume the superfluous commit. Didn't that use to be the case?
Can be rebased now that the first commit was merged.
Done. I was just now wondering why GitHub didn't just subsume the superfluous commit. Didn't that use to be the case?
I don't think it does, but merging the PR as is might still work and not introduce an extra commit. It's just that the PR itself still looks like it includes two new commits not in master, which is a bit confusing so I prefer to rebase anyway.