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.

Sign up for GitHub

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

Jump to bottom

Implement VK_MSFT_layered_driver extension support #1342

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Implement VK_MSFT_layered_driver extension support #1342

merged 1 commit into from
Nov 6, 2023

Conversation

charles-lunarg
Copy link
Collaborator

This extension reorders physical devices enumerated through the windows EnumerateAdapterPhysicalDevices whenever multiple drivers exist for the same LUID. This is so that if a driver is considered a 'layered implementation', eg Dozen's Vulkan on Dx12, the physical device corresponding to the native vulkan driver is preferred.

@jenatali Would greatly appreciate it if you could verify this works for the situations described.

Fixes #981

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 66786.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2266 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2266 passed.

jjulianoatnv
jjulianoatnv reviewed Oct 26, 2023
View reviewed changes
Copy link
Contributor

@jjulianoatnv jjulianoatnv left a comment

Choose a reason for hiding this comment

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

I reviewed some pieces before exceeding my time budget to review, and I left review comments along the way.
Very long functions with very deep nesting are difficult to review.

loader/loader_common.h Outdated Show resolved Hide resolved
loader/loader_windows.c Outdated Show resolved Hide resolved
loader/loader_windows.c Show resolved Hide resolved
loader/loader_windows.c Outdated Show resolved Hide resolved
loader/loader_windows.c Outdated Show resolved Hide resolved
loader/loader_windows.c Outdated Show resolved Hide resolved
loader/loader_windows.c Outdated Show resolved Hide resolved
loader/loader_windows.c Outdated Show resolved Hide resolved
loader/loader_windows.c Show resolved Hide resolved
@charles-lunarg
Copy link
Collaborator Author

@jjulianoatnv Thank you for the deep review! You are absolutely right that deeply nested functions are tough to review. Means that it needs some refactoring to make it more understandable for sure, as evidenced by some of your comments.

@jenatali
Copy link
Contributor

I'm just returning from parental leave, and Dozen doesn't yet implement the relevant extension here. When I get a chance I'll get that hooked up and then give this a try, as well as a review. Thanks!

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 74340.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2305 running.

@charles-lunarg
Copy link
Collaborator Author

@jjulianoatnv You were absolutely right about the function being waaay to big. That happened due to just wanting to implement the logic without regard for clarity. The force push renames some variables for clarity, adds several needed comments, and breaks the big function into 3, the original one which calls the two child functions, one for actually calling EnumerateAdapterPhysicalDevices, and the other for sorting the physical devices based on whether they are layered or not.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2305 passed.

jenatali
jenatali reviewed Nov 2, 2023
View reviewed changes
loader/loader_windows.c Outdated Show resolved Hide resolved
jenatali
jenatali suggested changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@jenatali jenatali left a comment

Choose a reason for hiding this comment

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

One outstanding comment that's definitely a bug and needs to be fixed. Other than that LGTM.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 76515.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2310 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2310 passed.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 76541.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2311 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2311 failed.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 76559.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2312 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2312 failed.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 76574.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2313 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2313 failed.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 76594.

1 similar comment
@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 76594.

@charles-lunarg
Copy link
Collaborator Author

With both reviewers noting that loader_phys_dev_per_icd is confusing, I've done a pass renaming the structs/parameters/variables that should hopefully be clearer in what purpose they all serve.

The loader_phys_dev_per_icd is now named loader_icd_physical_devices which at least gives some indication that it is the physical devices associated with a single ICD. Also renamed the parameters, dropping 'sorted' since thats superfluous for what is just an array.

Apologies for the force push spam - my windows VM has decided to die on me and so I'm making the changes to windows only codepaths without a compiler in front of me.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2314 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2314 failed.

@charles-lunarg
Implement VK_MSFT_layered_driver extension support
30c9553 
This extension reorders physical devices enumerated through the windows
EnumerateAdapterPhysicalDevices whenever multiple drivers exist for the same
LUID. This is so that if a driver is considered a 'layered implementation',
eg Dozen's Vulkan on Dx12, the physical device corresponding to the native
vulkan driver is preferred.

The implementation of this extension cause some renaming of structs, variables,
and parameters to make the code easier to understand, such as renaming
the struct loader_phys_devs_per_icd to loader_icd_physical_devices.
During the review process there was quite a bit of confusion about what the
struct's purpose was that should be much clearer now.
@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 77203.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2316 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2316 passed.

@charles-lunarg charles-lunarg merged commit 8849936 into KhronosGroup:main Nov 6, 2023
47 checks passed
@charles-lunarg charles-lunarg deleted the msft_layered_driver_ext_support branch November 6, 2023 16:08
@jenatali jenatali mentioned this pull request Jan 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkY-LunarG MarkY-LunarG Awaiting requested review from MarkY-LunarG

@jjulianoatnv jjulianoatnv Awaiting requested review from jjulianoatnv

@jenatali jenatali Awaiting requested review from jenatali

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sort order unspecified on Windows when multiple ICDs report physical devices for the same LUID
4 participants
@charles-lunarg @ci-tester-lunarg @jenatali @jjulianoatnv