-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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] Reland: add interface for submitting multiple command buffers at once. #50180
[Impeller] Reland: add interface for submitting multiple command buffers at once. #50180
Conversation
std::shared_ptr<CommandBuffer> command_buffer) const { | ||
pending_command_buffers_->command_buffers.push_back( | ||
std::move(command_buffer)); | ||
// Metal systems seem to have a limit on the number of command buffers that |
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.
This is the big change.
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.
This still seems arbitrary. Can we flush pending command buffers when we can't create more or is there a delay between the flush and when we can acquire more buffers.
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.
Or have the magic number of 10 as well as the flush and retry on allocation failure. Just having a backend specific magic number seems icky.
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.
Thinking about it more I should just make it immediately flush on metal, since we get no benefit from batching.
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.
unfortunately there is no allocation failure, instead [queue commandBufferWithDescriptor:desc];
appears to hang.
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.
removed the limit, now we Record -> Flush.
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.
Oh well. Not having the arbitrary limit looks cleaner though.
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.
Yeah but it doesn't really matter for metal, I don't think we'll have a way to batch submit with that API ever.
Did you mean to remove the autosub label? |
…mmand buffers at once. (flutter/engine#50180)
…142564) flutter/engine@d20ed24...e6e1d6b 2024-01-30 skia-flutter-autoroll@skia.org Roll Dart SDK from 734aae9604e8 to 488e33cd39de (1 revision) (flutter/engine#50185) 2024-01-30 jonahwilliams@google.com [Impeller] Reland: add interface for submitting multiple command buffers at once. (flutter/engine#50180) 2024-01-30 iinozemtsev@google.com Support running sound null safe kernels from flutter_jit_runner (flutter/engine#50002) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC matanl@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Reland of: #50139
Metal does not seem to like it when we collect 50+ command buffers at once. Adjust the aiks context logic to regularly flush the cmd buffers.