From f2c5c1e49116a0b60eb144a5503803b7b77df055 Mon Sep 17 00:00:00 2001 From: AWoloszyn Date: Tue, 30 Oct 2018 15:27:51 -0400 Subject: [PATCH] Stop aborting on some handle failures. Actually guard in some cases with handle failures, this is especially important in calls do doVkCmd*. If we don't handle this then we can desynchronize queue submissions. --- gapis/api/vulkan/api/buffer.api | 17 +- gapis/api/vulkan/api/image.api | 168 +++++++++--------- .../api/properties_features_requirements.api | 11 +- gapis/api/vulkan/api/queue.api | 18 +- gapis/api/vulkan/api/synchronization.api | 18 +- gapis/api/vulkan/errors.api | 4 +- gapis/api/vulkan/extensions/khr_swapchain.api | 7 +- 7 files changed, 127 insertions(+), 116 deletions(-) diff --git a/gapis/api/vulkan/api/buffer.api b/gapis/api/vulkan/api/buffer.api index 5edadb5cca..00b850609b 100644 --- a/gapis/api/vulkan/api/buffer.api +++ b/gapis/api/vulkan/api/buffer.api @@ -129,15 +129,16 @@ cmd void vkDestroyBuffer( VkDevice device, VkBuffer buffer, AllocationCallbacks pAllocator) { - if !(device in Devices) { vkErrorInvalidDevice(device) } - if (buffer != as!VkBuffer(0)) { - bufferObject := Buffers[buffer] - if (bufferObject.Memory != null) { - // If the memory is deleted first, then do not try to remove ourselves. - delete(bufferObject.Memory.BoundObjects, - as!u64(buffer)) + if !(device in Devices) { vkErrorInvalidDevice(device) } else { + if (buffer != as!VkBuffer(0)) { + bufferObject := Buffers[buffer] + if (bufferObject.Memory != null) { + // If the memory is deleted first, then do not try to remove ourselves. + delete(bufferObject.Memory.BoundObjects, + as!u64(buffer)) + } + delete(Buffers, buffer) } - delete(Buffers, buffer) } } diff --git a/gapis/api/vulkan/api/image.api b/gapis/api/vulkan/api/image.api index 2a1410f00b..d2a61ec81e 100644 --- a/gapis/api/vulkan/api/image.api +++ b/gapis/api/vulkan/api/image.api @@ -284,25 +284,26 @@ cmd void vkDestroyImage( VkImage image, AllocationCallbacks pAllocator) { if !(device in Devices) { vkErrorInvalidDevice(device) } - if !(image in Images) { vkErrorInvalidImage(image) } - if (image != as!VkImage(0)) { - imageObject := Images[image] - if (imageObject.BoundMemory != null) { - // If the memory is deleted first, then do not try to remove ourselves. - delete(imageObject.BoundMemory.BoundObjects, as!u64(image)) - } - delete(Images, image) - for _ , _ , v in ImageViews { - if v.Image != null { - if v.Image.VulkanHandle == image { - v.Image = null + if !(image in Images) { vkErrorInvalidImage(image) } else { + if (image != as!VkImage(0)) { + imageObject := Images[image] + if (imageObject.BoundMemory != null) { + // If the memory is deleted first, then do not try to remove ourselves. + delete(imageObject.BoundMemory.BoundObjects, as!u64(image)) + } + delete(Images, image) + for _ , _ , v in ImageViews { + if v.Image != null { + if v.Image.VulkanHandle == image { + v.Image = null + } } } - } - for i in (0 .. LastPresentInfo.PresentImageCount) { - if (LastPresentInfo.PresentImages[i] != null) { - if (LastPresentInfo.PresentImages[i].VulkanHandle == image) { - LastPresentInfo.PresentImages[i] = null + for i in (0 .. LastPresentInfo.PresentImageCount) { + if (LastPresentInfo.PresentImages[i] != null) { + if (LastPresentInfo.PresentImages[i].VulkanHandle == image) { + LastPresentInfo.PresentImages[i] = null + } } } } @@ -330,60 +331,61 @@ cmd VkResult vkBindImageMemory( VkDeviceSize memoryOffset) { if !(device in Devices) { vkErrorInvalidDevice(device) } if !(memory in DeviceMemories) { vkErrorInvalidDeviceMemory(memory) } - if !(image in Images) { vkErrorInvalidImage(image) } - imageObject := Images[image] - imageObject.BoundMemory = DeviceMemories[memory] - imageObject.BoundMemoryOffset = memoryOffset - DeviceMemories[memory].BoundObjects[as!u64(image)] = memoryOffset - - for _ , _ , aspectBit in unpackImageAspectFlags(imageObject.ImageAspect) { - aspect := imageObject.Aspects[aspectBit] - for j in (0 .. imageObject.Info.ArrayLayers) { - for i in (0 .. imageObject.Info.MipLevels) { - level := aspect.Layers[j].Levels[i] - elementAndTexelBlockSize := getElementAndTexelBlockSize(imageObject.Info.Format) - depthElementSize := getDepthElementSize(imageObject.Info.Format, false) - // Roundup the width and height in the number of blocks. - widthInBlocks := roundUpTo(level.Width, elementAndTexelBlockSize.TexelBlockSize.Width) - heightInBlocks := roundUpTo(level.Height, elementAndTexelBlockSize.TexelBlockSize.Height) - elementSize := switch (aspectBit) { - case VK_IMAGE_ASPECT_COLOR_BIT: - elementAndTexelBlockSize.ElementSize - case VK_IMAGE_ASPECT_DEPTH_BIT: - depthElementSize - case VK_IMAGE_ASPECT_STENCIL_BIT: - // stencil element is always 1 byte wide - as!u32(1) - } - tightlyPackedSize := widthInBlocks * heightInBlocks * level.Depth * elementSize - - // If the image has LINEAR tiling and the image level has layout - // PREINITIALIZED and size larger than our calculated tightly packed - // size, link the data back to the bound device memory. Otherwise - // creates its own shadow memory pool. - // TODO: If the image as a whole requires more memory than we - // calculated, we should link the data back to the bound device memory - // no matter whether the tiling is LINEAR or OPTIMAL. But we need to - // come up with a 'linear layout' in GAPID. - if (imageObject.Info.Tiling == VK_IMAGE_TILING_LINEAR) && - (level.Layout == VK_IMAGE_LAYOUT_PREINITIALIZED) && - (level.LinearLayout != null) && - (as!u64(level.LinearLayout.size) > as!u64(tightlyPackedSize)) { - loffset := as!u64(memoryOffset + level.LinearLayout.offset) - lsize := as!u64(level.LinearLayout.size) - level.Data = imageObject.BoundMemory.Data[loffset:loffset + lsize] - } else { - level.Data = make!u8(tightlyPackedSize) + if !(image in Images) { vkErrorInvalidImage(image) } else { + imageObject := Images[image] + imageObject.BoundMemory = DeviceMemories[memory] + imageObject.BoundMemoryOffset = memoryOffset + DeviceMemories[memory].BoundObjects[as!u64(image)] = memoryOffset + + for _ , _ , aspectBit in unpackImageAspectFlags(imageObject.ImageAspect) { + aspect := imageObject.Aspects[aspectBit] + for j in (0 .. imageObject.Info.ArrayLayers) { + for i in (0 .. imageObject.Info.MipLevels) { + level := aspect.Layers[j].Levels[i] + elementAndTexelBlockSize := getElementAndTexelBlockSize(imageObject.Info.Format) + depthElementSize := getDepthElementSize(imageObject.Info.Format, false) + // Roundup the width and height in the number of blocks. + widthInBlocks := roundUpTo(level.Width, elementAndTexelBlockSize.TexelBlockSize.Width) + heightInBlocks := roundUpTo(level.Height, elementAndTexelBlockSize.TexelBlockSize.Height) + elementSize := switch (aspectBit) { + case VK_IMAGE_ASPECT_COLOR_BIT: + elementAndTexelBlockSize.ElementSize + case VK_IMAGE_ASPECT_DEPTH_BIT: + depthElementSize + case VK_IMAGE_ASPECT_STENCIL_BIT: + // stencil element is always 1 byte wide + as!u32(1) + } + tightlyPackedSize := widthInBlocks * heightInBlocks * level.Depth * elementSize + + // If the image has LINEAR tiling and the image level has layout + // PREINITIALIZED and size larger than our calculated tightly packed + // size, link the data back to the bound device memory. Otherwise + // creates its own shadow memory pool. + // TODO: If the image as a whole requires more memory than we + // calculated, we should link the data back to the bound device memory + // no matter whether the tiling is LINEAR or OPTIMAL. But we need to + // come up with a 'linear layout' in GAPID. + if (imageObject.Info.Tiling == VK_IMAGE_TILING_LINEAR) && + (level.Layout == VK_IMAGE_LAYOUT_PREINITIALIZED) && + (level.LinearLayout != null) && + (as!u64(level.LinearLayout.size) > as!u64(tightlyPackedSize)) { + loffset := as!u64(memoryOffset + level.LinearLayout.offset) + lsize := as!u64(level.LinearLayout.size) + level.Data = imageObject.BoundMemory.Data[loffset:loffset + lsize] + } else { + level.Data = make!u8(tightlyPackedSize) + } } } } - } - if (Images[image].Info.DedicatedAllocationNV != null) && (DeviceMemories[memory].DedicatedAllocationNV == null) { - vkErrorExpectNVDedicatedlyAllocatedHandle("VkImage", as!u64(image)) - } - if (Images[image].Info.DedicatedAllocationNV == null) && (DeviceMemories[memory].DedicatedAllocationNV != null) { - vkErrorExpectNVDedicatedlyAllocatedHandle("VkDeviceMemory", as!u64(memory)) + if (Images[image].Info.DedicatedAllocationNV != null) && (DeviceMemories[memory].DedicatedAllocationNV == null) { + vkErrorExpectNVDedicatedlyAllocatedHandle("VkImage", as!u64(image)) + } + if (Images[image].Info.DedicatedAllocationNV == null) && (DeviceMemories[memory].DedicatedAllocationNV != null) { + vkErrorExpectNVDedicatedlyAllocatedHandle("VkDeviceMemory", as!u64(memory)) + } } return ? } @@ -419,23 +421,23 @@ cmd VkResult vkCreateImageView( image_view_create_info := pCreateInfo[0] handle := ? - if !(image_view_create_info.image in Images) { vkErrorInvalidImage(image_view_create_info.image) } - imageViewObject := new!ImageViewObject(Device: device, - VulkanHandle: handle, - Image: Images[image_view_create_info.image], - Type: image_view_create_info.viewType, - Format: image_view_create_info.format, - Components: image_view_create_info.components, - SubresourceRange: image_view_create_info.subresourceRange - ) - if ((0xFFFFFFFF - as!u32(imageViewObject.Image.ImageAspect)) & as!u32(image_view_create_info.subresourceRange.aspectMask)) != 0 { - vkErrorInvalidImageAspect(imageViewObject.Image.VulkanHandle, - as!VkImageAspectFlagBits(image_view_create_info.subresourceRange.aspectMask)) + if !(image_view_create_info.image in Images) { vkErrorInvalidImage(image_view_create_info.image) } else { + imageViewObject := new!ImageViewObject(Device: device, + VulkanHandle: handle, + Image: Images[image_view_create_info.image], + Type: image_view_create_info.viewType, + Format: image_view_create_info.format, + Components: image_view_create_info.components, + SubresourceRange: image_view_create_info.subresourceRange + ) + if ((0xFFFFFFFF - as!u32(imageViewObject.Image.ImageAspect)) & as!u32(image_view_create_info.subresourceRange.aspectMask)) != 0 { + vkErrorInvalidImageAspect(imageViewObject.Image.VulkanHandle, + as!VkImageAspectFlagBits(image_view_create_info.subresourceRange.aspectMask)) + } + if pView == null { vkErrorNullPointer("VkImageView") } + pView[0] = handle + ImageViews[handle] = imageViewObject } - if pView == null { vkErrorNullPointer("VkImageView") } - pView[0] = handle - ImageViews[handle] = imageViewObject - return ? } diff --git a/gapis/api/vulkan/api/properties_features_requirements.api b/gapis/api/vulkan/api/properties_features_requirements.api index b359eaadbd..c53e2b8713 100644 --- a/gapis/api/vulkan/api/properties_features_requirements.api +++ b/gapis/api/vulkan/api/properties_features_requirements.api @@ -269,11 +269,12 @@ cmd void vkGetImageMemoryRequirements( VkImage image, VkMemoryRequirements* pMemoryRequirements) { if !(device in Devices) { vkErrorInvalidDevice(device) } - if !(image in Images) { vkErrorInvalidImage(image) } - requirements := ? - if pMemoryRequirements == null { vkErrorNullPointer("VkMemoryRequirements") } - pMemoryRequirements[0] = requirements - Images[image].MemoryRequirements = requirements + if !(image in Images) { vkErrorInvalidImage(image) } else { + requirements := ? + if pMemoryRequirements == null { vkErrorNullPointer("VkMemoryRequirements") } + pMemoryRequirements[0] = requirements + Images[image].MemoryRequirements = requirements + } } @indirect("VkDevice") diff --git a/gapis/api/vulkan/api/queue.api b/gapis/api/vulkan/api/queue.api index 00fdeaaf68..7b32e6f31d 100644 --- a/gapis/api/vulkan/api/queue.api +++ b/gapis/api/vulkan/api/queue.api @@ -175,10 +175,11 @@ cmd VkResult vkQueueSubmit( } leaveSubcontext() if (fence != 0) { // 'fence' parameter, unrelated to 'fence' keyword below - if (Fences[fence].Signaled) { vkErrorInvalidFence(fence) } - LastBoundQueue.PendingCommands[len(LastBoundQueue.PendingCommands)] - = new!CommandReference(as!VkCommandBuffer(0), 0, cmd_vkNoCommand, 0, None, - as!VkSemaphore(0), null, fence) + if (Fences[fence].Signaled) { vkErrorInvalidFence(fence) } else { + LastBoundQueue.PendingCommands[len(LastBoundQueue.PendingCommands)] + = new!CommandReference(as!VkCommandBuffer(0), 0, cmd_vkNoCommand, 0, None, + as!VkSemaphore(0), null, fence) + } } execPendingCommands(queue, true) @@ -333,10 +334,11 @@ cmd VkResult vkQueueBindSparse( leaveSubcontext() if (fence != 0) { // 'fence' parameter, unrelated to 'fence' keyword below - if (Fences[fence].Signaled) { vkErrorInvalidFence(fence) } - LastBoundQueue.PendingCommands[len(LastBoundQueue.PendingCommands)] - = new!CommandReference(as!VkCommandBuffer(0), 0, cmd_vkNoCommand, 0, None, - as!VkSemaphore(0), null, fence) + if (Fences[fence].Signaled) { vkErrorInvalidFence(fence) } else { + LastBoundQueue.PendingCommands[len(LastBoundQueue.PendingCommands)] + = new!CommandReference(as!VkCommandBuffer(0), 0, cmd_vkNoCommand, 0, None, + as!VkSemaphore(0), null, fence) + } } execPendingCommands(queue, true) diff --git a/gapis/api/vulkan/api/synchronization.api b/gapis/api/vulkan/api/synchronization.api index e248c15837..d540c9a41f 100644 --- a/gapis/api/vulkan/api/synchronization.api +++ b/gapis/api/vulkan/api/synchronization.api @@ -355,9 +355,10 @@ sub void dovkCmdWaitEvents(ref!vkCmdWaitEventsArgs args) { } if len(LastBoundQueue.PendingEvents) == 0 { for _ , _ , b in args.ImageMemoryBarriers { - if !(b.image in Images) { vkErrorInvalidImage(b.image) } - image := Images[b.image] + if !(b.image in Images) { vkErrorInvalidImage(b.image) } else { + image := Images[b.image] transitionImageLayout(image, b.subresourceRange, b.oldLayout, b.newLayout) + } } } } @@ -421,12 +422,13 @@ cmd void vkCmdWaitEvents( sub void dovkCmdPipelineBarrier(ref!vkCmdPipelineBarrierArgs args) { for _ , _ , v in args.ImageMemoryBarriers { - if !(v.image in Images) { vkErrorInvalidImage(v.image) } - image := Images[v.image] - transitionImageLayout(image, v.subresourceRange, v.oldLayout, v.newLayout) - if v.oldLayout == VK_IMAGE_LAYOUT_UNDEFINED { - writeImageSubresource(image, v.subresourceRange) - updateImageQueue(image, v.subresourceRange) + if !(v.image in Images) { vkErrorInvalidImage(v.image) } else { + image := Images[v.image] + transitionImageLayout(image, v.subresourceRange, v.oldLayout, v.newLayout) + if v.oldLayout == VK_IMAGE_LAYOUT_UNDEFINED { + writeImageSubresource(image, v.subresourceRange) + updateImageQueue(image, v.subresourceRange) + } } } } diff --git a/gapis/api/vulkan/errors.api b/gapis/api/vulkan/errors.api index d8355befa3..ba44e5bf79 100644 --- a/gapis/api/vulkan/errors.api +++ b/gapis/api/vulkan/errors.api @@ -58,14 +58,17 @@ sub void vkErrorInvalidPhysicalDevice(VkPhysicalDevice dev) { sub void vkErrorInvalidDevice(VkDevice dev) { vkErrorInvalidHandle("VkDevice", as!u64(dev)) + abort } sub void vkErrorInvalidQueue(VkQueue queue) { vkErrorInvalidHandle("VkQueue", as!u64(queue)) + abort } sub void vkErrorInvalidCommandBuffer(VkCommandBuffer cmdbuf) { vkErrorInvalidHandle("VkCommandBuffer", as!u64(cmdbuf)) + abort } sub void vkErrorCommandBufferIncomplete(VkCommandBuffer cmdbuf) { @@ -187,7 +190,6 @@ sub void vkErrorInvalidDisplayMode(VkDisplayModeKHR mode) { sub void vkErrorInvalidHandle(string handleType, u64 handle) { vkErrInvalidHandle(handleType, handle) - abort } sub void vkErrorNullPointer(string pointerType) { diff --git a/gapis/api/vulkan/extensions/khr_swapchain.api b/gapis/api/vulkan/extensions/khr_swapchain.api index 9edd996d86..1064d4d50f 100644 --- a/gapis/api/vulkan/extensions/khr_swapchain.api +++ b/gapis/api/vulkan/extensions/khr_swapchain.api @@ -243,9 +243,10 @@ cmd VkResult vkAcquireNextImageKHR( Semaphores[semaphore].Signaled = true } if (fence != 0) { - if (Fences[fence].Signaled) { vkErrorInvalidFence(fence) } - Fences[fence].Signaled = true - recordFenceSignal(fence) + if (Fences[fence].Signaled) { vkErrorInvalidFence(fence) } else { + Fences[fence].Signaled = true + recordFenceSignal(fence) + } } Swapchains[swapchain].ImagesAcquired[imageIndex] = true recordAcquireNextImage(swapchain, imageIndex)