[10.x] Metal support updates [Hold] #1795
Conversation
…ATFORM_LANE_COUNT if they're available and device prefers it
…e, or when MSAA TextureArray support is limited
…regardless of the OS)
sebastienlagarde
requested review from
FrancescoC-unity,
EvgeniiG,
JulienIgnace-Unity,
jessebarker,
fabien-unity,
julienf-unity,
kaychang-unity and
Wilfrid-Unity
| @@ -199,13 +199,6 @@ | |||
| }, | |||
| "url": "https://artifactory.prd.cds.internal.unity3d.com/artifactory/api/npm/upm-candidates" | |||
| }, | |||
| "com.unity.purchasing": { | |||
There was a problem hiding this comment.
@TomasKiniulis FYI
| @@ -191,13 +191,6 @@ | |||
| }, | |||
| "url": "https://artifactory.prd.cds.internal.unity3d.com/artifactory/api/npm/upm-candidates" | |||
| }, | |||
| "com.unity.purchasing": { | |||
There was a problem hiding this comment.
@TomasKiniulis fyi
| @@ -3048,6 +3026,7 @@ static void ClearLightList(in BuildGPULightListParameters parameters, CommandBuf | |||
| cmd.SetComputeBufferParam(parameters.clearLightListCS, parameters.clearLightListKernel, HDShaderIDs._LightListToClear, bufferToClear); | |||
| cmd.SetComputeIntParam(parameters.clearLightListCS, HDShaderIDs._LightListEntries, bufferToClear.count); | |||
|
|
|||
| // TODO: Round on DeviceInfo.optimalThreadGroupSize so we have optimal thread for ClearList kernel | |||
There was a problem hiding this comment.
In current shape of things, won't this give the wrong number of groups dispatched? We still set NR_THREADS to 32 in the shader if that's the optimal lane count.
There was a problem hiding this comment.
It's a comment that this needs to be updated once PLATFORM_LANE_COUNT_32 path is enabled in ClearLightLists.compute, it's currently commented out
There was a problem hiding this comment.
What I mean is that if anywhere PLATFORM_LANE_COUNT is defined != 64 this will fail. Unless we are sure nowhere that will be defined.
(This is in the shader)
#ifdef PLATFORM_LANE_COUNT
#define NR_THREADS PLATFORM_LANE_COUNT
#else
#define NR_THREADS 64 // default to 64 threads per group
#endif
| if (samples == MSAASamples.None) | ||
| return samples; | ||
|
|
||
| GraphicsFormat ldrFormat = SystemInfo.GetGraphicsFormat(DefaultFormat.LDR); |
There was a problem hiding this comment.
Why do you use SystemInfo.GetGraphicsFormat(DefaultFormat.LDR); ? to check this.
We need to check every format that will use MSAA.
i.e ColorBufferFormat (which can be selected to be RGBA8 and RGBA16F) for example and other RT like R32 we use for depth in prepass when MSAA is enabled and other stuff. We don't have a single format here.
Is the intent of this function to only check if RGBA8 MSAA sample count is supported or I miss something?
There was a problem hiding this comment.
The idea is to use any texture format to figure out what is the generic supported range of MSAA samples on your GfxDevice, it's an improvement over to current assumption.
I don't think our C# APIs have any better way to query a supported MSAA range in other way (if your GfxDevice doesn't support 8x but 2/4 only, or if some really old GPUs wouldn't support 2x). Shaders could pick up some 8x variant for MSTextures use, so this isn't something that we can silently fallback underneath SRP within GfxDevice implementation.
| return samples; | ||
|
|
||
| GraphicsFormat ldrFormat = SystemInfo.GetGraphicsFormat(DefaultFormat.LDR); | ||
| MSAASamples[] probe = {MSAASamples.MSAA2x, MSAASamples.MSAA4x, MSAASamples.MSAA8x}; |
There was a problem hiding this comment.
This is dangerous, if we add MSAA16x in the future this will not be catch, we should generate the array from the enum .
i.e var valuesAsArray = Enum.GetValues(typeof(Enumnum));
There was a problem hiding this comment.
How are you suggesting to automatically handle MSAASamples.MSAAx mapping into FormatUsage.MSAAx that would still be a problem?
| int minSampleCount = 0; | ||
| int maxSampleCount = 0; | ||
|
|
||
| // Some GPUs might not support MSAA2x, so probe the supported range for clamping |
There was a problem hiding this comment.
Maybe comment that it is for Metal API . Other platform don't have this problem
There was a problem hiding this comment.
I believe some APIs allow for the implementation to give you a different sample count than you asked for, but I'm not sure we can always probe for that. Maybe Antti can clarify what this is defending against.
There was a problem hiding this comment.
Apple GPUs don't support MSAA 8x, but if we're checking the highest "generic format" MSAA range supported, you might as well check what is the lowest, because historically there has been GPUs outside of Metal domain that haven't supported 2x, but 4x only.
| @@ -40,7 +40,6 @@ unsafe struct ShaderVariablesDebugDisplay | |||
| public int _DebugSingleShadowIndex; | |||
|
|
|||
| public int _DebugProbeVolumeMode; | |||
| public Vector3 _DebugDisplayPad0; | |||
There was a problem hiding this comment.
Question: Why are we not catching issue currently with our test. We have debug mode test.
I would like to add a graphic test to catch this for the future, what kind of test should I add? thanks
There was a problem hiding this comment.
This is harmless, but takes up extra space in the constant buffer. We need a generic mechanism to validate/enforce something like "std140" from Khronos that works across all APIs (and we don't have that at the moment).
| @@ -1,5 +1,8 @@ | |||
| #pragma kernel ClearDispatchIndirect | |||
| #pragma kernel ClearDrawProceduralIndirect | |||
|
|
|||
| #pragma multi_compile _ PLATFORM_LANE_COUNT_32 | |||
There was a problem hiding this comment.
Given that this is static per platform, why a multi compile vs defining PLATFORM_LANE_COUNT in relevant include files?
There was a problem hiding this comment.
Using multi compiles will slow down build times so I think we should avoid it if possible.
There was a problem hiding this comment.
For optimizing against specific GPU models, we need to be able to pick up PLATFORM_LANE_COUNT_32 during runtime based on the actual GPU model used (AMD vs Apple GPU)
| passData.depthAsColorBuffer = builder.WriteTexture(renderGraph.CreateTexture(new TextureDesc(Vector2.one, true, true) | ||
| { colorFormat = GraphicsFormat.R32_SFloat, clearBuffer = true, clearColor = Color.black, bindTextureMS = true, enableMSAA = true, name = "DepthAsColorMSAA" })); | ||
| { colorFormat = depthAsColorMSAAFormat, clearBuffer = true, clearColor = Color.black, bindTextureMS = true, enableMSAA = true, name = "DepthAsColorMSAA" })); |
There was a problem hiding this comment.
dimension must also be set to dimensionMSAA here.
This is currently done implicitly in the constructor of TextureDesc where slices and dimension are set based on the xrReady parameter but now that we have a specific one for MSAA we need to make it explicit.
| @@ -10,6 +10,18 @@ | |||
| #pragma kernel TileLightListGen_DepthRT_SrcBigTile_Oblique LIGHTLISTGEN=TileLightListGen_DepthRT_SrcBigTile_Oblique ENABLE_DEPTH_TEXTURE_BACKPLANE USE_TWO_PASS_TILED_LIGHTING USE_OBLIQUE_MODE | |||
| #pragma kernel TileLightListGen_DepthRT_MSAA_SrcBigTile_Oblique LIGHTLISTGEN=TileLightListGen_DepthRT_MSAA_SrcBigTile_Oblique ENABLE_DEPTH_TEXTURE_BACKPLANE MSAA_ENABLED USE_TWO_PASS_TILED_LIGHTING USE_OBLIQUE_MODE | |||
|
|
|||
| #pragma kernel TileLightListGen_NoDepthRT_LE LIGHTLISTGEN=TileLightListGen_NoDepthRT_LE PLATFORM_LANE_COUNT_32 | |||
There was a problem hiding this comment.
We should simply use a multi compile here, like for the other one. I don't see the need for a "LE" thing here.
There was a problem hiding this comment.
I guess I have a global misunderstand about what "LE" mean. it is just to have 32 vs 64? Or does it mean Lightweight?
There was a problem hiding this comment.
LE was originally a short for something of LowEnd, for DeviceInfo to prefer a specific suffix if they happen to exist. I thought about renaming it as _32 as well.
ComputeGgxIblSampleData.compute has nothing to do with PLATFORM_LANE_COUNT_32, so it made sense to use _LE naming convention there to differentiate MAX_IBL_SAMPLE_CNT=89/MAX_IBL_SAMPLE_CNT=34 kernel pickup to runtime.
| int w = Mathf.RoundToInt(viewportWidth * screenFraction); | ||
| int h = Mathf.RoundToInt(viewportHeight * screenFraction); | ||
| int d = sliceCount; | ||
| const int kMax3DTextureSize = 2048; // TODO: There's no SystemInfo.maxTextureSize equivalent for 3D textures |
There was a problem hiding this comment.
So it's bad but I we need to solve it with adding SystemInfo.maxTextureSize for 3D texture as you said. So we should at least put a warning here Debug.Warning() to inform there is an issue with the same (and that it is a limit on metal platform).
There was a problem hiding this comment.
This has nothing to do with Metal, it's a generic limitation on DX11 as well. When using volumetrics dynamic resolution slider, you will easily go over the supported limit on any target. We've talked about this with @EvgeniiG before, besides checking for the maximum resolution, maybe there's some improvements that could be made into the actual dynamic resolution scaler itself? Now i was able to hit the resolution limits around 70% of the slider
There was a problem hiding this comment.
Changes need to be done to the Render Graph code path.
Unfortunately this code path is not enabled by default yet and given the design, most render texture creation code is duplicated in there so msaa changes need to be replicated.
Here's the exhaustive (I hope :p ) list of the places that need to be updated:
HDRenderPipeline.RenderGraph.cs
- CreateColorBuffer
- ResolveMSAAColor (device resolve logic)
- ResolveMotionVector (device resolve logic)
HDRenderPipeline.Debug.cs
- RenderDebugViewMaterial
HDRenderPipeline.LightLoop.cs
- CreateDiffuseLightingBuffer
HDRenderPipeline.Prepass.cs
- CreateDepthBuffer (already done)
- CreateNormalBuffer
- CreateDecalPrepassBuffer
- CreateMotionVectorBuffer
- RenderDepthPrepass (in msaa condition where we create a R32F buffer)
- ResolvePrepassBuffers (device resolve logic)
HDRenderPipeline.SubsurfaceScattering.cs
- CreateSSSBuffer
Also, it's better if you test this as well with the render graph path. You can run with -rendergraph-tests command line to enable render graph by default (or hardcode enableRenderGraphTests to true in HDRenderPipeline.cs)
| RWTexture2D<float4> outputResult; // [MAX_SAMPLE_CNT x UNITY_SPECCUBE_LOD_STEPS] | ||
|
|
||
| #pragma kernel ComputeGgxIblSampleData | ||
| #pragma kernel ComputeGgxIblSampleData COMPUTEGGGXIBLSAMPLEDATA=ComputeGgxIblSampleData MAX_IBL_SAMPLE_CNT=89 |
There was a problem hiding this comment.
I guess I have a global misunderstand about what "LE" mean. it is just to have 32 vs 64? Or does it mean Lightweight?
here it is not a matter of 32 vs 64 thread size, it is for performance. Mobile and switch don't have the power to process the 89 sample of PC / console, so we use a lower number of sample.
We shouldn't change this.
There was a problem hiding this comment.
For future Macs, we need both of these to be available based on GPU used during runtime.
| // Set the motion vector input texture | ||
| Shader.SetGlobalTexture(HDShaderIDs._MotionVectorTextureMS, m_MotionVectorsMSAART); | ||
| if (m_MotionVectorsSupport) | ||
| cmd.ResolveAntiAliasedSurface(m_MotionVectorsMSAART.rt, m_MotionVectorsRT.rt); |
There was a problem hiding this comment.
I don't see how this code can work. We can't rely on "auto" resolve for our buffer. If we use a shader for the resolve, it is because we need to do particular software operation to do the resolve (like taking the closted depth sample), we don't blindly average vector value for example.
See
D:\newSRP\Graphics\com.unity.render-pipelines.high-definition\Runtime\RenderPipeline\RenderPass\MSAA\DepthValues.shader
So it mean we are block for Metal? auto resolve will give us poor result and we can't used our software resolve? or it was just an optimization?
| if (DeviceInfo.requiresExplicitMSAAResolve == false && | ||
| CompareRTHandle(msaaTarget, simpleTarget)) | ||
| { | ||
| cmd.ResolveAntiAliasedSurface(msaaTarget.rt, simpleTarget.rt); |
There was a problem hiding this comment.
Same issue as above, our color resolve rely on a tonemap operation for the average, it is not just an average. We can't rely on auto resolve.
| @@ -5,9 +5,14 @@ | |||
|
|
|||
| public class SetToCameraNearPlane : MonoBehaviour | |||
| { | |||
| #if UNITY_EDITOR | |||
There was a problem hiding this comment.
I will report this in another PR, thanks for the fix
There was a problem hiding this comment.
you can remove it from this PR
There was a problem hiding this comment.
Overall I would like the thought of everyone regarding the 32 vs 64 thread change in this PR.
As I can read it is static (mobile / switch use 32 and other platform use 64 for now.
So I don't see the need for all the change and extra PLATFORM_LANE_COUNT_32, in particular as it is not handled correctly everywhere.
So two solution here.
Either:
- We assume PLATFORM_LANE_COUNT is define statically and we remove the various change here including the "LE".
- We assume PLATFORM_LANE_COUNT is dynamic (which is true for PC, don't know for mobile), and in this case we should correctly handle it. i.e Everywhere we use PLATFORM_LANE_COUNT we do add a multicompile and we select one variant based on current platform or GPU card (we could check for example if we have NVidia string in name of GPU card and select the right variant to used based on this in addition to do it per platform. If we chose this path, we then add an option to strip the unused variant in the stripper (for example when building for PS4, we reject the various 32 thread variant.
I have looked at all the shader relying on PLATFORM_LANE_COUNT and they are all compute / shader that are low compile time (i.e it is not a forward or deferred shader), so it should be rather easily doable and shouldn't add any extra build time expect on platform like PC (and maybe mobile if there is case for non 32?).
What are people thought? would allow to have more optimized code for PC (but after that imply problem on test side). It all depends how much platform required a variant, and as of today it was mainly only PC we have done this optimization.
|
Depends on thought of people we may simply simplify this PR so it work for metal and do the other architecture change (32 vs 64) on another PR. Note: I am also concern by the auto resolve for MSAA. This one can't work with our code as we need explicit software resolve. What are our alternative here? Lastly, this PR break our rendergraph as the code is not done for it in the PR, we can handle it for you, but we need first to solve the other problem. |
| @@ -14,4 +18,52 @@ public enum MSAASamples | |||
| /// <summary>MSAA 8X.</summary> | |||
| MSAA8x = 8 | |||
| } | |||
|
|
|||
| public static class MSAASamplesExtensions | |||
There was a problem hiding this comment.
There is currently no point in making this public so I think we should make it internal.
|
|
||
| namespace UnityEngine.Rendering.HighDefinition | ||
| { | ||
| public class DeviceInfo |
There was a problem hiding this comment.
Same as above, there is currently no point in making this public so I think we should make it internal.
There was a problem hiding this comment.
@julienf-unity , @aet : what is not working with VFXGraph + non-array for RT ? Let's fix that instead of adding all these changes to TextureXR, and the extra API complexity around it.
| /// <summary> | ||
| /// Dimension of XR MSAA textures. | ||
| /// </summary> | ||
| public static TextureDimension dimensionMSAA |
There was a problem hiding this comment.
Please do no add this extra API just for Metal.
Let's fix the VFX Graph instead to deal correctly with non-array.
There was a problem hiding this comment.
Ok.. so we've had these known issues in Spaceship demo's main menu (an on purpose screen glitch is gray instead of coloured) and VFX sparkles not bouncing off from the surfaces as expected, and we've had people working on this project and talking about this limitation/difference against Metal backend a year now? :)
All discussions so far that I've seen suggest that there's something in Metal that should be fixed. But we really can't move on to enabling SPI/XR stuff anytime soon, and on some platforms you shouldn't expect XR/SPI at all.
It makes sense for VFX to share TextureXR, but it's showing this problem where it's clearly depending on an API that locks you into XR/SPI assumptions, VFXGraph should work on targets without SPI too, where you still have texture arrays available.
There was a problem hiding this comment.
Yes problem with this is that the "correct" fix is tricky:
- We can add a type for CameraBuffers in VFX Graph that will abstract the actual TextureXR. Problem with that is that we lose some interoperability with Shader Graph (as they sont support TextureXR)... So we have a problem UX wise here.
Or we can also solve it by not having TextureXR leak outside of shader code which means SPR API to access camera buffer always return Texture2D (Was the case before but it was changed for RTX I dont exactly remember why). We could also fix the interop issues by allowing a Texture2D view on a texture2DArray first slice...
But I agree with Fabien that fixing it just by changing the type in metal is not an actual fix. Issues still remain for other platforms that dont support XR like XBox for instance.
There was a problem hiding this comment.
And also a problem of when stuff can get properly fixed vs having something working in the meanwhile :) My changes are not super intrusive though within TextureXR, and it will allow fixing the same issue on other platforms without XR/SPI as needed, but of course I tried to restrict actual changes to Metal runtime only when possible
There was a problem hiding this comment.
The MSAA texture array support situation is more unfortunate though. We always have features that "not everyone" depends on automatically, but even on current desktop mac gpu feature table, the availability depends too much to make it a hard requirement. For iOS it's simply not available at all
There was a problem hiding this comment.
Sure, but it will add a new public API that we'll have to maintain for many years ... let's please avoid that and fix the root issue.
@julienf-unity : is the Xbox issue tracked anywhere? I'll be happy to help out with fixing VFX/Shader Graph to properly support texarray or not.
There was a problem hiding this comment.
@fabien-unity how do we fix the root issue of having a Graphics Device without MSAA texture support, other than not using it when it's not available? :) It's a less common problem than limited XR/SPI availability, but equally could happen elsewhere too than just Metal. The main interest for HDRP users is deferred rendering in any case
There was a problem hiding this comment.
@fabien-unity Xbox issue is tracked via a single ticket for some failing graphics tests. specifically the depth collision one. https://fogbugz.unity3d.com/f/cases/1256635/
We can have a meeting to talk about how to fix the TextureXR issue. @JulienIgnace-Unity you might want to be here.
| { | ||
| // TODO: Could conditionally enable threadExecutionWidth32/disable preferComputeKernels based on GPU | ||
| // SystemInfo.hasHiddenSurfaceRemovalOnGPU == true on Apple GPUs | ||
| requiresExplicitMSAAResolve = false; |
There was a problem hiding this comment.
so this PR does not yet enable PLATFORM_LANE_COUNT_32 path when running on Mac/Apple GPU, but that's the ultimate goal for this, while cleaning up existing mobile/switch situation
…abled, always treat real as float. Use explicit half precision optimizations instead.
sebastienlagarde
changed the title
Purpose of this PR
Improve Metal support on macOS Intel/Apple Silicon.
Testing status
Manual Tests: What did you do?
Automated Tests: What did you setup? (Add a screenshot or the reference image of the test please)
Yamato: (Select your branch):
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics
Any test projects to go with this to help reviewers?
Comments to reviewers
Notes for the reviewers you have assigned.