Skip to content

Commit

Permalink
Fix occasional GPU crash when a smaller descriptor set replaces a lar…
Browse files Browse the repository at this point in the history
…ger one.

If a descriptor set with fewer than 64 descriptors replaced one with more,
the resources it contained were not made resident to the GPU, due to
MVKBitArray thrashing incorrectly when transitioning between static and
dynamic memory allocations.

- Overhaul MVKBitArray design:
  - Never downsize memory allocation unless reset() is called.
  - No longer support static allocation for smaller sizes, to avoid thrashing
    between dynamic and static allocs when frequently resizing up and down.
  - Use realloc() instead of malloc/copy to improve performance.
  - Simplify tracking of partially and fully disabled sections.
  - Rename setXX() and clearXX() functions to enableXX() & disableXX().
  - Rename several internal functions.
  - getIndexOfFirstEnabledBit() & enumerateEnabledBits()
	no longer have an option to disable the bit.
- MVKDescriptorPool track highest descriptor set allocated,
  instead of querying MVKBitArray.
  • Loading branch information
billhollings committed Sep 27, 2024
1 parent b2d7ad6 commit 80fb6ee
Show file tree
Hide file tree
Showing 4 changed files with 136 additions and 187 deletions.
4 changes: 2 additions & 2 deletions MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ - (void)setDepthBoundsTestAMD:(BOOL)enable minDepth:(float)minDepth maxDepth:(fl
if (dsChanged) {
auto& usageDirty = _metalUsageDirtyDescriptors[descSetIndex];
usageDirty.resize(descSet->getDescriptorCount());
usageDirty.setAllBits();
usageDirty.enableAllBits();
}

// Update dynamic buffer offsets
Expand Down Expand Up @@ -717,7 +717,7 @@ - (void)setDepthBoundsTestAMD:(BOOL)enable minDepth:(float)minDepth maxDepth:(fl
MVKCommandEncoderState::markDirty();
if (_cmdEncoder->isUsingMetalArgumentBuffers()) {
for (uint32_t dsIdx = 0; dsIdx < kMVKMaxDescriptorSetCount; dsIdx++) {
_metalUsageDirtyDescriptors[dsIdx].setAllBits();
_metalUsageDirtyDescriptors[dsIdx].enableAllBits();
}
}
}
Expand Down
7 changes: 4 additions & 3 deletions MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.h
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,9 @@ class MVKDescriptorPool : public MVKVulkanAPIDeviceObject {

MVKSmallVector<MVKDescriptorSet> _descriptorSets;
MVKBitArray _descriptorSetAvailablility;
id<MTLBuffer> _metalArgumentBuffer;
NSUInteger _nextMetalArgumentBufferOffset;
MVKMTLBufferAllocator _mtlBufferAllocator;
id<MTLBuffer> _metalArgumentBuffer = nil;
NSUInteger _nextMetalArgumentBufferOffset = 0;

MVKDescriptorTypePool<MVKUniformBufferDescriptor> _uniformBufferDescriptors;
MVKDescriptorTypePool<MVKStorageBufferDescriptor> _storageBufferDescriptors;
Expand All @@ -322,7 +322,8 @@ class MVKDescriptorPool : public MVKVulkanAPIDeviceObject {
MVKDescriptorTypePool<MVKUniformTexelBufferDescriptor> _uniformTexelBufferDescriptors;
MVKDescriptorTypePool<MVKStorageTexelBufferDescriptor> _storageTexelBufferDescriptors;

VkDescriptorPoolCreateFlags _flags;
VkDescriptorPoolCreateFlags _flags = 0;
size_t _maxAllocDescSetCount = 0;
};


Expand Down
28 changes: 13 additions & 15 deletions MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.mm
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
for (uint32_t bindIdx = 0; bindIdx < bindCnt; bindIdx++) {
auto& dslBind = _bindings[bindIdx];
if (context.isResourceUsed(spvExecModels[stage], descSetIndex, dslBind.getBinding())) {
bindingUse.setBit(bindIdx);
bindingUse.enableBit(bindIdx);
descSetIsUsed = true;
}
}
Expand Down Expand Up @@ -628,9 +628,9 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
bool& dynamicAllocation,
MVKDescriptorPool* pool) {
VkResult errRslt = VK_ERROR_OUT_OF_POOL_MEMORY;
size_t availDescIdx = _availability.getIndexOfFirstSetBit();
size_t availDescIdx = _availability.getIndexOfFirstEnabledBit();
if (availDescIdx < size()) {
_availability.clearBit(availDescIdx); // Mark the descriptor as taken
_availability.disableBit(availDescIdx); // Mark the descriptor as taken
*pMVKDesc = &_descriptors[availDescIdx];
(*pMVKDesc)->reset(); // Reset descriptor before reusing.
dynamicAllocation = false;
Expand All @@ -657,7 +657,7 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
DescriptorClass* pFirstDesc = _descriptors.data();
int64_t descIdx = pDesc >= pFirstDesc ? pDesc - pFirstDesc : pFirstDesc - pDesc;
if (descIdx >= 0 && descIdx < size()) {
_availability.setBit(descIdx);
_availability.enableBit(descIdx);
} else {
mvkDesc->destroy();
}
Expand All @@ -666,13 +666,13 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
// Preallocated descriptors will be reset when they are reused
template<typename DescriptorClass>
void MVKDescriptorTypePool<DescriptorClass>::reset() {
_availability.setAllBits();
_availability.enableAllBits();
}

template<typename DescriptorClass>
size_t MVKDescriptorTypePool<DescriptorClass>::getRemainingDescriptorCount() {
size_t enabledCount = 0;
_availability.enumerateEnabledBits(false, [&](size_t bitIdx) { enabledCount++; return true; });
_availability.enumerateEnabledBits([&](size_t bitIdx) { enabledCount++; return true; });
return enabledCount;
}

Expand Down Expand Up @@ -740,7 +740,7 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
uint64_t mtlArgBuffEncAlignedSize = mvkAlignByteCount(mtlArgBuffEncSize, getMetalFeatures().mtlBufferAlignment);

size_t dsCnt = _descriptorSetAvailablility.size();
_descriptorSetAvailablility.enumerateEnabledBits(true, [&](size_t dsIdx) {
_descriptorSetAvailablility.enumerateEnabledBits([&](size_t dsIdx) {
bool isSpaceAvail = true; // If not using Metal arg buffers, space will always be available.
MVKDescriptorSet* mvkDS = &_descriptorSets[dsIdx];
NSUInteger mtlArgBuffOffset = mvkDS->getMetalArgumentBuffer().getMetalArgumentBufferOffset();
Expand Down Expand Up @@ -772,11 +772,12 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
if (rslt) {
freeDescriptorSet(mvkDS, false);
} else {
_descriptorSetAvailablility.disableBit(dsIdx);
_maxAllocDescSetCount = std::max(_maxAllocDescSetCount, dsIdx + 1);
*pVKDS = (VkDescriptorSet)mvkDS;
}
return false;
} else {
_descriptorSetAvailablility.setBit(dsIdx); // We didn't consume this one after all, so it's still available
return true;
}
});
Expand All @@ -800,7 +801,7 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
mvkDS->free(isPoolReset);
if ( !isPoolReset ) {
size_t dsIdx = mvkDS - _descriptorSets.data();
_descriptorSetAvailablility.setBit(dsIdx);
_descriptorSetAvailablility.enableBit(dsIdx);
}
} else {
reportError(VK_ERROR_INITIALIZATION_FAILED, "A descriptor set is being returned to a descriptor pool that did not allocate it.");
Expand All @@ -810,11 +811,10 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
// Free allocated descriptor sets and reset descriptor pools.
// Don't waste time freeing desc sets that were never allocated.
VkResult MVKDescriptorPool::reset(VkDescriptorPoolResetFlags flags) {
size_t dsCnt = _descriptorSetAvailablility.getLowestNeverClearedBitIndex();
for (uint32_t dsIdx = 0; dsIdx < dsCnt; dsIdx++) {
for (uint32_t dsIdx = 0; dsIdx < _maxAllocDescSetCount; dsIdx++) {
freeDescriptorSet(&_descriptorSets[dsIdx], true);
}
_descriptorSetAvailablility.setAllBits();
_descriptorSetAvailablility.enableAllBits();

_uniformBufferDescriptors.reset();
_storageBufferDescriptors.reset();
Expand All @@ -830,6 +830,7 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
_storageTexelBufferDescriptors.reset();

_nextMetalArgumentBufferOffset = 0;
_maxAllocDescSetCount = 0;

return VK_SUCCESS;
}
Expand Down Expand Up @@ -1003,9 +1004,6 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
}

void MVKDescriptorPool::initMetalArgumentBuffer(const VkDescriptorPoolCreateInfo* pCreateInfo) {
_metalArgumentBuffer = nil;
_nextMetalArgumentBufferOffset = 0;

if ( !isUsingMetalArgumentBuffers() ) { return; }

auto& mtlFeats = getMetalFeatures();
Expand Down
Loading

0 comments on commit 80fb6ee

Please sign in to comment.