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 NVIDIA Management Library #15

Closed
wants to merge 7 commits into from
Closed

Add support for NVIDIA Management Library #15

wants to merge 7 commits into from

Conversation

Saancreed
Copy link
Collaborator

Try to load nvml.dll and if successful, use it to implement two device queries (utilization rates and temperature) in a manner that's roughly equivalent to observed behavior on Windows (but I admit that error handling was mostly a guesswork 🙈). This will require something like my wine-nvml to be available within the Wine prefix, otherwise newly implemented functions will return a NO_IMPLEMENTATION error to calling applications.

Some things to consider:

  • Are the two new functions implemented accurately enough? They will need some real world testing on more games than just a few Ubisoft titles I have thrown at it so far.
  • How / when should we instanciate and initialize NVML? Maybe it would be cleaner to make such an instance a part of NvapiAdapterRegistry? Using a shared extern instance doesn't seem elegant to me, and I'm not even sure if it's a technically correct approach.
  • Should we care if applications stubbornly call a function that is bound to always return an error because NVML wasn't found? Maybe we should avoid registering our functions in nvapi_interface.cpp if we can't load NVML at that point?
  • Perhaps we could also provide a more accurate implemetation of NvAPI_GPU_GetArchInfo in case NVML was successfully loaded? Would it require me to add more functions to wine-nvml?

Some things I've noticed:

  • On Windows, Nvidia provides only a 64–bit version of nvml.dll. However, on Linux, there is also a 32–bit version of libnvidia-ml.so. This means I can actually provide something in Wine that's not available on Windows 🙃
  • The documentation states that NvAPI keeps a reference counter on how many times it has been initialized, which leads me to believe that a call chain of Initialize, Initialize, Unload, SYS_GetDriverAndBranchVersion should succeed, but right now the Unload call with delete NvapiAdapterRegistry no matter how many times Initialize was called before that. I've tried not to introduce too many changes there that could be deemed out of scope of this PR, but it's still something that should probably be taken care of.

@SveSop
Copy link
Contributor

SveSop commented Jun 3, 2021

If you need some more tests for nvapi, i would recommend GPU Caps Viewer : https://www.geeks3d.com/20210518/gpu-caps-viewer-1-51-0-released/
This also have GPUShark for even more info grabbing.

Atleast easier to do testing stuff than firing up games every time you do tweaks :)

@jp7677 jp7677 self-assigned this Jun 4, 2021
@SveSop
Copy link
Contributor

SveSop commented Jun 4, 2021

It is indeed interesting with wine-nvml - but i think its a slight pitfall needing to load nvml.dll.so, as this seems a wee bit troublesome for proton at times. I have not really had much success using a dll-override when using proton and a renamed dll.so.

Should we care if applications stubbornly call a function that is bound to always return an error because NVML wasn't found? Maybe we should avoid registering our functions in nvapi_interface.cpp if we can't load NVML at that point?

I can see slowdowns being caused by repeated calls in games/apps that uses this. My limited testing from the Unigine benchmarks have not really shown issues when you get a "unimplemented" function, but what would happen if you get repeatedly calls to a not-found library i cant tell :) Depending on implementation of the app's nvapi call, it CAN be significant, and that is something dxvk-nvapi really cannot control.
I did some testing quite a while ago where i added a sleep timer on a nvapi "reply", and that causes severe stutters for that test. If games/apps that use eg. temperature reading will wait until processing further when doing the call, whatever milliseconds used on the call will mean lower frames.

Maybe some sort of "early check", so that if the lib is not found, all "nvml related functions" is being dropped for that session? I don't know.. needs more testing i guess.

@Saancreed
Copy link
Collaborator Author

I have not really had much success using a dll-override when using proton and a renamed dll.so.

I haven't really tested it with Proton, but have you tried without any dll override in place? Because Wine can see the .dll.so as a builtin module, setting override to native only could actually prevent it from being loaded.

trace:loaddll:build_module Loaded L"C:\\windows\\system32\\nvml.dll" at 00007FA6EC0B0000: builtin

Fwiw, you could also try dropping all the files into Proton's Wine libdirs, like so: Proton - Experimental/files/lib64/wine/{nvml.dll.so,fakedlls/nvml.dll}. Maybe this approach will work better, though I can't be sure.

I can see slowdowns being caused by repeated calls in games/apps that uses this. My limited testing from the Unigine benchmarks have not really shown issues when you get a "unimplemented" function, but what would happen if you get repeatedly calls to a not-found library i cant tell :) Depending on implementation of the app's nvapi call, it CAN be significant, and that is something dxvk-nvapi really cannot control.

My hope is that it won't be any slower than the actual query performed by NvAPI on Windows, but doing just two nullptr checks has a good chance of being fast enough. And if any user wishes to disable NVML integration (even with wine-nvml installed) because somehow NVML call turns out to be too expensive to execute, they could just do something like export WINEDLLOVERRIDES=nvml= to prevent the library from being loaded.

Maybe some sort of "early check", so that if the lib is not found, all "nvml related functions" is being dropped for that session? I don't know.. needs more testing i guess.

Yeah, I've considered that, but decided against it unless we find a real case where it would be useful to do. No point doing such aggressive premature optimization and risk an overengineered solution if it turns out to be unnecessary 😛

@jp7677
Copy link
Owner

jp7677 commented Jun 4, 2021

Hi @Saancreed, thanks a lot for this PR. Would be cool indeed to have those two GPU related methods implemented. Some initial thoughts:

  • Do you have plans to get wine-nvml into wine staging or even beter upstream wine or Proton. I’m a bit hesitant to add the dependency for just very few users which I guess happens when wine-nvml is never added to wine-staging or friends.
  • Regarding architecture, the consuming side looks cool, getting an nvml device from the nvapi adapter. I share your thoughts that moving the module handle to the registry might be a better idea. We might pass nvml as a contructor argument to the adapter. Actually I should be doing that for the vulkan module too, but that would be a different story.
  • Please move the external headers to /inc and use h as file extension for header files
  • I think we should keep nvml as a n opt in, not opt out. Thus we should log we we found nvml, but log nothing when it hasn’t been found.
  • You could consider creating a separate PR for the clangd cache to keep this PR focused.
  • Thanks for the pointer about keeping a counter for how often initialization has been called. Feel free to open an issue for picking this up later.

Let me know what you think!

PS: Performance is surely not an issue here. Consumers might be retarded, but I don’t think those methods ever end up in a performance critical path :)

@Saancreed
Copy link
Collaborator Author

Do you have plans to get wine-nvml into wine staging or even beter upstream wine or Proton. I’m a bit hesitant to add the dependency for just very few users which I guess happens when wine-nvml is never added to wine-staging or friends.

I understand your concerns; keeping support for an additional external library, even if completely optional, still requires extra maintenance effort. To be honest, back when I started this project of mine it was my intention to try upstreaming it to Wine when majority of functions is implemented, but sometime after that I've realized that implementing NVML's interface (which consists of about 240 functions) is quite a big project and supporting even half of them will require a lot of work (which is probably just as hard to automate correctly).

So now I'm left with a not-so-small library that is slowly being implemented in my free time and, while already able to offer a few very useful functions, might not be upsteamable in its current state (especially since there is ongoing effort to convert libraries to PE modules and wine-nvml which is just a wrapper for Linux–native library doesn't really fit into that image). I still hope that at some point it will be accepted into Wine proper, but I expect it to happen later than sooner, and that's still assuming that upsteam is fine with directly using NVML API header in Wine source and PE conversion turns out to be simple or not necessary at all.

But maybe Valve will be interested in including it in Proton until then 🙂

Regarding architecture, the consuming side looks cool, getting an nvml device from the nvapi adapter. I share your thoughts that moving the module handle to the registry might be a better idea. We might pass nvml as a contructor argument to the adapter.

Did you mean "to the adapter registry"? If so then yeah, I suppose that could work. In that case, how should we store our instance? I imagine a private std::shared_ptr<Nvml> field plus a std::shared_ptr<Nvml> GetNvml() method would do, or we could let the registry take full ownership with a unique_ptr (which would probably require us to make the registry impossible to copy just like Nvml itself and would complicate accessing it in consuming functions, unless we expose it to consumers as a raw pointer maybe).

Please move the external headers to /inc and use h as file extension for header files

Sure, will do.

I think we should keep nvml as a n opt in, not opt out. Thus we should log we we found nvml, but log nothing when it hasn’t been found.

The safest way would be to log both cases I think, but yeah, I can do that. I imagine that in case it could not be loaded, the NVML not loaded: No implementation message coming from new functions is a good enough hint.

You could consider creating a separate PR for the clangd cache to keep this PR focused.

I'll just drop that commit and handle it on my own.

Thanks for the pointer about keeping a counter for how often initialization has been called. Feel free to open an issue for picking this up later.

Will do, once I confirm that's indeed supposed to work on Windows.

@SveSop
Copy link
Contributor

SveSop commented Jun 4, 2021

Not that i am an authority on what gets into wine or not (or staging), but getting a project implemented upstream is.. probably not a "next week" kind of thing, since they have kinda shown before that its rather strict when it comes to code compliance in that regard.

I tried to copy the dll.so's into lib64/wine , and the .dll's into lib64/wine/fakedlls , but it did not seem to work. Does work with other winelib's tho, and it works with "regular" wine.

EDIT: Does wine-nvml compile just fine on a build-bot without any nvidia libraries installed?

@Saancreed
Copy link
Collaborator Author

Saancreed commented Jun 4, 2021

I tried to copy the dll.so's into lib64/wine , and the .dll's into lib64/wine/fakedlls , but it did not seem to work.

😞

EDIT: Actually, I just tried that and it worked for me:

284:info:vkd3d_get_vk_version: vkd3d-proton - applicationVersion: 2.3.1.
284:info:vkd3d_instance_init: vkd3d-proton - build: ec5b4ccecf799e7.
skipping config: /home/saancreed/.config/MangoHud/wine-MonsterHunterWorld.conf [ not found ]
skipping config: /home/saancreed/.local/share/Steam/steamapps/common/Proton - Experimental/files/bin/MangoHud.conf [ not found ]
skipping config: /home/saancreed/.config/MangoHud/wine64-preloader.conf [ not found ]
parsing config: /home/saancreed/.config/MangoHud/MangoHud.conf [ ok ]
284:fixme:d3d12_find_physical_device: Could not find Vulkan physical device for DXGI adapter.
284:warn:d3d12_find_physical_device: Using first available physical device...
MANGOHUD: Uploading is disabled (permit_upload = 0)
WINE VERSION = wine-6.3

284:warn:vkd3d_memory_info_find_global_mask: Blocking memory type 10 for use (PCI-pinned memory).
284:info:vkd3d_bindless_state_get_bindless_flags: Device does not support VK_VALVE_mutable_descriptor_type.
284:fixme:d3d12_device_caps_init_feature_options1: TotalLaneCount = 1536, may be inaccurate.
284:info:d3d12_device_determine_ray_tracing_tier: DXR support enabled.
19226.185:010c:011c:trace:loaddll:build_module Loaded L"Z:\\home\\saancreed\\.local\\share\\Steam\\steamapps\\common\\Monster Hunter World\\amd_ags_x64.dll" at 0000000249680000: builtin
19226.185:010c:011c:trace:loaddll:free_modref Unloaded module L"Z:\\home\\saancreed\\.local\\share\\Steam\\steamapps\\common\\Monster Hunter World\\amd_ags_x64.dll" : builtin
19226.185:010c:011c:trace:loaddll:build_module Loaded L"Z:\\home\\saancreed\\.local\\share\\Steam\\steamapps\\common\\Monster Hunter World\\amd_ags_x64.dll" at 0000000010BC0000: native
19226.189:010c:011c:trace:loaddll:build_module Loaded L"C:\\windows\\system32\\nvapi64.dll" at 000000021B3F0000: native
19226.190:010c:011c:trace:seh:NtQueryInformationThread (0x200,0,0x112c760,30,(nil))
NvAPI_QueryInterface 0xad298d3f: Unknown function ID
DXVK-NVAPI v0.3-9-g2b5896c+ (MonsterHunterWorld.exe)
19226.190:010c:011c:trace:loaddll:build_module Loaded L"C:\\windows\\system32\\nvml.dll" at 00007F6B5E490000: builtin

And I think I know why: I use Proton without Soldier runtime, which allows Wine to find my system libraries (and nvml.dll will fail to load if it can't dlopen into libnvidia-ml.so).

Does wine-nvml compile just fine on a build-bot without any nvidia libraries installed?

It should, it only needs gcc, meson, ninja and wine.

@jp7677
Copy link
Owner

jp7677 commented Jun 5, 2021

So now I'm left with a not-so-small library that is slowly being implemented in my free time and, while already able to offer a few very useful functions, might not be upsteamable in its current state (especially since there is ongoing effort to convert libraries to PE modules and wine-nvml which is just a wrapper for Linux–native library doesn't really fit into that image). I still hope that at some point it will be accepted into Wine proper, but I expect it to happen later than sooner, and that's still assuming that upsteam is fine with directly using NVML API header in Wine source and PE conversion turns out to be simple or not necessary at all.

But maybe Valve will be interested in including it in Proton until then slightly_smiling_face

The wine staging implementations of other nvidia related library are far from complete afaik, I think what you already have might fit. Though I just speak for myself since I'm not related to wine or wine-staging. Also dunno about the effort to convert to PE.

Did you mean "to the adapter registry"? If so then yeah, I suppose that could work. In that case, how should we store our instance? I imagine a private std::shared_ptr<Nvml> field plus a std::shared_ptr<Nvml> GetNvml() method would do, or we could let the registry take full ownership with a unique_ptr (which would probably require us to make the registry impossible to copy just like Nvml itself and would complicate accessing it in consuming functions, unless we expose it to consumers as a raw pointer maybe).

Yeah, that's what I meant. Using a unique_ptr sounds about right. I played a bit with it today, seems to do the job. That said, I'm also still learning a lot :). I guess we just have to find out if this is really is the best fit.

I'll just drop that commit and handle it on my own.

I'm happy to add that ignore with a separate PR if it improves your work flow.

Will do, once I confirm that's indeed supposed to work on Windows.

Cool, thanks!

@jp7677
Copy link
Owner

jp7677 commented Jun 5, 2021

PS: I'm sorry for causing some conflicts by adding some commits myself on master.

@jp7677
Copy link
Owner

jp7677 commented Jun 6, 2021

PPS: I've played a bit with unique_ptrin c14c2e2 Not sure yet if this really improves the code, but doesn't look that bad either.

@Saancreed
Copy link
Collaborator Author

The wine staging implementations of other nvidia related library are far from complete afaik, I think what you already have might fit. Though I just speak for myself since I'm not related to wine or wine-staging. Also dunno about the effort to convert to PE.

PE conversion and usage of Nvidia's header (in a bit hackish way because of abuse of known macros) are two things I'm most worried about, other stuff should be easy enough to resolve.

Yeah, that's what I meant. Using a unique_ptr sounds about right. I played a bit with it today, seems to do the job. That said, I'm also still learning a lot :). I guess we just have to find out if this is really is the best fit.

Well, my C++ skills were dulled by years of writing mostly in C# with only a bit of good old C 😅

I'm happy to add that ignore with a separate PR if it improves your work flow.

Up to you, I've already handled it on my side.

PS: I'm sorry for causing some conflicts by adding some commits myself on master.

No worries, I'll handle it while applying your suggestions. Until then, I'll mark this PR as WIP.

PPS: I've played a bit with unique_ptr in c14c2e2 Not sure yet if this really improves the code, but doesn't look that bad either.

unique_ptr itself looks good to me, but I don't really see a reason why an adapter should hold a reference to Vulkan instance, passing it as raw pointer/reference during initialization like before is probably fine too. But whatever version you choose, I'll adjust my own code to match.

@Saancreed Saancreed marked this pull request as draft June 6, 2021 15:06
@jp7677
Copy link
Owner

jp7677 commented Jun 6, 2021

Yeah, that's what I meant. Using a unique_ptr sounds about right. I played a bit with it today, seems to do the job. That said, I'm also still learning a lot :). I guess we just have to find out if this is really is the best fit.

Well, my C++ skills were dulled by years of writing mostly in C# with only a bit of good old C sweat_smile

This sounds very familiar to me, my background the past years (actually lots of years) has also been mostly C# and the surrounding ecosystem ;)

I'm happy to add that ignore with a separate PR if it improves your work flow.

Up to you, I've already handled it on my side.

Alright, thanks. I'll keep the current ignores until somebody else suggests to modify them.

PPS: I've played a bit with unique_ptr in c14c2e2 Not sure yet if this really improves the code, but doesn't look that bad either.

unique_ptr itself looks good to me, but I don't really see a reason why an adapter should hold a reference to Vulkan instance, passing it as raw pointer/reference during initialization like before is probably fine too. But whatever version you choose, I'll adjust my own code to match.

Thanks for the feedback. Yeah, I'm also a bit torn about keeping a reference in the adapter. I did this with nvml in the back of my head. The vulkan reference is (for now) only needed during initialization, but for nvml we would need a reference also later when asking the nvml device from the adapter. I would love to keep the handling of calls into vulkan and nvml the same way, thus having the unique_ptr for both in the registry (for loading and freeing the external lib) and have references in the adapter(s) for using them. May be we will get use cases where the vulkan reference is also needed later. Please let me know what you think! Reading the all-knowing internet, it also seems that having references instead of pointers as members is a bit controversial. So not sure yet what the best way is.

@Saancreed
Copy link
Collaborator Author

Thanks for the feedback. Yeah, I'm also a bit torn about keeping a reference in the adapter. I did this with nvml in the back of my head. The vulkan reference is (for now) only needed during initialization, but for nvml we would need a reference also later when asking the nvml device from the adapter. I would love to keep the handling of calls into vulkan and nvml the same way, thus having the unique_ptr for both in the registry (for loading and freeing the external lib) and have references in the adapter(s) for using them. May be we will get use cases where the vulkan reference is also needed later. Please let me know what you think! Reading the all-knowing internet, it also seems that having references instead of pointers as members is a bit controversial. So not sure yet what the best way is.

Makes sense, but I still believe we could get away with Vulkan* and Nvml* passed at initialization time (where, if Nvml* is not nullptr, we call stuff like nvmlGetDeviceHandleByPciBusId). We can store the only pointer in unique_ptrs in the registry and temporarily expose them to NvAPI_* functions with something like Nvml* NvapiAdapterRegistry::GetNvml(). It wouldn't be good encapsulation, but it also wouldn't be the first time where an API exposes a pointer to some internal structure that must not be freed by the caller. Afterwards, when a function like NvAPI_GPU_GetThermalSettings is called, we access that with Nvml* nvml = nvapiAdapterRegistry->GetNvml(); if (nvml == nullptr) return NoImplementation(…);.

(By the way, shouldn't we call nvapiAdapterRegistry.reset() in NvAPI_Initialize if nvapiAdapterRegistry->Initialize() fails?)

Anyway, I've rebased my PR on top of your current master and applied some of the changes you'd requested. I'll rework initialization once we figure out how to deal with it correctly.

@SveSop
Copy link
Contributor

SveSop commented Jun 10, 2021

I was able to implement NVAPI_GetTachReading, so i find it kinda cool tbh.

@Saancreed I think stuff getting into staging needs to be submitted to the main wine tree, and if it gets rejected but deemed a good enough idea, it might be put in staging for testing and further development. Atleast how i think stuff works when talking about getting stuff upstreamed. I dunno if you have considered that?

@jp7677
Copy link
Owner

jp7677 commented Jun 11, 2021

(By the way, shouldn't we call nvapiAdapterRegistry.reset() in NvAPI_Initialize if nvapiAdapterRegistry->Initialize() fails?)

Thanks, yes. Done on master.

@Saancreed
Copy link
Collaborator Author

I think stuff getting into staging needs to be submitted to the main wine tree, and if it gets rejected but deemed a good enough idea, it might be put in staging for testing and further development. Atleast how i think stuff works when talking about getting stuff upstreamed. I dunno if you have considered that?

@SveSop That's the long term plan, yes, but first I'd have to replace the build system with Wine's and make some changes w.r.t. code style at the very least.

@SveSop
Copy link
Contributor

SveSop commented Jun 11, 2021

They (WineHQ dev's) tend to be very picky, and you would also need to write some tests for this i guess. Not sure how detailed that needs to be.
What happens if nvapi and nvml is present when you have a AMD gpu (or Intel). Would this cause slowdowns or error spam? Would need to be vendor neutral if being built and installed with wine or wine-staging i guess.

@Saancreed
Copy link
Collaborator Author

@SveSop

They (WineHQ dev's) tend to be very picky, and you would also need to write some tests for this i guess. Not sure how detailed that needs to be.

Well, fwiw there are some Nvidia library wrappers in wine-staging that don't seem to have any tests attached. That said, I wouldn't mind writing a few tests for wine-nvml but I'm not sure if there's any point if they would work only when Nvidia's proprietary driver is available.

What happens if nvapi and nvml is present when you have a AMD gpu (or Intel). Would this cause slowdowns or error spam? Would need to be vendor neutral if being built and installed with wine or wine-staging i guess.

Can't speak for NvAPI, but in case of NVML:

  • It doesn't care if you have a GPU from another vendor, only that you have one from Nvidia (you can have both).
  • If you have no Nvidia GPUs, you also probably don't have libnvidia-ml.so, in which case my nvml.dll.so will refuse to load and applications must handle it correctly on their own (same as if they tried it on Nvidia-less Windows).
  • Even if you happen to have libnvidia-ml.so but Nvidia's driver is not loaded, nvmlInit_v2 will return NVML_ERROR_DRIVER_NOT_LOADED to the calling application.
  • If an application instead calls nvmlInitWithFlags(NVML_INIT_FLAG_NO_GPUS), I'd expect it to handle the possibility of NVML reporting no GPUs. Not sure if this allows NVML to be loaded without Nvidia's driver though.
  • There is some logging, but most of it is only printed on TRACE debug level.

Anyway, I think that this PR is not the best place for such a discussion. If you have any further questions, feel free to open an issue in my wine-nvml repository.

@SveSop
Copy link
Contributor

SveSop commented Jun 11, 2021

Will keep nvml discussions to the correct repo.
I was thinking about this a bit wrong, cos in the case of wine-staging's nvapi implementation, it will not load nvml anyway, and when proton is not used with PROTON_ENABLE_NVAPI it will not be used either. Sorry for the noise :)

@Saancreed
Copy link
Collaborator Author

Rebased and updated, should be good to go now. I'm not convinced that duplicating NVML methods in NvapiAdapterRegistry is preferable over something like const Nvml* NvapiAdapterRegistry::GetNvml() const but I'll do as you prefer.

Although now that I think about it, maybe I should reduce the log spam somehow, because just a few minutes of AC Valhalla can produce thousands of log lines like this:

NvAPI_GPU_GetThermalSettings: OK
NvAPI_GPU_GetThermalSettings: No NVML device: Handle invalidated
NvAPI_GPU_GetDynamicPstatesInfoEx: OK
NvAPI_GPU_GetDynamicPstatesInfoEx: No NVML device: Handle invalidated

(The second GPU is AMD integrated).

@Saancreed Saancreed marked this pull request as ready for review June 12, 2021 17:45
@jp7677
Copy link
Owner

jp7677 commented Jun 14, 2021

Closing this PR in favor of #20 which is based on the work done here.

@jp7677 jp7677 closed this Jun 14, 2021
@Saancreed Saancreed deleted the nvml branch June 25, 2022 20:30
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.

None yet

3 participants