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

Layer violates LLP_LAYER_16 #2033

Open
Rob2309 opened this issue Jan 4, 2023 · 18 comments
Open

Layer violates LLP_LAYER_16 #2033

Rob2309 opened this issue Jan 4, 2023 · 18 comments
Assignees

Comments

@Rob2309
Copy link

Rob2309 commented Jan 4, 2023

I am currently trying to understand the Interface between the Loader and a Layer. I found out about LLP_LAYER_16, which states that vkEnumerateDeviceExtensionProperties must only return its own extensions when pLayerName is specifying the current layer. It seems to me like almost all Khronos layers violate this requirement by appending their extensions to the list of extensions supported by the driver.
Am I understanding something completely wrong?

As an example, the synchronization2 layer does this, probably to mask the fact that the synchronization2 extension is implemented in a layer, but is this allowed by the spec?

I am asking this here instead of in the Loader repo because it is not directly an issue with the loader.

@jeremyg-lunarg
Copy link

@MarkY-LunarG & @charles-lunarg any recommendations on how to proceed with this? The relevant code is here.

@jeremyg-lunarg jeremyg-lunarg self-assigned this Jan 4, 2023
@charles-lunarg
Copy link

. It seems to me like almost all Khronos layers violate this requirement by appending their extensions to the list of extensions supported by the driver.

If most layers are doing that, then most layers are written incorrectly.

The Vulkan spec states for vkEnumerateDeviceExtensionProperties:

When pLayerName parameter is NULL, only extensions provided by the Vulkan implementation or by implicitly enabled layers are returned. When pLayerName is the name of a layer, the device extensions provided by that layer are returned.
https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkEnumerateDeviceExtensionProperties.html

@Rob2309
Copy link
Author

Rob2309 commented Jan 10, 2023

The layers probably do it this way so that the implemented extensions appear as if the driver supports them directly, which makes initialization and extension discovery a bit easier on the user's side. I think that is generally a good idea, so maybe there should be a way to do this in accordance with the spec.

@charles-lunarg
Copy link

Upon further review, I actually think the specification is wrong here.

The way it was written assumes that layers which aren't implicitly enabled do not provide any extensions. Except in other parts of the spec, it claims that implicit layers only differ in them 'being enabled through mechanisms outside of the API'. The functionality of layers is the same for both types, so calling out that explicit layers can't add extensions is inconsistent.

Section 43.3:
Except where otherwise specified, implicitly enabled and explicitly enabled layers differ only in the way they are enabled, and the order in which they are loaded. Explicitly enabling a layer that is implicitly enabled results in this layer being loaded as an implicitly enabled layer; it has no additional effect.

Yes, this has the carve out that allows vkEnumerateDeviceExtensionProperties to state that only implicit layers are allow to add extensions, but I would argue that this restriction is not desirable.

Layers such as VK_LAYER_KHRONOS_synchronization2 would not be possible if they couldn't add VK_KHR_synchronization2 as an 'available' extension. This is one example of many where layers add extensions for the convenience of the user, regardless of whether they are implicit or explicit.

FYI - Layers don't 'know' they are implicit or explicit. Sure, the author of a layer may code the layer in such a manner, but there are almost no differences in the layer code between an implicit layer and explicit layer.

@jeremyg-lunarg
Copy link

Thanks @charles-lunarg, should this be converted to a Loader or something-else issue?

@charles-lunarg
Copy link

Vulkan-Docs, because the spec should be clarified to not exclude explicit layers.

@charles-lunarg
Copy link

Oh and yes, should have a corresponding Vulkan Loader issue since LLP_LAYER_16 is a loader concept.

@jeremyg-lunarg jeremyg-lunarg transferred this issue from KhronosGroup/Vulkan-ExtensionLayer Jan 10, 2023
@Rob2309
Copy link
Author

Rob2309 commented Jan 12, 2023

So what would the "correct" behavior for a layer be now? If a layer wants to implement support for an extension, should it modify the extensions returned by vkEnumerateDeviceExtensionProperties or should it conform to the LLP_LAYER_16 requirement, waiting for the loader code to be corrected?

@Rob2309
Copy link
Author

Rob2309 commented Jan 12, 2023

The way it was written assumes that layers which aren't implicitly enabled do not provide any extensions. Except in other parts of the spec, it claims that implicit layers only differ in them 'being enabled through mechanisms outside of the API'. The functionality of layers is the same for both types, so calling out that explicit layers can't add extensions is inconsistent.

Another area where this mechanism seems unwanted is VK_EXT_tooling_info, only gives information about the enabled tools if either the driver directly supports it or at least one implicit layer supports it.
Explicit layers are unable to reliably report their tooling info when conforming to the loader requirements.

@r-potter
Copy link
Contributor

The way it was written assumes that layers which aren't implicitly enabled do not provide any extensions. Except in other parts of the spec, it claims that implicit layers only differ in them 'being enabled through mechanisms outside of the API'. The functionality of layers is the same for both types, so calling out that explicit layers can't add extensions is inconsistent.

This doesn't seem correct to me. My expectation is that explicit layers can add extensions, and I don't think we intended to prohibit that.

The whole purpose of being able to call vkEnumerateDeviceExtensionProperties with a non-null layerName is to identify what extensions an explicit layer might provide. With implicit layers the application doesn't even know the layer is present (i.e. they don't show up in vkEnumerateInstanceLayerProperties), so it cannot know what layerName to provide.

@MarkY-LunarG
Copy link
Contributor

MarkY-LunarG commented Jan 12, 2023

Are we leaning towards saying this is allowed for implicit layers?

In a nut-shell:

Implicit layers must add extensions they implement to the list of returned extensions in all cases (including with (`layerName` == nullptr) unless they already exist in the list.
Explicit layers will only return their list if explicitly called out by `layerName` == myName.

@Rob2309
Copy link
Author

Rob2309 commented Jan 13, 2023

Are we leaning towards saying this is allowed for implicit layers?

In a nut-shell:


Implicit layers must add extensions they implement to the list of returned extensions in all cases (including with (`layerName` == nullptr) unless they already exist in the list.

Explicit layers will only return their list if explicitly called out by `layerName` == myName.

This is currently automatically handled by the loader correctly, it automatically calls vkEnumerateDeviceExtensionProperties with pLayerName==someLayer for implicit layers and adds the returned extensions to the final list of device extensions. It doesn't do this for explicit layers, meaning the function behaves exactly as intended by the spec then. This means, when following LLP_LAYER_16, everything works as specified in the spec.

The problem I see is that the official layers do not conform to the loader requirement and manually add their extensions, breaking the specified functionality of vkEnumerateDeviceExtensionProperties.
However, I also think this behavior is a bit awkward with e.g. VK_EXT_tooling_info.

@r-potter
Copy link
Contributor

r-potter commented Jan 13, 2023

Implicit layers must add extensions they implement to the list of returned extensions in all cases (including with (layerName == nullptr) unless they already exist in the list.
Explicit layers will only return their list if explicitly called out by layerName == myName.

That's not quite what I was describing. My opinion is:

  1. All layers should respond to calls to vkEnumerateDeviceExtensionProperties where layerName == myName with just the list of extensions they implement.
  2. All layers should generally do nothing in the layerName == NULL case
  3. The loader should respond to applications calling vkEnumerateDeviceExtensionProperties with layerName == NULL by calling vkEnumerateDeviceExtensionProperties with the appropriate layerName set for each implicit layer, and concatenate/dedup the results.

The result of this is that layers only need to handle the case where layerName == myName, and will then work in both the implicit and explicit case. In my opinion, if Khronos layers aren't doing that, then they are incorrectly written.

I'm not quite following the issue with VK_EXT_tooling_info, beyond that every layer that wants to be reported needs to implement it, and correctly handle passing the call down the chain

@Rob2309
Copy link
Author

Rob2309 commented Jan 13, 2023

I'm not quite following the issue with VK_EXT_tooling_info, beyond that every layer that wants to be reported needs to implement it, and correctly handle passing the call down the chain

This makes the usage of such "general information" extensions very cumbersome imo. When using this extension, an application will mostly not care which layer implements it, as the return value of vkGetPhysicalDeviceToolPropertiesEXT is a list of ALL layers/tools that are enabled.
So if no implicit layer supports this extension, an application will have to loop through all layers in order to ensure that ANY one of them supports this extension before activating it.

@r-potter
Copy link
Contributor

Okay. That's fair. Hopefully the fact we made it core in 1.3 helps a little there, but it obviously doesn't solve the issue on all platforms today.

@Rob2309
Copy link
Author

Rob2309 commented Jan 13, 2023

That definitely helps a lot 👍

@MarkY-LunarG
Copy link
Contributor

This is currently automatically handled by the loader correctly, it automatically calls vkEnumerateDeviceExtensionProperties with pLayerName==someLayer for implicit layers and adds the returned extensions to the final list of device extensions. It doesn't do this for explicit layers, meaning the function behaves exactly as intended by the spec then. This means, when following LLP_LAYER_16, everything works as specified in the spec.

Thanks for the clarification @Rob2309

@MarkY-LunarG
Copy link
Contributor

Ah, actually, the loader gets this info from the JSON, I honestly didn't remember since I've been out of digging into this side of stuff with the loader for a few years now.

Basically, if the layer defines extensions in the appropriate section of the JSON, it gets added by the loader. [BTW, @charles-lunarg found it].

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

No branches or pull requests

5 participants