diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 8277fc75..f65cd41c 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -15856,8 +15856,9 @@ void main() { // P7 gate 5 (g5-msprobe): THE ARM ORDER OF THE MULTISAMPLE DEPTH/STENCIL RESOLVE IS A MEASURED // PROPERTY OF THIS DEVICE. WireDepthResolveArm.h says why (the Adreno 830's no-draw // VK_KHR_depth_stencil_resolve pass writes nothing), WireDepthResolveProbe.h what is run. The - // answer is a device property, so it is memoized per process like the PRIMITIVES_GENERATED - // probe's, and logged once with every reading - the line the device evidence quotes. + // answer is a device property, so it is memoized per DEVICE IDENTITY (WireDepthResolveArmCache: + // vendor, device, driver version, pipeline-cache UUID - codex closeout finding 7), and logged once + // per identity with every reading - the line the device evidence quotes. // // THE MEASUREMENT CAN BE REPLACED, AND ONLY HERE: MGITEST_MAGMA_DEPTH_RESOLVE_PROBE=bug|clean // hands ChooseWireDepthResolveArm a canned measurement instead of running the probe - no host @@ -15875,9 +15876,20 @@ void main() { // (the probe asks that per format): without it the shader pass is the only arm. const Bool renderPassArmAvailable = !MagmaWireForcedShaderDepthResolve() && m_wireCreateRenderPass2 && (m_wireDepthResolveModes & VK_RESOLVE_MODE_SAMPLE_ZERO_BIT) != 0; - static const WireDepthResolveArmChoice s_choice = [&]() { - const char* knobValue = std::getenv("MGITEST_MAGMA_DEPTH_RESOLVE_PROBE"); - const WireDepthResolveProbeKnob knob = ParseWireDepthResolveProbeKnob(knobValue); + const char* knobValue = std::getenv("MGITEST_MAGMA_DEPTH_RESOLVE_PROBE"); + const WireDepthResolveProbeKnob knob = ParseWireDepthResolveProbeKnob(knobValue); + // Memoized per DEVICE IDENTITY, not per process (codex closeout finding 7; see + // WireDepthResolveArmCache): a second renderer on another device or driver probes for itself. + WireDepthResolveDeviceIdentity identity; + identity.vendorID = m_physicalDevice.properties.vendorID; + identity.deviceID = m_physicalDevice.properties.deviceID; + identity.driverVersion = m_physicalDevice.properties.driverVersion; + std::memcpy(identity.pipelineCacheUUID, m_physicalDevice.properties.pipelineCacheUUID, + sizeof(identity.pipelineCacheUUID)); + identity.renderPassArmAvailable = renderPassArmAvailable; + identity.knob = knob; + static WireDepthResolveArmCache s_choices; + const WireDepthResolveArmChoice resolved = s_choices.Resolve(identity, [&]() { if (knob == WireDepthResolveProbeKnob::Unrecognised) MGLOG_W("DirectVulkan: MGITEST_MAGMA_DEPTH_RESOLVE_PROBE=%s is not `bug`, `clean` or " "`elide-subject`; the resolve probe runs as if it were unset", knobValue); @@ -15923,8 +15935,8 @@ void main() { MGLOG_I("DirectVulkan: depth/stencil resolve probe %s", DescribeWireDepthResolveFormat(reading).c_str()); return choice; - }(); - m_wirePreferShaderDepthResolve = s_choice.preferShader; + }); + m_wirePreferShaderDepthResolve = resolved.preferShader; } #endif diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h index aefcb7ae..a9285340 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h @@ -564,7 +564,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // target unwritten while the shader control resolved - the shader arm then resolves // first and the render-pass arm is the fallback (P7 gate 5, g5-msprobe). Bool m_wirePreferShaderDepthResolve = false; - // Runs the probe (memoized per process) and sets the member above. Called at the end of + // Runs the probe (memoized per device identity) and sets the member above. Called at the end of // device creation, after ArmPrimGenReroute: it records on m_graphicsQueue. void ArmWireDepthResolveOrder(); Bool BlitWireColorToDefault(WireImage source, WireImage destination, diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/WireDepthResolveArm.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/WireDepthResolveArm.h index a9d240cf..65791f44 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/WireDepthResolveArm.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/WireDepthResolveArm.h @@ -9,6 +9,10 @@ #include +#include +#include +#include + // P7 gate 5 (g5-msrbo, g5-msprobe): WHICH ARM RESOLVES A MULTISAMPLE DEPTH/STENCIL ASPECT FIRST. // // The wire arm has two (WireFramebuffer.inc, ResolveWireDepthStencil): a render pass that carries @@ -213,6 +217,61 @@ namespace MobileGL::MG_Backend::DirectVulkan { return choice; } + // WHOSE VERDICT IT IS (codex closeout finding 7). The verdict is a property of ONE device and + // driver, and a process can hold more than one: inproc tears a renderer down and initializes + // another, possibly on a different physical device or under a different driver. The choice was + // a function-static - the first renderer's answer for every renderer after it - so a second + // device skipped its own probe and took an order measured elsewhere. It is now memoized per + // IDENTITY: the device (vendor, device, driver version, pipeline-cache UUID - the last changes + // with the driver build even where the version number does not) plus the two inputs the choice + // itself depends on (whether a render-pass arm exists to judge, and the test knob). The same + // identity is still probed once per process; a different one is probed for itself. + struct WireDepthResolveDeviceIdentity { + Uint32 vendorID = 0; + Uint32 deviceID = 0; + Uint32 driverVersion = 0; + Uint8 pipelineCacheUUID[16] = {}; + Bool renderPassArmAvailable = false; + WireDepthResolveProbeKnob knob = WireDepthResolveProbeKnob::Measure; + + Bool operator==(const WireDepthResolveDeviceIdentity& other) const { + return vendorID == other.vendorID && deviceID == other.deviceID && driverVersion == other.driverVersion && + std::memcmp(pipelineCacheUUID, other.pipelineCacheUUID, sizeof(pipelineCacheUUID)) == 0 && + renderPassArmAvailable == other.renderPassArmAvailable && knob == other.knob; + } + }; + + class WireDepthResolveArmCache { + public: + // The identity's memoized choice, or `decide()`'s - called at most once per identity, under + // the cache's lock (a probe records on the device's queue; two renderers of one identity + // must not both run it). `decided`, when given, says whether this call ran `decide`. + template + WireDepthResolveArmChoice Resolve(const WireDepthResolveDeviceIdentity& identity, DecideFn&& decide, + Bool* decided = nullptr) { + std::lock_guard lock(m_mutex); + for (const auto& entry : m_entries) { + if (entry.first == identity) { + if (decided) *decided = false; + return entry.second; + } + } + WireDepthResolveArmChoice choice = decide(); + m_entries.emplace_back(identity, choice); + if (decided) *decided = true; + return choice; + } + + SizeT Size() const { + std::lock_guard lock(m_mutex); + return m_entries.size(); + } + + private: + mutable std::mutex m_mutex; + Vector> m_entries; + }; + // One format's readings, both arms: "D24_UNORM_S8_UINT x4: depth expected 0x400000 sentinel // 0xbfffff: render pass 0/16 (first 0xbfffff, sentinel 16), shader control 16/16 (first // 0x400000) / stencil ...". diff --git a/MobileGL/MG_Test/Pipeline/PipelineQuirkTest.cpp b/MobileGL/MG_Test/Pipeline/PipelineQuirkTest.cpp index b79fa22f..0a31ff83 100644 --- a/MobileGL/MG_Test/Pipeline/PipelineQuirkTest.cpp +++ b/MobileGL/MG_Test/Pipeline/PipelineQuirkTest.cpp @@ -724,3 +724,88 @@ TEST(PipelineQuirkTest, WireDepthResolveProbeKnobReplacesTheMeasurementNotTheVer GTEST_SKIP() << "the wire resolve arms exist only in the disaggregated build"; #endif } + +// Codex closeout finding 7 (cf-magma): THE VERDICT BELONGS TO A DEVICE, NOT TO THE PROCESS. The +// choice was a function-static in VulkanRenderer::ArmWireDepthResolveOrder, so a renderer on a +// second physical device, or under a changed driver, in the same process took the first device's +// order without probing. ArmWireDepthResolveOrder now asks WireDepthResolveArmCache, keyed by +// (vendor, device, driver version, pipeline-cache UUID) plus the choice's own inputs; this pins that +// two identities are decided independently and that one identity is still decided once. +TEST(PipelineQuirkTest, WireDepthResolveVerdictIsMemoizedPerDeviceIdentityNotPerProcess) { +#if MOBILEGL_BUILD_DISAGGREGATED + using MobileGL::MG_Backend::DirectVulkan::WireDepthResolveArmCache; + using MobileGL::MG_Backend::DirectVulkan::WireDepthResolveArmChoice; + using MobileGL::MG_Backend::DirectVulkan::WireDepthResolveDeviceIdentity; + WireDepthResolveArmCache cache; + Int probes = 0; + const auto probeReading = [&](FakeArms arms) { + return [&probes, arms] { + ++probes; + return ChooseWireDepthResolveArm(WireDepthResolveProbeKnob::Measure, true, [arms] { + return FakeMeasurement({FakeFormat("D24_UNORM_S8_UINT", arms, arms)}); + }); + }; + }; + // The Redmi's device, whose render pass writes nothing ... + WireDepthResolveDeviceIdentity adreno; + adreno.vendorID = 0x5143; + adreno.deviceID = 0x44050001; + adreno.driverVersion = 0x80000000u | 512; + adreno.pipelineCacheUUID[0] = 0xA8; + adreno.renderPassArmAvailable = true; + // ... and, later in the same process, a device whose render pass resolves. + WireDepthResolveDeviceIdentity lavapipe; + lavapipe.vendorID = 0x10005; + lavapipe.deviceID = 0; + lavapipe.driverVersion = 1; + lavapipe.pipelineCacheUUID[0] = 0x3E; + lavapipe.renderPassArmAvailable = true; + + Bool decided = false; + const WireDepthResolveArmChoice first = cache.Resolve(adreno, probeReading(kRenderPassWroteNothing), &decided); + EXPECT_TRUE(decided); + EXPECT_TRUE(first.preferShader); + const WireDepthResolveArmChoice second = cache.Resolve(lavapipe, probeReading(kBothArmsResolve), &decided); + EXPECT_TRUE(decided) << "a second device in the process must run its OWN probe"; + EXPECT_EQ(second.verdict, WireDepthResolveProbeVerdict::Clean); + EXPECT_FALSE(second.preferShader) << "the second device took the first device's verdict"; + EXPECT_EQ(probes, 2); + + // The same device again is still decided once: the probe stays once per device per process. + const WireDepthResolveArmChoice again = cache.Resolve(adreno, probeReading(kBothArmsResolve), &decided); + EXPECT_FALSE(decided); + EXPECT_TRUE(again.preferShader); + EXPECT_EQ(probes, 2); + + // Each field of the identity separates: a driver update (version or pipeline-cache UUID alone), + // another device of the vendor, and the choice's own inputs. + const auto separates = [&](WireDepthResolveDeviceIdentity other, const char* what) { + const Int before = probes; + const auto choice = cache.Resolve(other, probeReading(kBothArmsResolve), &decided); + EXPECT_TRUE(decided) << what << " reused another identity's verdict"; + EXPECT_EQ(probes, before + 1) << what; + EXPECT_FALSE(choice.preferShader) << what; + }; + auto updatedDriver = adreno; + updatedDriver.driverVersion += 1; + separates(updatedDriver, "a changed driver version"); + auto rebuiltDriver = adreno; + rebuiltDriver.pipelineCacheUUID[15] = 0x01; + separates(rebuiltDriver, "a changed pipeline-cache UUID"); + auto sibling = adreno; + sibling.deviceID += 1; + separates(sibling, "another device of the same vendor"); + auto otherVendor = adreno; + otherVendor.vendorID = 0x13B5; + separates(otherVendor, "another vendor with the same device id"); + auto noRenderPassArm = adreno; + noRenderPassArm.renderPassArmAvailable = false; + separates(noRenderPassArm, "a renderer without the render-pass arm"); + auto forcedClean = adreno; + forcedClean.knob = WireDepthResolveProbeKnob::ForceClean; + separates(forcedClean, "a different probe knob"); + EXPECT_EQ(cache.Size(), 8u); +#else + GTEST_SKIP() << "the wire resolve arms exist only in the disaggregated build"; +#endif +}