Merge pull request #1165 from cdavis5e/sync-fixes

MVKSync: Miscellaneous fixes.

MoltenVK/MoltenVK/GPUObjects/MVKImage.h

@@ -438,7 +438,11 @@ typedef struct  {
 } MVKPresentTimingInfo;
 
 /** Tracks a semaphore and fence for later signaling. */
-typedef std::pair<MVKSemaphore*, MVKFence*> 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<CAMetalDrawable> 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<MTLCommandBuffer> mtlCmdBuff);
+	void signal(const MVKSwapchainSignaler& signaler, id<MTLCommandBuffer> mtlCmdBuff);
-	void signalPresentationSemaphore(id<MTLCommandBuffer> mtlCmdBuff);
+	void signalPresentationSemaphore(const MVKSwapchainSignaler& signaler, id<MTLCommandBuffer> mtlCmdBuff);
-	static void markAsTracked(MVKSwapchainSignaler& signaler);
+	static void markAsTracked(const MVKSwapchainSignaler& signaler);
-	static void unmarkAsTracked(MVKSwapchainSignaler& signaler);
+	static void unmarkAsTracked(const MVKSwapchainSignaler& signaler);
 	void renderWatermark(id<MTLCommandBuffer> mtlCmdBuff);
 
 	id<CAMetalDrawable> _mtlDrawable;
+	id<MTLCommandBuffer> _presentingMTLCmdBuff;
 	MVKSwapchainImageAvailability _availability;
 	MVKSmallVector<MVKSwapchainSignaler, 1> _availabilitySignalers;
 	MVKSwapchainSignaler _preSignaler;

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<mutex> 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<MTLCommandBuffer> 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<MTLCommandBuffer> mtlCmdBuff) {
+void MVKPresentableSwapchainImage::signalPresentationSemaphore(const MVKSwapchainSignaler& signaler, id<MTLCommandBuffer> mtlCmdBuff) {
-	lock_guard<mutex> lock(_availabilityLock);
+	MVKSemaphore* mvkSem = signaler.semaphore;
-
+	if (mvkSem) { mvkSem->encodeDeferredSignal(mtlCmdBuff, signaler.semaphoreSignalToken); }
-	if ( !_availabilitySignalers.empty() ) {
-		MVKSemaphore* mvkSem = _availabilitySignalers.front().first;
-		if (mvkSem) { mvkSem->encodeSignal(mtlCmdBuff, 0); }
-	}
 }
 
 // Signal either or both of the semaphore and fence in the specified tracker pair.
-void MVKPresentableSwapchainImage::signal(MVKSwapchainSignaler& signaler, id<MTLCommandBuffer> mtlCmdBuff) {
+void MVKPresentableSwapchainImage::signal(const MVKSwapchainSignaler& signaler, id<MTLCommandBuffer> mtlCmdBuff) {
-	if (signaler.first) { signaler.first->encodeSignal(mtlCmdBuff, 0); }
+	if (signaler.semaphore) { signaler.semaphore->encodeDeferredSignal(mtlCmdBuff, signaler.semaphoreSignalToken); }
-	if (signaler.second) { signaler.second->signal(); }
+	if (signaler.fence) { signaler.fence->signal(); }
 }
 
 // Tell the semaphore and fence that they are being tracked for future signaling.
-void MVKPresentableSwapchainImage::markAsTracked(MVKSwapchainSignaler& signaler) {
+void MVKPresentableSwapchainImage::markAsTracked(const MVKSwapchainSignaler& signaler) {
-	if (signaler.first) { signaler.first->retain(); }
+	if (signaler.semaphore) { signaler.semaphore->retain(); }
-	if (signaler.second) { signaler.second->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) {
+void MVKPresentableSwapchainImage::unmarkAsTracked(const MVKSwapchainSignaler& signaler) {
-	if (signaler.first) { signaler.first->release(); }
+	if (signaler.semaphore) { signaler.semaphore->release(); }
-	if (signaler.second) { signaler.second->release(); }
+	if (signaler.fence) { signaler.fence->release(); }
 }
 
 
@@ -1291,6 +1269,8 @@ id<CAMetalDrawable> MVKPresentableSwapchainImage::getCAMetalDrawable() {
 void MVKPresentableSwapchainImage::presentCAMetalDrawable(id<MTLCommandBuffer> mtlCmdBuff,
 														  MVKPresentTimingInfo presentTimingInfo) {
 
+	lock_guard<mutex> 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<MTLCommandBuffer> 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<MTLCommandBuffer> mcb) {
-		makeAvailable();
+		makeAvailable(signaler);
 		release();
 	}];
 
-	signalPresentationSemaphore(mtlCmdBuff);
+	signalPresentationSemaphore(signaler, mtlCmdBuff);
 }
 
 void MVKPresentableSwapchainImage::presentCAMetalDrawable(id<CAMetalDrawable> 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() {

MoltenVK/MoltenVK/GPUObjects/MVKSync.h

@@ -159,6 +159,27 @@ public:
 	 */
 	virtual void encodeSignal(id<MTLCommandBuffer> 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<MTLCommandBuffer> 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<MTLCommandBuffer> mtlCmdBuff, uint64_t) override;
 	void encodeSignal(id<MTLCommandBuffer> mtlCmdBuff, uint64_t) override;
+	uint64_t deferSignal() override;
+	void encodeDeferredSignal(id<MTLCommandBuffer> 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<MTLCommandBuffer> mtlCmdBuff, uint64_t) override;
+	void encodeWait(id<MTLCommandBuffer> mtlCmdBuff, uint64_t value) override;
-	void encodeSignal(id<MTLCommandBuffer> mtlCmdBuff, uint64_t) override;
+	void encodeSignal(id<MTLCommandBuffer> mtlCmdBuff, uint64_t value) override;
+	uint64_t deferSignal() override;
+	void encodeDeferredSignal(id<MTLCommandBuffer> 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<MTLCommandBuffer> mtlCmdBuff, uint64_t) override;
 	void encodeSignal(id<MTLCommandBuffer> mtlCmdBuff, uint64_t) override;
+	uint64_t deferSignal() override;
+	void encodeDeferredSignal(id<MTLCommandBuffer> 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<MTLCommandBuffer> mtlCmdBuff, uint64_t deferToken) override {
+		return encodeSignal(mtlCmdBuff, deferToken);
+	}
 
 	/** Returns the current value of the semaphore counter. */
 	virtual uint64_t getCounterValue() = 0;

MoltenVK/MoltenVK/GPUObjects/MVKSync.mm

@@ -96,6 +96,14 @@ void MVKSemaphoreMTLFence::encodeSignal(id<MTLCommandBuffer> mtlCmdBuff, uint64_
 	[mtlCmdEnc endEncoding];
 }
 
+uint64_t MVKSemaphoreMTLFence::deferSignal() {
+	return 0;
+}
+
+void MVKSemaphoreMTLFence::encodeDeferredSignal(id<MTLCommandBuffer> 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<MTLCommandBuffer> mtlCmdBuff, uint64_t) {
-	[mtlCmdBuff encodeWaitForEvent: _mtlEvent value: _mtlEventValue++];
+	if (mtlCmdBuff) { [mtlCmdBuff encodeWaitForEvent: _mtlEvent value: _mtlEventValue++]; }
 }
 
-// Nil mtlCmdBuff will do nothing.
 void MVKSemaphoreMTLEvent::encodeSignal(id<MTLCommandBuffer> 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<MTLCommandBuffer> 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<MTLCommandBuffer> mtlCmdBuff, uint64_
 	if ( !mtlCmdBuff ) { _blocker.release(); }
 }
 
+uint64_t MVKSemaphoreEmulated::deferSignal() {
+	return 0;
+}
+
+void MVKSemaphoreEmulated::encodeDeferredSignal(id<MTLCommandBuffer> 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();
 			}