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

Direct 3D 12: Implement proper fallback for format casting #88027

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Feb 6, 2024

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 #88027 (comment).)

@RandomShaper RandomShaper added this to the 4.3 milestone Feb 6, 2024
@RandomShaper RandomShaper requested a review from a team as a code owner February 6, 2024 18:42
@DarioSamo
Copy link
Contributor

I ran into this exact issue so I should confirm later if it works.

@DarioSamo DarioSamo self-assigned this Feb 7, 2024
@DarioSamo
Copy link
Contributor

DarioSamo commented Feb 8, 2024

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]

@RandomShaper
Copy link
Member Author

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.

@DarioSamo
Copy link
Contributor

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.

@RandomShaper RandomShaper force-pushed the d3d12_sdfgi_cool branch 3 times, most recently from 240d958 to e84f7d0 Compare February 8, 2024 15:01
@RandomShaper
Copy link
Member Author

Are you sure sure that a driver update won't do the trick for you?

@DarioSamo
Copy link
Contributor

Are you sure sure that a driver update won't do the trick for you?

I don't have more driver updates to do. 😞

@RandomShaper
Copy link
Member Author

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

@RandomShaper
Copy link
Member Author

Rebased! (And retested.)

@RandomShaper RandomShaper force-pushed the d3d12_sdfgi_cool branch 3 times, most recently from 1f76d61 to 5938407 Compare February 15, 2024 19:09
Copy link
Contributor

@DarioSamo DarioSamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback does work for me, there's just a couple of changes we might want to do like addressing the bitset that is using more bytes than it needs to. I haven't spotted anything on the implementation of the fallback itself.

@RandomShaper RandomShaper force-pushed the d3d12_sdfgi_cool branch 2 times, most recently from f5f2018 to a63db29 Compare February 20, 2024 11:58
@RandomShaper RandomShaper force-pushed the d3d12_sdfgi_cool branch 2 times, most recently from ff28397 to 5004a7e Compare February 27, 2024 14:47
@akien-mga
Copy link
Member

Can be rebased now that the first commit was merged.

@RandomShaper
Copy link
Member Author

RandomShaper commented Feb 27, 2024

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?

@akien-mga
Copy link
Member

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.

@akien-mga akien-mga merged commit d1a6fd3 into godotengine:master Feb 27, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the d3d12_sdfgi_cool branch February 28, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants