[HGI] Cleanup of vertex bindings, split off HgiMetal step functions

pxr/imaging/hdSt/pipelineDrawBatch.cpp

@@ -1137,51 +1137,42 @@ _GetVertexBuffersForDrawing(_BindingState const & state)
 }
 
 uint32_t
-_BindVertexBuffersForViewTransformation(
-    HgiGraphicsCmds * gfxCmds,
+_VertexBufferBindingsForViewTransformation(
+    HgiVertexBufferBindingVector &bindings,
     _BindingState const & state)
 {
-    uint32_t const nextBinding = 0; // start binding from zero
-
     // Bind the dispatchBuffer drawing coordinate resource views
     HdStBufferResourceSharedPtr resource =
                 state.dispatchBuffer->GetEntireResource();
+
+    bindings.emplace_back(resource->GetHandle(),
+                          (uint32_t)resource->GetOffset(),
+                          0);
 
-    HgiBufferHandleVector buffers{resource->GetHandle()};
-    std::vector<uint32_t> byteOffsets{
-        static_cast<uint32_t>(resource->GetOffset())};
-
-    gfxCmds->BindVertexBuffers(nextBinding, buffers, byteOffsets);
-
-    return static_cast<uint32_t>(nextBinding + buffers.size());
+    return static_cast<uint32_t>(bindings.size());
 }
 
 uint32_t
-_BindVertexBuffersForDrawing(
-    HgiGraphicsCmds * gfxCmds,
+_VertexBufferBindingsForDrawing(
+    HgiVertexBufferBindingVector &bindings,
     _BindingState const & state)
 {
-    uint32_t const nextBinding = // continue binding subsequent locations
-        _BindVertexBuffersForViewTransformation(gfxCmds, state);
-
-    // Bind the vertexBar resources
-    HgiBufferHandleVector buffers;
-    std::vector<uint32_t> byteOffsets;
+    uint32_t nextBinding = // continue binding subsequent locations
+        _VertexBufferBindingsForViewTransformation(bindings, state);
 
     for (auto const & namedResource : state.vertexBar->GetResources()) {
         HdBinding const binding = state.binder.GetBinding(namedResource.first);
         HdStBufferResourceSharedPtr const & resource = namedResource.second;
 
         if (binding.GetType() == HdBinding::VERTEX_ATTR) {
-            buffers.push_back(resource->GetHandle());
-            byteOffsets.push_back(
-                static_cast<uint32_t>(resource->GetOffset()));
+            bindings.emplace_back(resource->GetHandle(),
+                                  static_cast<uint32_t>(resource->GetOffset()),
+                                  nextBinding);
+            nextBinding++;
         }
     }
 
-    gfxCmds->BindVertexBuffers(nextBinding, buffers, byteOffsets);
-
-    return static_cast<uint32_t>(nextBinding + buffers.size());
+    return nextBinding;
 }
 
 } // annonymous namespace
@@ -1280,7 +1271,9 @@ HdSt_PipelineDrawBatch::ExecuteDraw(
             hgi->CreateResourceBindings(bindingsDesc);
     gfxCmds->BindResources(resourceBindings);
 
-    _BindVertexBuffersForDrawing(gfxCmds, state);
+    HgiVertexBufferBindingVector bindings;
+    _VertexBufferBindingsForDrawing(bindings, state);
+    gfxCmds->BindVertexBuffers(bindings);
 
     if (drawIndirect) {
         _ExecuteDrawIndirect(gfxCmds, state.indexBar);
@@ -1532,7 +1525,9 @@ HdSt_PipelineDrawBatch::_ExecuteFrustumCull(
             hgi->CreateResourceBindings(bindingsDesc);
     cullGfxCmds->BindResources(resourceBindings);
 
-    _BindVertexBuffersForViewTransformation(cullGfxCmds.get(), state);
+    HgiVertexBufferBindingVector bindings;
+    _VertexBufferBindingsForDrawing(bindings, state);
+    cullGfxCmds->BindVertexBuffers(bindings);
 
     GfMatrix4f const &cullMatrix = GfMatrix4f(renderPassState->GetCullMatrix());
     GfVec2f const &drawRangeNdc = renderPassState->GetDrawingRangeNDC();

pxr/imaging/hdSt/unitTestHelper.cpp

@@ -400,7 +400,7 @@ HdSt_TextureTestDriver::Draw(HgiTextureHandle const &colorDst,
     gfxCmds->PushDebugGroup("Debug HdSt_TextureTestDriver");
     gfxCmds->BindResources(_resourceBindings);
     gfxCmds->BindPipeline(_pipeline);
-    gfxCmds->BindVertexBuffers(0, {_vertexBuffer}, {0});
+    gfxCmds->BindVertexBuffers({{_vertexBuffer, 0, 0}});
     gfxCmds->SetViewport(viewport);
     gfxCmds->SetConstantValues(_pipeline, HgiShaderStageFragment, 0, 
         _constantsData.size(), _constantsData.data());

pxr/imaging/hdx/colorCorrectionTask.cpp

@@ -487,7 +487,7 @@ HdxColorCorrectionTask::_ApplyColorCorrection(
     gfxCmds->PushDebugGroup("ColorCorrection");
     gfxCmds->BindResources(_resourceBindings);
     gfxCmds->BindPipeline(_pipeline);
-    gfxCmds->BindVertexBuffers(0, {_vertexBuffer}, {0});
+    gfxCmds->BindVertexBuffers({{_vertexBuffer, 0, 0}});
     const GfVec4i vp(0, 0, dimensions[0], dimensions[1]);
     _screenSize[0] = static_cast<float>(dimensions[0]);
     _screenSize[1] = static_cast<float>(dimensions[1]);

pxr/imaging/hdx/fullscreenShader.cpp

@@ -688,7 +688,7 @@ HdxFullscreenShader::_Draw(
     gfxCmds->PushDebugGroup(_debugName.c_str());
     gfxCmds->BindResources(_resourceBindings);
     gfxCmds->BindPipeline(_pipeline);
-    gfxCmds->BindVertexBuffers(0, {_vertexBuffer}, {0});
+    gfxCmds->BindVertexBuffers({{_vertexBuffer, 0, 0}});
     gfxCmds->SetViewport(viewport);
 
     if (!_constantsData.empty()) {

pxr/imaging/hdx/visualizeAovTask.cpp

@@ -510,7 +510,7 @@ HdxVisualizeAovTask::_ApplyVisualizationKernel(
     gfxCmds->PushDebugGroup("Visualize AOV");
     gfxCmds->BindResources(_resourceBindings);
     gfxCmds->BindPipeline(_pipeline);
-    gfxCmds->BindVertexBuffers(0, {_vertexBuffer}, {0});
+    gfxCmds->BindVertexBuffers({{_vertexBuffer, 0, 0}});
     const GfVec4i vp(0, 0, dimensions[0], dimensions[1]);
     _screenSize[0] = static_cast<float>(dimensions[0]);
     _screenSize[1] = static_cast<float>(dimensions[1]);

pxr/imaging/hgi/graphicsCmds.h

@@ -99,14 +99,9 @@ class HgiGraphicsCmds : public HgiCmds
         const void* data) = 0;
 
     /// Binds the vertex buffer(s) that describe the vertex attributes.
-    /// `firstBinding` the first index to which buffers are bound (usually 0).
-    /// `byteOffsets` offset to where the data of each buffer starts, in bytes.
-    /// `strides` the size of a vertex in each of the buffers.
     HGI_API
     virtual void BindVertexBuffers(
-        uint32_t firstBinding,
-        HgiBufferHandleVector const& buffers,
-        std::vector<uint32_t> const& byteOffsets) = 0;
+        HgiVertexBufferBindingVector const &bindings) = 0;
 
     /// Records a draw command that renders one or more instances of primitives
     /// using the number of vertices provided.

pxr/imaging/hgi/resourceBindings.h

@@ -206,6 +206,25 @@ class HgiResourceBindings
 using HgiResourceBindingsHandle = HgiHandle<HgiResourceBindings>;
 using HgiResourceBindingsHandleVector = std::vector<HgiResourceBindingsHandle>;
 
+struct HgiVertexBufferBinding
+{
+    HGI_API
+    HgiVertexBufferBinding(HgiBufferHandle const &buffer,
+                           uint32_t byteOffset,
+                           uint32_t index)
+        : buffer(buffer)
+        , byteOffset(byteOffset)
+        , index(index)
+    {
+    }
+
+    HgiBufferHandle buffer;
+    uint32_t byteOffset;
+    uint32_t index;
+};
+
+using HgiVertexBufferBindingVector = std::vector<HgiVertexBufferBinding>;
+
 
 PXR_NAMESPACE_CLOSE_SCOPE
 

pxr/imaging/hgiGL/graphicsCmds.cpp

@@ -127,12 +127,10 @@ HgiGLGraphicsCmds::SetConstantValues(
 
 void
 HgiGLGraphicsCmds::BindVertexBuffers(
-    uint32_t firstBinding,
-    HgiBufferHandleVector const& vertexBuffers,
-    std::vector<uint32_t> const& byteOffsets)
+    HgiVertexBufferBindingVector const &bindings)
 {
     _ops.push_back( 
-        HgiGLOps::BindVertexBuffers(firstBinding, vertexBuffers, byteOffsets) );
+        HgiGLOps::BindVertexBuffers(bindings) );
 }
 
 void

pxr/imaging/hgiGL/graphicsCmds.h

@@ -82,9 +82,7 @@ class HgiGLGraphicsCmds final : public HgiGraphicsCmds
 
     HGIGL_API
     void BindVertexBuffers(
-        uint32_t firstBinding,
-        HgiBufferHandleVector const& buffers,
-        std::vector<uint32_t> const& byteOffsets) override;
+        HgiVertexBufferBindingVector const &bindings) override;
 
     HGIGL_API
     void Draw(

pxr/imaging/hgiGL/ops.cpp

@@ -580,27 +580,22 @@ HgiGLOps::SetConstantValues(
 
 HgiGLOpsFn
 HgiGLOps::BindVertexBuffers(
-    uint32_t firstBinding,
-    HgiBufferHandleVector const& vertexBuffers,
-    std::vector<uint32_t> const& byteOffsets)
+    HgiVertexBufferBindingVector const &bindings)
 {
-    return [firstBinding, vertexBuffers, byteOffsets] {
+    return [bindings] {
         TRACE_SCOPE("HgiGLOps::BindVertexBuffers");
-        TF_VERIFY(byteOffsets.size() == vertexBuffers.size());
-        TF_VERIFY(byteOffsets.size() == vertexBuffers.size());
 
         // XXX use glBindVertexBuffers to bind all VBs in one go.
-        for (size_t i=0; i<vertexBuffers.size(); i++) {
-            HgiBufferHandle bufHandle = vertexBuffers[i];
-            HgiGLBuffer* buf = static_cast<HgiGLBuffer*>(bufHandle.Get());
+        for (HgiVertexBufferBinding const &binding : bindings) {
+            HgiGLBuffer* buf = static_cast<HgiGLBuffer*>(binding.buffer.Get());
             HgiBufferDesc const& desc = buf->GetDescriptor();
 
             TF_VERIFY(desc.usage & HgiBufferUsageVertex);
 
             glBindVertexBuffer(
-                firstBinding + i,
+                binding.index,
                 buf->GetBufferId(),
-                byteOffsets[i],
+                binding.byteOffset,
                 desc.vertexStride);
         }
 

pxr/imaging/hgiGL/ops.h

@@ -134,9 +134,7 @@ class HgiGLOps
 
     HGIGL_API
     static HgiGLOpsFn BindVertexBuffers(
-        uint32_t firstBinding,
-        HgiBufferHandleVector const& buffers,
-        std::vector<uint32_t> const& byteOffsets);
+        HgiVertexBufferBindingVector const &bindings);
 
     HGIGL_API
     static HgiGLOpsFn Draw(

pxr/imaging/hgiMetal/CMakeLists.txt

@@ -21,20 +21,22 @@ pxr_library(hgiMetal
 
     PUBLIC_HEADERS
         api.h
+        blitCmds.h
+        buffer.h
         capabilities.h
         computeCmds.h
         computePipeline.h
         diagnostic.h
-        hgi.h
-        texture.h
-        blitCmds.h
-        buffer.h
         graphicsCmds.h
         graphicsPipeline.h
+        hgi.h
+
         resourceBindings.h
         sampler.h
         shaderFunction.h
         shaderProgram.h
+        stepFunctions.h
+        texture.h
 
     PRIVATE_HEADERS
         conversions.h
@@ -58,6 +60,7 @@ pxr_library(hgiMetal
         shaderGenerator.mm
         shaderProgram.mm
         shaderSection.mm
+        stepFunctions.mm
         texture.mm
 
     RESOURCE_FILES

pxr/imaging/hgiMetal/graphicsCmds.h

@@ -27,6 +27,8 @@
 #include "pxr/pxr.h"
 #include "pxr/base/gf/vec4i.h"
 #include "pxr/imaging/hgiMetal/api.h"
+#include "pxr/imaging/hgiMetal/hgi.h"
+#include "pxr/imaging/hgiMetal/stepFunctions.h"
 #include "pxr/imaging/hgi/graphicsCmds.h"
 #include <cstdint>
 
@@ -70,9 +72,7 @@ class HgiMetalGraphicsCmds final : public HgiGraphicsCmds
 
     HGIMETAL_API
     void BindVertexBuffers(
-        uint32_t firstBinding,
-        HgiBufferHandleVector const& buffers,
-        std::vector<uint32_t> const& byteOffsets) override;
+        HgiVertexBufferBindingVector const &bindings) override;
 
     HGIMETAL_API
     void Draw(
@@ -145,86 +145,18 @@ class HgiMetalGraphicsCmds final : public HgiGraphicsCmds
     void _SyncArgumentBuffer();
     void _VegaIndirectFix();
 
-    // We implement multi-draw indirect commands on Metal by encoding
-    // separate draw commands for each draw.
-    //
-    // Some aspects of drawing command primitive input assembly work
-    // differently on Metal than other graphics APIs. There are two
-    // concerns that we need to account for while processing a buffer
-    // with multiple indirect draw commands.
-    //
-    // 1) Metal does not support a vertex attrib divisor, so in order to
-    // have vertex attributes which advance once per draw command we use
-    // a constant vertex buffer step function and advance the vertex buffer
-    // binding offset explicitly by executing setVertexBufferOffset for
-    // the vertex buffers associated with "perDrawCommand" vertex attributes.
-    //
-    // 2) Metal does not support a base vertex offset for control point
-    // vertex attributes when drawing patches. It is inconvenient and
-    // expensive to encode a distinct controlPointIndex buffer for each
-    // draw that shares a patch topology. Instead, we use a per patch
-    // control point vertex buffer step function, and explicitly advance
-    // the vertex buffer binding offset by executing setVertexBufferOffset
-    // for the vertex buffers associated with "perPatchControlPoint"
-    // vertex attributes.
-
-    struct _VertexBufferStepFunctionDesc
-    {
-        _VertexBufferStepFunctionDesc(
-                uint32_t bindingIndex,
-                uint32_t byteOffset,
-                uint32_t vertexStride)
-            : bindingIndex(bindingIndex)
-            , byteOffset(byteOffset)
-            , vertexStride(vertexStride)
-            { }
-        uint32_t bindingIndex;
-        uint32_t byteOffset;
-        uint32_t vertexStride;
-    };
-
-    using _StepFunctionDescVector = std::vector<_VertexBufferStepFunctionDesc>;
-
-    _StepFunctionDescVector _vertexBufferStepFunctionDescs;
-    _StepFunctionDescVector _patchBaseVertexBufferStepFunctionDescs;
-
-    void _InitVertexBufferStepFunction(HgiGraphicsPipeline const * pipeline);
-    void _BindVertexBufferStepFunction(uint32_t byteOffset,
-                                       uint32_t bindingIndex);
-
-    void _SetVertexBufferStepFunctionOffsets(
-        id<MTLRenderCommandEncoder> encoder,
-        uint32_t baseInstance);
-
-    void _SetPatchBaseVertexBufferStepFunctionOffsets(
-        id<MTLRenderCommandEncoder> encoder,
-        uint32_t baseVertex);
-
     struct CachedEncoderState {
         CachedEncoderState();
 
         void ResetCachedEncoderState();
 
-        // This is based on the size aclocated for HgiMetalArgumentOffsetBufferVS->FS
-        // But ideally we'd pick it up from a common header
-        #define MAX_METAL_VERTEX_BUFFER_BINDINGS 64
-
-        void AddVertexBinding(
-            uint32_t bindingIndex, id<MTLBuffer> buffer, uint32_t byteOffset);
-
         MTLViewport viewport;
         MTLScissorRect scissorRect;
 
         HgiMetalResourceBindings* resourceBindings;
         HgiMetalGraphicsPipeline* graphicsPipeline;
         id<MTLBuffer> argumentBuffer;
-
-        struct {
-            id<MTLBuffer> vertexBuffer;
-            uint32_t      byteOffset;
-        } VertexBufferBindingDesc[MAX_METAL_VERTEX_BUFFER_BINDINGS];
-
-        uint32_t numVertexBufferBindings;
+        HgiVertexBufferBindingVector vertexBindings;
     } _CachedEncState;
 
     HgiMetal* _hgi;
@@ -242,6 +174,7 @@ class HgiMetalGraphicsCmds final : public HgiGraphicsCmds
     bool _enableParallelEncoder;
     bool _primitiveTypeChanged;
     uint32 _maxNumEncoders;
+    HgiMetalStepFunctions _stepFunctions;
 };
 
 PXR_NAMESPACE_CLOSE_SCOPE