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: Add Metal backend #10754

Merged
merged 12 commits into from Jul 23, 2022
Merged

VideoBackends: Add Metal backend #10754

merged 12 commits into from Jul 23, 2022

Conversation

TellowKrinkle
Copy link
Contributor

Adds a Metal backend.

Currently includes code from lots of other PRs. Will shrink once those merge. PRing now because people wanted builds to test.

@Pokechu22
Copy link
Contributor

Dolphin's build infrastructure won't build versions that contain merge conflicts; please rebase.

vendor = DriverDetails::VENDOR_APPLE;
double version = 0;
if (@available(macOS 13, iOS 16, *))
version = 3.0;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it would be better to use macOS version instead of the Metal version for DriverDetails, since Apple may fix driver bugs in minor macOS updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then wouldn't we have to figure out some versioning system that unifies iOS and macOS versions? Then again if they did fix a bug in a minor update, there's no guarantee it would come to iOS and macOS at the same time...

Copy link
Member

Choose a reason for hiding this comment

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

Technically, you don't need to worry about iOS since Dolphin has no official iOS support. (That being said, having more things upstreamed would make my fork easier to maintain...)

Copy link
Member

Choose a reason for hiding this comment

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

OK, so I had a chat with @JMC47 and they said it should be fine to merge some iOS support upstream.

I think we should use the OS version instead of the Metal version for DriverDetails, and just have separate entries for iOS and macOS. That's probably the easiest and cleanest way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay
I put it in as major * 100 + minor, but if you prefer we can enjoy our floating point rounding fun and go for major + minor / 100

bool supports_apple4 = false;
if (@available(iOS 13, *))
{
supports_mac1 = [device supportsFamily:MTLGPUFamilyMac1];
Copy link
Member

Choose a reason for hiding this comment

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

You're checking for a Mac GPU family in code that runs on iOS. Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought M1 iPads might support it? Don't have one to test though.

@shuffle2
Copy link
Contributor

@TellowKrinkle you can run Tools\lint.sh or use your editor's .clang-format integration to automatically fix lint issues.

@pizuz
Copy link

pizuz commented Jun 17, 2022

Cool! My quick Wind Waker test shows quite a performance boost. However, anything containing shaders makes Dolphin crash on shader pre-compilation.

My machine:
iMac Late 2013 on macOS Catalina 10.15.7
3,1 GHz Quad-Core Intel Core i7
NVIDIA GeForce GT 750M 1 GB

metal_shader_crash.txt

@shuffle2
Copy link
Contributor

spirv_cross::report_and_abort

https://github.com/KhronosGroup/SPIRV-Cross/blob/master/spirv_cross_error_handling.hpp#L49

seems debug build should print something useful

@pizuz
Copy link

pizuz commented Jun 17, 2022

spirv_cross::report_and_abort

https://github.com/KhronosGroup/SPIRV-Cross/blob/master/spirv_cross_error_handling.hpp#L49

seems debug build should print something useful

Yes, a debug build for macOS would be a good idea…

@TellowKrinkle
Copy link
Contributor Author

@TellowKrinkle you can run Tools\lint.sh or use your editor's .clang-format integration to automatically fix lint issues.

I did that and accepted the reasonable looking changes. The rest are on my list of things to mess with so clang-format accepts them. If you happen to know why it wants to misalign these method calls, that would be very helpful to know.

clang-format telling me to indent method calls so the colons are one space away from lining up

Cool! My quick Wind Waker test shows quite a performance boost. However, anything containing shaders makes Dolphin crash on shader pre-compilation.

My machine: iMac Late 2013 on macOS Catalina 10.15.7 3,1 GHz Quad-Core Intel Core i7 NVIDIA GeForce GT 750M 1 GB

metal_shader_crash.txt

I've been having a lot of issues with uid caches not getting invalidated. We'll figure it out by the time things get merged, but for now, try deleting all the uidcache files in your ~/Library/Application Support/Dolphin/Cache directory and see if that fixes anything.

@shuffle2
Copy link
Contributor

@TellowKrinkle you can run Tools\lint.sh or use your editor's .clang-format integration to automatically fix lint issues.

I did that and accepted the reasonable looking changes. The rest are on my list of things to mess with so clang-format accepts them. If you happen to know why it wants to misalign these method calls, that would be very helpful to know.

I don't know, but some quick playing around shows that using Webkit or Mozilla BasedOnStyle makes clang-format output it how you like (I assume). So you can probably find some setting to tweak that specific thing.

tbh I personally just let the editor autoformat and ignore the specifics.

@TellowKrinkle
Copy link
Contributor Author

Okay looks like it was an issue with the indentation level. Using longer variable names fixes it.

The rest of the formatting is stuff that's fixed in the (separate) PRs for some of the other pieces of this, so it should clear up when those get merged and I rebase over

@pizuz
Copy link

pizuz commented Jun 18, 2022

I've been having a lot of issues with uid caches not getting invalidated. We'll figure it out by the time things get merged, but for now, try deleting all the uidcache files in your ~/Library/Application Support/Dolphin/Cache directory and see if that fixes anything.

That did the trick, thanks. Didn't find any graphical corruption, yet and it seems very stable so far. Most slowdowns I had with MoltenVK are practically gone. I can even go to higher resolutions, now, which was never possible with either OpenGL or Vulkan. Apparently, the MoltenVK overhead was indeed quite substantial.

@nolrinale
Copy link
Contributor

Rebuilt today with your latest changes and while playing PSO tested the new MSAA support and there are small line artifacts in some textures in the backgrounds with it enabled, they display correctly if you deactivate MSAA, tested with both MSAA 2x and 4x, none of this happens with the SSAA's

Is not too noticeable (but its still there) if you upscale beyond 3x or 4x or 5x + enabling MSAA
These screenies were taken with 2x upscale, 24bit color, Scaled EFB Copy

With MSAA
image

Without MSAA
image

FIFO
FIFOMetal03PSO.dff.zip

@nolrinale
Copy link
Contributor

Upon further inspection I can confirm MSAA is working as intended, the artifacts seem in my previous post were caused by the way the game handles the textures so it can be considered a normal behavior.

@Rumi-Larry
Copy link

Upon further inspection I can confirm MSAA is working as intended, the artifacts seem in my previous post were caused by the way the game handles the textures so it can be considered a normal behavior.

Does enabling vertex rounding fix it?

@nolrinale
Copy link
Contributor

Vertex Rounding was already enabled when the screenshots were taken.

@dvessel
Copy link
Contributor

dvessel commented Jun 23, 2022

Not sure this is accurate but this is interesting. Running 4x native and it’s consuming about 2 watts. Vulkan used to average about 14w.

pCzwOZ1.-.Imgur.mp4

And the power usage does go up as expected when doing other things. If this is reporting correctly, this is incredible.

@nolrinale
Copy link
Contributor

Yes thats how its meant to be, it is a sight to behold!

@dvessel
Copy link
Contributor

dvessel commented Jun 23, 2022

Vulkan used to average about 14w.

Correcting this bit. I just check with Vulkan on latest master and it’s only using 1w more on average for the same intro. I understand it’s not a particularly demanding track but Dolphin Vulkan never consumed so little power until now.

The demanding tracks definitely pushes the CPU harder and that’s where Metal shines here.

Copy link
Member

@OatmealDome OatmealDome left a comment

Choose a reason for hiding this comment

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

Code seems OK. Tested a variety of games and they all work fine. LGTM!

@JMC47
Copy link
Contributor

JMC47 commented Jul 23, 2022

Looks like this has been reviewed by a macOS maintainer and a GPU backend maintainer. Only one thing left to do.

@JMC47 JMC47 merged commit 89c4fde into dolphin-emu:master Jul 23, 2022
@TellowKrinkle TellowKrinkle deleted the Metal branch July 23, 2022 09:05
@TellowKrinkle TellowKrinkle restored the Metal branch July 23, 2022 20:58
@TellowKrinkle TellowKrinkle deleted the Metal branch August 19, 2022 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet