[PATCH v4 0/34] MR294: vkd3d: Store command list commands in a buffer until executed.
It's no problem to send fewer of these per MR. I have included the complete set because all but the last introduce no functional changes, and upstreaming a smaller set would leave the changes in a half-done state with unnecessary buffering. -- v4: vkd3d: Store command list commands in a buffer until executed. vkd3d: Store WriteBufferImmediate() arguments in a buffer. vkd3d: Store ExecuteIndirect() arguments in a buffer. vkd3d: Store SetPredication() arguments in a buffer. vkd3d: Store ResolveQueryData() arguments in a buffer. vkd3d: Store EndQuery() arguments in a buffer. vkd3d: Store BeginQuery() arguments in a buffer. vkd3d: Store d3d12_command_list_clear_uav() arguments in a buffer. vkd3d: Store ClearRenderTargetView() arguments in a buffer. vkd3d: Store ClearDepthStencilView() arguments in a buffer. vkd3d: Store OMSetRenderTargets() arguments in a buffer. vkd3d: Store SOSetTargets() arguments in a buffer. vkd3d: Store IASetVertexBuffers() arguments in a buffer. vkd3d: Store IASetIndexBuffer() arguments in a buffer. vkd3d: Store d3d12_command_list_set_root_descriptor() arguments in a buffer. vkd3d: Store d3d12_command_list_set_root_cbv() arguments in a buffer. vkd3d: Store d3d12_command_list_set_root_constants() arguments in a buffer. vkd3d: Store d3d12_command_list_set_descriptor_table() arguments in a buffer. vkd3d: Store d3d12_command_list_set_root_signature() arguments in a buffer. vkd3d: Add an internal refcount to struct d3d12_root_signature. vkd3d: Store ResourceBarrier() arguments in a buffer. vkd3d: Store SetPipelineState() arguments in a buffer. vkd3d: Store OMSetStencilRef() arguments in a buffer. vkd3d: Store OMSetBlendFactor() arguments in a buffer. vkd3d: Store RSSetScissorRects() arguments in a buffer. vkd3d: Store RSSetViewports() arguments in a buffer. vkd3d: Store IASetPrimitiveTopology() arguments in a buffer. vkd3d: Store ResolveSubresource() arguments in a buffer. vkd3d: Store CopyResource() arguments in a buffer. vkd3d: Store CopyTextureRegion() arguments in a buffer. vkd3d: Store CopyBufferRegion() arguments in a buffer. vkd3d: Store Dispatch() arguments in a buffer. vkd3d: Store DrawIndexedInstanced() arguments in a buffer. vkd3d: Store DrawInstanced() arguments in a buffer. This merge request has too many patches to be relayed via email. Please visit the URL below to see the contents of the merge request. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294
OTOH, you don't set `list->allocator = NULL` any more. Is that intended?
Yes, the allocator is needed later, e.g. some commands need to store views in it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_43124
On Fri Aug 18 15:03:32 2023 +0000, Giovanni Mascellani wrote:
That's preexisting, but is there a legitimate situation in which `allocator->current_command_list` and `list` can differ? Would it make sense to emit a warning in that case? Looks like this should not occur and we can warn about it, but it should be a separate patch.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_43125
On Fri Aug 18 15:03:33 2023 +0000, Giovanni Mascellani wrote:
Could we move this inside `d3d12_command_list_state_flush()` (and possibly rename `d3d12_command_list_state_flush()` to `d3d12_command_list_flush()`)? I think it communicates more evidently that the command list state is kept alive just while flushing and then discarded. This is not the last patch series, and the `list_state` declaration will be moved elsewhere. The `cmd_list` parameter to `d3d12_command_list_state_flush()` will disappear too.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_43126
Using a callback here works, but I'd prefer having explicit operation IDs.
This series has a small but significant performance cost, so I would like to avoid loading the handler pointer from a table.
That seems somewhat speculative, but in any case, why not add a vkd3d_array_reserve() call to d3d12_command_heap_init() then?
In a later patch the buffer cannot be reused simply, so this will be changed so the minimum is the largest of the command list's previous capacities, and the previous capacity is set to an initial value in `d3d12_command_heap_init()`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_43128
Using a callback here works, but I'd prefer having explicit operation IDs.
This series has a small but significant performance cost, so I would like to avoid loading the handler pointer from a table.
I'm not sure it makes a difference, but I'm also not sure loading that pointer from the command buffer instead is necessarily better/faster than loading it from a fixed table. If we want to optimise this code we should perhaps look at the threaded dispatch techniques typically used by JIT compilers and interpreters. I have a suspicion that the kind of operations we're doing here are complex enough that dispatch isn't going to be much of a bottleneck, but I could of course be wrong. In any case, the way to go about it would be to start with the naive implementation to establish a baseline, and then show that making it more complicated improves things.
In a later patch the buffer cannot be reused simply, so this will be changed so the minimum is the largest of the command list's previous capacities, and the previous capacity is set to an initial value in `d3d12_command_heap_init()`.
Is that patch in this series? I wasn't immediately able to find it. It's generally best to avoid depending on later patches for justifying code though. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_43311
participants (2)
-
Conor McCarthy (＠cmccarthy) -
Henri Verbeet (＠hverbeet)