[PATCH v2 2/2] winevulkan: Add registry key for manipulating quirks

[Liam Middlebrook]

On 9/9/20 6:47 PM, Brendan Shanks wrote:
> 
> 
>> On Sep 8, 2020, at 6:44 PM, Liam Middlebrook <lmiddlebrook at nvidia.com 
>> <mailto:lmiddlebrook at 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 at nvidia.com <mailto:lmiddlebrook at nvidia.com>> wrote:
>>>>
>>>> Tested with Vulkan CTS and WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS.
>>>>
>>>> Signed-off-by: Liam Middlebrook <lmiddlebrook at nvidia.com 
>>>> <mailto:lmiddlebrook at nvidia.com>>
>>>> Signed-off-by: Daniel Koch <dkoch at nvidia.com <mailto:dkoch at 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
>