[PATCH v2 1/2] winevulkan: fail on explicit layer usage
Fixes failure in dEQP-VK.api.device_init.create_instance_layer_name_abuse which expects all layers passed in as part of test to cause instance creation to fail. A quirk bit WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS has been added to optionally restore the previous functionality. Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com> Signed-off-by: Daniel Koch <dkoch(a)nvidia.com> --- dlls/winevulkan/vulkan.c | 14 ++++++++++---- dlls/winevulkan/vulkan_private.h | 1 + 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index 1b359d22c8c..a8beef126bd 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -335,7 +335,7 @@ static void wine_vk_init_once(void) * driver is responsible for handling e.g. surface extensions. */ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo *src, - VkInstanceCreateInfo *dst) + VkInstanceCreateInfo *dst, struct VkInstance_T *object) { unsigned int i; VkResult res; @@ -351,8 +351,14 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo /* ICDs don't support any layers, so nothing to copy. Modern versions of the loader * filter this data out as well. */ - dst->enabledLayerCount = 0; - dst->ppEnabledLayerNames = NULL; + if (object->quirks & WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS) { + dst->enabledLayerCount = 0; + dst->ppEnabledLayerNames = NULL; + WARN("Ignoring explicit layers!\n"); + } else if (dst->enabledLayerCount) { + FIXME("Loading explicit layers is not supported by winevulkan!\n"); + return VK_ERROR_LAYER_NOT_PRESENT; + } TRACE("Enabled %u instance extensions.\n", dst->enabledExtensionCount); for (i = 0; i < dst->enabledExtensionCount; i++) @@ -673,7 +679,7 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, } object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE; - res = wine_vk_instance_convert_create_info(create_info, &create_info_host); + res = wine_vk_instance_convert_create_info(create_info, &create_info_host, object); if (res != VK_SUCCESS) { wine_vk_instance_free(object); diff --git a/dlls/winevulkan/vulkan_private.h b/dlls/winevulkan/vulkan_private.h index b048108f7a6..4bcc4de440d 100644 --- a/dlls/winevulkan/vulkan_private.h +++ b/dlls/winevulkan/vulkan_private.h @@ -39,6 +39,7 @@ #define WINEVULKAN_QUIRK_GET_DEVICE_PROC_ADDR 0x00000001 #define WINEVULKAN_QUIRK_ADJUST_MAX_IMAGE_COUNT 0x00000002 +#define WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS 0x00000004 struct vulkan_func { -- 2.17.1
Tested with Vulkan CTS and WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS. Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com> Signed-off-by: Daniel Koch <dkoch(a)nvidia.com> --- v2: fixup comment style dlls/winevulkan/vulkan.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index a8beef126bd..19093d47390 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -662,6 +662,7 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, const VkApplicationInfo *app_info; struct VkInstance_T *object; VkResult res; + HKEY key; TRACE("create_info %p, allocator %p, instance %p\n", create_info, allocator, instance); @@ -679,6 +680,22 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, } object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE; + /* Load optional WineVulkan quirks bits from registry, see vulkan_private.h + * for a list of quirks. + */ + if (RegOpenKeyA(HKEY_CURRENT_USER, "Software\\Wine\\Vulkan", &key) == 0) + { + DWORD type, value, size; + size = sizeof(value); + if (RegQueryValueExA(key, "Quirks", NULL, &type, (LPBYTE)&value, &size) == 0 + && type == REG_DWORD) + { + object->quirks = value; + TRACE("Loaded Quirks value %x\n", value); + } + RegCloseKey(key); + } + res = wine_vk_instance_convert_create_info(create_info, &create_info_host, object); if (res != VK_SUCCESS) { -- 2.17.1
Hi Liam,
On Sep 8, 2020, at 12:46 PM, Liam Middlebrook <lmiddlebrook(a)nvidia.com> wrote:
Tested with Vulkan CTS and WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS.
Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com> Signed-off-by: Daniel Koch <dkoch(a)nvidia.com> --- v2: fixup comment style
dlls/winevulkan/vulkan.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index a8beef126bd..19093d47390 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -662,6 +662,7 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, const VkApplicationInfo *app_info; struct VkInstance_T *object; VkResult res; + HKEY key;
TRACE("create_info %p, allocator %p, instance %p\n", create_info, allocator, instance);
@@ -679,6 +680,22 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, } object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE;
+ /* Load optional WineVulkan quirks bits from registry, see vulkan_private.h + * for a list of quirks. + */
There’s a specially-formatted comment that is used to highlight Wine registry keys, like: /* @@ Wine registry key: HKCU\Software\Wine\Vulkan */ Also, this seems like a good fit to support AppDefaults in the registry. Thanks, Brendan
Hi Brendan, On 9/8/20 1:22 PM, Brendan Shanks wrote:
Hi Liam,
On Sep 8, 2020, at 12:46 PM, Liam Middlebrook <lmiddlebrook(a)nvidia.com> wrote:
Tested with Vulkan CTS and WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS.
Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com> Signed-off-by: Daniel Koch <dkoch(a)nvidia.com> --- v2: fixup comment style
dlls/winevulkan/vulkan.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index a8beef126bd..19093d47390 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -662,6 +662,7 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, const VkApplicationInfo *app_info; struct VkInstance_T *object; VkResult res; + HKEY key;
TRACE("create_info %p, allocator %p, instance %p\n", create_info, allocator, instance);
@@ -679,6 +680,22 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, } object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE;
+ /* Load optional WineVulkan quirks bits from registry, see vulkan_private.h + * for a list of quirks. + */
There’s a specially-formatted comment that is used to highlight Wine registry keys, like:
/* @@ Wine registry key: HKCU\Software\Wine\Vulkan */
Ah cool, I was looking through examples in other places and must have missed that. I'll add that in later on, but I figure we may want to hash out the below a bit more first, before I send any follow-up patches.
Also, this seems like a good fit to support AppDefaults in the registry.
Yeah, I think that makes sense to do. Rather than using the process name like is done elsewhere, how would you feel about making use of pApplicationName and pEngineName from VkApplicationInfo? The regkey structure could be something like: HKCU\Software\Wine\Vulkan\ApplicationName\%pApplicationName%\Quirks HKCU\Software\Wine\Vulkan\EngineName\%pEngineName%\Quirks Although it'd also be nice to try to account for versions there, but maybe doing that from the registry isn't the right spot. Ideally we shouldn't really need to be adding any more of these application-specific workarounds anyways, but for older poorly-behaving applications they'll always be needed. Another though I had on this (+CC Georg) was that we should probably invert WINEVULKAN_QUIRK_ADJUST_MAX_IMAGE_COUNT so that the default is zero, possibly rename to add a _DONT_. Thinking further on quirk defaults, I wonder if they should be handled as binary, or if trinary is a better fit (unset, forced on, forced off). Right now the quirks bits have us choosing some reasonable default for all applications, and then either using the prospective regkey to enable a quirk, or having winevulkan pick up on some runtime-specific state to determine if a quirk needs to be enabled or not. Although if we're thinking down that route, maybe it would be easiest to have each quirk split to a separate regkey-value, and then track those settings in a structure. Although, maybe that's all a bit much, and we should really just grab the current EXE name for any App specific matching, and use the single Quirks regkey-value. Curious to hear your thoughts on this. Thanks, Liam Middlebrook
Thanks, Brendan
On Sep 8, 2020, at 6:44 PM, Liam Middlebrook <lmiddlebrook(a)nvidia.com> wrote:
Hi Brendan,
On 9/8/20 1:22 PM, Brendan Shanks wrote:
Hi Liam,
On Sep 8, 2020, at 12:46 PM, Liam Middlebrook <lmiddlebrook(a)nvidia.com> wrote:
Tested with Vulkan CTS and WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS.
Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com> Signed-off-by: Daniel Koch <dkoch(a)nvidia.com> --- v2: fixup comment style
dlls/winevulkan/vulkan.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index a8beef126bd..19093d47390 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -662,6 +662,7 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, const VkApplicationInfo *app_info; struct VkInstance_T *object; VkResult res; + HKEY key;
TRACE("create_info %p, allocator %p, instance %p\n", create_info, allocator, instance);
@@ -679,6 +680,22 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, } object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE;
+ /* Load optional WineVulkan quirks bits from registry, see vulkan_private.h + * for a list of quirks. + */ There’s a specially-formatted comment that is used to highlight Wine registry keys, like: /* @@ Wine registry key: HKCU\Software\Wine\Vulkan */
Ah cool, I was looking through examples in other places and must have missed that. I'll add that in later on, but I figure we may want to hash out the below a bit more first, before I send any follow-up patches.
Also, this seems like a good fit to support AppDefaults in the registry.
Yeah, I think that makes sense to do. Rather than using the process name like is done elsewhere, how would you feel about making use of pApplicationName and pEngineName from VkApplicationInfo?
The regkey structure could be something like: HKCU\Software\Wine\Vulkan\ApplicationName\%pApplicationName%\Quirks HKCU\Software\Wine\Vulkan\EngineName\%pEngineName%\Quirks
Yeah I like this idea. Some day we’ll need to apply a quirk for “Game.exe” or “Launcher.exe”, and this will come in handy. I guess in this case you would check them from most to least specific, so application name->engine name->executable name. And then each level would be checked and the first quirks value found gets used?
Although it'd also be nice to try to account for versions there, but maybe doing that from the registry isn't the right spot. Ideally we shouldn't really need to be adding any more of these application-specific workarounds anyways, but for older poorly-behaving applications they'll always be needed.
Maybe version could be done with another level of keys, like 'Vulkan\ApplicationName\%pApplicationName%\%applicationVersion%\Quirks’. This starts to get complicated though, maybe it should wait until a clear need arises. Plus, a version number check would often need to be ‘less-than some version number’, which is even more complicated.
Another though I had on this (+CC Georg) was that we should probably invert WINEVULKAN_QUIRK_ADJUST_MAX_IMAGE_COUNT so that the default is zero, possibly rename to add a _DONT_.
Thinking further on quirk defaults, I wonder if they should be handled as binary, or if trinary is a better fit (unset, forced on, forced off). Right now the quirks bits have us choosing some reasonable default for all applications, and then either using the prospective regkey to enable a quirk, or having winevulkan pick up on some runtime-specific state to determine if a quirk needs to be enabled or not. Although if we're thinking down that route, maybe it would be easiest to have each quirk split to a separate regkey-value, and then track those settings in a structure.
Yeah I think you would want all quirks to default to zero, so if you add new quirks the existing per-app registry values won’t need to change. Trinary or separate values are tempting, but it adds a lot of complexity vs just a bit mask. To me, I think it’s worth sticking with a single bit mask unless you can really see the use for something more complicated. Lately I’ve been working on a similar system for per-app overriding the vendor/device ID returned in VkPhysicalDeviceProperties, and hitting some similar issues. These would both be living in vulkan.c, maybe some functions can be shared for all the registry code. Brendan
On 9/9/20 6:47 PM, Brendan Shanks wrote:
On Sep 8, 2020, at 6:44 PM, Liam Middlebrook <lmiddlebrook(a)nvidia.com <mailto:lmiddlebrook(a)nvidia.com>> wrote:
Hi Brendan,
On 9/8/20 1:22 PM, Brendan Shanks wrote:
Hi Liam,
On Sep 8, 2020, at 12:46 PM, Liam Middlebrook <lmiddlebrook(a)nvidia.com <mailto:lmiddlebrook(a)nvidia.com>> wrote:
Tested with Vulkan CTS and WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS.
Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com <mailto:lmiddlebrook(a)nvidia.com>> Signed-off-by: Daniel Koch <dkoch(a)nvidia.com <mailto:dkoch(a)nvidia.com>> --- v2: fixup comment style
dlls/winevulkan/vulkan.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index a8beef126bd..19093d47390 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -662,6 +662,7 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, const VkApplicationInfo *app_info; struct VkInstance_T *object; VkResult res; + HKEY key;
TRACE("create_info %p, allocator %p, instance %p\n", create_info, allocator, instance);
@@ -679,6 +680,22 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, } object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE;
+ /* Load optional WineVulkan quirks bits from registry, see vulkan_private.h + * for a list of quirks. + */ There’s a specially-formatted comment that is used to highlight Wine registry keys, like: /* @@ Wine registry key: HKCU\Software\Wine\Vulkan */
Ah cool, I was looking through examples in other places and must have missed that. I'll add that in later on, but I figure we may want to hash out the below a bit more first, before I send any follow-up patches.
Also, this seems like a good fit to support AppDefaults in the registry.
Yeah, I think that makes sense to do. Rather than using the process name like is done elsewhere, how would you feel about making use of pApplicationName and pEngineName from VkApplicationInfo?
The regkey structure could be something like: HKCU\Software\Wine\Vulkan\ApplicationName\%pApplicationName%\Quirks HKCU\Software\Wine\Vulkan\EngineName\%pEngineName%\Quirks
Yeah I like this idea. Some day we’ll need to apply a quirk for “Game.exe” or “Launcher.exe”, and this will come in handy.
I guess in this case you would check them from most to least specific, so application name->engine name->executable name. And then each level would be checked and the first quirks value found gets used?
Yeah, I was planning on doing four levels: 1. pApplicationName 2. pEngineName 3. exe name 4. global
Although it'd also be nice to try to account for versions there, but maybe doing that from the registry isn't the right spot. Ideally we shouldn't really need to be adding any more of these application-specific workarounds anyways, but for older poorly-behaving applications they'll always be needed.
Maybe version could be done with another level of keys, like 'Vulkan\ApplicationName\%pApplicationName%\%applicationVersion%\Quirks’. This starts to get complicated though, maybe it should wait until a clear need arises. Plus, a version number check would often need to be ‘less-than some version number’, which is even more complicated.
Sounds good. Generally speaking as long as pApplicationNames are provided and somewhat unique, I think that'll take us fairly far.
Another though I had on this (+CC Georg) was that we should probably invert WINEVULKAN_QUIRK_ADJUST_MAX_IMAGE_COUNT so that the default is zero, possibly rename to add a _DONT_.
Thinking further on quirk defaults, I wonder if they should be handled as binary, or if trinary is a better fit (unset, forced on, forced off). Right now the quirks bits have us choosing some reasonable default for all applications, and then either using the prospective regkey to enable a quirk, or having winevulkan pick up on some runtime-specific state to determine if a quirk needs to be enabled or not. Although if we're thinking down that route, maybe it would be easiest to have each quirk split to a separate regkey-value, and then track those settings in a structure.
Yeah I think you would want all quirks to default to zero, so if you add new quirks the existing per-app registry values won’t need to change.
Agreed, I'm also thinking that any quirks by default shouldn't reduce how conformant a WineVulkan implementation is (not to say that we are currently passing CTS, but it shouldn't make matters any worse).
Trinary or separate values are tempting, but it adds a lot of complexity vs just a bit mask. To me, I think it’s worth sticking with a single bit mask unless you can really see the use for something more complicated.
Lately I’ve been working on a similar system for per-app overriding the vendor/device ID returned in VkPhysicalDeviceProperties, and hitting some similar issues. These would both be living in vulkan.c, maybe some functions can be shared for all the registry code.
Thanks, I'll try to keep that in mind when I write out the next series with all of this then. Thanks, Liam Middlebrook
Brendan
Hi Liam, On Sep 9, 2020, at 03:44 AM, Liam Middlebrook <lmiddlebrook(a)nvidia.com> wrote:
Hi Brendan,
On 9/8/20 1:22 PM, Brendan Shanks wrote:
Hi Liam,
On Sep 8, 2020, at 12:46 PM, Liam Middlebrook <lmiddlebrook(a)nvidia.com> wrote:
Tested with Vulkan CTS and WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS.
Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com> Signed-off-by: Daniel Koch <dkoch(a)nvidia.com> --- v2: fixup comment style
dlls/winevulkan/vulkan.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index a8beef126bd..19093d47390 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -662,6 +662,7 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, const VkApplicationInfo *app_info; struct VkInstance_T *object; VkResult res; + HKEY key;
TRACE("create_info %p, allocator %p, instance %p\n", create_info, allocator, instance);
@@ -679,6 +680,22 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, } object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE;
+ /* Load optional WineVulkan quirks bits from registry, see vulkan_private.h + * for a list of quirks. + */
There’s a specially-formatted comment that is used to highlight Wine registry keys, like:
/* @@ Wine registry key: HKCU\Software\Wine\Vulkan */
Ah cool, I was looking through examples in other places and must have missed that. I'll add that in later on, but I figure we may want to hash out the below a bit more first, before I send any follow-up patches.
Also, this seems like a good fit to support AppDefaults in the registry.
Yeah, I think that makes sense to do. Rather than using the process name like is done elsewhere, how would you feel about making use of pApplicationName and pEngineName from VkApplicationInfo?
The regkey structure could be something like: HKCU\Software\Wine\Vulkan\ApplicationName\%pApplicationName%\Quirks HKCU\Software\Wine\Vulkan\EngineName\%pEngineName%\Quirks
Although it'd also be nice to try to account for versions there, but maybe doing that from the registry isn't the right spot. Ideally we shouldn't really need to be adding any more of these application-specific workarounds anyways, but for older poorly-behaving applications they'll always be needed.
Another though I had on this (+CC Georg) was that we should probably invert WINEVULKAN_QUIRK_ADJUST_MAX_IMAGE_COUNT so that the default is zero, possibly rename to add a _DONT_.
I agree, inverting it makes sense because we want that quirk to be activated by default. It fixes a very common mistake that will probably keep getting made, so maintaining app profiles for it seems like a lot of work for no reason and a functional regression at this point. And since the behavior with that quirk enabled is closer to what windows drivers report, there should be no application that breaks because of it. Thanks, Georg
Thinking further on quirk defaults, I wonder if they should be handled as binary, or if trinary is a better fit (unset, forced on, forced off). Right now the quirks bits have us choosing some reasonable default for all applications, and then either using the prospective regkey to enable a quirk, or having winevulkan pick up on some runtime-specific state to determine if a quirk needs to be enabled or not. Although if we're thinking down that route, maybe it would be easiest to have each quirk split to a separate regkey-value, and then track those settings in a structure.
Although, maybe that's all a bit much, and we should really just grab the current EXE name for any App specific matching, and use the single Quirks regkey-value. Curious to hear your thoughts on this.
Thanks,
Liam Middlebrook
Thanks, Brendan
participants (3)
-
Brendan Shanks -
Dad Schoorse -
Liam Middlebrook