From 645dd96f69915dbf3c2008659e8a3360c30b09d9 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 22 Sep 2026 20:02:07 -0400 Subject: [PATCH] [MG_Backend, MG_IntegrationTest] (P7): a flipped multisample depth or stencil resolve lands mirrored and a scaled one declines, and neither ends the session ID-P7-31 review, should-fix 2. ResolveWireDepthStencil required sx1 > sx0, sy1 > sy0 and identical signed spans, and took the session with Magma:multisample-depth-resolve-region otherwise - so glBlitFramebuffer(0, h, w, 0, 0, 0, w, h, COLOR|DEPTH) out of a multisample framebuffer resolved its colour correctly and then died on its depth. GL 4.6 core 18.3.1 asks for identical DIMENSIONS, not identical corners. * spans are signed; everything downstream (renderArea, the shader arm's rectangle, the buffer copy offsets, BlitDepthAcrossFormats' corners) works off the min corner; * a SCALE is INVALID_OPERATION in 18.3.1 and now declines by rule I (a) with the colour arm's three lines (MGLOG_E_ONCE, RecordTextureCopyError, return); * a same-format Y mirror is done in the copy-out: one VkBufferImageCopy per row, buffer row r written to destination row dyMin + height - 1 - r; * an X mirror (a region per texel is not a copy) and a Y mirror across two formats (BlitDepthAcrossFormats, shared with the monolith, has no flip) decline by name. The region Fatal stays for a rectangle outside either image. Scenario DepthStencilReadbackMatrixScenario.AFlippedMultisampleResolveMirrorsTheBandsAnd- AScaleDeclines, registered as MsFlip. on all three magma arms (no knob): unequal depth bands, COLOR|DEPTH in one flipped call with colour and depth asserted the same way up, then a scaled depth blit that must be INVALID_OPERATION and leave the destination alone. Red once (R-16, executed and reverted, split and spawn each): * the old guard restored -> both arms die by Fatal{UnmigratedVerb, "Magma:multisample-depth-resolve-region"}; * the scale decline turned back into that Fatal -> the flip half passes, both arms die at the scale; * the per-row copy-out bypassed (if (!mirrorY) forced true) -> both arms live and fail "flipped resolve: the destination's bottom band: 768 of 768 depth values differ from 0.75; first bad value 0.25" (and the top band the other way). --- .../DirectVulkan/Renderer/WireFramebuffer.inc | 98 ++++++++++++++++--- MobileGL/MG_IntegrationTest/CMakeLists.txt | 11 +++ .../DepthStencilReadbackMatrixScenario.cpp | 93 ++++++++++++++++++ 3 files changed, 188 insertions(+), 14 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/WireFramebuffer.inc b/MobileGL/MG_Backend/DirectVulkan/Renderer/WireFramebuffer.inc index 1d96e605..486dcc9e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/WireFramebuffer.inc +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/WireFramebuffer.inc @@ -434,15 +434,65 @@ void VulkanRenderer::ResolveWireDepthStencil(WireImage source, WireImage destination, GLint sx0, GLint sy0, GLint sx1, GLint sy1, GLint dx0, GLint dy0, GLint dx1, GLint dy1) { - const Int64 width = static_cast(sx1) - sx0; - const Int64 height = static_cast(sy1) - sy0; - if (!width || !height) return; + // P7 wave 2-B2 (review round): A REVERSED RECTANGLE IS NOT A MALFORMED ONE. This guard + // used to require `sx1 > sx0` and `sy1 > sy0` and take the session with a named Fatal + // otherwise - so `glBlitFramebuffer(0, h, w, 0, 0, 0, w, h, COLOR|DEPTH)` out of a + // multisample framebuffer blitted its COLOUR correctly (the colour arm has handled a + // flip since P5f and now handles a scale too) and then died on its DEPTH. GL 4.6 core + // 18.3.1 asks for identical DIMENSIONS, not identical corners - Mesa compares absolute + // spans - so a flip is a legal blit that has to produce a mirrored picture. + // + // Spans are therefore signed and everything downstream works off the min corner. + const Int64 spanX = static_cast(sx1) - sx0; + const Int64 spanY = static_cast(sy1) - sy0; + const Int64 dstSpanX = static_cast(dx1) - dx0; + const Int64 dstSpanY = static_cast(dy1) - dy0; + if (!spanX || !spanY || !dstSpanX || !dstSpanY) return; + const Int64 width = spanX < 0 ? -spanX : spanX; + const Int64 height = spanY < 0 ? -spanY : spanY; + // A SCALE has no arm here and is not supposed to: 18.3.1 makes a multisample blit whose + // rectangles differ in size INVALID_OPERATION. Rule I (a), the same three lines the + // colour arm's multisample-destination decline uses. + if (width != (dstSpanX < 0 ? -dstSpanX : dstSpanX) || + height != (dstSpanY < 0 ? -dstSpanY : dstSpanY)) { + MGLOG_E_ONCE("DirectVulkan::ResolveWireDepthStencil: a %lldx%lld source rectangle " + "cannot reach a %lldx%lld destination rectangle - a multisample resolve " + "carries no scale; declining the blit", + static_cast(width), static_cast(height), + static_cast(dstSpanX < 0 ? -dstSpanX : dstSpanX), + static_cast(dstSpanY < 0 ? -dstSpanY : dstSpanY)); + RecordTextureCopyError("ResolveWireDepthStencil", ErrorCode::InvalidOperation, + "glBlitFramebuffer from a multisampled read framebuffer requires " + "identical source and destination rectangle dimensions."); + return; + } + const Bool mirrorX = (spanX < 0) != (dstSpanX < 0); + const Bool mirrorY = (spanY < 0) != (dstSpanY < 0); + const GLint sxMin = std::min(sx0, sx1), syMin = std::min(sy0, sy1); + const GLint dxMin = std::min(dx0, dx1), dyMin = std::min(dy0, dy1); if (source.isDefault || destination.isDefault || source.is3D || destination.is3D || - width <= 0 || height <= 0 || width != static_cast(dx1) - dx0 || - height != static_cast(dy1) - dy0 || sx0 < 0 || sy0 < 0 || dx0 < 0 || dy0 < 0 || - static_cast(sx1) > source.extent.width || static_cast(sy1) > source.extent.height || - static_cast(dx1) > destination.extent.width || static_cast(dy1) > destination.extent.height) + sxMin < 0 || syMin < 0 || dxMin < 0 || dyMin < 0 || + static_cast(sxMin + width) > source.extent.width || + static_cast(syMin + height) > source.extent.height || + static_cast(dxMin + width) > destination.extent.width || + static_cast(dyMin + height) > destination.extent.height) MagmaWireFatal("multisample-depth-resolve-region"); + // WHICH MIRRORS HAVE AN ARM. The copy-out below moves the resolved aspect through a + // buffer, and a buffer copy addresses ROWS: one VkBufferImageCopy per row, walked + // backwards, is a vertical mirror for free. A HORIZONTAL one would need a region per + // TEXEL, which is not a copy any more - and re-encoding across formats + // (BlitDepthAcrossFormats, shared with the monolith arm) has no flip either. Both + // decline by name; ยง12 in notes/p7/magma-b2.md carries them. + if (mirrorX || (mirrorY && source.format != destination.format)) { + MGLOG_E_ONCE("DirectVulkan::ResolveWireDepthStencil: a multisample resolve mirrored on " + "%s (source format %d, destination format %d) has no arm - the copy-out " + "reverses whole rows and nothing smaller; declining the blit", + mirrorX ? "X" : "Y across two formats", + static_cast(source.format), static_cast(destination.format)); + RecordTextureCopyError("ResolveWireDepthStencil", ErrorCode::InvalidOperation, + "This device cannot mirror that multisample depth/stencil blit."); + return; + } VkImageAspectFlags formatAspects = VK_IMAGE_ASPECT_DEPTH_BIT; if (source.format == VK_FORMAT_S8_UINT) formatAspects = VK_IMAGE_ASPECT_STENCIL_BIT; else if (source.format == VK_FORMAT_D16_UNORM_S8_UINT || source.format == VK_FORMAT_D24_UNORM_S8_UINT || @@ -554,10 +604,10 @@ VkRenderPassBeginInfo begin{VK_STRUCTURE_TYPE_RENDER_PASS_BEGIN_INFO}; begin.renderPass = pass; begin.framebuffer = framebuffer; - begin.renderArea = {{sx0, sy0}, {static_cast(width), static_cast(height)}}; + begin.renderArea = {{sxMin, syMin}, {static_cast(width), static_cast(height)}}; vkCmdBeginRenderPass(m_frameContext.GetCurrent().commandBuffer, &begin, VK_SUBPASS_CONTENTS_INLINE); vkCmdEndRenderPass(m_frameContext.GetCurrent().commandBuffer); - } else if (!ResolveWireDepthStencilWithShader(source, resolved, source.aspect, sx0, sy0, + } else if (!ResolveWireDepthStencilWithShader(source, resolved, source.aspect, sxMin, syMin, static_cast(width), static_cast(height))) { // NEITHER ARM, and rule I (a) says what that is: this device has no // VK_KHR_depth_stencil_resolve and the pass cannot stand in for it either - the @@ -612,7 +662,7 @@ MagmaWireFatal("multisample-depth-resolve-buffer"); VkBufferImageCopy copy{}; copy.imageSubresource = {source.aspect, 0, 0, 1}; - copy.imageOffset = {sx0, sy0, 0}; + copy.imageOffset = {sxMin, syMin, 0}; copy.imageExtent = {static_cast(width), static_cast(height), 1}; vkCmdCopyImageToBuffer(frame.commandBuffer, resolved.image, VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL, aspectBuffer->GetHandle(), 1, ©); @@ -625,16 +675,36 @@ vkCmdPipelineBarrier(frame.commandBuffer, VK_PIPELINE_STAGE_TRANSFER_BIT, VK_PIPELINE_STAGE_TRANSFER_BIT, 0, 0, nullptr, 1, &barrier, 0, nullptr); copy.imageSubresource = {source.aspect, destination.level, destination.layer, 1}; - copy.imageOffset = {dx0, dy0, 0}; - vkCmdCopyBufferToImage(frame.commandBuffer, aspectBuffer->GetHandle(), destination.image, - VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, 1, ©); + copy.imageOffset = {dxMin, dyMin, 0}; + if (!mirrorY) { + vkCmdCopyBufferToImage(frame.commandBuffer, aspectBuffer->GetHandle(), destination.image, + VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, 1, ©); + } else { + // THE VERTICAL MIRROR, and it costs one region per row rather than a pass. The + // buffer holds the resolved band bottom row first (the image copy above wrote it + // tightly packed from syMin upwards); writing row r of that buffer to + // destination row dyMin + height - 1 - r reverses the band. bufferRowLength / + // bufferImageHeight stay 0: each region describes exactly one tight row, so the + // default "packed to imageExtent" is what is wanted. + Vector rows(static_cast(height), copy); + const VkDeviceSize rowBytes = static_cast(width) * texelSize; + for (Int64 row = 0; row < height; ++row) { + auto& region = rows[static_cast(row)]; + region.bufferOffset = static_cast(row) * rowBytes; + region.imageOffset = {dxMin, static_cast(dyMin + height - 1 - row), 0}; + region.imageExtent = {static_cast(width), 1, 1}; + } + vkCmdCopyBufferToImage(frame.commandBuffer, aspectBuffer->GetHandle(), destination.image, + VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, + static_cast(rows.size()), rows.data()); + } } else { // A logical GL depth format may use different native renderbuffer // and texture formats. Re-encode only after producing real resolved // samples. Keep all scratch objects local across this helper's flush. if (!BlitDepthAcrossFormats(frame, resolved.image, resolved.format, &scratch->layout, 0, 0, destination.image, destination.format, destination.trackedLayout, destination.level, destination.layer, - sx0, sy0, dx0, dy0, static_cast(width), static_cast(height), + sxMin, syMin, dxMin, dyMin, static_cast(width), static_cast(height), VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, source.aspect == VK_IMAGE_ASPECT_STENCIL_BIT)) MagmaWireFatal("multisample-depth-resolve-conversion"); diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index c44a589a..859a10e2 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -3206,6 +3206,17 @@ if (MOBILEGL_BUILD_DISAGGREGATED) ${mglItestArmProperties} ENVIRONMENT "${mglItestMagmaWireEnvironment}\;MGITEST_MAGMA_FORCE_SHADER_DEPTH_RESOLVE=${shaderDepthResolve}\;MOBILEGL_LOG_FILE_PATH=${CMAKE_CURRENT_BINARY_DIR}/p7-msresolve${mglItestArmLogTag}-shader${shaderDepthResolve}.log") endforeach() + # P7 wave 2-B2 (review round): a FLIPPED multisample depth/stencil resolve is a legal blit + # that used to end the session, and a SCALED one is INVALID_OPERATION that used to end it + # too. Both shapes are decided on the rectangles, before any capability question, so this + # entry needs NO knob and runs on all three arms. + gtest_discover_tests(MobileGLIntegrationTest + TEST_PREFIX "DirectVulkan.${mglItestArm}.MsFlip." + TEST_FILTER "DepthStencilReadbackMatrixScenario.AFlippedMultisampleResolveMirrorsTheBandsAndAScaleDeclines" + DISCOVERY_TIMEOUT 30 + PROPERTIES LABELS "${mglItestMagmaLabels}" TIMEOUT ${MGL_ITEST_TIMEOUT} + ${mglItestArmProperties} + ENVIRONMENT "${mglItestMagmaWireEnvironment}\;MOBILEGL_LOG_FILE_PATH=${CMAKE_CURRENT_BINARY_DIR}/p7-msflip${mglItestArmLogTag}.log") foreach(inPlaceCase CopyImageInPlaceAcrossLevelsAndLayers CopyImageInPlaceOverlapDeclinesAndLeavesTheLevelAlone) gtest_discover_tests(MobileGLIntegrationTest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackMatrixScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackMatrixScenario.cpp index afdd2f0e..369c4998 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackMatrixScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackMatrixScenario.cpp @@ -35,6 +35,7 @@ // only checked "no GL error" would pass against a readback that never touched the buffer, // which is precisely how this whole cluster hid for so long. +#include #include #include #include @@ -404,6 +405,98 @@ namespace MGITest { Gl().EndFrame(); } + // P7 wave 2-B2 (review round): A MULTISAMPLE DEPTH RESOLVE THAT FLIPS, AND ONE THAT SCALES. + // + // GL 4.6 core 18.3.1 asks a multisample blit for identical rectangle DIMENSIONS, not for + // identical corners - Mesa compares absolute spans - so `glBlitFramebuffer(0, h, w, 0, ...)` + // out of a multisample framebuffer is legal and owes a mirrored picture, while a blit whose + // sizes differ is INVALID_OPERATION and owes nothing. The wire arm used to end the SESSION + // on both: `Magma:multisample-depth-resolve-region` covered the reversed rectangle and the + // scaled one under one Fatal. + // + // COLOUR AND DEPTH IN ONE CALL, because that is the shape that made this visible: the colour + // arm has handled a flip since P5f and handles a scale as of this package, so + // GL_COLOR_BUFFER_BIT|GL_DEPTH_BUFFER_BIT resolved its colour correctly and then died on its + // depth. Asserting both aspects of the same blit is what pins them together. + // + // THE BANDS ARE UNEQUAL ON PURPOSE (0.25 bottom / 0.75 top): a flip that did not happen + // returns the bands the right way up, and a "flip" that merely reordered the readback would + // move the colour too - so colour and depth are checked to agree about which way up they are. + TEST_F(DepthStencilReadbackMatrixScenario, AFlippedMultisampleResolveMirrorsTheBandsAndAScaleDeclines) { + if (!Ready()) return; + DepthSource multisampled = MakeRenderbufferSource(GL_DEPTH24_STENCIL8, 4); + if (!SourceIsUsable()) { + DestroySource(multisampled); + GTEST_SKIP() << "this driver cannot host a 4x multisample DEPTH24_STENCIL8 renderbuffer"; + } + FirstGLError(); + glDisable(GL_SCISSOR_TEST); + glViewport(0, 0, kWidth, kHeight); + glDepthMask(GL_TRUE); + glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE); + glEnable(GL_SCISSOR_TEST); + glScissor(0, 0, kWidth, kHeight / 2); + glClearDepth(0.25); + glClearColor(1, 0, 0, 1); + glClear(GL_DEPTH_BUFFER_BIT | GL_COLOR_BUFFER_BIT); + glScissor(0, kHeight / 2, kWidth, kHeight - kHeight / 2); + glClearDepth(0.75); + glClearColor(0, 1, 0, 1); + glClear(GL_DEPTH_BUFFER_BIT | GL_COLOR_BUFFER_BIT); + glDisable(GL_SCISSOR_TEST); + ASSERT_EQ(FirstGLError(), 0u) << "banding the multisample source"; + + DepthSource resolved = MakeTextureSource(GL_DEPTH24_STENCIL8); + ASSERT_TRUE(SourceIsUsable()); + ClearDepthStencil(GL_DEPTH24_STENCIL8, 0.5f, 3); + glClearColor(0, 0, 1, 1); + glClear(GL_COLOR_BUFFER_BIT); + ASSERT_EQ(FirstGLError(), 0u) << "priming the resolve destination"; + + // Y REVERSED ON THE SOURCE SIDE, identical dimensions. + glBindFramebuffer(GL_READ_FRAMEBUFFER, multisampled.fbo); + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, resolved.fbo); + glDisable(GL_SCISSOR_TEST); + glBlitFramebuffer(0, kHeight, kWidth, 0, 0, 0, kWidth, kHeight, + GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT, GL_NEAREST); + glFinish(); + EXPECT_EQ(FirstGLError(), 0u) << "a flipped multisample resolve is a legal blit"; + + glBindFramebuffer(GL_FRAMEBUFFER, resolved.fbo); + // The bands come back swapped: the destination's bottom rows hold what was the top. + const std::vector bottom = ReadDepthFloat(0, 0, kWidth, kHeight / 4); + EXPECT_EQ(FirstGLError(), 0u); + ExpectAllDepth(bottom, 0.75f, "flipped resolve: the destination's bottom band"); + const std::vector top = ReadDepthFloat(0, kHeight - kHeight / 4, kWidth, kHeight / 4); + EXPECT_EQ(FirstGLError(), 0u); + ExpectAllDepth(top, 0.25f, "flipped resolve: the destination's top band"); + // Colour went the same way up, through the arm that could already do this. + std::array colour{}; + glReadPixels(kWidth / 2, 1, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, colour.data()); + EXPECT_EQ(colour, (std::array{0, 255, 0, 255})) + << "flipped resolve: colour and depth must agree about which way up the blit landed"; + + // A SCALE, which is INVALID_OPERATION and must leave the destination as it is. + ClearDepthStencil(GL_DEPTH24_STENCIL8, 0.5f, 3); + ASSERT_EQ(FirstGLError(), 0u); + glBindFramebuffer(GL_READ_FRAMEBUFFER, multisampled.fbo); + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, resolved.fbo); + glBlitFramebuffer(0, 0, kWidth, kHeight, 0, 0, kWidth / 2, kHeight / 2, + GL_DEPTH_BUFFER_BIT, GL_NEAREST); + glFinish(); // the decline's error rides a later reply under a transport + EXPECT_EQ(FirstGLError(), GLenum(GL_INVALID_OPERATION)) + << "a scaled multisample resolve is INVALID_OPERATION, not a picture"; + glBindFramebuffer(GL_FRAMEBUFFER, resolved.fbo); + ExpectAllDepth(ReadDepthFloat(0, 0, kWidth, kHeight), 0.5f, + "the declined scale must have left the destination alone"); + EXPECT_EQ(FirstGLError(), 0u) << "the session survived the decline"; + + DestroySource(resolved); + DestroySource(multisampled); + glBindFramebuffer(GL_FRAMEBUFFER, 0); + Gl().EndFrame(); + } + // A read whose rectangle is NOT the whole attachment. The staging copy has to carry // the requested rect (not the origin) and hand back its rows bottom-up, which a // full-extent uniform read is a fixed point of and therefore cannot see.