diff --git a/generator/templates/dawn/native/ObjectType.cpp b/generator/templates/dawn/native/ObjectType.cpp index 90308f56b4e..bed4f227368 100644 --- a/generator/templates/dawn/native/ObjectType.cpp +++ b/generator/templates/dawn/native/ObjectType.cpp @@ -44,6 +44,8 @@ namespace {{native_namespace}} { {% endfor %} case ObjectType::BindGroupLayoutInternal: return "BindGroupLayoutInternal"; + case ObjectType::SwapChain: + return "SwapChain"; default: DAWN_UNREACHABLE(); } diff --git a/generator/templates/dawn/native/ObjectType.h b/generator/templates/dawn/native/ObjectType.h index c5a5a55d24a..48424e30258 100644 --- a/generator/templates/dawn/native/ObjectType.h +++ b/generator/templates/dawn/native/ObjectType.h @@ -44,9 +44,10 @@ namespace {{native_namespace}} { // Additional internal object types. Keep kExtraObjectTypes in sync when updating. BindGroupLayoutInternal, + SwapChain, }; - constexpr size_t kExtraObjectTypes = 1; + constexpr size_t kExtraObjectTypes = 2; template using PerObjectType = ityp::array; diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index c812391bc11..0e7454b3434 100644 --- a/src/dawn/native/Device.cpp +++ b/src/dawn/native/Device.cpp @@ -510,7 +510,7 @@ void DeviceBase::DestroyObjects() { // can destroy the frontend cache. // clang-format off - static constexpr std::array kObjectTypeDependencyOrder = { + static constexpr std::array kObjectTypeDependencyOrder = { // Encoders first, nothing refers to them. ObjectType::ComputePassEncoder, ObjectType::RenderPassEncoder, @@ -533,7 +533,8 @@ void DeviceBase::DestroyObjects() { ObjectType::SharedTextureMemory, ObjectType::SharedFence, ObjectType::ExternalTexture, - ObjectType::Texture, // Note that Textures own the TextureViews. + ObjectType::SwapChain, // Note that SwapChains own their current Texture. + ObjectType::Texture, // Note that Textures own the TextureViews. ObjectType::QuerySet, ObjectType::Sampler, ObjectType::Buffer, diff --git a/src/dawn/native/Surface.cpp b/src/dawn/native/Surface.cpp index 1abcaa14e86..874c8c31515 100644 --- a/src/dawn/native/Surface.cpp +++ b/src/dawn/native/Surface.cpp @@ -513,14 +513,34 @@ MaybeError Surface::Configure(const SurfaceConfiguration* configIn) { return {}; } +void Surface::DetachSwapChain(SwapChainBase* swapChain) { + DAWN_ASSERT(swapChain != nullptr); + DAWN_ASSERT(swapChain->GetSurface() == this); + + if (mSwapChain.Get() == swapChain) { + swapChain->DetachFromSurface(); + mSwapChain = nullptr; + // A failed Configure() with another device leaves the previous swapchain attached, in + // which case the surface stays configured with that other device. + if (mCurrentDevice.Get() == swapChain->GetDevice()) { + mCurrentDevice = nullptr; + } + } else if (mRecycledSwapChain.Get() == swapChain) { + swapChain->DetachFromSurface(); + mRecycledSwapChain = nullptr; + } else { + DAWN_UNREACHABLE(); + } +} + MaybeError Surface::Unconfigure() { if (IsError()) { DAWN_CHECK(mSwapChain == nullptr); DAWN_CHECK(mCurrentDevice == nullptr); return DAWN_VALIDATION_ERROR("%s is invalid.", this); } + // Unconfiguring an unconfigured surface is a no-op. mCurrentDevice = nullptr; - DAWN_INVALID_IF(!mSwapChain.Get(), "%s is not configured.", this); if (mSwapChain != nullptr) { if (mRecycledSwapChain != nullptr) { diff --git a/src/dawn/native/Surface.h b/src/dawn/native/Surface.h index 4e0ba99e0de..fc9318c554e 100644 --- a/src/dawn/native/Surface.h +++ b/src/dawn/native/Surface.h @@ -119,6 +119,12 @@ class Surface final : public ErrorMonad { const std::string& GetLabel() const; + // Called by `swapChain` when its device is destroyed while it is still attached to this + // surface. The swapchain is detached and the surface drops it. If it was the current + // swapchain the surface becomes unconfigured, as if Unconfigure() had been called, except + // that the swapchain cannot be recycled. + void DetachSwapChain(SwapChainBase* swapChain); + // Dawn API void APIConfigure(const SurfaceConfiguration* config); wgpu::Status APIGetCapabilities(AdapterBase* adapter, SurfaceCapabilities* capabilities) const; diff --git a/src/dawn/native/SwapChain.cpp b/src/dawn/native/SwapChain.cpp index b7e0192d2ab..3f5eb550c00 100644 --- a/src/dawn/native/SwapChain.cpp +++ b/src/dawn/native/SwapChain.cpp @@ -57,7 +57,7 @@ TextureDescriptor GetSwapChainBaseTextureDescriptor(SwapChainBase* swapChain) { SwapChainBase::SwapChainBase(DeviceBase* device, Surface* surface, const SurfaceConfiguration* config) - : mDevice(device), + : ApiObjectBase(device, kLabelNotImplemented), mWidth(config->width), mHeight(config->height), mFormat(config->format), @@ -72,6 +72,7 @@ SwapChainBase::SwapChainBase(DeviceBase* device, } mViewFormats.push_back(viewFormat); } + GetObjectTrackingList()->Track(this); } FormatSet SwapChainBase::ComputeViewFormatSet() const { @@ -90,6 +91,18 @@ SwapChainBase::~SwapChainBase() { DAWN_CHECK(!mAttached); } +void SwapChainBase::DestroyImpl(DestroyReason reason) { + // The surface has a Ref on the swapchains attached to it and detaches them before dropping + // that Ref, so a swapchain that is being deleted is already detached. The swapchain can + // still be attached when the device is destroyed though: detach it so that its backend + // resources are released before the device's, and so that the surface stops using a + // swapchain of a destroyed device. + if (mAttached) { + mSurface->DetachSwapChain(this); + } + DAWN_ASSERT(!mAttached); +} + void SwapChainBase::DetachFromSurface() { if (mAttached) { DetachFromSurfaceImpl(); @@ -99,6 +112,7 @@ void SwapChainBase::DetachFromSurface() { } void SwapChainBase::SetIsAttached() { + DAWN_ASSERT(!mAttached); mAttached = true; } @@ -143,9 +157,10 @@ MaybeError SwapChainBase::Present() { return {}; } -DeviceBase* SwapChainBase::GetDevice() const { - return mDevice.Get(); +ObjectType SwapChainBase::GetType() const { + return ObjectType::SwapChain; } + uint32_t SwapChainBase::GetWidth() const { return mWidth; } diff --git a/src/dawn/native/SwapChain.h b/src/dawn/native/SwapChain.h index 3db02fe830e..6704c640b20 100644 --- a/src/dawn/native/SwapChain.h +++ b/src/dawn/native/SwapChain.h @@ -46,7 +46,7 @@ struct SwapChainTextureInfo { wgpu::SurfaceGetCurrentTextureStatus status = wgpu::SurfaceGetCurrentTextureStatus::Error; }; -class SwapChainBase : public RefCounted { +class SwapChainBase : public ApiObjectBase { public: SwapChainBase(DeviceBase* device, Surface* surface, const SurfaceConfiguration* config); @@ -54,6 +54,7 @@ class SwapChainBase : public RefCounted { // // - The surface it is attached to is being destroyed. // - The swapchain is being replaced by another one on the surface. + // - The device that created it is being destroyed (see DestroyImpl). // // 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. @@ -61,25 +62,12 @@ class SwapChainBase : public RefCounted { // 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. - // - // Likewise the call for the swapchain being destroyed must be done in the backend's - // swapchain's destructor since C++ says it is UB to call virtual methods in the base class - // destructor. void DetachFromSurface(); void SetIsAttached(); - // TODO(crbug.com/dawn/831): - // APIRelease() can be called without any synchronization guarantees so we need to use a Release - // method that will call LockAndDeleteThis() on destruction. - // This is because losing the last reference to the SwapChain will detach its surface which - // explicitly destroys the current texture. Explicit destruction of textures is not thread safe - // yet. - // Hot path and intended to be shadowed. - // NOLINTNEXTLINE(bugprone-derived-method-shadowing-base-method) - void APIRelease() { ReleaseAndLockBeforeDestroy(); } - - DeviceBase* GetDevice() const; + ObjectType GetType() const override; + uint32_t GetWidth() const; uint32_t GetHeight() const; wgpu::TextureFormat GetFormat() const; @@ -99,13 +87,15 @@ class SwapChainBase : public RefCounted { ~SwapChainBase() override; private: + // Detaches the swapchain from its surface if it is still attached, which is the case when the + // device is destroyed while the surface is still configured with it. + void DestroyImpl(DestroyReason reason) override; + void SetChildLabel(ApiObjectBase* child) const; // Get a format set from mViewFormats (equivalent information, but easier to validate the // current texture) FormatSet ComputeViewFormatSet() const; - Ref mDevice; - bool mAttached = false; uint32_t mWidth = 0; uint32_t mHeight = 0; diff --git a/src/dawn/tests/end2end/SurfaceTests.cpp b/src/dawn/tests/end2end/SurfaceTests.cpp index 0fbeeaf0249..44a7d091f8a 100644 --- a/src/dawn/tests/end2end/SurfaceTests.cpp +++ b/src/dawn/tests/end2end/SurfaceTests.cpp @@ -574,6 +574,146 @@ TEST_P(SurfaceTests, PresentWithoutGet) { ASSERT_EQ(wgpu::Status::Success, presentStatus); } +// Releasing a configured surface after its device was destroyed must not crash. Regression test +// for the Vulkan backend dereferencing the destroyed device's FencedDeleter when the swapchain was +// detached by the surface destructor. +TEST_P(SurfaceTests, ReleaseSurfaceAfterDeviceDestroy) { + wgpu::Surface surface = CreateTestSurface(); + wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface); + surface.Configure(&config); + + wgpu::SurfaceTexture surfaceTexture; + surface.GetCurrentTexture(&surfaceTexture); + ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0}); + ASSERT_EQ(wgpu::Status::Success, surface.Present()); + + DestroyDevice(); + + // The surface is released at the end of the test body, after the device was destroyed. +} + +// Same as ReleaseSurfaceAfterDeviceDestroy but with the swapchain parked as the surface's recycled +// swapchain by Unconfigure() instead of being the current one. +TEST_P(SurfaceTests, ReleaseSurfaceAfterUnconfigureThenDeviceDestroy) { + // TODO(crbug.com/500766623): Fails due to backend validation errors on + // Windows 11/AMD RX 5500 XT w/ D3D12. + DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled()); + + wgpu::Surface surface = CreateTestSurface(); + wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface); + surface.Configure(&config); + + wgpu::SurfaceTexture surfaceTexture; + surface.GetCurrentTexture(&surfaceTexture); + ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0}); + ASSERT_EQ(wgpu::Status::Success, surface.Present()); + + surface.Unconfigure(); + DestroyDevice(); +} + +// Releasing a configured surface after the last external reference to its device was dropped +// (which destroys the device) must not crash. +TEST_P(SurfaceTests, ReleaseSurfaceAfterDeviceReleased) { + // TODO(crbug.com/500766623): Fails due to backend validation errors on + // Windows 11/AMD RX 5500 XT w/ D3D12. + DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled()); + + wgpu::Device device2 = CreateDevice(); + + wgpu::Surface surface = CreateTestSurface(); + wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface); + config.device = device2; + surface.Configure(&config); + + wgpu::SurfaceTexture surfaceTexture; + surface.GetCurrentTexture(&surfaceTexture); + ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0}, device2); + ASSERT_EQ(wgpu::Status::Success, surface.Present()); + + surfaceTexture.texture = nullptr; + device2 = nullptr; +} + +// A surface configured with a destroyed device can be configured again with a new device. +TEST_P(SurfaceTests, ReconfigureWithNewDeviceAfterDestroy) { + // TODO(crbug.com/dawn/269): Creating the IDXGISwapChain1 for the new device fails with + // E_ACCESSDENIED on D3D11 because the swapchain of the destroyed device is still alive, like + // in SwitchingDevice. + DAWN_SUPPRESS_TEST_IF(IsD3D11()); + + // TODO(crbug.com/500766623): Fails due to backend validation errors on + // Windows 11/AMD RX 5500 XT w/ D3D12. + DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled()); + + wgpu::Surface surface = CreateTestSurface(); + wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface); + surface.Configure(&config); + + wgpu::SurfaceTexture surfaceTexture; + surface.GetCurrentTexture(&surfaceTexture); + ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0}); + ASSERT_EQ(wgpu::Status::Success, surface.Present()); + + DestroyDevice(); + + wgpu::Device device2 = CreateDevice(); + config.device = device2; + surface.Configure(&config); + + surface.GetCurrentTexture(&surfaceTexture); + ASSERT_EQ(wgpu::SurfaceGetCurrentTextureStatus::SuccessOptimal, surfaceTexture.status); + ClearTexture(surfaceTexture.texture, {0.0, 1.0, 0.0, 1.0}, device2); + ASSERT_EQ(wgpu::Status::Success, surface.Present()); +} + +// Unconfiguring a surface after its device was destroyed must not crash. Destroying the device +// already unconfigured the surface, so this is a no-op like unconfiguring an unconfigured surface. +TEST_P(SurfaceTests, UnconfigureAfterDeviceDestroy) { + // TODO(crbug.com/500766623): Fails due to backend validation errors on + // Windows 11/AMD RX 5500 XT w/ D3D12. + DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled()); + + wgpu::Surface surface = CreateTestSurface(); + wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface); + surface.Configure(&config); + + wgpu::SurfaceTexture surfaceTexture; + surface.GetCurrentTexture(&surfaceTexture); + ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0}); + ASSERT_EQ(wgpu::Status::Success, surface.Present()); + + DestroyDevice(); + + surface.Unconfigure(); +} + +// Getting the current texture or presenting after the device was destroyed must not crash. The +// surface behaves as if it was unconfigured. +TEST_P(SurfaceTests, GetCurrentTextureAfterDeviceDestroy) { + // TODO(crbug.com/500766623): Fails due to backend validation errors on + // Windows 11/AMD RX 5500 XT w/ D3D12. + DAWN_SUPPRESS_TEST_IF(IsWindows11() && IsAMD() && IsD3D12() && IsBackendValidationEnabled()); + + wgpu::Surface surface = CreateTestSurface(); + wgpu::SurfaceConfiguration config = GetPreferredConfiguration(surface); + surface.Configure(&config); + + wgpu::SurfaceTexture surfaceTexture; + surface.GetCurrentTexture(&surfaceTexture); + ClearTexture(surfaceTexture.texture, {1.0, 0.0, 0.0, 1.0}); + ASSERT_EQ(wgpu::Status::Success, surface.Present()); + + DestroyDevice(); + + // The surface is unconfigured and has no device anymore, so the validation errors are + // reported to the instance. + surface.GetCurrentTexture(&surfaceTexture); + EXPECT_EQ(surfaceTexture.status, wgpu::SurfaceGetCurrentTextureStatus::Error); + EXPECT_EQ(surfaceTexture.texture, nullptr); + ASSERT_EQ(wgpu::Status::Error, surface.Present()); +} + // Check that all surfaces must support RenderAttachment. TEST_P(SurfaceTests, RenderAttachmentAlwaysSupported) { wgpu::Surface surface = CreateTestSurface();