Vulkan: AMD devices not using threading

hrydgard / ppsspp

A PSP emulator for Android, Windows, Mac and Linux, written in C++. Want to contribute? Join us on Discord at https://discord.gg/5NJB6dD or just send pull requests / issues. For discussion use the forums at forums.ppsspp.org.

https://www.ppsspp.org

Other

11.21k stars 2.17k forks source link

Vulkan: AMD devices not using threading #10643

Closed unknownbrackets closed 3 years ago

unknownbrackets commented 6 years ago

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]

hrydgard commented 5 years ago

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

unknownbrackets commented 5 years ago

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

https://github.com/hrydgard/ppsspp/blob/f2244f789e6c64fede1cea006804875f1c7aec78/ext/native/thin3d/VulkanRenderManager.cpp#L138-L141

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

-[Unknown]

hrydgard commented 5 years ago

Oh right, I forgot that flag still existed.

hrydgard commented 5 years ago

We really should re-test this...

RinMaru commented 5 years ago

I have AMD anything i can do?

hrydgard commented 5 years ago

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 commented 5 years ago

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

gameblabla commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

LunaMoo commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@LunaMoo Well just look at the first post of this thread, and https://github.com/hrydgard/ppsspp/issues/10097.

hrydgard commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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]