From d2818e486c9eff3e9e97499c0407491601be5a6e Mon Sep 17 00:00:00 2001 From: Mark Lobodzinski Date: Wed, 13 Jan 2016 10:23:15 -0700 Subject: [PATCH] layers: LX257, Validate Uniform/Storage buffer offset alignments --- layers/device_limits.cpp | 104 ++++++++++++++++++++++++++++------ layers/device_limits.h | 2 + layers/draw_state.cpp | 38 +++++++++++-- layers/draw_state.h | 3 + layers/vk_layer_utils.cpp | 12 ++++ layers/vk_layer_utils.h | 1 + layers/vk_validation_layer_details.md | 4 ++ 7 files changed, 142 insertions(+), 22 deletions(-) diff --git a/layers/device_limits.cpp b/layers/device_limits.cpp index e46149f..fbb9af3 100644 --- a/layers/device_limits.cpp +++ b/layers/device_limits.cpp @@ -58,17 +58,18 @@ struct devExts { // This struct will be stored in a map hashed by the dispatchable object struct layer_data { - debug_report_data *report_data; - std::vector logging_callback; - VkLayerDispatchTable* device_dispatch_table; - VkLayerInstanceDispatchTable* instance_dispatch_table; - devExts device_extensions; + debug_report_data *report_data; + std::vector logging_callback; + VkLayerDispatchTable *device_dispatch_table; + VkLayerInstanceDispatchTable *instance_dispatch_table; + devExts device_extensions; // Track state of each instance - unique_ptr instanceState; - unique_ptr physicalDeviceState; - VkPhysicalDeviceFeatures actualPhysicalDeviceFeatures; - VkPhysicalDeviceFeatures requestedPhysicalDeviceFeatures; - VkPhysicalDeviceProperties physicalDeviceProperties; + unique_ptr instanceState; + unique_ptr physicalDeviceState; + VkPhysicalDeviceFeatures actualPhysicalDeviceFeatures; + VkPhysicalDeviceFeatures requestedPhysicalDeviceFeatures; + unordered_map physDevPropertyMap; + // Track physical device per logical device VkPhysicalDevice physicalDevice; // Vector indices correspond to queueFamilyIndex @@ -81,7 +82,6 @@ struct layer_data { device_extensions(), instanceState(nullptr), physicalDeviceState(nullptr), - physicalDeviceProperties(), actualPhysicalDeviceFeatures(), requestedPhysicalDeviceFeatures(), physicalDevice() @@ -257,7 +257,6 @@ VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkEnumeratePhysicalDevices(VkInst phy_dev_data->physicalDeviceState = unique_ptr(new PHYSICAL_DEVICE_STATE()); // Init actual features for each physical device my_data->instance_dispatch_table->GetPhysicalDeviceFeatures(pPhysicalDevices[i], &(phy_dev_data->actualPhysicalDeviceFeatures)); - my_data->instance_dispatch_table->GetPhysicalDeviceProperties(pPhysicalDevices[i], &(phy_dev_data->physicalDeviceProperties)); } } return result; @@ -449,13 +448,17 @@ VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkCreateDevice(VkPhysicalDevice g if (skipCall) return VK_ERROR_VALIDATION_FAILED_EXT; - layer_data *my_device_data = get_my_data_ptr(get_dispatch_key(*pDevice), layer_data_map); - VkResult result = my_device_data->device_dispatch_table->CreateDevice(gpu, pCreateInfo, pAllocator, pDevice); + layer_data *device_data = get_my_data_ptr(get_dispatch_key(*pDevice), layer_data_map); + VkResult result = device_data->device_dispatch_table->CreateDevice(gpu, pCreateInfo, pAllocator, pDevice); if (result == VK_SUCCESS) { - my_device_data->report_data = layer_debug_report_create_device(phy_dev_data->report_data, *pDevice); + device_data->report_data = layer_debug_report_create_device(phy_dev_data->report_data, *pDevice); createDeviceRegisterExtensions(pCreateInfo, *pDevice); - my_device_data->physicalDevice = gpu; + device_data->physicalDevice = gpu; + + // Get physical device properties for this device + phy_dev_data->instance_dispatch_table->GetPhysicalDeviceProperties(gpu, &(phy_dev_data->physDevPropertyMap[*pDevice])); } + return result; } @@ -517,6 +520,71 @@ VK_LAYER_EXPORT VKAPI_ATTR void VKAPI_CALL vkGetDeviceQueue(VkDevice device, uin dev_data->device_dispatch_table->GetDeviceQueue(device, queueFamilyIndex, queueIndex, pQueue); } +VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkBindBufferMemory( + VkDevice device, + VkBuffer buffer, + VkDeviceMemory mem, + VkDeviceSize memoryOffset) +{ + layer_data *dev_data = get_my_data_ptr(get_dispatch_key(device), layer_data_map); + VkResult result = VK_ERROR_VALIDATION_FAILED_EXT; + VkBool32 skipCall = VK_FALSE; + + VkDeviceSize uniformAlignment = dev_data->physDevPropertyMap[device].limits.minUniformBufferOffsetAlignment; + if (vk_safe_modulo(memoryOffset, uniformAlignment) != 0) { + skipCall |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, 0, + __LINE__, DEVLIMITS_INVALID_UNIFORM_BUFFER_OFFSET, "DL", + "vkBindBufferMemory(): memoryOffset %#" PRIxLEAST64 " must be a multiple of device limit minUniformBufferOffsetAlignment %#" PRIxLEAST64, + memoryOffset, uniformAlignment); + } + + if (VK_FALSE == skipCall) { + result = dev_data->device_dispatch_table->BindBufferMemory(device, buffer, mem, memoryOffset); + } + return result; +} + +VK_LAYER_EXPORT VKAPI_ATTR void VKAPI_CALL vkUpdateDescriptorSets( + VkDevice device, + uint32_t descriptorWriteCount, + const VkWriteDescriptorSet *pDescriptorWrites, + uint32_t descriptorCopyCount, + const VkCopyDescriptorSet *pDescriptorCopies) +{ + layer_data* dev_data = get_my_data_ptr(get_dispatch_key(device), layer_data_map); + VkResult result = VK_ERROR_VALIDATION_FAILED_EXT; + VkBool32 skipCall = VK_FALSE; + + for (auto i = 0; i < descriptorWriteCount; i++) { + if ((pDescriptorWrites[i].descriptorType == VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER) || + (pDescriptorWrites[i].descriptorType == VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC)) { + VkDeviceSize uniformAlignment = dev_data->physDevPropertyMap[device].limits.minUniformBufferOffsetAlignment; + for (auto j = 0; j < pDescriptorWrites[i].descriptorCount; j++) { + if (vk_safe_modulo(pDescriptorWrites[i].pBufferInfo[j].offset, uniformAlignment) != 0) { + skipCall |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, 0, + __LINE__, DEVLIMITS_INVALID_UNIFORM_BUFFER_OFFSET, "DL", + "vkUpdateDescriptorSets(): pDescriptorWrites[%d].pBufferInfo[%d].offset (%#" PRIxLEAST64 ") must be a multiple of device limit minUniformBufferOffsetAlignment %#" PRIxLEAST64, + i, j, pDescriptorWrites[i].pBufferInfo[j].offset, uniformAlignment); + } + } + } else if ((pDescriptorWrites[i].descriptorType == VK_DESCRIPTOR_TYPE_STORAGE_BUFFER) || + (pDescriptorWrites[i].descriptorType == VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC)) { + VkDeviceSize storageAlignment = dev_data->physDevPropertyMap[device].limits.minStorageBufferOffsetAlignment; + for (auto j = 0; j < pDescriptorWrites[i].descriptorCount; j++) { + if (vk_safe_modulo(pDescriptorWrites[i].pBufferInfo[j].offset, storageAlignment) != 0) { + skipCall |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, 0, + __LINE__, DEVLIMITS_INVALID_STORAGE_BUFFER_OFFSET, "DL", + "vkUpdateDescriptorSets(): pDescriptorWrites[%d].pBufferInfo[%d].offset (%#" PRIxLEAST64 ") must be a multiple of device limit minStorageBufferOffsetAlignment %#" PRIxLEAST64, + i, j, pDescriptorWrites[i].pBufferInfo[j].offset, storageAlignment); + } + } + } + } + if (skipCall == VK_FALSE) { + dev_data->device_dispatch_table->UpdateDescriptorSets(device, descriptorWriteCount, pDescriptorWrites, descriptorCopyCount, pDescriptorCopies); + } +} + VK_LAYER_EXPORT VKAPI_ATTR void VKAPI_CALL vkCmdUpdateBuffer( VkCommandBuffer commandBuffer, VkBuffer dstBuffer, @@ -648,6 +716,10 @@ VK_LAYER_EXPORT VKAPI_ATTR PFN_vkVoidFunction VKAPI_CALL vkGetDeviceProcAddr(VkD return (PFN_vkVoidFunction) vkFreeCommandBuffers; if (!strcmp(funcName, "vkCmdUpdateBuffer")) return (PFN_vkVoidFunction) vkCmdUpdateBuffer; + if (!strcmp(funcName, "vkBindBufferMemory")) + return (PFN_vkVoidFunction) vkBindBufferMemory; + if (!strcmp(funcName, "vkUpdateDescriptorSets")) + return (PFN_vkVoidFunction) vkUpdateDescriptorSets; if (!strcmp(funcName, "vkCmdFillBuffer")) return (PFN_vkVoidFunction) vkCmdFillBuffer; diff --git a/layers/device_limits.h b/layers/device_limits.h index 944d5b7..5caba24 100644 --- a/layers/device_limits.h +++ b/layers/device_limits.h @@ -41,6 +41,8 @@ typedef enum _DEV_LIMITS_ERROR DEVLIMITS_COUNT_MISMATCH, // App requesting a count value different than actual value DEVLIMITS_INVALID_QUEUE_CREATE_REQUEST, // Invalid queue requested based on queue family properties DEVLIMITS_LIMITS_VIOLATION, // Driver-specified limits/properties were exceeded + DEVLIMITS_INVALID_UNIFORM_BUFFER_OFFSET, // Uniform buffer offset violates device limit granularity + DEVLIMITS_INVALID_STORAGE_BUFFER_OFFSET, // Storage buffer offset violates device limit granularity } DEV_LIMITS_ERROR; typedef enum _CALL_STATE diff --git a/layers/draw_state.cpp b/layers/draw_state.cpp index a9c809b..e2a81fb 100644 --- a/layers/draw_state.cpp +++ b/layers/draw_state.cpp @@ -123,6 +123,7 @@ struct layer_data { // Current render pass VkRenderPassBeginInfo renderPassBeginInfo; uint32_t currentSubpass; + unordered_map physDevPropertyMap; layer_data() : report_data(nullptr), @@ -2931,13 +2932,15 @@ static void createDeviceRegisterExtensions(const VkDeviceCreateInfo* pCreateInfo VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkCreateDevice(VkPhysicalDevice gpu, const VkDeviceCreateInfo* pCreateInfo, const VkAllocationCallbacks* pAllocator, VkDevice* pDevice) { - layer_data* dev_data = get_my_data_ptr(get_dispatch_key(*pDevice), layer_data_map); - VkResult result = dev_data->device_dispatch_table->CreateDevice(gpu, pCreateInfo, pAllocator, pDevice); - // TODOSC : shouldn't need any customization here + layer_data *instance_data = get_my_data_ptr(get_dispatch_key(gpu), layer_data_map); + layer_data *dev_data = get_my_data_ptr(get_dispatch_key(*pDevice), layer_data_map); + VkResult result = dev_data->device_dispatch_table->CreateDevice(gpu, pCreateInfo, pAllocator, pDevice); + if (result == VK_SUCCESS) { - layer_data *my_instance_data = get_my_data_ptr(get_dispatch_key(gpu), layer_data_map); - dev_data->report_data = layer_debug_report_create_device(my_instance_data->report_data, *pDevice); + dev_data->report_data = layer_debug_report_create_device(instance_data->report_data, *pDevice); createDeviceRegisterExtensions(pCreateInfo, *pDevice); + // Get physical device limits for this device + instance_data->instance_dispatch_table->GetPhysicalDeviceProperties(gpu, &(instance_data->physDevPropertyMap[*pDevice])); } return result; } @@ -4002,6 +4005,7 @@ VK_LAYER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkAllocateCommandBuffers(VkDevice resetCB(dev_data, pCommandBuffer[i]); pCB->commandBuffer = pCommandBuffer[i]; pCB->createInfo = *pCreateInfo; + pCB->device = device; } } } @@ -4371,7 +4375,29 @@ VK_LAYER_EXPORT VKAPI_ATTR void VKAPI_CALL vkCmdBindDescriptorSets(VkCommandBuff skipCall |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_DESCRIPTOR_SET_EXT, (uint64_t) pDescriptorSets[i], __LINE__, DRAWSTATE_INVALID_DYNAMIC_OFFSET_COUNT, "DS", "descriptorSet #%u (%#" PRIxLEAST64 ") requires %u dynamicOffsets, but only %u dynamicOffsets are left in pDynamicOffsets array. There must be one dynamic offset for each dynamic descriptor being bound.", i, (uint64_t) pDescriptorSets[i], pSet->pLayout->dynamicDescriptorCount, (dynamicOffsetCount - totalDynamicDescriptors)); - } else { // Store dynamic offsets with the set + } else { // Validate and store dynamic offsets with the set + // Validate Dynamic Offset Minimums + uint32_t cur_dyn_offset = totalDynamicDescriptors; + for (uint32_t d = 0; d < pSet->descriptorCount; d++) { + if (pSet->pLayout->descriptorTypes[i] == VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC) { + if (vk_safe_modulo(pDynamicOffsets[cur_dyn_offset], dev_data->physDevPropertyMap[pCB->device].limits.minUniformBufferOffsetAlignment) != 0) { + skipCall |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, 0, + __LINE__, DRAWSTATE_INVALID_UNIFORM_BUFFER_OFFSET, "DS", + "vkCmdBindDescriptorSets(): pDynamicOffsets[%d] is %d but must be a multiple of device limit minUniformBufferOffsetAlignment %#" PRIxLEAST64, + cur_dyn_offset, pDynamicOffsets[cur_dyn_offset], dev_data->physDevPropertyMap[pCB->device].limits.minUniformBufferOffsetAlignment); + } + cur_dyn_offset++; + } else if (pSet->pLayout->descriptorTypes[i] == VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC) { + if (vk_safe_modulo(pDynamicOffsets[cur_dyn_offset], dev_data->physDevPropertyMap[pCB->device].limits.minStorageBufferOffsetAlignment) != 0) { + skipCall |= log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VK_DEBUG_REPORT_OBJECT_TYPE_PHYSICAL_DEVICE_EXT, 0, + __LINE__, DRAWSTATE_INVALID_STORAGE_BUFFER_OFFSET, "DS", + "vkCmdBindDescriptorSets(): pDynamicOffsets[%d] is %d but must be a multiple of device limit minStorageBufferOffsetAlignment %#" PRIxLEAST64, + cur_dyn_offset, pDynamicOffsets[cur_dyn_offset], dev_data->physDevPropertyMap[pCB->device].limits.minStorageBufferOffsetAlignment); + } + cur_dyn_offset++; + } + } + // Store offsets const uint32_t* pStartDynOffset = pDynamicOffsets + totalDynamicDescriptors; pSet->dynamicOffsets.assign(pStartDynOffset, pStartDynOffset + pSet->pLayout->dynamicDescriptorCount); totalDynamicDescriptors += pSet->pLayout->dynamicDescriptorCount; diff --git a/layers/draw_state.h b/layers/draw_state.h index e632e7e..4f20d47 100755 --- a/layers/draw_state.h +++ b/layers/draw_state.h @@ -99,6 +99,8 @@ typedef enum _DRAW_STATE_ERROR DRAWSTATE_DOUBLE_DESTROY, // Destroying an object twice DRAWSTATE_OBJECT_INUSE, // Destroying an object in use by a command buffer DRAWSTATE_QUEUE_FORWARD_PROGRESS, // Queue cannot guarantee forward progress + DRAWSTATE_INVALID_UNIFORM_BUFFER_OFFSET, // Dynamic Uniform Buffer Offsets violate device limit + DRAWSTATE_INVALID_STORAGE_BUFFER_OFFSET, // Dynamic Storage Buffer Offsets violate device limit } DRAW_STATE_ERROR; typedef enum _SHADER_CHECKER_ERROR { @@ -476,6 +478,7 @@ typedef struct _GLOBAL_CB_NODE { VkCommandBufferAllocateInfo createInfo; VkCommandBufferBeginInfo beginInfo; VkFence fence; // fence tracking this cmd buffer + VkDevice device; // device this DB belongs to uint64_t numCmds; // number of cmds in this CB uint64_t drawCount[NUM_DRAW_TYPES]; // Count of each type of draw in this CB CB_STATE state; // Track cmd buffer update state diff --git a/layers/vk_layer_utils.cpp b/layers/vk_layer_utils.cpp index 4eb1e90..cfa2f9b 100644 --- a/layers/vk_layer_utils.cpp +++ b/layers/vk_layer_utils.cpp @@ -584,3 +584,15 @@ unsigned int vk_format_get_channel_count(VkFormat format) { return vk_format_table[format].channel_count; } + +// Perform a zero-tolerant modulo operation +VkDeviceSize vk_safe_modulo(VkDeviceSize dividend, VkDeviceSize divisor) +{ + VkDeviceSize result = 0; + if (divisor != 0) { + result = dividend % divisor; + } + return result; +} + + diff --git a/layers/vk_layer_utils.h b/layers/vk_layer_utils.h index 5e34502..c375995 100644 --- a/layers/vk_layer_utils.h +++ b/layers/vk_layer_utils.h @@ -111,6 +111,7 @@ bool vk_format_is_compressed(VkFormat format); size_t vk_format_get_size(VkFormat format); unsigned int vk_format_get_channel_count(VkFormat format); VkFormatCompatibilityClass vk_format_get_compatibility_class(VkFormat format); +VkDeviceSize vk_safe_modulo(VkDeviceSize dividend, VkDeviceSize divisor); static inline int u_ffs(int val) { diff --git a/layers/vk_validation_layer_details.md b/layers/vk_validation_layer_details.md index 1011c3e..7cde37d 100644 --- a/layers/vk_validation_layer_details.md +++ b/layers/vk_validation_layer_details.md @@ -71,6 +71,8 @@ The VK_LAYER_LUNARG_draw_state layer tracks state leading into Draw cmds. This i | Verify Memory Buffer Not In Use | Validate that memory buffer being freed is not in use | OBJECT_INUSE | vkDestroyBuffer | TBD | None | | Verify Get Queries| Validate that that queries are properly initialized and synchronized | INVALID_QUERY | vkGetFenceStatus vkQueueWaitIdle vkWaitForFences vkDeviceWaitIdle | TBD | None | | Live Semaphore | When waiting on a semaphore, need to make sure that the semaphore is live and therefore can be signalled, otherwise queue is stalled and cannot make forward progress. | QUEUE_FORWARD_PROGRESS | vkQueueSubmit vkQueueBindSparse vkQueuePresentKHR vkAcquireNextImageKHR | TODO | Create test | +| Storage Buffer Alignment | Storage Buffer offsets in BindDescriptorSets must agree with offset alignment device limit | INVALID_STORAGE_BUFFER_OFFSET | vkCmdBindDescriptorSets | TODO | Create test | +| Uniform Buffer Alignment | Uniform Buffer offsets in BindDescriptorSets must agree with offset alignment device limit | INVALID_UNIFORM_BUFFER_OFFSET | vkCmdBindDescriptorSets | TODO | Create test | | NA | Enum used for informational messages | NONE | | NA | None | | NA | Enum used for errors in the layer itself. This does not indicate an app issue, but instead a bug in the layer. | INTERNAL_ERROR | | NA | None | | NA | Enum used when VK_LAYER_LUNARG_draw_state attempts to allocate memory for its own internal use and is unable to. | OUT_OF_MEMORY | | NA | None | @@ -322,6 +324,8 @@ For the second category of errors, VK_LAYER_LUNARG_device_limits stores its own | Valid Image Resource Size | When creating an image, ensure the the total image resource size is less than the queried device maximum resource size | LIMITS_VIOLATION | vkCreateImage | CreateImageResourceSizeViolation | NA | | Alignment | When updating a buffer, data should be aligned on 4 byte boundaries | LIMITS_VIOLATION | vkCmdUpdateBuffer | UpdateBufferAlignment | NA | | Alignment | When filling a buffer, data should be aligned on 4 byte boundaries | LIMITS_VIOLATION | vkCmdFillBuffer | UpdateBufferAlignment | NA | +| Storage Buffer Alignment | Storage Buffer offsets must agree with offset alignment device limit | INVALID_STORAGE_BUFFER_OFFSET | vkBindBufferMemory vkUpdateDescriptorSets | TODO | Create test | +| Uniform Buffer Alignment | Uniform Buffer offsets must agree with offset alignment device limit | INVALID_UNIFORM_BUFFER_OFFSET | vkBindBufferMemory vkUpdateDescriptorSets | TODO | Create test | | NA | Enum used for informational messages | NONE | | NA | None | ### VK_LAYER_LUNARG_device_limits Pending Work -- 2.7.4