Merge pull request #2355 from billhollings/mvkbitarray-redesign

Fix occasional GPU crash when a smaller descriptor set replaces a larger one.
KhronosGroup · Sep 28, 2024 · 2307b08 · 2307b08
2 parents b2d7ad6 + 80fb6ee
commit 2307b08
Show file tree

Hide file tree

Showing 4 changed files with 136 additions and 187 deletions.
diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm
@@ -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
@@ -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();
 		}
 	}
 }

diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.h b/MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.h
@@ -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;
@@ -322,7 +322,8 @@ class MVKDescriptorPool : public MVKVulkanAPIDeviceObject {
 	MVKDescriptorTypePool<MVKUniformTexelBufferDescriptor> _uniformTexelBufferDescriptors;
 	MVKDescriptorTypePool<MVKStorageTexelBufferDescriptor> _storageTexelBufferDescriptors;
 
-	VkDescriptorPoolCreateFlags _flags;
+	VkDescriptorPoolCreateFlags _flags = 0;
+	size_t _maxAllocDescSetCount = 0;
 };
 
 

diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.mm b/MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.mm
@@ -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;
 		}
 	}
@@ -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;
@@ -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();
 	}
@@ -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;
 }
 
@@ -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();
@@ -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;
 		}
 	});
@@ -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.");
@@ -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();
@@ -830,6 +830,7 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
 	_storageTexelBufferDescriptors.reset();
 
 	_nextMetalArgumentBufferOffset = 0;
+	_maxAllocDescSetCount = 0;
 
 	return VK_SUCCESS;
 }
@@ -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();