diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index cdc368f8..aa74065f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -15150,12 +15150,26 @@ void main() { VK_EXT_SHADER_STENCIL_EXPORT_EXTENSION_NAME); // P7 gate 5 (g5-msrbo): the Adreno's no-draw depth/stencil resolve pass writes nothing, // so the shader resolve goes first there (WireDepthResolveArm.h says why and where). + // + // THE VENDOR THE POLICY READS CAN BE SUBSTITUTED, AND ONLY HERE (review round). No host + // lane runs on a Qualcomm device, so the order this line picks for the Redmi ran nowhere + // a lane could see it: dropping this assignment, or the `m_wirePreferShaderDepthResolve ||` + // in ResolveWireDepthStencil, left every lane green and put + // KHR-GL46.direct_state_access.renderbuffers_storage_multisample back at 54 Fail lines on + // the device. MGITEST_MAGMA_DEPTH_RESOLVE_VENDOR_ID (strtoul base 0, so `0x5143`) replaces + // the id handed to WirePrefersShaderDepthResolve and nothing else - no other vendor + // check reads it - and the DirectVulkan.{Split,Spawn}.MsResolveQ./MsFlipQ. entries set it + // to Qualcomm's id and read ResolveWireDepthStencil's arm line back from the server log. + Uint32 depthResolvePolicyVendor = m_physicalDevice.properties.vendorID; + if (const char* substitute = std::getenv("MGITEST_MAGMA_DEPTH_RESOLVE_VENDOR_ID"); + substitute != nullptr && *substitute != '\0') + depthResolvePolicyVendor = static_cast(std::strtoul(substitute, nullptr, 0)); m_wirePreferShaderDepthResolve = MG_Config::Transport != MG_Config::TransportMode::Monolith && - WirePrefersShaderDepthResolve(m_physicalDevice.properties.vendorID); + WirePrefersShaderDepthResolve(depthResolvePolicyVendor); if (m_wirePreferShaderDepthResolve) - MGLOG_I("DirectVulkan: vendor 0x%x - the wire arm resolves multisample depth/stencil with the " - "shader pass first (the no-draw resolve render pass is the fallback)", - m_physicalDevice.properties.vendorID); + MGLOG_I("DirectVulkan: vendor 0x%x (policy vendor 0x%x) - the wire arm resolves multisample " + "depth/stencil with the shader pass first (the no-draw resolve render pass is the fallback)", + m_physicalDevice.properties.vendorID, depthResolvePolicyVendor); #endif if ((descriptorIndexingCore || descriptorIndexingExtension) && getPhysicalDeviceFeatures2 != nullptr && getPhysicalDeviceProperties2 != nullptr) { diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/WireFramebuffer.inc b/MobileGL/MG_Backend/DirectVulkan/Renderer/WireFramebuffer.inc index f1c3df46..6b91b9c7 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/WireFramebuffer.inc +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/WireFramebuffer.inc @@ -738,6 +738,37 @@ const Bool resolvedByShader = shaderFirst && ResolveWireDepthStencilWithShader(source, resolved, source.aspect, sxMin, syMin, static_cast(width), static_cast(height)); + // WHICH ARM RESOLVED IS SAID, ONCE PER ARM (review round). Lavapipe resolves correctly + // through both, so the pixels cannot tell a lane which one ran; these two lines can, and + // the MsResolveQ./MsFlipQ. entries (Qualcomm's arm order substituted on the host) and the + // MsResolve1./MsFlip1. ones (the extension arm dropped) read them back from the server + // log - DepthStencilReadbackMatrixScenario::ExpectTheShaderResolveArmWhereTheLaneAsks. + // Keep the two phrases in step with that reader. + // + // AND A PREFERENCE THAT FELL BACK IS NOT SILENT: where the shader arm goes first it goes + // first because the render pass is known to leave its target unwritten on that device, + // so a shader decline (a stencil aspect without VK_EXT_shader_stencil_export, a format + // that is not SAMPLED_IMAGE + DEPTH_STENCIL_ATTACHMENT, a failed pipeline create - + // WireMultisampleResolve.inc) that lands on the render pass is the resolve most likely + // to read back as zeroes with no GL error. A recorded debt (notes/p7/g5-msrbo.md ยง4). + if (resolvedByShader) { + MGLOG_I_ONCE("DirectVulkan::ResolveWireDepthStencil: resolved by the shader pass " + "(prefer-shader=%d, render-pass arm available=%d)", + m_wirePreferShaderDepthResolve ? 1 : 0, extensionResolve ? 1 : 0); + } else if (extensionResolve) { + if (m_wirePreferShaderDepthResolve) + MGLOG_W_ONCE("DirectVulkan::ResolveWireDepthStencil: this device prefers the shader pass, " + "which did not resolve aspect 0x%x of a %u-sample format %d (stencil export=%d, " + "source usage 0x%x); falling back to the no-draw VK_KHR_depth_stencil_resolve " + "render pass, which on the device that set the preference left its target " + "unwritten", + static_cast(source.aspect), static_cast(source.samples), + static_cast(source.format), m_wireShaderStencilExport ? 1 : 0, + static_cast(source.usageFlags)); + MGLOG_I_ONCE("DirectVulkan::ResolveWireDepthStencil: resolved by the " + "VK_KHR_depth_stencil_resolve render pass (prefer-shader=%d)", + m_wirePreferShaderDepthResolve ? 1 : 0); + } if (!resolvedByShader && extensionResolve) { TransitionWireImage(source, VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL); TransitionWireImage(resolved, VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL); diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 98524cd2..46c26927 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -3370,8 +3370,9 @@ if (MOBILEGL_BUILD_DISAGGREGATED) # depth/stencil format: the wire arm moved every single-sample depth/stencil aspect with # vkCmdBlitImage where the monolith copies at 1:1, and for one aspect of a packed format # that moved the whole native word (a depth-only blit replaced the destination's stencil - # on lavapipe). NO KNOB: the packed-aspect leg is red on every host without the copy arm, - # so all three arms carry it. + # on lavapipe). NO KNOB: the packed-aspect leg is red on lavapipe without the copy arm + # (measured there only - a driver whose one-aspect blit keeps the other aspect stays green + # without it), and nothing in it is read on the server, so all three arms carry it. gtest_discover_tests(MobileGLIntegrationTest TEST_PREFIX "DirectVulkan.${mglItestArm}.DsBlit." TEST_FILTER "DepthStencilReadbackMatrixScenario.ASingleSampledDepthStencilBlitCopiesEveryAspect" @@ -3404,6 +3405,42 @@ if (MOBILEGL_BUILD_DISAGGREGATED) ${mglItestArmProperties} ENVIRONMENT "${mglItestMagmaWireEnvironment}\;MGITEST_MAGMA_FORCE_SHADER_DEPTH_RESOLVE=1\;MOBILEGL_LOG_FILE_PATH=${CMAKE_CURRENT_BINARY_DIR}/p7-msflip${mglItestArmLogTag}-shader1.log") endif() + # ---- P7 gate 5 (g5-msrbo review round): THE QUALCOMM ARM ORDER, ON A HOST ------------- + # + # What made KHR-GL46.direct_state_access.renderbuffers_storage_multisample pass on the + # Redmi is an arm ORDER: on a Qualcomm device ResolveWireDepthStencil takes the shader + # pass first, because the Adreno's no-draw VK_KHR_depth_stencil_resolve render pass left + # its target unwritten. Lavapipe resolves correctly through both arms, so no pixel on this + # host can see the order, and without these entries dropping it went unnoticed by every + # lane. MGITEST_MAGMA_DEPTH_RESOLVE_VENDOR_ID substitutes the vendor id the server's arm + # policy reads (VulkanRenderer.cpp, next to m_wirePreferShaderDepthResolve) - nothing + # else reads it - and the two resolve cases then assert their pixels AND that the + # server's private log says the shader pass resolved and the render pass never did + # (DepthStencilReadbackMatrixScenario::ExpectTheShaderResolveArmWhereTheLaneAsks, which + # asserts the same of the MsResolve1./MsFlip1. entries above). + # + # Split+spawn only, the server-side-knob reason above, named in spawn_lane_parity.py's + # MAGMA_SERVER_ENV_KNOB_NO_TCP. + # + # Red once (executed, reverted): drop `m_wirePreferShaderDepthResolve ||` from + # ResolveWireDepthStencil's `shaderFirst` (WireFramebuffer.inc) and all four entries fail + # on the arm assertion with their pixels green. + if(mglItestServerEnvKnobArm) + foreach(qualcommOrderCase MsResolveQ:AResolvedMultisampleDepthReadsBackFromTheDestination + MsFlipQ:AFlippedMultisampleResolveMirrorsTheBandsAndAScaleDeclines) + string(REPLACE ":" ";" qualcommOrderPair "${qualcommOrderCase}") + list(GET qualcommOrderPair 0 qualcommOrderTail) + list(GET qualcommOrderPair 1 qualcommOrderTest) + string(TOLOWER "${qualcommOrderTail}" qualcommOrderLog) + gtest_discover_tests(MobileGLIntegrationTest + TEST_PREFIX "DirectVulkan.${mglItestArm}.${qualcommOrderTail}." + TEST_FILTER "DepthStencilReadbackMatrixScenario.${qualcommOrderTest}" + DISCOVERY_TIMEOUT 30 + PROPERTIES LABELS "${mglItestMagmaLabels}" TIMEOUT ${MGL_ITEST_TIMEOUT} + ${mglItestArmProperties} + ENVIRONMENT "${mglItestMagmaWireEnvironment}\;MGITEST_MAGMA_DEPTH_RESOLVE_VENDOR_ID=0x5143\;MOBILEGL_LOG_FILE_PATH=${CMAKE_CURRENT_BINARY_DIR}/p7-${qualcommOrderLog}${mglItestArmLogTag}.log") + endforeach() + endif() 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 32988588..d842eb54 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackMatrixScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/DepthStencilReadbackMatrixScenario.cpp @@ -38,12 +38,15 @@ #include #include #include +#include #include #include #include #include "../Harness/HeadlessGL.h" +#include "../Harness/PipeStatsWindow.h" #include "../Harness/ScenarioFixture.h" +#include "../Harness/SplitLane.h" #ifdef GLAPI #undef GLAPI @@ -227,6 +230,51 @@ namespace MGITest { ? " - which is the poison value, so nothing was written at all" : ""); } + + // P7 gate 5 (g5-msrbo review round): WHICH WIRE ARM RESOLVED, WHERE THE ENTRY SAYS + // WHICH ONE IT HAS TO BE. Lavapipe resolves a multisample depth/stencil aspect + // correctly through both of ResolveWireDepthStencil's arms, so the pixel assertions + // above cannot tell them apart - and the fix that made + // KHR-GL46.direct_state_access.renderbuffers_storage_multisample pass on the Redmi is + // exactly an arm ORDER (the shader pass first on Qualcomm, because the Adreno's no-draw + // resolve render pass leaves its target unwritten). Two kinds of entry name the arm: + // - MsResolveQ. / MsFlipQ. set MGITEST_MAGMA_DEPTH_RESOLVE_VENDOR_ID to Qualcomm's id, + // the one input the server's arm policy reads in place of the device's vendor; + // - MsResolve1. / MsFlip1. set MGITEST_MAGMA_FORCE_SHADER_DEPTH_RESOLVE=1, which + // drops the render-pass arm altogether. + // Either way the server must have resolved with the shader pass and never with the + // render pass. The claim is read off the SERVER's private log (the backend runs on the + // apply thread, the server role, on every arm), where ResolveWireDepthStencil says + // once per arm which one ran; keep the two phrases in step with WireFramebuffer.inc. + // Every other entry - the device's own vendor, whatever order that picks - asserts + // nothing here. + // + // Red once (executed, reverted): drop `m_wirePreferShaderDepthResolve ||` from + // ResolveWireDepthStencil's `shaderFirst` and both Q entries fail here on split and + // spawn with the render-pass line in the log, while their pixels stay green. + static void ExpectTheShaderResolveArmWhereTheLaneAsks(const char* what) { + const std::string vendor = SplitLane::MarkerValue("MGITEST_MAGMA_DEPTH_RESOLVE_VENDOR_ID"); + const bool qualcommOrder = !vendor.empty() && std::strtoul(vendor.c_str(), nullptr, 0) == 0x5143ul; + const bool renderPassDropped = SplitLane::MarkerIsOne("MGITEST_MAGMA_FORCE_SHADER_DEPTH_RESOLVE"); + if (!qualcommOrder && !renderPassDropped) return; + if (PipeStatsWindow::ServerLibraryLogPath().empty()) { + ADD_FAILURE() << what << ": the entry names the resolve arm (vendor=" << vendor + << ", force-shader=" << renderPassDropped + << ") but configured no MOBILEGL_LOG_FILE_PATH, and the server's log " + "is the only place the arm is visible"; + return; + } + glFinish(); + const std::string server = PipeStatsWindow::ReadServerLogSince(PipeStatsWindow::LogMark{}); + EXPECT_NE(server.find("ResolveWireDepthStencil: resolved by the shader pass"), std::string::npos) + << what << ": the server never resolved with the shader pass (vendor=" << vendor + << ", force-shader=" << renderPassDropped << ")"; + EXPECT_EQ(server.find("ResolveWireDepthStencil: resolved by the VK_KHR_depth_stencil_resolve render pass"), + std::string::npos) + << what << ": the server resolved with the no-draw render pass, the arm that leaves its " + "target unwritten on the Adreno (vendor=" << vendor + << ", force-shader=" << renderPassDropped << ")"; + } }; // ---- glReadPixels across the source kinds ----------------------------------- @@ -398,6 +446,7 @@ namespace MGITest { ExpectAllDepth(ReadDepthFloat(0, 0, kWidth, kHeight), 0.375f, "stencil-only resolve preserves destination depth"); EXPECT_EQ(FirstGLError(), 0u); + ExpectTheShaderResolveArmWhereTheLaneAsks("depth-only and stencil-only resolves"); DestroySource(resolved); DestroySource(multisampled); @@ -562,6 +611,9 @@ namespace MGITest { << " stencil values are wrong; first at (" << firstX << ", " << firstY << "): got " << firstGot << ", want " << firstWant; } + // The two flipped resolves above are the only ones in this case that reach an arm: every + // leg below declines on its shape first. + ExpectTheShaderResolveArmWhereTheLaneAsks("flipped depth and narrow stencil resolves"); // A SCALE, which is INVALID_OPERATION and must leave the destination as it is. ClearDepthStencil(GL_DEPTH24_STENCIL8, 0.5f, 3); @@ -693,8 +745,10 @@ namespace MGITest { // the monolith arm copies at 1:1 - and for ONE aspect of a packed format the blit moved the // whole native word. // - // THE SUB-RECTANGLE LEG is the red one without the copy arm, on any host: a depth-only blit - // of a packed DEPTH24_STENCIL8 replaced the destination's stencil on lavapipe. It also pins + // THE SUB-RECTANGLE LEG is the red one without the copy arm on lavapipe (the only host it was + // measured on): a depth-only blit of a packed DEPTH24_STENCIL8 replaced the destination's + // stencil there. A driver whose one-aspect blit keeps the other aspect stays green without + // the copy arm, so this leg is a lavapipe pin, not a universal one. It also pins // the min corners on both sides and a 1-byte stencil row whose width is not a multiple of 4 // (the buffer round trip's padded stride). THE SCALED LEG is the shape that still takes // vkCmdBlitImage, so the copy arm must not have swallowed it. diff --git a/scripts/ci/spawn_lane_parity.py b/scripts/ci/spawn_lane_parity.py index 7d00d8e7..1cf15c8a 100644 --- a/scripts/ci/spawn_lane_parity.py +++ b/scripts/ci/spawn_lane_parity.py @@ -85,8 +85,13 @@ MAGMA_INPROC_ONLY = ("MagmaRunAheadScenario.", "MagmaWireCacheScenario.") # private log with the stats channel and MOBILEGL_IPC_WIRE_DEFERRED_MB=8 in the entry's environment - # server-process environment again, so the same two arms and the same tcp absence. Its three cases # are three tails (one key each) under the `.Reclaim.` prefix. +# P7 gate 5 (g5-msrbo review round): `.MsResolveQ.` / `.MsFlipQ.` set +# MGITEST_MAGMA_DEPTH_RESOLVE_VENDOR_ID, which the SERVER's depth/stencil resolve arm policy reads +# in place of the device's vendor id - the knob-free `.MsResolve0.` / `.MsFlip.` keep their tcp +# entries. MAGMA_SERVER_ENV_KNOB_NO_TCP = (".ShaderMip1.", ".ShaderMip2.", ".DepthMip.", ".DefaultBlitShape1.", ".MsResolve1.", ".StaleSerial.", ".MsFlip1.", + ".MsResolveQ.", ".MsFlipQ.", ".Reclaim.MagmaWireReclaimScenario.RespecifiesWithNoDrawBetweenKeepTheLiveStoreCountBounded", ".Reclaim.MagmaWireReclaimScenario.RespecifyAndDrawEachStoreInOneFrameStaysWithinTheDeferredBudget", ".Reclaim.MagmaWireReclaimScenario.ManySmallRespecifyAndDrawRoundsStayUnderTheStoreCountCeiling",