Soundness with new swapchain model on Vulkan #3184
Comments
This was referenced Mar 28, 2020
bors bot
added a commit
to gfx-rs/portability
that referenced
this issue
Aug 5, 2020
210: Port to the new swapchain model r=kvark a=kvark Closes #208 Fixes #212 Also has general fixes to: - push constant offsets in `gfxCreatePipelineLayout` - destination subresources in `gfxCmdResolveImage` The PR is inviting for discussion: @grovesNL @msiglreith I'm not 100% sure what to do yet. The Good: 1. Being able to remove the old swapchain from gfx-rs COMPLETELY. It was hacky on all the backends other than Vulkan, quite complicated on Metal, and didn't work very in general. 2. Fullscreen support! It's not ideal yet (occasional hitches), but it was pretty much non-functional before. In general, stuff that runs is more robust this change. The Bad: 1. Portability implementation is a bit more complex now, but not extraordinary so. In general, I think it's a win if we move some complication out of gfx-rs into gfx-portability, since gfx-rs has more users, it's more important to keep clean. 2. We can't support recording a render pass that uses a swapchain image if either: - the image is not acquired at the moment of recording - the command buffer is re-usable. We expect the users to strictly acquire-record-submit-present. The Ugly: 1. Our KHR swapchain implementation is more limited: no support for any usage other than COLOR_ATTACHMENT. This is technically valid in Vulkan, but for some reason many apps expect to transfer to/from swapchain images. So this is fine for the matter of CTS and VkPI, but can introduce some friction in real world testing. 2. The new swapchain model itself needs to be evolved a bit more, according to gfx-rs/gfx#3184 . We don't know how exactly: there is both a risk (that we'll need to redo something here) and an opportunity (that we'll support other usages, for example). Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
bors bot
added a commit
to gfx-rs/portability
that referenced
this issue
Aug 5, 2020
210: Port to the new swapchain model r=kvark a=kvark Closes #208 Fixes #212 Also has general fixes to: - push constant offsets in `gfxCreatePipelineLayout` - destination subresources in `gfxCmdResolveImage` The PR is inviting for discussion: @grovesNL @msiglreith I'm not 100% sure what to do yet. The Good: 1. Being able to remove the old swapchain from gfx-rs COMPLETELY. It was hacky on all the backends other than Vulkan, quite complicated on Metal, and didn't work very in general. 2. Fullscreen support! It's not ideal yet (occasional hitches), but it was pretty much non-functional before. In general, stuff that runs is more robust this change. The Bad: 1. Portability implementation is a bit more complex now, but not extraordinary so. In general, I think it's a win if we move some complication out of gfx-rs into gfx-portability, since gfx-rs has more users, it's more important to keep clean. 2. We can't support recording a render pass that uses a swapchain image if either: - the image is not acquired at the moment of recording - the command buffer is re-usable. We expect the users to strictly acquire-record-submit-present. The Ugly: 1. Our KHR swapchain implementation is more limited: no support for any usage other than COLOR_ATTACHMENT. This is technically valid in Vulkan, but for some reason many apps expect to transfer to/from swapchain images. So this is fine for the matter of CTS and VkPI, but can introduce some friction in real world testing. 2. The new swapchain model itself needs to be evolved a bit more, according to gfx-rs/gfx#3184 . We don't know how exactly: there is both a risk (that we'll need to redo something here) and an opportunity (that we'll support other usages, for example). Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
|
@kvark any updates on this? What's the blocker here; research, decision or effort? |
|
There is actually an update, it's in that LunarG issue. It was re-opened, so we are blocked on them either fixing this, or stating the resolution on whether what we do is sound or not. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Related to #3015
The latest update from LunarG says (KhronosGroup/Vulkan-ValidationLayers#12 (comment)) our approach is unsound:
The text was updated successfully, but these errors were encountered: