[PATCH v3 0/34] MR294: vkd3d: Store command list commands in a buffer until executed.

[Conor McCarthy (＠cmccarthy)]

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.

--
  v3: 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

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d/command.c:
> return CONTAINING_RECORD(iface, struct d3d12_command_list, ID3D12GraphicsCommandList3_iface);
>  }
>
> -static void d3d12_command_list_invalidate_current_framebuffer(struct d3d12_command_list *list)
> +static void d3d12_command_list_invalidate_current_framebuffer(struct d3d12_command_list_state *list)

Cosmetic, but here and in similar functions the function name prefix should probably become `d3d12_command_list_state_`.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_42745

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d/command.c:
> -    if (list->allocator)
> +static HRESULT STDMETHODCALLTYPE d3d12_command_list_Close(ID3D12GraphicsCommandList3 *iface)
> +{
> +    struct d3d12_command_list *list = impl_from_ID3D12GraphicsCommandList3(iface);
> +
> +    TRACE("iface %p.\n", iface);
> +
> +    if (!list->is_recording)
>      {
> -        d3d12_command_allocator_remove_command_list(list->allocator, list);
> -        list->allocator = NULL;
> +        WARN("Command list is not in the recording state.\n");
> +        return E_FAIL;
>      }
>
> +    if (list->allocator)
> You already check for `list->allocator` to be valid inside `d3d12_command_allocatore_remove_command_list()`.

OTOH, you don't set `list->allocator = NULL` any more. Is that intended?

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_42746

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d/command.c:
> static void d3d12_command_allocator_remove_command_list(struct d3d12_command_allocator *allocator,
>          const struct d3d12_command_list *list)
>  {
> -    if (allocator->current_command_list == list)
> +    if (allocator && allocator->current_command_list == list)
> 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?

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_42747

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d/command.c:
> return S_OK;
>  }
>
> -static void d3d12_command_list_reset_state(struct d3d12_command_list *list,
> -        ID3D12PipelineState *initial_pipeline_state)
> +static void d3d12_command_list_state_flush(struct d3d12_command_list_state *list,
> +        struct d3d12_command_list *cmd_list)

That's also quite cosmetic, but in general (and particularly here) I'd try to call the `d3d12_command_list_state` parameter with a name list `state` rather than `list`, so there is less confusion with the other parameter.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_42748

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d/command.c:
> UINT command_list_count, ID3D12CommandList * const *command_lists)
>  {
>      struct d3d12_command_queue *command_queue = impl_from_ID3D12CommandQueue(iface);
> +    struct d3d12_command_list_state list_state;

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.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_42749

[Henri Verbeet (＠hverbeet)]

> ```diff
> +typedef void (*d3d12_command_list_handler)(struct d3d12_command_list *list, const void *data);
> +
> +struct draw_instanced_op
> +{
> +    d3d12_command_list_handler handler;
> +    unsigned int vertex_count_per_instance;
> +    unsigned int instance_count;
> +    unsigned int start_vertex_location;
> +    unsigned int start_instance_location;
> +};
> ```

Using a callback here works, but I'd prefer having explicit operation IDs.
> ```diff
> +    header_size = offsetof(struct command_op_packet, data[0]);
> +    packet_size = offsetof(struct command_op_packet, data[size]);
> +    packet_size = (packet_size + header_size - 1) & ~(header_size - 1);
> ```

I.e., "packet_size = align(packet_size, header_size)".
> ```diff
> +    new_size = command_heap->data_size + packet_size;
> +    if (!vkd3d_array_reserve((void **)&command_heap->data, &command_heap->data_capacity, max(new_size, 1024u), 1))
> +    {
> ```

So we want this array to be at least 1024 bytes large. That seems somewhat speculative, but in any case, why not add a vkd3d_array_reserve() call to d3d12_command_heap_init() then? I'd also feel slightly better about this if "1024" was an appropriately named constant.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/294#note_42860