[Impeller] Join Vulkan queue submissions. #49870
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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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. |
| @@ -55,13 +55,24 @@ bool CommandBufferVK::OnSubmitCommands(CompletionCallback callback) { | |||
| if (!encoder_) { | |||
| encoder_ = encoder_factory_->Create(); | |||
| } | |||
| if (!callback) { | |||
| return encoder_->Submit(); | |||
| auto context = context_.lock(); | |||
There was a problem hiding this comment.
The problem with this approach is that I don't really know if this is being created during a frame workload or not. If this is an image decoding command buffer, or something being used for a layout transition on Vulkan, then it won't get submitted until we render a frame.
Maybe that is OK, so we won't technically queue up iO thread work until its consumed by a frame, but so what?
There was a problem hiding this comment.
Well, besides the problem with synchronization I guess.
|
I'm not really sure that I like this approach. I think WebGPU has a reasonable API for this, where there is an abstract queue object and command buffers must be submitted to it (rather than having a magic method that tracks context state to self submit). With this sort of API, entity pass could collect the command buffers in order and then submit all at once. That would require some breaks to the API to add some sort of per-frame context, rather than hiding this information in the context classes (which should probably be less stateful). |
jonahwilliams
changed the title
|
Doing #50139 instead. |
…nce. (#50139) The Impeller Vulkan backend benefits from batching submission to the vk graphics queue. Managing this automatically is non-trivial and adds surprising/fragile thread based behavior, see: #49870 Instead, introduce an impeller::CommandQueue object that command buffers must be submitted to in lieu of CommandBuffer->Submit, which has been made private. TLDR old ```c++ buffer->Submit(); ``` new ```c++ context.GetQueue()->Submit({buffer}); ``` The Metal and GLES implementations internally just call the private CommandBuffer->Submit, though there may be future opportunities to simplify here. The Vulkan implementation is where the meat is. Aiks takes advantage of this by storing all command buffers on the aiks context while rendering a frame, and then performing one submit in aiks_context render. I don't think this will introduce any thread safety problems, as we don't guarantee much about aiks context - nor do we use it in a multithreaded context as far as I know. Other tasks such as image upload still just directly submit their command buffers via the queue. Fixes flutter/flutter#141123
…fers at once." (#50174) Reverts #50139 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: The Impeller Vulkan backend benefits from batching submission to the vk graphics queue. Managing this automatically is non-trivial and adds surprising/fragile thread based behavior, see: #49870 Instead, introduce an impeller::CommandQueue object that command buffers must be submitted to in lieu of CommandBuffer->Submit, which has been made private. TLDR old ```c++ buffer->Submit(); ``` new ```c++ context.GetQueue()->Submit({buffer}); ``` The Metal and GLES implementations internally just call the private CommandBuffer->Submit, though there may be future opportunities to simplify here. The Vulkan implementation is where the meat is. Aiks takes advantage of this by storing all command buffers on the aiks context while rendering a frame, and then performing one submit in aiks_context render. I don't think this will introduce any thread safety problems, as we don't guarantee much about aiks context - nor do we use it in a multithreaded context as far as I know. Other tasks such as image upload still just directly submit their command buffers via the queue. Fixes flutter/flutter#141123
Submitting to the graphics queue can be quite expensive. At least on Adreno devices, this queue submission needs to acquire a lock which means that it can block for longer than expected periods of time. Command buffers can be submitted together, with specified order, so collect the encoders and submit at once before presenting the swapchain.
Impeller should be able to submit command buffers from any thread, and some command buffer submissions (such as image upload) won't end up as part of a swapchain presentation that could otherwise perform the flush. Therefore, this adds some tracking of the thread that the command buffer is created on. We implicitly treat command buffers created as part of the raster thread as part of a group that can be deferred for submission at once.
I don't really love this approach, but sharing for discussion.