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

[Liam Middlebrook]

Hi Brendan,

On 9/8/20 1:22 PM, Brendan Shanks wrote:
> Hi Liam,
> > On Sep 8, 2020, at 12:46 PM, Liam Middlebrook  wrote:
> >
> > Tested with Vulkan CTS and WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS.
> >
> > Signed-off-by: Liam Middlebrook 
> > Signed-off-by: Daniel Koch
> >
> > 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