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

nvapi: Reflex support through LatencyFleX #66

Merged
merged 3 commits into from
Feb 9, 2022
Merged

Conversation

ishitatsuyuki
Copy link
Contributor

@ishitatsuyuki ishitatsuyuki commented Jan 15, 2022

This has been tested for a while on Radeon 5700 XT, but I haven't received any user reports about NV.

Edit: apparently multiple users have been using Nvidia to run this fine.

Some implementation questions:

  • Does lfx.h/cpp belongs to the sysinfo folder?
  • Do we need a mock for Lfx? If so, how should we create it? Put it inside the factory?

@jp7677 jp7677 linked an issue Jan 15, 2022 that may be closed by this pull request
@jp7677 jp7677 self-assigned this Jan 15, 2022
@jp7677
Copy link
Owner

jp7677 commented Jan 15, 2022

Thanks a lot for the PR and the questions regarding how to wire this all together. This is what comes to my mind after briefly looking at this PR:

  • Move lfx.h and lfx.cpp into a new folder src/d3d/. LatencyFlex is not related to sysinfo things, but to D3D, I think it deserves its own place.
  • Creation of lfx (std::make_unique<Lfx>()) should indeed go into the ResourceFactory, similar to nvml and vulkan. That way we keep a single place for creating all external dependencies. This makes also the wiring easier for the tests. The ResourceFactory is then no longer just about sysinfo dependencies, so we should move it one level up to src.
  • Not sure yet if the following is just overhead: I think we might need one new class to wire the nvapi methods together with the lfx wrapper, similar what the adapterRegistry is for sysinfo and where the (currently limited) actual work might happen. May be we can call this D3DInstance (pretty bad name, but can't find something better now).

About tests, yeah, I prefer to have tests for the different return codes (with and without lfx) and validations that we actually call lfx, similar like the tests for nvml related methods. The tests for NvAPI_D3D12_GetGraphicsCapabilities returns OK also have a similar setup.

Let me know your thoughts.

@ishitatsuyuki
Copy link
Contributor Author

I think all of these suggestions look good, and that clears a lot of my confusions about code organization. Thanks! I'll try to implement this in the next few days.

@ishitatsuyuki ishitatsuyuki force-pushed the lfx branch 5 times, most recently from dd26cbd to 46dea35 Compare January 22, 2022 14:35
@ishitatsuyuki ishitatsuyuki marked this pull request as ready for review January 22, 2022 14:45
@ishitatsuyuki
Copy link
Contributor Author

Thanks for the patience, I have addressed the review comments and added some tests.

@ishitatsuyuki ishitatsuyuki changed the title draft: nvapi: Reflex support through LatencyFleX nvapi: Reflex support through LatencyFleX Jan 22, 2022
@ishitatsuyuki
Copy link
Contributor Author

CLion auto refactor went rogue in a few places, removed that and another unrelated change.

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 improving this PR. I've added some review comments, mostly just smaller things. Overall this looks already really good.

src/d3d/lfx.cpp Outdated Show resolved Hide resolved
src/d3d/nvapi_d3d_instance.cpp Outdated Show resolved Hide resolved
src/d3d/nvapi_d3d_instance.h Outdated Show resolved Hide resolved
src/nvapi.cpp Outdated Show resolved Hide resolved
src/nvapi_d3d.cpp Outdated Show resolved Hide resolved
tests/nvapi_d3d.cpp Outdated Show resolved Hide resolved
tests/nvapi_d3d.cpp Outdated Show resolved Hide resolved
tests/nvapi_d3d.cpp Outdated Show resolved Hide resolved
src/d3d/nvapi_d3d_instance.cpp Outdated Show resolved Hide resolved
src/d3d/nvapi_d3d_instance.cpp Outdated Show resolved Hide resolved
@ishitatsuyuki ishitatsuyuki force-pushed the lfx branch 5 times, most recently from 8882376 to c985e96 Compare January 24, 2022 12:13
@ishitatsuyuki
Copy link
Contributor Author

Thanks for another iteration of review comments, the code now feels a bit more cleaner. I have pushed v2 to address the review comments, plus a rebase to also apply the refactor into newly added tests.

I'll push v3 when I have addressed #66 (comment) (and any extra review comments if you have).

src/d3d/nvapi_d3d_instance.cpp Outdated Show resolved Hide resolved
src/d3d/nvapi_d3d_instance.cpp Outdated Show resolved Hide resolved
src/d3d/lfx.h Outdated Show resolved Hide resolved
tests/nvapi_tests.cpp Show resolved Hide resolved
tests/nvapi_d3d.cpp Outdated Show resolved Hide resolved
tests/nvapi_d3d.cpp Outdated Show resolved Hide resolved
@jp7677
Copy link
Owner

jp7677 commented Jan 24, 2022

Thanks for another iteration of review comments, the code now feels a bit more cleaner. I have pushed v2 to address the review comments, plus a rebase to also apply the refactor into newly added tests.

I'll push v3 when I have addressed #66 (comment) (and any extra review comments if you have).

Hi @ishitatsuyuki thanks again for your changes. Please see my answers to your questions and some other smaller comments. I hope your are not yet tired about all the nits and tests, we are really close ;)

@ishitatsuyuki
Copy link
Contributor Author

Just a status update: I'm still working through refactoring the binding DLLs but I'm a bit short on time due to exams. Can't promise when I can get back to this, but I'll try to not delay this too much.

Copy link
Contributor Author

@ishitatsuyuki ishitatsuyuki left a comment

Choose a reason for hiding this comment

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

v3 changes are now pushed. I did a rebase, and went a bit lazy by squashing v2 into v1. Hopefully that's fine.
A LFX update with new DLL naming will be published soon after I sort out some documentation changes.

tests/nvapi_tests.cpp Show resolved Hide resolved
src/d3d/lfx.cpp Outdated Show resolved Hide resolved
@ishitatsuyuki
Copy link
Contributor Author

Pushed v4, reorganizing the fallback logic. I also added back the cast of GetProcAddress result to void* since the compiler gives a false positive warning otherwise.

@jp7677 jp7677 merged commit 4b709aa into jp7677:master Feb 9, 2022
@jp7677
Copy link
Owner

jp7677 commented Feb 9, 2022

All good now and merged, thanks a lot for the contribution and for your patience!

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.

[question] Reflex support possible?
2 participants