Fix synchronization issue with locking MTLArgumentEncoder.
Move mutex lock for using MTLArgumentEncoder to MVKMTLArgumentEncoder
to be tracked along with the MTLArgumentEncoder it guards.
diff --git a/Docs/Whats_New.md b/Docs/Whats_New.md
index 8a2ffb8..6f87c16 100644
--- a/Docs/Whats_New.md
+++ b/Docs/Whats_New.md
@@ -26,6 +26,7 @@
to find a cached shader, by only considering resources from the current shader stage.
- Rename `kMVKShaderStageMax` to `kMVKShaderStageCount`.
- Fix crash when requesting `MTLCommandBuffer` logs in runtime debug mode on older OS versions.
+- Fix synchronization issue with locking `MTLArgumentEncoder` for Metal Argument Buffers.
- Protect against crash when retrieving `MTLTexture` when `VkImage` has no `VkDeviceMemory` bound.
- Adjust some `VkPhysicalDeviceLimits` values for Vulkan and Metal compliance.
- Fix internal reference from `SPIRV_CROSS_NAMESPACE_OVERRIDE` to `SPIRV_CROSS_NAMESPACE`.
diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm
index 1ff5a9c..96ea826 100644
--- a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm
+++ b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm
@@ -488,27 +488,28 @@
void MVKResourcesCommandEncoderState::encodeMetalArgumentBuffer(MVKShaderStage stage) {
if ( !_cmdEncoder->isUsingMetalArgumentBuffers() ) { return; }
- // The Metal arg encoder can only write to one arg buffer at a time (it holds the arg buffer),
- // so we need to lock out other access to it while we are writing to it.
- MVKPipeline* pipeline = getPipeline();
- lock_guard<mutex> lock(pipeline->_mtlArgumentEncodingLock);
+ bool useDescSetArgBuff = _cmdEncoder->isUsingDescriptorSetMetalArgumentBuffers();
+ MVKPipeline* pipeline = getPipeline();
uint32_t dsCnt = pipeline->getDescriptorSetCount();
for (uint32_t dsIdx = 0; dsIdx < dsCnt; dsIdx++) {
auto* descSet = _boundDescriptorSets[dsIdx];
if ( !descSet ) { continue; }
- id<MTLArgumentEncoder> mtlArgEncoder = nil;
+ auto* dsLayout = descSet->getLayout();
+
+ // The Metal arg encoder can only write to one arg buffer at a time (it holds the arg buffer),
+ // so we need to lock out other access to it while we are writing to it.
+ auto& mvkArgEnc = useDescSetArgBuff ? dsLayout->getMTLArgumentEncoder() : pipeline->getMTLArgumentEncoder(dsIdx, stage);
+ lock_guard<mutex> lock(mvkArgEnc.mtlArgumentEncodingLock);
+
id<MTLBuffer> mtlArgBuffer = nil;
NSUInteger metalArgBufferOffset = 0;
-
- auto* dsLayout = descSet->getLayout();
- if (dsLayout->isUsingDescriptorSetMetalArgumentBuffers()) {
- mtlArgEncoder = dsLayout->getMTLArgumentEncoder().getMTLArgumentEncoder();
+ id<MTLArgumentEncoder> mtlArgEncoder = mvkArgEnc.getMTLArgumentEncoder();
+ if (useDescSetArgBuff) {
mtlArgBuffer = descSet->getMetalArgumentBuffer();
metalArgBufferOffset = descSet->getMetalArgumentBufferOffset();
} else {
- mtlArgEncoder = pipeline->getMTLArgumentEncoder(dsIdx, stage).getMTLArgumentEncoder();
// TODO: Source a different arg buffer & offset for each pipeline-stage/desccriptors set
// Also need to only encode the descriptors that are referenced in the shader.
// MVKMTLArgumentEncoder could include an MVKBitArray to track that and have it checked below.
diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.h b/MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.h
index fdced31..1e9dddc 100644
--- a/MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.h
+++ b/MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.h
@@ -34,8 +34,13 @@
#pragma mark -
#pragma mark MVKDescriptorSetLayout
-/** Holds and manages the lifecycle of a MTLArgumentEncoder. The encoder can only be set once. */
+/**
+ * Holds and manages the lifecycle of a MTLArgumentEncoder. The encoder can
+ * only be set once, and copying this object results in an uninitialized
+ * empty object, since mutex and MTLArgumentEncoder can/should not be copied.
+ */
struct MVKMTLArgumentEncoder {
+ std::mutex mtlArgumentEncodingLock;
NSUInteger mtlArgumentEncoderSize = 0;
id<MTLArgumentEncoder> getMTLArgumentEncoder() { return _mtlArgumentEncoder; }
@@ -44,6 +49,10 @@
_mtlArgumentEncoder = mtlArgEnc; // takes ownership
mtlArgumentEncoderSize = mtlArgEnc.encodedLength;
}
+
+ MVKMTLArgumentEncoder(const MVKMTLArgumentEncoder& other) {}
+ MVKMTLArgumentEncoder& operator=(const MVKMTLArgumentEncoder& other) { return *this; }
+ MVKMTLArgumentEncoder() {}
~MVKMTLArgumentEncoder() { [_mtlArgumentEncoder release]; }
private:
diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h
index 0f69331..f262b82 100644
--- a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h
+++ b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h
@@ -188,9 +188,6 @@
/** Returns the number of descriptor sets in this pipeline layout. */
uint32_t getDescriptorSetCount() { return _descriptorSetCount; }
- /** A mutex lock to protect access to the Metal argument encoders. */
- std::mutex _mtlArgumentEncodingLock;
-
/** Constructs an instance for the device. layout, and parent (which may be NULL). */
MVKPipeline(MVKDevice* device, MVKPipelineCache* pipelineCache, MVKPipelineLayout* layout, MVKPipeline* parent);
