Extend `ID3D12DXVKInteropDevice` into `ID3D12DXVKInteropDevice1` to add `VkCommandBuffer` interop #2053

liam-middlebrook · 2024-07-31T21:10:06Z

Add functionality for grabbing the VkCommandBuffer which is internal to a ID3D12CommandList object. This allows for applications to do interop using DX12 synchronization primitives but enqueue work onto queues which are otherwise not exposed through traditional DX12 APIs.

This will be used as a basis for dxvk-nvapi to create a DX12 offering of the VK_NV_optical_flow functionality. Tagging @jp7677 and @Saancreed from that project as any feedback received here will be relevant to a follow-up pull request I will be making there.

Two open points of discussion:

Interface Naming

Should this be called ID3D12DXVKInteropDevice1 ? It has a high overlap in functionality with ID3D12DXVKInteropDevice so it seemed appropriate for it to inherit the methods. Saancreed suggested via Discord that it may make sense to rename ID3D12DXVKInteropDevice to ID3D12VKInteropDevice (keeping the same GUID, thus preserving ABI).

Scope of VkCommandBuffer retrieval

I chose to keep a simpler implementation where we can only retrieve the internal VkCommandBuffer from command lists which are reset with an interop-enabled ID3D12CommandAllocator. We could also in theory define some more fine-grained synchronization contracts so that the internal command buffer could be retrieved from non-interop command lists.

I don't see a pressing use-case for that to be needed right now, but I can think of plenty reasons why it may be attractive though. One may be to move the VK calls for CubinComputeShader* functionality back into dxvk-nvapi, where it can just record dispatches into the command-buffer directly rather than relying on vkd3d-proton to be updated in order to support new functions in that space. I'm unclear on what the maintenance burden would be for supporting this kind of thing (I know that vkd3d-proton does commandlist-internal work tracking and this would definitely complicate that if it had to account for a mix of internally+externally recorded work).

HansKristian-Work

The headers for commits should be vkd3d: not command:.

As for the name of the interface, I don't really care if it's ID3D12VKInterop or not, that's more up to @doitsujin.

libs/vkd3d/command.c

include/vkd3d_device_vkd3d_ext.idl

libs/vkd3d/device_vkd3d_ext.c

HansKristian-Work · 2024-08-01T10:11:14Z

I'm unclear on what the maintenance burden would be for supporting this kind of thing (I know that vkd3d-proton does commandlist-internal work tracking and this would definitely complicate that if it had to account for a mix of internally+externally recorded work).

I think this could work if there's a "begin interop" where we effectively invalidate all state in the command list (d3d12_invalidate_all_state), and flush any pending tracking. That basically means doing work that Close command list does.

Invalidation in vkd3d-proton won't work if the interop command buffer does really silly stuff like enabling things like queries and predication and not restoring that state, but that should be the responsibility of the interop client.

Saancreed · 2024-08-01T19:15:22Z

tests/d3d12_dxvk_interop_device.c

+    if (FAILED(ID3D12DXVKInteropDevice1_BeginVkCommandBufferInterop(interop_device,
+        (ID3D12CommandList*)command_list,
+        &vkCmdBuf)))
+    {
+        ok(hr == S_OK, "ID3D12DXVKInteropDevice1_GetVulkanCmdHandles failed %#x.\n", hr);


Shouldn't this now be something like the following?

Suggested change

if (FAILED(ID3D12DXVKInteropDevice1_BeginVkCommandBufferInterop(interop_device,

(ID3D12CommandList*)command_list,

&vkCmdBuf)))

{

ok(hr == S_OK, "ID3D12DXVKInteropDevice1_GetVulkanCmdHandles failed %#x.\n", hr);

if (FAILED(hr = ID3D12DXVKInteropDevice1_BeginVkCommandBufferInterop(interop_device,

(ID3D12CommandList*)command_list,

&vkCmdBuf)))

{

ok(hr == S_OK, "ID3D12DXVKInteropDevice1_BeginVkCommandBufferInterop failed %#x.\n", hr);

HansKristian-Work · 2024-08-02T10:36:44Z

libs/vkd3d/device_vkd3d_ext.c

+    if (!cmd_list)
+        return E_INVALIDARG;
+
+    d3d12_command_list_invalidate_all_state(cmd_list);


This isn't quite enough to make the interop work, but it's a start. We can probably flesh that out after this lands. Just add a FIXME comment here for now.

HansKristian-Work · 2024-08-02T10:38:56Z

tests/d3d12_dxvk_interop_device.c

+        return;
+    }
+
+    if (FAILED(ID3D12DXVKInteropDevice1_BeginVkCommandBufferInterop(interop_device,


hr is not set here.

HansKristian-Work · 2024-08-02T10:40:12Z

tests/d3d12_dxvk_interop_device.c

+    ID3D12DXVKInteropDevice1_GetVulkanResourceInfo1(interop_device,
+        buffer,
+        (uint64_t*)&vkBuffer,
+        &offset,


This needs to be UINT64 to get the 8 byte alignment on 32-bit. Both the handle and offset it seems.

HansKristian-Work · 2024-08-02T10:41:26Z

libs/vkd3d/breadcrumbs.c

@@ -599,21 +599,30 @@ void vkd3d_breadcrumb_tracer_report_device_lost(struct vkd3d_breadcrumb_tracer *
    if (device->vk_info.NV_device_diagnostic_checkpoints)
    {
        /* vkGetQueueCheckpointDataNV does not require us to synchronize access to the queue. */
-        queue_family_info = d3d12_device_get_vkd3d_queue_family(device, D3D12_COMMAND_LIST_TYPE_DIRECT);
+        queue_family_info = d3d12_device_get_vkd3d_queue_family(device,
+                                D3D12_COMMAND_LIST_TYPE_DIRECT,


Just keep this on one line, the indents here are a bit goofy. Ditto the similar code below.

HansKristian-Work · 2024-08-02T10:45:42Z

include/vkd3d_device_vkd3d_ext.idl

+    HRESULT GetVulkanResourceInfo1(ID3D12Resource *resource, UINT64 *vk_handle, UINT64 *buffer_offset, VkFormat *format);
+    HRESULT CreateInteropCommandQueue(const D3D12_COMMAND_QUEUE_DESC *pDesc, UINT32 vk_queue_family_index, ID3D12CommandQueue **ppQueue);
+    HRESULT CreateInteropCommandAllocator(D3D12_COMMAND_LIST_TYPE type, UINT32 vk_queue_family_index, ID3D12CommandAllocator **ppAllocator);
+    HRESULT BeginVkCommandBufferInterop(ID3D12CommandList *pCmdList, VkCommandBuffer *pCommandBuffer);


I think we need an EndVkCommandBufferInterop as well, just in case we need to support certain things in the future like conditional rendering and queries. For now, just implement it as a noop.

HansKristian-Work

The overall approach is reasonable I think and is not that invasive. Mostly nits now.

HansKristian-Work

This LGTM now, but needs to change the command: to vkd3d: on the commits as requested earlier.

…_info Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

…ly indices Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

Prepare for ID3D12DxvkInteropDevice1 by replacing usage of ID3D12DXVKInteropDevice * with a new typedefed d3d12_dxvk_interop_device_iface *. Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

Used for creating+submitting VkCommandBuffer handles in the context of D3D12 synchronization primitives. Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

HansKristian-Work

LGTM. Passes test suite on both NV and RADV.

liam-middlebrook force-pushed the interop-cmdbuf-sharing branch 2 times, most recently from 7cdb3d7 to 6aa1b2c Compare July 31, 2024 21:26

HansKristian-Work requested changes Aug 1, 2024

View reviewed changes

liam-middlebrook force-pushed the interop-cmdbuf-sharing branch from 6aa1b2c to ee8057c Compare August 1, 2024 18:34

Saancreed reviewed Aug 1, 2024

View reviewed changes

HansKristian-Work reviewed Aug 2, 2024

View reviewed changes

HansKristian-Work requested changes Aug 2, 2024

View reviewed changes

liam-middlebrook force-pushed the interop-cmdbuf-sharing branch from ee8057c to 72bf174 Compare August 9, 2024 18:55

HansKristian-Work requested changes Aug 12, 2024

View reviewed changes

liam-middlebrook added 5 commits August 12, 2024 09:47

vkd3d: Refactor allocator/queue creation to accept vkd3d_queue_family…

cbfbc86

…_info Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

vkd3d: Add command_allocator/queue creation paths using vk queue fami…

98a998e

…ly indices Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

vkd3d: Introduce d3d12_dxvk_interop_device_iface *

cea2d8c

Prepare for ID3D12DxvkInteropDevice1 by replacing usage of ID3D12DXVKInteropDevice * with a new typedefed d3d12_dxvk_interop_device_iface *. Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

vkd3d: Introduce DXVKInteropDevice1

dabb13b

Used for creating+submitting VkCommandBuffer handles in the context of D3D12 synchronization primitives. Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

tests: Add test for ID3D12DXVKInteropDevice1 cmdbuf sharing

13bbdc8

Signed-off-by: Liam Middlebrook <lmiddlebrook@nvidia.com>

liam-middlebrook force-pushed the interop-cmdbuf-sharing branch from 72bf174 to 13bbdc8 Compare August 12, 2024 16:48

HansKristian-Work approved these changes Aug 13, 2024

View reviewed changes

HansKristian-Work merged commit 1e873ca into HansKristian-Work:master Aug 13, 2024
3 checks passed

Saancreed mentioned this pull request Sep 25, 2024

Should DLSS 3 Frame Generation work today? jp7677/dxvk-nvapi#103

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend `ID3D12DXVKInteropDevice` into `ID3D12DXVKInteropDevice1` to add `VkCommandBuffer` interop #2053

Extend `ID3D12DXVKInteropDevice` into `ID3D12DXVKInteropDevice1` to add `VkCommandBuffer` interop #2053

liam-middlebrook commented Jul 31, 2024

HansKristian-Work left a comment

HansKristian-Work commented Aug 1, 2024 •

edited

Loading

Saancreed Aug 1, 2024

HansKristian-Work Aug 2, 2024

HansKristian-Work Aug 2, 2024

HansKristian-Work Aug 2, 2024

HansKristian-Work Aug 2, 2024

HansKristian-Work Aug 2, 2024 •

edited

Loading

HansKristian-Work left a comment

HansKristian-Work left a comment

HansKristian-Work left a comment

Extend ID3D12DXVKInteropDevice into ID3D12DXVKInteropDevice1 to add VkCommandBuffer interop #2053

Extend ID3D12DXVKInteropDevice into ID3D12DXVKInteropDevice1 to add VkCommandBuffer interop #2053

Conversation

liam-middlebrook commented Jul 31, 2024

Two open points of discussion:

Interface Naming

Scope of VkCommandBuffer retrieval

HansKristian-Work left a comment

Choose a reason for hiding this comment

HansKristian-Work commented Aug 1, 2024 • edited Loading

Saancreed Aug 1, 2024

Choose a reason for hiding this comment

HansKristian-Work Aug 2, 2024

Choose a reason for hiding this comment

HansKristian-Work Aug 2, 2024

Choose a reason for hiding this comment

HansKristian-Work Aug 2, 2024

Choose a reason for hiding this comment

HansKristian-Work Aug 2, 2024

Choose a reason for hiding this comment

HansKristian-Work Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

HansKristian-Work left a comment

Choose a reason for hiding this comment

HansKristian-Work left a comment

Choose a reason for hiding this comment

HansKristian-Work left a comment

Choose a reason for hiding this comment

Extend `ID3D12DXVKInteropDevice` into `ID3D12DXVKInteropDevice1` to add `VkCommandBuffer` interop #2053

Extend `ID3D12DXVKInteropDevice` into `ID3D12DXVKInteropDevice1` to add `VkCommandBuffer` interop #2053

HansKristian-Work commented Aug 1, 2024 •

edited

Loading

HansKristian-Work Aug 2, 2024 •

edited

Loading