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

Remove special treatment for Android in video settings #11095

Merged
merged 1 commit into from Oct 8, 2022

Conversation

K0bin
Copy link
Contributor

@K0bin K0bin commented Sep 25, 2022

Having a submission thread with Vulkan is common practice and recommended basically everywhere. I'd expect most Vulkan apps on Android to have one, so I don't see why this is disabled on Android. The more frequent flushing is only done when there are CPU readbacks (for example EFB peeks) and giving the GPU a head start in those cases is definitely beneficial too.
This relies on #11090 to not slow things down.

@JosJuice
Copy link
Member

You need to update the default value of the setting here too:

@TellowKrinkle
Copy link
Contributor

Did you notice the flush helping with anything? We should already do it after encoding a download in the vertex manager:

@K0bin
Copy link
Contributor Author

K0bin commented Sep 26, 2022

We should already do it after encoding a download in the vertex manager:

No, the vertex manager flushes at specific points before the draw calls that are just before a readback in previous frames.

Did you notice the flush helping with anything?

It didn't really. For some reason, syncing on EFB copies and peeks is still quite a bit slower with Vulkan than GL.

@TellowKrinkle
Copy link
Contributor

No, the vertex manager flushes at specific points before the draw calls that are just before a readback in previous frames.

With deferred readbacks on, it should flush after each readback unless there's a bunch in a row
Otherwise, it immediately waits for the readback to complete, which forces a flush
Either way, a flush is immediate in both cases with the exception of those specific cases in deferred readbacks. If those are an issue, we should be solving this by adjusting that, not throwing extra flushes into the Vk backend.

@K0bin K0bin changed the title Remove Android Vulkan exceptions and flush after copying to staging texture Remove Android Vulkan exceptions Sep 26, 2022
@K0bin
Copy link
Contributor Author

K0bin commented Sep 26, 2022

I dropped the flush commit.

@iwubcode
Copy link
Contributor

iwubcode commented Oct 1, 2022

11090 was merged, can this be rebased?

Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

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

Code wise LGTM. Someone needs to test though, I don't have any Android devices.

@K0bin
Copy link
Contributor Author

K0bin commented Oct 1, 2022

11090 was merged, can this be rebased?

GitHub says it doesn't need a rebase. It also doesn't touch the same parts as that PR.

@brujo5
Copy link

brujo5 commented Oct 1, 2022

It work fine on my poco X3 pro. I have not had any regression. except that Vulkan is no longer that far behind OpenGL.

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.

See #7739 for when this was originally added. It looks like there's still an outdated comment from that PR:

<string name="backend_multithreading">Backend Multithreading</string> <!--Backend Multithreading is only disabled by default on Android -->

Source/Core/Core/Config/GraphicsSettings.cpp Show resolved Hide resolved
@Pokechu22
Copy link
Contributor

Code looks fine to me now, but the commit/PR message should probably use "special cases" instead of "exceptions" to avoid confusion with C++ exceptions or similar. Maybe "Remove Android special-cased default settings"?

@Pokechu22 Pokechu22 changed the title Remove Android Vulkan exceptions Remove special treatment for Android in video settings Oct 2, 2022
@AdmiralCurtiss AdmiralCurtiss merged commit da27a3e into dolphin-emu:master Oct 8, 2022
11 checks passed
@K0bin K0bin deleted the misc-vulkan branch October 8, 2022 20:33
@golivax
Copy link

golivax commented Oct 9, 2022

Just wanted to mention that, for the first time, Skyward Sword runs at full speed on a SD855 using the Vulkan Backend. Awesome job, folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants