-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Impeller] Patch up sundry issues in the Vulkan backend. #39905
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to keep iterating on and get things moving.
If Kaushik's comments aren't addressed in this patch we should file bugs to fix them, but most of them seem small.
2157935
to
1a00401
Compare
} | ||
} | ||
|
||
bool SetContents(const TextureDescriptor& desc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @iskakaushik for a closer look.
Kaushik already did most of the great work. This patch gets us to a point where all the playgrounds tests pass and the common performance optimizations are in place. All known non-compute Impeller features should be implemented. There are iOS only performance optimizations that haven’t been opted into because these are behind define guards. These need to be generalized. Without that, the performance of this backend will lag behind that of the Metal backend but only because it is not an apples to apples comparison. The general rubric was to keep the code and concepts as similar to the Metal backend as possible. The list of updates, all to the Vulkan Impeller backend: * MSAA is wired up. * Depth and stencil attachments and pipeline states are wired up. * Got rid of Vulkan specific hacks in the inline pass context that were preventing clips from working. * Storage modes for both device buffer and texture allocation are respected. This includes optimal usage of tile memory for device transient attachments. * Host coherent memory for textures is no longer a requirement with explicit mapping management and write flushes. * Texture uploads should be optimized for the UMA case without needing a staging buffer. That entire pipeline has been reworked. * Textures track their current layout and ensure they get to the right layout based on usage without redundant transitions. * Cube textures are now supported. * Mipmapping has been reworked to correct image layout errors. With the new texture layout transition management, blit passes should be a whole lot easier to read and reason about. * Allocator allocations are named using Vulkan debug utilities and the allocator (VMA). Comes in handy when chasing leaks. All of which are chased down. * Left some handy utilities in there to debug resource leaks after context shutdown. These are validation errors. * Debug groups are pushed around render pass command encoding as well as the entire pass contents. This mimics the behavior of the Metal backend and it should be easy to map traces using both backends in Xcode and RenderDoc. * Command buffer submission allows for tracking the life cycles of all resources referenced in the command stream and ensuring that the objects stay alive till past the submission of the command buffer. All use-after-free issues due to this class of issue have been chased down. * Command pools are now context global instead of being created per pass. * Descriptor pool are now context global instead of being created per pass. Individual descriptor types are now reference counted and returned to the pool when not needed. The pool is still fixed size though (and hence relatively large). * All instances of global waitIdle on the device are removed during normal operation. The only times a waitIdle may happen is during swapchain recreation and context destruction. * Swapchain adapt to them going out of date with the underlying surface and seamlessly resize as necessary on the next drawable acquisition. * Playgrounds are resizable. * Pipeline front face and cull modes are respected. * Clears for all attachments are respected. * Maximum textures sizes supported on the device are respected instead of hardcoding the minimum Vulkan requirement. Fixes flutter/flutter#112388 Fixes flutter/flutter#112648 Fixes flutter/flutter#112647 and a few other issues...
a198350
to
d3d2a1c
Compare
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
Kaushik already did most of the great work. This patch gets us to a point where
all the playgrounds tests pass and the common performance optimizations are in
place. All known non-compute Impeller features should be implemented. There are
iOS only performance optimizations that haven’t been opted into because these
are behind define guards. These need to be generalized. Without that, the
performance of this backend will lag behind that of the Metal backend but only
because it is not an apples to apples comparison.
The general rubric was to keep the code and concepts as similar to the Metal
backend as possible.
The list of updates, all to the Vulkan Impeller backend:
preventing clips from working.
This includes optimal usage of tile memory for device transient attachments.
mapping management and write flushes.
buffer. That entire pipeline has been reworked.
based on usage without redundant transitions.
texture layout transition management, blit passes should be a whole lot easier
to read and reason about.
(VMA). Comes in handy when chasing leaks. All of which are chased down.
shutdown. These are validation errors.
entire pass contents. This mimics the behavior of the Metal backend and it
should be easy to map traces using both backends in Xcode and RenderDoc.
referenced in the command stream and ensuring that the objects stay alive till
past the submission of the command buffer. All use-after-free issues due to
this class of issue have been chased down.
Individual descriptor types are now reference counted and returned to the pool
when not needed. The pool is still fixed size though (and hence relatively
large).
operation. The only times a waitIdle may happen is during swapchain recreation
and context destruction.
seamlessly resize as necessary on the next drawable acquisition.
hardcoding the minimum Vulkan requirement.
Fixes flutter/flutter#112388
Fixes flutter/flutter#112648
Fixes flutter/flutter#112647 and a few other issues...