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

[Dad Schoorse]

Hi Liam,

On Sep 9, 2020, at 03:44 AM, Liam Middlebrook 
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

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_.

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