From 7fe4963985d8ae44159243d8babff25cf830bca7 Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Wed, 6 Sep 2023 16:16:11 -0400 Subject: [PATCH 1/2] Guard against CAMetalDrawable with invalid pixel format. - Calling nextDrawable may result in a nil drawable, or a drawable with no pixel format. Attempt several times to retrieve a drawable with a valid pixel format, and if unsuccessful, return an error from vkQueuePresentKHR() and vkAcquireNextImageKHR(), to force swapchain to be re-created. - Reorganize MVKQueuePresentSurfaceSubmission::execute() to detect drawable with invalid format, attach MTLCommandBuffer completion handler just before commit, and delay enqueuing MTLCommandBuffer until commit. - Refactor mvkOSVersionIsAtLeast() for clarity (unrelated). --- Common/MVKOSExtensions.h | 17 +++++++++------- Docs/Whats_New.md | 1 + MoltenVK/MoltenVK/GPUObjects/MVKImage.h | 2 +- MoltenVK/MoltenVK/GPUObjects/MVKImage.mm | 23 ++++++++++++++++------ MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm | 25 ++++++++++++------------ 5 files changed, 42 insertions(+), 26 deletions(-) diff --git a/Common/MVKOSExtensions.h b/Common/MVKOSExtensions.h index 79d89216..e824ba06 100644 --- a/Common/MVKOSExtensions.h +++ b/Common/MVKOSExtensions.h @@ -39,27 +39,30 @@ static const MVKOSVersion kMVKOSVersionUnsupported = std::numeric_limits= minVer; } +static inline bool mvkOSVersionIsAtLeast(MVKOSVersion minVer) { return mvkOSVersion() >= minVer; } /** * Returns whether the operating system version is at least the appropriate min version. - * The constant kMVKOSVersionUnsupported can be used for either value to cause the test - * to always fail on that OS, which is useful for indidicating functionalty guarded by + * The constant kMVKOSVersionUnsupported can be used for any of the values to cause the test + * to always fail on that OS, which is useful for indidicating that functionalty guarded by * this test is not supported on that OS. */ -inline bool mvkOSVersionIsAtLeast(MVKOSVersion macOSMinVer, MVKOSVersion iOSMinVer, MVKOSVersion visionOSMinVer) { +static inline bool mvkOSVersionIsAtLeast(MVKOSVersion macOSMinVer, + MVKOSVersion iOSMinVer, + MVKOSVersion visionOSMinVer) { #if MVK_MACOS return mvkOSVersionIsAtLeast(macOSMinVer); #endif +#if MVK_IOS_OR_TVOS + return mvkOSVersionIsAtLeast(iOSMinVer); +#endif #if MVK_VISIONOS return mvkOSVersionIsAtLeast(visionOSMinVer); -#elif MVK_IOS_OR_TVOS - return mvkOSVersionIsAtLeast(iOSMinVer); #endif } diff --git a/Docs/Whats_New.md b/Docs/Whats_New.md index 3e476dd3..836a60a9 100644 --- a/Docs/Whats_New.md +++ b/Docs/Whats_New.md @@ -20,6 +20,7 @@ Released TBD - Fix rare case where vertex attribute buffers are not bound to Metal when no other bindings change between pipelines. +- Fix case where a `CAMetalDrawable` with invalid pixel format causes onscreen flickering. - Improve behavior of swapchain image presentation stalls caused by Metal regression. - Add several additional performance trackers, available via logging, or the `mvk_private_api.h` API. - Update `MVK_CONFIGURATION_API_VERSION` and `MVK_PRIVATE_API_VERSION` to `38`. diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.h b/MoltenVK/MoltenVK/GPUObjects/MVKImage.h index fb7c3dfa..1479f724 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.h @@ -454,7 +454,7 @@ public: #pragma mark Metal /** Presents the contained drawable to the OS. */ - void presentCAMetalDrawable(id mtlCmdBuff, MVKImagePresentInfo presentInfo); + VkResult presentCAMetalDrawable(id mtlCmdBuff, MVKImagePresentInfo presentInfo); /** Called when the presentation begins. */ void beginPresentation(const MVKImagePresentInfo& presentInfo); diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm index b632e78b..1769df11 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm @@ -1258,7 +1258,6 @@ VkResult MVKPresentableSwapchainImage::acquireAndSignalWhenAvailable(MVKSemaphor // This is not done earlier so the texture is retained for any post-processing such as screen captures, etc. releaseMetalDrawable(); - VkResult rslt = VK_SUCCESS; auto signaler = MVKSwapchainSignaler{fence, semaphore, semaphore ? semaphore->deferSignal() : 0}; if (_availability.isAvailable) { _availability.isAvailable = false; @@ -1271,7 +1270,7 @@ VkResult MVKPresentableSwapchainImage::acquireAndSignalWhenAvailable(MVKSemaphor id mtlCmdBuff = nil; if (mvkSem && mvkSem->isUsingCommandEncoding()) { mtlCmdBuff = _device->getAnyQueue()->getMTLCommandBuffer(kMVKCommandUseAcquireNextImage); - if ( !mtlCmdBuff ) { rslt = VK_ERROR_OUT_OF_POOL_MEMORY; } + if ( !mtlCmdBuff ) { setConfigurationResult(VK_ERROR_OUT_OF_POOL_MEMORY); } } signal(signaler, mtlCmdBuff); [mtlCmdBuff commit]; @@ -1283,19 +1282,29 @@ VkResult MVKPresentableSwapchainImage::acquireAndSignalWhenAvailable(MVKSemaphor } markAsTracked(signaler); - return rslt; + return getConfigurationResult(); } +// Calling nextDrawable may result in a nil drawable, or a drawable with no pixel format. +// Attempt several times to retrieve a good drawable, and set an error to trigger the +// swapchain to be re-established if one cannot be retrieved. id MVKPresentableSwapchainImage::getCAMetalDrawable() { if ( !_mtlDrawable ) { @autoreleasepool { + bool hasInvalidFormat = false; uint32_t attemptCnt = _swapchain->getImageCount() * 2; // Attempt a resonable number of times for (uint32_t attemptIdx = 0; !_mtlDrawable && attemptIdx < attemptCnt; attemptIdx++) { uint64_t startTime = _device->getPerformanceTimestamp(); _mtlDrawable = [_swapchain->_surface->getCAMetalLayer().nextDrawable retain]; // retained _device->addPerformanceInterval(_device->_performanceStatistics.queue.retrieveCAMetalDrawable, startTime); + hasInvalidFormat = _mtlDrawable && !_mtlDrawable.texture.pixelFormat; + if (hasInvalidFormat) { releaseMetalDrawable(); } + } + if (hasInvalidFormat) { + setConfigurationResult(reportError(VK_ERROR_OUT_OF_DATE_KHR, "CAMetalDrawable with valid format could not be acquired after %d attempts.", attemptCnt)); + } else if ( !_mtlDrawable ) { + setConfigurationResult(reportError(VK_ERROR_OUT_OF_POOL_MEMORY, "CAMetalDrawable could not be acquired after %d attempts.", attemptCnt)); } - if ( !_mtlDrawable ) { reportError(VK_ERROR_OUT_OF_POOL_MEMORY, "CAMetalDrawable could not be acquired after %d attempts.", attemptCnt); } } } return _mtlDrawable; @@ -1303,8 +1312,8 @@ id MVKPresentableSwapchainImage::getCAMetalDrawable() { // Present the drawable and make myself available only once the command buffer has completed. // Pass MVKImagePresentInfo by value because it may not exist when the callback runs. -void MVKPresentableSwapchainImage::presentCAMetalDrawable(id mtlCmdBuff, - MVKImagePresentInfo presentInfo) { +VkResult MVKPresentableSwapchainImage::presentCAMetalDrawable(id mtlCmdBuff, + MVKImagePresentInfo presentInfo) { lock_guard lock(_availabilityLock); _swapchain->renderWatermark(getMTLTexture(0), mtlCmdBuff); @@ -1363,6 +1372,8 @@ void MVKPresentableSwapchainImage::presentCAMetalDrawable(id m }]; signalPresentationSemaphore(signaler, mtlCmdBuff); + + return getConfigurationResult(); } // Pass MVKImagePresentInfo by value because it may not exist when the callback runs. diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm index c104deed..f53cb71d 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKQueue.mm @@ -623,12 +623,6 @@ MVKQueueFullCommandBufferSubmission::MVKQueueFullCommandBufferSubmission(MVKQ // The semaphores know what to do. VkResult MVKQueuePresentSurfaceSubmission::execute() { id mtlCmdBuff = _queue->getMTLCommandBuffer(kMVKCommandUseQueuePresent); - [mtlCmdBuff enqueue]; - - // Add completion handler that will destroy this submission only once the MTLCommandBuffer - // is finished with the resources retained here, including the wait semaphores. - // Completion handlers are also added in presentCAMetalDrawable() to retain the swapchain images. - [mtlCmdBuff addCompletedHandler: ^(id mtlCB) { this->finish(); }]; for (auto& ws : _waitSemaphores) { auto& sem4 = ws.first; @@ -637,15 +631,22 @@ VkResult MVKQueuePresentSurfaceSubmission::execute() { } for (int i = 0; i < _presentInfo.size(); i++ ) { - _presentInfo[i].presentableImage->presentCAMetalDrawable(mtlCmdBuff, _presentInfo[i]); + setConfigurationResult(_presentInfo[i].presentableImage->presentCAMetalDrawable(mtlCmdBuff, _presentInfo[i])); } - [mtlCmdBuff commit]; + if ( !mtlCmdBuff ) { setConfigurationResult(VK_ERROR_OUT_OF_POOL_MEMORY); } // Check after images may set error. - // If an error occurred and the MTLCommandBuffer was not created, call finish() directly. - if ( !mtlCmdBuff ) { finish(); } - - return mtlCmdBuff ? VK_SUCCESS : VK_ERROR_OUT_OF_POOL_MEMORY; + // Add completion callback to the MTLCommandBuffer to call finish(), + // or if the MTLCommandBuffer could not be created, call finish() directly. + // Retrieve the result first, because finish() will destroy this instance. + VkResult rslt = getConfigurationResult(); + if (mtlCmdBuff) { + [mtlCmdBuff addCompletedHandler: ^(id mtlCB) { this->finish(); }]; + [mtlCmdBuff commit]; + } else { + finish(); + } + return rslt; } void MVKQueuePresentSurfaceSubmission::finish() { From 6c6139ca929d2edd01930b3034c2ccd5adc55705 Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Thu, 7 Sep 2023 09:33:40 -0400 Subject: [PATCH 2/2] Update Common/MVKOSExtensions.h Co-authored-by: Chip Davis --- Common/MVKOSExtensions.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Common/MVKOSExtensions.h b/Common/MVKOSExtensions.h index e824ba06..13d864da 100644 --- a/Common/MVKOSExtensions.h +++ b/Common/MVKOSExtensions.h @@ -49,7 +49,7 @@ static inline bool mvkOSVersionIsAtLeast(MVKOSVersion minVer) { return mvkOSVers /** * Returns whether the operating system version is at least the appropriate min version. * The constant kMVKOSVersionUnsupported can be used for any of the values to cause the test - * to always fail on that OS, which is useful for indidicating that functionalty guarded by + * to always fail on that OS, which is useful for indicating that functionalty guarded by * this test is not supported on that OS. */ static inline bool mvkOSVersionIsAtLeast(MVKOSVersion macOSMinVer,