Skip to content

Arm: Optimize away unnecessary clear empty RenderPass (Vulkan) in 2DRP. #4149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SergioAlapontARM
Copy link

@SergioAlapontARM SergioAlapontARM commented Apr 12, 2021

The current logic of ScriptableRenderer assumes that RenderPasses using the camera target as colorAttachment will render to it immediately. If that's not the case it will result in empty RenderPasses in Vulkan clearing the RT which will end up in storing and loading to/from memory instead of staying on-chip. This patch gives support to override that logic to assume responsibility of clearing the camera color target and clears the attachments in "Render2DLightingPass.cs" only when needed. The following image shows the impact of this change in GPU counters (measured LostCrypt using Vulkan on a Mali-G77 device).

  • No significant changes in frame times were appreciated.

As we can see, the fragment workloads have been reduced resulting in an improvement of both bandwidth and "GPU Active Cycles".

BeforeVsAfter

…iptableRenderPass can assume responsibility of clearing the target manually.
@@ -932,7 +932,7 @@ private bool IsRenderPassEnabled(ScriptableRenderPass renderPass)
void SetRenderPassAttachments(CommandBuffer cmd, ScriptableRenderPass renderPass, ref CameraData cameraData)
{
Camera camera = cameraData.camera;
ClearFlag cameraClearFlag = GetCameraClearFlag(ref cameraData);
ClearFlag cameraClearFlag = renderPass.overrideCameraClear ? ClearFlag.None : GetCameraClearFlag(ref cameraData);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in 2D renderer you never care about camera clear flags? In this case I’d rather suggest to add logic for 2D renderer to override functionality of GetCameraClearFlags instead of adding internal API to ScriptableRenderPass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, any feedback on the injected passes issue?. As it stands, either with the current approach or if I override the camera clear flags to NONE in 2DRenderer, an earlier pass happening before Render2DLightingPass would render to a non-cleared camera RT and we would later clear such target overriding whatever was in there. I think the way to go here is to check whether there are ScriptableRenderFeatures queued and decide whether to allow clearing or not based on that. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SergioAlapontARM
Copy link
Author

Adding @tomzig16 as I believe he is the one managing our (Arm's) PRs.
@phi-lira, so I think adding the changes you propose and making sure the early clear still happens when there are injected features should work fine in all scenarios. How should we proceed with this PR?

@UnityPaul UnityPaul requested a review from unity-cchu April 23, 2021 14:28
@github-actions
Copy link

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)

URP
/.yamato%252Fall-urp.yml%2523PR_URP_trunk
With changes to URP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants