From efba2530f99ce5873bd3448b3e974cbda419f461 Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Thu, 20 Feb 2020 10:17:04 +0100 Subject: [PATCH] rhi: Do not rely on unspecified relation between readbacks and FramesInFlight Add a new queriable resource limit value MaxAsyncReadbackFrames. Change the autotest to rely on this instead of relying on the unspecified, works-by-accident relation between readbacks and FramesInFlight. This way even if the behavior diverges in some backend in the future, clients (well written ones, that is), will continue to function correctly. Also clarify the docs for FramesInFlight, and change d3d and gl to return the correct value (which is 1 from QRhi perspective; the expanded docs now explain a bit what this really means and what it does not). Change-Id: I0486715570a9e6fc5d3dc431694d1712875dfe01 Reviewed-by: Andy Nichols --- src/gui/rhi/qrhi.cpp | 53 +++++++++++++++++++++------- src/gui/rhi/qrhi_p.h | 3 +- src/gui/rhi/qrhid3d11.cpp | 8 ++++- src/gui/rhi/qrhigles2.cpp | 6 +++- src/gui/rhi/qrhimetal.mm | 2 ++ src/gui/rhi/qrhinull.cpp | 4 ++- src/gui/rhi/qrhivulkan.cpp | 2 ++ tests/auto/gui/rhi/qrhi/tst_qrhi.cpp | 10 +++--- 8 files changed, 66 insertions(+), 22 deletions(-) diff --git a/src/gui/rhi/qrhi.cpp b/src/gui/rhi/qrhi.cpp index f857142bd7..584fbb263d 100644 --- a/src/gui/rhi/qrhi.cpp +++ b/src/gui/rhi/qrhi.cpp @@ -627,18 +627,27 @@ Q_LOGGING_CATEGORY(QRHI_LOG_INFO, "qt.rhi.general") is what some OpenGL ES implementations provide. \value FramesInFlight The number of frames the backend may keep "in - flight". The value has no relevance, and is unspecified, with backends like - OpenGL and Direct3D 11. With backends like Vulkan or Metal, it is the - responsibility of QRhi to block whenever starting a new frame and finding - the CPU is already \c{N - 1} frames ahead of the GPU (because the command - buffer submitted in frame no. \c{current} - \c{N} has not yet completed). - The value N is what is returned from here, and is typically 2. This can be - relevant to applications that integrate rendering done directly with the - graphics API, as such rendering code may want to perform double (if the - value is 2) buffering for resources, such as, buffers, similarly to the - QRhi backends themselves. The current frame slot index (a value running 0, - 1, .., N-1, then wrapping around) is retrievable from - QRhi::currentFrameSlot(). + flight": with backends like Vulkan or Metal, it is the responsibility of + QRhi to block whenever starting a new frame and finding the CPU is already + \c{N - 1} frames ahead of the GPU (because the command buffer submitted in + frame no. \c{current} - \c{N} has not yet completed). The value N is what + is returned from here, and is typically 2. This can be relevant to + applications that integrate rendering done directly with the graphics API, + as such rendering code may want to perform double (if the value is 2) + buffering for resources, such as, buffers, similarly to the QRhi backends + themselves. The current frame slot index (a value running 0, 1, .., N-1, + then wrapping around) is retrievable from QRhi::currentFrameSlot(). The + value is 1 for backends where the graphics API offers no such low level + control over the command submission process. Note that pipelining may still + happen even when this value is 1 (some backends, such as D3D11, are + designed to attempt to enable this, for instance, by using an update + strategy for uniform buffers that does not stall the pipeline), but that is + then not controlled by QRhi and so not reflected here in the API. + + \value MaxAsyncReadbackFrames The number of \l{QRhi::endFrame()}{submitted} + frames (including the one that contains the readback) after which an + asynchronous texture or buffer readback is guaranteed to complete upon + \l{QRhi::beginFrame()}{starting a new frame}. */ /*! @@ -4339,7 +4348,15 @@ void QRhiResourceUpdateBatch::uploadStaticBuffer(QRhiBuffer *buf, const void *da is supported only when the QRhi::ReadBackNonUniformBuffer feature is reported as supported. - \a readBackTexture(), QRhi::isFeatureSupported() + \note The asynchronous readback is guaranteed to have completed when one of + the following conditions is met: \l{QRhi::finish()}{finish()} has been + called; or, at least \c N frames have been \l{QRhi::endFrame()}{submitted}, + including the frame that issued the readback operation, and the + \l{QRhi::beginFrame()}{recording of a new frame} has been started, where \c + N is the \l{QRhi::resourceLimit()}{resource limit value} returned for + QRhi::MaxAsyncReadbackFrames. + + \sa readBackTexture(), QRhi::isFeatureSupported(), QRhi::resourceLimit() */ void QRhiResourceUpdateBatch::readBackBuffer(QRhiBuffer *buf, int offset, int size, QRhiBufferReadbackResult *result) { @@ -4430,6 +4447,16 @@ void QRhiResourceUpdateBatch::copyTexture(QRhiTexture *dst, QRhiTexture *src, co happens with a byte ordered format. A \l{QRhiTexture::RGBA8}{RGBA8} texture maps therefore to byte ordered QImage formats, such as, QImage::Format_RGBA8888. + + \note The asynchronous readback is guaranteed to have completed when one of + the following conditions is met: \l{QRhi::finish()}{finish()} has been + called; or, at least \c N frames have been \l{QRhi::endFrame()}{submitted}, + including the frame that issued the readback operation, and the + \l{QRhi::beginFrame()}{recording of a new frame} has been started, where \c + N is the \l{QRhi::resourceLimit()}{resource limit value} returned for + QRhi::MaxAsyncReadbackFrames. + + \sa readBackBuffer(), QRhi::resourceLimit() */ void QRhiResourceUpdateBatch::readBackTexture(const QRhiReadbackDescription &rb, QRhiReadbackResult *result) { diff --git a/src/gui/rhi/qrhi_p.h b/src/gui/rhi/qrhi_p.h index 7d03ac8aaa..be96ebbf05 100644 --- a/src/gui/rhi/qrhi_p.h +++ b/src/gui/rhi/qrhi_p.h @@ -1448,7 +1448,8 @@ public: TextureSizeMin = 1, TextureSizeMax, MaxColorAttachments, - FramesInFlight + FramesInFlight, + MaxAsyncReadbackFrames }; ~QRhi(); diff --git a/src/gui/rhi/qrhid3d11.cpp b/src/gui/rhi/qrhid3d11.cpp index 883aa68df7..221ec6e6e5 100644 --- a/src/gui/rhi/qrhid3d11.cpp +++ b/src/gui/rhi/qrhid3d11.cpp @@ -482,7 +482,13 @@ int QRhiD3D11::resourceLimit(QRhi::ResourceLimit limit) const case QRhi::MaxColorAttachments: return 8; case QRhi::FramesInFlight: - return 2; // dummy + // From our perspective. What D3D does internally is another question + // (there could be pipelining, helped f.ex. by our MAP_DISCARD based + // uniform buffer update strategy), but that's out of our hands and + // does not concern us here. + return 1; + case QRhi::MaxAsyncReadbackFrames: + return 1; default: Q_UNREACHABLE(); return 0; diff --git a/src/gui/rhi/qrhigles2.cpp b/src/gui/rhi/qrhigles2.cpp index f2b9f7081b..df0b255bed 100644 --- a/src/gui/rhi/qrhigles2.cpp +++ b/src/gui/rhi/qrhigles2.cpp @@ -787,7 +787,11 @@ int QRhiGles2::resourceLimit(QRhi::ResourceLimit limit) const case QRhi::MaxColorAttachments: return caps.maxDrawBuffers; case QRhi::FramesInFlight: - return 2; // dummy + // From our perspective. What the GL impl does internally is another + // question, but that's out of our hands and does not concern us here. + return 1; + case QRhi::MaxAsyncReadbackFrames: + return 1; default: Q_UNREACHABLE(); return 0; diff --git a/src/gui/rhi/qrhimetal.mm b/src/gui/rhi/qrhimetal.mm index 5b77086983..b0813bfc77 100644 --- a/src/gui/rhi/qrhimetal.mm +++ b/src/gui/rhi/qrhimetal.mm @@ -581,6 +581,8 @@ int QRhiMetal::resourceLimit(QRhi::ResourceLimit limit) const return 8; case QRhi::FramesInFlight: return QMTL_FRAMES_IN_FLIGHT; + case QRhi::MaxAsyncReadbackFrames: + return QMTL_FRAMES_IN_FLIGHT; default: Q_UNREACHABLE(); return 0; diff --git a/src/gui/rhi/qrhinull.cpp b/src/gui/rhi/qrhinull.cpp index 4c59900aa6..8c07e09b32 100644 --- a/src/gui/rhi/qrhinull.cpp +++ b/src/gui/rhi/qrhinull.cpp @@ -146,7 +146,9 @@ int QRhiNull::resourceLimit(QRhi::ResourceLimit limit) const case QRhi::MaxColorAttachments: return 8; case QRhi::FramesInFlight: - return 2; // dummy + return 1; + case QRhi::MaxAsyncReadbackFrames: + return 1; default: Q_UNREACHABLE(); return 0; diff --git a/src/gui/rhi/qrhivulkan.cpp b/src/gui/rhi/qrhivulkan.cpp index 840eed5a79..5d6e9b21bc 100644 --- a/src/gui/rhi/qrhivulkan.cpp +++ b/src/gui/rhi/qrhivulkan.cpp @@ -4009,6 +4009,8 @@ int QRhiVulkan::resourceLimit(QRhi::ResourceLimit limit) const return int(physDevProperties.limits.maxColorAttachments); case QRhi::FramesInFlight: return QVK_FRAMES_IN_FLIGHT; + case QRhi::MaxAsyncReadbackFrames: + return QVK_FRAMES_IN_FLIGHT; default: Q_UNREACHABLE(); return 0; diff --git a/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp b/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp index ee6ac950c9..ab435caa40 100644 --- a/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp +++ b/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp @@ -1672,9 +1672,9 @@ void tst_QRhi::renderToWindowSimple() QVERIFY(pipeline->build()); - const int framesInFlight = rhi->resourceLimit(QRhi::FramesInFlight); - QVERIFY(framesInFlight >= 1); - const int FRAME_COUNT = framesInFlight + 1; + const int asyncReadbackFrames = rhi->resourceLimit(QRhi::MaxAsyncReadbackFrames); + // one frame issues the readback, then we do MaxAsyncReadbackFrames more to ensure the readback completes + const int FRAME_COUNT = asyncReadbackFrames + 1; bool readCompleted = false; QRhiReadbackResult readResult; QImage result; @@ -1721,8 +1721,8 @@ void tst_QRhi::renderToWindowSimple() } // The readback is asynchronous here. However it is guaranteed that it - // finished at latest after rendering QRhi::FramesInFlight frames after the - // one that enqueues the readback. + // finished at latest after rendering QRhi::MaxAsyncReadbackFrames frames + // after the one that enqueues the readback. QVERIFY(readCompleted); QVERIFY(readbackWidth > 0);