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

Sort order unspecified on Windows when multiple ICDs report physical devices for the same LUID #981

Closed
jenatali opened this issue Jul 22, 2022 · 8 comments · Fixed by #1342
Labels
enhancement New feature or request

Comments

@jenatali
Copy link
Contributor

Describe the bug

VulkanOn12 aka Dozen would like to use ICD loader interface version 6 and support vk_icdEnumerateAdapterPhysicalDevices. Since it's a layered implementation, it can report a physical device for every GPU that the underlying API (D3D12) supports. The problem arises when both Dozen and a hardware vendor want to report a physical device for the same GPU. In that case, it's unclear / arbitrary which order those physical devices would appear in the final Vulkan device enumeration.

In an ideal world, the hardware vendor's ICD's device would appear first, followed by the layered driver's device, before moving in to the next GPU in the system and repeating the same sort. In other words, the only time the layered driver would claim the first spot in the final device enumeration is if the system's preferred GPU doesn't have a vendor-provided Vulkan driver available.

@jenatali jenatali added the bug Something isn't working label Jul 22, 2022
@charles-lunarg
Copy link
Collaborator

This isn't unique to Dozen from my understanding. AMD on linux systems have issues where both the RADV and proprietary drivers report the same VkPhysicalDevice. The loader currently has a basic ordering system in place which makes the order consistent but doesn't have any strong 'preference' as to which goes first.

Therefore any solution to the issue on Windows should ideally also apply to linux.

It is possible to special case the Dozen vs "actual vulkan driver" here, (either by matching the name or through the Dozen-specific loader search mechanism) but that is a short sided solution unless no better alternative can be found.

@jenatali
Copy link
Contributor Author

The (minor) difference for Windows though is that the mechanism of how the driver was discovered can be leveraged to determine which driver is more appropriate for a given hardware device. If the driver was discovered because the ICD JSON was in the registry key for a GPU, then it seems reasonable for that to be the primary ICD for that GPU, as opposed to a globally-registered ICD that happens to expose a physical device for that GPU.

I do agree that it'd be great to be able to solve this problem all-up though.

@charles-lunarg
Copy link
Collaborator

While it would be nice to have some sort of solution for all platforms, I think in the case of Dozen it is preferable to special case the order, as there is a clear difference between the 'vulkan driver' and the 'vulkan on dx12 driver'. Whereas on linux, there is no clear distinction between an open and closed source driver for the same hardware.

I think making the Dozen driver appear later in the list should be simple enough to resolve this issue, especially because there are already issues open for the order physical devices should be in, #657

@charles-lunarg
Copy link
Collaborator

There is actually a good case for using the same solution here as there would be for #778, where it is desirable to put the 'real' gpu ahead of any software implementations.

A general ordering of physical devices that I don't think anyone would mind is Real hardware, layered implementation on real hardware, then lastly software implementations.

The VkPhysicalDeviceType lets us distinguish real hardware from software implementations, but there is no distinction for layered implementations. This could have another field added to the driver manifest, could result in a new extension exposing a new physical device type, or some other way for the loader to know why physical devices are layered.

I would prefer a solution that works for all layered implementations, but I could be persuaded differently.

@jenatali
Copy link
Contributor Author

This could have another field added to the driver manifest, could result in a new extension exposing a new physical device type, or some other way for the loader to know why physical devices are layered.

I don't think a new physical device type is the right option, because already-existing apps might not be expecting to see "layered" as a device type. It'd also need to be 2, to handle layered hardware and layered software, which is a case we'd like to support.

I think exposing the fact that it's layered out of the API is probably a good idea, so a new physical device property might be useful in some future core revision of the API, but for now a simple extension presence check would be sufficient, I think. Then the loader can query for that extension as well, and use it to help determine sort order?

I've never created a Vulkan extension before, but if that seems like the right approach to you, then I'm happy to give it a try?

@charles-lunarg
Copy link
Collaborator

charles-lunarg commented Oct 17, 2022

I agree, this isn't a device type but rather a 'property' of a physical device.

In my mind, this is a very basic extension, one that adds a new struct to add to the vkGetPhysicalDeviceProperties2 pNext chain.
A boolean 'yes/no' for if its layered, and maybe for fun a char field to denote which API its layered ontop of.

struct VkPhysicalDeviceLayeredImplementationPropertiesEXT { // name subject to change
    VkStructureType sType;
    const void* pNext;
    VkBool32 isLayered;
    char underlyingAPI[VK_MAX_EXTENSION_NAME_SIZE];
};

Adding a new Vulkan extension isn't hard but does have some procedural hurdles. This is something that should be done in the private Vulkan repo if possible, else would need to be moved to the public https://github.com/KhronosGroup/Vulkan-Docs repo.

@charles-lunarg charles-lunarg added enhancement New feature or request and removed bug Something isn't working labels Oct 26, 2022
@linyaa-kiwi
Copy link

cc @versalinyaa

@charles-lunarg
Copy link
Collaborator

With the public release of VK_MSFT_layered_driver, a PR should now be possible to write. How it'll be implemented may not be as simple as I'd like (device sorting never is) but it is at least possible now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants