From 4ce84fcfe15039173e326b8a88468ed9afb2a199 Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Thu, 21 Oct 2021 23:45:54 +0000 Subject: [PATCH] Improving Vulkan backend validation messages. Improves validation messages in various Vulkan backend files: - vulkan/DeviceVk.cpp - vulkan/ShaderModuleVk.cpp - vulkan/SwapChainVk.cpp - vulkan/TextureVk.cpp - vulkan/external_memory/MemoryServiceDmaBuf.cpp - vulkan/external_memory/MemoryServiceOpaqueFD.cpp - vulkan/external_memory/MemoryServiceZirconHandle.cpp - vulkan/external_semaphore/SemaphoreServiceFD.cpp - vulkan/external_semaphore/SemaphoreServiceZirconHandle.cpp Bug: dawn:563 Change-Id: I521fecc29e7919413aa6210eff050848689296a1 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/67122 Commit-Queue: Brandon Jones Auto-Submit: Brandon Jones Reviewed-by: Austin Eng --- src/dawn_native/Surface.cpp | 24 ++++++++++++ src/dawn_native/Surface.h | 5 +++ src/dawn_native/vulkan/DeviceVk.cpp | 18 +++++---- src/dawn_native/vulkan/ShaderModuleVk.cpp | 8 +--- src/dawn_native/vulkan/SwapChainVk.cpp | 35 ++++++++--------- src/dawn_native/vulkan/TextureVk.cpp | 38 ++++++++----------- .../external_memory/MemoryServiceDmaBuf.cpp | 24 ++++++------ .../external_memory/MemoryServiceOpaqueFD.cpp | 16 ++++---- .../MemoryServiceZirconHandle.cpp | 17 ++++----- .../external_semaphore/SemaphoreServiceFD.cpp | 4 +- .../SemaphoreServiceZirconHandle.cpp | 5 +-- 11 files changed, 104 insertions(+), 90 deletions(-) diff --git a/src/dawn_native/Surface.cpp b/src/dawn_native/Surface.cpp index 56027a9e68..505fc60929 100644 --- a/src/dawn_native/Surface.cpp +++ b/src/dawn_native/Surface.cpp @@ -30,6 +30,30 @@ namespace dawn_native { + absl::FormatConvertResult AbslFormatConvert( + Surface::Type value, + const absl::FormatConversionSpec& spec, + absl::FormatSink* s) { + switch (value) { + case Surface::Type::MetalLayer: + s->Append("MetalLayer"); + break; + case Surface::Type::WindowsHWND: + s->Append("WindowsHWND"); + break; + case Surface::Type::WindowsCoreWindow: + s->Append("WindowsCoreWindow"); + break; + case Surface::Type::WindowsSwapChainPanel: + s->Append("WindowsSwapChainPanel"); + break; + case Surface::Type::Xlib: + s->Append("Xlib"); + break; + } + return {true}; + } + #if defined(DAWN_ENABLE_BACKEND_METAL) bool InheritsFromCAMetalLayer(void* obj); #endif // defined(DAWN_ENABLE_BACKEND_METAL) diff --git a/src/dawn_native/Surface.h b/src/dawn_native/Surface.h index a137736857..d3b3b692d8 100644 --- a/src/dawn_native/Surface.h +++ b/src/dawn_native/Surface.h @@ -100,6 +100,11 @@ namespace dawn_native { uint32_t mXWindow = 0; }; + absl::FormatConvertResult AbslFormatConvert( + Surface::Type value, + const absl::FormatConversionSpec& spec, + absl::FormatSink* s); + // For the benefit of template generation. using SurfaceBase = Surface; diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp index 1360d3c76b..48a8b19205 100644 --- a/src/dawn_native/vulkan/DeviceVk.cpp +++ b/src/dawn_native/vulkan/DeviceVk.cpp @@ -745,16 +745,16 @@ namespace dawn_native { namespace vulkan { } // Check services support this combination of handle type / image info - if (!mExternalSemaphoreService->Supported()) { - return DAWN_VALIDATION_ERROR("External semaphore usage not supported"); - } - if (!mExternalMemoryService->SupportsImportMemory( + DAWN_INVALID_IF(!mExternalSemaphoreService->Supported(), + "External semaphore usage not supported"); + + DAWN_INVALID_IF( + !mExternalMemoryService->SupportsImportMemory( VulkanImageFormat(this, textureDescriptor->format), VK_IMAGE_TYPE_2D, VK_IMAGE_TILING_OPTIMAL, VulkanImageUsage(usage, GetValidInternalFormat(textureDescriptor->format)), - VK_IMAGE_CREATE_ALIAS_BIT_KHR)) { - return DAWN_VALIDATION_ERROR("External memory usage not supported"); - } + VK_IMAGE_CREATE_ALIAS_BIT_KHR), + "External memory usage not supported"); // Create an external semaphore to signal when the texture is done being used DAWN_TRY_ASSIGN(*outSignalSemaphore, @@ -815,7 +815,9 @@ namespace dawn_native { namespace vulkan { if (ConsumedError(ValidateTextureDescriptor(this, textureDescriptor))) { return nullptr; } - if (ConsumedError(ValidateVulkanImageCanBeWrapped(this, textureDescriptor))) { + if (ConsumedError(ValidateVulkanImageCanBeWrapped(this, textureDescriptor), + "validating that a Vulkan image can be wrapped with %s.", + textureDescriptor)) { return nullptr; } diff --git a/src/dawn_native/vulkan/ShaderModuleVk.cpp b/src/dawn_native/vulkan/ShaderModuleVk.cpp index 9cad708689..b8b74753f4 100644 --- a/src/dawn_native/vulkan/ShaderModuleVk.cpp +++ b/src/dawn_native/vulkan/ShaderModuleVk.cpp @@ -112,8 +112,6 @@ namespace dawn_native { namespace vulkan { } // Creation of VkShaderModule is deferred to this point when using tint generator - std::ostringstream errorStream; - errorStream << "Tint SPIR-V writer failure:" << std::endl; // Remap BindingNumber to BindingIndex in WGSL shader using BindingRemapper = tint::transform::BindingRemapper; @@ -159,10 +157,8 @@ namespace dawn_native { namespace vulkan { options.emit_vertex_point_size = true; options.disable_workgroup_init = GetDevice()->IsToggleEnabled(Toggle::DisableWorkgroupInit); auto result = tint::writer::spirv::Generate(&program, options); - if (!result.success) { - errorStream << "Generator: " << result.error << std::endl; - return DAWN_VALIDATION_ERROR(errorStream.str().c_str()); - } + DAWN_INVALID_IF(!result.success, "An error occured while generating SPIR-V: %s.", + result.error); std::vector spirv = std::move(result.spirv); DAWN_TRY( diff --git a/src/dawn_native/vulkan/SwapChainVk.cpp b/src/dawn_native/vulkan/SwapChainVk.cpp index c0f856fca3..95ba482d13 100644 --- a/src/dawn_native/vulkan/SwapChainVk.cpp +++ b/src/dawn_native/vulkan/SwapChainVk.cpp @@ -182,7 +182,8 @@ namespace dawn_native { namespace vulkan { break; } - return DAWN_VALIDATION_ERROR("Unsupported surface type for Vulkan"); + return DAWN_FORMAT_VALIDATION_ERROR("Unsupported surface type (%s) for Vulkan.", + surface->GetType()); } VkPresentModeKHR ToVulkanPresentMode(wgpu::PresentMode mode) { @@ -241,9 +242,10 @@ namespace dawn_native { namespace vulkan { // TODO(crbug.com/dawn/269): figure out what should happen when surfaces are used by // multiple backends one after the other. It probably needs to block until the backend // and GPU are completely finished with the previous swapchain. - if (previousSwapChain->GetBackendType() != wgpu::BackendType::Vulkan) { - return DAWN_VALIDATION_ERROR("vulkan::SwapChain cannot switch between APIs"); - } + DAWN_INVALID_IF(previousSwapChain->GetBackendType() != wgpu::BackendType::Vulkan, + "Vulkan SwapChain cannot switch backend types from %s to %s.", + previousSwapChain->GetBackendType(), wgpu::BackendType::Vulkan); + // TODO(crbug.com/dawn/269): use ToBackend once OldSwapChainBase is removed. SwapChain* previousVulkanSwapChain = static_cast(previousSwapChain); @@ -251,9 +253,8 @@ namespace dawn_native { namespace vulkan { // Vulkan devices on different VkInstances. Probably needs to block too! VkInstance previousInstance = ToBackend(previousSwapChain->GetDevice())->GetVkInstance(); - if (previousInstance != ToBackend(GetDevice())->GetVkInstance()) { - return DAWN_VALIDATION_ERROR("vulkan::SwapChain cannot switch between instances"); - } + DAWN_INVALID_IF(previousInstance != ToBackend(GetDevice())->GetVkInstance(), + "Vulkan SwapChain cannot switch between Vulkan instances."); // The previous swapchain is a dawn_native::vulkan::SwapChain so we can reuse its // VkSurfaceKHR provided since they are on the same instance. @@ -389,23 +390,23 @@ namespace dawn_native { namespace vulkan { } if (!hasBGRA8Unorm) { return DAWN_INTERNAL_ERROR( - "Vulkan swapchain must support BGRA8Unorm with SRGB colorspace"); + "Vulkan SwapChain must support BGRA8Unorm with sRGB colorspace."); } config.format = VK_FORMAT_B8G8R8A8_UNORM; config.colorSpace = VK_COLOR_SPACE_SRGB_NONLINEAR_KHR; config.wgpuFormat = wgpu::TextureFormat::BGRA8Unorm; // Only the identity transform with opaque alpha is supported for now. - if ((surfaceInfo.capabilities.supportedTransforms & - VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR) == 0) { - return DAWN_VALIDATION_ERROR("Vulkan swapchain must support the identity transform"); - } + DAWN_INVALID_IF((surfaceInfo.capabilities.supportedTransforms & + VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR) == 0, + "Vulkan SwapChain must support the identity transform."); + config.transform = VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR; - if ((surfaceInfo.capabilities.supportedCompositeAlpha & - VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR) == 0) { - return DAWN_VALIDATION_ERROR("Vulkan swapchain must support opaque alpha"); - } + DAWN_INVALID_IF((surfaceInfo.capabilities.supportedCompositeAlpha & + VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR) == 0, + "Vulkan SwapChain must support opaque alpha."); + config.alphaMode = VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR; // Choose the number of images for the swapchain= and clamp it to the min and max from the @@ -453,7 +454,7 @@ namespace dawn_native { namespace vulkan { // then we'll need to have a second fallback that uses a blit shader :( if ((supportedUsages & VK_IMAGE_USAGE_TRANSFER_DST_BIT) == 0) { return DAWN_INTERNAL_ERROR( - "Swapchain cannot fallback to a blit because of a missing " + "SwapChain cannot fallback to a blit because of a missing " "VK_IMAGE_USAGE_TRANSFER_DST_BIT"); } config.usage = VK_IMAGE_USAGE_TRANSFER_DST_BIT; diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp index 70fbeee776..93c048685f 100644 --- a/src/dawn_native/vulkan/TextureVk.cpp +++ b/src/dawn_native/vulkan/TextureVk.cpp @@ -527,21 +527,18 @@ namespace dawn_native { namespace vulkan { MaybeError ValidateVulkanImageCanBeWrapped(const DeviceBase*, const TextureDescriptor* descriptor) { - if (descriptor->dimension != wgpu::TextureDimension::e2D) { - return DAWN_VALIDATION_ERROR("Texture must be 2D"); - } + DAWN_INVALID_IF(descriptor->dimension != wgpu::TextureDimension::e2D, + "Texture dimension (%s) is not %s.", descriptor->dimension, + wgpu::TextureDimension::e2D); - if (descriptor->mipLevelCount != 1) { - return DAWN_VALIDATION_ERROR("Mip level count must be 1"); - } + DAWN_INVALID_IF(descriptor->mipLevelCount != 1, "Mip level count (%u) is not 1.", + descriptor->mipLevelCount); - if (descriptor->size.depthOrArrayLayers != 1) { - return DAWN_VALIDATION_ERROR("Array layer count must be 1"); - } + DAWN_INVALID_IF(descriptor->size.depthOrArrayLayers != 1, + "Array layer count (%u) is not 1.", descriptor->size.depthOrArrayLayers); - if (descriptor->sampleCount != 1) { - return DAWN_VALIDATION_ERROR("Sample count must be 1"); - } + DAWN_INVALID_IF(descriptor->sampleCount != 1, "Sample count (%u) is not 1.", + descriptor->sampleCount); return {}; } @@ -667,9 +664,8 @@ namespace dawn_native { namespace vulkan { external_memory::Service* externalMemoryService) { VkFormat format = VulkanImageFormat(ToBackend(GetDevice()), GetFormat().format); VkImageUsageFlags usage = VulkanImageUsage(GetInternalUsage(), GetFormat()); - if (!externalMemoryService->SupportsCreateImage(descriptor, format, usage)) { - return DAWN_VALIDATION_ERROR("Creating an image from external memory is not supported"); - } + DAWN_INVALID_IF(!externalMemoryService->SupportsCreateImage(descriptor, format, usage), + "Creating an image from external memory is not supported."); mExternalState = ExternalState::PendingAcquire; @@ -731,14 +727,12 @@ namespace dawn_native { namespace vulkan { VkImageLayout* releasedNewLayout) { Device* device = ToBackend(GetDevice()); - if (mExternalState == ExternalState::Released) { - return DAWN_VALIDATION_ERROR("Can't export signal semaphore from signaled texture"); - } + DAWN_INVALID_IF(mExternalState == ExternalState::Released, + "Can't export a signal semaphore from signaled texture %s.", this); - if (mExternalAllocation == VK_NULL_HANDLE) { - return DAWN_VALIDATION_ERROR( - "Can't export signal semaphore from destroyed / non-external texture"); - } + DAWN_INVALID_IF( + mExternalAllocation == VK_NULL_HANDLE, + "Can't export a signal semaphore from destroyed or non-external texture %s.", this); ASSERT(mSignalSemaphore != VK_NULL_HANDLE); diff --git a/src/dawn_native/vulkan/external_memory/MemoryServiceDmaBuf.cpp b/src/dawn_native/vulkan/external_memory/MemoryServiceDmaBuf.cpp index d3611d9d8c..90b63734fa 100644 --- a/src/dawn_native/vulkan/external_memory/MemoryServiceDmaBuf.cpp +++ b/src/dawn_native/vulkan/external_memory/MemoryServiceDmaBuf.cpp @@ -55,7 +55,7 @@ namespace dawn_native { namespace vulkan { namespace external_memory { return count; } } - return DAWN_VALIDATION_ERROR("DRM format modifier not supported"); + return DAWN_FORMAT_VALIDATION_ERROR("DRM format modifier not supported."); } } // anonymous namespace @@ -154,9 +154,9 @@ namespace dawn_native { namespace vulkan { namespace external_memory { ResultOrError Service::GetMemoryImportParams( const ExternalImageDescriptor* descriptor, VkImage image) { - if (descriptor->type != ExternalImageType::DmaBuf) { - return DAWN_VALIDATION_ERROR("ExternalImageDescriptor is not a dma-buf descriptor"); - } + DAWN_INVALID_IF(descriptor->type != ExternalImageType::DmaBuf, + "ExternalImageDescriptor is not a ExternalImageDescriptorDmaBuf."); + const ExternalImageDescriptorDmaBuf* dmaBufDescriptor = static_cast(descriptor); VkDevice device = mDevice->GetVkDevice(); @@ -177,9 +177,9 @@ namespace dawn_native { namespace vulkan { namespace external_memory { memoryRequirements.memoryTypeBits &= fdProperties.memoryTypeBits; int memoryTypeIndex = mDevice->GetResourceMemoryAllocator()->FindBestTypeIndex( memoryRequirements, MemoryKind::Opaque); - if (memoryTypeIndex == -1) { - return DAWN_VALIDATION_ERROR("Unable to find appropriate memory type for import"); - } + DAWN_INVALID_IF(memoryTypeIndex == -1, + "Unable to find an appropriate memory type for import."); + MemoryImportParams params = {memoryRequirements.size, static_cast(memoryTypeIndex)}; return params; @@ -188,9 +188,7 @@ namespace dawn_native { namespace vulkan { namespace external_memory { ResultOrError Service::ImportMemory(ExternalMemoryHandle handle, const MemoryImportParams& importParams, VkImage image) { - if (handle < 0) { - return DAWN_VALIDATION_ERROR("Trying to import memory with invalid handle"); - } + DAWN_INVALID_IF(handle < 0, "Importing memory with an invalid handle."); VkMemoryDedicatedAllocateInfo memoryDedicatedAllocateInfo; memoryDedicatedAllocateInfo.sType = VK_STRUCTURE_TYPE_MEMORY_DEDICATED_ALLOCATE_INFO; @@ -220,9 +218,9 @@ namespace dawn_native { namespace vulkan { namespace external_memory { ResultOrError Service::CreateImage(const ExternalImageDescriptor* descriptor, const VkImageCreateInfo& baseCreateInfo) { - if (descriptor->type != ExternalImageType::DmaBuf) { - return DAWN_VALIDATION_ERROR("ExternalImageDescriptor is not a dma-buf descriptor"); - } + DAWN_INVALID_IF(descriptor->type != ExternalImageType::DmaBuf, + "ExternalImageDescriptor is not a dma-buf descriptor."); + const ExternalImageDescriptorDmaBuf* dmaBufDescriptor = static_cast(descriptor); VkPhysicalDevice physicalDevice = ToBackend(mDevice->GetAdapter())->GetPhysicalDevice(); diff --git a/src/dawn_native/vulkan/external_memory/MemoryServiceOpaqueFD.cpp b/src/dawn_native/vulkan/external_memory/MemoryServiceOpaqueFD.cpp index 70db38b7b7..ad2c4c23d5 100644 --- a/src/dawn_native/vulkan/external_memory/MemoryServiceOpaqueFD.cpp +++ b/src/dawn_native/vulkan/external_memory/MemoryServiceOpaqueFD.cpp @@ -88,9 +88,9 @@ namespace dawn_native { namespace vulkan { namespace external_memory { ResultOrError Service::GetMemoryImportParams( const ExternalImageDescriptor* descriptor, VkImage image) { - if (descriptor->type != ExternalImageType::OpaqueFD) { - return DAWN_VALIDATION_ERROR("ExternalImageDescriptor is not an OpaqueFD descriptor"); - } + DAWN_INVALID_IF(descriptor->type != ExternalImageType::OpaqueFD, + "ExternalImageDescriptor is not an OpaqueFD descriptor."); + const ExternalImageDescriptorOpaqueFD* opaqueFDDescriptor = static_cast(descriptor); @@ -102,15 +102,13 @@ namespace dawn_native { namespace vulkan { namespace external_memory { ResultOrError Service::ImportMemory(ExternalMemoryHandle handle, const MemoryImportParams& importParams, VkImage image) { - if (handle < 0) { - return DAWN_VALIDATION_ERROR("Trying to import memory with invalid handle"); - } + DAWN_INVALID_IF(handle < 0, "Importing memory with an invalid handle."); VkMemoryRequirements requirements; mDevice->fn.GetImageMemoryRequirements(mDevice->GetVkDevice(), image, &requirements); - if (requirements.size > importParams.allocationSize) { - return DAWN_VALIDATION_ERROR("Requested allocation size is too small for image"); - } + DAWN_INVALID_IF(requirements.size > importParams.allocationSize, + "Requested allocation size (%u) is smaller than the image requires (%u).", + importParams.allocationSize, requirements.size); VkImportMemoryFdInfoKHR importMemoryFdInfo; importMemoryFdInfo.sType = VK_STRUCTURE_TYPE_IMPORT_MEMORY_FD_INFO_KHR; diff --git a/src/dawn_native/vulkan/external_memory/MemoryServiceZirconHandle.cpp b/src/dawn_native/vulkan/external_memory/MemoryServiceZirconHandle.cpp index 6dbfcf99a5..c12bd85c96 100644 --- a/src/dawn_native/vulkan/external_memory/MemoryServiceZirconHandle.cpp +++ b/src/dawn_native/vulkan/external_memory/MemoryServiceZirconHandle.cpp @@ -88,9 +88,9 @@ namespace dawn_native { namespace vulkan { namespace external_memory { ResultOrError Service::GetMemoryImportParams( const ExternalImageDescriptor* descriptor, VkImage image) { - if (descriptor->type != ExternalImageType::OpaqueFD) { - return DAWN_VALIDATION_ERROR("ExternalImageDescriptor is not an OpaqueFD descriptor"); - } + DAWN_INVALID_IF(descriptor->type != ExternalImageType::OpaqueFD, + "ExternalImageDescriptor is not an OpaqueFD descriptor."); + const ExternalImageDescriptorOpaqueFD* opaqueFDDescriptor = static_cast(descriptor); @@ -102,15 +102,14 @@ namespace dawn_native { namespace vulkan { namespace external_memory { ResultOrError Service::ImportMemory(ExternalMemoryHandle handle, const MemoryImportParams& importParams, VkImage image) { - if (handle == ZX_HANDLE_INVALID) { - return DAWN_VALIDATION_ERROR("Trying to import memory with invalid handle"); - } + DAWN_INVALID_IF(handle == ZX_HANDLE_INVALID, "Importing memory with an invalid handle."); VkMemoryRequirements requirements; mDevice->fn.GetImageMemoryRequirements(mDevice->GetVkDevice(), image, &requirements); - if (requirements.size > importParams.allocationSize) { - return DAWN_VALIDATION_ERROR("Requested allocation size is too small for image"); - } + DAWN_INVALID_IF( + requirements.size > importParams.allocationSize, + "Requested allocation size (%u) is smaller than the required image size (%u).", + importParams.allocationSize, requirements.size); VkImportMemoryZirconHandleInfoFUCHSIA importMemoryHandleInfo; importMemoryHandleInfo.sType = diff --git a/src/dawn_native/vulkan/external_semaphore/SemaphoreServiceFD.cpp b/src/dawn_native/vulkan/external_semaphore/SemaphoreServiceFD.cpp index b25005645c..b81f96c33e 100644 --- a/src/dawn_native/vulkan/external_semaphore/SemaphoreServiceFD.cpp +++ b/src/dawn_native/vulkan/external_semaphore/SemaphoreServiceFD.cpp @@ -67,9 +67,7 @@ namespace dawn_native { namespace vulkan { namespace external_semaphore { } ResultOrError Service::ImportSemaphore(ExternalSemaphoreHandle handle) { - if (handle < 0) { - return DAWN_VALIDATION_ERROR("Trying to import semaphore with invalid handle"); - } + DAWN_INVALID_IF(handle < 0, "Importing a semaphore with an invalid handle."); VkSemaphore semaphore = VK_NULL_HANDLE; VkSemaphoreCreateInfo info; diff --git a/src/dawn_native/vulkan/external_semaphore/SemaphoreServiceZirconHandle.cpp b/src/dawn_native/vulkan/external_semaphore/SemaphoreServiceZirconHandle.cpp index 656b9fe835..d5bad9023e 100644 --- a/src/dawn_native/vulkan/external_semaphore/SemaphoreServiceZirconHandle.cpp +++ b/src/dawn_native/vulkan/external_semaphore/SemaphoreServiceZirconHandle.cpp @@ -60,9 +60,8 @@ namespace dawn_native { namespace vulkan { namespace external_semaphore { } ResultOrError Service::ImportSemaphore(ExternalSemaphoreHandle handle) { - if (handle == ZX_HANDLE_INVALID) { - return DAWN_VALIDATION_ERROR("Trying to import semaphore with invalid handle"); - } + DAWN_INVALID_IF(handle == ZX_HANDLE_INVALID, + "Importing a semaphore with an invalid handle."); VkSemaphore semaphore = VK_NULL_HANDLE; VkSemaphoreCreateInfo info;