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

Draft: Fix VUID-VkSwapchainCreateInfoKHR-presentMode-02839 #8321

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

r-potter
Copy link
Contributor

Including a VkSurfacePresentModeEXT in the call to GetSurfaceCapabilities makes this check more strict than it should be.

This will require WG discussion. Internal Khronos spec issue to follow

This PR: #6351 was introduced to handle a case where chaining a VkSurfacePresentModeEXT lowered the minImageCount. However, we have also observed cases were it instead increases it, which causes validation errors in the opposite direction.

This PR makes validation layers actually check what VUID-VkSwapchainCreateInfoKHR-presentMode-02839 says the rule is (i.e. the value returned by the unextended function). Unfortunately this will go back to throwing errors in the case that triggered #6351. The spec has this note describing this functionality, but no VUs corresponding to it, and so its unclear both how to validate this, and how to correctly use the functionality:

If VkSwapchainPresentModesCreateInfoEXT is provided to swapchain creation, the requirements for forward progress may be less strict. For example, a FIFO swapchain might only require 2 images to guarantee forward progress, but a MAILBOX one might require 4. Without the per-present image counts, such an implementation would have to return 4 in VkSurfaceCapabilitiesKHR::minImageCount, which pessimizes FIFO. Conversely, an implementation may return a low number for minImageCount, but internally bump the image count when application queries vkGetSwapchainImagesKHR, which can surprise applications, and is not discoverable until swapchain creation. Using VkSurfacePresentModeEXT and VkSwapchainPresentModesCreateInfoEXT together effectively removes this problem.
VkSwapchainPresentModesCreateInfoEXT is required for the specification to be backwards compatible with applications that do not know about, or make use of this feature.

Including a VkSurfacePresentModeEXT in the call to
GetSurfaceCapabilities makes this check more strict than it should be.
@r-potter r-potter requested a review from a team as a code owner July 25, 2024 16:02
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 223790.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 17118 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 17118 passed.

artem-lunarg added a commit to artem-lunarg/Vulkan-ValidationLayers that referenced this pull request Nov 1, 2024
Why better:
- Simpler then the previous fix (no need to sync between two groups)
- Potential fixes most blacklisted Android WSI tests.
- Potentially solves a problem raised by Ralph or at least handles
  additional cases that are not handled correctly now (failures of
  our Android WSI is likely one of the manifestation of the problem):
  KhronosGroup#8321
artem-lunarg added a commit to artem-lunarg/Vulkan-ValidationLayers that referenced this pull request Nov 1, 2024
Why better:
- Simpler then the previous fix (no need to sync between two groups)
- Fixes some blacklisted Android WSI tests.
- Potentially solves a problem raised by Ralph or at least handles
  additional cases that are not handled correctly now (failures of
  our Android WSI is likely one of the manifestation of the problem):
  KhronosGroup#8321
artem-lunarg added a commit that referenced this pull request Nov 1, 2024
Why better:
- Simpler then the previous fix (no need to sync between two groups)
- Fixes some blacklisted Android WSI tests.
- Potentially solves a problem raised by Ralph or at least handles
  additional cases that are not handled correctly now (failures of
  our Android WSI is likely one of the manifestation of the problem):
  #8321
@artem-lunarg
Copy link
Contributor

@r-potter #8794 PR handles both the original use case from #6351 and also the situation you mentioned above (we had similar errors on our Android CI).

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.

3 participants