[Fix, Test] (MG_State, DirectVulkan): order host writes to an adopted store after recorded GPU work - a SubData issued after a dispatch landed in coherent memory before the deferred dispatch executed, so its increments overwrote the newer bytes; un-skip the DirectVulkan half of the SubData-after-dispatch scenario

This commit is contained in:
2026-08-28 03:01:15 -04:00
parent 08419a1fe6
commit ff426da3a9
2 changed files with 31 additions and 17 deletions
@@ -226,25 +226,21 @@ void main() {
} }
// A CPU glBufferSubData issued AFTER a dispatch, read back with NO further GPU work in // A CPU glBufferSubData issued AFTER a dispatch, read back with NO further GPU work in
// between. The DirectGLES backend queues app SubData ranges for the draw-time staged-copy // between. Each backend has its own way to invert this pair, and both are pinned here.
// flush (the upload ring) instead of uploading in place, and readback of a GPU-written // DirectGLES queues app SubData ranges for the draw-time staged-copy flush (the upload
// buffer overwrites the frontend shadow with the driver copy - so if the readback path // ring) instead of uploading in place, and readback of a GPU-written buffer overwrites
// forgets to flush the queued range first, the newer CPU write is REVERTED by the readback // the frontend shadow with the driver copy - so if the readback path forgets to flush the
// and offset 0 reads the dispatch's value instead of the reseed. Offset 4 pins the other // queued range first, the newer CPU write is REVERTED by the readback and offset 0 reads
// direction: the flush must not clobber GPU results outside the written range. // the dispatch's value instead of the reseed. DirectVulkan adopts the buffer into
// coherent GPU memory the moment the dispatch resolves its descriptor, so the SubData
// write lands in the very bytes the GPU reads - while the dispatch still sits recorded in
// the deferred frame command buffer. Unless the frontend retires that pending work before
// writing the adopted store (BufferObject::UploadSubData), the dispatch executes ON TOP
// of the reseed and offset 0 reads reseed + increments instead of the reseed. Offset 4
// pins the other direction for both: the upload must leave bytes outside its range - the
// dispatch's results - untouched.
TEST_F(AtomicCounterScenario, SubDataAfterDispatchSurvivesAnImmediateReadback) { TEST_F(AtomicCounterScenario, SubDataAfterDispatchSurvivesAnImmediateReadback) {
if (!Ready() || IsSkipped()) return; if (!Ready() || IsSkipped()) return;
// DirectGLES-only for now. DirectVulkan fails this case with or without the upload
// ring, on revisions that predate it: its buffer uploads submit immediately while the
// dispatch sits in the deferred frame command buffer, so the GPU increments the
// RESEEDED value (reads 4242 + increments instead of 4242) - a pre-existing
// upload-vs-recorded-work ordering gap in that backend, kept visible here rather than
// silently absorbed. Un-skip once DirectVulkan orders app uploads against already
// recorded GPU work.
if (Gl().BackendName() != std::string("DirectGLES")) {
GTEST_SKIP() << "SubData-after-dispatch ordering is a known DirectVulkan gap; this case pins the "
"DirectGLES readback pre-flush only";
}
const GLuint zero = MakeCounterBuffer(0, {0u, 0u}); const GLuint zero = MakeCounterBuffer(0, {0u, 0u});
MakeCounterBuffer(1, {0u}); MakeCounterBuffer(1, {0u});
@@ -253,6 +253,18 @@ namespace MobileGL::MG_State::GLState {
"UploadSubData out of bounds: atOffset (%zu) + data.size (%zu) > m_size (%zu)", atOffset, "UploadSubData out of bounds: atOffset (%zu) + data.size (%zu) > m_size (%zu)", atOffset,
data.size, m_size); data.size, m_size);
// An adopted store's Bytes() IS the memory the GPU reads, and a backend that
// defers work (DirectVulkan's frame command buffer) may still be holding a
// recorded-but-unsubmitted dispatch that GL orders this write AFTER. Writing
// the mapping now would land the bytes underneath that dispatch - its
// increments then execute on top of the newer data and invert the call order.
// Retire the pending GPU writes first, as FillSubData already does. Shadow-
// backed stores need none of this: the Memcpy below touches only the shadow,
// and the backend's SubData op does its own ordering against in-flight work.
if (m_resource.IsGpuResident()) {
SyncGpuWrites();
}
Memcpy(m_resource.Bytes() + atOffset, data.data, data.size); Memcpy(m_resource.Bytes() + atOffset, data.data, data.size);
NotifyContentWrite(atOffset, data.size); NotifyContentWrite(atOffset, data.size);
} }
@@ -305,6 +317,12 @@ namespace MobileGL::MG_State::GLState {
size, m_size); size, m_size);
src->SyncGpuWrites(); src->SyncGpuWrites();
// The DESTINATION needs the same ordering as UploadSubData: an adopted store is
// written in place, so pending recorded GPU writes to it must retire before the
// copy lands or they would execute on top of it.
if (m_resource.IsGpuResident()) {
SyncGpuWrites();
}
Memcpy(m_resource.Bytes() + dstOffset, src->m_resource.Bytes() + srcOffset, size); Memcpy(m_resource.Bytes() + dstOffset, src->m_resource.Bytes() + srcOffset, size);
NotifyContentWrite(dstOffset, size); NotifyContentWrite(dstOffset, size);
} }