UUM-65454 MaxSubpass discrepancy with RenderPassSetup and NativePassCompiler

JIRA: https://jira.unity3d.com/browse/UUM-65454

When merging subpasses together in a NativePass, we don't check if there will be more than 8 native render subpasses. This PR adds a check for this when verifying if a certain graph pass can be merged into a Native Pass, as well as a test. If merging the graph pass would result in too many native subpasses, we simply create a new Native Pass

Author: RoseHirigoyen

Packages/com.unity.render-pipelines.core/Runtime/RenderGraph/Compiler/NativePassCompiler.cs

@@ -27,6 +27,7 @@ internal struct RenderGraphInputInfo
 
         internal const int k_EstimatedPassCount = 100;
         internal const int k_EstimatedResourceCountPerType = 50;
+        internal const int k_MaxSubpass = 8; // Needs to match with RenderPassSetup.h
 
         NativeList<AttachmentDescriptor> m_BeginRenderPassAttachments;
 
@@ -662,7 +663,7 @@ void DetectMemoryLessResources()
             }
         }
 
-        private bool IsSameNativeSubPass(ref SubPassDescriptor a, ref SubPassDescriptor b)
+        internal static bool IsSameNativeSubPass(ref SubPassDescriptor a, ref SubPassDescriptor b)
         {
             if (a.flags != b.flags
              || a.colorOutputs.Length != b.colorOutputs.Length
@@ -1250,7 +1251,7 @@ internal unsafe void ExecuteBeginRenderPass(InternalRenderGraphContext rgContext
                 var graphPassNamesForDebugSpan = ReadOnlySpan<byte>.Empty;
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
                 if(RenderGraph.enableValidityChecks)
-                { 
+                {
                     graphPassNamesForDebug.Clear();
 
                     nativePass.GetGraphPassNames(contextData, graphPassNamesForDebug);

Packages/com.unity.render-pipelines.core/Runtime/RenderGraph/Compiler/PassesData.cs

@@ -15,7 +15,7 @@ internal struct PassInputData
     }
 
     // Per pass info on outputs to the pass
-    [DebuggerDisplay("PassOutputData: Res({resource.index})")] 
+    [DebuggerDisplay("PassOutputData: Res({resource.index})")]
     internal struct PassOutputData
     {
         public ResourceHandle resource;
@@ -101,7 +101,7 @@ internal struct PassData
         public int fragmentInfoHeight;
         public int fragmentInfoVolumeDepth;
         public int fragmentInfoSamples;
-        
+
         public int waitOnGraphicsFencePassId; // -1 if no fence wait is needed, otherwise the passId to wait on
 
         public bool asyncCompute;
@@ -460,6 +460,7 @@ internal enum PassBreakReason
         NonRasterPass, // The next pass is a non-raster pass
         DifferentDepthTextures, // The next pass uses a different depth texture (and we only allow one in a whole NRP)
         AttachmentLimitReached, // Adding the next pass would have used more attachments than allowed
+        SubPassLimitReached, // Addind the next pass would have generated more subpasses than allowed
         EndOfGraph, // The last pass in the graph was reached
         FRStateMismatch, // One pass is using foveated rendering and the other not
         Merged, // I actually got merged
@@ -491,6 +492,7 @@ public PassBreakAudit(PassBreakReason reason, int breakPass)
             "The next pass is not a raster render pass.",
             "The next pass uses a different depth buffer. All passes in the native render pass need to use the same depth buffer.",
             $"The limit of {FixedAttachmentArray<PassFragmentData>.MaxAttachments} native pass attachments would be exceeded when merging with the next pass.",
+            $"The limit of {NativePassCompiler.k_MaxSubpass} native subpasses would be exceeded when merging with the next pass.",
             "This is the last pass in the graph, there are no other passes to merge.",
             "The the next pass uses a different foveated rendering state",
             "The next pass got merged into this pass.",
@@ -536,7 +538,7 @@ public NativePassData(ref PassData pass, CompilerContextData ctx)
             samples = pass.fragmentInfoSamples;
             hasDepth = pass.fragmentInfoHasDepth;
             hasFoveatedRasterization = pass.hasFoveatedRasterization;
-            
+
             loadAudit = new FixedAttachmentArray<LoadAudit>();
             storeAudit = new FixedAttachmentArray<StoreAudit>();
             breakAudit = new PassBreakAudit(PassBreakReason.NotOptimized, -1);
@@ -651,7 +653,7 @@ public static PassBreakAudit CanMerge(CompilerContextData contextData, int activ
             // Gather which attachments to add to the current renderpass
             var attachmentsToTryAdding = new FixedAttachmentArray<PassFragmentData>();
 
-            // We can't have more than the maximum amout of attachments in a given native renderpass
+            // We can't have more than the maximum amount of attachments in a given native renderpass
             int currAvailableAttachmentSlots = FixedAttachmentArray<PassFragmentData>.MaxAttachments - nativePass.fragments.size;
 
             foreach (ref readonly var fragment in passToMerge.Fragments(contextData))
@@ -714,10 +716,183 @@ public static PassBreakAudit CanMerge(CompilerContextData contextData, int activ
                 }
             }
 
+            if (CountNativePassesAfterMerge(contextData, nativePass, passToMerge) > NativePassCompiler.k_MaxSubpass)
+            {
+                return new PassBreakAudit(PassBreakReason.SubPassLimitReached, passIdToMerge);
+            }
+
             // All is good! Pass can be merged into active native pass
             return new PassBreakAudit(PassBreakReason.Merged, passIdToMerge);
         }
 
+        // This function must follow the implementation of NativePassCompiler.PrepareNativeRenderPass
+        static int CountNativePassesAfterMerge(CompilerContextData contextData, NativePassData nativePass, PassData passToMerge)
+        {
+            var combinedFragmentList = nativePass.fragments;
+
+            // Depth needs special handling if the native pass doesn't have depth and merges with a pass that does
+            // as we require the depth attachment to be at index 0
+            if (!nativePass.hasDepth && passToMerge.fragmentInfoHasDepth)
+            {
+                nativePass.hasDepth = true;
+                combinedFragmentList.Add(contextData.fragmentData[passToMerge.firstFragment]);
+                var size = combinedFragmentList.size;
+                if (size > 1)
+                    (combinedFragmentList[0], combinedFragmentList[size-1]) = (combinedFragmentList[size-1], combinedFragmentList[0]);
+            }
+
+            // Update versions and flags of existing attachments and
+            // add any new attachments
+            foreach (ref readonly var newAttach in passToMerge.Fragments(contextData))
+            {
+                bool alreadyAttached = false;
+
+                for (int i = 0; i < combinedFragmentList.size; ++i)
+                {
+                    ref var existingAttach = ref combinedFragmentList[i];
+                    if (existingAttach.resource.index == newAttach.resource.index)
+                    {
+                        // Update the attached version access flags and version
+                        existingAttach.accessFlags |= newAttach.accessFlags;
+                        existingAttach.resource.version = newAttach.resource.version;
+                        alreadyAttached = true;
+                        break;
+                    }
+                }
+
+                if (!alreadyAttached)
+                {
+                    combinedFragmentList.Add(newAttach);
+                }
+            }
+
+            foreach (ref readonly var newAttach in passToMerge.FragmentInputs(contextData))
+            {
+                bool alreadyAttached = false;
+
+                for (int i = 0; i < combinedFragmentList.size; ++i)
+                {
+                    ref var existingAttach = ref combinedFragmentList[i];
+                    if (existingAttach.resource.index == newAttach.resource.index)
+                    {
+                        // Update the attached version access flags and version
+                        existingAttach.accessFlags |= newAttach.accessFlags;
+                        existingAttach.resource.version = newAttach.resource.version;
+                        alreadyAttached = true;
+                        break;
+                    }
+                }
+
+                if (!alreadyAttached)
+                {
+                    combinedFragmentList.Add(newAttach);
+                }
+            }
+
+            int numNativeSubpasses = 0;
+
+            // Fill out the subpass descriptors for the native renderpasses
+            // NOTE: Not all graph subpasses get an actual native pass:
+            // - There could be passes that do only non-raster ops (like setglobal) and have no attachments. They don't get a native pass
+            // - Renderpasses that use exactly the same rendertargets at the previous pass use the same native pass. This is because
+            //   nextSubpass is expensive on some platforms (even if its' essentially a no-op as it's using the same attachments).
+            SubPassDescriptor lastPass = new SubPassDescriptor();
+
+            for (var graphPassIndex = 0; graphPassIndex < nativePass.numGraphPasses + 1; ++graphPassIndex)
+            {
+                SubPassDescriptor desc = new SubPassDescriptor();
+                var graphPass = graphPassIndex < nativePass.numGraphPasses
+                    ? contextData.passData.ElementAt(nativePass.firstGraphPass + graphPassIndex)
+                    : passToMerge;
+
+
+                // We have no output attachments, this is an "empty" raster pass doing only non-rendering command so skip it.
+                if (graphPass.numFragments == 0)
+                {
+                    continue;
+                }
+
+                // If depth ends up being bound only because of merging we explicitly say that we will not write to it
+                // which could have been implied by leaving the flag to None
+                if (!graphPass.fragmentInfoHasDepth && nativePass.hasDepth)
+                {
+                    desc.flags = SubPassFlags.ReadOnlyDepth;
+                }
+
+                // MRT attachments
+                {
+                    int fragmentIdx = 0;
+                    int colorOffset = (graphPass.fragmentInfoHasDepth) ? -1 : 0;
+
+                    desc.colorOutputs = new AttachmentIndexArray(graphPass.numFragments + colorOffset);
+
+                    foreach (ref readonly var fragment in graphPass.Fragments(contextData))
+                    {
+                        // Check if we're handling the depth attachment
+                        if (graphPass.fragmentInfoHasDepth && fragmentIdx == 0)
+                        {
+                            desc.flags = (fragment.accessFlags.HasFlag(AccessFlags.Write))
+                                ? SubPassFlags.None
+                                : SubPassFlags.ReadOnlyDepth;
+                        }
+
+                        // It's a color attachment
+                        else
+                        {
+                            // Find the index of this subpass's attachment in the native renderpass attachment list
+                            int colorAttachmentIdx = -1;
+                            for (int fragmentId = 0; fragmentId < combinedFragmentList.size; ++fragmentId)
+                            {
+                                if (combinedFragmentList[fragmentId].resource.index == fragment.resource.index)
+                                {
+                                    colorAttachmentIdx = fragmentId;
+                                    break;
+                                }
+                            }
+
+                            // Set up the color indexes
+                            desc.colorOutputs[fragmentIdx + colorOffset] = colorAttachmentIdx;
+                        }
+
+                        fragmentIdx++;
+                    }
+                }
+
+                // FB-fetch attachments
+                {
+                    int inputIndex = 0;
+
+                    desc.inputs = new AttachmentIndexArray(graphPass.numFragmentInputs);
+
+                    foreach (ref readonly var fragmentInput in graphPass.FragmentInputs(contextData))
+                    {
+                        // Find the index of this subpass's attachment in the native renderpass attachment list
+                        int inputAttachmentIdx = -1;
+                        for (int fragmentId = 0; fragmentId < combinedFragmentList.size; ++fragmentId)
+                        {
+                            if (combinedFragmentList[fragmentId].resource.index == fragmentInput.resource.index)
+                            {
+                                inputAttachmentIdx = fragmentId;
+                                break;
+                            }
+                        }
+
+                        desc.inputs[inputIndex] = inputAttachmentIdx;
+                        inputIndex++;
+                    }
+                }
+
+                // Check if we can merge the native sub pass with the previous one
+                if (numNativeSubpasses == 0 || !NativePassCompiler.IsSameNativeSubPass(ref desc, ref lastPass))
+                {
+                    lastPass = desc;
+                    numNativeSubpasses++;
+                }
+            }
+
+            return numNativeSubpasses;
+        }
+
         public static PassBreakAudit TryMerge(CompilerContextData contextData, int activeNativePassId, int passIdToMerge)
         {
             var passBreakAudit = CanMerge(contextData, activeNativePassId, passIdToMerge);

Packages/com.unity.render-pipelines.core/Tests/Editor/NativePassCompilerRenderGraphTests.cs

@@ -411,6 +411,32 @@ public void TooManyAttachmentsBreaksPass()
             Assert.AreEqual(Rendering.RenderGraphModule.NativeRenderPassCompiler.PassBreakReason.AttachmentLimitReached, passes[0].breakAudit.reason);
         }
 
+        [Test]
+        public void NativeSubPassesLimitNotExceeded()
+        {
+            var g = AllocateRenderGraph();
+            var buffers = ImportAndCreateBuffers(g);
+
+            // Native subpasses limit is 8 so go above
+            for (int i = 0; i < Rendering.RenderGraphModule.NativeRenderPassCompiler.NativePassCompiler.k_MaxSubpass + 2; i++)
+            {
+                using var builder = g.AddRasterRenderPass<RenderGraphTestPassData>($"TestPass_{i}", out var passData);
+                builder.SetInputAttachment(buffers.extraBuffers[1 - i % 2], 0);
+
+                builder.SetRenderAttachmentDepth(buffers.depthBuffer);
+                builder.SetRenderAttachment(buffers.extraBuffers[i % 2], 1);
+
+                builder.SetRenderFunc((RenderGraphTestPassData data, RasterGraphContext context) => { });
+            }
+
+            var result = g.CompileNativeRenderGraph(g.ComputeGraphHash());
+            var passes = result.contextData.GetNativePasses();
+
+            Assert.AreEqual(2, passes.Count);
+            Assert.AreEqual(Rendering.RenderGraphModule.NativeRenderPassCompiler.NativePassCompiler.k_MaxSubpass, passes[0].numGraphPasses);
+            Assert.AreEqual(Rendering.RenderGraphModule.NativeRenderPassCompiler.PassBreakReason.SubPassLimitReached, passes[0].breakAudit.reason);
+        }
+
         [Test]
         public void AllocateFreeInMergedPassesWorks()
         {
@@ -814,7 +840,7 @@ public void ResolveMSAAImportColor()
             importInfoDepth.msaaSamples = 4;
             importInfoDepth.volumeDepth = 1;
             importInfoDepth.format = GraphicsFormat.D32_SFloat_S8_UInt;
-            
+
             ImportResourceParams importResourceParams = new ImportResourceParams();
             importResourceParams.clearOnFirstUse = true;
             importResourceParams.discardOnLastUse = true;
@@ -846,7 +872,7 @@ public void ResolveMSAAImportColor()
             var result = g.CompileNativeRenderGraph(g.ComputeGraphHash());
             var passes = result.contextData.GetNativePasses();
 
-            // Validate nr pass 
+            // Validate nr pass
             Assert.AreEqual(1, passes.Count);
             Assert.AreEqual(2, passes[0].attachments.size);
             Assert.AreEqual(1, passes[0].numGraphPasses);