Fix Random Crashes when Using Pool with Freeable Descriptor Sets #2388
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changes made so that `_nextMetalArgumentBufferOffset` is updated after
`mvkDS->allocate()` is called and called each time the highest index
set is written
Fixes 2 cases where _nextMetalArgumentBufferOffset would become out of date:
- if the offset was set, and then `mvkDS->allocate()` fails, the reserved
space wouldn't actually be in use
- if the descriptor set in the highest index is freed, then re-used (in
a `CREATE_FREE_DESCRIPTOR_SET_BIT` pool), the new set may have a larger arg
buffer size than the original. It would still be allowed to use that slot
in this case (since there is no argbuffer offset in `nextDSIdx` to limit it),
but would not update `_nextMetalArgumentBufferOffset` (since that only
happens when adding a new set at the end of the list). This results in the
argument buffer region overlapping with the next set to use
`_nextMetalArgumentBufferOffset` as its base offset
Quick fix for the issue, just checking if the returned bit is enabled, and skipping the iteration of the loop if it is not Avoids issue where sometimes `getIndexOfFirstEnabledBit()` will return bits that are not set -- a proper fix in that function should still be implemented
|
Replaced by PR #2392. |
|
Yes PR #2392 fixes the issue Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some small changes to fix crashes and random descriptor set issues I was running into when using a pool with
FREE_DESCRIPTOR_SETenabledFirst issue was caused by
_nextMetalArgumentBufferOffsetin the pool becoming out-of-date if the allocated set in the highest index is freed and re-used. Argument buffers of any size would be allowed to use that slot, but the offset wasn't adjusted, resulting in some argument buffer ranges overlappingEventually crashing with the error:
[mvk-error] VK_ERROR_OUT_OF_DEVICE_MEMORY: Lost VkDevice after MTLCommandBuffer "vkQueueSubmit MTLCommandBuffer on Queue 0-0 (0x406e75600)" execution failed (code 3): Caused GPU Address Fault Error (0000000b:kIOGPUCommandBufferCallbackErrorPageFault)Second issue is caused by a bug in
enumerateEnabledBits()where it is enumerating bits that were disabled.Specifically, in the case where
startIndexis higher than the first enabled bit, but the next enabled bit is in a different section thanstartIndex, it looks like it just return the first index in the next section, instead of the next enabled bit.I just added a quick fix so that the enumerate func checks whether the returned bit is enabled before calling the functor. More changes to the MVKBitArray functions might be called for, but I hope this is helpful 😄