Re: [RFC PATCH 9/9] winex11.drv: Thunk vkGetPhysicalDeviceProperties2 to report LUID.

[Zhiyi Zhang]

On 12/20/18 5:52 AM, Józef Kucia wrote:
> On Tue, Dec 18, 2018 at 5:25 PM Zhiyi Zhang  wrote:
> > Signed-off-by: Zhiyi Zhang
> >
> > dlls/vulkan-1/tests/vulkan.c |  4 +-
> >  dlls/winevulkan/make_vulkan  |  6 ++-
> >  dlls/winex11.drv/vulkan.c    | 81 ++++++++++++++++++++++++++++++++++++
> >  include/wine/vulkan_driver.h |  8 +++-
> >  4 files changed, 95 insertions(+), 4 deletions(-)
>
> I think that this should be implemented in winevulkan instead. It
> isn't winex11-specific and I don't see a reason to duplicate it in
> every Wine graphics driver.

Right. The reason for me thunk it in graphics driver is to avoid the
host struct conversion code in winevulkan. But now that you mention
the duplication in graphics driver, I guess thunking it in winevulkan would
be better indeed. I already had a version for it.
> > +static BOOL wine_get_adapter_luid(const GUID *uuid, LUID *luid)
> > +{
> > - HDEVINFO devinfo;
> > - SP_DEVINFO_DATA devinfo_data = {sizeof(SP_DEVINFO_DATA)};
> > - DEVPROPTYPE property_type;
> > - GUID result;
> > - UINT i;
> > -
> > - devinfo = SetupDiGetClassDevsW(&GUID_DEVCLASS_DISPLAY, NULL, NULL, DIGCF_PRESENT);
> > - if (devinfo == INVALID_HANDLE_VALUE)
> > -
> >    return FALSE;
> > -
> > - for (i = 0; SetupDiEnumDeviceInfo(devinfo, i, &devinfo_data); ++i)
> > - {
> > -
> >    property_type = DEVPROP_TYPE_UINT64;
> > -
> >    if (!SetupDiGetDevicePropertyW(devinfo, &devinfo_data, &DEVPROPKEY_DISPLAY_ADAPTER_UUID, &property_type,
> > -
> >                                   (BYTE *)&result, sizeof(result), NULL, 0))
> > -
> >        continue;
> > -
> > -
> >    if (IsEqualGUID(&result, uuid))
> > -
> >    {
> > -
> >        if (SetupDiGetDevicePropertyW(devinfo, &devinfo_data, &DEVPROPKEY_DISPLAY_ADAPTER_LUID, &property_type,
> > -
> >                                      (BYTE *)luid, sizeof(*luid), NULL, 0))
> > -
> >            return TRUE;
> > -
> >    }
> > - }
>
> You should call SetupDiDestroyDeviceInfoList().

Thanks, missed this one.
> > +static void wine_fill_physical_device_id_luid(VkPhysicalDeviceProperties2 *properties)
> > +{
> > - VkPhysicalDeviceIDProperties *id;
> > - struct wine_vk_structure_header *header;
> > -
> > - if (!properties->pNext)
> > -
> >    return;
> > -
> > - for (header = properties->pNext; header; header = header->pNext)
> > - {
>
> The if statement before the loop is redundant.
> > +static void X11DRV_vkGetPhysicalDeviceProperties2(VkPhysicalDevice phys_dev, VkPhysicalDeviceProperties2 *properties)
> > +{
> > - pvkGetPhysicalDeviceProperties2(phys_dev, properties);
> > - wine_fill_physical_device_id_luid(properties);
> >
> > +}
> > -
> >
> > +static void X11DRV_vkGetPhysicalDeviceProperties2KHR(VkPhysicalDevice phys_dev, VkPhysicalDeviceProperties2 *properties)
> > +{
> > - X11DRV_vkGetPhysicalDeviceProperties2(phys_dev, properties);
> >
> > +}
>
> You cannot assume that vkGetPhysicalDeviceProperties2() is available
> when vkGetPhysicalDeviceProperties2KHR() is called. We should call
> vkGetPhysicalDeviceProperties2KHR().

Thanks for the heads up.