[PATCH 0/4] MR394: vkd3d-shader: sm1 texture interface.
This defines (and, broadly, implements) the API for mapping sm1 combined samplers to the target environment. This is possibly the simplest bit of API for sm1 support thus far. It does not actually involve any API changes. Rather, each combined sampler is split into two descriptors, one for the sampler and one for the texture, both with the same binding index as the original combined sampler. -- I feel rather positive about this bit of API. it required very little design concern or implementation difficulty on the vkd3d side. On the Wine side, it required quite a lot of change, but I think pretty much all of that change was positive on its own merits, and should probably be applied independently of the sm1/Vulkan work. We've talked about translating wined3d_device_set_sampler_state() and wined3d_device_set_texture() to wined3d_device_set_sampler() and wined3d_device_set_shader_resource_view() for quite some time now, and that was pretty much exactly what was required to make this work—that and choosing the obvious mapping of "same index for both state objects". The most obvious alternative to this design, to me, would be to reroll struct vkd3d_shader_interface_info (or extend it without rerolling), adding a new array for mapping a combined sampler in the *source* environment to two separate descriptors in the *target* environment, i.e. struct vkd3d_shader_resource_binding in reverse. We would probably also want to design a way to map a combined sampler to a combined sampler (a sixth array? Or reuse struct vkd3d_shader_descriptor_binding with a new descriptor type?) I suppose the advantage of such an alternative design would be that it reports "one" descriptor as "one", not "two", but the associated complexity seems far greater, and accordingly I don't really want to give it the time of day. (Note also that this patch set handles combined -> combined mapping without any extra effort). I will of course yield to maintainer preference, though. -- As before, I've prepared proof-of-concept code for Wine. The branches are here: https://gitlab.winehq.org/zfigura/vkd3d/-/commits/himavant5 https://gitlab.winehq.org/zfigura/wine/-/commits/himavant6 The relevant test can be run with: make tests/shader_runner.exe && WINE_D3D_CONFIG=renderer=vulkan wine tests/shader_runner.exe ../vkd3d/tests/hlsl/tex2D.shader_test These branches are rather larger than the previous ones. By way of summary, the vkd3d branch contains these patches, plus about two dozen patches to hook up sm1 tests and to fix various corner cases surrounding semantic translation (much of which should be rewritten with a VSIR-based "lowering" approach in mind). The Wine branch contains: * Wine merge request 4027, * the patches for sm1 constant buffers, * a set of patches to transform individual sampler states into sampler objects (of which [1] is the most interesting), and * a set of patches to transform textures into SRVs (of which [2] is the most interesting). As described above, there is not actually any part of the Wine side that touches the vkd3d-shader interface. [1] https://gitlab.winehq.org/zfigura/wine/-/commit/984e478763c99157a6ec49388fe5... [2] https://gitlab.winehq.org/zfigura/wine/-/commit/722c1bf1f6a7691c9ed431bd4333... -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/394
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/d3d_asm.c | 3 ++- libs/vkd3d-shader/vkd3d_shader_private.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index 6a3513a28..1867aee94 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -637,7 +637,7 @@ static void shader_dump_decl_usage(struct vkd3d_d3d_asm_compiler *compiler, { struct vkd3d_string_buffer *buffer = &compiler->buffer; - if (semantic->resource.reg.reg.type == VKD3DSPR_SAMPLER) + if (semantic->resource.reg.reg.type == VKD3DSPR_COMBINED_SAMPLER) { switch (semantic->resource_type) { @@ -926,6 +926,7 @@ static void shader_dump_register(struct vkd3d_d3d_asm_compiler *compiler, const shader_addline(buffer, "aL"); break; + case VKD3DSPR_COMBINED_SAMPLER: case VKD3DSPR_SAMPLER: shader_addline(buffer, "s"); is_descriptor = true; diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index af75ef3bd..c33ddcc63 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -488,7 +488,7 @@ enum vkd3d_shader_register_type VKD3DSPR_CONSTINT = 7, VKD3DSPR_COLOROUT = 8, VKD3DSPR_DEPTHOUT = 9, - VKD3DSPR_SAMPLER = 10, + VKD3DSPR_COMBINED_SAMPLER = 10, VKD3DSPR_CONST2 = 11, VKD3DSPR_CONST3 = 12, VKD3DSPR_CONST4 = 13, @@ -504,6 +504,7 @@ enum vkd3d_shader_register_type VKD3DSPR_IMMCONSTBUFFER, VKD3DSPR_PRIMID, VKD3DSPR_NULL, + VKD3DSPR_SAMPLER, VKD3DSPR_RESOURCE, VKD3DSPR_UAV, VKD3DSPR_OUTPOINTID, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/394
From: Zebediah Figura <zfigura(a)codeweavers.com> --- include/vkd3d_shader.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/include/vkd3d_shader.h b/include/vkd3d_shader.h index 01356ce39..98c991102 100644 --- a/include/vkd3d_shader.h +++ b/include/vkd3d_shader.h @@ -388,6 +388,15 @@ enum vkd3d_shader_d3dbc_constant_register * - \a binding.binding = 5 * - \a binding.count = 1 * + * For legacy Direct3D bytecode, which uses registers representing combined + * resources and samplers, both the sampler and resource bindings should be + * specified in this array, using the same register_index value. + * In order to instead map a legacy Direct3D combined sampler to a combined + * sampler in the host environment, use + * struct vkd3d_shader_combined_resource_sampler, and set both + * \ref vkd3d_shader_combined_resource_sampler.resource_index and + * \ref vkd3d_shader_combined_resource_sampler.sampler_index to the same value. + * * This structure is used in struct vkd3d_shader_interface_info. */ struct vkd3d_shader_resource_binding @@ -423,6 +432,11 @@ struct vkd3d_shader_resource_binding * Describes the mapping of a Direct3D resource-sampler pair to a combined * sampler (i.e. sampled image). * + * This structure can also be used to map a legacy Direct3D sampler to a + * combined sampler in the host environment. + * In this case both \ref resource_index and \ref sampler_index should be set + * to the register index of the Direct3D sampler. + * * This structure is used in struct vkd3d_shader_interface_info. */ struct vkd3d_shader_combined_resource_sampler -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/394
From: Zebediah Figura <zfigura(a)codeweavers.com> This does not handle 1.x samplers yet. --- libs/vkd3d-shader/vkd3d_shader_main.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index a427d996b..9702cf185 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -837,6 +837,15 @@ static void vkd3d_shader_scan_sampler_declaration(struct vkd3d_shader_scan_conte d->flags |= VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_SAMPLER_COMPARISON_MODE; } +static void vkd3d_shader_scan_combined_sampler_declaration( + struct vkd3d_shader_scan_context *context, const struct vkd3d_shader_semantic *semantic) +{ + vkd3d_shader_scan_add_descriptor(context, VKD3D_SHADER_DESCRIPTOR_TYPE_SAMPLER, &semantic->resource.reg.reg, + &semantic->resource.range, VKD3D_SHADER_RESOURCE_NONE, VKD3D_SHADER_RESOURCE_DATA_UINT); + vkd3d_shader_scan_add_descriptor(context, VKD3D_SHADER_DESCRIPTOR_TYPE_SRV, &semantic->resource.reg.reg, + &semantic->resource.range, semantic->resource_type, VKD3D_SHADER_RESOURCE_DATA_FLOAT); +} + static void vkd3d_shader_scan_resource_declaration(struct vkd3d_shader_scan_context *context, const struct vkd3d_shader_resource *resource, enum vkd3d_shader_resource_type resource_type, enum vkd3d_shader_resource_data_type resource_data_type, @@ -945,6 +954,12 @@ static int vkd3d_shader_scan_instruction(struct vkd3d_shader_scan_context *conte vkd3d_shader_scan_sampler_declaration(context, instruction); break; case VKD3DSIH_DCL: + if (instruction->declaration.semantic.resource.reg.reg.type == VKD3DSPR_COMBINED_SAMPLER) + { + vkd3d_shader_scan_combined_sampler_declaration(context, &instruction->declaration.semantic); + break; + } + /* fall through */ case VKD3DSIH_DCL_UAV_TYPED: vkd3d_shader_scan_typed_resource_declaration(context, instruction); break; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/394
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/ir.c | 69 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index a97747b51..43f6cb43e 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -1350,6 +1350,72 @@ static void remove_dead_code(struct vkd3d_shader_parser *parser) } } +static enum vkd3d_result normalise_combined_samplers(struct vkd3d_shader_parser *parser) +{ + unsigned int i; + + for (i = 0; i < parser->instructions.count; ++i) + { + struct vkd3d_shader_instruction *ins = &parser->instructions.elements[i]; + struct vkd3d_shader_src_param *srcs; + + switch (ins->handler_idx) + { + case VKD3DSIH_TEX: + if (!(srcs = shader_src_param_allocator_get(&parser->instructions.src_params, 3))) + return VKD3D_ERROR_OUT_OF_MEMORY; + memset(srcs, 0, sizeof(*srcs) * 3); + + ins->handler_idx = VKD3DSIH_SAMPLE; + + srcs[0] = ins->src[0]; + + srcs[1].reg.type = VKD3DSPR_RESOURCE; + srcs[1].reg.idx[0] = ins->src[1].reg.idx[0]; + srcs[1].reg.idx[1] = ins->src[1].reg.idx[0]; + srcs[1].reg.idx_count = 2; + srcs[1].reg.data_type = VKD3D_DATA_RESOURCE; + srcs[1].swizzle = VKD3D_SHADER_NO_SWIZZLE; + + srcs[2].reg.type = VKD3DSPR_SAMPLER; + srcs[2].reg.idx[0] = ins->src[1].reg.idx[0]; + srcs[2].reg.idx[1] = ins->src[1].reg.idx[0]; + srcs[2].reg.idx_count = 2; + srcs[2].reg.data_type = VKD3D_DATA_SAMPLER; + + ins->src = srcs; + ins->src_count = 3; + break; + + case VKD3DSIH_TEXBEM: + case VKD3DSIH_TEXBEML: + case VKD3DSIH_TEXCOORD: + case VKD3DSIH_TEXDEPTH: + case VKD3DSIH_TEXDP3: + case VKD3DSIH_TEXDP3TEX: + case VKD3DSIH_TEXLDD: + case VKD3DSIH_TEXLDL: + case VKD3DSIH_TEXM3x2PAD: + case VKD3DSIH_TEXM3x2TEX: + case VKD3DSIH_TEXM3x3DIFF: + case VKD3DSIH_TEXM3x3PAD: + case VKD3DSIH_TEXM3x3SPEC: + case VKD3DSIH_TEXM3x3TEX: + case VKD3DSIH_TEXM3x3VSPEC: + case VKD3DSIH_TEXREG2AR: + case VKD3DSIH_TEXREG2GB: + case VKD3DSIH_TEXREG2RGB: + FIXME("Unhandled instruction %#x.\n", ins->handler_idx); + return VKD3D_ERROR_NOT_IMPLEMENTED; + + default: + break; + } + } + + return VKD3D_OK; +} + enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, const struct vkd3d_shader_compile_info *compile_info) { @@ -1380,6 +1446,9 @@ enum vkd3d_result vkd3d_shader_normalise(struct vkd3d_shader_parser *parser, if (result >= 0) remove_dead_code(parser); + if (result >= 0) + result = normalise_combined_samplers(parser); + if (result >= 0 && TRACE_ON()) vkd3d_shader_trace(instructions, &parser->shader_version); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/394
This defines (and, broadly, implements) the API for mapping sm1 combined samplers to the target environment. This is possibly the simplest bit of API for sm1 support thus far.
It does not actually involve any API changes. Rather, each combined sampler is split into two descriptors, one for the sampler and one for the texture, both with the same binding index as the original combined sampler.
\--
I feel rather positive about this bit of API. it required very little design concern or implementation difficulty on the vkd3d side.
Right, this is essentially how this was intended to work. An argument could perhaps be made that VKD3DSPR_COMBINED_SAMPLER is somewhat superfluous because the distinction between VKD3DSPR_COMBINED_SAMPLER and VKD3DSPR_SAMPLER is implied by the shader model, but I think the added clarity is helpful. There's perhaps also an argument that we should describe how d3dbc descriptors are mapped to vkd3d-shader descriptors in the vkd3d_shader_scan_descriptor_info documentation, much like we do for d3dbc shader constants. The instructions for vkd3d_shader_resource_binding and vkd3d_shader_combined_resource_sampler would then logically fall out of that.
```diff +static enum vkd3d_result normalise_combined_samplers(struct vkd3d_shader_parser *parser) +{ ... + FIXME("Unhandled instruction %#x.\n", ins->handler_idx); + return VKD3D_ERROR_NOT_IMPLEMENTED; ```
vkd3d_shader_parser_error(), please. Conveniently, we already have VKD3D_SHADER_ERROR_VSIR_NOT_IMPLEMENTED. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/394#note_48012
An argument could perhaps be made that VKD3DSPR_COMBINED_SAMPLER is somewhat superfluous because the distinction between VKD3DSPR_COMBINED_SAMPLER and VKD3DSPR_SAMPLER is implied by the shader model, but I think the added clarity is helpful.
Yes. In general I rather like the idea of not making the API semantics depend on the shader model, at least inasmuch as it affects the output. I suppose in this case it's also redundant because sm1 uses different instructions (TEX et al.) but I still think the clarity is helpful here.
There's perhaps also an argument that we should describe how d3dbc descriptors are mapped to vkd3d-shader descriptors in the vkd3d_shader_scan_descriptor_info documentation, much like we do for d3dbc shader constants. The instructions for vkd3d_shader_resource_binding and vkd3d_shader_combined_resource_sampler would then logically fall out of that.
Yeah, I agree, this shouldn't be in two different places. I'll change this in v2.
vkd3d_shader_parser_error(), please. Conveniently, we already have VKD3D_SHADER_ERROR_VSIR_NOT_IMPLEMENTED.
Will do. There was precedent elsewhere but that one was my fault too, I'll add a patch to fix it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/394#note_48051
participants (3)
-
Henri Verbeet (＠hverbeet) -
Zebediah Figura
-
Zebediah Figura (＠zfigura)