library: Split capabilities creation from init

[GrinlexGH]

Fixes: #734

Previously vpCreateCapabilities() both allocated the VpCapabilities object and resolved all Vulkan function pointers via a VpCapabilitiesCreateInfo argument, including apiVersion, which does not make sense outside of an instance/device context.

Split this into three explicit steps:

• vpCreateCapabilities() now only allocates the object.
• vpInitialize() resolves the global-level Vulkan functions (vkCreateInstance, vkEnumerateInstanceExtensionProperties, etc.), either statically or via an application-supplied GetInstanceProcAddr.
• vpLoadInstance() resolves the instance-level functions (vkCreateDevice, vkGetPhysicalDevice*2, etc.) once a VkInstance exists, with an optional flag to fall back to the VK_KHR_get_physical_device_properties2 entry points on Vulkan 1.0 implementations.

This removes the apiVersion field, which is no longer needed for validation, and drops GetDeviceProcAddr from VpVulkanFunctions since device functions are now loaded through the instance.

VP_PROFILE_CREATE_STATIC_BIT is renamed to VP_CAPABILITIES_CREATE_STATIC_BIT and is only available
when static loader linking is possible; a new VP_CAPABILITIES_CREATE_DYNAMIC_BIT flag covers the dynamic loading case.

Update tests, the mock Vulkan API, and the generated library template to use the new two-step init/load flow.

_{AI Free}

[ci-tester-lunarg]

Author GrinlexGH not on autobuild list. Waiting for curator authorization before starting CI build.

[ci-tester-lunarg]

Author GrinlexGH not on autobuild list. Waiting for curator authorization before starting CI build.

[CLAassistant]

All committers have signed the CLA.

[ci-tester-lunarg]

Author GrinlexGH not on autobuild list. Waiting for curator authorization before starting CI build.

[ci-tester-lunarg]

Author GrinlexGH not on autobuild list. Waiting for curator authorization before starting CI build.

[christophe-lunarg]

I pull the branch to #926 to trigger Internal C.I. which fails on macOS run of TEST(no_prototypes, create_instance_with_dynamic_pointers).

consoleText.txt

Considering that on macOS we typically expect the Vulkan Loader and the Vulkan driver (MoltenVK or KosmicKrips) to be packaged with the Vulkan application, I can imagine DispatchLoaderDynamic would work pretty differently. Maybe it's just fine to disable the test on macOS

[ci-tester-lunarg]

Author GrinlexGH not on autobuild list. Waiting for curator authorization before starting CI build.

[ci-tester-lunarg]

Author GrinlexGH not on autobuild list. Waiting for curator authorization before starting CI build.

[GrinlexGH]

As stated here: https://github.com/KhronosGroup/MoltenVK#using-the-vulkan-sdk, on macOS you have to create an instance with VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR, if this fix doesn't work, you can disable this test for macOS

[christophe-lunarg]

Open to here what you think about these comments.

[christophe-lunarg]

Did you consider automatically calling this function in vpCreateInstance to load the Vulkan device functions?

[GrinlexGH]

I copied the behavior of these functions from volk and thought about it. I initially did this because I thought it was always explicit > implicit, plus, architecturally, instance creation shouldn't be tied to initialization. In theory, if a library user decides to create multiple VpCapabilities, their initialization won't be tied to instance creation, but will the user actually do that? Also, in theory, the user could get an instance from something other than vpCreateInstance, which would also allow for proper initialization of the VpCapabilities object, but again, it's unlikely the user will do this, although they can. I don't know whether to allow this option, because the valid use of the library is a single VpCapabilities and sequential creation of instance and devices using vulkan profiles functions. In fact, this library is quite high-level and architecturally it does not harm in any way, so I can implicitly call vpLoadInstance inside vpCreateInstance, but let the user call vpLoadInstance before vpCreateInstance if he needs to

[GrinlexGH]

But what should happen if the instance was created successfully, but vpLoadInstance failed? Should vkInstance be automatically deleted?

[christophe-lunarg]

Ok, it makes sense to me to keep the option of calling vpLoadInstance and call it implicitly in vpCreateInstance.

Also, I am not convinced VpCapabilities is the correct abstraction. We needed something to store the function pointer for "custom" function pointer but I don't think it's not right.

[christophe-lunarg]

It's not clear to me what's the purpose of VP_DISABLE_STATIC_LINKING. Maybe it's just a documentation request.

[GrinlexGH]

I created this in case the user doesn't want to use function declarations even if VK_NO_PROTOTYPES isn't defined. Although it's unlikely to be useful anywhere, I think it's best to remove it

[christophe-lunarg]

It would have be great to keep the API backward compatible.

I created VP_USE_OBJECT only for this purpose but it's something I'd like to remove and have the Vulkan developer responsible for the loading and the Vulkan API.

[GrinlexGH]

How much should backward compatibility be maintained?

[christophe-lunarg]

I think it's fine to break compatibility with the VP_USE_OBJECT code path which is explicitly marked "beta".

[christophe-lunarg]

I think the vpLoadInstance and vpInitialize functions are a little be confusing to me. Probably just an naming issue.

Maybe the VpCapabilities_T should be completely redesigned and considering it's not enabled by default I think it would be ok. Probably VpCapabilities is not a great name either and in a first place. Maybe VpInstance?

For example, we could create a per VkInstance table to store the function loaded with ImportInstanceVulkanFunctions_Dynamic.

I could picture something like vpLoadGlobalFunc replacing vpInitialize:

VpVulkanFunctions vulkanFunctions;
vpLoadGlobalFunc(dl.vkGetInstanceProcAddr, vulkanFunctions);

Where vpLoadGlobalFunc is just a helper function to fill global functions using dl.vkGetInstanceProcAddr. This leave the posibility for Vulkan developer to fill either function manually.

vpCreateInstance would return VK_ERROR_INITIALIZATION_FAILED if pVulkanFunctions is not initialized/validated when VK_NO_PROTOTYPES and VP_DISABLE_STATIC_LINKING are not defined.

Otherwise, vpCreateInstance would use the static function automatically.

[GrinlexGH]

I think a table for instance -> VpCapabilities object would be an unnecessarily complex functionality
It's a good idea to use the vpLoadGlobalFunc helper. This could eliminate the _STATIC/_DYNAMIC_BIT altogether.
I think it could look something like this:

VpVulkanFunctions vulkanFunctions;

// optional dynamic loading
if (wantDynamicLoading) {
  // Loads only global dynamic functions
  vpLoadGlobalFunc(dl.vkGetInstanceProcAddr, &vulkanFunctions);
}

VpCapabilities capabilities {};
VpCapabilitiesCreateInfo createInfo;
createInfo.pVulkanFunctions = vulkanFunctions;

// If global functions have not been loaded (pVulkanFunctions is nullptr or doesn't have gipa), it will try to load static versions if VK_NO_PROTOTYPES is not defined, otherwise it will return an error

// If global functions have been loaded (capabilities have gipa), it will load the remaining functions in vpCreateInstance via vkGetInstanceProcAddr, and will not try to load static functions

// by the way if you rename VpCapabilities to VpInstance, you will create a conflict in function names :(
vpCreateCapabilities(&createInfo, nullptr, &capabilities);

By the way, this will also solve the problem of backward compatibility, so apiVersion and flags can just be deprecated

but how to dynamically initialize a singleton in this case?

I can leave the global vpInitialize function, but without the first capabilities parameter, which would initialize the global singleton, so it could look something like this:

// VP_USE_OBJECT is not defined

// optional dynamic loading
if (wantDynamicLoading) {
    vk::detail::DispatchLoaderDynamic dl;
    dl.init();

    VpVulkanFunctions vulkanFunctions{};
    vpLoadGlobalFunc(dl.vkGetInstanceProcAddr, &vulkanFunctions);

    VpCapabilitiesCreateInfo createInfo{};
    createInfo.pVulkanFunctions = &vulkanFunctions;

    vpInitialize(&createInfo);
}
// If not dynamicly initialized, singleton will implicitly load static functions in Get method if VK_NO_PROTOTYPES is not defined (otherwise return an error)
vpGetInstanceProfileSupport(layername, &profile, &supported);
// If dynamicly initialized, singleton will load remaining functions here
// Or you can do vpLoadInstance here, if you already created instance by yourself
vpCreateInstance(&vpInstanceCreateInfo, nullptr, &instance);

[GrinlexGH]

You can also remove the public vpLoadGlobalFunc and load functions dynamically in vpInitialize or vpCreateCapabilities if the user passed gipa to pVulkanFunctions in create info

[christophe-lunarg]

Agreed with removing _STATIC/_DYNAMIC_BIT.

Regarding "I think a table for instance -> VpCapabilities object would be an unnecessarily complex functionality" I was thinking about this as an implementation detail of a VpInstance object, removing the VpCapabilities object.

I'll try some things...

[christophe-lunarg]

Regarding "but how to dynamically initialize a singleton in this case?"

The VpCapabilities was introduced to support custom function pointers and the singleton was introduced for API backward compatibility.

Once stable, I would like to simplify all this and only have the VP_USE_OBJECT code path, deprecate the other code path and them remove it.

So I suggest to support the dynamic mode only with the VP_USE_OBJECT code path.

What do you think of this?

[GrinlexGH]

dynamic loading support only for VP_USE_OBJECT sounds logical, considering that the old code was actually compiled only for static linking

Old code with singleton will use implicit static initialization, if static functions are not available (VK_NO_PROTOTYPES is defined and VP_USE_OBJECT is not defined), code can produce explicit #error macro (or left the user with function nullptr dereference)

we can also rename VpCapabilities to VpLoader or VpDispatcher (dispatcher is already used in vulkan.hpp), so we can avoid name collisions

I think the final version could look something like this:

VkInstance instance{};
VpDispatcher dispatcher{};

if (wantDynamicLoading) {
    VpVulkanFunctions vulkanFunctions{};
    vulkanFunctions.GetInstanceProcAddr = reinterpret_cast<PFN_vkGetInstanceProcAddr>(SDL_Vulkan_GetVkGetInstanceProcAddr());

    VpDispatcherCreateInfo dispatcherCreateInfo{};
    dispatcherCreateInfo.pVulkanFunctions = vulkanFunctions;

    // If pVulkanFunctions->GetInstanceProcAddr is not null, it will get global functions here via GetInstanceProcAddr, if it fails, return an error
    vpCreateDispatcher(&dispatcherCreateInfo, nullptr, &dispatcher);

    if (wantCreateInstanceByMyCode) {
        instance = create_instance_by_my_code();
        vpLoadInstance(dispatcher, instance, VP_LOAD_INSTANCE_HAS_GPDP2_BIT);
    } else {
        ...
        // If created with GetInstanceProcAddr (we can add internal bool flag for it), it will try to get rest of the functions via gipa, if it fails, return the valid VkInstance, left instance deletion to the user, and return an error
        vpCreateInstance(capabilities, ..., &instance);
    }

    vpDestroyDispatcher(nullptr, &dispatcher);
} else if (wantStaticLoading) {
    VpVulkanFunctions vulkanFunctions{};

    VpDispatcherCreateInfo dispatcherCreateInfo{};
    dispatcherCreateInfo.pVulkanFunctions = vulkanFunctions;
    // or
    dispatcherCreateInfo.pVulkanFunctions = nullptr;

    // if VK_NO_PROTOTYPES is defined, return an error
    vpCreateDispatcher(&dispatcherCreateInfo, nullptr, &dispatcher);
    
    if (wantCreateInstanceByMyCode) {
        // Will not do anything
        instance = create_instance_by_my_code();
        vpLoadInstance(dispatcher, instance, VP_LOAD_INSTANCE_HAS_GPDP2_BIT);
    } else {
        ...
        // Also will not load any functions
        vpCreateInstance(capabilities, ..., &instance);
    }
    
    vpDestroyDispatcher(nullptr, &dispatcher);
}

[GrinlexGH]

I would push the changes with renamed functions and structures, but I do not know how to maintain backward compatibility. Wait for vulkan 1.5?

[christophe-lunarg]

Here is the design I am suggesting: #926

There are still some issues that I didn't had time to fix, mocked tests not passing because the static functions are used instead of the Mock functions I believe.

I'll continue tomorrow but feel free to give me your feedback.

[christophe-lunarg]

As stated here: https://github.com/KhronosGroup/MoltenVK#using-the-vulkan-sdk, on macOS you have to create an instance with VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR, if this fix doesn't work, you can disable this test for macOS

Ah yes very good point. I ran again the internal C.I. tests.

[ci-tester-lunarg]

Author GrinlexGH not on autobuild list. Waiting for curator authorization before starting CI build.

[ci-tester-lunarg]

Author GrinlexGH not on autobuild list. Waiting for curator authorization before starting CI build.

[christophe-lunarg]

As stated here: https://github.com/KhronosGroup/MoltenVK#using-the-vulkan-sdk, on macOS you have to create an instance with VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR, if this fix doesn't work, you can disable this test for macOS

The internal tests are fixed

[christophe-lunarg]

Closing this issue which is resolved with #926

Thanks @GrinlexGH for your contribution !