Support for transient attachments #8021

mdagois · 2024-08-07T08:16:49Z

Changes

Added support for transient attachment in the VulkanTexture class.
Used transient attachment for the MSAA sidecar texture.

Testing on a Pixel 8 Pro, AGI traces show a difference of roughly 100MB in GPU memory consumption.
The device was rebooted after each test to reduce the noise.

Figure 1: Transient disabled

Figure 2: Transient enabled

The actual difference is likely smaller.
The resolution is 1008x1920 with 4 samples and a 32-bit format (VK_FORMAT_B10G11R11_UFLOAT_PACK32).
In that configuration, the reduction in memory consumption should be around 30MB.

For reference, here are the perfetto traces: perfetto_traces.zip.

mdagois · 2024-08-08T04:57:26Z

As an additional note, I used the vkGetDeviceMemoryCommitment function to make sure the memory was not committed when using the transient attachment.
The results are that, for the allocation size was 30,965,760 bytes, the committed size is zero.
This is the expected behavior.
Unfortunately, the vkGetDeviceMemoryCommitment function cannot be used for non-lazily allocated memory, so we cannot do a apple-to-apple comparison.

poweifeng

I think this looks good, but it might be missing a bit, which is the part that was undone in #7680. See also the "// DONT_CARE is critical here" comment in this document: https://arm-software.github.io/vulkan-sdk/multisampling.html

mdagois · 2024-08-13T04:47:57Z

I think this looks good, but it might be missing a bit, which is the part that was undone in #7680. See also the "// DONT_CARE is critical here" comment in this document: https://arm-software.github.io/vulkan-sdk/multisampling.html

That means the number I got showing a 100MB saving is probably just happenstance then. :)
Let me try with the method you told me about offline (adb shell dumpsys meminfo app.name).

mdagois · 2024-08-14T04:22:01Z

I think this looks good, but it might be missing a bit, which is the part that was undone in #7680. See also the "// DONT_CARE is critical here" comment in this document: https://arm-software.github.io/vulkan-sdk/multisampling.html

I fixed the issue by setting the loadOp based on whether the attachement uses lazily allocated memory.
Please have a look.

mdagois · 2024-08-14T04:29:25Z

With the discard storeop, I can see the committed memory stays at zero for the MSAA attachement for as long as the application runs.
When switching to a store storeop, I can see the committed memory turning to 30MB.
So, the transient system seems to work as intended.

poweifeng

awesome work!

pixelflinger · 2024-08-15T05:32:32Z

filament/backend/src/vulkan/VulkanDriver.cpp

+                assert_invariant(sidecar);
+                if (sidecar && sidecar->isTransientAttachment()) {
+                    rpkey.usesLazilyAllocatedMemory |= (1 << i);
+                    assert_invariant(sidecar->samples > 1);


move this assert just bellow the sidecar assert.

pixelflinger · 2024-08-15T05:35:19Z

filament/backend/src/vulkan/VulkanTexture.cpp

+        // Lazily allocated memory is available.
+        context.isLazilyAllocatedMemorySupported() &&
+        // Usage consists of attachment flags only.
+        !any(tusage & ~TextureUsage::ALL_ATTACHMENTS) &&


use none() instead of !any()

pixelflinger · 2024-08-15T05:41:09Z

filament/backend/src/vulkan/VulkanHandles.cpp

+                // Clear all usage flags that are not related to attachments, so that we can
+                // use the transient usage flag.
+                const TextureUsage usage = texture->usage & TextureUsage::ALL_ATTACHMENTS;


Is this correct? what if there was some other usage bits set? Shouldn't we instead passing the usage as is and let the VulkanTexture decide if it can use it?

As far as I understand, texture->usage is the usage of the resolved texture.
We cannot use that for MSAA, as the requirements are stricter for transient attachments.

In this particular case, the SAMPLEABLE flag is raised for the original texture, but it is not necessary nor desirable for the MSAA texture, and it prevents us from setting the attachment as transient. We cannot have any flags besides the four ***_ATTACHMENT_BIT for transient attachments.

Thinking about it... we could eventually remove the preferTransientAttachment parameter from the VulkanTexture. I don't think there is a case where you would not want to create a transient attachment if it is possible to do so.
What do you think?

As far as I understand, texture->usage is the usage of the resolved texture. We cannot use that for MSAA, as the requirements are stricter for transient attachments.

In this particular case, the SAMPLEABLE flag is raised for the original texture, but it is not necessary nor desirable for the MSAA texture, and it prevents us from setting the attachment as transient. We cannot have any flags besides the four ***_ATTACHMENT_BIT for transient attachments.

mh... I want to understand why the SAMPLEABLE flag is set then. Did the framegraph set it? if it did, it's probably that it needs it?

I don't know the frame graph well enough to confirm that. I am not sure if that helps, but by the time ResourceAllocator::createTexture is called, the SAMPLEABLE flag is already there.

However, I think it is not relevant for MSAA. Whether the resolved texture is sampled or not should not impact the MSAA texture usage in any way. But, we still need to use the right attachment flags, hence the mask with TextureUsage::ALL_ATTACHMENTS.

As discussed offline, I checked the way filament handles the MSAA resolve.
The VK_IMAGE_USAGE_TRANSFER_SRC_BIT flag is only needed when using vkCmdResolveImage. Filament uses the more efficient VkSubpassDescription::pResolveAttachments for the sidecar textures, so we are safe masking the flags unrelated to attachments.

pixelflinger · 2024-08-15T05:43:17Z

filament/backend/src/vulkan/VulkanTexture.cpp

+    const VkImageUsageFlags blittable =
+       useTransientAttachment ?
+       0U :
+       VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_TRANSFER_SRC_BIT;


We should probably revisit why blitable usages are systematically added, regardless of the BLIT_ flags -- we shouldn't have to do these shenanigans with blitable.

Totally agree. I think forcing everything to be blittable silently is not good, because of the potential use cases, like transient, where it gets in the way.
It is a broader change though, unrelated with transient support perse. Should we just add a separate todo?

yes, let's add a todo, and maybe you can have a look next? (or we will). you can even file a github issue.

I can definitely have a look. I think some usage flags that are silently added might also be getting in the way of protected content.

pixelflinger · 2024-08-28T20:00:09Z

@mdagois could you rebase the branch and resolve the conflicts? thanks!

mdagois · 2024-09-02T07:33:37Z

@mdagois could you rebase the branch and resolve the conflicts? thanks!

I rebased the branch.
I also simplified the code in the constructor (like removing the preferTransient boolean) since we don't have the blittable flags raised by default anymore.

Attachment tweaks Minor tweaks Set the load operation of the attachment based on whether it is transient or not Fixed a bug using the wrong texture Addressed some comments in the PR Remove the boolean to select transient

mdagois · 2024-10-03T03:54:13Z

@pixelflinger @poweifeng I rebased the branch and tested it again. It still work as intended. :)
Only minor changes were necessary (around the new texture state). Please check the PR again.

mdagois force-pushed the transient branch from ec41016 to b3a3ff3 Compare August 7, 2024 08:46

pixelflinger requested review from poweifeng and pixelflinger August 8, 2024 20:00

poweifeng reviewed Aug 12, 2024

View reviewed changes

mdagois force-pushed the transient branch from bb0d2d7 to 98b71b0 Compare August 14, 2024 04:20

poweifeng approved these changes Aug 14, 2024

View reviewed changes

poweifeng added the internal Issue/PR does not affect clients label Aug 14, 2024

pixelflinger requested changes Aug 15, 2024

View reviewed changes

mdagois requested a review from pixelflinger August 16, 2024 04:37

mdagois force-pushed the transient branch from 409943b to 576cb71 Compare August 21, 2024 06:13

mdagois mentioned this pull request Aug 22, 2024

Refactored the handling of usage flags in the VulkanTexture constructor #8074

Merged

mdagois force-pushed the transient branch from 576cb71 to 77cd78d Compare September 2, 2024 07:26

mdagois force-pushed the transient branch from 77cd78d to 13bbf9a Compare September 5, 2024 06:32

Transient attachment support

3f4fe03

Attachment tweaks Minor tweaks Set the load operation of the attachment based on whether it is transient or not Fixed a bug using the wrong texture Addressed some comments in the PR Remove the boolean to select transient

mdagois force-pushed the transient branch from 8345715 to 3f4fe03 Compare October 3, 2024 03:15

poweifeng approved these changes Oct 7, 2024

View reviewed changes

Merge branch 'main' into transient

19c5516

pixelflinger approved these changes Oct 8, 2024

View reviewed changes

Merge branch 'main' into transient

08a9330

poweifeng enabled auto-merge (squash) October 8, 2024 23:08

poweifeng disabled auto-merge October 8, 2024 23:08

poweifeng enabled auto-merge (squash) October 8, 2024 23:08

poweifeng merged commit 1b0ff3a into google:main Oct 8, 2024
9 checks passed

mdagois deleted the transient branch October 9, 2024 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for transient attachments #8021

Support for transient attachments #8021

mdagois commented Aug 7, 2024

mdagois commented Aug 8, 2024

poweifeng left a comment

mdagois commented Aug 13, 2024

mdagois commented Aug 14, 2024

mdagois commented Aug 14, 2024

poweifeng left a comment

pixelflinger Aug 15, 2024

mdagois Aug 16, 2024

pixelflinger Aug 15, 2024

mdagois Aug 16, 2024

pixelflinger Aug 15, 2024

mdagois Aug 16, 2024 •

edited

Loading

mdagois Aug 16, 2024

pixelflinger Aug 16, 2024

mdagois Aug 19, 2024

mdagois Aug 21, 2024

pixelflinger Aug 15, 2024

mdagois Aug 16, 2024 •

edited

Loading

pixelflinger Aug 16, 2024

mdagois Aug 19, 2024

pixelflinger commented Aug 28, 2024

mdagois commented Sep 2, 2024

mdagois commented Oct 3, 2024

Support for transient attachments #8021

Support for transient attachments #8021

Conversation

mdagois commented Aug 7, 2024

mdagois commented Aug 8, 2024

poweifeng left a comment

Choose a reason for hiding this comment

mdagois commented Aug 13, 2024

mdagois commented Aug 14, 2024

mdagois commented Aug 14, 2024

poweifeng left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdagois Aug 16, 2024 • edited Loading

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

Choose a reason for hiding this comment

mdagois Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixelflinger commented Aug 28, 2024

mdagois commented Sep 2, 2024

mdagois commented Oct 3, 2024

mdagois Aug 16, 2024 •

edited

Loading

mdagois Aug 16, 2024 •

edited

Loading