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

Shouldn't layered drivers (as in VK_MSFT_layered_driver extension support) be reported only if VK_MSFT_layered_driver extension support is requested during vkCreateInstance? #2299

Open
zaiste-linganer opened this issue Jan 26, 2024 · 12 comments

Comments

@zaiste-linganer
Copy link

zaiste-linganer commented Jan 26, 2024

With recent loader updates and MSFT changes, we see workloads broken on repeated LUIDs, when Windows exposes layered drivers.

Old workloads does not need to be aware of the presence of layered drivers, and those should not be exposed, unless workload specifically asks for one (for example, by enabling VK_MSFT_layered_driver extension in VkInstanceCreateInfo).

@zaiste-linganer zaiste-linganer changed the title Shouldn Shouldn't layered drivers (as in VK_MSFT_layered_driver extension support) be reported only if VK_MSFT_layered_driver extension support is requested during vkCreateInstance? Jan 26, 2024
@zaiste-linganer
Copy link
Author

@jenatali @charles-lunarg, I was always assuming that layered drivers will be exposed ONLY if the extension is explicitly requested. Now, since Jan 23, we see a lot of failures from the workloads expecting LUID to be unique (my guess is that somehow, Dozen drivers started to be exposed in Win11).

Wasn't that original idea to hide layered devices for old workloads? I can't find exact language in the #2121

@jenatali
Copy link
Contributor

workloads broken on repeated LUIDs

Are you able to elaborate on which apps are impacted? The original intent of that extension was to provide information to the loader for the purpose of sorting, not hiding.

@zaiste-linganer
Copy link
Author

workloads broken on repeated LUIDs

Are you able to elaborate on which apps are impacted? The original intent of that extension was to provide information to the loader for the purpose of sorting, not hiding.

My specific case is Google Play Games, but I'm afraid it will affect other gfxstream based workloads as well.

Sorting will not help the applications that believes that LU in LUID means Locally Unique - as we do :) We have parts of the workload that basically uses LUID as the key to dictionaries - and that basically explodes our stack.

It would be much better if applications that may want to query layered devices would be able to explicitly request such entries returned from vkEnumeratePhysicalDevices - so all the other apps are not risking receiving repeated LUID devices...

@jenatali
Copy link
Contributor

That sounds reasonable to me. After sorting, the loader can uniquify based on LUID unless the app enabled an extension.

@zaiste-linganer
Copy link
Author

zaiste-linganer commented Jan 26, 2024

Thanks @jenatali. That sounds like a good way forward, where layered-drivers aware applications can use layered drivers at will, while those drivers are hidden from applications not aware of those implementations, that can get problems by duplicated ID.

@versalinyaa what would be the best way forward? Should we add modification to extension language (and spec) via SI TSG, combined with loader changes? Can we add that to next SI TSG meeting agenda if needed? Or shall we progress with PR/MR changes to spec/loader?

@oddhack
Copy link
Contributor

oddhack commented Jan 31, 2024

Tagging for the SI TSG. At least, it seems to be in their scope more than the main maintenance call.

@r-potter
Copy link
Contributor

That sounds reasonable to me. After sorting, the loader can uniquify based on LUID unless the app enabled an extension.

This presumably requires an instance extension though, because you'd need to have the loader do any filtering before vkEnumeratePhysicalDevices is called or any logical device is created?

Currently VK_MSFT_layered_driver is listed in the XML as a device extension, so it wouldn't be valid to enable via vkCreateInstance.

@zaiste-linganer
Copy link
Author

Currently VK_MSFT_layered_driver is listed in the XML as a device extension

Well, as it currently operates on vkGetPhysicalDeviceProperites level, this seems to be oversight of its own... or am I missing anything? Yes, I definitely agree it should be instance extension.

Tagging for the SI TSG. At least, it seems to be in their scope more than the main maintenance call.

Thank you! I don't have rights, it seems, to set the label here... I agree that it looks like SI related issue :)

@r-potter
Copy link
Contributor

Well, as it currently operates on vkGetPhysicalDeviceProperites level, this seems to be oversight of its own... or am I missing anything? Yes, I definitely agree it should be instance extension.

We allow device extensions to add support for physical device property queries (which is all that VK_MSFT_layered_driver currently does) for the corresponding device on which the extension is supported. In that sense, it's a valid device extension.

If the extension starts impacting what other devices are exposed (i.e. adding filtering), then it becomes instance-level functionality

@zaiste-linganer
Copy link
Author

If the extension starts impacting what other devices are exposed (...)
Well, one can argue that this extension does change the order how devices are enumerated - and even if not declared at the instance level. Whenever such a device is present, it will change the order of the other devices enumerations...

But I definitely see your point. My (obviously wrong) belief from the very old times of VK1.0 (where I used to be attending spec discussions) left me with the belief that device extension has to be declared at the logical device creation time, and aplied on the device-dispatched functions level.

With that being said, as this extension actually affects implementation of sorting devices at loader/instance level, shouldn't it be end of the day instance one anyway?

@zaiste-linganer
Copy link
Author

@ShabbyX as it seems it kind of overlaps with maintenance7 scope. Would you like to provide your thoughts on that?

@cubanismo
Copy link

With that being said, as this extension actually affects implementation of sorting devices at loader/instance level, shouldn't it be end of the day instance one anyway?

I think that's just a side effect of the loader implementation. Nothing in the spec says the loader sorts devices based on this information, so the spec itself doesn't actually violate any instance Vs. device rules.

Sorting will not help the applications that believes that LU in LUID means Locally Unique - as we do :) We have parts of the workload that basically uses LUID as the key to dictionaries - and that basically explodes our stack.

The wording describing LUID is a bit unfortunate. It's meant to describe a resource used to back the physical device, not uniquely map to a VkPhysicalDevice handle. If you read the later spec language, that's pretty clear, but the initial text is:

deviceLUID is an array of VK_LUID_SIZE uint8_t values representing a locally unique identifier for the device.

That should probably be updated to read:

deviceLUID is an array of VK_LUID_SIZE uint8_t values representing a locally unique identifier for the OS-level device referred to by the physical device.

To make it clearer there isn't necessarily a 1:1 mapping between LUID and VkPhysicalDevice.

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

No branches or pull requests

6 participants
@cubanismo @oddhack @r-potter @jenatali @zaiste-linganer and others