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

NVML 515 #89

Merged
merged 13 commits into from
Jun 24, 2022
Merged

NVML 515 #89

merged 13 commits into from
Jun 24, 2022

Conversation

Saancreed
Copy link
Collaborator

An attempt to reimplement a few NVAPI functions using better NVML counterparts from newer API versions. Not tested yet because the only Windows machine I have at hand is a very small VM I set up on my laptop and doing GPU passthrough is annoying so I wanted to gather some feedback and ensure you're willing to accept a PR like this before actually getting my hands dirty 😅

Some notes:

  • I still need to fix/update tests
  • we no longer require all NVML function to be present in the loaded library
  • old implementations are kept as a fallback in case new functions are not available
  • nvmlDeviceGetDynamicPstatesInfo and nvmlDeviceGetThermalSettings seem to be almost direct "ports" of NvAPI_GPU_GetDynamicPstatesInfoEx and NvAPI_GPU_GetThermalSettings respectively, but apparently they are not exactly the same (pending some testing):
    • nvmlDeviceGetDynamicPstatesInfo has flags which are "reserved for future use", while NvAPI_GPU_GetDynamicPstatesInfoEx has one already documented flag
    • thermal controller enums have known values ordered differently and NVML knows about a few more than NVAPI
    • it's undocumented/unknown whether nvmlDeviceGetThermalSettings accepts NVAPI_THERMAL_TARGET_ALL for sensorIndex to retrieve info for all sensors like NvAPI_GPU_GetThermalSettings does
    • returned temperature values are unsigned, making nvmlDeviceGetThermalSettings closer to V1 version of NvAPI_GPU_GetThermalSettings, fortunately we should be able to safely cast values from expected range to signed integers for V2 and just never report negative values this way
  • there are a few more low hanging fruits which can be implemented using new functions:
    • NvAPI_GPU_GetCurrentPCIEDownstreamWidth using nvmlDeviceGetCurrPcieLinkWidth (wasn't this previously stubbed out?)
    • NvAPI_GPU_GetIRQ using nvmlDeviceGetIrqNum
    • NvAPI_GPU_GetGpuCoreCount using nvmlDeviceGetNumGpuCores

In the meantime, I'm working on the next version of wine-nvml that uses Wine's unixlib syscall interface and automatically generates code from NVIDIA's API header, which in theory should make adding support for new functions as they get released much easier in the future.

@jp7677
Copy link
Owner

jp7677 commented Jun 8, 2022

Hi @Saancreed, thanks a lot for already opening this draft PR and really cool that you are working again on the nvml side here and in your repo.
I’ve only looked briefly but indeed, seeing much closer nvml counterparts to nvapi methods removes a lot of guesswork and simplifies things. I’m even in favor of removing the fallback paths to the old implementation. I guess most people using dxvk-nvapi i.c.w. wine-nvml are using latest driver version anyway and if not, nothing substantially breaks.
I’m certainly interested in going forward with this PR? How would you like to proceed?

PS: I haven’t looked, but would be cool if the new headers allows us later to add an implementation to https://github.com/jp7677/dxvk-nvapi/blob/master/src/nvapi_gpu.cpp#L619 since that method is used in UE4, though I have never investigated what UE4 does once this call succeeds.
Edit: Seems that UE4 uses this only to detect overclocking for adding this fact to the logs.

@SveSop
Copy link
Contributor

SveSop commented Jun 16, 2022

PS: I haven’t looked, but would be cool if the new headers allows us later to add an implementation to https://github.com/jp7677/dxvk-nvapi/blob/master/src/nvapi_gpu.cpp#L619 since that method is used in UE4, though I have never investigated what UE4 does once this call succeeds. Edit: Seems that UE4 uses this only to detect overclocking for adding this fact to the logs.

I am not really aware of anything breaking without this function. Nifty to have when benchmarking or whatnot for those windows tools that uses that (Unigine Valley/Heaven comes to mind).

@Saancreed
Copy link
Collaborator Author

Oh dear, having tests crash on me with no debug symbols is not a fun experience. But they should be working now. Some more notes:

  • GetDynamicPstatesInfo and GetBusType seem to be as reasonable as I initially expected so they remain unchanged from the initial push.
  • I wouldn't drop old implementations yet, not only because users might be using older NV driver but also they might be using older wine-nvml, like the one shipped by Lutris. We can afford to let them stay for a while, I think.
  • This needs a quite recent build of wine-nvml. You can build one from the source or get artifacts from some recent Actions run here but the latter require glibc 2.35 or newer. Mind the new installation instructions, it seems that copying stuff directly into Wine tree is now required (but maybe it could be solved by WINEDLLPATH enviroment variable? not sure about this)

The only functional change is adapting GetThermalSettings for two quirks I initially missed:

  1. NVML claims defaultMinTemp is unsigned, which doesn't appear to be the case so I reinterpret it as signed to get a meaningful value.
  2. NvAPI and NVML both (should) return total number of sensors in .count but sensor array is filled based on sensorIndex:
    • for 15, count members are filled (or so I presume because my own laptop has only 1 sensor total)
    • for sensorIndex < count, one member is filled
    • for any other value, the array is untouched

Anyway, I think it's ready for review now so I'm marking it as such.

@Saancreed Saancreed marked this pull request as ready for review June 18, 2022 23:04
@jp7677 jp7677 self-assigned this Jun 19, 2022
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.

I finished the first round, looks really good!
Just in case you haven't already, could you also run clang-format with the rules in the project?

src/nvapi_gpu.cpp Outdated Show resolved Hide resolved
src/nvapi_gpu.cpp Outdated Show resolved Hide resolved
src/nvapi_gpu.cpp Outdated Show resolved Hide resolved
src/sysinfo/nvml.cpp Show resolved Hide resolved
tests/nvapi_d3d.cpp Outdated Show resolved Hide resolved
tests/nvapi_sysinfo.cpp Outdated Show resolved Hide resolved
src/nvapi_gpu.cpp Show resolved Hide resolved
tests/nvapi_sysinfo.cpp Outdated Show resolved Hide resolved
@Saancreed Saancreed requested a review from jp7677 June 24, 2022 21:06
@jp7677 jp7677 merged commit f5efc9a into jp7677:master Jun 24, 2022
@Saancreed Saancreed deleted the nvml-extras branch June 24, 2022 22:54
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.

3 participants