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

Add support for Reflex #147

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Conversation

esullivan-nvidia
Copy link
Contributor

The intent of this PR is to enable Reflex for all D3D11, and D3D12 titles using dxvk-nvapi. It does this through a new device interface called ID3DLowLatencyDevice, which will be exposed from vkd3d-proton, and dxvk.

To provide compatibility with LatencyFleX this change will only use the ID3DLowLatencyDevice interface when LatencyFleX is not detected.

@esullivan-nvidia esullivan-nvidia force-pushed the nv_low_latency2 branch 2 times, most recently from 0bc516f to a4a5e80 Compare October 14, 2023 07:33
Copy link
Collaborator

@Saancreed Saancreed left a comment

Choose a reason for hiding this comment

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

I have a few minor suggestions, see the comments. For now, tests can be fixed using a patch like this:

diff --git a/tests/nvapi_d3d.cpp b/tests/nvapi_d3d.cpp
index 2b32d69..a3d59e4 100644
--- a/tests/nvapi_d3d.cpp
+++ b/tests/nvapi_d3d.cpp
@@ -121,6 +121,9 @@ TEST_CASE("D3D Reflex/LatencyFleX depending methods succeed", "[.d3d]") {
 
     auto e = ConfigureDefaultTestEnvironment(*dxgiFactory, *vulkan, *nvml, *lfx, adapter, output);
 
+    ALLOW_CALL(unknown, QueryInterface(ID3DLowLatencyDevice::guid, _))
+        .RETURN(E_NOINTERFACE);
+
     ALLOW_CALL(*lfx, IsAvailable())
         .RETURN(false);
 
@@ -223,17 +226,17 @@ TEST_CASE("D3D Reflex/LatencyFleX depending methods succeed", "[.d3d]") {
         SECTION("Sleep returns NoImplementation") {
             REQUIRE(NvAPI_D3D_Sleep(&unknown) == NVAPI_NO_IMPLEMENTATION);
         }
-    }
 
-    SECTION("GetLatency returns no-implementation") {
-        NV_LATENCY_RESULT_PARAMS params;
-        params.version = NV_LATENCY_RESULT_PARAMS_VER;
-        REQUIRE(NvAPI_D3D_GetLatency(&unknown, &params) == NVAPI_NO_IMPLEMENTATION);
-    }
+        SECTION("GetLatency returns no-implementation") {
+            NV_LATENCY_RESULT_PARAMS params;
+            params.version = NV_LATENCY_RESULT_PARAMS_VER;
+            REQUIRE(NvAPI_D3D_GetLatency(&unknown, &params) == NVAPI_NO_IMPLEMENTATION);
+        }
 
-    SECTION("SetLatencyMarker returns no-implementation") {
-        NV_LATENCY_MARKER_PARAMS params;
-        params.version = NV_LATENCY_MARKER_PARAMS_VER;
-        REQUIRE(NvAPI_D3D_SetLatencyMarker(&unknown, &params) == NVAPI_NO_IMPLEMENTATION);
+        SECTION("SetLatencyMarker returns no-implementation") {
+            NV_LATENCY_MARKER_PARAMS params;
+            params.version = NV_LATENCY_MARKER_PARAMS_VER;
+            REQUIRE(NvAPI_D3D_SetLatencyMarker(&unknown, &params) == NVAPI_NO_IMPLEMENTATION);
+        }
     }
 }
diff --git a/tests/nvapi_tests_private.h b/tests/nvapi_tests_private.h
index e126e61..ff1856a 100644
--- a/tests/nvapi_tests_private.h
+++ b/tests/nvapi_tests_private.h
@@ -2,6 +2,7 @@
 
 #include "../src/nvapi_private.h"
 #include "../src/nvapi_globals.h"
+#include "../src/d3d/nvapi_d3d_low_latency_device.h"
 #include "../src/d3d11/nvapi_d3d11_device.h"
 #include "../src/d3d12/nvapi_d3d12_device.h"
 #include "../inc/catch_amalgamated.hpp"

Having new tests for covering case where VK_NV_ll2 is supported would be nice but perhaps Jens will be okay with us writing them later 🙂

src/nvapi_d3d.cpp Outdated Show resolved Hide resolved
src/nvapi_d3d.cpp Outdated Show resolved Hide resolved
@@ -29,7 +29,13 @@

enum D3D12_VK_EXTENSION : uint32_t {
D3D12_VK_NVX_BINARY_IMPORT = 0x1,
D3D12_VK_NVX_IMAGE_VIEW_HANDLE = 0x2
D3D12_VK_NVX_IMAGE_VIEW_HANDLE = 0x2,
D3D12_VK_NV_LOW_LATENCY_2 = 0x3
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be both unused and unnecessary, seeing as we can already query extension support via ID3DLowLatencyDevice::SupportsLowLatency. But I guess the validity of this comment depends on whether maintainers of DXVK and vkd3d-proton agree that ID3DLowLatencyDevice::SupportsLowLatency is the way to go.

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 only added this to have a complete list of supported NV extensions. Agreed that it isn't necessary though. I will wait for the vkd3d-proton feedback before removing it though.

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 a lot for this PR, very cool! See my and Saancreed's comments for some smaller nits and suggestions. Overall this fits nicely into the existing code base.
Regarding the test suite, it would of course be cool if you could add representative tests. That said the testing setup is not that accessible with all the mocking and probably requires a deep dive. If your time is limited, I'm fine if you just follow Saancreed's suggestion to make CI happy and we will take care of the tests afterwards.

src/d3d/nvapi_d3d_instance.cpp Outdated Show resolved Hide resolved
src/nvapi_d3d.cpp Outdated Show resolved Hide resolved
src/d3d/nvapi_d3d_low_latency_device.h Outdated Show resolved Hide resolved
src/d3d/nvapi_d3d_low_latency_device.h Outdated Show resolved Hide resolved
src/d3d/nvapi_d3d_low_latency_device.cpp Outdated Show resolved Hide resolved
src/nvapi_d3d.cpp Outdated Show resolved Hide resolved
src/d3d/nvapi_d3d_instance.h Outdated Show resolved Hide resolved
@adamnv
Copy link
Contributor

adamnv commented Oct 17, 2023

Support for the low-latency extensions just got released in the 545.23.06 NVIDIA driver so this is usable/testable now. (Propagating this note to all three project PRs, apologies for any duplication! 😸 )

src/nvapi_d3d.cpp Outdated Show resolved Hide resolved
src/nvapi_d3d.cpp Outdated Show resolved Hide resolved
@esullivan-nvidia esullivan-nvidia force-pushed the nv_low_latency2 branch 2 times, most recently from b879770 to ca6092e Compare October 24, 2023 07:00
@esullivan-nvidia
Copy link
Contributor Author

Thanks for all of the review feedback, I really appreciate it. I have addressed most of the comments now, sorry for the delay. Additional comments should be resolved much faster. I will take a look at the tests tomorrow.

@esullivan-nvidia
Copy link
Contributor Author

I have added tests for the D3DLowLatencyDevice path. Let me know if you have any changes you would like me to incorporate. Any feedback is appreciated.

@jp7677
Copy link
Owner

jp7677 commented Oct 30, 2023

@esullivan-nvidia Thanks a lot for including the feedback and also for taking the time to extend the tests. Perfectly and all good from my side! We can merge this as soon as the DXVK/VKD3D-Proton PRs are merged. Thanks a lot!

(Reminder to self to also adjust the readme when this PR has landed)

loathingKernel added a commit to loathingKernel/Proton that referenced this pull request Nov 22, 2023
Requires at least Nvidia 545.xx drivers.

The patches are from the following pull requests:
[wine](ValveSoftware/wine#200)
[dxvk-nvapi](jp7677/dxvk-nvapi#147)
[vkd3d-proton](HansKristian-Work/vkd3d-proton#1739)
[dxvk](doitsujin/dxvk#3690)

Thanks also goes to @ptr1337 for initially building and testing the patchset.
loathingKernel added a commit to loathingKernel/Proton that referenced this pull request Nov 22, 2023
Requires at least Nvidia 545.xx drivers.

The patches are from the following pull requests:
[wine](ValveSoftware/wine#200)
[dxvk-nvapi](jp7677/dxvk-nvapi#147)
[vkd3d-proton](HansKristian-Work/vkd3d-proton#1739)
[dxvk](doitsujin/dxvk#3690)

Thanks also goes to @ptr1337 for initially building and testing the patchset.
@jp7677
Copy link
Owner

jp7677 commented Jan 19, 2024

@esullivan-nvidia please see #151 (comment) when you have time to get back to this PR. Unfortunately the PR with the MSVC additions came in-between and now master conflicts with your work.

@ishitatsuyuki
Copy link
Contributor

Since I also happen to be testing this patch I have a branch here with the necessary modifications for rebase applied. Please feel free to take it: https://github.com/ishitatsuyuki/dxvk-nvapi/tree/nv_low_latency2

Jan200101 pushed a commit to Jan200101/NorthstarProton that referenced this pull request Jan 28, 2024
Requires at least Nvidia 545.xx drivers.

The patches are from the following pull requests:
[wine](ValveSoftware/wine#200)
[dxvk-nvapi](jp7677/dxvk-nvapi#147)
[vkd3d-proton](HansKristian-Work/vkd3d-proton#1739)
[dxvk](doitsujin/dxvk#3690)

Thanks also goes to @ptr1337 for initially building and testing the patchset.
@iddm
Copy link
Contributor

iddm commented Feb 1, 2024

Excuse me for jumping in like that, but are we still waiting for any changes? Can we fix the conflicts on our own? It doesn't look like the NVIDIA employees have all the time in the world to work on such Linux things as that, from my little understanding of time. Can I help with the rebase, or would it require the changes by the original author?

Thanks, just want to help finish this sooner rather than later.

@ishitatsuyuki
Copy link
Contributor

Everything works if you compile it, there is even a rebase from myself up there. NVIDIA has their own timeline, it's fine, in the meantime just compile it by yourself.

@iddm

This comment was marked as off-topic.

@Avsynthe

This comment was marked as off-topic.

The intent of this commit is to enable Reflex for all D3D11,
and D3D12 titles using dxvk-nvapi. It does this through a new
device interface called ID3DLowLatencyDevice. This interface
will be implemented by ID3D12Device in vkd3d-proton, and
ID3D11Device in dxvk.

To provide compatibility with LatencyFleX this change will
only use the ID3DLowLatencyDevice interface when LatencyFleX
is not detected.
@esullivan-nvidia
Copy link
Contributor Author

@ishitatsuyuki Thank you for doing that rebase, I really appreciate it. I have applied it to this branch now.

@jp7677 jp7677 merged commit 53dfd10 into jp7677:master Mar 1, 2024
2 checks passed
@jp7677
Copy link
Owner

jp7677 commented Mar 1, 2024

Thank a lot @esullivan-nvidia for your hard work on this one!

@esullivan-nvidia
Copy link
Contributor Author

Thank a lot @esullivan-nvidia for your hard work on this one!

Thanks again to you and everyone else who helped review this PR!

MrDuartePT added a commit to MrDuartePT/mrduarte-ebuilds that referenced this pull request Mar 2, 2024
* nvidia reflex merged
jp7677/dxvk-nvapi#147 (comment)
HansKristian-Work/vkd3d-proton#1739 (comment)

Signed-off-by: Gonçalo Negrier Duarte <gonegrier.duarte@gmail.com>
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.

7 participants