From 7780b83fdc54bf0d1dafbb1de682e841ce1ffb4c Mon Sep 17 00:00:00 2001 From: moudgils <47460854+moudgils@users.noreply.github.com> Date: Mon, 28 Jun 2021 17:57:06 -0700 Subject: [PATCH] Fix SSR Reflections on Mac amongst other things (#1618) Fix writing to the correct mip in Metal Set default m_outputScale to 1 in order to fix a 1/0 error Add support for logging/printing errors pertaining to GPU crashes. Setting Release queue's collection latency to MaxFrames. Fix managed mem synchronization related offset bug --- .../ReflectionScreenSpaceBlurChildPass.h | 2 +- .../Source/Platform/Mac/RHI/Metal_RHI_Mac.cpp | 5 ++- .../Atom/RHI/Metal/Code/Source/RHI/Buffer.cpp | 10 ++++++ Gems/Atom/RHI/Metal/Code/Source/RHI/Buffer.h | 6 ++++ .../Source/RHI/CommandQueueCommandBuffer.cpp | 32 +++++++++++++++++-- .../Atom/RHI/Metal/Code/Source/RHI/Device.cpp | 2 +- Gems/Atom/RHI/Metal/Code/Source/RHI/Scope.cpp | 1 + 7 files changed, 53 insertions(+), 5 deletions(-) diff --git a/Gems/Atom/Feature/Common/Code/Source/ReflectionScreenSpace/ReflectionScreenSpaceBlurChildPass.h b/Gems/Atom/Feature/Common/Code/Source/ReflectionScreenSpace/ReflectionScreenSpaceBlurChildPass.h index 9bbe9f3a2a..a579aa0a16 100644 --- a/Gems/Atom/Feature/Common/Code/Source/ReflectionScreenSpace/ReflectionScreenSpaceBlurChildPass.h +++ b/Gems/Atom/Feature/Common/Code/Source/ReflectionScreenSpace/ReflectionScreenSpaceBlurChildPass.h @@ -51,7 +51,7 @@ namespace AZ PassType m_passType; uint32_t m_mipLevel = 0; RHI::Size m_imageSize; - float m_outputScale = 0.0f; + float m_outputScale = 1.0f; }; } // namespace RPI } // namespace AZ diff --git a/Gems/Atom/RHI/Metal/Code/Source/Platform/Mac/RHI/Metal_RHI_Mac.cpp b/Gems/Atom/RHI/Metal/Code/Source/Platform/Mac/RHI/Metal_RHI_Mac.cpp index 35ce1ce931..99a3c427bb 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/Platform/Mac/RHI/Metal_RHI_Mac.cpp +++ b/Gems/Atom/RHI/Metal/Code/Source/Platform/Mac/RHI/Metal_RHI_Mac.cpp @@ -144,6 +144,7 @@ namespace Platform } mappedData += request.m_byteOffset; response.m_data = mappedData; + buffer.SetMapRequestOffset(request.m_byteOffset); break; } default: @@ -159,6 +160,8 @@ namespace Platform { AZ::Metal::Buffer& buffer = static_cast(bufferBase); //Ony need to handle MTLStorageModeManaged memory. - SynchronizeBufferOnCPU(buffer.GetMemoryView().GetGpuAddress>(), buffer.GetMemoryView().GetOffset(), buffer.GetMemoryView().GetSize()); + SynchronizeBufferOnCPU(buffer.GetMemoryView().GetGpuAddress>(), + buffer.GetMemoryView().GetOffset() + buffer.GetMapRequestOffset(), + buffer.GetMemoryView().GetSize()); } } diff --git a/Gems/Atom/RHI/Metal/Code/Source/RHI/Buffer.cpp b/Gems/Atom/RHI/Metal/Code/Source/RHI/Buffer.cpp index e655b859cb..e634ae39bb 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/RHI/Buffer.cpp +++ b/Gems/Atom/RHI/Metal/Code/Source/RHI/Buffer.cpp @@ -35,6 +35,16 @@ namespace AZ } } + void Buffer::SetMapRequestOffset(const uint32_t mapRequestOffset) + { + m_mapRequestOffset = mapRequestOffset; + } + + const uint32_t Buffer::GetMapRequestOffset() const + { + return m_mapRequestOffset; + } + void Buffer::ReportMemoryUsage(RHI::MemoryStatisticsBuilder& builder) const { //[GFX TODO][ATOM-493] - Report memory usage support diff --git a/Gems/Atom/RHI/Metal/Code/Source/RHI/Buffer.h b/Gems/Atom/RHI/Metal/Code/Source/RHI/Buffer.h index 9e9971b73b..e31aa4b33d 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/RHI/Buffer.h +++ b/Gems/Atom/RHI/Metal/Code/Source/RHI/Buffer.h @@ -32,6 +32,9 @@ namespace AZ const MemoryView& GetMemoryView() const; MemoryView& GetMemoryView(); + void SetMapRequestOffset(const uint32_t mapRequestOffset); + const uint32_t GetMapRequestOffset() const; + private: Buffer() = default; friend class BufferPool; @@ -57,6 +60,9 @@ namespace AZ // The number of resolve operations pending for this buffer. AZStd::atomic m_pendingResolves = 0; + + // Offset related to the Map request. We need to cache it for cpu/gpu synchronization. + uint32_t m_mapRequestOffset = 0; }; } diff --git a/Gems/Atom/RHI/Metal/Code/Source/RHI/CommandQueueCommandBuffer.cpp b/Gems/Atom/RHI/Metal/Code/Source/RHI/CommandQueueCommandBuffer.cpp index ab593105e0..df5c14a337 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/RHI/CommandQueueCommandBuffer.cpp +++ b/Gems/Atom/RHI/Metal/Code/Source/RHI/CommandQueueCommandBuffer.cpp @@ -5,6 +5,7 @@ * */ +#include #include #include @@ -24,8 +25,35 @@ namespace AZ id CommandQueueCommandBuffer::AcquireMTLCommandBuffer() { AZ_Assert(m_mtlCommandBuffer==nil, "Previous command buffer was not commited"); + //Create a new command buffer - m_mtlCommandBuffer = [m_hwQueue commandBuffer]; +#if defined(__IPHONE_14_0) || defined(__MAC_11_0) + if(@available(iOS 14.0, macOS 11.0, *)) + { + if(RHI::BuildOptions::IsDebugBuild) + { + //There is a perf cost associated with enhanced command buffer errors so only enabling them for debug builds. + MTLCommandBufferDescriptor* mtlCommandBufferDesc = [[MTLCommandBufferDescriptor alloc] init]; + mtlCommandBufferDesc.errorOptions = MTLCommandBufferErrorOptionEncoderExecutionStatus; + m_mtlCommandBuffer = [m_hwQueue commandBufferWithDescriptor:mtlCommandBufferDesc]; + + [m_mtlCommandBuffer addCompletedHandler:^(id buffer) + { + // check command buffer's status for errors, print out all of its contents + MTLCommandBufferStatus stat = buffer.status; + if (stat == MTLCommandBufferStatusError) + { + NSLog(@"%@",buffer.error); + abort(); + } + }]; + } + } +#endif + if(m_mtlCommandBuffer == nil) + { + m_mtlCommandBuffer = [m_hwQueue commandBuffer]; + } //we call retain here as this CB is active across the autoreleasepools of multiple threads. Calling //retain here means that if the current thread's autoreleasepool gets drained this CB will not die. @@ -91,7 +119,7 @@ namespace AZ AZ_Assert(false,"Insufficient memory"); break; case MTLCommandBufferErrorInvalidResource: - AZ_Assert(false,"The command buffer referenced an invlid resource. This error is most commonly caused when caller deletes a resource before executing a command buffer that refers to it"); + AZ_Assert(false,"This error is most commonly caused when the caller deletes a resource before executing a command buffer that refers to it. It would also trigger if the caller deletes the resource while the GPU is working on the command buffer"); break; default: break; diff --git a/Gems/Atom/RHI/Metal/Code/Source/RHI/Device.cpp b/Gems/Atom/RHI/Metal/Code/Source/RHI/Device.cpp index 387ebd6f27..6797ccd141 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/RHI/Device.cpp +++ b/Gems/Atom/RHI/Metal/Code/Source/RHI/Device.cpp @@ -46,7 +46,7 @@ namespace AZ { { ReleaseQueue::Descriptor releaseQueueDescriptor; - releaseQueueDescriptor.m_collectLatency = descriptor.m_frameCountMax - 1; + releaseQueueDescriptor.m_collectLatency = descriptor.m_frameCountMax; m_releaseQueue.Init(releaseQueueDescriptor); } diff --git a/Gems/Atom/RHI/Metal/Code/Source/RHI/Scope.cpp b/Gems/Atom/RHI/Metal/Code/Source/RHI/Scope.cpp index 00e5ed979b..d176078563 100644 --- a/Gems/Atom/RHI/Metal/Code/Source/RHI/Scope.cpp +++ b/Gems/Atom/RHI/Metal/Code/Source/RHI/Scope.cpp @@ -188,6 +188,7 @@ namespace AZ { m_renderPassDescriptor.colorAttachments[colorAttachmentIndex].slice = imgViewDescriptor.m_arraySliceMin; } + m_renderPassDescriptor.colorAttachments[colorAttachmentIndex].level = imgViewDescriptor.m_mipSliceMin; } MTLRenderPassColorAttachmentDescriptor* colorAttachment = m_renderPassDescriptor.colorAttachments[colorAttachmentIndex];