All committers have signed the CLA.

Also changing destStageFlags = VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT; to destStageFlags = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT;

We used to use VK_PIPELINE_STAGE_ALL_COMMANDS_BIT but ran into a validation error in an update to the Vulkan SDK. This flags a genuine validation error. See issue #1092 and PR #1148 for details. What version of the Vulkan SDK are you using and are you testing with validation on?

I'm using 1.4.313.2 and yes, around Vulkan code changes the validation layer is turned on to ensure that it's clean. I'll upgrade and try again. The solution really might be as simple as ORing with VK_PIPELINE_STAGE_TRANSFER_BIT.

I'm using 1.4.313.2

The improved validation was post 1.4.313. The reporter of issue #1092 was using 1.4.335.

I'm using 1.4.313.2

The improved validation was post 1.4.313. The reporter of issue #1092 was using 1.4.335.

You were right. This turned out to be unnecessary and I've reverted the change related to it. I realized I could just set the initial layout to TRANSFER_DST via the UploadEx() call and that would automatically line it up for access on the transfer queue. Subsequently, it could just be transitioned towards SHADER_READ_ONLY during the QFOT. The net result of all of this being illustrated here: toomuchvoltage/HighOmega-public@3d27a36

Hi @MarkCallow I just added 82a5bad to demonstrate sample usage of the guarded callbacks. Truth is, it won't stress test the feature nor is that really feasible with the current single-texture test cases. Even if there were test cases requiring multiple textures, the guards within would need to be used application-wide where ever applicable. (i.e. if the graphics queue is creating textures, that would mean re-use for all accesses to graphics queue. Or any arena VkDeviceMemory accesses globally.)

If you feel like this is unnecessary, I can revert. Eager to hear back.

If you feel like this is unnecessary, I can revert. Eager to hear back.

It is great to have a test even if it is not a stress test.

I would love to have non-interactive tests of the uploaders, maybe using gtest like texturetests, but I have no idea how to run such tests on GHA CI runners. Do they headless Vulkan or OpenGL graphics?

I will properly review this PR early next week. Please be aware that I will not merge this until v5.0.0 has been released. I can't give a date for that at present.

Hi @MarkCallow , just circling back on this. It's perfectly fine if this goes out post-5.0.0. Truth is these are on-the-field improvements resulting from a commercial game on Steam shipped with LibKTX2. I'm hesitant to link it since I personally wouldn't feel comfortable with the self promotion here, but of course figuring out the title is trivial given my handle. And I personally do not have experience with GHA CI, but I suspect paid plans (which this should be?) should have no issues with GPU'd instances.

Hi @MarkCallow , appreciate the feedback. The guarded memory callbacks are absolutely necessary. That said, I may have done a more heavy handed version than is necessary with VMA since I used my own pattern from my engine (which obviously needs to be more explicit): https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L313-L474

VMA's allocation, image/buffer bind and free calls are all thread-safe. However, mapping and unmapping calls are not. They only check to ensure that no VkDeviceMemory arena is mapped twice. But no guarantees if you try to map the same one from separate threads (to the best of my understanding). The memory guard is also necessary when accessing AllocMemCWrapperDirectory, irrespective of whether VMA is used or not. I will be pushing an updated unit test addressing these.

For an expert pair of eyes, I would solicit Adam Sawicki's advice. He is the original author of VMA. His handle is @sawickiap on GitHub. I'm confident he's within reach for Khronos.

All done @MarkCallow , ready for another pair of eyes.

All done @MarkCallow , ready for another pair of eyes.

Thanks. Working on finding a reviewer.

One of my Khronos colleagues asked codex to analyze this. This is what it said.

• [P1] Queue locking does not make a shared ktxVulkanDeviceInfo thread-safe. The mutex is acquired only around vkQueueSubmit (

  KTX-Software/lib/src/vkloader.c

  Lines 1317 to 1320 in 339c047

  	if (useQueueMutex) queueMutexCallbacks->queueLockFuncPtr();
  	VK_CHECK_RESULT(
  	vdi->vkFuncs.vkQueueSubmit(vdi->queue, 1, &submitInfo, copyFence));
  	if (useQueueMutex) queueMutexCallbacks->queueUnlockFuncPtr();

  ), but every upload begins and records into the single vdi->cmdBuffer (

  KTX-Software/lib/src/vkloader.c

  Lines 1027 to 1030 in 339c047

  	VK_CHECK_RESULT(
  	vdi->vkFuncs.vkBeginCommandBuffer(vdi->cmdBuffer, &cmdBufBeginInfo)
  	);

  ). Concurrent calls sharing a vdi therefore concurrently access the same Vulkan command buffer, violating Vulkan’s external-synchronization requirements. Either use per-call/per-thread command buffers and command pools, or explicitly require each concurrent caller to supply a thread-confined vdi and command pool. Until then, the documentation’s “fully thread-safe (https://github.com/KhronosGroup/KTX-Software/blob/339c04731e38806a288caf7ca4820accdfd08af6/lib/include/ktxvulkan.h#L247-L251)” claim is unsafe.
  • [P2] Queue callbacks need state and preferably the queue handle. The callbacks take no arguments (

    KTX-Software/lib/include/ktxvulkan.h

    Lines 137 to 138 in 339c047

    	typedef void (*ktxVulkanTexture_queueLockFuncPtr)();
    	typedef void (*ktxVulkanTexture_queueUnlockFuncPtr)();

    ), so a C application cannot associate a mutex with a particular VkQueue except through global state. A global mutex is correct but unnecessarily serializes independent queues, undermining the optimization this API introduces. I’d add void* userData to the callback struct and invoke callbacks with that and/or vdi->queue.
  • [P2] The “guarded” sample allocator contains a data race. mt64() is called before acquiring memoryAccessGuard (

    KTX-Software/tests/loadtests/vkloadtests/VulkanLoadTestSample.cpp

    Lines 136 to 164 in 339c047

    	std::mutex memoryAccessGuard;
    	uint64_t AllocMemCWrapperGuarded(VkMemoryAllocateInfo* allocInfo, VkMemoryRequirements* memReq, uint64_t* numPages)
    	{
    	uint64_t allocId = mt64();
    	VmaAllocationCreateInfo pCreateInfo = {};
    	if ((cachedDevMemProps.memoryTypes[allocInfo->memoryTypeIndex].propertyFlags & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT) ||
    	(cachedDevMemProps.memoryTypes[allocInfo->memoryTypeIndex].propertyFlags & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT))
    	{
    	pCreateInfo.usage = VMA_MEMORY_USAGE_CPU_TO_GPU;
    	pCreateInfo.flags = VMA_ALLOCATION_CREATE_HOST_ACCESS_SEQUENTIAL_WRITE_BIT;
    	}
    	else
    	{
    	pCreateInfo.usage = VMA_MEMORY_USAGE_GPU_ONLY;
    	}
    	pCreateInfo.memoryTypeBits = memReq->memoryTypeBits;
    	VmaAllocation allocation;
    	VkResult result = vmaAllocateMemory(vmaAllocator, memReq, &pCreateInfo, &allocation, VMA_NULL);
    	if (result != VK_SUCCESS)
    	{
    	return 0ull;
    	}
    	{
    	std::lock_guard<std::mutex> lk(memoryAccessGuard);
    	AllocMemCWrapperDirectory[allocId].allocation = allocation;
    	AllocMemCWrapperDirectory[allocId].mapSize = memReq->size;
    	}

    ). std::mt19937_64 mutates internal state, so concurrent guarded allocations have undefined behavior and can produce corrupt or duplicate IDs. Generate the ID under the mutex, or preferably use a nonzero atomic counter.
  • [P2] The new load-error handling still performs incomplete cleanup. On ktxTexture_LoadImageData failure (

    KTX-Software/lib/src/vkloader.c

    Lines 1170 to 1180 in 339c047

    	kResult = ktxTexture_LoadImageData(This,
    	pMappedStagingBuffer,
    	(ktx_size_t)memAllocInfo.allocationSize);
    	if (kResult != KTX_SUCCESS)
    	{
    	if (!useSuballocator)
    	vdi->vkFuncs.vkUnmapMemory(vdi->device, stagingMemory);
    	else
    	subAllocatorCallbacks->memoryUnmapFuncPtr(stagingAllocId, 0ull);
    	return kResult;
    	}

    ), the code unmaps and returns but leaks copyRegions, the staging buffer, and its allocation; it also leaves the command buffer recording. This needs a common cleanup path.

  Coverage note: the new “[threaded]” sample creates one worker and immediately joins it, so no accesses ever overlap (

  KTX-Software/tests/loadtests/vkloadtests/Texture.cpp

  Lines 128 to 140 in 339c047

  	if (useQueueGuard == UseQueueGuard::Yes)
  	{
  	std::thread uploaderThread([&]() {
  	ktxresult = ktxTexture_VkUploadEx_WithSuballocatorAndQueueGuard(kTexture, &vdi, &texture,
  	static_cast<VkImageTiling>(tiling),
  	VK_IMAGE_USAGE_SAMPLED_BIT,
  	VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL,
  	&subAllocatorCallbacksGuarded, &queueGuardCallbacks);
  	});
  	uploaderThread.join(); // We really have no choice here. This crucible/test-environment only loads one texture at a time.
  	// Additionally `QUEUE_GUARD_CALLBACKS::queueAccessGuard` and `QUEUE_GUARD_CALLBACKS::memoryAccessGuard` would
  	// need to be re-used when arena `VkDeviceMemory`s or the same `VkQueue`s are being accessed simultaneously.
  	}

  ). It cannot detect the command-buffer or RNG races above. A deterministic two-caller test—using separate thread-confined VDIs if that is the intended contract—would be much more valuable.

@toomuchvoltage you have already pointed out the last item. It sounds like we need to look at the vdi queue handling. What do you think?

Hi, I'm the developer of the VMA library. I'm sorry for the delayed response.

Mapping in VMA is thread-safe.

About raw Vulkan (functions vkMapMemory, vkUnmapMemory), you are right:

• Mapping a single VkDeviceMemory block multiple times (whether the same or disjoint memory regions) is illegal.
• It is not thread-safe.

However, using the recommended library functions vmaMapMemory, vmaUnmapMemory (or other convenient ways like VMA_ALLOCATION_CREATE_MAPPED_BIT flag or vmaCopyMemoryToAllocation function):

• They are thread-safe, synchronized internally (unless you specified VMA_ALLOCATOR_CREATE_EXTERNALLY_SYNCHRONIZED_BIT).
• You can map the same allocation or different allocations coming from the same VkDeviceMemory block multiple times (mapping is reference-counted internally).
• You can safely map different allocations coming from the same VkDeviceMemory block because VMA internally maps the entire memory block, not just the region of that one allocation.

For more information, see this documentation chapter:
https://gpuopen-librariesandsdks.github.io/VulkanMemoryAllocator/html/memory_mapping.html

Thanks @sawickiap. @toomuchvoltage do you have any comments on this or the codex review?

Hi @MarkCallow @sawickiap , deeply appreciate the reviews. Fantastic to be informed about vmaMapMemory and vmaUnmapMemory being thread-safe.

On the P1 issue: once again, since I was applying the learning from my own engine, I simply brought the assumptions as well for general use. The point is absolutely correct and my engine has a thread-safe cache for per-thread KTX VDIs.

Thread-safe objects:
https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L52-L53

Creation:
https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4510-L4519

Destruction:
https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4166-L4172

I guess this becoming the general usage pattern was implicit in my assumptions. We can ask for assurances on this in the documentation.

On the first P2 issue: my engine effectively has 1 copy queue, 1 transfer queue and 1 graphics queue. Is it safe to assume that this is a globally recommended pattern? Is it safe or performant for other engines to have multiple transfer queues for example? If so, we can simply pass a typed VkQueue to the function where there's no ambiguity about what is we want guarded.

On the second P2 issue: Very fine point regarding mt64(). I'll address that.

On the third P2 issue: Good point, I guess this really falls under an overall discovery in this process that can be addressed in this PR.

I'll get another commit together to address these.

Hi @MarkCallow , I just pushed a commit addressing the raised issues. Note that I finally decided to pass a VkQueue handle to the queue guard callbacks. This makes their roles much more explicit. Additionally, from what I gather online not all IHVs support multiple queues per family. And in practice it may not yield a performance benefit in all cases. (Knowing this is naturally compatible with the design we have.) It would be interesting to verify this with Khronos's perhaps broader overview of what is actually available in the wild.

Perhaps https://vulkan.gpuinfo.org can answer your questions about support for multiple queues per family.

In one of your commit messages you write that a per-thread VDI is necessary. If that is the case does it not make sense to pass the device from that to the allocator callbacks, similar to what was requested in issue #1212? Doing so would require changing the signatures of the allocator callback functions. I wonder if there is any way to do that without having to create another set of *_VkUploadEx_WithSubAllocator functions.

In one of your commit messages you write that a per-thread VDI is necessary. If that is the case does it no make sense to pass the device from that to the allocator callbacks, similar to what was requested in issue #1212? Doing so would require changing the signatures of the allocator callback functions. I wonder if there is any way to do that without having to create another set of *_VkUploadEx_WithSubAllocator functions.

Hi @MarkCallow, no that is not necessary. ktxTexture_VkUploadEx(), ktxTexture_VkUploadEx_WithSuballocator() and ktxTexture_VkUploadEx_WithSuballocatorAndQueueGuard() all readily take in a KTX VDI (second parameter). In fact the quoted snippets demonstrate how a per thread KTX VDI is provided in a thread-safe manner:

Thread-safe objects: https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L52-L53

Creation: https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4510-L4519

Destruction: https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4166-L4172

Each ktx2VDIPools.dir[ThreadID].elem (with ThreadID being thread_local and generated with mt64()) is a per-thread KTX VDI supplying its own command pool and command buffer to UploadEx(). In fact these elements are reference counted via ktx2VDIPools.dir[ThreadID].elemCount perhaps much in the same manner and spirit that VMA handles maps/unmaps internally. Once elemCount goes to zero, the KTX VDI is deemed unnecessary and destroyed (i.e. all last images created with this VDI are gone). These operations are synchronized via ktx2VDIPools.mtx. My own status quo usage issues with mt64() not-withstanding, this is effectively sufficient for our UploadEx() set of calls to complete their tasks efficiently and in a thread-safe manner. This solution is currently shipped and tested in the video game I screenshotted in the original post (coupled with performance metrics and the environment specs). It shares the same core engine with the repo posted above.

Ultimately, what is being proposed in #1212 breaks separation of concerns. A type erased void * being passed in would leave the actual implementation guessing as to what it even means in the first place. I obviously welcome the original author of 1212 to fork and create their own interface and implementation. However, I would be very hesitant to modify the standard version advertised globally taking in nebulous type-erased parameters with no well defined purpose.

I will proceed to prepare a commit for exhaustive clean-ups in all failure cases of UploadEx() shortly.

All done @MarkCallow . A quick review of the specification for freeUploadResources() would be appreciated to see if it is in line with other specifications. Note that it is used to clean-up mappableMemory and the texture's main allocationId rather than any staging resources in the linear tiling case (as staging resources do not seem to apply there).

EDIT: Force pushed to get the checks running again. There was a test infrastructure failure and I wanted to make sure I'm not introducing new issues with this commit.

Hi @MarkCallow , I thought I needed to address the point a bit more directly in the interest of the library. Sorry if we're already discussed this a fair bit.

If that is the case does it not make sense to pass the device from that to the allocator callbacks, similar to what was requested in issue #1212?

The callbacks that are designed at the moment -- dating back to the original suballocator PR -- are shims around very specific operations that are either doable directly in Vulkan (with some sophistication) or have VMA exposed calls (such as the ones you're seeing in the tests: alloc, bind-image/buffer, map/unmap, free). Basic usage for VMA has none of that. It's basically just vmaCreateBuffer(), vmaCreateImage(). See: https://gpuopen-librariesandsdks.github.io/VulkanMemoryAllocator/html/quick_start.html . I'm willing to bet it was Adam's foresight that resulted in the exposed calls that we're featuring in our samples today. Or possibly industry feedback.

Resultingly, the parameters that were chosen for those callbacks are to enable those operations. Nothing more. They should -- at least as far as the standard presented to world -- not be designed to be a vehicle for brokering external data. Allocation IDs and page counts suffice. Basic Vulkan types coupled with those are sufficient for those calls to conclude their tasks effectively.

Amending new links since a recent push shifted line numbers:

Thread-safe objects: https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L53-L53

Creation: https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4566-L4575

Destruction: https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4231-L4237

EDIT: they shifted again due to incorporating all feedback from this PR.

Hi again @MarkCallow , hope all is well. Just circling back to see if there's additional feedback. I actually pushed a commit to my own repo that now incorporates all feedback from this PR: toomuchvoltage/HighOmega-public@085bc1d (both thread-safe mt64() and VMA-style thread-safe maps/unmaps are demonstrated, the latter now makes failures between maps/unmaps inside UploadEx() much less dangerous).

Eager to take note of any further potential refinement suggestions.

I've been distracted by other work. I'm sorry for the delay.

A quick review of the specification for freeUploadResources() would be appreciated to see if it is in line with other specifications.

I don't think we have any other similar functions. It seems fine though for some of the calls it does nothing.

If that is the case does it not make sense to pass the device from that to the allocator callbacks, similar to what was requested in issue #1212?

Note that I was not proposing a "type-erased" parameter. I was proposing passing a VkDevice. You have demonstrated it is not necessary so we will leave things as they are.

Note that I was not proposing a "type-erased" parameter. I was proposing passing a VkDevice. You have demonstrated it is not necessary so we will leave things as they are.

Truth is there's a probably a slight bit of incomplete abstraction with the current callbacks around VkDevice. I wasn't thinking of a multi-GPU environment. That said, simply making the following thread_local satisfies the ability to upload different textures to different devices under the same Vulkan instance.

https://github.com/toomuchvoltage/HighOmega-public/blob/master/HighOmega/src/gl.cpp#L318

The reality is that the decision of the target device will be made around to which workload the texture belongs. That in all likelihood will happen long before UploadEx() even affecting which VDI to use since the image and the backing memory need to be on the same device as per the spec. That decision point can change deviceCached for the thread and since the callbacks will happen on the same thread, the right value will be visible to them.

Hi @MarkCallow , just responded to the comments. Would appreciate further guidance.

Hi @MarkCallow just committed a change based on the documentation feedback. I still haven't fully read up on #1224 but it's very intriguing at a cursory glance.

The new comments are good. Thanks.

Unfortunately I am still not in a position to be able to merge this to main. As soon as v5.0.0 is released I will.

Thank you @MarkCallow . Totally understandable, no rush.