From b5268e0e2c1b9764a8c4e8a59b981a41659f4798 Mon Sep 17 00:00:00 2001 From: Jon Creighton Date: Thu, 12 May 2022 11:49:00 +0100 Subject: [PATCH] Remove excess parameters, change vega fix to member, support caps in pso --- pxr/imaging/hdSt/pipelineDrawBatch.cpp | 17 ++--- pxr/imaging/hdSt/pipelineDrawBatch.h | 2 - pxr/imaging/hgiMetal/graphicsCmds.h | 1 + pxr/imaging/hgiMetal/graphicsCmds.mm | 92 ++++++++++++------------ pxr/imaging/hgiMetal/graphicsPipeline.h | 7 +- pxr/imaging/hgiMetal/graphicsPipeline.mm | 14 ++-- 6 files changed, 65 insertions(+), 68 deletions(-) diff --git a/pxr/imaging/hdSt/pipelineDrawBatch.cpp b/pxr/imaging/hdSt/pipelineDrawBatch.cpp index 9643fe8253..ef406f265e 100644 --- a/pxr/imaging/hdSt/pipelineDrawBatch.cpp +++ b/pxr/imaging/hdSt/pipelineDrawBatch.cpp @@ -47,7 +47,6 @@ #include "pxr/imaging/hgi/blitCmds.h" #include "pxr/imaging/hgi/blitCmdsOps.h" -#include "pxr/imaging/hgi/graphicsCmds.h" #include "pxr/imaging/hgi/graphicsPipeline.h" #include "pxr/imaging/hgi/resourceBindings.h" @@ -1284,9 +1283,9 @@ HdSt_PipelineDrawBatch::ExecuteDraw( _BindVertexBuffersForDrawing(gfxCmds, state); if (drawIndirect) { - _ExecuteDrawIndirect(gfxCmds, _dispatchBuffer, state.indexBar); + _ExecuteDrawIndirect(gfxCmds, state.indexBar); } else { - _ExecuteDrawImmediate(gfxCmds, _dispatchBuffer, state.indexBar); + _ExecuteDrawImmediate(gfxCmds, state.indexBar); } hgi->DestroyResourceBindings(&resourceBindings); @@ -1298,12 +1297,11 @@ HdSt_PipelineDrawBatch::ExecuteDraw( void HdSt_PipelineDrawBatch::_ExecuteDrawIndirect( HgiGraphicsCmds * gfxCmds, - HdStDispatchBufferSharedPtr const & dispatchBuffer, HdStBufferArrayRangeSharedPtr const & indexBar) { TRACE_FUNCTION(); - HdStBufferResourceSharedPtr paramBuffer = dispatchBuffer-> + HdStBufferResourceSharedPtr paramBuffer = _dispatchBuffer-> GetBufferArrayRange()->GetResource(HdTokens->drawDispatch); if (!TF_VERIFY(paramBuffer)) return; @@ -1311,7 +1309,7 @@ HdSt_PipelineDrawBatch::_ExecuteDrawIndirect( gfxCmds->DrawIndirect( paramBuffer->GetHandle(), paramBuffer->GetOffset(), - dispatchBuffer->GetCount(), + _dispatchBuffer->GetCount(), paramBuffer->GetStride()); } else { HdStBufferResourceSharedPtr indexBuffer = @@ -1322,7 +1320,7 @@ HdSt_PipelineDrawBatch::_ExecuteDrawIndirect( indexBuffer->GetHandle(), paramBuffer->GetHandle(), paramBuffer->GetOffset(), - dispatchBuffer->GetCount(), + _dispatchBuffer->GetCount(), paramBuffer->GetStride(), _drawCommandBuffer, _patchBaseVertexByteOffset); @@ -1332,13 +1330,12 @@ HdSt_PipelineDrawBatch::_ExecuteDrawIndirect( void HdSt_PipelineDrawBatch::_ExecuteDrawImmediate( HgiGraphicsCmds * gfxCmds, - HdStDispatchBufferSharedPtr const & dispatchBuffer, HdStBufferArrayRangeSharedPtr const & indexBar) { TRACE_FUNCTION(); - uint32_t const drawCount = dispatchBuffer->GetCount(); - uint32_t const strideUInt32 = dispatchBuffer->GetCommandNumUints(); + uint32_t const drawCount = _dispatchBuffer->GetCount(); + uint32_t const strideUInt32 = _dispatchBuffer->GetCommandNumUints(); if (!_useDrawIndexed) { for (uint32_t i = 0; i < drawCount; ++i) { diff --git a/pxr/imaging/hdSt/pipelineDrawBatch.h b/pxr/imaging/hdSt/pipelineDrawBatch.h index 80d0ffd127..531d1961e9 100644 --- a/pxr/imaging/hdSt/pipelineDrawBatch.h +++ b/pxr/imaging/hdSt/pipelineDrawBatch.h @@ -132,12 +132,10 @@ class HdSt_PipelineDrawBatch : public HdSt_DrawBatch void _ExecuteDrawIndirect( HgiGraphicsCmds * gfxCmds, - HdStDispatchBufferSharedPtr const & dispatchBuffer, HdStBufferArrayRangeSharedPtr const & indexBar); void _ExecuteDrawImmediate( HgiGraphicsCmds * gfxCmds, - HdStDispatchBufferSharedPtr const & dispatchBuffer, HdStBufferArrayRangeSharedPtr const & indexBar); void _ExecuteFrustumCull( diff --git a/pxr/imaging/hgiMetal/graphicsCmds.h b/pxr/imaging/hgiMetal/graphicsCmds.h index 6f0687047f..47cbc74a3b 100644 --- a/pxr/imaging/hgiMetal/graphicsCmds.h +++ b/pxr/imaging/hgiMetal/graphicsCmds.h @@ -143,6 +143,7 @@ class HgiMetalGraphicsCmds final : public HgiGraphicsCmds void _CreateArgumentBuffer(); void _SyncArgumentBuffer(); + void _VegaIndirectFix(); // We implement multi-draw indirect commands on Metal by encoding // separate draw commands for each draw. diff --git a/pxr/imaging/hgiMetal/graphicsCmds.mm b/pxr/imaging/hgiMetal/graphicsCmds.mm index cb0f7a0bc6..5d7fbf1674 100644 --- a/pxr/imaging/hgiMetal/graphicsCmds.mm +++ b/pxr/imaging/hgiMetal/graphicsCmds.mm @@ -39,21 +39,6 @@ PXR_NAMESPACE_OPEN_SCOPE -static void -_VegaIndirectFix(HgiMetal* hgi, - id encoder, - MTLPrimitiveType primType) -{ - if (hgi->GetCapabilities()->requiresIndirectDrawFix) { - // Fix for Vega in macOS before 12.0. There is state leakage between - // indirect draw of different prim types which results in a GPU crash. - // Flush with a null draw through the direct path. - [encoder drawPrimitives:primType - vertexStart:0 - vertexCount:0]; - } -} - HgiMetalGraphicsCmds::CachedEncoderState::CachedEncoderState() { numVertexBufferBindings = MAX_METAL_VERTEX_BUFFER_BINDINGS; @@ -288,6 +273,27 @@ _enableParallelEncoder = enable; } +void +HgiMetalGraphicsCmds::_VegaIndirectFix() +{ + if (!_primitiveTypeChanged) { + return; + } + + if (!_hgi->GetCapabilities()->requiresIndirectDrawFix) { + return; + } + // Fix for Vega in macOS before 12.0. There is state leakage between + // indirect draw of different prim types which results in a GPU crash. + // Flush with a null draw through the direct path. + id encoder = _GetEncoder(); + MTLPrimitiveType mtlType = + HgiMetalConversions::GetPrimitiveType(_primitiveType); + [encoder drawPrimitives:mtlType + vertexStart:0 + vertexCount:0]; +} + uint32_t HgiMetalGraphicsCmds::_GetNumEncoders() { @@ -416,11 +422,10 @@ if (_argumentBuffer) { if (_argumentBuffer.storageMode != MTLStorageModeShared && [_argumentBuffer respondsToSelector:@selector(didModifyRange:)]) { - NSRange range = NSMakeRange(0, _argumentBuffer.length); ARCH_PRAGMA_PUSH ARCH_PRAGMA_INSTANCE_METHOD_NOT_FOUND - [_argumentBuffer didModifyRange:range]; + [_argumentBuffer didModifyRange:{0, _argumentBuffer.length}]; ARCH_PRAGMA_POP } _argumentBuffer = nil; @@ -434,7 +439,7 @@ double y = vp[1]; double w = vp[2]; double h = vp[3]; - + // Viewport is inverted in the y. Along with the front face winding order // being inverted. // This combination allows us to emulate the OpenGL coordinate space on @@ -628,7 +633,7 @@ MTLPrimitiveType type=HgiMetalConversions::GetPrimitiveType(_primitiveType); id encoder = _GetEncoder(); - + _SetVertexBufferStepFunctionOffsets(encoder, baseInstance); if (_primitiveType == HgiPrimitiveTypePatchList) { @@ -664,13 +669,13 @@ uint32_t drawCount, uint32_t stride) { - _SyncArgumentBuffer(); - - HgiMetalBuffer* drawBuf = - static_cast(drawParameterBuffer.Get()); + MTLPrimitiveType mtlType = + HgiMetalConversions::GetPrimitiveType(_primitiveType); + id drawBufferId = + static_cast(drawParameterBuffer.Get())->GetBufferId(); + const HgiCapabilities *capabilities = _hgi->GetCapabilities(); - MTLPrimitiveType type=HgiMetalConversions::GetPrimitiveType(_primitiveType); - + _SyncArgumentBuffer(); static const uint32_t _drawCallsPerThread = 256; const uint32_t numEncoders = std::min( std::max(drawCount / _drawCallsPerThread, 1U), @@ -703,14 +708,13 @@ [encoder drawPatches:controlPointCount patchIndexBuffer:NULL patchIndexBufferOffset:0 - indirectBuffer:drawBuf->GetBufferId() + indirectBuffer:drawBufferId indirectBufferOffset:bufferOffset]; } } else { - if (_primitiveTypeChanged) { - _VegaIndirectFix(_hgi, encoder, type); - } + _VegaIndirectFix(); + for (uint32_t offset = encoderOffset; offset < encoderOffset + encoderCount; ++offset) { @@ -718,8 +722,8 @@ const uint32_t bufferOffset = drawBufferByteOffset + (offset * stride); - [encoder drawPrimitives:type - indirectBuffer:drawBuf->GetBufferId() + [encoder drawPrimitives:mtlType + indirectBuffer:drawBufferId indirectBufferOffset:bufferOffset]; } } @@ -741,7 +745,8 @@ HgiMetalBuffer* indexBuf = static_cast(indexBuffer.Get()); - MTLPrimitiveType type=HgiMetalConversions::GetPrimitiveType(_primitiveType); + MTLPrimitiveType mtlType = + HgiMetalConversions::GetPrimitiveType(_primitiveType); id encoder = _GetEncoder(); @@ -762,7 +767,9 @@ instanceCount:instanceCount baseInstance:baseInstance]; } else { - [encoder drawIndexedPrimitives:type + _VegaIndirectFix(); + + [encoder drawIndexedPrimitives:mtlType indexCount:indexCount indexType:MTLIndexTypeUInt32 indexBuffer:indexBuf->GetBufferId() @@ -785,16 +792,14 @@ std::vector const& drawParameterBufferUInt32, uint32_t patchBaseVertexByteOffset) { - _SyncArgumentBuffer(); - - id indexBufferId = - static_cast(indexBuffer.Get())->GetBufferId(); + MTLPrimitiveType mtlType = + HgiMetalConversions::GetPrimitiveType(_primitiveType); id drawBufferId = static_cast(drawParameterBuffer.Get())->GetBufferId(); + id indexBufferId = + static_cast(indexBuffer.Get())->GetBufferId(); - MTLPrimitiveType type = - HgiMetalConversions::GetPrimitiveType(_primitiveType); - + _SyncArgumentBuffer(); static const uint32_t _drawCallsPerThread = 256; const uint32_t numEncoders = std::min( std::max(drawCount / _drawCallsPerThread, 1U), @@ -846,9 +851,8 @@ } } else { - if (_primitiveTypeChanged) { - _VegaIndirectFix(_hgi, encoder, type); - } + _VegaIndirectFix(); + for (uint32_t offset = encoderOffset; offset < encoderOffset + encoderCount; ++offset) { @@ -857,7 +861,7 @@ const uint32_t bufferOffset = drawBufferByteOffset + (offset * stride); - [encoder drawIndexedPrimitives:type + [encoder drawIndexedPrimitives:mtlType indexType:MTLIndexTypeUInt32 indexBuffer:indexBufferId indexBufferOffset:0 diff --git a/pxr/imaging/hgiMetal/graphicsPipeline.h b/pxr/imaging/hgiMetal/graphicsPipeline.h index f290c01e2d..cb968312d4 100644 --- a/pxr/imaging/hgiMetal/graphicsPipeline.h +++ b/pxr/imaging/hgiMetal/graphicsPipeline.h @@ -30,9 +30,6 @@ #include "pxr/imaging/hgiMetal/api.h" -#include - - PXR_NAMESPACE_OPEN_SCOPE class HgiMetal; @@ -60,8 +57,8 @@ class HgiMetalGraphicsPipeline final : public HgiGraphicsPipeline HgiMetalGraphicsPipeline(const HgiMetalGraphicsPipeline&) = delete; void _CreateVertexDescriptor(); - void _CreateDepthStencilState(id device); - void _CreateRenderPipelineState(id device); + void _CreateDepthStencilState(HgiMetal *hgi); + void _CreateRenderPipelineState(HgiMetal *hgi); MTLVertexDescriptor *_vertexDescriptor; id _depthStencilState; diff --git a/pxr/imaging/hgiMetal/graphicsPipeline.mm b/pxr/imaging/hgiMetal/graphicsPipeline.mm index 5de699b5ee..0cf392ec6f 100644 --- a/pxr/imaging/hgiMetal/graphicsPipeline.mm +++ b/pxr/imaging/hgiMetal/graphicsPipeline.mm @@ -46,8 +46,8 @@ , _constantTessFactors(nil) { _CreateVertexDescriptor(); - _CreateDepthStencilState(hgi->GetPrimaryDevice()); - _CreateRenderPipelineState(hgi->GetPrimaryDevice()); + _CreateDepthStencilState(hgi); + _CreateRenderPipelineState(hgi); } HgiMetalGraphicsPipeline::~HgiMetalGraphicsPipeline() @@ -103,9 +103,7 @@ } // Describe each vertex attribute in the vertex buffer - for (size_t loc = 0; loc device) +HgiMetalGraphicsPipeline::_CreateRenderPipelineState(HgiMetal *hgi) { MTLRenderPipelineDescriptor *stateDesc = [[MTLRenderPipelineDescriptor alloc] init]; @@ -228,6 +226,7 @@ stateDesc.vertexDescriptor = _vertexDescriptor; NSError *error = NULL; + id device = hgi->GetPrimaryDevice(); _renderPipelineState = [device newRenderPipelineStateWithDescriptor:stateDesc error:&error]; @@ -261,7 +260,7 @@ } void -HgiMetalGraphicsPipeline::_CreateDepthStencilState(id device) +HgiMetalGraphicsPipeline::_CreateDepthStencilState(HgiMetal *hgi) { MTLDepthStencilDescriptor *depthStencilStateDescriptor = [[MTLDepthStencilDescriptor alloc] init]; @@ -295,6 +294,7 @@ _CreateStencilDescriptor(_descriptor.depthState.stencilBack); } + id device = hgi->GetPrimaryDevice(); _depthStencilState = [device newDepthStencilStateWithDescriptor:depthStencilStateDescriptor]; [depthStencilStateDescriptor release];