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 Reflex support #237

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Vulkan Reflex support #237

wants to merge 15 commits into from

Conversation

Saancreed
Copy link
Collaborator

Implemented via both NvAPI_Vulkan_* entrypoints and corresponding Linux-side Vulkan layer.

Closes #166.

@Saancreed Saancreed force-pushed the vk-reflex branch 3 times, most recently from ec73e4f to ded553e Compare December 18, 2024 21:42
@Saancreed
Copy link
Collaborator Author

Okay, I think this is now ready for initial round of review. Still has some hacks but I have no idea how to rid of them. And I'll try to test this version again with various games I happen to have installed, for now I know that at least vk_streamline sample works with this.

Feel free to ask about strange stuff in there; I lived with a variant of this code for months so I might have got used to the weirdness.

@Saancreed Saancreed requested a review from jp7677 December 18, 2024 21:56
@Saancreed Saancreed marked this pull request as ready for review December 18, 2024 21:56
@Saancreed
Copy link
Collaborator Author

Tested with Enshrouded, Portal RTX and Portal Prelude RTX, appears to work fine with them. Still no idea how to convince No Man's Sky and Indiana Jones and the Great Circle to let me activate Reflex.

Remaining titles that would be nice to have tested (that I know of):

  • Path of Exile 2
  • Red Dead Redemption 2
  • Tom Clancy's Rainbow Six Extraction
  • Tom Clancy's Rainbow Six Siege? Does it still have its Vulkan renderer?

@Saancreed
Copy link
Collaborator Author

I figured out the problem NMS and Indy had: Streamline was thinking that NvLowLatencyVk.dll shipped with the game wasn't correctly signed, most likely due to Wine not implementing the signature verification functions (correctly?). With that issue… sidestepped, both NMS and Indy load the library and make Reflex calls, but:

  • NMS only calls SetSleepMode and SetLatencyMarker, it doesn't seem to be performing the actual latency sleeps.
  • Indy works, but spams the log with unsupported NV_VULKAN_LATENCY_MARKER_TYPE (13) which doesn't exist as a Vulkan latency marker type but is likely related to PCLMarker::eControllerInputSample.

@jp7677
Copy link
Owner

jp7677 commented Dec 20, 2024

Thanks for the PR! Cool to have Vulkan Reflex.

I did a first pass on the nvapi side of things for some initial thoughts. The gist is more or less that it would be nice to integrate more with the existing structure:

  • nvapi_vkreflex.cpp should be nvapi_vulkan.cpp to be consistent with other entry points file (due to NvAPI_Vulkan_ )
  • On the D3D side we have NvapiD3dLowLatencyDevice, what about introducing NvapiVulkanLowLatencyDevice for details and Vulkan initialization so that the entry point files are mostly for logging, arguments checking and status codes? The VkReflexContextData is I guess already quite close to this.
  • Other global things do live in nvapi_globals, the context map (or map of VulkanLowLatencyDevices) could also live there.
  • With the mind of tests it would be cool to reuse and extend the existing Vulkan class for abstracting the VK calls. Creating should still go through ResourceFactory::CreateVulkan with a new overload for just the library name.

@jp7677
Copy link
Owner

jp7677 commented Dec 20, 2024

Indy works, but spams the log with unsupported NV_VULKAN_LATENCY_MARKER_TYPE (13) which doesn't exist as a Vulkan latency marker type but is likely related to PCLMarker::eControllerInputSample.

Impressive find. Probably a good idea to log only once per invalid marker type (also on the D3D side).

@Saancreed
Copy link
Collaborator Author

  • nvapi_vkreflex.cpp should be nvapi_vulkan.cpp to be consistent with other entry points file (due to NvAPI_Vulkan_ )

I actually tried that first, only to be told to go away with my nonsense by Meson:

ERROR: Multiple producers for Ninja target "src/nvapi64.dll.p/nvapi_vulkan.cpp.obj". Please rename your targets.

I think nvapi_vulkan.cpp somehow collides with nvapi/vulkan.cpp, but that sounds incredibly stupid.

@jp7677
Copy link
Owner

jp7677 commented Dec 20, 2024

  • nvapi_vkreflex.cpp should be nvapi_vulkan.cpp to be consistent with other entry points file (due to NvAPI_Vulkan_ )

I actually tried that first, only to be told to go away with my nonsense by Meson:

ERROR: Multiple producers for Ninja target "src/nvapi64.dll.p/nvapi_vulkan.cpp.obj". Please rename your targets.

I think nvapi_vulkan.cpp somehow collides with nvapi/vulkan.cpp, but that sounds incredibly stupid.

ah yeah, it is exactly this... I remember hitting this when playing with nvofapi initially. Meson flattens paths by converting the separator to underscore which conflicts in this case. Maybe rename nvapi/vulkan.cpp to nvapi/vk.cpp?

@oscarbg
Copy link

oscarbg commented Dec 21, 2024

through

Sorry to ask here, but does the latest patch, and/or Indiana update2 fix proton black screen with dlss3 frame gen? Or is at least a step in the right direction?

@Saancreed
Copy link
Collaborator Author

It does not. For now you should assume that any game with Vulkan renderer that isn't Portal RTX won't have DLSS Frame Generation function correctly.

Indy is a bit more of a special case because it's trying to use VK-CUDA interop for DLFG, we are investigating this here: SveSop/nvcuda#9

@oscarbg
Copy link

oscarbg commented Dec 23, 2024

@Saancreed thanks for keeping me up to date.. will follow that thread

@Saancreed Saancreed force-pushed the vk-reflex branch 2 times, most recently from 6a4feb9 to bc9ac84 Compare December 26, 2024 18:57
@Saancreed
Copy link
Collaborator Author

Pushed new version, but with changes (when compared to previous revision) as fixup commits, to hopefully make reviewing them easier. Now to figure out what Proton's mingw wants me to do to make it happy…

Copy link
Owner

@jp7677 jp7677 left a comment

Choose a reason for hiding this comment

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

Thanks for the rework of the nvapi side. See comments for a few last thoughts. It would also be nice to have some tests covering the basics, but I'm fine to do this in a separate PR (but we shouldn't forget since everything is in place now for that).

src/nvapi_globals.h Outdated Show resolved Hide resolved
src/nvapi/nvapi_vulkan_low_latency_device.cpp Show resolved Hide resolved
src/nvapi_vulkan.cpp Show resolved Hide resolved
@jp7677
Copy link
Owner

jp7677 commented Dec 29, 2024

Looking at the layer, some initial thoughts:

  • The layer itself looks fine at first look, also good idea to use vkroots.
  • We should also built the layer as part of CI including proper naming of the artifact on a tag
  • We also need to document building and installing in a user home. May be separate readme in the layer directory with link from the main readme?
  • May be we can repeat the version.h generation and log the version on Init()?
  • Not sure yet if we should use different env names for the layer. May be it is easier to reuse the existing names.

@Saancreed Saancreed force-pushed the vk-reflex branch 4 times, most recently from 2ac8f12 to 5687a8b Compare December 30, 2024 14:08
@jp7677
Copy link
Owner

jp7677 commented Dec 30, 2024

LGTM wrt the NVAPI side. Suggested change for avoiding a nullptr check for the resource factory (because that already happens in NvAPI_Vulkan_InitLowLatencyDevice) and for keeping the Initialize() signature identical with e.g. adapter registry:

Diff
diff --git a/src/nvapi/nvapi_vulkan_low_latency_device.cpp b/src/nvapi/nvapi_vulkan_low_latency_device.cpp
index 7472aac..f53e434 100644
--- a/src/nvapi/nvapi_vulkan_low_latency_device.cpp
+++ b/src/nvapi/nvapi_vulkan_low_latency_device.cpp
@@ -5,21 +5,18 @@ namespace dxvk {
     std::unordered_map<VkDevice, NvapiVulkanLowLatencyDevice> NvapiVulkanLowLatencyDevice::m_lowLatencyDeviceMap = {};
     std::mutex NvapiVulkanLowLatencyDevice::m_mutex = {};
 
-    bool NvapiVulkanLowLatencyDevice::TryInitialize(ResourceFactory* resourceFactory) {
+    bool NvapiVulkanLowLatencyDevice::Initialize(ResourceFactory& resourceFactory) {
         std::scoped_lock lock{m_mutex};
 
         if (m_vk && m_vk->IsAvailable())
             return true;
 
-        if (resourceFactory == nullptr)
-            return false;
-
-        m_vk = resourceFactory->CreateVulkan("vulkan-1.dll");
+        m_vk = resourceFactory.CreateVulkan("vulkan-1.dll");
 
         if (m_vk && m_vk->IsAvailable())
             return true;
 
-        m_vk = resourceFactory->CreateVulkan("winevulkan.dll");
+        m_vk = resourceFactory.CreateVulkan("winevulkan.dll");
 
         return m_vk && m_vk->IsAvailable();
     }
diff --git a/src/nvapi/nvapi_vulkan_low_latency_device.h b/src/nvapi/nvapi_vulkan_low_latency_device.h
index fbeb9f8..55b086d 100644
--- a/src/nvapi/nvapi_vulkan_low_latency_device.h
+++ b/src/nvapi/nvapi_vulkan_low_latency_device.h
@@ -6,7 +6,7 @@
 namespace dxvk {
     class NvapiVulkanLowLatencyDevice {
       public:
-        static bool TryInitialize(ResourceFactory* resourceFactory);
+        static bool Initialize(ResourceFactory& resourceFactory);
 
         [[nodiscard]] static std::pair<NvapiVulkanLowLatencyDevice*, VkResult> GetOrCreate(VkDevice device);
         [[nodiscard]] static NvapiVulkanLowLatencyDevice* Get(VkDevice device);
diff --git a/src/nvapi_vulkan.cpp b/src/nvapi_vulkan.cpp
index 74ff113..43449be 100644
--- a/src/nvapi_vulkan.cpp
+++ b/src/nvapi_vulkan.cpp
@@ -16,7 +16,7 @@ extern "C" {
             return ApiNotInitialized(n);
 
         static std::once_flag initialized{};
-        std::call_once(initialized, []() { NvapiVulkanLowLatencyDevice::TryInitialize(resourceFactory.get()); });
+        std::call_once(initialized, []() { NvapiVulkanLowLatencyDevice::Initialize(*resourceFactory); });
 
         auto device = reinterpret_cast<VkDevice>(vkDevice);
         auto semaphore = reinterpret_cast<VkSemaphore*>(signalSemaphoreHandle);

@Saancreed
Copy link
Collaborator Author

Weird, when I initially tried passing as reference clangd complained about binding to temporary value. But your version works so I applied it, thanks.

@jp7677
Copy link
Owner

jp7677 commented Jan 2, 2025

I've played with the current version with RDR2 and seems to work nicely (according to the logs, my reflexes wont feel the difference ;)). Also the layer setup is really simple now by just pointing VK_ADD_IMPLICIT_LAYER_PATH to the layer directory and declaring DXVK_NVAPI_VKREFLEX=1. Very nice!

As (partially) discussed:

  • I'm fine to enable the layer by default so that explicitly setting DXVK_NVAPI_VKREFLEX is not needed. Manually setting up the layer setup is needed anyway, at least for Proton. Up to you with what you are comfortable with.
  • Also no strong preference here: may be easier to keep the environment variable for logging the same with nvapi/nvofapi, thus DXVK_NVAPI_LOG_LEVEL with info and trace. We can introduce e.g. warn if that fits.
  • I think it makes sense to issue a clear log statement in NvAPI_Vulkan_InitLowLatencyDevice i.c.w. VK_ERROR_EXTENSION_NOT_PRESENT to guide users to a missing/wrong layer setup.

@Saancreed
Copy link
Collaborator Author

  • I'm fine to enable the layer by default so that explicitly setting DXVK_NVAPI_VKREFLEX is not needed. Manually setting up the layer setup is needed anyway, at least for Proton. Up to you with what you are comfortable with.

I'd prefer to keep it disabled by default, even if manual setup is required to set it up. I don't trust myself that it won't interfere with other stuff just yet, and once installed it would activate it in other games for no reason.

  • Also no strong preference here: may be easier to keep the environment variable for logging the same with nvapi/nvofapi, thus DXVK_NVAPI_LOG_LEVEL with info and trace. We can introduce e.g. warn if that fits.

I kind of like having it separate.

  • I think it makes sense to issue a clear log statement in NvAPI_Vulkan_InitLowLatencyDevice i.c.w. VK_ERROR_EXTENSION_NOT_PRESENT to guide users to a missing/wrong layer setup.

Right, will do.

Copy link
Owner

@jp7677 jp7677 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the readme additions. See some minor suggestions.

Once the remaining fixup commits are squashed, this PR LGTM!

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Saancreed
Copy link
Collaborator Author

I also added a notice about potential glibc version mismatches because our CI is running on Arch, so less surprises for Ubuntu users is probably a good idea 🐧

Copy link
Owner

@jp7677 jp7677 left a comment

Choose a reason for hiding this comment

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

2 typos

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Owner

@jp7677 jp7677 left a comment

Choose a reason for hiding this comment

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

LGTM!

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

Successfully merging this pull request may close these issues.

vk_streamline: Vulkan reflex support (frame limiter) fails to work
3 participants