Skip to content
Snippets Groups Projects

tests/shader_runner_metal: Implement texture support.

Open Feifan He requested to merge Feifan/vkd3d:msl-texture-sampler into master
4 unresolved threads

Merge request reports

Members who can merge are allowed to add commits.
Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Feifan He added 1 commit

    added 1 commit

    • ad83862c - vkd3d-shader/msl: Implement VKD3DSIH_LD.

    Compare with previous version

  • Feifan He added 1 commit

    added 1 commit

    • 411fa325 - vkd3d-shader/msl: Implement VKD3DSIH_LD.

    Compare with previous version

    • 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?

    • Author Contributor

      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.

    • Please register or sign in to reply
    • 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.)

    • Author Contributor

      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
    • Please register or sign in to reply
    • 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."

    • Author Contributor

      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
    • Please register or sign in to reply
    • 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.

    • Author Contributor

      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 MTLArgumentDescriptors 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
    • Please register or sign in to reply
  • 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 using newArgumentEncoderWithBufferIndex:, while we use newArgumentEncoderWithArguments: 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.
  • Giovanni Mascellani mentioned in merge request !1448

    mentioned in merge request !1448

Please register or sign in to reply
Loading