MoltenVK
MoltenVK copied to clipboard
KhronosGroup
WIP: Transform feedback
This is a WIP to provide support to get Transform Feedback into MoltenVk
CMake build capability is an experiment at this time and currently only supports macOS builds.
Hi, curious if that work, is part of the present crossover 23 beta1 release which says: “We’re pleased to announce that this release also includes initial support for geometry shaders and transform feedback on macOS” Of course assuming is implemented on MoltenVK side..
Hi, curious if that work, is part of the present crossover 23 beta1 release which says: “We’re pleased to announce that this release also includes initial support for geometry shaders and transform feedback on macOS” Of course assuming is implemented on MoltenVK side..
I no longer work at CW, but IIRC they were investigating (at Apple's urging) the use of mesh shaders to implement that. This is for when the device doesn't support mesh shaders.
Hi, curious if that work, is part of the present crossover 23 beta1 release which says: “We’re pleased to announce that this release also includes initial support for geometry shaders and transform feedback on macOS” Of course assuming is implemented on MoltenVK side..
It doesn't seem to be implemented on the MVK side it seems rather done on the wine side, in fact using dxvk GS won't work at all
Would it not make sense to split the cmake building changes and transform feedback feature work into two different PRs?
Apparently, the other Asahi team has come up with some magic to make transform feedback and geometry shaders work. They presented it at XDC 2023, see https://www.youtube.com/live/Gg4eSAP1uc4?feature=shared&t=14049
Hi, just curious, how far approx. we are from this PR being feature complete (even if buggy).. seems to take a lot of work..
Hi, just curious, how far approx. we are from this PR being feature complete (even if buggy).. seems to take a lot of work..
Not much longer. Hopefully, it'll be feature-complete today.
How does this PR handle transform feedback with tessellation? MoltenVK advertises tessellation so this interaction must work.
What about transform feedback with triangle strips? Line strips? Triangle fans? With these topologies, it is essential to implement transform feedback at the level of primitives, rather than vertices… the outputs are meant to be “tessellated” into the underlying lines or triangles.
Primitive restart, in combination with each type? This one is especially tricky with topologies like triangle fans, which I believe MoltenVK supports nowadays.
What about indirect draws?
And the various transform feedback queries? Will those work with indirect draws? Or with primitive restart? I guess this one is gated behind a feature bit, but that bit is presumably required for Direct3D layering (ie what people expect from a feature complete implementation of transform feedback)
How does the passthrough program work if the transform feedback buffer overflows? (meant to clamp to the maximum number of primitives as required - does this implementation handle that?)
All of this functionality is required for correctness. These aren’t really “gotchas”, and now that there exists a conformant open source driver stack for Apple hardware that supports transform feedback, there’s no excuse to merge known broken code and hurt overall standards adoption.
Furthermore, there’s parallel work under way to support geometry shaders, how does that fit into this approach? Will that work? Will that correctly tessellate the output strips into lines or triangles for output into the transform feedback buffer? What about with multiple streams? Are you queries set up for multiple streams? If geometry shader support is merged, all of this is similarly needed for correctness. Again, now that there are implementations of geometry shaders passing CTS (including the transform feedback interactions) for all current hardware supported by MoltenVK, there’s no excuse to not handle that correctly too.
Are you sure this will be feature complete today?
Hi, curious if that work, is part of the present crossover 23 beta1 release which says: “We’re pleased to announce that this release also includes initial support for geometry shaders and transform feedback on macOS” Of course assuming is implemented on MoltenVK side..
I no longer work at CW, but IIRC they were investigating (at Apple's urging) the use of mesh shaders to implement that. This is for when the device doesn't support mesh shaders.
At least now we know why apple was urging mesh shaders. This approach would certainly be leveraged by hardware Mesh in M3.
It concerns me that this PR has been marked “ready”, as the technical issues raised previously do not seem to be addressed (or acknowledged). From a cursory review, the issues with this implementation approach are not gotchas, but fundamental holes large enough to drive a truck through.
Are you sure this PR is ready?
Hi, yes, this PR is correctly marked as "ready" for review. We eagerly await hearing any identification of anything not working in this PR during a review. That's exactly how the process is meant to play out. We aim to ensure transform feedback works with the full Vulkan CTS tests that cover transform feedback functionality specified in the Vulkan Spec. Things that don't work, can be worked on in the future if they exist beyond the scope of this PR effort.
If you have any constructive ideas of anything not working with this approach, that would be helpful to hear. If you can identify better approaches; that would be great to hear, all of that is correctly a part of review for candidate submissions and we welcome the chance to hear about it. It's also especially great to hear where bugs in the code are from rigorous testing.
The CMake changes will be split from this PR. It is already in a separate PR I need to update when I find the time.
@gpx1000, instead of being being coy and evasive, you could just address the issues that Alyssa already raised. Why not indicate what is and what is not within the scope of your project? It would be much more useful for everyone else to estimate if this PR is actually addressing their needs, or just providing a partial solution.
We eagerly await hearing any identification of anything not working in this PR during a review. That's exactly how the process is meant to play out
Tessellation plus transform feedback is definitely broken here, and cannot be fixed without a fundamental re-architecture of both this transform feedback implementation, and MoltenVK’s tessellation. This is not a “portability” issue, it is surely possible to implement these features correctly on top of metal, but that’s not what’s done here. I would be shocked if CTS does not test this, the GLES CTS certainly does.
Geometry shaders plus transform feedback is going to be broken unless the transform feedback code gets rewritten when geometry shaders are introduced. It concerns me that that code is also seeking review right now, given its own set of brokenness, for example, around tessellation (again, geometry, shaders, must work with tessellation, and that cannot be made to work without re-writing MoltenVK’s tessellation implementation, which is unsuitable. Unfortunately, I don’t think you can even keep the existing Tess implementation as a fast path, because the APIs require invariance between similar tessellation invocations, which would effectively require the compute/mesh tessellator to match the Metal tessellator exactly. as the behaviour of medals tessellation is undocumented and presumably varies between hardware, that would be extremely difficult for a reverse, engineered, native driver to do and a non-starter for a layered implementation that also needs to be correct on eg Intel and AMD metal drivers.)
As Hans Kristian pointed out in the associated SPIRV PR, the “shade from the XFB buffers” approach is broken, because the XFB buffers are allowed to overflow without affecting rasterization.
Transform feedback in conjunction with primitive restart is tricky and I do not see any code in either PR that would handle this interaction correctly. This is especially relevant for moltenvk because Metal always uses primitive restart.
These are just a few of the issues raised weeks ago.
The feature set currently supported by the PR is meant to be within scope for this PR. It would make sense in the future to create a new PR to address tessellation shaders. It equally would make sense to start looking at how to get geometry shader support in transform feedback after geometry support is in MoltenVK.
The feature set currently supported by the PR is meant to be within scope for this PR.
If I understand Alyssa's concern correctly, this code is not extensible and will have to be rewritten to accommodate further changes like geometry shaders, in which case, it doesn't make sense to merge a half-baked implementation now, to rework it all down the line.
If this feature is added it should be added completely and correctly.
The feature set currently supported by the PR is meant to be within scope for this PR. It would make sense in the future to create a new PR to address tessellation shaders. It equally would make sense to start looking at how to get geometry shader support in transform feedback after geometry support is in MoltenVK.
MoltenVK advertises tessellation already, which means transform feedback must work with tessellation. This is core to transform feedback, and in a PR titled “Transform feedback”, it is absolutely in scope. Indeed, this interaction is the very reason that transform feedback exists in Vulkan, instead of requiring applications to simply use storage buffers. See https://www.gfxstrand.net/faith/blog/2018/10/transform-feedback-is-terrible-so-why/
There are two acceptable options if you want this code to be merged: disable tessellation in MoltenVK, or address the interaction.
Correctness is not optional.
Conformance is not optional.
There is no portability case to be made here. Given this entire feature is emulated, there is no excuse not to emulate it properly. If you believe that it is impossible to emulate correctly on top of Metal, then MoltenVK has no business implementing it at all — after all, transform feedback is an optional legacy wart on Vulkan. As the linked article explains, new Vulkan code and even Vulkan ports of existing games should not use this functionality. So adding this support does not advance MoltenVK’s mission of aiding portability. Neither can bugs in this support get overlooked in the name of portability.
Even if you don’t care about Vulkan, DX11 requires these features, and your users will run into trouble trying to use this code with DXVK running arbitrary games.
Correctness matters.
I'm all for adding new functionality within future PRs. However, the work here is reflective of the scoped out work decisions made during development. Nothing here prevents future work, nor does anything here do more than add functionality to moltenvk that it currently does not have. For the feature set that is scoped within this PR, it is complete. Future work will gain further features.
However, the work here is reflective of the scoped out work decisions made during development.
By whom were those decisions made? Did you check with the maintainers of MoltenVK to see if that would be accepted? Worth noting that the SPIRV-Cross side of this PR has had unattended feedback since September.
If you don't address the feedback and concerns raised, they won't be merged. The SPIRV-Cross maintainers and Alyssa have clarified that the current state of these PRs is unacceptable.
@IsaacMarovitz :
By whom were those decisions made?
As an open-source project, MoltenVK is dependent on contributions, and sometimes those contributions are driven by client needs that motivate them to contribute. This particular contribution was funded by a customer that had specific platform emulation targets to meet.
@gpx1000 / @cdavis5e :
TF is an incredibly complex and messy project, and a lot of great work has gone into trying to support it with this PR. Thanks for the contribution! This looks to be a creative start!
It would make sense in the future to create a new PR to address tessellation shaders
@alyssarosenzweig makes an excellent point that the main reason for reluctantly bolting TR onto Vulkan was to support its legacy use in tessellation and geometry pipelines. So at the very least, it would help to be clarify what specifically is in scope with this PR. What is included, and what are the client's use cases for the functionality as captured here?
There are some tessellation-area changes included here. To what extent does this PR support tessellation? At all?
If a specific funding client has a need for only a very small part of TF, and this PR only covers that, then I agree with the argument that advertising general TF support will only frustrate the majority of other users who will be expecting something reasonably close to conformity, and specifically solving the common TF use case of tessellation support.
If the client has use for the current limited functionality, what we might look at is hiding advertising the extension behind an environment variable, that the client (and anyone else that can make use of the limited support) can enable to expose that support. And we'll need to include documentation that clearly indicates what those users can expect if they expose the TF extension.
Other options could be to leave this PR open as WIP, or to pull it into a branch, for further work.
We're moving this PR back into WIP state to get tess shader support from both popular demand and agreement with customer.
So Alyssa Rosenzweig seems to have added this feature to her Linux driver in the mean time. Is that enough guidance for someone to make this work? See https://www.youtube.com/live/pDsksRBLXPk