Skip to content
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

Multiview support for vulkan #7892

Merged
merged 14 commits into from
Jul 26, 2024
Merged

Multiview support for vulkan #7892

merged 14 commits into from
Jul 26, 2024

Conversation

GrantComm
Copy link
Contributor

@GrantComm GrantComm commented May 30, 2024

FIXES=[332425392]

Adds multiview support for vulkan. This is done by adding a layerCount to the renderTarget, which is used to determine if multiview is available and being used in the current renderpass.

@GrantComm GrantComm marked this pull request as draft May 30, 2024 21:30
@pixelflinger pixelflinger added the internal Issue/PR does not affect clients label May 30, 2024
build.sh Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanDriver.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanDriver.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanContext.h Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanDriver.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/platform/VulkanPlatform.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/platform/VulkanPlatform.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanTexture.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanTexture.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanFboCache.cpp Outdated Show resolved Hide resolved
@@ -73,7 +73,7 @@ VkImageSubresourceRange VulkanAttachment::getSubresourceRange() const {
.baseMipLevel = uint32_t(level),
.levelCount = 1,
.baseArrayLayer = uint32_t(layer),
.layerCount = 1,
.layerCount = uint32_t(viewCount) > 0 ? uint32_t(viewCount) : texture->getLayerCount() > 1 ? texture->getLayerCount() : 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a neat pick but I don't think the first cast is necessary viewCount > 0 ?. I also think that it would be cleaner and more readable if we just moved the whole logic for layerCount comewhere else and then here just do .layerCount = layerCount

filament/backend/src/vulkan/VulkanDriver.cpp Outdated Show resolved Hide resolved
@GrantComm GrantComm force-pushed the multiview branch 2 times, most recently from 5745c4e to 2a899a3 Compare June 14, 2024 21:33
filament/backend/src/vulkan/VulkanFboCache.cpp Outdated Show resolved Hide resolved
if (info.texture) {
rpkey.viewCount = info.texture->getLayerCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary, you already set the viewCount above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed because there are times when the renderTarget's layer count is 2. But the underlying vulkan texture's layer count is 1. If there is a vulkan texture, then the viewcount needs to be set to the value from the texture. Without it validation layers errors occur:

VULKAN ERROR: (Validation) Validation Error: [ VUID-VkFramebufferCreateInfo-renderPass-04536 ] | MessageID = 0x5e2eb360 | vkCreateFramebuffer(): pCreateInfo->pAttachments[1] has a layer count (1) less than or equal to the highest bit in the view mask (1) of subpass 0. The Vulkan spec states: If renderPass was specified with non-zero view masks, each element of pAttachments that is used as an input, color, resolve, or depth/stencil attachment by renderPass must have a layerCount greater than the index of the most significant bit set in any of those view masks (https://vulkan.lunarg.com/doc/view/1.3.268.0/linux/1.3-extensions/vkspec.html#VUID-VkFramebufferCreateInfo-renderPass-04536)

and a segmentation fault crashes the program.

One caveat: If there were ever a case when there is a texture of samplerType::SAMPLER_2D_ARRAY that is not being used in a multiview configuration, it would trigger the multiview structure being added to the renderpass. To get around that. I could pass the value of mContext.isMultiviewEnabled() as part of the RenderPassKey to add an additional check before chaining multiviewCreateInfo to the renderpass.

Copy link
Contributor

Choose a reason for hiding this comment

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

the texture should not have a layer count. But, for now, I propose both RenderTarget and Attachment will both have layerCounts. You can do an assert here to verify that the render target and attachment counts are the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not vulkan texture, but underlying vulkan image in the texture. When the gtlf_viewer application is not in stereo mode, the layercount of the underlying vulkan image is 1 but the layercount for the vulkan attachment and vulkan rendertarget are both 2. All three (vulkan texture's vkimage, vulkan attachment, and vulkan rendertarget) are all 2, only when the application is in stereo mode.

Without the current use of the layercount from the vulkan texture's vkImage, the validation error listed above is raised and a crash occurs

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there might be a bug here. If the rendertarget has 2 layers, then the backing image of the attachments should also have at least 2 layers. If you see the rendertarget somehow has greater number of layers than any of the attachments, then it's a bug IIUC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, bug fix is in this pr: #7987

filament/backend/src/vulkan/VulkanFboCache.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanHandles.h Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanHandles.h Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanTexture.cpp Show resolved Hide resolved
filament/backend/src/vulkan/VulkanContext.h Show resolved Hide resolved
@poweifeng
Copy link
Contributor

FIXES=[332425392]

Please add more detail to the "commit message". You can modify your commit message with git commit --amend

Copy link
Contributor

@poweifeng poweifeng left a comment

Choose a reason for hiding this comment

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

this looks really good and really close. Just some leftover nits.

filament/backend/src/vulkan/VulkanDriver.cpp Show resolved Hide resolved
filament/backend/src/vulkan/VulkanDriver.cpp Show resolved Hide resolved
filament/backend/src/vulkan/VulkanDriver.cpp Show resolved Hide resolved
filament/backend/src/vulkan/VulkanFboCache.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanFboCache.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanTexture.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanFboCache.cpp Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanFboCache.cpp Outdated Show resolved Hide resolved
@GrantComm GrantComm requested a review from poweifeng July 24, 2024 04:26
@GrantComm GrantComm marked this pull request as ready for review July 24, 2024 04:32
Copy link
Contributor

@poweifeng poweifeng left a comment

Choose a reason for hiding this comment

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

LGTM except for two minor things.

Can you also make sure @z3moon signs off on the changes to
filament/src/RendererUtils.cpp
filament/src/fg/FrameGraphRenderPass.h

?

Thank you.

filament/backend/src/vulkan/VulkanHandles.h Outdated Show resolved Hide resolved
filament/backend/src/vulkan/VulkanDriver.cpp Outdated Show resolved Hide resolved
@poweifeng poweifeng enabled auto-merge (squash) July 26, 2024 07:17
@poweifeng poweifeng merged commit b54a204 into google:main Jul 26, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Issue/PR does not affect clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants