From ef1990306db85658ffde6ab901478fc24f63eba0 Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Sun, 21 Mar 2021 21:53:16 -0400 Subject: [PATCH 1/2] Enhancements and fixes to temporary MTLBuffer allocation. Check allocations are larger than MTLBuffer alignment to avoid Metal errors. Align returning safely with allocating safely, and change callers to use MVKMTLBufferAllocation::returnToPool() globally, to ensure this match. Fix use of const on returnToPool(); Move MVKCommandEncoder::copyToTempMTLBufferAllocation() and support setting private and dedicated flags. Align constructor parameter order on MVKMTLBufferAllocationPool with that of MVKMTLBufferAllocator. Move subclass overrides of MVKObjectPool::newObject() to protected access. --- MoltenVK/MoltenVK/Commands/MVKCommand.h | 6 +++--- MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h | 4 +++- .../MoltenVK/Commands/MVKCommandBuffer.mm | 13 ++++-------- .../Commands/MVKMTLBufferAllocation.h | 20 +++++++++++-------- .../Commands/MVKMTLBufferAllocation.mm | 18 ++++++++++------- MoltenVK/MoltenVK/GPUObjects/MVKDevice.h | 5 ++--- 6 files changed, 35 insertions(+), 31 deletions(-) diff --git a/MoltenVK/MoltenVK/Commands/MVKCommand.h b/MoltenVK/MoltenVK/Commands/MVKCommand.h index 04e42a7b..685625ba 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommand.h +++ b/MoltenVK/MoltenVK/Commands/MVKCommand.h @@ -38,11 +38,11 @@ public: /** Returns the Vulkan API opaque object controlling this object. */ MVKVulkanAPIObject* getVulkanAPIObject() override { return nullptr; } - /** Returns a new command instance. */ - T* newObject() override { return new T(); } - MVKCommandTypePool(bool isPooling = true) : MVKObjectPool(isPooling) {} +protected: + T* newObject() override { return new T(); } + }; diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h index 5b3d1b61..f2f40dd7 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h +++ b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h @@ -372,6 +372,9 @@ public: /** Get a temporary MTLBuffer that will be returned to a pool after the command buffer is finished. */ const MVKMTLBufferAllocation* getTempMTLBuffer(NSUInteger length, bool isPrivate = false, bool isDedicated = false); + /** Copy the bytes to a temporary MTLBuffer that will be returned to a pool after the command buffer is finished. */ + const MVKMTLBufferAllocation* copyToTempMTLBufferAllocation(const void* bytes, NSUInteger length, bool isPrivate = false, bool isDedicated = false); + /** Returns the command encoding pool. */ MVKCommandEncodingPool* getCommandEncodingPool(); @@ -470,7 +473,6 @@ protected: void finishQueries(); void setSubpass(MVKCommand* passCmd, VkSubpassContents subpassContents, uint32_t subpassIndex); void clearRenderArea(); - const MVKMTLBufferAllocation* copyToTempMTLBufferAllocation(const void* bytes, NSUInteger length); NSString* getMTLRenderCommandEncoderName(); VkSubpassContents _subpassContents; diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm index c90647bb..8396ea33 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm +++ b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm @@ -647,15 +647,10 @@ void MVKCommandEncoder::setComputeBytes(id mtlEncoder, } } +// Return the MTLBuffer allocation to the pool once the command buffer is done with it const MVKMTLBufferAllocation* MVKCommandEncoder::getTempMTLBuffer(NSUInteger length, bool isPrivate, bool isDedicated) { const MVKMTLBufferAllocation* mtlBuffAlloc = getCommandEncodingPool()->acquireMTLBufferAllocation(length, isPrivate, isDedicated); - MVKMTLBufferAllocationPool* pool = mtlBuffAlloc->getPool(); - - // Return the MTLBuffer allocation to the pool once the command buffer is done with it - [_mtlCmdBuffer addCompletedHandler: ^(id mcb) { - pool->returnObjectSafely((MVKMTLBufferAllocation*)mtlBuffAlloc); - }]; - + [_mtlCmdBuffer addCompletedHandler: ^(id mcb) { mtlBuffAlloc->returnToPool(); }]; return mtlBuffAlloc; } @@ -664,8 +659,8 @@ MVKCommandEncodingPool* MVKCommandEncoder::getCommandEncodingPool() { } // Copies the specified bytes into a temporary allocation within a pooled MTLBuffer, and returns the MTLBuffer allocation. -const MVKMTLBufferAllocation* MVKCommandEncoder::copyToTempMTLBufferAllocation(const void* bytes, NSUInteger length) { - const MVKMTLBufferAllocation* mtlBuffAlloc = getTempMTLBuffer(length); +const MVKMTLBufferAllocation* MVKCommandEncoder::copyToTempMTLBufferAllocation(const void* bytes, NSUInteger length, bool isPrivate, bool isDedicated) { + const MVKMTLBufferAllocation* mtlBuffAlloc = getTempMTLBuffer(length, isPrivate, isDedicated); void* pBuffData = mtlBuffAlloc->getContents(); mlock(pBuffData, length); memcpy(pBuffData, bytes, length); diff --git a/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.h b/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.h index c9bc9449..e2232255 100644 --- a/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.h +++ b/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.h @@ -51,7 +51,7 @@ public: MVKMTLBufferAllocationPool* getPool() const { return _pool; } /** Returns this object back to the pool that created it. */ - void returnToPool(); + void returnToPool() const; /** Constructs this instance with the specified pool as its origin. */ MVKMTLBufferAllocation(MVKMTLBufferAllocationPool* pool, @@ -82,16 +82,18 @@ public: /** Returns the Vulkan API opaque object controlling this object. */ MVKVulkanAPIObject* getVulkanAPIObject() override { return _device->getVulkanAPIObject(); }; - /** Returns a new MVKMTLBufferAllocation instance. */ - MVKMTLBufferAllocation* newObject() override; - /** Configures this instance to dispense MVKMTLBufferAllocation instances of the specified size. */ - MVKMTLBufferAllocationPool(MVKDevice* device, NSUInteger allocationLength, MTLStorageMode mtlStorageMode, bool isDedicated); + MVKMTLBufferAllocationPool(MVKDevice* device, NSUInteger allocationLength, bool makeThreadSafe, + bool isDedicated, MTLStorageMode mtlStorageMode); ~MVKMTLBufferAllocationPool() override; protected: - uint32_t calcMTLBufferAllocationCount(); + friend class MVKMTLBufferAllocation; + + MVKMTLBufferAllocation* newObject() override; + void returnAllocation(MVKMTLBufferAllocation* ba) { _isThreadSafe ? returnObjectSafely(ba) : returnObject(ba); } + uint32_t calcMTLBufferAllocationCount(); void addMTLBuffer(); NSUInteger _nextOffset; @@ -100,6 +102,7 @@ protected: MTLStorageMode _mtlStorageMode; MVKSmallVector, 64> _mtlBuffers; MVKDevice* _device; + bool _isThreadSafe; }; @@ -138,14 +141,15 @@ public: * next power-of-two value that is at least as big as the specified maximum size. * If makeThreadSafe is true, a lock will be applied when an allocation is acquired. */ - MVKMTLBufferAllocator(MVKDevice* device, NSUInteger maxRegionLength, bool makeThreadSafe = false, bool isDedicated = false, MTLStorageMode mtlStorageMode = MTLStorageModeShared); + MVKMTLBufferAllocator(MVKDevice* device, NSUInteger maxRegionLength, bool makeThreadSafe = false, + bool isDedicated = false, MTLStorageMode mtlStorageMode = MTLStorageModeShared); ~MVKMTLBufferAllocator() override; protected: MVKSmallVector _regionPools; NSUInteger _maxAllocationLength; - bool _makeThreadSafe; + bool _isThreadSafe; }; diff --git a/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm b/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm index 91fb2b1c..ff93739b 100644 --- a/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm +++ b/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm @@ -24,7 +24,7 @@ MVKVulkanAPIObject* MVKMTLBufferAllocation::getVulkanAPIObject() { return _pool->getVulkanAPIObject(); }; -void MVKMTLBufferAllocation::returnToPool() { _pool->returnObjectSafely(this); } +void MVKMTLBufferAllocation::returnToPool() const { _pool->returnAllocation((MVKMTLBufferAllocation*)this); } #pragma mark - @@ -50,10 +50,11 @@ void MVKMTLBufferAllocationPool::addMTLBuffer() { } -MVKMTLBufferAllocationPool::MVKMTLBufferAllocationPool(MVKDevice* device, NSUInteger allocationLength, MTLStorageMode mtlStorageMode, bool isDedicated) - : MVKObjectPool(true) { +MVKMTLBufferAllocationPool::MVKMTLBufferAllocationPool(MVKDevice* device, NSUInteger allocationLength, bool makeThreadSafe, + bool isDedicated, MTLStorageMode mtlStorageMode) : MVKObjectPool(true) { _device = device; _allocationLength = allocationLength; + _isThreadSafe = makeThreadSafe; _mtlBufferLength = _allocationLength * (isDedicated ? 1 : calcMTLBufferAllocationCount()); _mtlStorageMode = mtlStorageMode; _nextOffset = _mtlBufferLength; // Force a MTLBuffer to be added on first access @@ -80,10 +81,13 @@ MVKMTLBufferAllocationPool::~MVKMTLBufferAllocationPool() { const MVKMTLBufferAllocation* MVKMTLBufferAllocator::acquireMTLBufferRegion(NSUInteger length) { MVKAssert(length <= _maxAllocationLength, "This MVKMTLBufferAllocator has been configured to dispense MVKMTLBufferRegions no larger than %lu bytes.", (unsigned long)_maxAllocationLength); + // Can't allocate a segment smaller than the minimum MTLBuffer alignment. + length = std::max(length, _device->_pMetalFeatures->mtlBufferAlignment); + // Convert max length to the next power-of-two exponent to use as a lookup NSUInteger p2Exp = mvkPowerOfTwoExponent(length); MVKMTLBufferAllocationPool* pRP = _regionPools[p2Exp]; - const MVKMTLBufferAllocation* region = _makeThreadSafe ? pRP->acquireObjectSafely() : pRP->acquireObject(); + const MVKMTLBufferAllocation* region = _isThreadSafe ? pRP->acquireObjectSafely() : pRP->acquireObject(); if (region) { [region->_mtlBuffer setPurgeableState: MTLPurgeableStateVolatile]; } @@ -91,8 +95,8 @@ const MVKMTLBufferAllocation* MVKMTLBufferAllocator::acquireMTLBufferRegion(NSUI } MVKMTLBufferAllocator::MVKMTLBufferAllocator(MVKDevice* device, NSUInteger maxRegionLength, bool makeThreadSafe, bool isDedicated, MTLStorageMode mtlStorageMode) : MVKBaseDeviceObject(device) { - _maxAllocationLength = maxRegionLength; - _makeThreadSafe = makeThreadSafe; + _maxAllocationLength = std::max(maxRegionLength, _device->_pMetalFeatures->mtlBufferAlignment); + _isThreadSafe = makeThreadSafe; // Convert max length to the next power-of-two exponent NSUInteger maxP2Exp = mvkPowerOfTwoExponent(_maxAllocationLength); @@ -101,7 +105,7 @@ MVKMTLBufferAllocator::MVKMTLBufferAllocator(MVKDevice* device, NSUInteger maxRe _regionPools.reserve(maxP2Exp + 1); NSUInteger allocLen = 1; for (uint32_t p2Exp = 0; p2Exp <= maxP2Exp; p2Exp++) { - _regionPools.push_back(new MVKMTLBufferAllocationPool(device, allocLen, mtlStorageMode, isDedicated)); + _regionPools.push_back(new MVKMTLBufferAllocationPool(device, allocLen, makeThreadSafe, isDedicated, mtlStorageMode)); allocLen <<= 1; } } diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKDevice.h b/MoltenVK/MoltenVK/GPUObjects/MVKDevice.h index b8b6dd34..adb1fbd0 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKDevice.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKDevice.h @@ -937,9 +937,6 @@ public: /** Returns the Vulkan API opaque object controlling this object. */ MVKVulkanAPIObject* getVulkanAPIObject() override { return _device; }; - /** Returns a new instance. */ - T* newObject() override { return new T(_device); } - /** * Configures this instance for the device, and either use pooling, or not, depending * on the value of isPooling, which defaults to true if not indicated explicitly. @@ -947,6 +944,8 @@ public: MVKDeviceObjectPool(MVKDevice* device, bool isPooling = true) : MVKObjectPool(isPooling), _device(device) {} protected: + T* newObject() override { return new T(_device); } + MVKDevice* _device; }; From 55a2fe6467c9a3d4e89317b6b80515f1a9c812f8 Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Mon, 22 Mar 2021 07:51:07 -0400 Subject: [PATCH 2/2] Remove private flag from MVKCommandEncoder::copyToTempMTLBufferAllocation(). MVKMTLBufferAllocationPool subclass from MVKDeviceTrackingMixin. MVKDeviceObjectPool subclass from MVKDeviceTrackingMixin. --- MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h | 2 +- MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm | 4 ++-- MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.h | 4 ++-- MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm | 6 ++++-- MoltenVK/MoltenVK/GPUObjects/MVKDevice.h | 8 ++++---- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h index f2f40dd7..43b45a58 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h +++ b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.h @@ -373,7 +373,7 @@ public: const MVKMTLBufferAllocation* getTempMTLBuffer(NSUInteger length, bool isPrivate = false, bool isDedicated = false); /** Copy the bytes to a temporary MTLBuffer that will be returned to a pool after the command buffer is finished. */ - const MVKMTLBufferAllocation* copyToTempMTLBufferAllocation(const void* bytes, NSUInteger length, bool isPrivate = false, bool isDedicated = false); + const MVKMTLBufferAllocation* copyToTempMTLBufferAllocation(const void* bytes, NSUInteger length, bool isDedicated = false); /** Returns the command encoding pool. */ MVKCommandEncodingPool* getCommandEncodingPool(); diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm index 8396ea33..b7866f72 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm +++ b/MoltenVK/MoltenVK/Commands/MVKCommandBuffer.mm @@ -659,8 +659,8 @@ MVKCommandEncodingPool* MVKCommandEncoder::getCommandEncodingPool() { } // Copies the specified bytes into a temporary allocation within a pooled MTLBuffer, and returns the MTLBuffer allocation. -const MVKMTLBufferAllocation* MVKCommandEncoder::copyToTempMTLBufferAllocation(const void* bytes, NSUInteger length, bool isPrivate, bool isDedicated) { - const MVKMTLBufferAllocation* mtlBuffAlloc = getTempMTLBuffer(length, isPrivate, isDedicated); +const MVKMTLBufferAllocation* MVKCommandEncoder::copyToTempMTLBufferAllocation(const void* bytes, NSUInteger length, bool isDedicated) { + const MVKMTLBufferAllocation* mtlBuffAlloc = getTempMTLBuffer(length, false, isDedicated); void* pBuffData = mtlBuffAlloc->getContents(); mlock(pBuffData, length); memcpy(pBuffData, bytes, length); diff --git a/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.h b/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.h index e2232255..7488305d 100644 --- a/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.h +++ b/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.h @@ -75,7 +75,7 @@ protected: * To return a MVKMTLBufferAllocation retrieved from this pool, back to this pool, * call the returnToPool() function on the MVKMTLBufferAllocation instance. */ -class MVKMTLBufferAllocationPool : public MVKObjectPool { +class MVKMTLBufferAllocationPool : public MVKObjectPool, public MVKDeviceTrackingMixin { public: @@ -91,6 +91,7 @@ public: protected: friend class MVKMTLBufferAllocation; + MVKBaseObject* getBaseObject() override { return this; }; MVKMTLBufferAllocation* newObject() override; void returnAllocation(MVKMTLBufferAllocation* ba) { _isThreadSafe ? returnObjectSafely(ba) : returnObject(ba); } uint32_t calcMTLBufferAllocationCount(); @@ -101,7 +102,6 @@ protected: NSUInteger _mtlBufferLength; MTLStorageMode _mtlStorageMode; MVKSmallVector, 64> _mtlBuffers; - MVKDevice* _device; bool _isThreadSafe; }; diff --git a/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm b/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm index ff93739b..2aabbf9f 100644 --- a/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm +++ b/MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm @@ -51,8 +51,10 @@ void MVKMTLBufferAllocationPool::addMTLBuffer() { MVKMTLBufferAllocationPool::MVKMTLBufferAllocationPool(MVKDevice* device, NSUInteger allocationLength, bool makeThreadSafe, - bool isDedicated, MTLStorageMode mtlStorageMode) : MVKObjectPool(true) { - _device = device; + bool isDedicated, MTLStorageMode mtlStorageMode) : + MVKObjectPool(true), + MVKDeviceTrackingMixin(device) { + _allocationLength = allocationLength; _isThreadSafe = makeThreadSafe; _mtlBufferLength = _allocationLength * (isDedicated ? 1 : calcMTLBufferAllocationCount()); diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKDevice.h b/MoltenVK/MoltenVK/GPUObjects/MVKDevice.h index adb1fbd0..60cc8e7a 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKDevice.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKDevice.h @@ -892,6 +892,7 @@ public: protected: MVKBaseObject* getBaseObject() override { return this; }; + }; @@ -929,11 +930,10 @@ protected: /** Manages a pool of instances of a particular object type that requires an MVKDevice during construction. */ template -class MVKDeviceObjectPool : public MVKObjectPool { +class MVKDeviceObjectPool : public MVKObjectPool, public MVKDeviceTrackingMixin { public: - /** Returns the Vulkan API opaque object controlling this object. */ MVKVulkanAPIObject* getVulkanAPIObject() override { return _device; }; @@ -941,12 +941,12 @@ public: * Configures this instance for the device, and either use pooling, or not, depending * on the value of isPooling, which defaults to true if not indicated explicitly. */ - MVKDeviceObjectPool(MVKDevice* device, bool isPooling = true) : MVKObjectPool(isPooling), _device(device) {} + MVKDeviceObjectPool(MVKDevice* device, bool isPooling = true) : MVKObjectPool(isPooling), MVKDeviceTrackingMixin(device) {} protected: T* newObject() override { return new T(_device); } + MVKBaseObject* getBaseObject() override { return this; }; - MVKDevice* _device; };