[PATCH 0/6] MR3165: dxgi: Do not let the D3D12 swapchain directly access the low level Vulkan queue in vkd3d (first part).
Later patches are on https://gitlab.winehq.org/giomasce/wine/-/commits/chianti, though they still require some cleanup and cosmetic changes. Currently the D3D12 swapchain accesses the low level Vulkan queue backing a vkd3d `ID3D12CommandQueue` using the `vkd3d_acquire_vk_queue()` call in order to submit frames for presentation. This causes a number of bugs because `vkd3d_acquire_vk_queue()` lets the caller submit to the Vulkan queue even if some operations are enqueued in the vkd3d internal op queue, which can lead to a frame being presented even if drawing work on it isn't finished (and not even queued, from the viewpoint of Vulkan!). In order to fix this, I propose to introduce to add a few calls to vkd3d (currently [in this branch](https://gitlab.winehq.org/giomasce/vkd3d/-/commits/mongibello)), to allow the caller to enqueue calls to `vkQueueSubmit()` and `vkQueuePresentKHR()` through the vkd3d internal queue. The D3D12 swapchain would then use this new API instead of directly calling `vkQueueSubmit()` and `vkQueuePresentKHR()` as it does now. Patches in this MR don't implement those changes yet, but prepare the ground with some tests and light refactoring. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3165
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/dxgi/tests/dxgi.c | 49 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/dlls/dxgi/tests/dxgi.c b/dlls/dxgi/tests/dxgi.c index d29c5577c14..f84f74398ec 100644 --- a/dlls/dxgi/tests/dxgi.c +++ b/dlls/dxgi/tests/dxgi.c @@ -4936,9 +4936,12 @@ static void test_swapchain_backbuffer_index(IUnknown *device, BOOL is_d3d12) DXGI_SWAP_CHAIN_DESC swapchain_desc; unsigned int index, expected_index; IDXGISwapChain3 *swapchain3; + ID3D12Device *d3d12_device; + ID3D12CommandQueue *queue; IDXGISwapChain *swapchain; HRESULT hr, expected_hr; IDXGIFactory *factory; + ID3D12Fence *fence; unsigned int i, j; ULONG refcount; RECT rect; @@ -5041,6 +5044,52 @@ static void test_swapchain_backbuffer_index(IUnknown *device, BOOL is_d3d12) ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); } + if (is_d3d12) + { + hr = IUnknown_QueryInterface(device, &IID_ID3D12CommandQueue, (void **)&queue); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + hr = ID3D12CommandQueue_GetDevice(queue, &IID_ID3D12Device, (void **)&d3d12_device); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + hr = ID3D12Device_CreateFence(d3d12_device, 0, 0, &IID_ID3D12Fence, (void **)&fence); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + hr = ID3D12CommandQueue_Wait(queue, fence, 1); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + hr = IDXGISwapChain_ResizeBuffers(swapchain, 0, + rect.right, rect.bottom, DXGI_FORMAT_UNKNOWN, swapchain_desc.Flags); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + /* The back buffer index is updated when Present() is + * called, not when frames are presented. */ + for (j = 0; j < swapchain_desc.BufferCount + 2; ++j) + { + index = IDXGISwapChain3_GetCurrentBackBufferIndex(swapchain3); + expected_index = j % swapchain_desc.BufferCount; + ok(index == expected_index, "Got back buffer index %u, expected %u.\n", index, expected_index); + hr = IDXGISwapChain3_Present(swapchain3, 0, 0); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + } + + index = IDXGISwapChain3_GetCurrentBackBufferIndex(swapchain3); + expected_index = j % swapchain_desc.BufferCount; + ok(index == expected_index, "Got back buffer index %u, expected %u.\n", index, expected_index); + + hr = ID3D12Fence_Signal(fence, 1); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + index = IDXGISwapChain3_GetCurrentBackBufferIndex(swapchain3); + expected_index = j % swapchain_desc.BufferCount; + ok(index == expected_index, "Got back buffer index %u, expected %u.\n", index, expected_index); + + refcount = ID3D12Fence_Release(fence); + ok(!refcount, "Fence has %lu references left.\n", refcount); + ID3D12Device_Release(d3d12_device); + ID3D12CommandQueue_Release(queue); + } + wait_device_idle(device); IDXGISwapChain3_Release(swapchain3); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3165
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/dxgi/tests/dxgi.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/dlls/dxgi/tests/dxgi.c b/dlls/dxgi/tests/dxgi.c index f84f74398ec..60efcbd4eec 100644 --- a/dlls/dxgi/tests/dxgi.c +++ b/dlls/dxgi/tests/dxgi.c @@ -7727,8 +7727,11 @@ static void test_swapchain_present_count(IUnknown *device, BOOL is_d3d12) }; UINT present_count, expected; + ID3D12Device *d3d12_device; + ID3D12CommandQueue *queue; IDXGISwapChain *swapchain; - unsigned int i; + ID3D12Fence *fence; + unsigned int i, j; HWND window; HRESULT hr; @@ -7785,6 +7788,42 @@ static void test_swapchain_present_count(IUnknown *device, BOOL is_d3d12) ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); ok(present_count == expected, "Got unexpected present count %u, expected %u.\n", present_count, expected); + if (is_d3d12) + { + hr = IUnknown_QueryInterface(device, &IID_ID3D12CommandQueue, (void **)&queue); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + hr = ID3D12CommandQueue_GetDevice(queue, &IID_ID3D12Device, (void **)&d3d12_device); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + hr = ID3D12Device_CreateFence(d3d12_device, 0, 0, &IID_ID3D12Fence, (void **)&fence); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + hr = ID3D12CommandQueue_Wait(queue, fence, 1); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + /* The present count is updated when Present() is called, + * not when frames are presented. */ + hr = IDXGISwapChain_Present(swapchain, 0, 0); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + expected = present_count + 1; + hr = IDXGISwapChain_GetLastPresentCount(swapchain, &present_count); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(present_count == expected, "Got unexpected present count %u, expected %u.\n", present_count, expected); + + hr = ID3D12Fence_Signal(fence, 1); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + + expected = present_count; + hr = IDXGISwapChain_GetLastPresentCount(swapchain, &present_count); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(present_count == expected, "Got unexpected present count %u, expected %u.\n", present_count, expected); + + ID3D12Fence_Release(fence); + ID3D12Device_Release(d3d12_device); + ID3D12CommandQueue_Release(queue); + } + IDXGISwapChain_Release(swapchain); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3165
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> This is an oversight in eec9c3a2f63555604dd6e357bb3280e82e14ff5b: vk_format specifies the format used for the frontend resources. The backend images could use a different format, depending on what the swapchain supports. Also, introduce the terms "frontend" and "backend" for the two sides of the swapchain, which are probably easier to understand. --- dlls/dxgi/swapchain.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/dlls/dxgi/swapchain.c b/dlls/dxgi/swapchain.c index f760e931bb1..df6eb793bbe 100644 --- a/dlls/dxgi/swapchain.c +++ b/dlls/dxgi/swapchain.c @@ -1050,21 +1050,22 @@ struct d3d12_swapchain VkDevice vk_device; VkPhysicalDevice vk_physical_device; - /* D3D12 side of the swapchain: these objects are visible to the - * IDXGISwapChain client, so they must never be recreated, except - * when ResizeBuffers*() is called. */ + /* D3D12 side of the swapchain (frontend): these objects are + * visible to the IDXGISwapChain client, so they must never be + * recreated, except when ResizeBuffers*() is called. */ unsigned int buffer_count; VkDeviceMemory vk_memory; VkImage vk_images[DXGI_MAX_SWAP_CHAIN_BUFFERS]; ID3D12Resource *buffers[DXGI_MAX_SWAP_CHAIN_BUFFERS]; unsigned int current_buffer_index; + VkFormat vk_format; - /* Vulkan side of the swapchain: these objects are also destroyed - * and recreated when the Vulkan swapchain becomes out of date or - * when the synchronization interval is changed; this operation - * should be transparent to the IDXGISwapChain client (except for - * timings: recreating the Vulkan swapchain creates a noticeable - * delay, unfortunately). */ + /* Vulkan side of the swapchain (backend): these objects are also + * destroyed and recreated when the Vulkan swapchain becomes out + * of date or when the synchronization interval is changed; this + * operation should be transparent to the IDXGISwapChain client + * (except for timings: recreating the Vulkan swapchain creates a + * noticeable delay, unfortunately). */ VkSwapchainKHR vk_swapchain; VkCommandPool vk_cmd_pool; VkImage vk_swapchain_images[DXGI_MAX_SWAP_CHAIN_BUFFERS]; @@ -1073,7 +1074,6 @@ struct d3d12_swapchain unsigned int vk_swapchain_width; unsigned int vk_swapchain_height; VkPresentModeKHR present_mode; - VkFormat vk_format; uint32_t vk_image_index; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3165
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- dlls/dxgi/swapchain.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/dxgi/swapchain.c b/dlls/dxgi/swapchain.c index df6eb793bbe..6bb86f413de 100644 --- a/dlls/dxgi/swapchain.c +++ b/dlls/dxgi/swapchain.c @@ -1630,11 +1630,13 @@ static void d3d12_swapchain_destroy_resources(struct d3d12_swapchain *swapchain) { vk_funcs->p_vkDestroyImage(swapchain->vk_device, swapchain->vk_images[i], NULL); swapchain->vk_images[i] = VK_NULL_HANDLE; - - vk_funcs->p_vkFreeMemory(swapchain->vk_device, swapchain->vk_memory, NULL); - swapchain->vk_memory = VK_NULL_HANDLE; } } + if (swapchain->vk_device) + { + vk_funcs->p_vkFreeMemory(swapchain->vk_device, swapchain->vk_memory, NULL); + swapchain->vk_memory = VK_NULL_HANDLE; + } } static HRESULT d3d12_swapchain_create_vulkan_swapchain(struct d3d12_swapchain *swapchain) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3165
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> This is to eventually allow d3d12_swapchain_queue_present() to operate on a certain VkImage even if current_buffer_index has already been modified since the corresponding Present() call. --- dlls/dxgi/swapchain.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dlls/dxgi/swapchain.c b/dlls/dxgi/swapchain.c index 6bb86f413de..726d57334a1 100644 --- a/dlls/dxgi/swapchain.c +++ b/dlls/dxgi/swapchain.c @@ -1980,14 +1980,13 @@ static HRESULT d3d12_swapchain_set_sync_interval(struct d3d12_swapchain *swapcha return d3d12_swapchain_create_vulkan_resources(swapchain); } -static VkResult d3d12_swapchain_queue_present(struct d3d12_swapchain *swapchain, VkQueue vk_queue) +static VkResult d3d12_swapchain_queue_present(struct d3d12_swapchain *swapchain, VkQueue vk_queue, VkImage vk_src_image) { const struct dxgi_vk_funcs *vk_funcs = &swapchain->vk_funcs; VkPresentInfoKHR present_info; VkCommandBuffer vk_cmd_buffer; VkSubmitInfo submit_info; VkImage vk_dst_image; - VkImage vk_src_image; VkResult vr; if (swapchain->vk_image_index == INVALID_VK_IMAGE_INDEX) @@ -2000,7 +1999,6 @@ static VkResult d3d12_swapchain_queue_present(struct d3d12_swapchain *swapchain, vk_cmd_buffer = swapchain->vk_cmd_buffers[swapchain->vk_image_index]; vk_dst_image = swapchain->vk_swapchain_images[swapchain->vk_image_index]; - vk_src_image = swapchain->vk_images[swapchain->current_buffer_index]; if ((vr = vk_funcs->p_vkResetCommandBuffer(vk_cmd_buffer, 0)) < 0) { @@ -2076,7 +2074,8 @@ static HRESULT d3d12_swapchain_present(struct d3d12_swapchain *swapchain, return E_FAIL; } - vr = d3d12_swapchain_queue_present(swapchain, vk_queue); + vr = d3d12_swapchain_queue_present(swapchain, vk_queue, + swapchain->vk_images[swapchain->current_buffer_index]); if (vr == VK_ERROR_OUT_OF_DATE_KHR) { vkd3d_release_vk_queue(swapchain->command_queue); @@ -2093,7 +2092,8 @@ static HRESULT d3d12_swapchain_present(struct d3d12_swapchain *swapchain, return E_FAIL; } - if ((vr = d3d12_swapchain_queue_present(swapchain, vk_queue)) < 0) + if ((vr = d3d12_swapchain_queue_present(swapchain, vk_queue, + swapchain->vk_images[swapchain->current_buffer_index])) < 0) ERR("Failed to present after recreating swapchain, vr %d.\n", vr); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3165
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> This is to eventually hand frontend and backend resource management to different code pieces. --- dlls/dxgi/swapchain.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/dlls/dxgi/swapchain.c b/dlls/dxgi/swapchain.c index 726d57334a1..54cfa372cb2 100644 --- a/dlls/dxgi/swapchain.c +++ b/dlls/dxgi/swapchain.c @@ -1612,13 +1612,11 @@ static void d3d12_swapchain_destroy_vulkan_resources(struct d3d12_swapchain *swa } } -static void d3d12_swapchain_destroy_resources(struct d3d12_swapchain *swapchain) +static void d3d12_swapchain_destroy_d3d12_resources(struct d3d12_swapchain *swapchain) { const struct dxgi_vk_funcs *vk_funcs = &swapchain->vk_funcs; unsigned int i; - d3d12_swapchain_destroy_vulkan_resources(swapchain); - for (i = 0; i < swapchain->desc.BufferCount; ++i) { if (swapchain->buffers[i]) @@ -1775,7 +1773,7 @@ static HRESULT d3d12_swapchain_create_vulkan_resources(struct d3d12_swapchain *s return d3d12_swapchain_create_command_buffers(swapchain); } -static HRESULT d3d12_swapchain_create_resources(struct d3d12_swapchain *swapchain) +static HRESULT d3d12_swapchain_create_d3d12_resources(struct d3d12_swapchain *swapchain) { HRESULT hr; @@ -1788,10 +1786,7 @@ static HRESULT d3d12_swapchain_create_resources(struct d3d12_swapchain *swapchai if (FAILED(hr = d3d12_swapchain_create_user_buffers(swapchain))) return hr; - if (FAILED(hr = d3d12_swapchain_create_image_resources(swapchain))) - return hr; - - return d3d12_swapchain_create_vulkan_resources(swapchain); + return d3d12_swapchain_create_image_resources(swapchain); } static inline struct d3d12_swapchain *d3d12_swapchain_from_IDXGISwapChain4(IDXGISwapChain4 *iface) @@ -1840,7 +1835,8 @@ static void d3d12_swapchain_destroy(struct d3d12_swapchain *swapchain) const struct dxgi_vk_funcs *vk_funcs = &swapchain->vk_funcs; void *vulkan_module = vk_funcs->vulkan_module; - d3d12_swapchain_destroy_resources(swapchain); + d3d12_swapchain_destroy_vulkan_resources(swapchain); + d3d12_swapchain_destroy_d3d12_resources(swapchain); if (swapchain->frame_latency_event) CloseHandle(swapchain->frame_latency_event); @@ -2312,6 +2308,7 @@ static HRESULT d3d12_swapchain_resize_buffers(struct d3d12_swapchain *swapchain, DXGI_SWAP_CHAIN_DESC1 *desc, new_desc; unsigned int i; ULONG refcount; + HRESULT hr; if (flags) FIXME("Ignoring flags %#x.\n", flags); @@ -2361,9 +2358,14 @@ static HRESULT d3d12_swapchain_resize_buffers(struct d3d12_swapchain *swapchain, && desc->Format == new_desc.Format && desc->BufferCount == new_desc.BufferCount) return S_OK; - d3d12_swapchain_destroy_resources(swapchain); + d3d12_swapchain_destroy_vulkan_resources(swapchain); + d3d12_swapchain_destroy_d3d12_resources(swapchain); swapchain->desc = new_desc; - return d3d12_swapchain_create_resources(swapchain); + + if (FAILED(hr = d3d12_swapchain_create_d3d12_resources(swapchain))) + return hr; + + return d3d12_swapchain_create_vulkan_resources(swapchain); } static HRESULT STDMETHODCALLTYPE d3d12_swapchain_ResizeBuffers(IDXGISwapChain4 *iface, @@ -3045,7 +3047,13 @@ static HRESULT d3d12_swapchain_init(struct d3d12_swapchain *swapchain, IWineDXGI ID3D12CommandQueue_AddRef(swapchain->command_queue = queue); ID3D12Device_AddRef(swapchain->device = device); - if (FAILED(hr = d3d12_swapchain_create_resources(swapchain))) + if (FAILED(hr = d3d12_swapchain_create_d3d12_resources(swapchain))) + { + d3d12_swapchain_destroy(swapchain); + return hr; + } + + if (FAILED(hr = d3d12_swapchain_create_vulkan_resources(swapchain))) { d3d12_swapchain_destroy(swapchain); return hr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3165
participants (2)
-
Giovanni Mascellani -
Giovanni Mascellani (ļ¼ giomasce)