From 0d5247be345493fb699e54dc0eaad41af35fa87b Mon Sep 17 00:00:00 2001 From: moudgils Date: Tue, 1 Jun 2021 09:58:45 -0700 Subject: [PATCH] Fix metal shader pipeline crashes for LuminanceHistogramGenerator and MorphTargetCS due to the use of atomic operations with typed buffers. Switching them to use Structured buffers. Plus misc cleanup --- .../Common/Assets/Shaders/MorphTargets/MorphTargetCS.shader | 4 +--- .../Common/Assets/Shaders/MorphTargets/MorphTargetSRG.azsli | 2 +- .../Assets/Shaders/PostProcessing/LuminanceHeatmap.azsl | 4 ++-- .../Shaders/PostProcessing/LuminanceHistogramGenerator.azsl | 2 +- .../PostProcessing/LuminanceHistogramGenerator.shader | 4 +--- .../Assets/Shaders/SkinnedMesh/LinearSkinningPassSRG.azsli | 2 +- .../PostProcessing/LuminanceHistogramGeneratorPass.cpp | 2 +- .../Source/SkinnedMesh/SkinnedMeshOutputStreamManager.cpp | 4 ++-- .../RHI/Code/Include/Atom/RHI/ShaderResourceGroupData.h | 1 - Gems/Atom/RHI/DX12/Code/Source/RHI/CommandList.h | 6 ++++++ 10 files changed, 16 insertions(+), 15 deletions(-) diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetCS.shader b/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetCS.shader index 95ffc36a11..08b1e7c298 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetCS.shader +++ b/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetCS.shader @@ -10,7 +10,5 @@ "type": "Compute" } ] - }, - "DisabledRHIBackends": ["metal"] - + } } diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetSRG.azsli b/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetSRG.azsli index 7ec5b43368..171e803c2c 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetSRG.azsli +++ b/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetSRG.azsli @@ -16,7 +16,7 @@ ShaderResourceGroup MorphTargetPassSrg : SRG_PerPass { - RWBuffer m_accumulatedDeltas; + RWStruturedBuffer m_accumulatedDeltas; } // This class represents the data that is passed to the morph target compute shader of an individual delta diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHeatmap.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHeatmap.azsl index df92e36f9a..4559b6c9ef 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHeatmap.azsl +++ b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHeatmap.azsl @@ -1,4 +1,4 @@ -/* + /* * All or portions of this file Copyright (c) Amazon.com, Inc. or its affiliates or * its licensors. * @@ -37,7 +37,7 @@ ShaderResourceGroup PassSrg : SRG_PerPass Texture2D m_sceneLuminance; // This should be of size NUM_HISTOGRAM_BINS. - Buffer m_histogram; + StructuredBuffer m_histogram; Sampler LinearSampler { diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.azsl index 05cc870eea..9d01a12fd6 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.azsl +++ b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.azsl @@ -20,7 +20,7 @@ ShaderResourceGroup PassSrg : SRG_PerPass { Texture2D m_inputTexture; - RWBuffer m_outputTexture; + RWStructuredBuffer m_outputTexture; } groupshared uint shared_histogramBins[NUM_HISTOGRAM_BINS]; diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.shader b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.shader index 566144bab8..f3dd11e11a 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.shader +++ b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.shader @@ -12,7 +12,5 @@ "type": "Compute" } ] - }, - "DisabledRHIBackends": ["metal"] - + } } diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/SkinnedMesh/LinearSkinningPassSRG.azsli b/Gems/Atom/Feature/Common/Assets/Shaders/SkinnedMesh/LinearSkinningPassSRG.azsli index 5a9e44bade..e5407d2d9e 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/SkinnedMesh/LinearSkinningPassSRG.azsli +++ b/Gems/Atom/Feature/Common/Assets/Shaders/SkinnedMesh/LinearSkinningPassSRG.azsli @@ -16,7 +16,7 @@ ShaderResourceGroup PassSrg : SRG_PerPass { - RWBuffer m_skinnedMeshOutputStream; + RWStructuredBuffer m_skinnedMeshOutputStream; } ShaderResourceGroup InstanceSrg : SRG_PerDraw diff --git a/Gems/Atom/Feature/Common/Code/Source/PostProcessing/LuminanceHistogramGeneratorPass.cpp b/Gems/Atom/Feature/Common/Code/Source/PostProcessing/LuminanceHistogramGeneratorPass.cpp index 758c21bc4e..a3c494dee9 100644 --- a/Gems/Atom/Feature/Common/Code/Source/PostProcessing/LuminanceHistogramGeneratorPass.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/PostProcessing/LuminanceHistogramGeneratorPass.cpp @@ -72,7 +72,7 @@ namespace AZ desc.m_bufferName = AZStd::string::format("LuminanceHistogramBuffer_%s", uuidString.c_str()); desc.m_elementSize = sizeof(uint32_t); desc.m_byteCount = NumHistogramBins * sizeof(uint32_t); - desc.m_elementFormat = RHI::Format::R32_UINT; + desc.m_elementFormat = RHI::Format::Unknown; m_histogram = RPI::BufferSystemInterface::Get()->CreateBufferFromCommonPool(desc); AZ_Assert(m_histogram != nullptr, "Unable to allocate buffer"); } diff --git a/Gems/Atom/Feature/Common/Code/Source/SkinnedMesh/SkinnedMeshOutputStreamManager.cpp b/Gems/Atom/Feature/Common/Code/Source/SkinnedMesh/SkinnedMeshOutputStreamManager.cpp index 1cb782d8b1..1b14611e84 100644 --- a/Gems/Atom/Feature/Common/Code/Source/SkinnedMesh/SkinnedMeshOutputStreamManager.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/SkinnedMesh/SkinnedMeshOutputStreamManager.cpp @@ -67,8 +67,8 @@ namespace AZ creator.SetBuffer(nullptr, 0, bufferDescriptor); RHI::BufferViewDescriptor viewDescriptor; - viewDescriptor.m_elementFormat = RHI::Format::R32_FLOAT; - viewDescriptor.m_elementSize = RHI::GetFormatSize(viewDescriptor.m_elementFormat); + viewDescriptor.m_elementFormat = RHI::Format::Unknown; + viewDescriptor.m_elementSize = sizeof(float); viewDescriptor.m_elementCount = aznumeric_cast(m_sizeInBytes) / viewDescriptor.m_elementSize; viewDescriptor.m_elementOffset = 0; creator.SetBufferViewDescriptor(viewDescriptor); diff --git a/Gems/Atom/RHI/Code/Include/Atom/RHI/ShaderResourceGroupData.h b/Gems/Atom/RHI/Code/Include/Atom/RHI/ShaderResourceGroupData.h index ce2c6c77ec..afcab28a1d 100644 --- a/Gems/Atom/RHI/Code/Include/Atom/RHI/ShaderResourceGroupData.h +++ b/Gems/Atom/RHI/Code/Include/Atom/RHI/ShaderResourceGroupData.h @@ -410,7 +410,6 @@ namespace AZ // For any other type the buffer view's element size should match the stride. if (shaderInputBuffer.m_strideSize != bufferViewDescriptor.m_elementSize) { - // [GFX TODO][ATOM-5735][AZSL] ByteAddressBuffer shader input is setting a stride of 16 instead of 4 AZ_Error("ShaderResourceGroupData", false, "Buffer Input '%s[%d]': Does not match expected stride size %d", shaderInputBuffer.m_name.GetCStr(), arrayIndex, bufferViewDescriptor.m_elementSize); return false; diff --git a/Gems/Atom/RHI/DX12/Code/Source/RHI/CommandList.h b/Gems/Atom/RHI/DX12/Code/Source/RHI/CommandList.h index a6fe57f6d8..6f09ea1d5f 100644 --- a/Gems/Atom/RHI/DX12/Code/Source/RHI/CommandList.h +++ b/Gems/Atom/RHI/DX12/Code/Source/RHI/CommandList.h @@ -271,6 +271,12 @@ namespace AZ ShaderResourceBindings& bindings = GetShaderResourceBindingsByPipelineType(pipelineType); const PipelineState* pipelineState = static_cast(item.m_pipelineState); + if(!pipelineState) + { + AZ_Assert(false, "Pipeline state not provided"); + return false; + } + bool updatePipelineState = m_state.m_pipelineState != pipelineState; // The pipeline state gets set first. if (updatePipelineState)