layers: Handle oldSwapchain image reuse

layers/state_tracker/state_tracker.cpp

@@ -4259,6 +4259,22 @@ void DeviceState::RecordCreateSwapchainState(VkResult result, const VkSwapchainC
                                                            swapchain_images[i], image_ci.format, image_ci.tiling);
                 auto image_state = CreateImageState(swapchain_images[i], image_ci.ptr(), swapchain->VkHandle(), i, format_features);
 
+                // Detect image resuse from the old swapchain
+                //
+                // NOTE: The device image map uses VkImage as a key, so only one state object
+                // can exist per VkImage handle. In case of handle reuse:
+                //  * The image state for the new swapchain replaces the old state in the device map
+                //  * The old swapchain's image is updated to point to the new state object
+                if (old_swapchain_state) {
+                    for (auto& old_swapchain_image : old_swapchain_state->images) {
+                        const vvl::Image* old_image_state = old_swapchain_image.image_state;
+                        if (old_image_state && old_image_state->VkHandle() == image_state->VkHandle()) {
+                            old_swapchain_image.image_state = image_state.get();
+                            break;
+                        }
+                    }
+                }
+
                 // Create a copy since image state is needed after move. SetSwapchain modifies image substates.
                 auto image_state_ptr_copy = image_state;
                 Add(std::move(image_state_ptr_copy));
@@ -4270,6 +4286,7 @@ void DeviceState::RecordCreateSwapchainState(VkResult result, const VkSwapchainC
         }
         if (old_swapchain_state) {
             old_swapchain_state->new_swapchain = swapchain;
+            swapchain->old_swapchain = old_swapchain_state->shared_from_this();
         }
         Add(std::move(swapchain));
     } else {

layers/state_tracker/wsi_state.cpp

@@ -214,8 +214,32 @@ void Swapchain::Destroy() {
     for (auto& swapchain_image : images) {
         swapchain_image.ResetPresentWaitSemaphores();
         RemoveParent(swapchain_image.image_state);
-        dev_data.Destroy<vvl::Image>(swapchain_image.image_state->VkHandle());
-        // NOTE: We don't have access to dev_data.fake_memory.Free() here, but it is currently a no-op
+
+        bool destroy_image_state = true;
+        {
+            // A. We are destroying old swapchain, but the image state is still used by the new swapchain
+            if (new_swapchain) {
+                for (const SwapchainImage& new_swapchain_image : new_swapchain->images) {
+                    if (new_swapchain_image.image_state == swapchain_image.image_state) {
+                        destroy_image_state = false;
+                        break;
+                    }
+                }
+            }
+            // B. We are destroying new swapchain, but the image state is still used by the old swapchain
+            // (old swapchain is allowed to outlive new swapchain)
+            if (auto p_old_swapchain = old_swapchain.lock()) {
+                for (const SwapchainImage& old_swapchain_image : p_old_swapchain->images) {
+                    if (old_swapchain_image.image_state == swapchain_image.image_state) {
+                        destroy_image_state = false;
+                        break;
+                    }
+                }
+            }
+        }
+        if (destroy_image_state) {
+            dev_data.Destroy<vvl::Image>(swapchain_image.image_state->VkHandle());
+        }
     }
     images.clear();
 

layers/state_tracker/wsi_state.h

@@ -64,7 +64,7 @@ class Swapchain;
 enum class AcquireSyncStatus { NotSpecified, Signaled, WasWaitedOn };
 
 struct SwapchainImage {
-    vvl::Image *image_state = nullptr;
+    vvl::Image* image_state = nullptr;
 
     // Acquire state
     bool acquired = false;
@@ -136,6 +136,7 @@ class Swapchain : public StateObject, public SubStateManager<SwapchainSubState>
     // New swapchain state:
     // Present wait semaphores from the the old swapchain presentations.
     std::vector<std::shared_ptr<vvl::Semaphore>> old_swapchain_present_wait_semaphores;
+    std::weak_ptr<Swapchain> old_swapchain;
 
     // Number of bits set in VkPresentTimingInfoEXT::presentStageQueries for each present that hasn't been completed
     struct PresentTimingInfo {

layers/thread_tracker/thread_safety_validation.cpp

@@ -475,9 +475,28 @@ void Device::PostCallRecordDestroySwapchainKHR(VkDevice device, VkSwapchainKHR s
     DestroyObject(swapchain);
     // Host access to swapchain must be externally synchronized
     auto lock = WriteLockGuard(thread_safety_lock);
-    for (auto& image_handle : swapchain_wrapped_image_handle_map[swapchain]) {
+    for (VkImage image_handle : swapchain_wrapped_image_handle_map[swapchain]) {
         FinishWriteObject(image_handle, record_obj.location);
-        DestroyObject(image_handle);
+
+        // Swapchain can resue images from oldSwapchain.
+        // When deleting either new swapchain or old swapchain we need to check
+        // if the image can still be in use by the surviving swapchain
+        bool swapchain_image_reuse = false;
+        for (auto& [current_swapchain, images] : swapchain_wrapped_image_handle_map) {
+            if (current_swapchain == swapchain) {
+                continue;
+            }
+            for (VkImage another_swapchain_image : images) {
+                if (another_swapchain_image == image_handle) {
+                    swapchain_image_reuse = true;
+                    break;
+                }
+            }
+        }
+
+        if (!swapchain_image_reuse) {
+            DestroyObject(image_handle);
+        }
     }
     swapchain_wrapped_image_handle_map.erase(swapchain);
 }

tests/unit/wsi_positive.cpp

@@ -2124,7 +2124,7 @@ TEST_F(PositiveWsi, CreateSwapchainImagesWithExclusiveSharingMode) {
     vkt::Image image(*m_device, image_create_info, vkt::no_mem);
 }
 
-TEST_F(PositiveWsi, CreateSwapchainWithOldSwapchain) {
+TEST_F(PositiveWsi, CreateWithOldSwapchain) {
     AddSurfaceExtension();
     RETURN_IF_SKIP(Init());
     RETURN_IF_SKIP(InitSurface());
@@ -2150,6 +2150,171 @@ TEST_F(PositiveWsi, CreateSwapchainWithOldSwapchain) {
     vkt::Swapchain swapchain2(*m_device, swapchain_ci);
 }
 
+TEST_F(PositiveWsi, CreateWithOldSwapchainUniqueHandles) {
+    // https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/11939
+    // Test is non-deterministic, in case of regression it might take multiple runs to detect the issue
+    // (also depends on whether windowing system reuses images). Tested on nvidia hardware.
+    TEST_DESCRIPTION("Test reuse of swapchain images from oldSwapchain with unique handles enabled");
+    VkLayerSettingEXT layer_settings = {OBJECT_LAYER_NAME, "unique_handles", VK_LAYER_SETTING_TYPE_BOOL32_EXT, 1, &kVkFalse};
+    VkLayerSettingsCreateInfoEXT layer_settings_ci = vku::InitStructHelper();
+    layer_settings_ci.settingCount = 1;
+    layer_settings_ci.pSettings = &layer_settings;
+
+    VkValidationFeaturesEXT validation_features = vku::InitStructHelper();
+    validation_features.pNext = &layer_settings_ci;
+
+    AddSurfaceExtension();
+    RETURN_IF_SKIP(Init(nullptr, nullptr, &validation_features));
+    RETURN_IF_SKIP(InitSurface());
+    InitSwapchainInfo();
+
+    VkSurfaceCapabilitiesKHR surface_caps;
+    vk::GetPhysicalDeviceSurfaceCapabilitiesKHR(Gpu(), m_surface, &surface_caps);
+
+    VkSwapchainCreateInfoKHR swapchain_ci = vku::InitStructHelper();
+    swapchain_ci.surface = m_surface;
+    swapchain_ci.minImageCount = surface_caps.minImageCount;
+    swapchain_ci.imageFormat = m_surface_formats[0].format;
+    swapchain_ci.imageColorSpace = m_surface_formats[0].colorSpace;
+    swapchain_ci.imageExtent = surface_caps.minImageExtent;
+    swapchain_ci.imageArrayLayers = 1;
+    swapchain_ci.imageUsage = VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT;
+    swapchain_ci.preTransform = VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR;
+    swapchain_ci.compositeAlpha = m_surface_composite_alpha;
+    swapchain_ci.presentMode = m_surface_non_shared_present_mode;
+    vkt::Swapchain swapchain(*m_device, swapchain_ci);
+
+    swapchain_ci.oldSwapchain = swapchain;
+    vkt::Swapchain swapchain2(*m_device, swapchain_ci);
+
+    swapchain.Destroy();
+    swapchain2.Destroy();
+}
+
+TEST_F(PositiveWsi, CreateWithOldSwapchainUniqueHandles2) {
+    TEST_DESCRIPTION("Test reuse of swapchain images from oldSwapchain with unique handles enabled");
+    VkLayerSettingEXT layer_settings = {OBJECT_LAYER_NAME, "unique_handles", VK_LAYER_SETTING_TYPE_BOOL32_EXT, 1, &kVkFalse};
+    VkLayerSettingsCreateInfoEXT layer_settings_ci = vku::InitStructHelper();
+    layer_settings_ci.settingCount = 1;
+    layer_settings_ci.pSettings = &layer_settings;
+
+    VkValidationFeaturesEXT validation_features = vku::InitStructHelper();
+    validation_features.pNext = &layer_settings_ci;
+
+    AddSurfaceExtension();
+    RETURN_IF_SKIP(Init(nullptr, nullptr, &validation_features));
+    RETURN_IF_SKIP(InitSurface());
+    InitSwapchainInfo();
+
+    VkSurfaceCapabilitiesKHR surface_caps;
+    vk::GetPhysicalDeviceSurfaceCapabilitiesKHR(Gpu(), m_surface, &surface_caps);
+
+    VkSwapchainCreateInfoKHR swapchain_ci = vku::InitStructHelper();
+    swapchain_ci.surface = m_surface;
+    swapchain_ci.minImageCount = surface_caps.minImageCount;
+    swapchain_ci.imageFormat = m_surface_formats[0].format;
+    swapchain_ci.imageColorSpace = m_surface_formats[0].colorSpace;
+    swapchain_ci.imageExtent = surface_caps.minImageExtent;
+    swapchain_ci.imageArrayLayers = 1;
+    swapchain_ci.imageUsage = VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT;
+    swapchain_ci.preTransform = VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR;
+    swapchain_ci.compositeAlpha = m_surface_composite_alpha;
+    swapchain_ci.presentMode = m_surface_non_shared_present_mode;
+    vkt::Swapchain swapchain(*m_device, swapchain_ci);
+
+    swapchain_ci.oldSwapchain = swapchain;
+    vkt::Swapchain swapchain2(*m_device, swapchain_ci);
+
+    // Reverse destruction order comparing to CreateWithOldSwapchainUniqueHandles
+    swapchain2.Destroy();
+    swapchain.Destroy();
+}
+
+TEST_F(PositiveWsi, CreateWithOldSwapchainUniqueHandlesAndGetImages) {
+    TEST_DESCRIPTION("Test reuse of swapchain images from oldSwapchain with unique handles enabled");
+    VkLayerSettingEXT layer_settings = {OBJECT_LAYER_NAME, "unique_handles", VK_LAYER_SETTING_TYPE_BOOL32_EXT, 1, &kVkFalse};
+    VkLayerSettingsCreateInfoEXT layer_settings_ci = vku::InitStructHelper();
+    layer_settings_ci.settingCount = 1;
+    layer_settings_ci.pSettings = &layer_settings;
+
+    VkValidationFeaturesEXT validation_features = vku::InitStructHelper();
+    validation_features.pNext = &layer_settings_ci;
+
+    AddSurfaceExtension();
+    RETURN_IF_SKIP(Init(nullptr, nullptr, &validation_features));
+    RETURN_IF_SKIP(InitSurface());
+    InitSwapchainInfo();
+
+    VkSurfaceCapabilitiesKHR surface_caps;
+    vk::GetPhysicalDeviceSurfaceCapabilitiesKHR(Gpu(), m_surface, &surface_caps);
+
+    VkSwapchainCreateInfoKHR swapchain_ci = vku::InitStructHelper();
+    swapchain_ci.surface = m_surface;
+    swapchain_ci.minImageCount = surface_caps.minImageCount;
+    swapchain_ci.imageFormat = m_surface_formats[0].format;
+    swapchain_ci.imageColorSpace = m_surface_formats[0].colorSpace;
+    swapchain_ci.imageExtent = surface_caps.minImageExtent;
+    swapchain_ci.imageArrayLayers = 1;
+    swapchain_ci.imageUsage = VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT;
+    swapchain_ci.preTransform = VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR;
+    swapchain_ci.compositeAlpha = m_surface_composite_alpha;
+    swapchain_ci.presentMode = m_surface_non_shared_present_mode;
+    vkt::Swapchain swapchain(*m_device, swapchain_ci);
+
+    // Get images to trigger swapchain image tracking functionality in various validation objects
+    auto swapchain_images = swapchain.GetImages();
+
+    swapchain_ci.oldSwapchain = swapchain;
+    vkt::Swapchain swapchain2(*m_device, swapchain_ci);
+    auto swapchain2_images = swapchain2.GetImages();
+
+    swapchain.Destroy();
+    swapchain2.Destroy();
+}
+
+TEST_F(PositiveWsi, CreateWithOldSwapchainUniqueHandlesAndGetImages2) {
+    TEST_DESCRIPTION("Test reuse of swapchain images from oldSwapchain with unique handles enabled");
+    VkLayerSettingEXT layer_settings = {OBJECT_LAYER_NAME, "unique_handles", VK_LAYER_SETTING_TYPE_BOOL32_EXT, 1, &kVkFalse};
+    VkLayerSettingsCreateInfoEXT layer_settings_ci = vku::InitStructHelper();
+    layer_settings_ci.settingCount = 1;
+    layer_settings_ci.pSettings = &layer_settings;
+
+    VkValidationFeaturesEXT validation_features = vku::InitStructHelper();
+    validation_features.pNext = &layer_settings_ci;
+
+    AddSurfaceExtension();
+    RETURN_IF_SKIP(Init(nullptr, nullptr, &validation_features));
+    RETURN_IF_SKIP(InitSurface());
+    InitSwapchainInfo();
+
+    VkSurfaceCapabilitiesKHR surface_caps;
+    vk::GetPhysicalDeviceSurfaceCapabilitiesKHR(Gpu(), m_surface, &surface_caps);
+
+    VkSwapchainCreateInfoKHR swapchain_ci = vku::InitStructHelper();
+    swapchain_ci.surface = m_surface;
+    swapchain_ci.minImageCount = surface_caps.minImageCount;
+    swapchain_ci.imageFormat = m_surface_formats[0].format;
+    swapchain_ci.imageColorSpace = m_surface_formats[0].colorSpace;
+    swapchain_ci.imageExtent = surface_caps.minImageExtent;
+    swapchain_ci.imageArrayLayers = 1;
+    swapchain_ci.imageUsage = VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT;
+    swapchain_ci.preTransform = VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR;
+    swapchain_ci.compositeAlpha = m_surface_composite_alpha;
+    swapchain_ci.presentMode = m_surface_non_shared_present_mode;
+    vkt::Swapchain swapchain(*m_device, swapchain_ci);
+
+    // Get images to trigger swapchain image tracking functionality in various validation objects
+    auto swapchain_images = swapchain.GetImages();
+
+    swapchain_ci.oldSwapchain = swapchain;
+    vkt::Swapchain swapchain2(*m_device, swapchain_ci);
+    auto swapchain2_images = swapchain2.GetImages();
+
+    // Reverse destruction order comparing to CreateWithOldSwapchainUniqueHandlesAndGetImages
+    swapchain2.Destroy();
+    swapchain.Destroy();
+}
+
 TEST_F(PositiveWsi, OldSwapchainFromAnotherSurface) {
     TEST_DESCRIPTION("https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/10112");
     AddSurfaceExtension();