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

Vulkan: AMD devices not using threading #10643

Closed
unknownbrackets opened this issue Feb 25, 2018 · 16 comments · Fixed by #13864
Closed

Vulkan: AMD devices not using threading #10643

unknownbrackets opened this issue Feb 25, 2018 · 16 comments · Fixed by #13864
Labels
Milestone

Comments

@unknownbrackets
Copy link
Collaborator

In #10097 we saw issues with AMD drivers and had to disable by default.

On recent drivers (in that bug), enabling the thread worked, but gained no performance. We've so far left it disabled.

There's still a possibility that it could gain performance (OpenGL gains performance with the thread, after all) in the future. It may be a bug (perhaps we're handling fences in a way that doesn't work great on AMD), or it may be something AMD fixes in a driver update.

Opening this so we can track that.

-[Unknown]

@unknownbrackets unknownbrackets added this to the Future milestone Feb 25, 2018
@hrydgard hrydgard modified the milestones: Future, v1.7.0 Mar 12, 2018
@hrydgard hrydgard modified the milestones: v1.7.0, v1.8.0 Sep 16, 2018
@hrydgard
Copy link
Owner

We no longer have a way to disable threading, do we really need to keep this open?

@unknownbrackets
Copy link
Collaborator Author

This issue is specifically about these lines of code in Vulkan:

// Temporary AMD hack for issue #10097
if (vulkan_->GetPhysicalDeviceProperties(vulkan_->GetCurrentPhysicalDevice()).vendorID == VULKAN_VENDOR_AMD) {
useThread_ = false;
}

It's still using the threaded "method", but not threading the same way all other vendors do.

-[Unknown]

@hrydgard
Copy link
Owner

Oh right, I forgot that flag still existed.

@hrydgard hrydgard modified the milestones: v1.8.0, v1.9.0 Feb 6, 2019
@hrydgard
Copy link
Owner

We really should re-test this...

@RinMaru
Copy link

RinMaru commented Jun 27, 2019

I have AMD anything i can do?

@hrydgard
Copy link
Owner

If you can compile, please remove the "useThread_ = false" line in ppsspp/ext/native/thin3d/VulkanRenderManager.cpp , compile, and see if things work as they should.

@RinMaru
Copy link

RinMaru commented Jun 27, 2019

not sure how to do that no appvoyer? can you provide a binary?

@hrydgard hrydgard modified the milestones: v1.9.0, v1.10.0 Aug 9, 2019
@hrydgard hrydgard modified the milestones: v1.10.0, v1.11.0 Apr 27, 2020
@gameblabla
Copy link
Contributor

Is this windows specific ?
Removed

	// Temporary AMD hack for issue #10097
	if (vulkan_->GetPhysicalDeviceProperties().properties.vendorID == VULKAN_VENDOR_AMD) {
		useThread_ = false;
	}

from https://github.com/hrydgard/ppsspp/blob/master/Common/GPU/Vulkan/VulkanRenderManager.cpp#L210

... and it still worked fine for me.
I tested it on Void linux, Mesa 20.2, Ryzen 7 4750G APU with AMDGPU/RADV Vulkan drivers.
Whenever or not the bug is windows specific, the condition and useThread_ = false still get triggered on Linux despite the fact it works fine.

@LunaMoo
Copy link
Collaborator

LunaMoo commented Dec 4, 2020

Define "works fine".
Vulkan's multithreading was disabled on AMD gpu's because it had no benefit whatsoever and could even be a minor slowdown, not because it caused problems.

@hrydgard
Copy link
Owner

hrydgard commented Dec 4, 2020

No, it was actually disabled because early Windows Vulkan drivers indeed had problems. Since then we've just left it as it is.

@LunaMoo
Copy link
Collaborator

LunaMoo commented Dec 4, 2020

I recall it being disabled after my own report it doesn't benefit AMD gpu back when I had one, it always worked, it just never had any benefit of "working". But not sitting in anyone elses mind to know other reasons.

@gameblabla
Copy link
Contributor

Removing the workaround actually improves the speed by quite a significant amount, at least on Linux with latest Mesa drivers.

nothreads
^ With workaround in place

threads
^ With workaround removed and gone.

What i did was to boot up Battlegrounds 3 at the titlescreen and just uncap the framerate.

Settings
2020-12-04-132351_1920x1080_scrot
2020-12-04-132355_1920x1080_scrot
2020-12-04-132356_1920x1080_scrot

It's a poor benchmark but it doesn't seem to be any slower.

@hrydgard
Copy link
Owner

hrydgard commented Dec 4, 2020

@LunaMoo Well just look at the first post of this thread, and #10097.

@hrydgard hrydgard modified the milestones: v1.11.0, v1.12.0 Dec 4, 2020
@hrydgard
Copy link
Owner

hrydgard commented Dec 4, 2020

Let's experimentally turn it back on after the 1.11 release. In the meantime, most machines with an AMD gpu are gonna be "fast enough" anyway, so...

@LunaMoo
Copy link
Collaborator

LunaMoo commented Dec 4, 2020

@hrydgard ok, but following that issue it was more of a problem with windows update, either way even back then it ran on amd GPU, just without benefits.

@unknownbrackets
Copy link
Collaborator Author

If there are no performance benefits on some Windows devices, but no longer any driver bugs - I'd still say we should enable this everywhere. There's definitely benefit to all GPUs running the same way. I'm sure we could have special annoying double free issues in this alternate codepath, and there's no reason to maintain it if it has no upside.

-[Unknown]

unknownbrackets added a commit to unknownbrackets/ppsspp that referenced this issue Jan 3, 2021
Fixes hrydgard#10643.  Assumes affected drivers only supported 1.0 due to year 1.1
supporting drivers started coming out.
unknownbrackets added a commit to unknownbrackets/ppsspp that referenced this issue Jan 3, 2021
Fixes hrydgard#10643.  Assumes affected drivers only supported 1.0 due to year 1.1
supporting drivers started coming out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants