tests/shader_runner_metal: Implement texture support.
Merge request reports
Activity
Why do we need the vkd3d_shader_scan_descriptor_info structure in the MSL runner? It may not be wrong to do it this way, but we should be able to simply bind each resource to the binding index corresponding to its slot (i.e., resource->r.desc.slot). Relatedly, do we really need separate argument buffers for each stage? I think we should be able to share the same argument buffer between different stages?
Unfortunately we have to do this because the argument buffer struct in shader (which is generated from shader descriptor info) must match the argument descriptors provided to argument encoder, so the memory layout is consistent. Apparently most of our vs and ps have different argument buffer structs.
Unfortunately we have to do this because the argument buffer struct in shader (which is generated from shader descriptor info) must match the argument descriptors provided to argument encoder, so the memory layout is consistent.
We get information about which descriptors are used from "gen->descriptor_info", but binding information comes from "gen->interface_info". In particular, that's where we get the "[[id(n)]]" attribute used to map the descriptor to an argument buffer element. I.e., "gen->interface_info" should accurately reflect where descriptors are stored in the argument buffer, and that layout need not match the order of descriptors in "gen->descriptor_info". (And indeed, allowing the caller to specify the descriptor layout is much of the point of the vkd3d_shader_interface_info structure.)
It's a Metal restriction not I force it on purpose... Or I need to insert some padding fields for bindings that are inside "gen->interface_info" but not in "gen->descriptor_info", is that OK?
Edited by Feifan He
It's a Metal restriction not I force it on purpose... Or I need to insert some padding fields for bindings that are inside "gen->interface_info" but not in "gen->descriptor_info", is that OK?
Potentially, but it's still not clear to me why you'd need that. What is the specific Metal restriction you're running into? The Metal Shading Language Specification (version 3.2) says the following: "The same index may not be assigned to more than one member of an argument buffer. Manually assigned indices do not need to be contiguous, but they must be monotonically increasing."
As I have said the memory layouts must match. However a single
[[id]]
does not determine the offset of a binding. For example we have a constant buffer and a srv to be bound, the constant buffer is used by vertex shader and the srv is used by pixel shader.struct vkd3d_vs_descriptors { constant vkd3d_vec4 *cb_0 [[id(0)]]; } struct vkd3d_ps_descriptors { texture2d<float, access::read> srv_0 [[id(1)]]; }
so you may think it's enough to have only one argument encoder and argument buffer, but it doesn't work. because
srv_0
is started from byte offset 0 in the argument buffer struct of pixel shader, where as the "common argument encoder" will encode[[id(1)]]
at byte offset 8. To achieve the result you want, the generated shader code should be like this:struct vkd3d_vs_descriptors { constant vkd3d_vec4 *cb_0 [[id(0)]]; ulong srv_0_padding [[id(1)]]; // not necessary } struct vkd3d_ps_descriptors { ulong cb_0_padding [[id(0)]]; texture2d<float, access::read> srv_0 [[id(1)]]; }
Edited by Feifan He
As I have said the memory layouts must match. However a single
[[id]]
does not determine the offset of a binding. For example we have a constant buffer and a srv to be bound, the constant buffer is used by vertex shader and the srv is used by pixel shader.struct vkd3d_vs_descriptors { constant vkd3d_vec4 *cb_0 [[id(0)]]; } struct vkd3d_ps_descriptors { texture2d<float, access::read> srv_0 [[id(1)]]; }
so you may think it's enough to have only one argument encoder and argument buffer, but it doesn't work. because
srv_0
is started from byte offset 0 in the argument buffer struct of pixel shader, where as the "common argument encoder" will encode[[id(1)]]
at byte offset 8.Would you happen to have either a reference to the specification/documentation, or some sample code that I could use to look into that? It may just be my unfamiliarity with Metal/MSL, but "[[id(n)]]" not working as expected just seems suspicious.
Ultimately what matters is that the generated MSL conforms to what was requested by the API though; struct vkd3d_shader_interface_info specifically in this case. If that means that we have to add explicit padding to the "descriptors" structure in the generated MSL, so be it.
Would you happen to have either a reference to the specification/documentation, or some sample code that I could use to look into that? It may just be my unfamiliarity with Metal/MSL, but "[[id(n)]]" not working as expected just seems suspicious.
The provided example is already a good sample to demonstrate the issue. And "[[id(n)]]" only works as expected if the
MTLArgumentDescriptor
s match the argument buffer struct declared in shader code (and no you can't find this statement in any documentation...). Apparently we are emitting different structs in vs and ps in that case, and that's how it doesn't work.If that means that we have to add explicit padding to the "descriptors" structure in the generated MSL, so be it.
Ok I'll change to that
Edited by Feifan He
I significantly reworked Feifan's code to meet Henri's requests, see https://gitlab.winehq.org/giomasce/vkd3d/-/commits/msl.
- The binding index now control the physical offset in the argument buffer, rather than the
[[id(N)]]
decoration (which, AFAIU, is pretty much useless for us, as it is read only when usingnewArgumentEncoderWithBufferIndex:
, while we usenewArgumentEncoderWithArguments:
instead); in fact, I don't even generate the decoration anymore. - Correspondingly, padding is generated for binding indices which are not used.
- That also means that descriptor generation is now controlled by the binding table rather than by the descriptor table; that's not necessarily ideal, as it breaks the implicit assumptions about which fields are supposed to be unique, but I think it's a reasonable step before a more complete binding model for Metal is devised (the Metal generator interface is still assumed to be unstable anyway).
- I think I could now change the Metal shader runner to use the same argument buffer for all stages, but I haven't implemented that yet, and I won't have time to do that today. I'll probably try that at some point.
- The binding index now control the physical offset in the argument buffer, rather than the
mentioned in merge request !1448