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

Windows: Implement gpus #149

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Carterpersall
Copy link

@Carterpersall Carterpersall commented Mar 21, 2023

Hello, I'm a contributor to https://github.com/lptstr/winfetch (you've talked to the maintainer in #112), and would like to start applying some of the experience I gained there to this project.

  • Uses implementations from both https://github.com/Carterpersall/OxiFetch/blob/main/src/main.rs#L360 and Speed Up info_gpu lptstr/winfetch#155
  • Has multiple alternative implementations in case of the failure of a previous one
    • Implementation 1:
      • Gets the value of the LastSeen value in HKLM\SOFTWARE\Microsoft\DirectX\ and compares it with the lastseen values in its subkeys
        • If it matches with any of the subkeys, then that GPU is currently connected
        • Some systems lack the LastSeen value in the parent key, therefore a backup implementation is needed
    • Implementation 2:
      • Enumerates over the subkeys in HKLM\SYSTEM\CurrentControlSet\Enum\PCI\
      • Enumerates over each subkey and matches the DeviceDesc value (if it has one) with a list of strings
      • A possible issue with this one is in the case someone has a more obscure GPU model alongside a more common one, causing it to only find one of the two and not fall back to the other backup implementation
      • Uses EnumDisplayDevicesW to get every display adapter, formats its name, and adds it to a HashSet to prevent duplicates
    • Implementation 3:
      • Creates a WMI connection and queries Win32_VideoController
        • Iterates over the results and gets the value of Name

@CarterLi
Copy link

CarterLi commented Mar 21, 2023

I don't think matching GPU names is a good idea. There are actually other GPU vendors.

I used DXGI to fetch GPU devices if LastSeen in SOFTWARE\Microsoft\DirectX is not available. It's not as fast as the registry one, but is still much faster than querying WMI

https://github.com/LinusDierheimer/fastfetch/blob/b3da6b0e89c0decb9ea648e1d98a75fa6ac40225/src/detection/gpu/gpu_windows.cpp#L91

Both impls support GPU memory detection.

@Carterpersall
Copy link
Author

Carterpersall commented Mar 23, 2023

I don't think matching GPU names is a good idea. There are actually other GPU vendors.

Yeah, the list in that implementation was just one I threw together, it would need a significantly larger one to be able to cover most cases. I replaced it with another implementation for now

src/windows/mod.rs Outdated Show resolved Hide resolved
@Carterpersall
Copy link
Author

Carterpersall commented Mar 24, 2023

Implementation Speeds:

  • Command used: hyperfine --warmup 20 --prepare "cmd" --min-runs 200 ".\macchina.exe --show desktop-environment"
    • Temporarily added a print command into the desktop environment function in macchina for testing
    • Compiled in release mode
  • Registry
    • 13.8 ms
  • EnumDisplayDevices
    • 13.9 ms
  • DXGI
    • 20 ms
    • Returns video memory as well
  • WMI
    • 69.6 ms ± 52 ms

Speeds on Battery life:

  • Registry
    • 17.9 ms
  • EnumDisplayDevices
    • 17.8 ms
  • DXGI
    • 22 ms
  • WMI
    • 103.8 ms ± 63.7 ms

Speeds when heavily throttled (used power plans to downclock CPU to 1.06 GHz to simulate a slow computer)

  • Registry
    • 58.5 ms
  • EnumDisplayDevices
    • 57.2 ms
  • DXGI
    • 69.9 ms
  • WMI
    • 482.3 ms ± 42.1 ms

@CarterLi
Copy link

Because impl of EnumDisplayDevices has issues with same GPUs, I think it should be removed

EnumDisplayDevices generally won't fail, which makes other impls meaningless.

@CarterLi
Copy link

  • Returns video memory as well

image

Same as the values returned by DXGI

@Carterpersall
Copy link
Author

Carterpersall commented Mar 24, 2023

Because impl of EnumDisplayDevices has issues with same GPUs, I think it should be removed

I think my most recent commit has fixed that issue by getting the value that represents a GPUs location in the registry, and only adds the first entry

EnumDisplayDevices generally won't fail, which makes other impls meaningless.

Generally it won't, but it might be a good idea to at least keep the WMI implementation just in case things go wrong, like if the registry entries EnumDisplayDevices uses doesn't exist. Removing the DXGI implementation might be fine though since it seems to use the same registry entries as the first implementation.

Same as the values returned by DXGI

Good catch, I'll add it to the output.

@CarterLi
Copy link

CarterLi commented Mar 24, 2023

MSDN says that DeviceKey is reserved, so you should better not use it

https://learn.microsoft.com/en-us/windows/win32/api/wingdi/ns-wingdi-display_devicea

@CarterLi
Copy link

DXGI version doesnt rely on LastSeen, which only exists in Win11

@Carterpersall
Copy link
Author

MSDN says that DeviceKey is reserved, so you should better not use it

It is reserved, but that doesn't mean it doesn't output some useful value. Generally reserved means that the value was used internally during development or is reserved for future use. And if the returned value ends up being contrary to the expected one, the implementation will just fail and move on to the next one.

Theoretically I could just use the registry keys it uses, but it would likely be slower and would achieve the same result.

@CarterLi
Copy link

It's ok then, except that EDD_GET_DEVICE_INTERFACE_NAME should not be used here. EDD_GET_DEVICE_INTERFACE_NAME is meant to be used when fetching monitors, not adapters.

@Carterpersall
Copy link
Author

It's ok then, except that EDD_GET_DEVICE_INTERFACE_NAME should not be used here. EDD_GET_DEVICE_INTERFACE_NAME is meant to be used when fetching monitors, not adapters.

Good catch, I didn't see any speed improvement when setting it to 0 (EDD_GET_DEVICE_INTERFACE_NAME = 1) though. I'll remove it anyway to get rid of the unnecessary import.

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

Successfully merging this pull request may close these issues.

3 participants