From c639e17514181d498a6d29529d7967f86c2d71f3 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Mon, 12 Oct 2026 05:51:14 -0400 Subject: [PATCH] [Fix] (Espryt): barrier re-uploads into image-accessed textures ahead of the dispatch that reads them --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 12 +++++ MobileGL/MG_Backend/DirectGLES/Managers.cpp | 16 +++++++ MobileGL/MG_Backend/DirectGLES/Managers.h | 22 +++++++++ .../PartialUploadOverGpuContentScenario.cpp | 45 +++++++++++++++++++ 4 files changed, 95 insertions(+) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index aa05db78..2d0d0634 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -3094,6 +3094,12 @@ namespace MobileGL::MG_Backend::DirectGLES { return; } SyncImageTextureBindings(); + // The sweep may just have landed a re-upload into a texture the draw reaches through + // an image unit (the content clause above is one of the ways it gets here). + if (const GLbitfield barriers = TakeInPlaceTextureUploadBarrierBits(); + barriers != 0 && g_GLESFuncs.glMemoryBarrier) { + g_GLESFuncs.glMemoryBarrier(barriers); + } g_imageSweepShaderImagesSerial = st.ShaderImagesSerial; g_imageSweepTextureShutterSerial = st.TextureShutterSerial; g_imageSweepContextSerial = st.ContextSerial; @@ -6217,6 +6223,12 @@ namespace MobileGL::MG_Backend::DirectGLES { TextureImpl::SyncNeccessaryTextures(textureKeys); TextureImpl::SyncImageTextureBindings(); TextureImpl::MarkWritableImageBufferTexturesGpuWritten(); + // Both syncs above may have re-uploaded a texture this dispatch reads or stores + // (Managers.h, NoteInPlaceTextureUpload). + { + GLbitfield barriers = TextureImpl::TakeInPlaceTextureUploadBarrierBits(); + if (barriers != 0 && g_GLESFuncs.glMemoryBarrier) g_GLESFuncs.glMemoryBarrier(barriers); + } // P5e (pg), kimi row 92: the DISPATCH half of the same retirement - // MGPipeApplier().DispatchProgram, which set_dispatch_program carried. A compute-only // pipeline has no draw program at all, which is why the two applier fields are separate. diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 197cfbce..93e5e096 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -6203,6 +6203,20 @@ namespace MobileGL::MG_Backend::DirectGLES { return reinterpret_cast(static_cast(offset)); } + // See Managers.h. One flag for every context, like the image-unit high-water mark: a + // dispatch on another context can consume an upload's flag before the uploading + // context's own dispatch sees it - a hole only a shared texture re-uploaded on one context + // and image-accessed on another can fall into, and GL wants a fence between those anyway. + static Bool g_inPlaceTextureUploadPending = false; + + void NoteInPlaceTextureUpload() { g_inPlaceTextureUploadPending = true; } + + GLbitfield TakeInPlaceTextureUploadBarrierBits() { + if (!g_inPlaceTextureUploadPending) return 0; + g_inPlaceTextureUploadPending = false; + return GL_TEXTURE_UPDATE_BARRIER_BIT | GL_TEXTURE_FETCH_BARRIER_BIT | GL_SHADER_IMAGE_ACCESS_BARRIER_BIT; + } + static Uint GetNormFallbackComponentCount(TextureInternalFormat format) { switch (format) { case TextureInternalFormat::R8Snorm: @@ -8420,6 +8434,8 @@ namespace MobileGL::MG_Backend::DirectGLES { if (ringStaged) { BufferImpl::BindPixelUnpackBufferId(BufferImpl::UnpackRingBufferId()); } + // Ring-sourced or not: see NoteInPlaceTextureUpload. + NoteInPlaceTextureUpload(); if (MG_Util::PipeStats::Enabled()) { // One emission per (upload target, level) that ships texels; // the switch below turns it into either one union-box job or diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 0239d318..11262529 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -2076,6 +2076,28 @@ namespace MobileGL::MG_Backend::DirectGLES { Bool FollowCopyImageInStagedStore(MG_Pipe::MGPipeHandle source, Uint32 sourceLevel, const IntVec3& sourceOrigin, MG_Pipe::MGPipeHandle destination, Uint32 destinationLevel, const IntVec3& destinationOrigin, const IntVec3& size); + // A re-upload into storage the driver already holds is a copy the driver may run on a path + // of its own that the next shader's image loads and stores do not wait for. On Adreno, a + // compute dispatch that imageStore()s over a texture whose re-upload the same Prepare had + // just issued read back with up to 2.4k of 4096 texels wrong, while the same dispatch over + // freshly created textures was exact (KHR-GL43.compute_shader.resource-image). The two + // differ in exactly two ways: the fresh ones are allocated and filled from client memory, + // the re-uploads go through the unpack ring as a buffer-to-texture copy; and the fresh + // ones were never used by the GPU. Both are covered - every in-place level upload is + // noted, ring-sourced or not. + // + // GL orders a texture update ahead of every later shader access without a barrier, so + // the application has nothing to fix this with - and its own barriers could not reach it + // anyway: the upload is applied lazily, at the dispatch that consumes it, after any + // barrier the application issued. So the shader-access sites that can see such a texture + // through an image unit - every dispatch, and a draw whose image units were just + // re-synced - issue ONE barrier ahead of the shader, and only when an in-place upload was + // issued since the last one. Sampled access from a draw does not take it: that is the + // order every driver keeps on its own, and it is the per-draw hot path. + void NoteInPlaceTextureUpload(); + // The barrier bits owed for the uploads noted since the last call (0 when none), and + // the note cleared: the caller issues them, folded into any barrier of its own. + GLbitfield TakeInPlaceTextureUploadBarrierBits(); // Brings every texture the next draw reads - the touched units' bindings and the draw // FBO's texture attachments - onto the backend, through the two borrowed-pair memos // documented at their definitions. Declared here so tests can drive those memos directly. diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PartialUploadOverGpuContentScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PartialUploadOverGpuContentScenario.cpp index 9e4ce235..0def94a5 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/PartialUploadOverGpuContentScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/PartialUploadOverGpuContentScenario.cpp @@ -614,6 +614,51 @@ void main() { DestroyColorFbo(target); } + // The dispatch form of the case above, which is KHR-GL43.compute_shader.resource-image's + // shape: every pass re-uploads into the SAME image-bound texture and a dispatch stores + // over all of it. The upload is applied at the dispatch's own Prepare, after any barrier + // the application issued, so it is the backend that has to order it ahead of the stores. + TEST_F(PartialUploadOverGpuContentScenario, SubImageBeforeAnImageStoreDispatchLandsUnderTheStores) { + if (!Ready()) return; + const GLuint t = NewTexture(GL_TEXTURE_2D); + glBindTexture(GL_TEXTURE_2D, t); + glTexStorage2D(GL_TEXTURE_2D, 1, GL_RGBA8, kEdge, kEdge); + glBindTexture(GL_TEXTURE_2D, 0); + const char* computeSource = + "#version 430 core\n" + "layout(local_size_x = 8, local_size_y = 8) in;\n" + "layout(rgba8, binding = 0) writeonly uniform image2D u_img;\n" + "void main() { imageStore(u_img, ivec2(gl_GlobalInvocationID.xy), vec4(0.0, 1.0, 0.0, 1.0)); }\n"; + const GLuint shader = glCreateShader(GL_COMPUTE_SHADER); + glShaderSource(shader, 1, &computeSource, nullptr); + glCompileShader(shader); + const GLuint program = glCreateProgram(); + glAttachShader(program, shader); + glLinkProgram(program); + glDeleteShader(shader); + m_programs.push_back(program); + GLint linked = GL_FALSE; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + ASSERT_EQ(linked, GL_TRUE) << "compute program did not link"; + glBindImageTexture(0, t, 0, GL_FALSE, 0, GL_WRITE_ONLY, GL_RGBA8); + ASSERT_EQ(FirstGLError(), 0u) << "setup"; + const auto red = Fill(Rgba8(), kEdge * kEdge, 1.0f, 0.0f, 0.0f); + for (int pass = 0; pass < 3; ++pass) { + SCOPED_TRACE(::testing::Message() << "pass " << pass); + glBindTexture(GL_TEXTURE_2D, t); + glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, kEdge, kEdge, GL_RGBA, GL_UNSIGNED_BYTE, red.data()); + glBindTexture(GL_TEXTURE_2D, 0); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + glUseProgram(program); + glDispatchCompute(1, 1, 1); + glUseProgram(0); + glMemoryBarrier(GL_ALL_BARRIER_BITS); + ASSERT_EQ(FirstGLError(), 0u) << "dispatch"; + ExpectRgba8(t, false); + } + glBindImageTexture(0, 0, 0, GL_FALSE, 0, GL_READ_ONLY, GL_RGBA8); + } + // Two boxes in opposite corners: their union box is 7x7, of which the writes are 8 texels. TEST_F(PartialUploadOverGpuContentScenario, TwoDistantSubImagesKeepTheGpuClearBetweenThem) { if (!Ready()) return;