Fix buffer and heap out-of-sync in initExternalMemory()#2667
Fix buffer and heap out-of-sync in initExternalMemory()#2667billhollings merged 1 commit intoKhronosGroup:mainfrom
Conversation
If a buffer is created before a heap in MVKDeviceMemory, the buffer would be used in some places (for example `MVKDeviceMemory::map` and the heap would be used in other places (for example `MVKBuffer::getMTLBuffer()`). This leads to inconsistent memory mappings. Specifically, there are two cases when this happens: 1. When `vkAllocateMemory` is called with an imported buffer. 2. When `vkAllocateMemory` expects `HANDLE_TYPE_MTLBUFFER` to be exported. In either case, `ensureMTLBuffer()` gets called before `ensureMTLHeap()` which sets `_mtlBuffer` to a buffer not allocated from the placement heap. This change fixes this by making sure that if a placement heap is needed, we will always create it prior to allocating the buffer. In cases where the heap is not used at all (imported buffer), we will return error if the user tries to specify a `HANDLE_TYPE_MTLHEAP` for export.
|
|
billhollings
left a comment
billhollings
left a comment
There was a problem hiding this comment.
Thanks for submitting this change. While it may fix certain use cases, it also causes regression elsewhere. See my inline comments for more details.
|
|
||
| if (_image->_is2DViewOn3DImageCompatible && !dvcMem->ensureMTLHeap()) { | ||
| if (_image->_is2DViewOn3DImageCompatible && !dvcMem->getMTLHeap()) { | ||
| MVKAssert(0, "Creating a 2D view of a 3D texture currently requires a placement heap, which is not available."); |
There was a problem hiding this comment.
Hmmm...looks like this needs a bit more work for robustness. This update causes some regression.
This CTS test (among others) now triggers on this assertion, where it didn't before:
dEQP-VK.pipeline.monolithic.render_to_image.dedicated_allocation.3d.small.r8g8b8a8_unorm
There was a problem hiding this comment.
I believe that is an existing bug that's now being triggered due to the original check being incorrect:
if (_image->_is2DViewOn3DImageCompatible && !dvcMem->ensureMTLHeap()) {
ensureMTLHeap() will always return true here regardless if a heap exists. This is counter to what the assertion requires.
So there is another underlying bug that this commit exposes. However, I am not familiar with this part of the code to say confidently what it is. Do you think it makes sense if we drop the second commit and I'll file an issue on it instead?
There was a problem hiding this comment.
I went ahead and dropped the commit and filed #2668
There was a problem hiding this comment.
Thanks. This no longer crashes those CTS tests.
billhollings
left a comment
billhollings
left a comment
There was a problem hiding this comment.
Thanks for the fix. LGTM now.
3 participants
If a buffer is created before a heap in MVKDeviceMemory, the buffer would
be used in some places (for example
MVKDeviceMemory::mapand the heapwould be used in other places (for example
MVKBuffer::getMTLBuffer()).This leads to inconsistent memory mappings.
Specifically, there are two cases when this happens:
vkAllocateMemoryis called with an imported buffer.vkAllocateMemoryexpectsHANDLE_TYPE_MTLBUFFERto be exported.In either case,
ensureMTLBuffer()gets called beforeensureMTLHeap()which sets
_mtlBufferto a buffer not allocated from the placement heap.This change fixes this by making sure that if a placement heap is needed,
we will always create it prior to allocating the buffer. In cases where
the heap is not used at all (imported buffer), we will return error if the
user tries to specify a
HANDLE_TYPE_MTLHEAPfor export.