diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index 84210db9b9..e5454f8f68 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -1017,21 +1017,24 @@ namespace dawn_native { DAWN_TRY(ValidateSwapChainDescriptor(this, surface, descriptor)); } + // TODO(dawn:269): Remove this code path once implementation-based swapchains are removed. if (surface == nullptr) { DAWN_TRY_ASSIGN(*result, CreateSwapChainImpl(descriptor)); } else { ASSERT(descriptor->implementation == 0); NewSwapChainBase* previousSwapChain = surface->GetAttachedSwapChain(); - NewSwapChainBase* newSwapChain; - DAWN_TRY_ASSIGN(newSwapChain, - CreateSwapChainImpl(surface, previousSwapChain, descriptor)); + ResultOrError maybeNewSwapChain = + CreateSwapChainImpl(surface, previousSwapChain, descriptor); if (previousSwapChain != nullptr) { - ASSERT(!previousSwapChain->IsAttached()); + previousSwapChain->DetachFromSurface(); } - ASSERT(newSwapChain->IsAttached()); + NewSwapChainBase* newSwapChain = nullptr; + DAWN_TRY_ASSIGN(newSwapChain, std::move(maybeNewSwapChain)); + + newSwapChain->SetIsAttached(); surface->SetAttachedSwapChain(newSwapChain); *result = newSwapChain; } diff --git a/src/dawn_native/Surface.cpp b/src/dawn_native/Surface.cpp index ccd240c9df..4afe05ed45 100644 --- a/src/dawn_native/Surface.cpp +++ b/src/dawn_native/Surface.cpp @@ -151,8 +151,8 @@ namespace dawn_native { } } - NewSwapChainBase* Surface::GetAttachedSwapChain() const { - return mSwapChain; + NewSwapChainBase* Surface::GetAttachedSwapChain() { + return mSwapChain.Get(); } void Surface::SetAttachedSwapChain(NewSwapChainBase* swapChain) { diff --git a/src/dawn_native/Surface.h b/src/dawn_native/Surface.h index 048298b00b..5863109024 100644 --- a/src/dawn_native/Surface.h +++ b/src/dawn_native/Surface.h @@ -36,7 +36,7 @@ namespace dawn_native { Surface(InstanceBase* instance, const SurfaceDescriptor* descriptor); void SetAttachedSwapChain(NewSwapChainBase* swapChain); - NewSwapChainBase* GetAttachedSwapChain() const; + NewSwapChainBase* GetAttachedSwapChain(); // These are valid to call on all Surfaces. enum class Type { MetalLayer, WindowsHWND, Xlib }; @@ -61,7 +61,7 @@ namespace dawn_native { Type mType; // The swapchain will set this to null when it is destroyed. - NewSwapChainBase* mSwapChain = nullptr; + Ref mSwapChain; // MetalLayer void* mMetalLayer = nullptr; diff --git a/src/dawn_native/SwapChain.cpp b/src/dawn_native/SwapChain.cpp index 6358567f4f..965b4cbf47 100644 --- a/src/dawn_native/SwapChain.cpp +++ b/src/dawn_native/SwapChain.cpp @@ -261,7 +261,7 @@ namespace dawn_native { Surface* surface, const SwapChainDescriptor* descriptor) : SwapChainBase(device), - mAttached(true), + mAttached(false), mWidth(descriptor->width), mHeight(descriptor->height), mFormat(descriptor->format), @@ -277,18 +277,20 @@ namespace dawn_native { } ASSERT(!mAttached); - ASSERT(mSurface == nullptr); } void NewSwapChainBase::DetachFromSurface() { if (mAttached) { DetachFromSurfaceImpl(); - GetSurface()->SetAttachedSwapChain(nullptr); mSurface = nullptr; mAttached = false; } } + void NewSwapChainBase::SetIsAttached() { + mAttached = true; + } + void NewSwapChainBase::Configure(wgpu::TextureFormat format, wgpu::TextureUsage allowedUsage, uint32_t width, diff --git a/src/dawn_native/SwapChain.h b/src/dawn_native/SwapChain.h index 6e32e53404..efa8a5411f 100644 --- a/src/dawn_native/SwapChain.h +++ b/src/dawn_native/SwapChain.h @@ -94,13 +94,15 @@ namespace dawn_native { Surface* surface, const SwapChainDescriptor* descriptor); - // This is called when the swapchain is detached for any reason: + // This is called when the swapchain is detached when one of the following happens: // - // - The swapchain is being destroyed. // - The surface it is attached to is being destroyed. // - The swapchain is being replaced by another one on the surface. // - // The call for the old swapchain being replaced should be called inside the backend + // Note that the surface has a Ref on the last swapchain that was used on it so the + // SwapChain destructor will only be called after one of the things above happens. + // + // The call for the detaching previous swapchain should be called inside the backend // implementation of SwapChains. This is to allow them to acquire any resources before // calling detach to make a seamless transition from the previous swapchain. // @@ -109,6 +111,8 @@ namespace dawn_native { // destructor. void DetachFromSurface(); + void SetIsAttached(); + // Dawn API void Configure(wgpu::TextureFormat format, wgpu::TextureUsage allowedUsage, diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm index d449ab50c5..a9a1d1eaf5 100644 --- a/src/dawn_native/metal/DeviceMTL.mm +++ b/src/dawn_native/metal/DeviceMTL.mm @@ -163,7 +163,7 @@ namespace dawn_native { namespace metal { Surface* surface, NewSwapChainBase* previousSwapChain, const SwapChainDescriptor* descriptor) { - return new SwapChain(this, surface, previousSwapChain, descriptor); + return SwapChain::Create(this, surface, previousSwapChain, descriptor); } ResultOrError> Device::CreateTextureImpl(const TextureDescriptor* descriptor) { return AcquireRef(new Texture(this, descriptor)); diff --git a/src/dawn_native/metal/SwapChainMTL.h b/src/dawn_native/metal/SwapChainMTL.h index 19abc7facd..6f15c4b3cd 100644 --- a/src/dawn_native/metal/SwapChainMTL.h +++ b/src/dawn_native/metal/SwapChainMTL.h @@ -37,13 +37,15 @@ namespace dawn_native { namespace metal { class SwapChain final : public NewSwapChainBase { public: - SwapChain(Device* device, - Surface* surface, - NewSwapChainBase* previousSwapChain, - const SwapChainDescriptor* descriptor); + static ResultOrError Create(Device* device, + Surface* surface, + NewSwapChainBase* previousSwapChain, + const SwapChainDescriptor* descriptor); + ~SwapChain() override; private: - ~SwapChain() override; + using NewSwapChainBase::NewSwapChainBase; + MaybeError Initialize(NewSwapChainBase* previousSwapChain); CAMetalLayer* mLayer = nullptr; diff --git a/src/dawn_native/metal/SwapChainMTL.mm b/src/dawn_native/metal/SwapChainMTL.mm index f581da9835..7b0cbe26f7 100644 --- a/src/dawn_native/metal/SwapChainMTL.mm +++ b/src/dawn_native/metal/SwapChainMTL.mm @@ -57,22 +57,36 @@ namespace dawn_native { namespace metal { // SwapChain - SwapChain::SwapChain(Device* device, - Surface* surface, - NewSwapChainBase* previousSwapChain, - const SwapChainDescriptor* descriptor) - : NewSwapChainBase(device, surface, descriptor) { - ASSERT(surface->GetType() == Surface::Type::MetalLayer); + // static + ResultOrError SwapChain::Create(Device* device, + Surface* surface, + NewSwapChainBase* previousSwapChain, + const SwapChainDescriptor* descriptor) { + std::unique_ptr swapchain = + std::make_unique(device, surface, descriptor); + DAWN_TRY(swapchain->Initialize(previousSwapChain)); + return swapchain.release(); + } + + SwapChain::~SwapChain() { + DetachFromSurface(); + } + + MaybeError SwapChain::Initialize(NewSwapChainBase* previousSwapChain) { + ASSERT(GetSurface()->GetType() == Surface::Type::MetalLayer); if (previousSwapChain != nullptr) { // TODO(cwallez@chromium.org): figure out what should happen when surfaces are used by // multiple backends one after the other. It probably needs to block until the backend // and GPU are completely finished with the previous swapchain. - ASSERT(previousSwapChain->GetBackendType() == wgpu::BackendType::Metal); + if (previousSwapChain->GetBackendType() != wgpu::BackendType::Metal) { + return DAWN_VALIDATION_ERROR("metal::SwapChain cannot switch between APIs"); + } + previousSwapChain->DetachFromSurface(); } - mLayer = static_cast(surface->GetMetalLayer()); + mLayer = static_cast(GetSurface()->GetMetalLayer()); ASSERT(mLayer != nullptr); CGSize size = {}; @@ -91,10 +105,8 @@ namespace dawn_native { namespace metal { #endif // defined(DAWN_PLATFORM_MACOS) // There is no way to control Fifo vs. Mailbox in Metal. - } - SwapChain::~SwapChain() { - DetachFromSurface(); + return {}; } MaybeError SwapChain::PresentImpl() { diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index fc8d869461..d8ddfffa36 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -140,7 +140,7 @@ namespace dawn_native { namespace null { Surface* surface, NewSwapChainBase* previousSwapChain, const SwapChainDescriptor* descriptor) { - return new SwapChain(this, surface, previousSwapChain, descriptor); + return SwapChain::Create(this, surface, previousSwapChain, descriptor); } ResultOrError> Device::CreateTextureImpl(const TextureDescriptor* descriptor) { return AcquireRef(new Texture(this, descriptor, TextureBase::TextureState::OwnedInternal)); @@ -347,23 +347,31 @@ namespace dawn_native { namespace null { // SwapChain - SwapChain::SwapChain(Device* device, - Surface* surface, - NewSwapChainBase* previousSwapChain, - const SwapChainDescriptor* descriptor) - : NewSwapChainBase(device, surface, descriptor) { + // static + ResultOrError SwapChain::Create(Device* device, + Surface* surface, + NewSwapChainBase* previousSwapChain, + const SwapChainDescriptor* descriptor) { + std::unique_ptr swapchain = + std::make_unique(device, surface, descriptor); + DAWN_TRY(swapchain->Initialize(previousSwapChain)); + return swapchain.release(); + } + + MaybeError SwapChain::Initialize(NewSwapChainBase* previousSwapChain) { if (previousSwapChain != nullptr) { // TODO(cwallez@chromium.org): figure out what should happen when surfaces are used by // multiple backends one after the other. It probably needs to block until the backend // and GPU are completely finished with the previous swapchain. - ASSERT(previousSwapChain->GetBackendType() == wgpu::BackendType::Null); - previousSwapChain->DetachFromSurface(); + if (previousSwapChain->GetBackendType() != wgpu::BackendType::Null) { + return DAWN_VALIDATION_ERROR("null::SwapChain cannot switch between APIs"); + } } + + return {}; } - SwapChain::~SwapChain() { - DetachFromSurface(); - } + SwapChain::~SwapChain() = default; MaybeError SwapChain::PresentImpl() { mTexture->Destroy(); diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h index 08f84a03d7..2ffadbe87b 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -251,13 +251,15 @@ namespace dawn_native { namespace null { class SwapChain final : public NewSwapChainBase { public: - SwapChain(Device* device, - Surface* surface, - NewSwapChainBase* previousSwapChain, - const SwapChainDescriptor* descriptor); + static ResultOrError Create(Device* device, + Surface* surface, + NewSwapChainBase* previousSwapChain, + const SwapChainDescriptor* descriptor); + ~SwapChain() override; private: - ~SwapChain() override; + using NewSwapChainBase::NewSwapChainBase; + MaybeError Initialize(NewSwapChainBase* previousSwapChain); Ref mTexture; diff --git a/src/dawn_native/vulkan/SwapChainVk.cpp b/src/dawn_native/vulkan/SwapChainVk.cpp index 8d0804b013..d56c4306e4 100644 --- a/src/dawn_native/vulkan/SwapChainVk.cpp +++ b/src/dawn_native/vulkan/SwapChainVk.cpp @@ -189,7 +189,7 @@ namespace dawn_native { namespace vulkan { Device* device = ToBackend(GetDevice()); Adapter* adapter = ToBackend(GetDevice()->GetAdapter()); - VkSwapchainKHR oldVkSwapChain = VK_NULL_HANDLE; + VkSwapchainKHR previousVkSwapChain = VK_NULL_HANDLE; if (previousSwapChain != nullptr) { // TODO(cwallez@chromium.org): The first time a surface is used with a Device, check @@ -198,30 +198,33 @@ namespace dawn_native { namespace vulkan { // TODO(cwallez@chromium.org): figure out what should happen when surfaces are used by // multiple backends one after the other. It probably needs to block until the backend // and GPU are completely finished with the previous swapchain. - ASSERT(previousSwapChain->GetBackendType() == wgpu::BackendType::Vulkan); - - // The previous swapchain is a dawn_native::vulkan::SwapChain so we can reuse its - // VkSurfaceKHR provided they are on the same instance. - // TODO(cwallez@chromium.org): check they are the same instance. + if (previousSwapChain->GetBackendType() != wgpu::BackendType::Vulkan) { + return DAWN_VALIDATION_ERROR("vulkan::SwapChain cannot switch between APIs"); + } // TODO(cwallez@chromium.org): use ToBackend once OldSwapChainBase is removed. SwapChain* previousVulkanSwapChain = static_cast(previousSwapChain); - std::swap(previousVulkanSwapChain->mVkSurface, mVkSurface); // TODO(cwallez@chromium.org): Figure out switching a single surface between multiple - // Vulkan devices. Probably needs to block too, but could reuse the surface! - ASSERT(previousSwapChain->GetDevice() == GetDevice()); - - // The previous swapchain was on the same Vulkan device so we can use Vulkan's - // "oldSwapchain" mechanism to ensure a seamless transition. We track the old swapchain - // for release immediately so it is not leaked in case of an error. (Vulkan allows - // destroying it immediately after the call to vkCreateSwapChainKHR but tracking - // using the fenced deleter makes the code simpler). - std::swap(previousVulkanSwapChain->mSwapChain, oldVkSwapChain); - device->GetFencedDeleter()->DeleteWhenUnused(oldVkSwapChain); - - if (previousSwapChain != this) { - previousSwapChain->DetachFromSurface(); + // Vulkan devices on different VkInstances. Probably needs to block too! + VkInstance previousInstance = + ToBackend(previousSwapChain->GetDevice())->GetVkInstance(); + if (previousInstance != ToBackend(GetDevice())->GetVkInstance()) { + return DAWN_VALIDATION_ERROR("vulkan::SwapChain cannot switch between instances"); } + + // The previous swapchain is a dawn_native::vulkan::SwapChain so we can reuse its + // VkSurfaceKHR provided since they are on the same instance. + std::swap(previousVulkanSwapChain->mVkSurface, mVkSurface); + + // The previous swapchain was on the same Vulkan instance so we can use Vulkan's + // "oldSwapchain" mechanism to ensure a seamless transition. We track the previous + // swapchain for release immediately so it is not leaked in case of an error. (Vulkan + // allows destroying it immediately after the call to vkCreateSwapChainKHR but tracking + // using the fenced deleter makes the code simpler). + std::swap(previousVulkanSwapChain->mSwapChain, previousVkSwapChain); + ToBackend(previousSwapChain->GetDevice()) + ->GetFencedDeleter() + ->DeleteWhenUnused(previousVkSwapChain); } if (mVkSurface == VK_NULL_HANDLE) { @@ -252,7 +255,7 @@ namespace dawn_native { namespace vulkan { createInfo.compositeAlpha = VK_COMPOSITE_ALPHA_OPAQUE_BIT_KHR; // TODO createInfo.presentMode = mConfig.presentMode; createInfo.clipped = false; - createInfo.oldSwapchain = oldVkSwapChain; + createInfo.oldSwapchain = previousVkSwapChain; DAWN_TRY(CheckVkSuccess(device->fn.CreateSwapchainKHR(device->GetVkDevice(), &createInfo, nullptr, &*mSwapChain), diff --git a/src/dawn_native/vulkan/VulkanError.cpp b/src/dawn_native/vulkan/VulkanError.cpp index f329f0851e..e0655b959f 100644 --- a/src/dawn_native/vulkan/VulkanError.cpp +++ b/src/dawn_native/vulkan/VulkanError.cpp @@ -60,6 +60,12 @@ namespace dawn_native { namespace vulkan { return "VK_ERROR_FORMAT_NOT_SUPPORTED"; case VK_ERROR_FRAGMENTED_POOL: return "VK_ERROR_FRAGMENTED_POOL"; + + case VK_ERROR_SURFACE_LOST_KHR: + return "VK_ERROR_SURFACE_LOST_KHR"; + case VK_ERROR_NATIVE_WINDOW_IN_USE_KHR: + return "VK_ERROR_NATIVE_WINDOW_IN_USE_KHR"; + case VK_FAKE_DEVICE_OOM_FOR_TESTING: return "VK_FAKE_DEVICE_OOM_FOR_TESTING"; case VK_FAKE_ERROR_FOR_TESTING: diff --git a/src/tests/end2end/SwapChainTests.cpp b/src/tests/end2end/SwapChainTests.cpp index c81307a03a..2a4cd93442 100644 --- a/src/tests/end2end/SwapChainTests.cpp +++ b/src/tests/end2end/SwapChainTests.cpp @@ -136,9 +136,9 @@ TEST_P(SwapChainTests, DestroySurfaceAfterGet) { // Test switching between present modes. TEST_P(SwapChainTests, SwitchPresentMode) { - // For unclear reasons recreating the swapchain produces a debug report warning on NVIDIA and - // makes the test fail. - DAWN_SKIP_TEST_IF(IsVulkan() && IsNvidia()); + // Fails with "internal drawable creation failed" on the Windows NVIDIA CQ builders but not + // locally. + DAWN_SKIP_TEST_IF(IsWindows() && IsVulkan() && IsNvidia()); constexpr wgpu::PresentMode kAllPresentModes[] = { wgpu::PresentMode::Immediate, @@ -165,10 +165,6 @@ TEST_P(SwapChainTests, SwitchPresentMode) { // Test resizing the swapchain and without resizing the window. TEST_P(SwapChainTests, ResizingSwapChainOnly) { - // For unclear reasons recreating the swapchain produces a debug report warning on NVIDIA and - // makes the test fail. - DAWN_SKIP_TEST_IF(IsVulkan() && IsNvidia()); - for (int i = 0; i < 10; i++) { wgpu::SwapChainDescriptor desc = baseDescriptor; desc.width += i * 10; @@ -195,10 +191,6 @@ TEST_P(SwapChainTests, ResizingWindowOnly) { // Test resizing both the window and the swapchain at the same time. TEST_P(SwapChainTests, ResizingWindowAndSwapChain) { - // For unclear reasons recreating the swapchain produces a debug report warning on NVIDIA and - // makes the test fail. - DAWN_SKIP_TEST_IF(IsVulkan() && IsNvidia()); - for (int i = 0; i < 10; i++) { glfwSetWindowSize(window, 400 - 10 * i, 400 + 10 * i); glfwPollEvents(); @@ -219,9 +211,10 @@ TEST_P(SwapChainTests, ResizingWindowAndSwapChain) { // Test switching devices on the same adapter. TEST_P(SwapChainTests, SwitchingDevice) { - // For unclear reasons recreating the swapchain produces a debug report warning on NVIDIA and - // makes the test fail. - DAWN_SKIP_TEST_IF(IsVulkan() && IsNvidia()); + // The Vulkan Validation Layers incorrectly disallow gracefully passing a swapchain between two + // VkDevices using "vkSwapchainCreateInfoKHR::oldSwapchain". + // See https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/2256 + DAWN_SKIP_TEST_IF(IsVulkan() && IsBackendValidationEnabled()); wgpu::Device device2 = wgpu::Device::Acquire(GetAdapter().CreateDevice()); diff --git a/src/tests/end2end/SwapChainValidationTests.cpp b/src/tests/end2end/SwapChainValidationTests.cpp index e92bf847e9..e5c6cc205a 100644 --- a/src/tests/end2end/SwapChainValidationTests.cpp +++ b/src/tests/end2end/SwapChainValidationTests.cpp @@ -71,24 +71,38 @@ class SwapChainValidationTests : public DawnTest { // Checks that an OutputAttachment view is an error by trying to create a render pass on it. void CheckTextureViewIsError(wgpu::TextureView view) { - utils::ComboRenderPassDescriptor renderPassDesc({view}); - - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPassDesc); - pass.EndPass(); - ASSERT_DEVICE_ERROR(encoder.Finish()); + CheckTextureView(view, true, false); } - // Checks that an OutputAttachment view is an error by trying to create a render pass on it. + // Checks that an OutputAttachment view is an error by trying to submit a render pass on it. void CheckTextureViewIsDestroyed(wgpu::TextureView view) { + CheckTextureView(view, false, true); + } + + // Checks that an OutputAttachment view is valid by submitting a render pass on it. + void CheckTextureViewIsValid(wgpu::TextureView view) { + CheckTextureView(view, false, false); + } + + private: + void CheckTextureView(wgpu::TextureView view, bool errorAtFinish, bool errorAtSubmit) { utils::ComboRenderPassDescriptor renderPassDesc({view}); wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPassDesc); pass.EndPass(); - wgpu::CommandBuffer commands = encoder.Finish(); - ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); + if (errorAtFinish) { + ASSERT_DEVICE_ERROR(encoder.Finish()); + } else { + wgpu::CommandBuffer commands = encoder.Finish(); + + if (errorAtSubmit) { + ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); + } else { + queue.Submit(1, &commands); + } + } } }; @@ -181,12 +195,17 @@ TEST_P(SwapChainValidationTests, PresentWithoutCurrentView) { ASSERT_DEVICE_ERROR(swapchain.Present()); } -// Check that the current view is in the destroyed state after the swapchain is destroyed. -TEST_P(SwapChainValidationTests, ViewDestroyedAfterSwapChainDestruction) { +// Check that the current view isn't destroyed when the ref to the swapchain is lost because the +// swapchain is kept alive by the surface. Also check after we lose all refs to the surface, the +// texture is destroyed. +TEST_P(SwapChainValidationTests, ViewValidAfterSwapChainRefLost) { wgpu::SwapChain swapchain = device.CreateSwapChain(surface, &goodDescriptor); wgpu::TextureView view = swapchain.GetCurrentTextureView(); - swapchain = nullptr; + swapchain = nullptr; + CheckTextureViewIsValid(view); + + surface = nullptr; CheckTextureViewIsDestroyed(view); }