From 9efb8af92de180ea85ee8062ddcd4aee00dbd2c0 Mon Sep 17 00:00:00 2001 From: Petr Kraus Date: Sun, 10 Sep 2017 02:26:33 +0200 Subject: layers: Fix Graphics Pipeline pointers not ignored Some VkGraphicsPipelineCreateInfo pointers must be ignored under some conditions, but were not in the layers. Add relevant tests. Fix tests found broken (using depth or color without attachment in subpass) Change-Id: I3e2a3f61a52c72ce3a11483ff8b031189f4c61c9 --- layers/core_validation.cpp | 2 +- layers/core_validation_types.h | 28 ++++++++--- layers/parameter_validation.h | 10 +++- layers/parameter_validation_utils.cpp | 95 +++++++++++++++++++++++++++++++++-- layers/unique_objects.cpp | 66 +++++++++++++++++++++++- layers/unique_objects.h | 10 ++++ 6 files changed, 198 insertions(+), 13 deletions(-) (limited to 'layers') diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 78e72aaf..b210f8f9 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -4514,8 +4514,8 @@ VKAPI_ATTR VkResult VKAPI_CALL CreateGraphicsPipelines(VkDevice device, VkPipeli for (i = 0; i < count; i++) { pipe_state.push_back(std::unique_ptr(new PIPELINE_STATE)); - pipe_state[i]->initGraphicsPipeline(&pCreateInfos[i]); pipe_state[i]->render_pass_ci.initialize(GetRenderPassState(dev_data, pCreateInfos[i].renderPass)->createInfo.ptr()); + pipe_state[i]->initGraphicsPipeline(&pCreateInfos[i]); pipe_state[i]->pipeline_layout = *getPipelineLayout(dev_data, pCreateInfos[i].layout); } diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h index 928583cd..610539ab 100644 --- a/layers/core_validation_types.h +++ b/layers/core_validation_types.h @@ -576,7 +576,23 @@ class PIPELINE_STATE : public BASE_NODE { pipeline_layout() {} void initGraphicsPipeline(const VkGraphicsPipelineCreateInfo *pCreateInfo) { - graphicsPipelineCI.initialize(pCreateInfo); + bool uses_color_attachment = false; + bool uses_depthstencil_attachment = false; + if (pCreateInfo->subpass < render_pass_ci.subpassCount) { + const auto &subpass = render_pass_ci.pSubpasses[pCreateInfo->subpass]; + + for (uint32_t i = 0; i < subpass.colorAttachmentCount; ++i) { + if (subpass.pColorAttachments[i].attachment != VK_ATTACHMENT_UNUSED) { + uses_color_attachment = true; + break; + } + } + + if (subpass.pDepthStencilAttachment && subpass.pDepthStencilAttachment->attachment != VK_ATTACHMENT_UNUSED) { + uses_depthstencil_attachment = true; + } + } + graphicsPipelineCI.initialize(pCreateInfo, uses_color_attachment, uses_depthstencil_attachment); // Make sure compute pipeline is null VkComputePipelineCreateInfo emptyComputeCI = {}; computePipelineCI.initialize(&emptyComputeCI); @@ -585,15 +601,15 @@ class PIPELINE_STATE : public BASE_NODE { this->duplicate_shaders |= this->active_shaders & pPSSCI->stage; this->active_shaders |= pPSSCI->stage; } - if (pCreateInfo->pVertexInputState) { - const VkPipelineVertexInputStateCreateInfo *pVICI = pCreateInfo->pVertexInputState; + if (graphicsPipelineCI.pVertexInputState) { + const auto pVICI = graphicsPipelineCI.pVertexInputState; if (pVICI->vertexBindingDescriptionCount) { this->vertexBindingDescriptions = std::vector( pVICI->pVertexBindingDescriptions, pVICI->pVertexBindingDescriptions + pVICI->vertexBindingDescriptionCount); } } - if (pCreateInfo->pColorBlendState) { - const VkPipelineColorBlendStateCreateInfo *pCBCI = pCreateInfo->pColorBlendState; + if (graphicsPipelineCI.pColorBlendState) { + const auto pCBCI = graphicsPipelineCI.pColorBlendState; if (pCBCI->attachmentCount) { this->attachments = std::vector(pCBCI->pAttachments, pCBCI->pAttachments + pCBCI->attachmentCount); @@ -605,7 +621,7 @@ class PIPELINE_STATE : public BASE_NODE { computePipelineCI.initialize(pCreateInfo); // Make sure gfx pipeline is null VkGraphicsPipelineCreateInfo emptyGraphicsCI = {}; - graphicsPipelineCI.initialize(&emptyGraphicsCI); + graphicsPipelineCI.initialize(&emptyGraphicsCI, false, false); switch (computePipelineCI.stage.stage) { case VK_SHADER_STAGE_COMPUTE_BIT: this->active_shaders |= VK_SHADER_STAGE_COMPUTE_BIT; diff --git a/layers/parameter_validation.h b/layers/parameter_validation.h index 6ef3de2e..b9d153ce 100644 --- a/layers/parameter_validation.h +++ b/layers/parameter_validation.h @@ -25,8 +25,9 @@ #include #include #include -#include +#include #include +#include #include "vulkan/vulkan.h" #include "vk_enum_string_helper.h" @@ -80,6 +81,13 @@ struct layer_data { VkDevice device = VK_NULL_HANDLE; DeviceExtensions extensions; + struct SubpassesUsageStates { + std::unordered_set subpasses_using_color_attachment; + std::unordered_set subpasses_using_depthstencil_attachment; + }; + + std::unordered_map renderpasses_states; + VkLayerDispatchTable dispatch_table = {}; }; diff --git a/layers/parameter_validation_utils.cpp b/layers/parameter_validation_utils.cpp index 4f6f5b8c..11706cd9 100644 --- a/layers/parameter_validation_utils.cpp +++ b/layers/parameter_validation_utils.cpp @@ -82,6 +82,10 @@ extern bool parameter_validation_vkDestroyDebugReportCallbackEXT(VkInstance inst const VkAllocationCallbacks *pAllocator); extern bool parameter_validation_vkCreateCommandPool(VkDevice device, const VkCommandPoolCreateInfo *pCreateInfo, const VkAllocationCallbacks *pAllocator, VkCommandPool *pCommandPool); +extern bool parameter_validation_vkCreateRenderPass(VkDevice device, const VkRenderPassCreateInfo *pCreateInfo, + const VkAllocationCallbacks *pAllocator, VkRenderPass *pRenderPass); +extern bool parameter_validation_vkDestroyRenderPass(VkDevice device, VkRenderPass renderPass, + const VkAllocationCallbacks *pAllocator); // TODO : This can be much smarter, using separate locks for separate global data std::mutex global_lock; @@ -590,6 +594,78 @@ VKAPI_ATTR VkResult VKAPI_CALL vkCreateQueryPool(VkDevice device, const VkQueryP return result; } +VKAPI_ATTR VkResult VKAPI_CALL vkCreateRenderPass(VkDevice device, const VkRenderPassCreateInfo *pCreateInfo, + const VkAllocationCallbacks *pAllocator, VkRenderPass *pRenderPass) { + layer_data *device_data = GetLayerDataPtr(get_dispatch_key(device), layer_data_map); + bool skip = false; + VkResult result = VK_ERROR_VALIDATION_FAILED_EXT; + + { + std::unique_lock lock(global_lock); + skip |= parameter_validation_vkCreateRenderPass(device, pCreateInfo, pAllocator, pRenderPass); + + typedef bool (*PFN_manual_vkCreateRenderPass)(VkDevice device, const VkRenderPassCreateInfo *pCreateInfo, + const VkAllocationCallbacks *pAllocator, VkRenderPass *pRenderPass); + PFN_manual_vkCreateRenderPass custom_func = (PFN_manual_vkCreateRenderPass)custom_functions["vkCreateRenderPass"]; + if (custom_func != nullptr) { + skip |= custom_func(device, pCreateInfo, pAllocator, pRenderPass); + } + } + + if (!skip) { + result = device_data->dispatch_table.CreateRenderPass(device, pCreateInfo, pAllocator, pRenderPass); + + // track the state necessary for checking vkCreateGraphicsPipeline (subpass usage of depth and color attachments) + if (result == VK_SUCCESS) { + std::unique_lock lock(global_lock); + const auto renderPass = *pRenderPass; + auto &renderpass_state = device_data->renderpasses_states[renderPass]; + + for (uint32_t subpass = 0; subpass < pCreateInfo->subpassCount; ++subpass) { + bool uses_color = false; + for (uint32_t i = 0; i < pCreateInfo->pSubpasses[subpass].colorAttachmentCount && !uses_color; ++i) + if (pCreateInfo->pSubpasses[subpass].pColorAttachments[i].attachment != VK_ATTACHMENT_UNUSED) uses_color = true; + + bool uses_depthstencil = false; + if (pCreateInfo->pSubpasses[subpass].pDepthStencilAttachment) + if (pCreateInfo->pSubpasses[subpass].pDepthStencilAttachment->attachment != VK_ATTACHMENT_UNUSED) + uses_depthstencil = true; + + if (uses_color) renderpass_state.subpasses_using_color_attachment.insert(subpass); + if (uses_depthstencil) renderpass_state.subpasses_using_depthstencil_attachment.insert(subpass); + } + } + } + return result; +} + +VKAPI_ATTR void VKAPI_CALL vkDestroyRenderPass(VkDevice device, VkRenderPass renderPass, const VkAllocationCallbacks *pAllocator) { + layer_data *device_data = GetLayerDataPtr(get_dispatch_key(device), layer_data_map); + bool skip = false; + + { + std::unique_lock lock(global_lock); + skip |= parameter_validation_vkDestroyRenderPass(device, renderPass, pAllocator); + + typedef bool (*PFN_manual_vkDestroyRenderPass)(VkDevice device, VkRenderPass renderPass, + const VkAllocationCallbacks *pAllocator); + PFN_manual_vkDestroyRenderPass custom_func = (PFN_manual_vkDestroyRenderPass)custom_functions["vkDestroyRenderPass"]; + if (custom_func != nullptr) { + skip |= custom_func(device, renderPass, pAllocator); + } + } + + if (!skip) { + device_data->dispatch_table.DestroyRenderPass(device, renderPass, pAllocator); + + // track the state necessary for checking vkCreateGraphicsPipeline (subpass usage of depth and color attachments) + { + std::unique_lock lock(global_lock); + device_data->renderpasses_states.erase(renderPass); + } + } +} + bool pv_vkCreateBuffer(VkDevice device, const VkBufferCreateInfo *pCreateInfo, const VkAllocationCallbacks *pAllocator, VkBuffer *pBuffer) { bool skip = false; @@ -1207,8 +1283,20 @@ bool pv_vkCreateGraphicsPipelines(VkDevice device, VkPipelineCache pipelineCache } } - // TODO: Conditional NULL check based on subpass depth/stencil attachment - if (pCreateInfos[i].pDepthStencilState != nullptr) { + bool uses_color_attachment = false; + bool uses_depthstencil_attachment = false; + { + const auto subpasses_uses_it = device_data->renderpasses_states.find(pCreateInfos[i].renderPass); + if (subpasses_uses_it != device_data->renderpasses_states.end()) { + const auto &subpasses_uses = subpasses_uses_it->second; + if (subpasses_uses.subpasses_using_color_attachment.count(pCreateInfos[i].subpass)) + uses_color_attachment = true; + if (subpasses_uses.subpasses_using_depthstencil_attachment.count(pCreateInfos[i].subpass)) + uses_depthstencil_attachment = true; + } + } + + if (pCreateInfos[i].pDepthStencilState != nullptr && uses_depthstencil_attachment) { skip |= validate_struct_pnext( report_data, "vkCreateGraphicsPipelines", ParameterName("pCreateInfos[%i].pDepthStencilState->pNext", ParameterName::IndexVector{i}), NULL, @@ -1302,8 +1390,7 @@ bool pv_vkCreateGraphicsPipelines(VkDevice device, VkPipelineCache pipelineCache } } - // TODO: Conditional NULL check based on subpass color attachment - if (pCreateInfos[i].pColorBlendState != nullptr) { + if (pCreateInfos[i].pColorBlendState != nullptr && uses_color_attachment) { skip |= validate_struct_pnext( report_data, "vkCreateGraphicsPipelines", ParameterName("pCreateInfos[%i].pColorBlendState->pNext", ParameterName::IndexVector{i}), NULL, diff --git a/layers/unique_objects.cpp b/layers/unique_objects.cpp index d324ce49..8af77724 100644 --- a/layers/unique_objects.cpp +++ b/layers/unique_objects.cpp @@ -328,7 +328,22 @@ VKAPI_ATTR VkResult VKAPI_CALL CreateGraphicsPipelines(VkDevice device, VkPipeli local_pCreateInfos = new safe_VkGraphicsPipelineCreateInfo[createInfoCount]; std::lock_guard lock(global_lock); for (uint32_t idx0 = 0; idx0 < createInfoCount; ++idx0) { - local_pCreateInfos[idx0].initialize(&pCreateInfos[idx0]); + bool uses_color_attachment = false; + bool uses_depthstencil_attachment = false; + { + const auto subpasses_uses_it = + device_data->renderpasses_states.find(Unwrap(device_data, pCreateInfos[idx0].renderPass)); + if (subpasses_uses_it != device_data->renderpasses_states.end()) { + const auto &subpasses_uses = subpasses_uses_it->second; + if (subpasses_uses.subpasses_using_color_attachment.count(pCreateInfos[idx0].subpass)) + uses_color_attachment = true; + if (subpasses_uses.subpasses_using_depthstencil_attachment.count(pCreateInfos[idx0].subpass)) + uses_depthstencil_attachment = true; + } + } + + local_pCreateInfos[idx0].initialize(&pCreateInfos[idx0], uses_color_attachment, uses_depthstencil_attachment); + if (pCreateInfos[idx0].basePipelineHandle) { local_pCreateInfos[idx0].basePipelineHandle = Unwrap(device_data, pCreateInfos[idx0].basePipelineHandle); } @@ -366,6 +381,55 @@ VKAPI_ATTR VkResult VKAPI_CALL CreateGraphicsPipelines(VkDevice device, VkPipeli return result; } +static void PostCallCreateRenderPass(layer_data *dev_data, const VkRenderPassCreateInfo *pCreateInfo, VkRenderPass renderPass) { + auto &renderpass_state = dev_data->renderpasses_states[renderPass]; + + for (uint32_t subpass = 0; subpass < pCreateInfo->subpassCount; ++subpass) { + bool uses_color = false; + for (uint32_t i = 0; i < pCreateInfo->pSubpasses[subpass].colorAttachmentCount && !uses_color; ++i) + if (pCreateInfo->pSubpasses[subpass].pColorAttachments[i].attachment != VK_ATTACHMENT_UNUSED) uses_color = true; + + bool uses_depthstencil = false; + if (pCreateInfo->pSubpasses[subpass].pDepthStencilAttachment) + if (pCreateInfo->pSubpasses[subpass].pDepthStencilAttachment->attachment != VK_ATTACHMENT_UNUSED) + uses_depthstencil = true; + + if (uses_color) renderpass_state.subpasses_using_color_attachment.insert(subpass); + if (uses_depthstencil) renderpass_state.subpasses_using_depthstencil_attachment.insert(subpass); + } +} + +VKAPI_ATTR VkResult VKAPI_CALL CreateRenderPass(VkDevice device, const VkRenderPassCreateInfo *pCreateInfo, + const VkAllocationCallbacks *pAllocator, VkRenderPass *pRenderPass) { + layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(device), layer_data_map); + VkResult result = dev_data->dispatch_table.CreateRenderPass(device, pCreateInfo, pAllocator, pRenderPass); + if (VK_SUCCESS == result) { + std::lock_guard lock(global_lock); + + PostCallCreateRenderPass(dev_data, pCreateInfo, *pRenderPass); + + *pRenderPass = WrapNew(dev_data, *pRenderPass); + } + return result; +} + +static void PostCallDestroyRenderPass(layer_data *dev_data, VkRenderPass renderPass) { + dev_data->renderpasses_states.erase(renderPass); +} + +VKAPI_ATTR void VKAPI_CALL DestroyRenderPass(VkDevice device, VkRenderPass renderPass, const VkAllocationCallbacks *pAllocator) { + layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(device), layer_data_map); + std::unique_lock lock(global_lock); + uint64_t renderPass_id = reinterpret_cast(renderPass); + renderPass = (VkRenderPass)dev_data->unique_id_mapping[renderPass_id]; + dev_data->unique_id_mapping.erase(renderPass_id); + lock.unlock(); + dev_data->dispatch_table.DestroyRenderPass(device, renderPass, pAllocator); + + lock.lock(); + PostCallDestroyRenderPass(dev_data, renderPass); +} + VKAPI_ATTR VkResult VKAPI_CALL CreateSwapchainKHR(VkDevice device, const VkSwapchainCreateInfoKHR *pCreateInfo, const VkAllocationCallbacks *pAllocator, VkSwapchainKHR *pSwapchain) { layer_data *my_map_data = GetLayerDataPtr(get_dispatch_key(device), layer_data_map); diff --git a/layers/unique_objects.h b/layers/unique_objects.h index 57b9dbf3..e604e915 100644 --- a/layers/unique_objects.h +++ b/layers/unique_objects.h @@ -21,6 +21,9 @@ #include "vulkan/vulkan.h" +#include +#include + #include "vk_layer_data.h" #include "vk_safe_struct.h" #include "vk_layer_utils.h" @@ -69,6 +72,13 @@ struct layer_data { std::unordered_map unique_id_mapping; // Map uniqueID to actual object handle VkPhysicalDevice gpu; + struct SubpassesUsageStates { + std::unordered_set subpasses_using_color_attachment; + std::unordered_set subpasses_using_depthstencil_attachment; + }; + // uses unwrapped handles + std::unordered_map renderpasses_states; + layer_data() : wsi_enabled(false), gpu(VK_NULL_HANDLE){}; }; -- cgit v1.2.3