[PATCH 0/33] 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.

-- 
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 impl_from_ID3D12CommandAllocator(iface);
>  }
>
> +struct draw_instanced_args
> +{
> +    unsigned int vertex_count_per_instance;

If you add a `struct d3d12_command_header header` field at the beginning of the struct representing each call (and also in `union d3d12_command_args`) you probably have less bookkeeping to do in `d3d12_command_heap_reserve()`, and maybe it's also clearer what you want to do. You don't even have to care that the size of the header is a multiple of the stride, I think.

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

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d/vkd3d_private.h:
> VKD3D_PIPELINE_BIND_POINT_COUNT = 0x2,
>  };
>
> +struct d3d12_command_heap
> +{
> +    size_t *commands;

Aligning the commands in the array to pointers makes totally sense (in C11 it would make even more sense to align to `max_align_t`, just to be sure), but I would still define the array as `void *`. Since you're going to type pun it a lot anyway, using `size_t *` just feels misleading towards the reader to me.

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

[Giovanni Mascellani (＠giomasce)]

I still have to work my way through the last patch, so I'm sending a couple of preliminary comments for the moment.

Also, after the last commit four tests fail here (on RADV and llvmpipe) with this assertion:
```
shader_runner: ../vkd3d/libs/vkd3d/command.c:434: root_signature_get_descriptor_table: Assertion `p->parameter_type == D3D12_ROOT_PARAMETER_TYPE_DESCRIPTOR_TABLE' failed.
```

The tests are:
* `compute.shader_test`
* `numthreads.shader_test`
* `uav-out-param.shader_test`
* `uav-load.shader_test`

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

[Henri Verbeet (＠hverbeet)]

Perhaps unsurprisingly, I'm seeing a lot of similarities between this and wined3d_command_list/wined3d_deferred_context. That code has the advantage of having seen a fair amount of production testing, as well as being familiar to existing wined3d contributors. Unless there's a compelling argument to deviate from that setup, could we please try to keep the vkd3d implementation similar to that one?

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