From b18b03b8fb8d1a79d14fead2475fea25d1c4b660 Mon Sep 17 00:00:00 2001 From: moudgils Date: Thu, 10 Jun 2021 14:40:52 -0700 Subject: [PATCH] Added comments and more format fixes --- .../Shaders/MorphTargets/MorphTargetSRG.azsli | 3 +++ .../LuminanceHistogramGenerator.azsl | 4 +++ .../RHI/DX12/Code/Source/RHI/Conversions.cpp | 10 +++++++ .../Metal/Code/Source/RHI/ArgumentBuffer.cpp | 26 +++++++++---------- .../Metal/Code/Source/RHI/ArgumentBuffer.h | 13 +++++++--- .../RHI/Metal/Code/Source/RHI/CommandList.h | 6 ++++- .../RHI/Metal/Code/Source/RHI/Conversions.cpp | 10 +++++++ .../RHI/Vulkan/Code/Source/RHI/Conversion.cpp | 6 +++++ 8 files changed, 60 insertions(+), 18 deletions(-) diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetSRG.azsli b/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetSRG.azsli index 83193c8559..f4b1beeb3e 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetSRG.azsli +++ b/Gems/Atom/Feature/Common/Assets/Shaders/MorphTargets/MorphTargetSRG.azsli @@ -16,6 +16,9 @@ ShaderResourceGroup MorphTargetPassSrg : SRG_PerPass { + //Since we do Interlocked atomic operations on this buffer it can not be RWBuffer due to broken MetalSL generation. + //It stems from the fact that typed buffers gets converted to textures and that breaks with atomic operations. + //In future we can handle this under the hood via our metal shader pipeline RWStructuredBuffer m_accumulatedDeltas; } diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.azsl index 9d01a12fd6..ee95515a2a 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.azsl +++ b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/LuminanceHistogramGenerator.azsl @@ -20,6 +20,10 @@ ShaderResourceGroup PassSrg : SRG_PerPass { Texture2D m_inputTexture; + + //Since we do Interlocked atomic operations on this buffer it can not be RWBuffer due to broken MetalSL generation. + //It stems from the fact that typed buffers gets converted to textures and that breaks with atomic operations. + //In future we can handle this under the hood via our metal shader pipeline RWStructuredBuffer m_outputTexture; } diff --git a/Gems/Atom/RHI/DX12/Code/Source/RHI/Conversions.cpp b/Gems/Atom/RHI/DX12/Code/Source/RHI/Conversions.cpp index 73d80ed78c..9776734158 100644 --- a/Gems/Atom/RHI/DX12/Code/Source/RHI/Conversions.cpp +++ b/Gems/Atom/RHI/DX12/Code/Source/RHI/Conversions.cpp @@ -1360,6 +1360,16 @@ namespace AZ uint32_t ConvertColorWriteMask(uint8_t writeMask) { uint32_t dflags = 0; + if(writeMask == 0) + { + return dflags; + } + + if(RHI::CheckBitsAll(writeMask, static_cast(RHI::WriteChannelMask::ColorWriteMaskAll))) + { + return D3D12_COLOR_WRITE_ENABLE_ALL; + } + if (RHI::CheckBitsAny(writeMask, static_cast(RHI::WriteChannelMask::ColorWriteMaskRed))) { dflags |= D3D12_COLOR_WRITE_ENABLE_RED; diff --git a/Gems/Atom/RHI/Metal/Code/Source/RHI/ArgumentBuffer.cpp b/Gems/Atom/RHI/Metal/Code/Source/RHI/ArgumentBuffer.cpp index 680481d417..49ff4146fc 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/RHI/ArgumentBuffer.cpp +++ b/Gems/Atom/RHI/Metal/Code/Source/RHI/ArgumentBuffer.cpp @@ -397,19 +397,18 @@ namespace AZ uint8_t numBitsSet = RHI::CountBitsSet(static_cast(srgResourcesVisInfo.m_constantDataStageMask)); if( numBitsSet > 0) { + id mtlconstantBufferResource = m_constantBuffer.GetGpuAddress>(); if(RHI::CheckBitsAny(srgResourcesVisInfo.m_constantDataStageMask, RHI::ShaderStageMask::Compute)) { - uint16_t arrayIndex = resourcesToMakeResidentCompute[MTLResourceUsageRead].m_resourceArrayLen; - resourcesToMakeResidentCompute[MTLResourceUsageRead].m_resourceArray[arrayIndex] = m_constantBuffer.GetGpuAddress>(); - resourcesToMakeResidentCompute[MTLResourceUsageRead].m_resourceArrayLen++; + uint16_t arrayIndex = resourcesToMakeResidentCompute[MTLResourceUsageRead].m_resourceArrayLen++; + resourcesToMakeResidentCompute[MTLResourceUsageRead].m_resourceArray[arrayIndex] = mtlconstantBufferResource; } else { MTLRenderStages mtlRenderStages = GetRenderStages(srgResourcesVisInfo.m_constantDataStageMask); AZStd::pair key = AZStd::make_pair(MTLResourceUsageRead, mtlRenderStages); - uint16_t arrayIndex = resourcesToMakeResidentGraphics[key].m_resourceArrayLen; - resourcesToMakeResidentGraphics[key].m_resourceArray[arrayIndex] = m_constantBuffer.GetGpuAddress>(); - resourcesToMakeResidentGraphics[key].m_resourceArrayLen++; + uint16_t arrayIndex = resourcesToMakeResidentGraphics[key].m_resourceArrayLen++; + resourcesToMakeResidentGraphics[key].m_resourceArray[arrayIndex] = mtlconstantBufferResource; } } } @@ -431,7 +430,8 @@ namespace AZ } else { - AZ_Assert(RHI::CheckBitsAny(visMaskIt->second, RHI::ShaderStageMask::Vertex) || RHI::CheckBitsAny(visMaskIt->second, RHI::ShaderStageMask::Fragment), "The visibility mask %i is not set for Vertex or fragment stage", visMaskIt->second); + bool isBoundToGraphics = RHI::CheckBitsAny(visMaskIt->second, RHI::ShaderStageMask::Vertex) || RHI::CheckBitsAny(visMaskIt->second, RHI::ShaderStageMask::Fragment); + AZ_Assert(isBoundToGraphics, "The visibility mask %i is not set for Vertex or fragment stage", visMaskIt->second); CollectResourcesForGraphics(commandEncoder, visMaskIt->second, it.second, resourcesToMakeResidentGraphics); } } @@ -480,9 +480,9 @@ namespace AZ AZ_Assert(false, "Undefined Resource type"); } } - uint16_t arrayIndex = resourcesToMakeResidentMap[resourceUsage].m_resourceArrayLen; - resourcesToMakeResidentMap[resourceUsage].m_resourceArray[arrayIndex] = resourceBindingData.m_resourcPtr->GetGpuAddress>(); - resourcesToMakeResidentMap[resourceUsage].m_resourceArrayLen++; + uint16_t arrayIndex = resourcesToMakeResidentMap[resourceUsage].m_resourceArrayLen++; + id mtlResourceToBind = resourceBindingData.m_resourcPtr->GetGpuAddress>(); + resourcesToMakeResidentMap[resourceUsage].m_resourceArray[arrayIndex] = mtlResourceToBind; } } @@ -516,9 +516,9 @@ namespace AZ } AZStd::pair key = AZStd::make_pair(resourceUsage, mtlRenderStages); - uint16_t arrayIndex = resourcesToMakeResidentMap[key].m_resourceArrayLen; - resourcesToMakeResidentMap[key].m_resourceArray[arrayIndex] = resourceBindingData.m_resourcPtr->GetGpuAddress>(); - resourcesToMakeResidentMap[key].m_resourceArrayLen++; + uint16_t arrayIndex = resourcesToMakeResidentMap[key].m_resourceArrayLen++; + id mtlResourceToBind = resourceBindingData.m_resourcPtr->GetGpuAddress>(); + resourcesToMakeResidentMap[key].m_resourceArray[arrayIndex] = mtlResourceToBind; } } } diff --git a/Gems/Atom/RHI/Metal/Code/Source/RHI/ArgumentBuffer.h b/Gems/Atom/RHI/Metal/Code/Source/RHI/ArgumentBuffer.h index 0825c07654..c4cfd17390 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/RHI/ArgumentBuffer.h +++ b/Gems/Atom/RHI/Metal/Code/Source/RHI/ArgumentBuffer.h @@ -126,12 +126,17 @@ namespace AZ uint16_t m_resourceArrayLen = 0; }; //Map to cache all the resources based on the usage as we can batch all the resources for a given usage - using ComputeResourcesToMakeResidentMap = AZStd::unordered_map; + using ComputeResourcesToMakeResidentMap = AZStd::unordered_map; //Map to cache all the resources based on the usage and shader stage as we can batch all the resources for a given usage/shader usage - using GraphicsResourcesToMakeResidentMap = AZStd::unordered_map, MetalResourceArray>; + using GraphicsResourcesToMakeResidentMap = AZStd::unordered_map, MetalResourceArray>; - void CollectResourcesForCompute(id encoder, const ResourceBindingsSet& resourceBindingData, ComputeResourcesToMakeResidentMap& resourcesToMakeResidentMap) const; - void CollectResourcesForGraphics(id encoder, RHI::ShaderStageMask visShaderMask, const ResourceBindingsSet& resourceBindingDataSet, GraphicsResourcesToMakeResidentMap& resourcesToMakeResidentMap) const; + void CollectResourcesForCompute(id encoder, + const ResourceBindingsSet& resourceBindingData, + ComputeResourcesToMakeResidentMap& resourcesToMakeResidentMap) const; + void CollectResourcesForGraphics(id encoder, + RHI::ShaderStageMask visShaderMask, + const ResourceBindingsSet& resourceBindingDataSet, + GraphicsResourcesToMakeResidentMap& resourcesToMakeResidentMap) const; //! Use visibility information to call UseResource on all resources for this Argument Buffer void ApplyUseResource(id encoder, const ResourceBindingsMap& resourceMap, diff --git a/Gems/Atom/RHI/Metal/Code/Source/RHI/CommandList.h b/Gems/Atom/RHI/Metal/Code/Source/RHI/CommandList.h index 9dd14cd175..8674124cc2 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/RHI/CommandList.h +++ b/Gems/Atom/RHI/Metal/Code/Source/RHI/CommandList.h @@ -104,7 +104,11 @@ namespace AZ using MetalArgumentBufferArray = AZStd::array, RHI::Limits::Pipeline::ShaderResourceGroupCountMax>; using MetalArgumentBufferArrayOffsets = AZStd::array; - void BindArgumentBuffers(RHI::ShaderStage shaderStage, uint16_t registerIdMin, uint16_t registerIdMax, MetalArgumentBufferArray& mtlArgBuffers, MetalArgumentBufferArrayOffsets mtlArgBufferOffsets); + void BindArgumentBuffers(RHI::ShaderStage shaderStage, + uint16_t registerIdMin, + uint16_t registerIdMax, + MetalArgumentBufferArray& mtlArgBuffers, + MetalArgumentBufferArrayOffsets mtlArgBufferOffsets); ShaderResourceBindings& GetShaderResourceBindingsByPipelineType(RHI::PipelineStateType pipelineType); diff --git a/Gems/Atom/RHI/Metal/Code/Source/RHI/Conversions.cpp b/Gems/Atom/RHI/Metal/Code/Source/RHI/Conversions.cpp index e41ba9bf2a..81e589e62e 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/RHI/Conversions.cpp +++ b/Gems/Atom/RHI/Metal/Code/Source/RHI/Conversions.cpp @@ -458,6 +458,16 @@ namespace AZ MTLColorWriteMask ConvertColorWriteMask(AZ::u8 writeMask) { MTLColorWriteMask colorMask = MTLColorWriteMaskNone; + if(writeMask == 0) + { + return colorMask; + } + + if(RHI::CheckBitsAll(writeMask, static_cast(RHI::WriteChannelMask::ColorWriteMaskAll))) + { + return MTLColorWriteMaskAll; + } + if (RHI::CheckBitsAny(writeMask, static_cast(RHI::WriteChannelMask::ColorWriteMaskRed))) { colorMask |= MTLColorWriteMaskRed; diff --git a/Gems/Atom/RHI/Vulkan/Code/Source/RHI/Conversion.cpp b/Gems/Atom/RHI/Vulkan/Code/Source/RHI/Conversion.cpp index 7945dd2894..5ce9731506 100644 --- a/Gems/Atom/RHI/Vulkan/Code/Source/RHI/Conversion.cpp +++ b/Gems/Atom/RHI/Vulkan/Code/Source/RHI/Conversion.cpp @@ -334,6 +334,12 @@ namespace AZ VkColorComponentFlags ConvertComponentFlags(uint8_t sflags) { VkColorComponentFlags dflags = 0; + + if(sflags == 0) + { + return dflags; + } + if (RHI::CheckBitsAny(sflags, static_cast(RHI::WriteChannelMask::ColorWriteMaskRed))) { dflags |= VK_COLOR_COMPONENT_R_BIT;