From 08d9fa8ea2e100db2cb36594e82a7a20c0a96310 Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Thu, 25 Feb 2021 16:37:06 -0500 Subject: [PATCH 1/3] Ensure queue submission message logging occurs before submission object is destroyed. Fixes to MVKQueueCommandBufferSubmission: - Consolidate MTLCommandBuffer completion callbacks, and ensure all other activities, including logging, which references the object, occurs before finish() is called. - Remove separate flag that indicates whether to track MTLCommandBuffer completion times. - Localize Vulkan error reporting to the queue instead of the device. --- MoltenVK/MoltenVK/GPUObjects/MVKQueue.h | 1 - MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm | 35 ++++++++++-------------- 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.h b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.h index b72edd45..512c17ad 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.h @@ -181,7 +181,6 @@ protected: MVKQueue* _queue; MVKSmallVector> _waitSemaphores; - bool _trackPerformance; }; diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm index e80be13a..e295b78c 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm @@ -244,8 +244,6 @@ MVKQueueSubmission::MVKQueueSubmission(MVKQueue* queue, uint32_t waitSemaphoreCount, const VkSemaphore* pWaitSemaphores) { _queue = queue; - _trackPerformance = mvkGetMVKConfiguration()->performanceTracking; - _waitSemaphores.reserve(waitSemaphoreCount); for (uint32_t i = 0; i < waitSemaphoreCount; i++) { _waitSemaphores.push_back(make_pair((MVKSemaphore*)pWaitSemaphores[i], (uint64_t)0)); @@ -315,25 +313,19 @@ void MVKQueueCommandBufferSubmission::commitActiveMTLCommandBuffer(bool signalCo // If using inline semaphore waiting, do so now. for (auto& ws : _waitSemaphores) { ws.first->encodeWait(nil, ws.second); } - MVKDevice* mkvDev = _queue->_device; + // If we need to signal completion, use getActiveMTLCommandBuffer() to ensure at least + // one MTLCommandBuffer is used, otherwise if this instance has no content, it will not + // finish(), signal the fence and semaphores ,and be destroyed. + // Use temp var for MTLCommandBuffer commit and release because completion callback + // may destroy this instance before this function ends. + id mtlCmdBuff = signalCompletion ? getActiveMTLCommandBuffer() : _activeMTLCommandBuffer; + _activeMTLCommandBuffer = nil; + + MVKDevice* mkvDev = _queue->getDevice(); uint64_t startTime = mkvDev->getPerformanceTimestamp(); - - // Use getActiveMTLCommandBuffer() to ensure at least one MTLCommandBuffer is used, - // otherwise if this instance has no content, it will not finish() and be destroyed. - if (signalCompletion || _trackPerformance) { - [getActiveMTLCommandBuffer() addCompletedHandler: ^(id mtlCmdBuff) { - mkvDev->addActivityPerformance(mkvDev->_performanceStatistics.queue.mtlCommandBufferCompletion, startTime); - if (signalCompletion) { this->finish(); } - }]; - } - - // Use temp vars because callback may destroy this instance before this function ends. - MVKDevice* device = _queue->getDevice(); - id mtlCmdBuff = _activeMTLCommandBuffer; - // If command buffer execution fails, log it, and mark the device lost. [mtlCmdBuff addCompletedHandler: ^(id mtlCB) { if (mtlCB.status == MTLCommandBufferStatusError) { - device->reportError(device->markLost(), "Command buffer %p \"%s\" execution failed (code %li): %s", mtlCB, mtlCB.label ? mtlCB.label.UTF8String : "", mtlCB.error.code, mtlCB.error.localizedDescription.UTF8String); + getVulkanAPIObject()->reportError(mkvDev->markLost(), "Command buffer %p \"%s\" execution failed (code %li): %s", mtlCB, mtlCB.label ? mtlCB.label.UTF8String : "", mtlCB.error.code, mtlCB.error.localizedDescription.UTF8String); // Some errors indicate we lost the physical device as well. switch (mtlCB.error.code) { case MTLCommandBufferErrorBlacklisted: @@ -342,7 +334,7 @@ void MVKQueueCommandBufferSubmission::commitActiveMTLCommandBuffer(bool signalCo #if MVK_MACOS && !MVK_MACCAT case MTLCommandBufferErrorDeviceRemoved: #endif - device->getPhysicalDevice()->setConfigurationResult(VK_ERROR_DEVICE_LOST); + mkvDev->getPhysicalDevice()->setConfigurationResult(VK_ERROR_DEVICE_LOST); break; } #if MVK_XCODE_12 @@ -372,9 +364,12 @@ void MVKQueueCommandBufferSubmission::commitActiveMTLCommandBuffer(bool signalCo } } #endif + + // Ensure finish() is the last thing the completetion callback does. + mkvDev->addActivityPerformance(mkvDev->_performanceStatistics.queue.mtlCommandBufferCompletion, startTime); + if (signalCompletion) { this->finish(); } }]; - _activeMTLCommandBuffer = nil; [mtlCmdBuff commit]; [mtlCmdBuff release]; // retained } From a4d36cb7579f361b1b149095b7a6778b8c15f244 Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Thu, 25 Feb 2021 17:19:53 -0500 Subject: [PATCH 2/3] Reduce debug log noise by only logging header msg for shader debug logs if at least one shader log exists. --- MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm index e295b78c..e75a85bf 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm @@ -358,8 +358,12 @@ void MVKQueueCommandBufferSubmission::commitActiveMTLCommandBuffer(bool signalCo } #if MVK_XCODE_12 if (mvkGetMVKConfiguration()->debugMode) { - MVKLogInfo("Shader log messages:"); + bool isFirstMsg = true; for (id log in mtlCB.logs) { + if (isFirstMsg) { + MVKLogInfo("Shader log messages:"); + isFirstMsg = false; + } MVKLogInfo("%s", log.description.UTF8String); } } From 68a0119f2117d1bcf686d0d72f431e3d4e3476ff Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Thu, 25 Feb 2021 21:09:14 -0500 Subject: [PATCH 3/3] Fix typo. --- MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm index e75a85bf..c502387a 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm @@ -321,11 +321,11 @@ void MVKQueueCommandBufferSubmission::commitActiveMTLCommandBuffer(bool signalCo id mtlCmdBuff = signalCompletion ? getActiveMTLCommandBuffer() : _activeMTLCommandBuffer; _activeMTLCommandBuffer = nil; - MVKDevice* mkvDev = _queue->getDevice(); - uint64_t startTime = mkvDev->getPerformanceTimestamp(); + MVKDevice* mvkDev = _queue->getDevice(); + uint64_t startTime = mvkDev->getPerformanceTimestamp(); [mtlCmdBuff addCompletedHandler: ^(id mtlCB) { if (mtlCB.status == MTLCommandBufferStatusError) { - getVulkanAPIObject()->reportError(mkvDev->markLost(), "Command buffer %p \"%s\" execution failed (code %li): %s", mtlCB, mtlCB.label ? mtlCB.label.UTF8String : "", mtlCB.error.code, mtlCB.error.localizedDescription.UTF8String); + getVulkanAPIObject()->reportError(mvkDev->markLost(), "Command buffer %p \"%s\" execution failed (code %li): %s", mtlCB, mtlCB.label ? mtlCB.label.UTF8String : "", mtlCB.error.code, mtlCB.error.localizedDescription.UTF8String); // Some errors indicate we lost the physical device as well. switch (mtlCB.error.code) { case MTLCommandBufferErrorBlacklisted: @@ -334,7 +334,7 @@ void MVKQueueCommandBufferSubmission::commitActiveMTLCommandBuffer(bool signalCo #if MVK_MACOS && !MVK_MACCAT case MTLCommandBufferErrorDeviceRemoved: #endif - mkvDev->getPhysicalDevice()->setConfigurationResult(VK_ERROR_DEVICE_LOST); + mvkDev->getPhysicalDevice()->setConfigurationResult(VK_ERROR_DEVICE_LOST); break; } #if MVK_XCODE_12 @@ -370,7 +370,7 @@ void MVKQueueCommandBufferSubmission::commitActiveMTLCommandBuffer(bool signalCo #endif // Ensure finish() is the last thing the completetion callback does. - mkvDev->addActivityPerformance(mkvDev->_performanceStatistics.queue.mtlCommandBufferCompletion, startTime); + mvkDev->addActivityPerformance(mvkDev->_performanceStatistics.queue.mtlCommandBufferCompletion, startTime); if (signalCompletion) { this->finish(); } }];