From 1d553062552b08fd132011fda5a8aa4e5bd0257c Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Thu, 21 Oct 2021 20:21:54 +0000 Subject: [PATCH] Improving D3D12 backend validation messages. Improves validation messages in various D3D12 backend files: - CommandBufferD3D12.cpp - ShaderModuleD3D12.cpp - SwapChainD3D12.cpp - TextureD3D12.cpp Bug: dawn:563 Change-Id: I00607012f5bec81780c419993fc32dc0984dad27 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/67143 Commit-Queue: Brandon Jones Commit-Queue: Austin Eng Auto-Submit: Brandon Jones Reviewed-by: Austin Eng --- src/dawn_native/d3d12/CommandBufferD3D12.cpp | 9 ++- src/dawn_native/d3d12/D3D12Backend.cpp | 4 +- src/dawn_native/d3d12/ShaderModuleD3D12.cpp | 37 +++++------ src/dawn_native/d3d12/SwapChainD3D12.cpp | 11 ++-- src/dawn_native/d3d12/TextureD3D12.cpp | 68 ++++++++++---------- 5 files changed, 60 insertions(+), 69 deletions(-) diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp index 702f683d61..e7d12a51a8 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp +++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp @@ -1068,11 +1068,10 @@ namespace dawn_native { namespace d3d12 { DispatchIndirectCmd* dispatch = mCommands.NextCommand(); // TODO(dawn:839): support [[num_workgroups]] for DispatchIndirect calls - if (lastPipeline->UsesNumWorkgroups()) { - return DAWN_VALIDATION_ERROR( - "Using a compute pipeline with [[num_workgroups]] in a " - "DispatchIndirect call is not implemented"); - } + DAWN_INVALID_IF(lastPipeline->UsesNumWorkgroups(), + "Using %s with [[num_workgroups]] in a DispatchIndirect call " + "is not implemented.", + lastPipeline); Buffer* buffer = ToBackend(dispatch->indirectBuffer.Get()); diff --git a/src/dawn_native/d3d12/D3D12Backend.cpp b/src/dawn_native/d3d12/D3D12Backend.cpp index bc9df608c6..52c5099fab 100644 --- a/src/dawn_native/d3d12/D3D12Backend.cpp +++ b/src/dawn_native/d3d12/D3D12Backend.cpp @@ -138,7 +138,9 @@ namespace dawn_native { namespace d3d12 { } if (backendDevice->ConsumedError( - ValidateTextureDescriptorCanBeWrapped(textureDescriptor))) { + ValidateTextureDescriptorCanBeWrapped(textureDescriptor), + "validating that a D3D12 external image can be wrapped with %s", + textureDescriptor)) { return nullptr; } diff --git a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp index 89b5825977..34ea0d50aa 100644 --- a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp +++ b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp @@ -341,9 +341,8 @@ namespace dawn_native { namespace d3d12 { ComPtr errors; DAWN_TRY(CheckHRESULT(result->GetErrorBuffer(&errors), "DXC get error buffer")); - std::string message = std::string("DXC compile failed with ") + - static_cast(errors->GetBufferPointer()); - return DAWN_VALIDATION_ERROR(message); + return DAWN_FORMAT_VALIDATION_ERROR("DXC compile failed with: %s", + static_cast(errors->GetBufferPointer())); } ComPtr compiledShader; @@ -370,14 +369,12 @@ namespace dawn_native { namespace d3d12 { ComPtr compiledShader; ComPtr errors; - if (FAILED(functions->d3dCompile(hlslSource.c_str(), hlslSource.length(), nullptr, - nullptr, nullptr, request.entryPointName, - targetProfile, request.compileFlags, 0, - &compiledShader, &errors))) { - std::string message = std::string("D3D compile failed with ") + - static_cast(errors->GetBufferPointer()); - return DAWN_VALIDATION_ERROR(message); - } + DAWN_INVALID_IF(FAILED(functions->d3dCompile( + hlslSource.c_str(), hlslSource.length(), nullptr, nullptr, nullptr, + request.entryPointName, targetProfile, request.compileFlags, 0, + &compiledShader, &errors)), + "D3D compile failed with: %s", + static_cast(errors->GetBufferPointer())); return std::move(compiledShader); } @@ -421,15 +418,13 @@ namespace dawn_native { namespace d3d12 { if (it != data->remappings.end()) { *remappedEntryPointName = it->second; } else { - if (request.disableSymbolRenaming) { - *remappedEntryPointName = request.entryPointName; - } else { - return DAWN_VALIDATION_ERROR( - "Could not find remapped name for entry point."); - } + DAWN_INVALID_IF(!request.disableSymbolRenaming, + "Could not find remapped name for entry point."); + + *remappedEntryPointName = request.entryPointName; } } else { - return DAWN_VALIDATION_ERROR("Transform output missing renamer data."); + return DAWN_FORMAT_VALIDATION_ERROR("Transform output missing renamer data."); } tint::writer::hlsl::Options options; @@ -439,10 +434,8 @@ namespace dawn_native { namespace d3d12 { options.root_constant_binding_point.binding = request.numWorkgroupsShaderRegister; } auto result = tint::writer::hlsl::Generate(&transformedProgram, 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 HLSL: %s", + result.error); return std::move(result.hlsl); } diff --git a/src/dawn_native/d3d12/SwapChainD3D12.cpp b/src/dawn_native/d3d12/SwapChainD3D12.cpp index 4554bccaa6..efc0a02187 100644 --- a/src/dawn_native/d3d12/SwapChainD3D12.cpp +++ b/src/dawn_native/d3d12/SwapChainD3D12.cpp @@ -166,9 +166,9 @@ namespace dawn_native { namespace d3d12 { // 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::D3D12) { - return DAWN_VALIDATION_ERROR("d3d12::SwapChain cannot switch between APIs"); - } + DAWN_INVALID_IF(previousSwapChain->GetBackendType() != wgpu::BackendType::D3D12, + "D3D12 SwapChain cannot switch backend types from %s to %s.", + previousSwapChain->GetBackendType(), wgpu::BackendType::D3D12); // TODO(crbug.com/dawn/269): use ToBackend once OldSwapChainBase is removed. SwapChain* previousD3D12SwapChain = static_cast(previousSwapChain); @@ -176,9 +176,8 @@ namespace dawn_native { namespace d3d12 { // TODO(crbug.com/dawn/269): Figure out switching an HWND between devices, it might // require just losing the reference to the swapchain, but might also need to wait for // all previous operations to complete. - if (GetDevice() != previousSwapChain->GetDevice()) { - return DAWN_VALIDATION_ERROR("d3d12::SwapChain cannot switch between devices"); - } + DAWN_INVALID_IF(GetDevice() != previousSwapChain->GetDevice(), + "D3D12 SwapChain cannot switch between D3D Devices"); // The previous swapchain is on the same device so we want to reuse it but it is still not // always possible. Because DXGI requires that a new swapchain be created if the diff --git a/src/dawn_native/d3d12/TextureD3D12.cpp b/src/dawn_native/d3d12/TextureD3D12.cpp index 81c4ba05ad..59f9424f03 100644 --- a/src/dawn_native/d3d12/TextureD3D12.cpp +++ b/src/dawn_native/d3d12/TextureD3D12.cpp @@ -423,21 +423,18 @@ namespace dawn_native { namespace d3d12 { } MaybeError ValidateTextureDescriptorCanBeWrapped(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("Depth 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 {}; } @@ -445,25 +442,27 @@ namespace dawn_native { namespace d3d12 { MaybeError ValidateD3D12TextureCanBeWrapped(ID3D12Resource* d3d12Resource, const TextureDescriptor* dawnDescriptor) { const D3D12_RESOURCE_DESC d3dDescriptor = d3d12Resource->GetDesc(); - if ((dawnDescriptor->size.width != d3dDescriptor.Width) || - (dawnDescriptor->size.height != d3dDescriptor.Height) || - (dawnDescriptor->size.depthOrArrayLayers != 1)) { - return DAWN_VALIDATION_ERROR("D3D12 texture size doesn't match descriptor"); - } + DAWN_INVALID_IF( + (dawnDescriptor->size.width != d3dDescriptor.Width) || + (dawnDescriptor->size.height != d3dDescriptor.Height) || + (dawnDescriptor->size.depthOrArrayLayers != 1), + "D3D12 texture size (Width: %u, Height: %u, DepthOrArraySize: 1) doesn't match Dawn " + "descriptor size (width: %u, height: %u, depthOrArrayLayers: %u).", + d3dDescriptor.Width, d3dDescriptor.Height, dawnDescriptor->size.width, + dawnDescriptor->size.height, dawnDescriptor->size.depthOrArrayLayers); const DXGI_FORMAT dxgiFormatFromDescriptor = D3D12TextureFormat(dawnDescriptor->format); - if (dxgiFormatFromDescriptor != d3dDescriptor.Format) { - return DAWN_VALIDATION_ERROR( - "D3D12 texture format must be compatible with descriptor format."); - } + DAWN_INVALID_IF( + dxgiFormatFromDescriptor != d3dDescriptor.Format, + "D3D12 texture format (%x) is not compatible with Dawn descriptor format (%s).", + d3dDescriptor.Format, dawnDescriptor->format); - if (d3dDescriptor.MipLevels != 1) { - return DAWN_VALIDATION_ERROR("D3D12 texture number of miplevels must be 1."); - } + DAWN_INVALID_IF(d3dDescriptor.MipLevels != 1, + "D3D12 texture number of miplevels (%u) is not 1.", + d3dDescriptor.MipLevels); - if (d3dDescriptor.DepthOrArraySize != 1) { - return DAWN_VALIDATION_ERROR("D3D12 texture array size must be 1."); - } + DAWN_INVALID_IF(d3dDescriptor.DepthOrArraySize != 1, + "D3D12 texture array size (%u) is not 1.", d3dDescriptor.DepthOrArraySize); // Shared textures cannot be multi-sample so no need to check those. ASSERT(d3dDescriptor.SampleDesc.Count == 1); @@ -487,7 +486,7 @@ namespace dawn_native { namespace d3d12 { break; } - return DAWN_VALIDATION_ERROR("DXGI format does not support cross-API sharing."); + return DAWN_FORMAT_VALIDATION_ERROR("DXGI format does not support cross-API sharing."); } // static @@ -496,9 +495,8 @@ namespace dawn_native { namespace d3d12 { Ref dawnTexture = AcquireRef(new Texture(device, descriptor, TextureState::OwnedInternal)); - if (dawnTexture->GetFormat().IsMultiPlanar()) { - return DAWN_VALIDATION_ERROR("Cannot create a multi-planar formatted texture directly"); - } + DAWN_INVALID_IF(dawnTexture->GetFormat().IsMultiPlanar(), + "Cannot create a multi-planar formatted texture directly"); DAWN_TRY(dawnTexture->InitializeAsInternalTexture()); return std::move(dawnTexture); @@ -522,10 +520,10 @@ namespace dawn_native { namespace d3d12 { // Importing a multi-planar format must be initialized. This is required because // a shared multi-planar format cannot be initialized by Dawn. - if (!isInitialized && dawnTexture->GetFormat().IsMultiPlanar()) { - return DAWN_VALIDATION_ERROR( - "Cannot create a multi-planar formatted texture without being initialized"); - } + DAWN_INVALID_IF( + !isInitialized && dawnTexture->GetFormat().IsMultiPlanar(), + "Cannot create a texture with a multi-planar format (%s) with uninitialized data.", + dawnTexture->GetFormat().format); dawnTexture->SetIsSubresourceContentInitialized(isInitialized, dawnTexture->GetAllSubresources());