diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index 1368d6ab..4a541a4f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -113,7 +113,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // checks the level's and the layer's signs) nor at the draw. It is the observable an empty // unit has, so the answer is the empty unit's (ResolveWirePlaceholderImage): a NULL storage // descriptor where the device enables VK_EXT_robustness2 nullDescriptor, else a storage - // placeholder private to the (binding, unit), cleared before each use, of the dimensionality + // placeholder private to the unit (per shape), cleared before each use, of the dimensionality // the SHADER declared. // // KHR-GL46.shader_image_load_store.incomplete_textures is the shape that found it: level 2 @@ -169,7 +169,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { const auto handle = storage ? state.BoundShaderImages[unit].Res : state.BoundSamplerViews[unit].Texture; // An EMPTY image unit is the same 8.26 invalid access as the named-but-missing texel // below, so it takes the same answer: a null storage descriptor where the device has - // one, else the (binding, unit)-private placeholder (codex closeout finding 2). + // one, else the unit-private placeholder (codex closeout finding 2). if (MG_Pipe::MGPipeHandleIsNull(handle)) return ResolveWirePlaceholderImage(commandBuffer, program, programObj, binding, storage, out, VK_FORMAT_UNDEFINED, static_cast(unit)); @@ -659,6 +659,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { #if MOBILEGL_BUILD_DISAGGREGATED for (auto& entry : m_wirePlaceholderImages) DestroyWirePlaceholderImage(entry.second); m_wirePlaceholderImages.clear(); + m_wirePrivateStoragePlaceholders = 0; m_wireInvalidStorageImagesBindNull = false; #endif diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h index a50f739c..81ac7825 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h @@ -13,6 +13,7 @@ #include "VkBufferManager.h" #include "VkSamplerManager.h" #include "VkTextureManager.h" +#include "WirePlaceholderKey.h" #include "../VkIncludes.h" #include @@ -67,9 +68,10 @@ namespace MobileGL::MG_Backend::DirectVulkan { // VK_EXT_robustness2's nullDescriptor at device creation. With it the unit binds a NULL // storage-image descriptor, which is exactly that rule. Without it (or under // MGITEST_MAGMA_FORCE_PRIVATE_IMAGE_PLACEHOLDER=1, the host lanes' way onto this arm) the - // unit binds a placeholder PRIVATE to its (binding, unit), cleared before each use, so no + // unit binds a placeholder PRIVATE to its unit (per shape), cleared before each use, so no // two units ever alias; what that cannot give is a load of the SAME unit after its own - // store inside one pass reading zero (CONTRACT-P7 §12). Called after Initialize. + // store inside one pass reading zero, nor privacy past kWirePrivateStoragePlaceholderCap + // placeholders (CONTRACT-P7 §12). Called after Initialize. void SetWireInvalidStorageImageArm(Bool deviceNullDescriptor); #endif // A command buffer (re)began recording: descriptor bindings recorded into @@ -220,31 +222,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { VkImageLayout layout = VK_IMAGE_LAYOUT_UNDEFINED; Uint32 layers = 1; }; - // The placeholder's shape (format, target, storage, shadow) and, for a STORAGE placeholder - // on the arm without a null descriptor, the (binding, unit) it is private to. A sampled - // placeholder is never written, so it stays shared: its binding and unit are ~0u. - struct WirePlaceholderKey { - Uint64 shape = 0; - Uint32 binding = ~0u; - Uint32 unit = ~0u; - Bool operator==(const WirePlaceholderKey& other) const { - return shape == other.shape && binding == other.binding && unit == other.unit; - } - }; - struct WirePlaceholderKeyHash { - SizeT operator()(const WirePlaceholderKey& key) const { - Uint64 h = key.shape * 0x9E3779B97F4A7C15ull; - h ^= (static_cast(key.binding) << 32 | key.unit) + 0x9E3779B97F4A7C15ull + (h << 6) + (h >> 2); - return static_cast(h); - } - }; + // Keyed by shape, and a storage placeholder on the arm without a null descriptor by its + // image unit too (WirePlaceholderKey.h: ChooseWirePlaceholderKey, and its cap). mutable UnorderedMap m_wirePlaceholderImages; + // How many of those are private to a unit (against kWirePrivateStoragePlaceholderCap). + mutable SizeT m_wirePrivateStoragePlaceholders = 0; // See SetWireInvalidStorageImageArm. Bool m_wireInvalidStorageImagesBindNull = false; // boundStorageFormat: for a storage binding with no reflected format, the format the // unit's glBindImageTexture named, when the unit holds a texture it cannot address // (GL 4.6 core 8.26); UNDEFINED keeps the numeric-domain R32 default. `unit` is the image - // unit of a storage binding (the key of its private placeholder); unused for a sampler. + // unit of a storage binding (what its private placeholder is private to); unused for a sampler. Bool ResolveWirePlaceholderImage(VkCommandBuffer commandBuffer, const MagmaProgramSource& program, const ProgramFactory::VkProgramObject& programObj, Uint32 binding, Bool storage, VkDescriptorImageInfo& out, VkFormat boundStorageFormat = VK_FORMAT_UNDEFINED, Uint32 unit = ~0u) const; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index f65cd41c..8523e0f5 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -15880,14 +15880,8 @@ void main() { 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; + const WireDepthResolveDeviceIdentity identity = + MakeWireDepthResolveDeviceIdentity(m_physicalDevice.properties, renderPassArmAvailable, knob); static WireDepthResolveArmCache s_choices; const WireDepthResolveArmChoice resolved = s_choices.Resolve(identity, [&]() { if (knob == WireDepthResolveProbeKnob::Unrecognised) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/WireDepthResolveArm.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/WireDepthResolveArm.h index 65791f44..ae8d56b2 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/WireDepthResolveArm.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/WireDepthResolveArm.h @@ -241,6 +241,23 @@ namespace MobileGL::MG_Backend::DirectVulkan { } }; + // The renderer's identity from its device's VkPhysicalDeviceProperties - the ONE place the fields + // are filled (VulkanRenderer::ArmWireDepthResolveOrder calls it), so the unit test proves that each + // property reaches the key: with only the cache tested, a field left out here stayed green. + inline WireDepthResolveDeviceIdentity MakeWireDepthResolveDeviceIdentity(const VkPhysicalDeviceProperties& properties, + Bool renderPassArmAvailable, + WireDepthResolveProbeKnob knob) { + WireDepthResolveDeviceIdentity identity; + identity.vendorID = properties.vendorID; + identity.deviceID = properties.deviceID; + identity.driverVersion = properties.driverVersion; + static_assert(sizeof(identity.pipelineCacheUUID) == sizeof(properties.pipelineCacheUUID)); + std::memcpy(identity.pipelineCacheUUID, properties.pipelineCacheUUID, sizeof(identity.pipelineCacheUUID)); + identity.renderPassArmAvailable = renderPassArmAvailable; + identity.knob = knob; + return identity; + } + class WireDepthResolveArmCache { public: // The identity's memoized choice, or `decide()`'s - called at most once per identity, under diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/WirePlaceholderImages.inc b/MobileGL/MG_Backend/DirectVulkan/Renderer/WirePlaceholderImages.inc index 14fce8dd..590ce79f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/WirePlaceholderImages.inc +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/WirePlaceholderImages.inc @@ -19,7 +19,7 @@ m_wireInvalidStorageImagesBindNull = deviceNullDescriptor && !forcePrivate; MGLOG_I("Magma wire: an invalid or unbound image unit binds %s (VK_EXT_robustness2 nullDescriptor %s%s)", m_wireInvalidStorageImagesBindNull ? "a NULL storage descriptor" - : "a placeholder private to its (binding, unit)", + : "a placeholder private to its unit", deviceNullDescriptor ? "enabled" : "unavailable", deviceNullDescriptor && forcePrivate ? "; MGITEST_MAGMA_FORCE_PRIVATE_IMAGE_PLACEHOLDER=1" : ""); } @@ -93,17 +93,21 @@ break; default: return false; } - // A STORAGE placeholder is private to its (binding, unit). Shared by shape alone, a store + // A STORAGE placeholder is private to its unit (per shape). Shared by shape alone, a store // through invalid unit A was visible, after a barrier, to a load through invalid unit B of // the same shape - one Vulkan image behind both (codex closeout finding 2). The clear // below still runs before each use; what a private image cannot hide is the SAME unit's - // own store to its own later load inside one pass (CONTRACT-P7 §12). - WirePlaceholderKey key; - key.shape = static_cast(format) | (static_cast(target) << 32) | - (static_cast(storage) << 48) | (static_cast(shadow) << 49); - if (storage) { - key.binding = binding; - key.unit = unit; + // own store to its own later load inside one pass (CONTRACT-P7 §12). Not per binding: GL + // aliases per unit, and a (binding, unit) key only multiplied allocations. Past + // kWirePrivateStoragePlaceholderCap a new (shape, unit) shares its shape's (WirePlaceholderKey.h). + const Uint64 shape = static_cast(format) | (static_cast(target) << 32) | + (static_cast(storage) << 48) | (static_cast(shadow) << 49); + const WirePlaceholderKey key = ChooseWirePlaceholderKey(shape, storage, unit, m_wirePrivateStoragePlaceholders, + [this](const WirePlaceholderKey& candidate) { return m_wirePlaceholderImages.count(candidate) != 0; }); + if (storage && unit != WirePlaceholderKey::kShared && key.unit == WirePlaceholderKey::kShared) { + MGLOG_W_ONCE("Magma wire: %zu image-unit-private placeholders exist (the cap); a further invalid or " + "unbound unit shares its shape's placeholder, so two such units of one shape can alias", + static_cast(m_wirePrivateStoragePlaceholders)); } auto found = m_wirePlaceholderImages.find(key); if (found == m_wirePlaceholderImages.end()) { @@ -194,11 +198,12 @@ if (vkCreateFramebuffer(m_device, &framebuffer, nullptr, &image.clearFramebuffer) != VK_SUCCESS) return fail(); } found = m_wirePlaceholderImages.emplace(key, image).first; + if (key.unit != WirePlaceholderKey::kShared) ++m_wirePrivateStoragePlaceholders; } auto& image = found->second; // Sampled incomplete textures remain immutable opaque black. A storage // placeholder is reset before each use, so a prior discarded store cannot - // leak into a later pass's loads through the same (binding, unit). + // leak into a later pass's loads through the same unit. if (storage || image.layout == VK_IMAGE_LAYOUT_UNDEFINED) { VkClearValue clear{}; if (!storage) { diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/WirePlaceholderKey.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/WirePlaceholderKey.h new file mode 100644 index 00000000..a3cbde8a --- /dev/null +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/WirePlaceholderKey.h @@ -0,0 +1,62 @@ +// MobileGL - MobileGL/MG_Backend/DirectVulkan/Renderer/WirePlaceholderKey.h +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +#pragma once + +#include + +// The pure half of UniformManager's wire placeholder cache (WirePlaceholderImages.inc): WHICH +// placeholder a binding with no usable image takes. A unit test pins it +// (MG_Test/Pipeline/PipelineQuirkTest.cpp); the Vulkan half that makes the image stays in the .inc. +#if MOBILEGL_BUILD_DISAGGREGATED +#include + +namespace MobileGL::MG_Backend::DirectVulkan { + // The placeholder's SHAPE (format, target, storage, shadow) and, for a STORAGE placeholder on + // the arm without a null descriptor, the image UNIT it is private to. GL aliases per UNIT, so the + // unit is the whole of the no-alias guarantee (codex closeout finding 2): two bindings that name + // one unit are one GL image, and keying them apart as well only multiplied allocations (cf-magma + // critic). A sampled placeholder is never written, so it stays shared: its unit is kShared. + struct WirePlaceholderKey { + static constexpr Uint32 kShared = ~0u; + Uint64 shape = 0; + Uint32 unit = kShared; + Bool operator==(const WirePlaceholderKey& other) const { return shape == other.shape && unit == other.unit; } + }; + + struct WirePlaceholderKeyHash { + SizeT operator()(const WirePlaceholderKey& key) const { + Uint64 h = key.shape * 0x9E3779B97F4A7C15ull; + h ^= static_cast(key.unit) + 0x9E3779B97F4A7C15ull + (h << 6) + (h >> 2); + return static_cast(h); + } + }; + + // Every private placeholder is its own vkCreateImage + vkAllocateMemory, freed only at Shutdown. + // (shape, unit) bounds them by shapes x kMGPipeMaxImageUnits, and a peer rotating formats and + // targets across the units could still walk that towards maxMemoryAllocationCount (4096 is the + // spec minimum). So past this many, a NEW (shape, unit) takes its shape's SHARED placeholder - the + // pre-fix aliasing, logged once by the caller - instead of another allocation; a unit that already + // has its private placeholder keeps it. Two per unit is far past any real program. + inline constexpr SizeT kWirePrivateStoragePlaceholderCap = 2 * static_cast(MG_Pipe::kMGPipeMaxImageUnits); + + // The key a placeholder of `shape` is found or made under. `privateStorage` = a storage binding on + // the arm without a null descriptor (with one, the caller never gets here); `unit` = its image + // unit; `privateCount` = how many private placeholders exist; `known(key)` = the cache holds `key`. + template + WirePlaceholderKey ChooseWirePlaceholderKey(Uint64 shape, Bool privateStorage, Uint32 unit, SizeT privateCount, + KnownFn&& known) { + WirePlaceholderKey key; + key.shape = shape; + if (!privateStorage || unit == WirePlaceholderKey::kShared) return key; + key.unit = unit; + if (privateCount < kWirePrivateStoragePlaceholderCap || known(key)) return key; + key.unit = WirePlaceholderKey::kShared; + return key; + } +} // namespace MobileGL::MG_Backend::DirectVulkan +#endif diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 233012f0..69f56e16 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -3495,8 +3495,10 @@ if (MOBILEGL_BUILD_DISAGGREGATED) # KHR-GL46.shader_image_load_store.incomplete_textures ended the inproc Redmi run at # Fatal{UnmigratedVerb, "Magma:image-view-window"}: the unit named level 2 of a texture # with only level 0, a monolith Fail turned into a SIGABRT. GL 4.6 core 8.26 makes that - # access invalid - zero loads, dropped stores - and the wire descriptor resolve now binds - # the storage placeholder for it. One entry per case for the private log path; NO KNOB, + # access invalid - zero loads, dropped stores - and the wire descriptor resolve now binds a + # null storage descriptor for it where the device enables VK_EXT_robustness2 nullDescriptor, + # else a placeholder private to the unit (UniformManager::ResolveWireImageDescriptor and + # ResolveWirePlaceholderImage). One entry per case for the private log path; NO KNOB, # so all three arms. The monolith `DirectVulkan.` entry of the same scenario skips by # name (it drops the draw: the gate 5 BASE, and a pull-build .text change to move). # ALevelPastATextureViewsWindowReadsZero joined once the F2 repair retired the transport @@ -3521,7 +3523,7 @@ if (MOBILEGL_BUILD_DISAGGREGATED) # # The two AStoreThrough... cases above run the null-descriptor arm on this host: lavapipe # enables VK_EXT_robustness2 nullDescriptor, so an invalid or empty storage unit binds a - # null descriptor. A device without it binds a placeholder PRIVATE to the (binding, unit) + # null descriptor. A device without it binds a placeholder PRIVATE to the unit (per shape) # instead; MGITEST_MAGMA_FORCE_PRIVATE_IMAGE_PLACEHOLDER=1 puts the server on that arm # (UniformManager::SetWireInvalidStorageImageArm), and the client reads the same marker to # expect it: B's load zero (no alias), A's own load of its store = the recorded CONTRACT-P7 diff --git a/MobileGL/MG_IntegrationTest/Scenarios/InvalidImageUnitScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/InvalidImageUnitScenario.cpp index afbcf133..d2f9754a 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/InvalidImageUnitScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/InvalidImageUnitScenario.cpp @@ -487,7 +487,7 @@ void main() { } // The Magma wire arm without a null storage descriptor (a device lacking // VK_EXT_robustness2 nullDescriptor; forced on the host by the knob): each invalid - // unit gets a placeholder private to its (binding, unit). B's load is still zero - + // unit gets a placeholder private to its unit. B's load is still zero - // the units no longer alias - but A's load of A's own store in the same pass is // not: that is the CONTRACT-P7 §12 residual, and here it is what PROVES the knob // reached the server (the null-descriptor arm would read zero there too). diff --git a/MobileGL/MG_Test/Pipeline/PipelineQuirkTest.cpp b/MobileGL/MG_Test/Pipeline/PipelineQuirkTest.cpp index 0a31ff83..898c23b5 100644 --- a/MobileGL/MG_Test/Pipeline/PipelineQuirkTest.cpp +++ b/MobileGL/MG_Test/Pipeline/PipelineQuirkTest.cpp @@ -14,6 +14,7 @@ #if MOBILEGL_BUILD_DISAGGREGATED #include #include +#include #endif using namespace MobileGL; @@ -730,9 +731,12 @@ TEST(PipelineQuirkTest, WireDepthResolveProbeKnobReplacesTheMeasurementNotTheVer // 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. +// two identities are decided independently and that one identity is still decided once. Every +// identity is built by MakeWireDepthResolveDeviceIdentity - the helper ArmWireDepthResolveOrder +// fills its key with - from VkPhysicalDeviceProperties, so a property the helper drops goes red too. TEST(PipelineQuirkTest, WireDepthResolveVerdictIsMemoizedPerDeviceIdentityNotPerProcess) { #if MOBILEGL_BUILD_DISAGGREGATED + using MobileGL::MG_Backend::DirectVulkan::MakeWireDepthResolveDeviceIdentity; using MobileGL::MG_Backend::DirectVulkan::WireDepthResolveArmCache; using MobileGL::MG_Backend::DirectVulkan::WireDepthResolveArmChoice; using MobileGL::MG_Backend::DirectVulkan::WireDepthResolveDeviceIdentity; @@ -747,19 +751,33 @@ TEST(PipelineQuirkTest, WireDepthResolveVerdictIsMemoizedPerDeviceIdentityNotPer }; }; // 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; + VkPhysicalDeviceProperties adrenoDevice{}; + adrenoDevice.vendorID = 0x5143; + adrenoDevice.deviceID = 0x44050001; + adrenoDevice.driverVersion = 0x80000000u | 512; + adrenoDevice.pipelineCacheUUID[0] = 0xA8; + const WireDepthResolveDeviceIdentity adreno = + MakeWireDepthResolveDeviceIdentity(adrenoDevice, true, WireDepthResolveProbeKnob::Measure); // ... 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; + VkPhysicalDeviceProperties lavapipeDevice{}; + lavapipeDevice.vendorID = 0x10005; + lavapipeDevice.deviceID = 0; + lavapipeDevice.driverVersion = 1; + lavapipeDevice.pipelineCacheUUID[0] = 0x3E; + const WireDepthResolveDeviceIdentity lavapipe = + MakeWireDepthResolveDeviceIdentity(lavapipeDevice, true, WireDepthResolveProbeKnob::Measure); + // The helper copies every property it is given, the UUID's last byte included. + adrenoDevice.pipelineCacheUUID[VK_UUID_SIZE - 1] = 0x5A; + const WireDepthResolveDeviceIdentity copied = + MakeWireDepthResolveDeviceIdentity(adrenoDevice, false, WireDepthResolveProbeKnob::ForceBug); + EXPECT_EQ(copied.vendorID, 0x5143u); + EXPECT_EQ(copied.deviceID, 0x44050001u); + EXPECT_EQ(copied.driverVersion, 0x80000000u | 512); + EXPECT_EQ(copied.pipelineCacheUUID[0], 0xA8); + EXPECT_EQ(copied.pipelineCacheUUID[VK_UUID_SIZE - 1], 0x5A); + EXPECT_FALSE(copied.renderPassArmAvailable); + EXPECT_EQ(copied.knob, WireDepthResolveProbeKnob::ForceBug); + adrenoDevice.pipelineCacheUUID[VK_UUID_SIZE - 1] = 0; Bool decided = false; const WireDepthResolveArmChoice first = cache.Resolve(adreno, probeReading(kRenderPassWroteNothing), &decided); @@ -777,35 +795,103 @@ TEST(PipelineQuirkTest, WireDepthResolveVerdictIsMemoizedPerDeviceIdentityNotPer 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) { + // Each property separates, through the helper: a driver update (version or pipeline-cache UUID + // alone), another device of the vendor, and the choice's own inputs. + const auto separates = [&](const VkPhysicalDeviceProperties& other, Bool renderPassArm, + WireDepthResolveProbeKnob knob, const char* what) { const Int before = probes; - const auto choice = cache.Resolve(other, probeReading(kBothArmsResolve), &decided); + const auto choice = cache.Resolve(MakeWireDepthResolveDeviceIdentity(other, renderPassArm, knob), + 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; + const auto measured = WireDepthResolveProbeKnob::Measure; + auto updatedDriver = adrenoDevice; 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; + separates(updatedDriver, true, measured, "a changed driver version"); + auto rebuiltDriver = adrenoDevice; + rebuiltDriver.pipelineCacheUUID[VK_UUID_SIZE - 1] = 0x01; + separates(rebuiltDriver, true, measured, "a changed pipeline-cache UUID (its last byte)"); + auto sibling = adrenoDevice; sibling.deviceID += 1; - separates(sibling, "another device of the same vendor"); - auto otherVendor = adreno; + separates(sibling, true, measured, "another device of the same vendor"); + auto otherVendor = adrenoDevice; 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"); + separates(otherVendor, true, measured, "another vendor with the same device id"); + separates(adrenoDevice, false, measured, "a renderer without the render-pass arm"); + separates(adrenoDevice, true, WireDepthResolveProbeKnob::ForceClean, "a different probe knob"); + EXPECT_EQ(cache.Size(), 8u); + // A property the identity does not hold does not split it: the device NAME is not the device. + auto renamed = adrenoDevice; + std::snprintf(renamed.deviceName, sizeof(renamed.deviceName), "%s", "Adreno (TM) 830 renamed"); + (void)cache.Resolve(MakeWireDepthResolveDeviceIdentity(renamed, true, measured), probeReading(kBothArmsResolve), + &decided); + EXPECT_FALSE(decided) << "the device name is not part of the identity"; EXPECT_EQ(cache.Size(), 8u); #else GTEST_SKIP() << "the wire resolve arms exist only in the disaggregated build"; #endif } + +// Codex closeout finding 2, the cf-magma critic's follow-up: WHICH PLACEHOLDER AN INVALID OR EMPTY +// STORAGE UNIT TAKES on the arm without a null descriptor. Private per (shape, UNIT) - GL aliases per +// unit, so the unit is the no-alias guarantee (the ImageUnitPrivate integration entries go red when it +// leaves the key) and the binding is not in the key at all - and bounded: every private placeholder +// is an allocation freed only at Shutdown, so past kWirePrivateStoragePlaceholderCap a NEW (shape, +// unit) shares its shape's placeholder while the units that have one keep it. A sampled placeholder +// is never written and stays shared. +TEST(PipelineQuirkTest, WirePlaceholderKeysArePrivatePerShapeAndUnitAndBounded) { +#if MOBILEGL_BUILD_DISAGGREGATED + using MobileGL::MG_Backend::DirectVulkan::ChooseWirePlaceholderKey; + using MobileGL::MG_Backend::DirectVulkan::kWirePrivateStoragePlaceholderCap; + using MobileGL::MG_Backend::DirectVulkan::WirePlaceholderKey; + using MobileGL::MG_Backend::DirectVulkan::WirePlaceholderKeyHash; + constexpr Uint32 kUnits = MobileGL::MG_Pipe::kMGPipeMaxImageUnits; + static_assert(kWirePrivateStoragePlaceholderCap >= kUnits, + "one shape on every image unit must stay private"); + UnorderedMap cache; + SizeT privateCount = 0; + // What ResolveWirePlaceholderImage does with the key: find it, or make (and count) it. + const auto take = [&](Uint64 shape, Bool storage, Uint32 unit) { + const WirePlaceholderKey key = ChooseWirePlaceholderKey(shape, storage, unit, privateCount, + [&](const WirePlaceholderKey& candidate) { return cache.count(candidate) != 0; }); + if (cache.emplace(key, 0).second && key.unit != WirePlaceholderKey::kShared) ++privateCount; + return key; + }; + const Uint64 rgba8Storage2D = 37u | (1ull << 48); + const Uint64 r32uiStorage3D = 98u | (4ull << 32) | (1ull << 48); + const Uint64 rgba8Sampled2D = 37u; + + // Units A and B of one shape: two placeholders (the finding) ... + const WirePlaceholderKey a = take(rgba8Storage2D, true, 2); + const WirePlaceholderKey b = take(rgba8Storage2D, true, 3); + EXPECT_FALSE(a == b) << "two invalid units of one shape share a placeholder"; + EXPECT_EQ(a.unit, 2u); + // ... and unit A through a second binding is A's own placeholder, not a third allocation. + EXPECT_TRUE(take(rgba8Storage2D, true, 2) == a) << "a second binding on unit A made another placeholder"; + EXPECT_EQ(privateCount, 2u); + // A sampled placeholder is shared by shape, whatever unit it came from. + EXPECT_EQ(take(rgba8Sampled2D, false, 2).unit, WirePlaceholderKey::kShared); + EXPECT_TRUE(take(rgba8Sampled2D, false, 7) == take(rgba8Sampled2D, false, 2)); + EXPECT_EQ(privateCount, 2u); + + // A peer rotating two shapes across every unit, then a third: the map stops at the cap (plus + // the shared placeholders), every unit that got a private placeholder keeps it, and the ones + // past the cap share their shape's. + for (Uint32 unit = 0; unit < kUnits; ++unit) { + (void)take(rgba8Storage2D, true, unit); + (void)take(r32uiStorage3D, true, unit); + } + for (Uint32 unit = 0; unit < kUnits; ++unit) { + const WirePlaceholderKey rotated = take(37u | (2ull << 32) | (1ull << 48), true, unit); + EXPECT_EQ(rotated.unit, WirePlaceholderKey::kShared) << "unit " << unit << " past the cap still allocated"; + } + EXPECT_EQ(privateCount, kWirePrivateStoragePlaceholderCap); + EXPECT_LE(cache.size(), kWirePrivateStoragePlaceholderCap + 3u); + EXPECT_TRUE(take(rgba8Storage2D, true, 3) == b) << "a unit that had its private placeholder lost it at the cap"; + EXPECT_EQ(take(r32uiStorage3D, true, kUnits - 1).unit, kUnits - 1); +#else + GTEST_SKIP() << "the wire placeholders exist only in the disaggregated build"; +#endif +} diff --git a/scripts/ci/spawn_lane_parity.py b/scripts/ci/spawn_lane_parity.py index 2e1ba615..801b08b9 100644 --- a/scripts/ci/spawn_lane_parity.py +++ b/scripts/ci/spawn_lane_parity.py @@ -94,7 +94,7 @@ MAGMA_INPROC_ONLY = ("MagmaRunAheadScenario.", "MagmaWireCacheScenario.") # the same server-side arm choice reads to run the REAL probe with its render-pass resolve left out. # Codex closeout finding 2 (cf-magma): the `.ImageUnitPrivate.` entries set # MGITEST_MAGMA_FORCE_PRIVATE_IMAGE_PLACEHOLDER=1, which the SERVER's UniformManager reads to bind a -# (binding, unit)-private placeholder for an invalid image unit instead of a null descriptor - the +# unit-private placeholder for an invalid image unit instead of a null descriptor - the # knob-free `.ImageUnitWindow.` copies of the same two cases keep their tcp entries. Two cases, so two # tails (one case each), as for `.Reclaim.`. MAGMA_SERVER_ENV_KNOB_NO_TCP = (".ShaderMip1.", ".ShaderMip2.", ".DepthMip.",