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

Add QCOM Multiview Per View Viewport extension support #3185

Closed

Conversation

mazvalente
Copy link

Add support for the extension VK_QCOM_multiview_per_view_viewports which allows for multiple viewports to be submitted in a single view. RenderDoc already supports this viewport/scissor configuration, and this change will allow applications which submit multiple viewports and scissors using the extension to render correctly.

Add support for the extension VK_QCOM_multiview_per_view_viewports which allows for multiple viewports to be submitted in a single view. RenderDoc already supports this viewport/scissor configuration, and this change will allow applications which submit multiple viewports and scissors using the extension to render correctly.
@mazvalente mazvalente force-pushed the Implement_multiviewperview_viewports branch from 123e3f4 to e747196 Compare December 26, 2023 22:47
@mazvalente mazvalente marked this pull request as ready for review January 3, 2024 18:18
Copy link
Owner

@baldurk baldurk left a comment

Choose a reason for hiding this comment

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

The extension itself seems fairly straightforward but this pull request seems kind of unfinished - was it marked as ready for review accidentally and it should still be a draft?

renderdoc/api/replay/vk_pipestate.h Outdated Show resolved Hide resolved
renderdoc/driver/vulkan/wrappers/vk_device_funcs.cpp Outdated Show resolved Hide resolved
renderdoc/driver/vulkan/vk_overlay.cpp Outdated Show resolved Hide resolved
renderdoc/driver/vulkan/vk_rendertexture.cpp Outdated Show resolved Hide resolved
@mazvalente mazvalente marked this pull request as draft January 10, 2024 23:28
Removed remnants of earlier code commits that should not have been in the final PR and deleted an unneeded change which added a struct to the DeviceCreateInfo struct if it wasn't already present.
@mazvalente mazvalente force-pushed the Implement_multiviewperview_viewports branch from 6a85e9b to 9cf74f9 Compare January 10, 2024 23:40
@mazvalente
Copy link
Author

mazvalente commented Jan 10, 2024

was it marked as ready for review accidentally and it should still be a draft?

It was marked as ready for review intentionally, but definitely should have received another pass before doing so, my apologies. I've cleaned up the suggested changes and will will reopen for review once I verify that everything is working properly.

@mazvalente mazvalente marked this pull request as ready for review January 16, 2024 18:58
Copy link
Owner

@baldurk baldurk left a comment

Choose a reason for hiding this comment

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

I'm a little unsure of things - the only changes remaining in this PR are those which purely allow and serialise it (aside from one change which forcibly enables it which I've left a comment on). In the previous version though you had modifications to the replay side which seemed designed to update handling for it.

You haven't explained why the difference - does this extension really need no modification whatsoever of the replay handling? If so, why were there changes before and how have you tested this to make sure that the replay handling is robust even when the extension is used?

renderdoc/driver/vulkan/wrappers/vk_device_funcs.cpp Outdated Show resolved Hide resolved
@mazvalente
Copy link
Author

You haven't explained why the difference - does this extension really need no modification whatsoever of the replay handling? If so, why were there changes before and how have you tested this to make sure that the replay handling is robust even when the extension is used?

You're right, this does require some clarification. All that multiviewperviewviewports does is give permission for Vulkan to use multiple viewports and scissors, which is already supported by the current VkCmdSetViewports/VkCmdSetScissors functions. From my testing with an app which submits multiple viewports and scissors it looks like there is no change needed on the replay side to enable the feature. Your mentioning that the feature should not be automatically enabled if the struct is attached suggests that I should add a check when submitting viewports to ensure that we have a form of multiviewport support enabled and otherwise either only accept the first submission or throw an error.

@baldurk
Copy link
Owner

baldurk commented Jan 24, 2024

Something sounds wrong here. Your description of what the extension does doesn't match up with its description in the spec or my understanding of it. Unextended vulkan absolutely does allow the use of multiple viewports and scissor regions and there is no extension needed for that, though there are extensions that expand its use like VK_EXT_shader_viewport_index_layer. This extension seems to change the behaviour of multiview passes to automatically use the multiview view index as a viewport index if no explicit viewport index is selected.

I don't understand what you're saying about only accepting 'the first submission' or throwing any kind of error. Can you explain exactly what you mean by that?

Changes code to no longer automatically enable the extension if the extension struct is present.
@mazvalente
Copy link
Author

You're right, I was misunderstanding the description. After testing the implementation by making shader changes to selectively enable multiview in shaders through VK_EXT_shader_viewport_index_layer, it appears that no replay changes are necessary. Without shader changes the multiview functionality follows the spec implementation of using the multiview index as viewport index, and if I set the viewportIndex in the shader I override that behavior instead following the VK_EXT_shader_viewport_index_layer logic.

I don't understand what you're saying about only accepting 'the first submission' or throwing any kind of error. Can you explain exactly what you mean by that?

I did not properly understand what I should have been doing to handle non-spec compliant situations, and was making bad assumptions, both on what the correct spec requirements were, and how to handle them. Please ignore it.

I've also uncovered one issue that I'll need to fix before putting this back in review, the debug overlay for Viewports/Scissors does not recognize the second viewport properly and is currently not displaying any overlay when selected for slice 1. I'll reopen this when I have a fix.

@mazvalente mazvalente marked this pull request as draft February 9, 2024 20:07
Modifies RenderOverlay to support multiple viewports and scissors and update the Viewport/Scissor overlay to render the correct scissor for the given view.
@mazvalente mazvalente force-pushed the Implement_multiviewperview_viewports branch from 98f90b9 to 5987480 Compare February 27, 2024 22:39
@mazvalente
Copy link
Author

This resolves the issue with vk_overlay not displaying the proper viewport/scissors region. If you know of a better way to retrieve the active slice value without modifying the RenderOverlay signature please advise me, but I don't believe it's retrievable without either changing the Overlay value to not be the enum or adding a new value in.

Add a dependency on multiviewperviewviewports being enabled to allow for the slice to determine the viewport/scissor allocation. This is not guaranteed to be the case if the feature is not enabled.
@mazvalente mazvalente marked this pull request as ready for review February 28, 2024 18:19
@baldurk
Copy link
Owner

baldurk commented Mar 5, 2024

I'm not meaning to be rude but I think there's still a misunderstanding here about how this extension works, and maybe how extensions work on Vulkan in general.

For that reason I'm closing this PR for now - could you please open this as a feature request issue for extension support and then it can be tracked that way?

@baldurk baldurk closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants