Fix synchronization issue with locking MTLArgumentEncoder.

Move mutex lock for using MTLArgumentEncoder to MVKMTLArgumentEncoder
to be tracked along with the MTLArgumentEncoder it guards.
This commit is contained in:
Bill Hollings 2021-05-14 22:17:08 -04:00
parent feb66390ca
commit 378b5d5bc9
4 changed files with 22 additions and 14 deletions

View File

@ -26,6 +26,7 @@ Released TBD
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`.

View File

@ -488,27 +488,28 @@ void MVKResourcesCommandEncoderState::bindDescriptorSet(uint32_t descSetIndex,
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.

View File

@ -34,8 +34,13 @@ class MVKResourcesCommandEncoderState;
#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 @@ struct MVKMTLArgumentEncoder {
_mtlArgumentEncoder = mtlArgEnc; // takes ownership
mtlArgumentEncoderSize = mtlArgEnc.encodedLength;
}
MVKMTLArgumentEncoder(const MVKMTLArgumentEncoder& other) {}
MVKMTLArgumentEncoder& operator=(const MVKMTLArgumentEncoder& other) { return *this; }
MVKMTLArgumentEncoder() {}
~MVKMTLArgumentEncoder() { [_mtlArgumentEncoder release]; }
private:

View File

@ -188,9 +188,6 @@ public:
/** 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);