Fix a race condition in GeometryView (#19009)

* Add a deviceMask to the constructor of GeometryView

Creating the DeviceGeometryViews on demand led to multithreading problems in the MeshDrawPacket

Signed-off-by: Martin Sattlecker <[email protected]>

* Fix test

Signed-off-by: Martin Sattlecker <[email protected]>

* Fix

Signed-off-by: Martin Sattlecker <[email protected]>

---------

Signed-off-by: Martin Sattlecker <[email protected]>

Gems/Atom/Feature/Common/Code/Source/AuxGeom/AuxGeomBase.h

@@ -117,7 +117,7 @@ namespace AZ
         //! Each dynamic primitive drawn through the AuxGeom draw interface is stored in the scene data as an instance of this struct
         struct PrimitiveBufferEntry
         {
-            RHI::GeometryView           m_geometryView;
+            RHI::GeometryView m_geometryView{ RHI::MultiDevice::AllDevices };
             AZ::Vector3                 m_center;       // used for depth sorting blended draws
             AuxGeomPrimitiveType        m_primitiveType;
             AuxGeomDepthReadType        m_depthReadType;

Gems/Atom/Feature/Common/Code/Source/AuxGeom/DynamicPrimitiveProcessor.cpp

@@ -44,9 +44,6 @@ namespace AZ
                 m_streamBufferViewsValidatedForLayout[primitiveType] = false;
             }
 
-            // We have a single stream (position and color are interleaved in the vertex buffer)
-            m_geometryView.GetStreamBufferViews().resize(1);
-
             m_scene = scene;
             InitShader();
 
@@ -218,7 +215,14 @@ namespace AZ
                 return false;
             }
             dynamicBuffer->Write(source.data(), static_cast<uint32_t>(sourceByteSize));
-            m_geometryView.GetStreamBufferViews()[0] = dynamicBuffer->GetStreamBufferView(sizeof(AuxGeomDynamicVertex));
+            if (m_geometryView.GetStreamBufferViews().empty())
+            {
+                m_geometryView.AddStreamBufferView(dynamicBuffer->GetStreamBufferView(sizeof(AuxGeomDynamicVertex)));
+            }
+            else
+            {
+                m_geometryView.SetStreamBufferView(0, dynamicBuffer->GetStreamBufferView(sizeof(AuxGeomDynamicVertex)));
+            }
             return true;
         }
 

Gems/Atom/Feature/Common/Code/Source/AuxGeom/DynamicPrimitiveProcessor.h

@@ -144,7 +144,7 @@ namespace AZ
             ShaderData m_shaderData;
 
             // Buffers for all primitives
-            RHI::GeometryView m_geometryView;
+            RHI::GeometryView m_geometryView{ RHI::MultiDevice::AllDevices };
 
             // Flags to see if stream buffer views have been validated for a prim type's layout
             bool m_streamBufferViewsValidatedForLayout[PrimitiveType_Count];

Gems/Atom/Feature/Common/Code/Source/AuxGeom/FixedShapeProcessor.h

@@ -88,9 +88,9 @@ namespace AZ
             //! We store a struct of this type for each fixed object geometry (both shapes and boxes)
             struct ObjectBuffers
             {
-                RHI::GeometryView m_pointGeometryView;
-                RHI::GeometryView m_lineGeometryView;
-                RHI::GeometryView m_triangleGeometryView;
+                RHI::GeometryView m_pointGeometryView{ RHI::MultiDevice::AllDevices };
+                RHI::GeometryView m_lineGeometryView{ RHI::MultiDevice::AllDevices };
+                RHI::GeometryView m_triangleGeometryView{ RHI::MultiDevice::AllDevices };
 
                 AZ::RHI::Ptr<AZ::RHI::Buffer> m_pointIndexBuffer;
                 AZ::RHI::Ptr<AZ::RHI::Buffer> m_lineIndexBuffer;

Gems/Atom/Feature/Common/Code/Source/ImGui/ImGuiPass.cpp

@@ -638,7 +638,7 @@ namespace AZ
                             }
                         }
 
-                        RHI::GeometryView geometryView;
+                        RHI::GeometryView geometryView{ RHI::MultiDevice::AllDevices };
                         geometryView.SetDrawArguments(RHI::DrawIndexed(vertexOffset, drawCmd.ElemCount, indexOffset));
                         geometryView.SetIndexBufferView(m_indexBufferView);
                         geometryView.AddStreamBufferView(m_vertexBufferView[0]);

Gems/Atom/Feature/Common/Code/Source/ReflectionProbe/ReflectionProbe.h

@@ -28,7 +28,7 @@ namespace AZ
         // shared data for rendering reflections, loaded and stored by the ReflectionProbeFeatureProcessor and passed to all probes
         struct ReflectionRenderData
         {
-            RHI::GeometryView m_geometryView;
+            RHI::GeometryView m_geometryView{ RHI::MultiDevice::AllDevices };
 
             RPI::Ptr<RPI::PipelineStateForDraw> m_stencilPipelineState;
             RPI::Ptr<RPI::PipelineStateForDraw> m_blendWeightPipelineState;

Gems/Atom/Feature/Common/Code/Source/Shadows/ProjectedShadowFeatureProcessor.h

@@ -156,7 +156,7 @@ namespace AZ::Render
         RHI::ShaderInputNameIndex m_shadowmapAtlasSizeIndex{ "m_shadowmapAtlasSize" };
         RHI::ShaderInputNameIndex m_invShadowmapAtlasSizeIndex{ "m_invShadowmapAtlasSize" };
 
-        RHI::GeometryView m_geometryView;
+        RHI::GeometryView m_geometryView{ RHI::MultiDevice::AllDevices };
 
         bool m_deviceBufferNeedsUpdate = false;
         bool m_shadowmapPassNeedsUpdate = true;

Gems/Atom/RHI/Code/Include/Atom/RHI/GeometryView.h

@@ -37,24 +37,24 @@ namespace AZ::RHI
     public:
         friend class StreamIterator<GeometryView, StreamBufferView>;
 
+        GeometryView(MultiDevice::DeviceMask deviceMask)
+        {
+            MultiDeviceObject::IterateDevices(
+                deviceMask,
+                [&](int deviceIndex)
+                {
+                    m_geometryViews.emplace(deviceIndex, DeviceGeometryView{});
+                    return true;
+                });
+        }
+
         inline DeviceGeometryView* GetDeviceGeometryView(int deviceIndex)
         {
             auto iter = m_geometryViews.find(deviceIndex);
             if (iter == m_geometryViews.end())
             {
-                DeviceGeometryView newGeometryView;
-                newGeometryView.SetDrawArguments( m_drawArguments.GetDeviceDrawArguments(deviceIndex) );
-                if (m_indexBufferView.IsValid())
-                {
-                    newGeometryView.SetIndexBufferView(m_indexBufferView.GetDeviceIndexBufferView(deviceIndex));
-                }
-                for (StreamBufferView& stream : m_streamBufferViews)
-                {
-                    newGeometryView.AddStreamBufferView(stream.GetDeviceStreamBufferView(deviceIndex));
-                }
-                newGeometryView.m_dummyStreamBufferIndex = m_dummyStreamBufferIndex;
-                m_geometryViews.emplace(deviceIndex, std::move(newGeometryView));
-                return &m_geometryViews[deviceIndex];
+                AZ_Error("GeometryView", false, "No DeviceGeometryView found for device index %d\n", deviceIndex);
+                return nullptr;
             }
             else
             {
@@ -103,7 +103,6 @@ namespace AZ::RHI
         // --- Stream Buffer Views ---
 
         const StreamBufferView& GetStreamBufferView(u8 idx) const { return m_streamBufferViews[idx]; }
-        AZStd::vector<StreamBufferView>& GetStreamBufferViews() { return m_streamBufferViews; }
         const AZStd::vector<StreamBufferView>& GetStreamBufferViews() const { return m_streamBufferViews; }
 
         inline void SetStreamBufferView(u8 idx, StreamBufferView streamBufferView)

Gems/Atom/RHI/Code/Tests/MultiDeviceDrawPacketTests.cpp

@@ -164,7 +164,7 @@ namespace UnitTest
         RHI::Ptr<RHI::ShaderResourceGroupPool> m_srgPool;
         AZStd::array<RHI::Ptr<RHI::ShaderResourceGroup>, RHI::Limits::Pipeline::ShaderResourceGroupCountMax> m_srgs;
         AZStd::array<uint8_t, sizeof(unsigned int) * 4> m_rootConstants;
-        RHI::GeometryView m_geometryView;
+        RHI::GeometryView m_geometryView{ LocalDeviceMask };
 
         AZStd::vector<MultiDeviceDrawItemData> m_drawItemDatas;
     };

Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Pass/FullscreenTrianglePass.h

@@ -82,7 +82,7 @@ namespace AZ
             RHI::DrawItem m_item;
 
             // Holds the geometry info for the draw call
-            RHI::GeometryView m_geometryView;
+            RHI::GeometryView m_geometryView{ RHI::MultiDevice::AllDevices };
 
             // The stencil reference value for the draw item
             uint32_t m_stencilRef;

Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Pass/Specific/ImageAttachmentPreviewPass.h

@@ -142,7 +142,7 @@ namespace AZ
                 RHI::DrawItem m_item{RHI::MultiDevice::AllDevices};
 
                 // Holds the geometry info for the draw call
-                RHI::GeometryView m_geometryView;
+                RHI::GeometryView m_geometryView{ RHI::MultiDevice::AllDevices };
 
                 // Key to pass to the SRG when desired shader variant isn't found
                 ShaderVariantKey m_shaderVariantKeyFallback;

Gems/Atom/RPI/Code/Source/RPI.Public/DynamicDraw/DynamicDrawContext.cpp

@@ -528,7 +528,7 @@ namespace AZ
 
             // --- Geometry View ---
 
-            RHI::GeometryView newGeometryView;
+            RHI::GeometryView newGeometryView{ RHI::MultiDevice::AllDevices };
 
             RHI::DrawIndexed drawIndexed;
             drawIndexed.m_indexCount = indexCount;
@@ -626,7 +626,7 @@ namespace AZ
 
             // --- Geometry View ---
 
-            RHI::GeometryView newGeometryView;
+            RHI::GeometryView newGeometryView{ RHI::MultiDevice::AllDevices };
             newGeometryView.SetDrawArguments(RHI::DrawLinear(vertexCount, 0));
 
             // Write data to vertex buffer and set up stream buffer views for DrawItem

Gems/Atom/RPI/Code/Source/RPI.Public/Model/ModelLod.cpp

@@ -56,7 +56,7 @@ namespace AZ
 
             for (const ModelLodAsset::Mesh& mesh : lodAsset->GetMeshes())
             {
-                Mesh meshInstance;
+                Mesh meshInstance{ RHI::MultiDevice::AllDevices };
 
                 const BufferAssetView& indexBufferAssetView = mesh.GetIndexBufferAssetView();
                 const Data::Asset<BufferAsset>& indexBufferAsset = indexBufferAssetView.GetBufferAsset();

Gems/AtomTressFX/Code/Rendering/HairRenderObject.h

@@ -382,7 +382,7 @@ namespace AZ
  
                 //! Index buffer for the render pass via draw calls - naming was kept
                 Data::Instance<RHI::Buffer> m_indexBuffer;
-                RHI::GeometryView m_geometryView;
+                RHI::GeometryView m_geometryView{ RHI::MultiDevice::AllDevices };
                 //-------------------------------------------------------------------
 
                 AZStd::mutex m_mutex;

Gems/DiffuseProbeGrid/Code/Source/Render/DiffuseProbeGrid.h

@@ -34,7 +34,7 @@ namespace AZ
             RHI::Ptr<RHI::ImagePool> m_imagePool;
             RHI::Ptr<RHI::BufferPool> m_bufferPool;
 
-            RHI::GeometryView m_geometryView;
+            RHI::GeometryView m_geometryView{ RHI::MultiDevice::AllDevices };
 
             // image views
             RHI::ImageViewDescriptor m_probeRayTraceImageViewDescriptor;

Gems/Stars/Code/Source/StarsFeatureProcessor.h

@@ -70,7 +70,7 @@ namespace AZ::Render
         RHI::ConstPtr<RHI::DrawPacket> BuildDrawPacket();
 
         RPI::Ptr<RPI::PipelineStateForDraw> m_meshPipelineState;
-        RHI::GeometryView m_geometryView;
+        RHI::GeometryView m_geometryView{ RHI::MultiDevice::AllDevices };
 
         Data::Instance<RPI::ShaderResourceGroup> m_drawSrg = nullptr;
         Data::Instance<RPI::Shader> m_shader = nullptr;

Gems/Terrain/Code/Source/TerrainRenderer/TerrainMeshManager.cpp

@@ -486,7 +486,7 @@ namespace Terrain
 
                 sector.m_geometryView.ClearStreamBufferViews();
 
-                AZStd::vector<AZ::RHI::StreamBufferView>& streamBufferViews = sector.m_geometryView.GetStreamBufferViews();
+                AZStd::vector<AZ::RHI::StreamBufferView> streamBufferViews;
                 streamBufferViews.resize(StreamIndex::Count);
                 streamBufferViews[StreamIndex::XYPositions] = CreateStreamBufferView(m_xyPositionsBuffer);
                 streamBufferViews[StreamIndex::Heights] = CreateStreamBufferView(sector.m_heightsNormalsBuffer);
@@ -503,6 +503,7 @@ namespace Terrain
                     streamBufferViews[StreamIndex::LodHeights] = CreateStreamBufferView(m_dummyLodHeightsNormalsBuffer);
                     streamBufferViews[StreamIndex::LodNormals] = CreateStreamBufferView(m_dummyLodHeightsNormalsBuffer, AZ::RHI::GetFormatSize(HeightFormat));
                 }
+                sector.m_geometryView.SetStreamBufferViews(streamBufferViews);
 
                 BuildDrawPacket(sector);
 

Gems/Terrain/Code/Source/TerrainRenderer/TerrainMeshManager.h

@@ -153,7 +153,10 @@ namespace Terrain
 
         struct Sector
         {
-            AZ::RHI::GeometryView m_geometryView;
+            using GeometryView = AZ::RHI::GeometryView;
+            static constexpr auto AllDevices = AZ::RHI::MultiDevice::AllDevices;
+
+            GeometryView m_geometryView{ AllDevices };
             AZ::Data::Instance<AZ::RPI::ShaderResourceGroup> m_srg;
             AZ::Aabb m_aabb = AZ::Aabb::CreateNull();
             AZStd::array<AZ::Aabb, 4> m_quadrantAabbs;
@@ -162,7 +165,9 @@ namespace Terrain
             // When drawing, either the m_rhiDrawPacket will be used, or some number of the m_rhiDrawPacketQuadrants
             AZ::RHI::ConstPtr<AZ::RHI::DrawPacket> m_rhiDrawPacket;
             AZStd::array<AZ::RHI::ConstPtr<AZ::RHI::DrawPacket>, 4> m_rhiDrawPacketQuadrant;
-            AZStd::array<AZ::RHI::GeometryView, 4> m_quadrantGeometryViews;
+            AZStd::array<GeometryView, 4> m_quadrantGeometryViews{
+                GeometryView{ AllDevices }, GeometryView{ AllDevices }, GeometryView{ AllDevices }, GeometryView{ AllDevices }
+            };
 
             AZ::Data::Instance<AZ::RPI::Buffer> m_heightsNormalsBuffer;
             AZ::Data::Instance<AZ::RPI::Buffer> m_lodHeightsNormalsBuffer;