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

VideoBackends: fix d3d12 subresource calculation #11224

Merged
merged 1 commit into from Oct 29, 2022

Conversation

iwubcode
Copy link
Contributor

This pulls out the D3D12 fix for calculating the subresource from my post processing overhaul.

See section "subresource indexing" in https://learn.microsoft.com/en-us/windows/win32/direct3d12/subresources

Example, if our level count is 2 and our layer count is 3. We want to access the 2nd level and the 2nd layer.

We'd have this in master:

CalcSubresources(1, 1) => 4

vs this with the change:

CalcSubresources(1, 1) => 3

@Pokechu22
Copy link
Contributor

The fix looks reasonable to me. Is this known to fix anything in practice on master? Also, is there any reason why D3D12CalcSubresource isn't in use? DXTexture.cpp (for D3D11) uses D3D11CalcSubresource.

@iwubcode
Copy link
Contributor Author

iwubcode commented Oct 29, 2022

Is this known to fix anything in practice on master

Not aware of anything, no. Then again, D3D12 is generally regarded as quite crashy. So who knows..

Also, is there any reason why D3D12CalcSubresource isn't in use? DXTexture.cpp (for D3D11) uses D3D11CalcSubresource.

Interesting, I wasn't aware of that function. Good question, not sure why that's not used. Maybe the helper was introduced to avoid the overhead of so many parameters, that's the only reason I can think of. Or maybe it just slipped the author's mind, this was in the original commit after-all.

I suppose I'll close this out. Someone can migrate to D3D12CalcSubresource instead (maybe I'll get to it at some point, gotta re-test with my PR though).

@iwubcode iwubcode closed this Oct 29, 2022
@iwubcode
Copy link
Contributor Author

iwubcode commented Oct 29, 2022

Ok, so decided to look at it more and see that D3D12CalcSubresource is not in the default header. It's in a helper header defined by MS: https://learn.microsoft.com/en-us/windows/win32/direct3d12/helper-functions-for-d3d12

Not sure if it's worth adding a whole header when just this function is fine. Maybe that was the original author's reasoning too. If we ever decide to introduce this header, we can revisit. Reopened for feedback.

@iwubcode iwubcode reopened this Oct 29, 2022
Copy link
Contributor

@Pokechu22 Pokechu22 left a comment

Choose a reason for hiding this comment

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

Ah, ok. We probably don't need to add the header. This implementation does match the implementation there (apart from not having planes), so it should be fine.

@AdmiralCurtiss AdmiralCurtiss merged commit 0716fa3 into dolphin-emu:master Oct 29, 2022
11 checks passed
@iwubcode iwubcode deleted the d3d12_subresource branch October 29, 2022 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants