From 898c03d77fe4ddba664b86965e47cbf3f893aa09 Mon Sep 17 00:00:00 2001 From: Chip Davis Date: Fri, 20 Nov 2020 16:15:28 -0600 Subject: [PATCH] MVKSync: Miscellaneous fixes. Fix reversed condition in `MVKTimelineSemaphoreEmulated`, which probably caused some test failures. Don't increment the `MTLEvent` binary semaphore's counter unless a command buffer is actually present to schedule a wait on. Defer signal operations when a swapchain image is not yet available. That way, the correct value will be signaled when the image is ready, instead of causing GPU lockups and timeouts. When a drawable is presented, immediately mark it available, instead of waiting until the command buffer finishes. Otherwise, the wrong semaphore could be signaled when an image is used twice in a row. This should fix the problems using `MTLEvent` binary semaphores with presentation, which was preventing us from enabling them by default when available. Special thanks to @apayen, whose [idea](https://github.com/KhronosGroup/MoltenVK/issues/803) and [change](https://github.com/apayen/MoltenVK/commit/a4ac715975ea3aa4cb95e91973b8f9010516304f) were the basis for this. --- MoltenVK/MoltenVK/GPUObjects/MVKImage.h | 17 +++-- MoltenVK/MoltenVK/GPUObjects/MVKImage.mm | 80 ++++++++++++------------ MoltenVK/MoltenVK/GPUObjects/MVKSync.h | 38 ++++++++++- MoltenVK/MoltenVK/GPUObjects/MVKSync.mm | 32 ++++++++-- 4 files changed, 113 insertions(+), 54 deletions(-) diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.h b/MoltenVK/MoltenVK/GPUObjects/MVKImage.h index aa1417a5..44cd261c 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.h @@ -438,7 +438,11 @@ typedef struct { } MVKPresentTimingInfo; /** Tracks a semaphore and fence for later signaling. */ -typedef std::pair MVKSwapchainSignaler; +struct MVKSwapchainSignaler { + MVKFence* fence; + MVKSemaphore* semaphore; + uint64_t semaphoreSignalToken; +}; /** Represents a Vulkan swapchain image that can be submitted to the presentation engine. */ @@ -470,15 +474,16 @@ protected: void presentCAMetalDrawable(id mtlDrawable, MVKPresentTimingInfo presentTimingInfo); void releaseMetalDrawable(); MVKSwapchainImageAvailability getAvailability(); - void makeAvailable(); + void makeAvailable(const MVKSwapchainSignaler& signaler); void acquireAndSignalWhenAvailable(MVKSemaphore* semaphore, MVKFence* fence); - void signal(MVKSwapchainSignaler& signaler, id mtlCmdBuff); - void signalPresentationSemaphore(id mtlCmdBuff); - static void markAsTracked(MVKSwapchainSignaler& signaler); - static void unmarkAsTracked(MVKSwapchainSignaler& signaler); + void signal(const MVKSwapchainSignaler& signaler, id mtlCmdBuff); + void signalPresentationSemaphore(const MVKSwapchainSignaler& signaler, id mtlCmdBuff); + static void markAsTracked(const MVKSwapchainSignaler& signaler); + static void unmarkAsTracked(const MVKSwapchainSignaler& signaler); void renderWatermark(id mtlCmdBuff); id _mtlDrawable; + id _presentingMTLCmdBuff; MVKSwapchainImageAvailability _availability; MVKSmallVector _availabilitySignalers; MVKSwapchainSignaler _preSignaler; diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm index 5a7d0e34..63fe437c 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm @@ -1179,30 +1179,12 @@ MVKSwapchainImageAvailability MVKPresentableSwapchainImage::getAvailability() { // Makes an image available for acquisition by the app. // If any semaphores are waiting to be signaled when this image becomes available, the // earliest semaphore is signaled, and this image remains unavailable for other uses. -void MVKPresentableSwapchainImage::makeAvailable() { +void MVKPresentableSwapchainImage::makeAvailable(const MVKSwapchainSignaler& signaler) { lock_guard lock(_availabilityLock); // Mark when this event happened, relative to that of other images _availability.acquisitionID = _swapchain->getNextAcquisitionID(); - // Mark this image as available if no semaphores or fences are waiting to be signaled. - _availability.isAvailable = _availabilitySignalers.empty(); - - MVKSwapchainSignaler signaler; - if (_availability.isAvailable) { - // If this image is available, signal the semaphore and fence that were associated - // with the last time this image was acquired while available. This is a workaround for - // when an app uses a single semaphore or fence for more than one swapchain image. - // Because the semaphore or fence will be signaled by more than one image, it will - // get out of sync, and the final use of the image would not be signaled as a result. - signaler = _preSignaler; - } else { - // If this image is not yet available, extract and signal the first semaphore and fence. - auto sigIter = _availabilitySignalers.begin(); - signaler = *sigIter; - _availabilitySignalers.erase(sigIter); - } - // Signal the semaphore and fence, and let them know they are no longer being tracked. signal(signaler, nil); unmarkAsTracked(signaler); @@ -1217,15 +1199,15 @@ void MVKPresentableSwapchainImage::acquireAndSignalWhenAvailable(MVKSemaphore* s // This is not done earlier so the texture is retained for any post-processing such as screen captures, etc. releaseMetalDrawable(); - auto signaler = make_pair(semaphore, fence); + auto signaler = MVKSwapchainSignaler{fence, semaphore, semaphore ? semaphore->deferSignal() : 0}; if (_availability.isAvailable) { _availability.isAvailable = false; - // If signalling through a MTLEvent, use an ephemeral MTLCommandBuffer. + // If signalling through a MTLEvent, and there's no command buffer presenting me, use an ephemeral MTLCommandBuffer. // Another option would be to use MTLSharedEvent in MVKSemaphore, but that might // impose unacceptable performance costs to handle this particular case. @autoreleasepool { - MVKSemaphore* mvkSem = signaler.first; + MVKSemaphore* mvkSem = signaler.semaphore; id mtlCmdBuff = (mvkSem && mvkSem->isUsingCommandEncoding() ? [_device->getAnyQueue()->getMTLCommandQueue() commandBufferWithUnretainedReferences] : nil); @@ -1243,31 +1225,27 @@ void MVKPresentableSwapchainImage::acquireAndSignalWhenAvailable(MVKSemaphore* s } // If present, signal the semaphore for the first waiter for the given image. -void MVKPresentableSwapchainImage::signalPresentationSemaphore(id mtlCmdBuff) { - lock_guard lock(_availabilityLock); - - if ( !_availabilitySignalers.empty() ) { - MVKSemaphore* mvkSem = _availabilitySignalers.front().first; - if (mvkSem) { mvkSem->encodeSignal(mtlCmdBuff, 0); } - } +void MVKPresentableSwapchainImage::signalPresentationSemaphore(const MVKSwapchainSignaler& signaler, id mtlCmdBuff) { + MVKSemaphore* mvkSem = signaler.semaphore; + if (mvkSem) { mvkSem->encodeDeferredSignal(mtlCmdBuff, signaler.semaphoreSignalToken); } } // Signal either or both of the semaphore and fence in the specified tracker pair. -void MVKPresentableSwapchainImage::signal(MVKSwapchainSignaler& signaler, id mtlCmdBuff) { - if (signaler.first) { signaler.first->encodeSignal(mtlCmdBuff, 0); } - if (signaler.second) { signaler.second->signal(); } +void MVKPresentableSwapchainImage::signal(const MVKSwapchainSignaler& signaler, id mtlCmdBuff) { + if (signaler.semaphore) { signaler.semaphore->encodeDeferredSignal(mtlCmdBuff, signaler.semaphoreSignalToken); } + if (signaler.fence) { signaler.fence->signal(); } } // Tell the semaphore and fence that they are being tracked for future signaling. -void MVKPresentableSwapchainImage::markAsTracked(MVKSwapchainSignaler& signaler) { - if (signaler.first) { signaler.first->retain(); } - if (signaler.second) { signaler.second->retain(); } +void MVKPresentableSwapchainImage::markAsTracked(const MVKSwapchainSignaler& signaler) { + if (signaler.semaphore) { signaler.semaphore->retain(); } + if (signaler.fence) { signaler.fence->retain(); } } // Tell the semaphore and fence that they are no longer being tracked for future signaling. -void MVKPresentableSwapchainImage::unmarkAsTracked(MVKSwapchainSignaler& signaler) { - if (signaler.first) { signaler.first->release(); } - if (signaler.second) { signaler.second->release(); } +void MVKPresentableSwapchainImage::unmarkAsTracked(const MVKSwapchainSignaler& signaler) { + if (signaler.semaphore) { signaler.semaphore->release(); } + if (signaler.fence) { signaler.fence->release(); } } @@ -1291,6 +1269,8 @@ id MVKPresentableSwapchainImage::getCAMetalDrawable() { void MVKPresentableSwapchainImage::presentCAMetalDrawable(id mtlCmdBuff, MVKPresentTimingInfo presentTimingInfo) { + lock_guard lock(_availabilityLock); + _swapchain->willPresentSurface(getMTLTexture(0), mtlCmdBuff); // Get current drawable now. Don't retrieve in handler, because a new drawable might be acquired by then. @@ -1299,14 +1279,32 @@ void MVKPresentableSwapchainImage::presentCAMetalDrawable(id m presentCAMetalDrawable(mtlDrwbl, presentTimingInfo); }]; + MVKSwapchainSignaler signaler; + // Mark this image as available if no semaphores or fences are waiting to be signaled. + _availability.isAvailable = _availabilitySignalers.empty(); + if (_availability.isAvailable) { + // If this image is available, signal the semaphore and fence that were associated + // with the last time this image was acquired while available. This is a workaround for + // when an app uses a single semaphore or fence for more than one swapchain image. + // Because the semaphore or fence will be signaled by more than one image, it will + // get out of sync, and the final use of the image would not be signaled as a result. + signaler = _preSignaler; + // Save the command buffer in case this image is acquired before presentation is finished. + } else { + // If this image is not yet available, extract and signal the first semaphore and fence. + auto sigIter = _availabilitySignalers.begin(); + signaler = *sigIter; + _availabilitySignalers.erase(sigIter); + } + // Ensure this image is not destroyed while awaiting MTLCommandBuffer completion retain(); [mtlCmdBuff addCompletedHandler: ^(id mcb) { - makeAvailable(); + makeAvailable(signaler); release(); }]; - signalPresentationSemaphore(mtlCmdBuff); + signalPresentationSemaphore(signaler, mtlCmdBuff); } void MVKPresentableSwapchainImage::presentCAMetalDrawable(id mtlDrawable, @@ -1360,7 +1358,7 @@ MVKPresentableSwapchainImage::MVKPresentableSwapchainImage(MVKDevice* device, _availability.acquisitionID = _swapchain->getNextAcquisitionID(); _availability.isAvailable = true; - _preSignaler = make_pair(nullptr, nullptr); + _preSignaler = MVKSwapchainSignaler{nullptr, nullptr, 0}; } MVKPresentableSwapchainImage::~MVKPresentableSwapchainImage() { diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKSync.h b/MoltenVK/MoltenVK/GPUObjects/MVKSync.h index 8dd79451..22bf671d 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKSync.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKSync.h @@ -159,6 +159,27 @@ public: */ virtual void encodeSignal(id mtlCmdBuff, uint64_t value) = 0; + /** + * Begin a deferred signal operation. + * + * A deferred signal acts like a normal signal operation, except that the signal op itself + * is not actually executed. A token is returned, which must be passed to encodeDeferredSignal() + * to complete the signal operation. + * + * This is intended to be used by MVKSwapchain, in order to properly implement semaphores + * for image availability, particularly with MTLEvent-based semaphores, to ensure the correct + * value is used in signal/wait operations. + */ + virtual uint64_t deferSignal() = 0; + + /** + * Complete a deferred signal operation. + * + * This is the other half of the deferSignal() method. The token that was returned + * from deferSignal() must be passed here. + */ + virtual void encodeDeferredSignal(id mtlCmdBuff, uint64_t deferToken) = 0; + /** Returns whether this semaphore uses command encoding. */ virtual bool isUsingCommandEncoding() = 0; @@ -182,6 +203,8 @@ class MVKSemaphoreMTLFence : public MVKSemaphore { public: void encodeWait(id mtlCmdBuff, uint64_t) override; void encodeSignal(id mtlCmdBuff, uint64_t) override; + uint64_t deferSignal() override; + void encodeDeferredSignal(id mtlCmdBuff, uint64_t) override; bool isUsingCommandEncoding() override { return true; } MVKSemaphoreMTLFence(MVKDevice* device, const VkSemaphoreCreateInfo* pCreateInfo); @@ -200,8 +223,10 @@ protected: class MVKSemaphoreMTLEvent : public MVKSemaphore { public: - void encodeWait(id mtlCmdBuff, uint64_t) override; - void encodeSignal(id mtlCmdBuff, uint64_t) override; + void encodeWait(id mtlCmdBuff, uint64_t value) override; + void encodeSignal(id mtlCmdBuff, uint64_t value) override; + uint64_t deferSignal() override; + void encodeDeferredSignal(id mtlCmdBuff, uint64_t deferToken) override; bool isUsingCommandEncoding() override { return true; } MVKSemaphoreMTLEvent(MVKDevice* device, const VkSemaphoreCreateInfo* pCreateInfo); @@ -223,6 +248,8 @@ class MVKSemaphoreEmulated : public MVKSemaphore { public: void encodeWait(id mtlCmdBuff, uint64_t) override; void encodeSignal(id mtlCmdBuff, uint64_t) override; + uint64_t deferSignal() override; + void encodeDeferredSignal(id mtlCmdBuff, uint64_t) override; bool isUsingCommandEncoding() override { return false; } MVKSemaphoreEmulated(MVKDevice* device, const VkSemaphoreCreateInfo* pCreateInfo); @@ -241,6 +268,13 @@ class MVKTimelineSemaphore : public MVKSemaphore { public: VkSemaphoreType getSemaphoreType() override { return VK_SEMAPHORE_TYPE_TIMELINE; } + uint64_t deferSignal() override { return 0; } + // Timeline semaphores cannot yet be used for signaling swapchain availability, because + // no interaction is yet defined. When it is, though, it's likely that a value must + // be supplied, just like when using them with command buffers. + void encodeDeferredSignal(id mtlCmdBuff, uint64_t deferToken) override { + return encodeSignal(mtlCmdBuff, deferToken); + } /** Returns the current value of the semaphore counter. */ virtual uint64_t getCounterValue() = 0; diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKSync.mm b/MoltenVK/MoltenVK/GPUObjects/MVKSync.mm index e5ea2c56..fcf160b3 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKSync.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKSync.mm @@ -96,6 +96,14 @@ void MVKSemaphoreMTLFence::encodeSignal(id mtlCmdBuff, uint64_ [mtlCmdEnc endEncoding]; } +uint64_t MVKSemaphoreMTLFence::deferSignal() { + return 0; +} + +void MVKSemaphoreMTLFence::encodeDeferredSignal(id mtlCmdBuff, uint64_t) { + encodeSignal(mtlCmdBuff, 0); +} + MVKSemaphoreMTLFence::MVKSemaphoreMTLFence(MVKDevice* device, const VkSemaphoreCreateInfo* pCreateInfo) : MVKSemaphore(device, pCreateInfo), _mtlFence([device->getMTLDevice() newFence]) {} //retained @@ -108,14 +116,20 @@ MVKSemaphoreMTLFence::~MVKSemaphoreMTLFence() { #pragma mark - #pragma mark MVKSemaphoreMTLEvent -// Nil mtlCmdBuff will do nothing. void MVKSemaphoreMTLEvent::encodeWait(id mtlCmdBuff, uint64_t) { - [mtlCmdBuff encodeWaitForEvent: _mtlEvent value: _mtlEventValue++]; + if (mtlCmdBuff) { [mtlCmdBuff encodeWaitForEvent: _mtlEvent value: _mtlEventValue++]; } } -// Nil mtlCmdBuff will do nothing. void MVKSemaphoreMTLEvent::encodeSignal(id mtlCmdBuff, uint64_t) { - [mtlCmdBuff encodeSignalEvent: _mtlEvent value: _mtlEventValue]; + if (mtlCmdBuff) { [mtlCmdBuff encodeSignalEvent: _mtlEvent value: _mtlEventValue]; } +} + +uint64_t MVKSemaphoreMTLEvent::deferSignal() { + return _mtlEventValue; +} + +void MVKSemaphoreMTLEvent::encodeDeferredSignal(id mtlCmdBuff, uint64_t deferToken) { + if (mtlCmdBuff) { [mtlCmdBuff encodeSignalEvent: _mtlEvent value: deferToken]; } } MVKSemaphoreMTLEvent::MVKSemaphoreMTLEvent(MVKDevice* device, const VkSemaphoreCreateInfo* pCreateInfo) : @@ -139,6 +153,14 @@ void MVKSemaphoreEmulated::encodeSignal(id mtlCmdBuff, uint64_ if ( !mtlCmdBuff ) { _blocker.release(); } } +uint64_t MVKSemaphoreEmulated::deferSignal() { + return 0; +} + +void MVKSemaphoreEmulated::encodeDeferredSignal(id mtlCmdBuff, uint64_t) { + encodeSignal(mtlCmdBuff, 0); +} + MVKSemaphoreEmulated::MVKSemaphoreEmulated(MVKDevice* device, const VkSemaphoreCreateInfo* pCreateInfo) : MVKSemaphore(device, pCreateInfo), _blocker(false, 1) {} @@ -219,7 +241,7 @@ void MVKTimelineSemaphoreEmulated::signalImpl(uint64_t value) { _value = value; _blocker.notify_all(); for (auto& sittersForValue : _sitters) { - if (sittersForValue.first < value) { continue; } + if (sittersForValue.first > value) { continue; } for (auto* sitter : sittersForValue.second) { sitter->signaled(); }