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

Modify virtual swapchain behavior #1222

Merged
merged 10 commits into from
Aug 29, 2023

Conversation

MarkY-LunarG
Copy link
Contributor

Modify virtual swapchain to create a command buffer per queue and when we get a vkQueuePresent, perform the blits in the same queue. This resolves the issue where a QueuePresent is presented inline in a queue, but has no wait semaphore information. Instead, the QueuePresent relied upon the previous submit (also in the same queue) to perform the synchronization.

Previously, the virtual swapchain had it's own Queue that it used which caused a race condition resulting in old images being used.

Also, instead of doing just a waitforidle, wait for all pending semaphores (which could include some that are pending but not submitted).
Also, don't wait for idle after the screenshot submit, wait for the fence. This way we don't have to keep idling the GPU.

Fixes: #1122

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 20859.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3069 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3069 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 21383.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3072 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3072 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 21423.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3073 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3073 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 21511.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3074 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3074 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 21990.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3075 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 22027.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3076 running.

@MarkY-LunarG MarkY-LunarG marked this pull request as draft August 11, 2023 16:51
@MarkY-LunarG
Copy link
Contributor Author

Moved to draft because I can't figure out why it's failing on internal CI systems yet. I'll move it back when that runs clean.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3076 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 22116.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3077 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 22136.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3078 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3078 failed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3121 passed.

framework/decode/vulkan_captured_swapchain.cpp Outdated Show resolved Hide resolved
framework/decode/vulkan_replay_consumer_base.cpp Outdated Show resolved Hide resolved
framework/decode/vulkan_replay_consumer_base.cpp Outdated Show resolved Hide resolved
framework/decode/vulkan_virtual_swapchain.cpp Outdated Show resolved Hide resolved
framework/decode/vulkan_virtual_swapchain.h Outdated Show resolved Hide resolved
framework/decode/vulkan_virtual_swapchain.cpp Outdated Show resolved Hide resolved
framework/decode/vulkan_virtual_swapchain.h Outdated Show resolved Hide resolved
framework/decode/vulkan_virtual_swapchain.cpp Outdated Show resolved Hide resolved
@MarkY-LunarG MarkY-LunarG force-pushed the marky_fix_screenshot_wait branch from a3ea9b3 to 1a28b3a Compare August 25, 2023 20:30
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 30775.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3138 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3138 passed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 30883.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3139 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3139 passed.

@locke-lunarg locke-lunarg self-requested a review August 28, 2023 18:18
Copy link
Contributor

@locke-lunarg locke-lunarg left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you, Mark.

@MarkY-LunarG
Copy link
Contributor Author

@bradgrantham-lunarg , does everything look good to you? Locke approved this but I don't want to push it prematurely.

Copy link
Contributor

@bradgrantham-lunarg bradgrantham-lunarg left a comment

Choose a reason for hiding this comment

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

I requested three last changes, then I think this is ready to go. Since I didn't mention these the last review, and repeatedly requesting changes probably gets annoying, I'd be happy to make them myself to this PR. Would that work for you?

struct SwapchainData
{
// Create a map that correlates copy command data with a queue family index.
std::map<size_t, CopyCmdData> copy_cmd_data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since vkGetPhysicalDeviceQueueFamilyProperties takes and modifies a uint32_t, can this be a std::map on a uint32_t? For that matter, since it's just on an integer key, can this be a std::unordered_map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std::map is faster. especially if the items are ordered, which they are. That's why I used map.

Copy link
Contributor

Choose a reason for hiding this comment

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

unordered_map uses hashes and is O(1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion, Brad showed me that std::map is slower than std::unordered_map on average. Switched.

framework/decode/vulkan_virtual_swapchain.cpp Outdated Show resolved Hide resolved
framework/decode/vulkan_virtual_swapchain.h Outdated Show resolved Hide resolved
Modify virtual swapchain to create a command buffer per queue and
and per swapchain image.  Then when a vkQueuePresent occurs,
perform the blits in the same queue as the presented queue to
reduce race conditions.
This resolves the issue where a QueuePresent is presented inline
in a queue, but has no wait semaphore information.  Instead,
the QueuePresent relied upon the previous submit (also in the same
queue) to perform the synchronization.

Previously, the virtual swapchain had it's own Queue that it used
which caused a race condition resulting in old images being used.

Fixes: LunarG#1122
Move data that is only ever used by the VulkanVirtualSwapchain
class into private members of that class.  Everything is now
accessed via a pointer to the PrivateData struct which is associated
with the VkSwapchainKHR handle in an unordered_map.
Thanks to @locke-lunarg for discovering validation message warnings
introduced by my last set of changes.  This fixes all the new
validation error messages introduced at least in the VkCube example.
Fixed an error that popped up on Windows Nvidia 32-bit
@MarkY-LunarG MarkY-LunarG force-pushed the marky_fix_screenshot_wait branch from be88125 to ceb3baf Compare August 29, 2023 15:50
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 32867.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 3146 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 32900.

@MarkY-LunarG MarkY-LunarG merged commit e2d16b3 into LunarG:dev Aug 29, 2023
@MarkY-LunarG MarkY-LunarG deleted the marky_fix_screenshot_wait branch August 29, 2023 17:01
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.

Virtual swapchain causes screenshots mismatch for recaptured files.
4 participants