Improving DLSS cpu load, and not recreating DLSS state every change of res #5231

kecho · 2021-07-26T17:42:04Z

Purpose of this PR

DLSS was recreating state every change of resolution for DRS. This was a bit of an oversight, caused by not checking the min and max resolution that DLSS suggest
Turns out that we should always query for the min and max resolution regardless if auto parameters are enabeld or not. This range is the only range were we are allowed to utilize hot DRS scaling. Otherwise, if we request outside this range, we must in turn recrete the DRS feature state.

The non technical summary: we optimized CPU usage and straining of system. DRS should run much smoother.

Fixed also a problem were if the viewport is odd size of pixels, the DRS scaler would return a 1 pixel extra. So to fix this, clamped the actualWidth/actualHeight post resolution.

Testing status

Tested on vulkan and dx11 using the test scene from the bug.
Tried to repro crash, could not. So ended up finding this issue and fixing it.
QA Must do a full test of DLSS: dx12, retest vulkan and do this with software and hardware drs. Check on CPU usage.

Comments to reviewers

I renamed some functions but the jist of the change is that we now call GetOptimalSettings every frame, use the safe ranges for DRS, and optionally use the sharpness / autoscaler on the setting.

github-actions · 2021-07-26T17:42:19Z

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/.yamato%252F_abv.yml%2523all_project_ci_2021.2
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

FrancescoC-unity · 2021-07-27T07:34:58Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/Camera/HDCamera.cs

-                actualWidth = scaledSize.x;
-                actualHeight = scaledSize.y;
+                //ensure that the scale is within and we dont round up an extra pixel
+                actualWidth = Math.Min(scaledSize.x, actualWidth);


Feels to me this should be done inside DynamicResolutionHandler.instance.GetScaledSize itself, I cannot think of a scenario in which we want to exceed.

Moreover if at some point we modify the dynamic resolution to scale up as well, it'd be good to have it self contained in the handler.

yeah its possible since the drs handler is aware of the viewport size.
Just need to be more careful with this though since now we are making this function dependant on a final viewport size and test a bit more.

Oh no I mean clamping to the thing that is passed to GetScaledSize (in this case is actualWidth/Height)

ah! right. Yes this should be easy

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/RenderPass/DLSSPass.cs

FrancescoC-unity · 2021-07-27T07:49:23Z

This range is the only range were we are allowed to utilize hot DRS scaling. Otherwise, if we request outside this range, we
must in turn recrete the DRS feature state.

For my curiosity: How inclusive are those "optimal settings"? Is there a threshold in which we thrash back and forth the recreation?

kecho · 2021-07-27T14:46:51Z

This range is the only range were we are allowed to utilize hot DRS scaling. Otherwise, if we request outside this range, we
must in turn recrete the DRS feature state.

For my curiosity: How inclusive are those "optimal settings"? Is there a threshold in which we thrash back and forth the recreation?

@FrancescoC-unity Depending on the quality, it can go down to 20% (for the high performance settings) up to 50% (for the high quality settings).
But is usually between 20% or 50%. Anything below they force you to recreate the state. If you dont, then you get internal exceptions in the library.

FrancescoC-unity · 2021-07-27T14:50:50Z

com.unity.render-pipelines.core/Runtime/Common/DynamicResolutionHandler.cs

@@ -501,6 +501,10 @@ public Vector2Int GetScaledSize(Vector2Int size)
            }

            Vector2Int scaledSize = ApplyScalesOnSize(size);
+
+            scaledSize.x = Math.Min(scaledSize.x, size.x);


I think we should do this in ApplyScalesOnSize though as that is public API too, specifically this variant :

Vector2Int ApplyScalesOnSize(Vector2Int size, Vector2 scales)

remi-chapelain

So, for now, I quickly tested and can still repro the crash.
I've narrowed the fact that it's DX12 related (doesn't happen in vulkan or dx11).
@kecho managed to repro it so hopefully we'll be able to find the cause.

in the meantime, there's a crash folder zipped, if needed.
Crash_2021-07-27_151018460.zip

kecho · 2021-07-27T20:27:42Z

So, for now, I quickly tested and can still repro the crash.
I've narrowed the fact that it's DX12 related (doesn't happen in vulkan or dx11).
@kecho managed to repro it so hopefully we'll be able to find the cause.

in the meantime, there's a crash folder zipped, if needed.
Crash_2021-07-27_151018460.zip

@remi-chapelain for now I have removed the fogbugz on this PR, and instead keep this PR as an optimization. So as long as DLSS is running well on this PR we should still pursue it.
I have another PR in ono with second attempt fix for the crash (fix in c++)

FrancescoC-unity · 2021-07-28T07:35:27Z

com.unity.render-pipelines.core/Runtime/Common/DynamicResolutionHandler.cs

@@ -421,8 +421,10 @@ public void Update(GlobalDynamicResolutionSettings settings, Action OnResolution
            if (!m_ForceSoftwareFallback && type == DynamicResolutionType.Hardware)
            {
                hardwareResolutionChanged = FlushScalableBufferManagerState();
-                if (ScalableBufferManager.widthScaleFactor != m_PrevHWScaleWidth ||
-                    ScalableBufferManager.heightScaleFactor != m_PrevHWScaleHeight)
+                float sfw = ScalableBufferManager.widthScaleFactor;


This change is a no-op right? I don't see the advantage as I tend to like more verbose naming vs acronyms, but am not opposed. However, if it is for clarity let me be an anal nitpicker and either we have sfw and sfh or wsf and hsf, now is a bit of a mixup of names.,

This is left over on my side! i was debugging stuff. Good catch!

FrancescoC-unity · 2021-07-28T07:40:56Z

com.unity.render-pipelines.core/Runtime/Common/DynamicResolutionHandler.cs

@@ -391,7 +391,7 @@ public void Update(GlobalDynamicResolutionSettings settings, Action OnResolution
        {
            ProcessSettings(settings);

-            if (!m_Enabled && !s_ActiveInstanceDirty)
+            if (!m_Enabled || !s_ActiveInstanceDirty)


I am not sure what this change is doing?

So s_ActiveInstanceDirty is determining whether we changed anything (is set to false at the end of this), so essentially this is going to call the update every 2 frames instead of every frame which of course is not what we want to do.
Am I missing something?

This should have been an or conditional.
Basically dont run this function if we already called it in the frame.
The reason is that we have 2 loops for cameras within the same frame.
Each loop calls UpdateAndUseCamera. So a camera calls Update() twice. But we only need to flush all the settings once per frame.

ClearSelectedCamera gets called at the end of the frame and sets this dirty flag to true.

THe problem is that this was false, it should be true. This fixes some crazy artifacting when resizing Hardware resolution scaler.

Gotcha! I missed the

ClearSelectedCamera gets called at the end of the frame and sets this dirty flag to true.

part.

Makes all sense.

FrancescoC-unity · 2021-07-28T14:28:09Z

@kecho still missing https://github.com/Unity-Technologies/Graphics/pull/5231/files#r677527579 and https://github.com/Unity-Technologies/Graphics/pull/5231/files#diff-e967f213a4a311774292867c47260bc3ca8c133c098e42f023b0e0809e55d318R342

rest LGTM

remi-chapelain

This doesn't fix the crash (reported here) but after some quick tests on the template, it seems to improve slightly performance around 10% give or take.

I also tested on Vulkan, DX11 and DX12 to be sure that it didn't cause any issues there.

kecho · 2021-07-28T15:25:00Z

Indeed. The crash will be fixed at a later point. I will do a debug session with @remi-chapelain since we can only repro it on his mahcine. This however is a good change for performance improvements.

kecho · 2021-07-29T19:38:22Z

@kecho still missing https://github.com/Unity-Technologies/Graphics/pull/5231/files#r677527579 and https://github.com/Unity-Technologies/Graphics/pull/5231/files#diff-e967f213a4a311774292867c47260bc3ca8c133c098e42f023b0e0809e55d318R342

rest LGTM
@FrancescoC-unity done!

remi-chapelain

In addition to the slight improvement, this PR fixes this issue (#1354184)

… resolution Changelog Moving complexity into the core package instead Spaces / and formatting Visual artifacts on hardware DRS because we were flushing the dynamic resolution too much Removing left over changes from debugging

kecho requested a review from FrancescoC-unity July 26, 2021 17:42

github-actions bot added the HDRP label Jul 26, 2021

kecho requested review from sebastienlagarde and remi-chapelain July 26, 2021 17:42

kecho force-pushed the HDRP/DLSSBetterDRS branch from c603ca7 to e874a3a Compare July 26, 2021 21:21

FrancescoC-unity reviewed Jul 27, 2021

View reviewed changes

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/RenderPass/DLSSPass.cs Outdated Show resolved Hide resolved

kecho force-pushed the HDRP/DLSSBetterDRS branch from e874a3a to 353ade3 Compare July 27, 2021 14:43

kecho requested a review from FrancescoC-unity July 27, 2021 14:45

FrancescoC-unity reviewed Jul 27, 2021

View reviewed changes

remi-chapelain reviewed Jul 27, 2021

View reviewed changes

kecho changed the title ~~[FogBugz # 1352848] Improving DLSS cpu load, and not recreating DLSS state every change of res~~ Improving DLSS cpu load, and not recreating DLSS state every change of res Jul 27, 2021

kecho requested review from remi-chapelain and FrancescoC-unity July 27, 2021 20:27

FrancescoC-unity reviewed Jul 28, 2021

View reviewed changes

kecho force-pushed the HDRP/DLSSBetterDRS branch from 73e7729 to a82c584 Compare July 28, 2021 13:40

kecho requested a review from FrancescoC-unity July 28, 2021 13:40

remi-chapelain approved these changes Jul 28, 2021

View reviewed changes

FrancescoC-unity mentioned this pull request Jul 29, 2021

[Fogbugz # 1352093] Fixing bad copy texture when DRS is on with ray tracing. #5232

Merged

kecho marked this pull request as ready for review July 29, 2021 19:08

kecho force-pushed the HDRP/DLSSBetterDRS branch from e34b39b to 09ce4aa Compare July 29, 2021 19:39

remi-chapelain self-requested a review July 30, 2021 07:28

FrancescoC-unity approved these changes Jul 30, 2021

View reviewed changes

remi-chapelain approved these changes Jul 30, 2021

View reviewed changes

sebastienlagarde approved these changes Jul 30, 2021

View reviewed changes

kecho added 2 commits July 30, 2021 08:49

Fixes from code review

c7f674d

kecho force-pushed the HDRP/DLSSBetterDRS branch from 09ce4aa to c7f674d Compare July 30, 2021 12:49

kecho merged commit ad100f7 into master Jul 30, 2021

kecho deleted the HDRP/DLSSBetterDRS branch July 30, 2021 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving DLSS cpu load, and not recreating DLSS state every change of res #5231

Improving DLSS cpu load, and not recreating DLSS state every change of res #5231

kecho commented Jul 26, 2021 •

edited

github-actions bot commented Jul 26, 2021 •

edited

FrancescoC-unity Jul 27, 2021

kecho Jul 27, 2021

FrancescoC-unity Jul 27, 2021

kecho Jul 27, 2021

FrancescoC-unity commented Jul 27, 2021

kecho commented Jul 27, 2021 •

edited

FrancescoC-unity Jul 27, 2021

remi-chapelain left a comment

kecho commented Jul 27, 2021

FrancescoC-unity Jul 28, 2021

kecho Jul 28, 2021

FrancescoC-unity Jul 28, 2021

kecho Jul 28, 2021

FrancescoC-unity Jul 28, 2021

FrancescoC-unity commented Jul 28, 2021 •

edited

remi-chapelain left a comment

kecho commented Jul 28, 2021

kecho commented Jul 29, 2021

remi-chapelain left a comment

Improving DLSS cpu load, and not recreating DLSS state every change of res #5231

Improving DLSS cpu load, and not recreating DLSS state every change of res #5231

Conversation

kecho commented Jul 26, 2021 • edited

Purpose of this PR

Testing status

Comments to reviewers

github-actions bot commented Jul 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrancescoC-unity commented Jul 27, 2021

kecho commented Jul 27, 2021 • edited

Choose a reason for hiding this comment

remi-chapelain left a comment

Choose a reason for hiding this comment

kecho commented Jul 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrancescoC-unity commented Jul 28, 2021 • edited

remi-chapelain left a comment

Choose a reason for hiding this comment

kecho commented Jul 28, 2021

kecho commented Jul 29, 2021

remi-chapelain left a comment

Choose a reason for hiding this comment

kecho commented Jul 26, 2021 •

edited

github-actions bot commented Jul 26, 2021 •

edited

kecho commented Jul 27, 2021 •

edited

FrancescoC-unity commented Jul 28, 2021 •

edited