From 02cc0ce83c9fbf0dab504f722668b6fe401f6195 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 27 Aug 2026 13:03:51 -0400 Subject: [PATCH] [Fix] (Review): scope the sample mask to a multisample target, version the sampled-set memo on completeness, clamp the mask word count, give the multisample placeholder every numeric domain, unwind the fixup revert one pass at a time, and bound the validator log --- .../DirectVulkan/Renderer/PipelineFactory.cpp | 2 +- .../DirectVulkan/Renderer/PipelineFactory.h | 22 ++- .../DirectVulkan/Renderer/ProgramFactory.cpp | 131 ++++++++++--- .../DirectVulkan/Renderer/ProgramFactory.h | 14 ++ .../DirectVulkan/Renderer/UniformManager.cpp | 98 +++++++--- .../DirectVulkan/Renderer/UniformManager.h | 21 ++- .../DirectVulkan/Renderer/VulkanRenderer.cpp | 106 +++++++++-- .../DirectVulkan/Renderer/VulkanRenderer.h | 23 ++- MobileGL/MG_IntegrationTest/CMakeLists.txt | 1 + .../Scenarios/SampleMaskScopeScenario.cpp | 178 ++++++++++++++++++ .../UnboundImageDescriptorScenario.cpp | 41 ++++ .../GLState/TextureState/TextureState.h | 11 +- .../MG_Util/BackendLoaders/Vulkan/Loader.cpp | 21 ++- .../ShaderTranspiler/ShaderCompiler.cpp | 27 +++ .../MG_Util/ShaderTranspiler/ShaderCompiler.h | 6 + 15 files changed, 615 insertions(+), 87 deletions(-) create mode 100644 MobileGL/MG_IntegrationTest/Scenarios/SampleMaskScopeScenario.cpp diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/PipelineFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/PipelineFactory.cpp index 52699ba4..752cc4e5 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/PipelineFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/PipelineFactory.cpp @@ -447,7 +447,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // GL_SAMPLE_MASK / glSampleMaski. Left at nullptr - which Vulkan reads as all-ones - until // now, so glSampleMaski was a silent no-op on this backend while DirectGLES forwarded it. // The pointer has to outlive the vkCreateGraphicsPipelines call, which the payload does. - ms.pSampleMask = &payload.sampleMask; + ms.pSampleMask = payload.sampleMask; VkPipelineDepthStencilStateCreateInfo depthStencil{VK_STRUCTURE_TYPE_PIPELINE_DEPTH_STENCIL_STATE_CREATE_INFO}; depthStencil.depthTestEnable = payload.depthTestEnable ? VK_TRUE : VK_FALSE; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/PipelineFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/PipelineFactory.h index 6cb4a0eb..318b9af7 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/PipelineFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/PipelineFactory.h @@ -44,13 +44,21 @@ namespace MobileGL::MG_Backend::DirectVulkan { // (VUID-VkPipelineMultisampleStateCreateInfo-sampleShadingEnable-00784). Bool sampleShadingEnable = false; Float minSampleShading = 0.0f; - // glEnable(GL_SAMPLE_MASK) + glSampleMaski, the fixed-function coverage mask. One - // word is the whole mask: GL_MAX_SAMPLE_MASK_WORDS is 1 on both backends, and Vulkan - // reads ceil(rasterizationSamples / 32) words. Pipeline state like the two above - - // Vulkan has no dynamic sample mask before VK_EXT_extended_dynamic_state3 - so it is - // hashed with them, and 0xffffffff (the GL default, and what a null pSampleMask - // means) has to keep producing the pipeline it always did. - Uint32 sampleMask = 0xffffffffu; + // glEnable(GL_SAMPLE_MASK) + glSampleMaski, the fixed-function coverage mask, already + // reduced to what GL says this draw gets (VulkanRenderer::ResolveEffectiveSampleMask: + // all-ones unless the target is genuinely multisampled). Pipeline state like the two + // above - Vulkan has no dynamic sample mask before VK_EXT_extended_dynamic_state3 - + // so it is hashed with them, and all-ones has to keep producing the pipeline a null + // pSampleMask always did. + // + // TWO words, though GL only ever fills the first. GL_MAX_SAMPLE_MASK_WORDS is clamped + // to 1 on both backends, so glSampleMaski writes index 0 and nothing else - but the + // count Vulkan READS is ceil(rasterizationSamples / 32), which is 2 on a 64-sample + // target, and GetAdvertisedMaxSamples does not cap the driver's sample count. A + // single Uint32 here let such a pipeline read one word past the member (the next + // struct field). The second word is all-ones: full coverage for samples 32..63, which + // is the only honest answer when GL has no state describing them. + Uint32 sampleMask[2] = {0xffffffffu, 0xffffffffu}; Uint32 subpass = 0; VkPrimitiveTopology topology = VK_PRIMITIVE_TOPOLOGY_TRIANGLE_LIST; Bool primitiveRestartEnable = false; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index 4d50c347..d7206562 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -381,13 +381,31 @@ namespace MobileGL::MG_Backend::DirectVulkan { return used; } + // What a failed validation says, for a caller that wants to put it in its own message. + struct SpirvValidationFailure { + String message; + Int result = 0; + SizeT index = 0; + }; + // Returns whether the module validates. The result used to be discarded everywhere: the // call was DEBUG-or-env gated and only logged, so an invalid module produced by a backend // transform went straight to vkCreateShaderModule. That is not a survivable outcome on // this hardware - Mali r54 SIGSEGVs building the pipeline instead of returning an error, // the same "not a validating entry point" behaviour PipelineFactory already documents for - // vkCreateGraphicsPipelines - so the one caller that feeds the driver now acts on it. - Bool ValidateTransformedSpirv(const Vector& spirv, ShaderStage shaderStage, Uint programExternalIndex) { + // vkCreateGraphicsPipelines - so the callers that feed the driver now act on it. + // + // This function does NOT log the failure at E any more. It used to, unlatched, on the + // stated grounds that "reaching here already requires the validation switch to be armed, + // which bounds the volume" - and that premise died when the two GetOrCreateProgram call + // sites became unconditional: MGLOG_E is live at the production INFO level, and Log.h's + // own rule is that anything at W or E on a repeatable path must be latched or demoted. + // The failure text now travels back through `outFailure` so the LATCHED call-site + // messages carry the VUID instead of an unlatched inner one repeating it; what stays here + // is the D-level detail and the process-wide counter the test lanes assert on. + Bool ValidateTransformedSpirv(const Vector& spirv, ShaderStage shaderStage, Uint programExternalIndex, + SpirvValidationFailure* outFailure = nullptr) { + if (outFailure != nullptr) *outFailure = {}; if (spirv.empty()) { return true; } @@ -411,18 +429,24 @@ namespace MobileGL::MG_Backend::DirectVulkan { spv_diagnostic diagnostic = nullptr; const spv_result_t result = spvValidateWithOptions(context, options, &binary, &diagnostic); if (result != SPV_SUCCESS) { - // MGLOG_E, unlatched: reaching here already requires the validation switch to - // be armed, which bounds the volume, and each VUID names a different defect. - // (Parked at MGLOG_I until the Log.h level ordering was fixed, when E was - // compiled out of every INFO build.) The latch is what a test harness asserts on. + const char* message = + diagnostic != nullptr && diagnostic->error != nullptr ? diagnostic->error : ""; + const SizeT index = diagnostic != nullptr ? diagnostic->position.index : 0; + // The test-lane signal (ShaderCompiler.h documents harnesses snapshotting it and + // asserting on the delta). Bumped for every failed validation, including one a + // caller goes on to recover from: a transform that produced an invalid module is + // a real defect whether or not this run survived it. MG_Util::ShaderTranspiler::ShaderCompiler::NoteSpirvValidationFailure(); - MGLOG_E( + if (outFailure != nullptr) { + *outFailure = {String(message), static_cast(result), index}; + } + MGLOG_D( "ProgramFactory::ValidateTransformedSpirv: validation failed for stage=%d program=%u result=%d index=%zu msg=%s", static_cast(shaderStage), programExternalIndex, static_cast(result), - diagnostic != nullptr ? diagnostic->position.index : 0, - diagnostic != nullptr && diagnostic->error != nullptr ? diagnostic->error : ""); + index, + message); } MOBILEGL_ASSERT( result == SPV_SUCCESS, @@ -3404,17 +3428,40 @@ namespace MobileGL::MG_Backend::DirectVulkan { // variable the link-time sanitize chain delisted from the entry-point interface // is invalid SPIR-V that Mali r54 turns into a SIGSEGV inside pipeline creation // rather than an error return. EnsureEntryPointInterface keeps them honest; this - // is the backstop, and the one place in this function where the fallback is - // still consistent with everything downstream, because `spv` is the input to - // both passes and the descriptor remap has not run yet. + // is the backstop. // - // Once per program on a cache miss, and only for the single stage that carries - // the fixups - not per draw and not per module. - if (!ValidateTransformedSpirv(moduleSpirvs[i], stages[i], program.GetExternalIndex())) { - MGLOG_E_ONCE("ProgramFactory: the clip/XFB fixups produced an invalid module for program %u " - "stage %d; keeping the untransformed one", - program.GetExternalIndex(), static_cast(stages[i])); - moduleSpirvs[i] = spv; + // The fallback UNWINDS ONE PASS AT A TIME, which matters because the two passes + // are not equally optional. Rewinding straight to `spv` would also throw away the + // XfbBuffer/XfbStride/Offset decorations, the TransformFeedback capability and the + // Xfb execution mode - while the renderer decides to call + // vkCmdBeginTransformFeedbackEXT purely from GL state and never looks at the + // module. That ships a pipeline whose last pre-rasterization stage has no Xfb mode + // into a transform-feedback span, violating + // VUID-vkCmdBeginTransformFeedbackEXT-None-04128 on exactly the driver class this + // guard exists for. So: try the post-XFB, pre-clip-fixup module first, which keeps + // capture working and costs only the clip-space remap. + // + // Once per program on a cache miss, and only for the single stage that carries the + // fixups - not per draw and not per module. + SpirvValidationFailure fixupFailure{}; + if (!ValidateTransformedSpirv(moduleSpirvs[i], stages[i], program.GetExternalIndex(), + &fixupFailure)) { + SpirvValidationFailure xfbFailure{}; + if (fixupInput != &spv && + ValidateTransformedSpirv(*fixupInput, stages[i], program.GetExternalIndex(), &xfbFailure)) { + MGLOG_E_ONCE("ProgramFactory: the clip fixup produced an invalid module for program %u " + "stage %d (%s); keeping the capture-decorated one, so this program draws " + "without the clip-space remap", + program.GetExternalIndex(), static_cast(stages[i]), + fixupFailure.message.c_str()); + moduleSpirvs[i] = *fixupInput; + } else { + MGLOG_E_ONCE("ProgramFactory: the clip/XFB fixups produced an invalid module for program %u " + "stage %d (%s); keeping the untransformed one", + program.GetExternalIndex(), static_cast(stages[i]), + fixupFailure.message.c_str()); + moduleSpirvs[i] = spv; + } } } else { moduleSpirvs[i] = spv; @@ -3633,10 +3680,28 @@ namespace MobileGL::MG_Backend::DirectVulkan { // introduce a reference to a delisted interface variable, the failure this whole // guard exists for. Anything that reaches this line names itself in the log of a // shipping build instead of dying anonymously inside the driver. - if (!ValidateTransformedSpirv(moduleSpv, stages[i], program.GetExternalIndex())) { + SpirvValidationFailure finalFailure{}; + if (!ValidateTransformedSpirv(moduleSpv, stages[i], program.GetExternalIndex(), &finalFailure)) { MGLOG_E_ONCE("ProgramFactory: handing vkCreateShaderModule an INVALID module for program %u stage %d - " - "a backend transform after the clip/XFB fixups broke it", + "a backend transform after the clip/XFB fixups broke it (%s)", + program.GetExternalIndex(), static_cast(stages[i]), + finalFailure.message.c_str()); + } + + // Does the stage the driver will treat as the last pre-rasterization one actually + // carry Xfb? Asked of the FINAL bytes, so it answers for whatever the whole transform + // chain produced - a rewound clip/XFB backstop, a capture pass that resolved no + // varying and changed nothing, anything later that might strip it. The renderer picks + // its capture commands from GL state alone and would otherwise open a span against a + // pipeline that cannot feed it. + if (stages[i] == fixupStage && (flags & ProgramFactory::CompileOptionBit::XfbCapture) && + program.GetTransformFeedbackVaryingCount() > 0 && + !MG_Util::ShaderTranspiler::ShaderCompiler::ModuleDeclaresTransformFeedback(moduleSpv)) { + MGLOG_E_ONCE("ProgramFactory: program %u was built as a transform-feedback capture variant but its " + "stage %d carries no Xfb execution mode; its capture spans will be declined rather " + "than recorded against a pipeline that cannot feed them", program.GetExternalIndex(), static_cast(stages[i])); + entry.xfbCaptureDeclined = true; } VkShaderModuleCreateInfo smci{VK_STRUCTURE_TYPE_SHADER_MODULE_CREATE_INFO}; @@ -4019,14 +4084,28 @@ namespace MobileGL::MG_Backend::DirectVulkan { } const Vector& spirv = binary.value().front(); + { + // Still switch-gated, unlike the two in GetOrCreateProgram: this stage is synthesized + // by MobileGL from a fixed template rather than transformed from application SPIR-V, + // so a failure here is a MobileGL bug to catch in a validating lane, not something a + // shipping build can be handed by an application. The message is latched all the same + // - the pass-through cache is keyed on patchVertices, so a broken template would + // otherwise re-report once per distinct patch size. + Bool validateThisOne = false; #if MOBILEGL_LOG_ACTIVE_LEVEL <= MOBILEGL_LOG_LEVEL_DEBUG - ValidateTransformedSpirv(spirv, ShaderStage::TessControl, 0); + validateThisOne = true; #else - if (m_enableSpirvValidation) { - MG_Util::ShaderTranspiler::ShaderCompiler::PrepareSpirvValidation(); - ValidateTransformedSpirv(spirv, ShaderStage::TessControl, 0); - } + validateThisOne = m_enableSpirvValidation; + if (validateThisOne) MG_Util::ShaderTranspiler::ShaderCompiler::PrepareSpirvValidation(); #endif + SpirvValidationFailure passthroughFailure{}; + if (validateThisOne && + !ValidateTransformedSpirv(spirv, ShaderStage::TessControl, 0, &passthroughFailure)) { + MGLOG_E_ONCE("ProgramFactory: the synthesized pass-through tessellation control stage for " + "patchVertices=%u does not validate (%s)", + patchVertices, passthroughFailure.message.c_str()); + } + } VkShaderModuleCreateInfo smci{VK_STRUCTURE_TYPE_SHADER_MODULE_CREATE_INFO}; smci.codeSize = spirv.size() * sizeof(Uint); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index 62f53253..eb841c71 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -202,6 +202,20 @@ namespace MobileGL::MG_Backend::DirectVulkan { // tessellation stages are present or neither // (VUID-VkGraphicsPipelineCreateInfo-pStages-00730). So the draw path has to supply // the pass-through stage GL describes; see GetOrCreatePassthroughTessControlStage. + // True when this program was built AS a transform-feedback capture variant but its + // last pre-rasterization module does NOT carry the Xfb execution mode - so the + // renderer must decline the capture span instead of issuing + // vkCmdBeginTransformFeedbackEXT against it + // (VUID-vkCmdBeginTransformFeedbackEXT-None-04128). + // + // Two ways to get here, and neither is visible from GL state, which is all + // BeginXfbCaptureForDraw otherwise consults: the clip/XFB validation backstop had to + // rewind past the capture decoration, or XfbCaptureDecoratePass resolved none of the + // requested varyings and returned without changing anything (its own MGLOG_E path) + // while its runner still reported success. Both used to ship a non-Xfb module under + // an Xfb-flagged cache entry - the flag and the layout are part of the program cache + // key, so it was sticky for every later captured draw of the program, not a glitch. + Bool xfbCaptureDeclined = false; Bool needsPassthroughTessControl = false; // ...and the pass-through this renderer can synthesize carries gl_Position and // nothing else, so it is only correct when the evaluation stage's inputs are diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index 336a1813..a2465a58 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -37,10 +37,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { // glGenTextures ever hands this out, and nothing looks a placeholder up by name - so the // id only has to stay clear of the application's, exactly like the sampled fallback's. constexpr Uint kUnboundStorageImageExternalIndex = 0xFFFFFF01u; - // The multisample sampled fallbacks. Separate ids for the same reason as the two above: - // they must not collide with anything glGenTextures can hand out. - constexpr Uint kFallbackTexture2DMultisampleExternalIndex = 0xFFFFFF02u; - constexpr Uint kFallbackTexture2DMultisampleArrayExternalIndex = 0xFFFFFF03u; + // The multisample sampled fallbacks: one per (target, numeric domain), because unlike the + // single-sampled fallback they cannot be reinterpreted into another domain at view time + // (see GetFallbackMultisampleTexture). Six reserved ids, contiguous from this base for the + // same reason as the two above - they must not collide with anything glGenTextures can + // hand out. + constexpr Uint kFallbackMultisampleExternalIndexBase = 0xFFFFFF02u; + constexpr Uint kFallbackMultisampleExternalIndexCount = 6u; // MobileGL's own stand-in textures, by the reserved ids above. Nothing an application can // do reaches one, so anything keyed on the GL object an application bound - image-unit @@ -49,8 +52,8 @@ namespace MobileGL::MG_Backend::DirectVulkan { if (texture == nullptr) return false; const Uint index = static_cast(texture->GetExternalIndex()); return index == kFallbackTexture2DExternalIndex || index == kUnboundStorageImageExternalIndex || - index == kFallbackTexture2DMultisampleExternalIndex || - index == kFallbackTexture2DMultisampleArrayExternalIndex; + (index >= kFallbackMultisampleExternalIndexBase && + index < kFallbackMultisampleExternalIndexBase + kFallbackMultisampleExternalIndexCount); } // The R32 member of each numeric class. Every one of the three is a MANDATORY-support @@ -375,8 +378,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { m_textureManager = nullptr; m_samplerManager = nullptr; m_fallbackTexture2D.reset(); - m_fallbackTexture2DMultisample.reset(); - m_fallbackTexture2DMultisampleArray.reset(); + m_fallbackMultisampleTextures.clear(); } void UniformManager::BeginFrame(Uint32 frameIndex) { @@ -513,7 +515,10 @@ namespace MobileGL::MG_Backend::DirectVulkan { texture = nullptr; } if (texture == nullptr) { - fallbackHolder = GetFallbackTexture(preferredTarget); + // The binding's sampler class, read here rather than through the `numericDomain` + // local further down (it is declared after this point): the multisample placeholder + // has to be built in the class the shader will read it in. + fallbackHolder = GetFallbackTexture(preferredTarget, programObj.samplerNumericDomainByBinding[binding]); texture = fallbackHolder.get(); if (texture == nullptr) { MGLOG_E_ONCE("ResolveSamplerDescriptor: no fallback texture available for binding=%u ('%s') " @@ -1384,7 +1389,8 @@ namespace MobileGL::MG_Backend::DirectVulkan { return outImageInfo.imageView != VK_NULL_HANDLE; } - SharedPtr UniformManager::GetFallbackTexture(TextureTarget target) const { + SharedPtr UniformManager::GetFallbackTexture( + TextureTarget target, SamplerNumericDomain numericDomain) const { // A multisample sampler cannot be served by the single-sampled 2D image below - its // descriptor demands a multisample view - so it gets its own placeholder rather than no // placeholder at all. Without one, ResolveSamplerDescriptor declined and @@ -1395,7 +1401,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // incomplete texture is undefined, not fatal, so the draw has to happen. if (target == TextureTarget::Texture2DMultisample || target == TextureTarget::Texture2DMultisampleArray) { - return GetFallbackMultisampleTexture(target); + return GetFallbackMultisampleTexture(target, numericDomain); } if (target != TextureTarget::Texture2D && target != TextureTarget::TextureRectangle) { MGLOG_E_ONCE("UniformManager::GetFallbackTexture: no fallback exists for target=%d", @@ -1403,6 +1409,10 @@ namespace MobileGL::MG_Backend::DirectVulkan { return nullptr; } + // The single-sampled fallback stays domain-agnostic: it is storage-image capable, so its + // image carries VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT and ResolveSampledImageViewFormat can + // hand an integer sampler an R8G8B8A8_UINT view of these same RGBA8 texels. A multisample + // image can never carry that bit, which is why the arm above needs one object per domain. if (m_fallbackTexture2D == nullptr) { auto fallbackTexture = MakeShared(kFallbackTexture2DExternalIndex); fallbackTexture->SetInternalFormat(TextureInternalFormat::RGBA8); @@ -1421,24 +1431,56 @@ namespace MobileGL::MG_Backend::DirectVulkan { } SharedPtr UniformManager::GetFallbackMultisampleTexture( - TextureTarget target) const { + TextureTarget target, SamplerNumericDomain numericDomain) const { + // ONE PLACEHOLDER PER NUMERIC DOMAIN, unlike the single-sampled fallback. + // + // A descriptor whose image format is in a different numeric class than the sampler that + // reads it needs a format-reinterpreting view, and building one needs + // VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT on the image. A multisample image can never have it: + // SyncTextureResource computes storageImageCapable as `!isMultisampleTexture && ...`, and + // the only other source of the bit is the sRGB twin, which RGBA8 is not. So an RGBA8 + // placeholder handed to a usampler2DMS made GetOrCreateSampledImageView bail with "needs + // mutable image format", ResolveSamplerDescriptor return false, and the draw be dropped - + // the exact outcome the placeholder exists to prevent, just reached later. Matching the + // image's own format to the sampler's class instead means no reinterpreting view is + // needed at all. const Bool arrayed = target == TextureTarget::Texture2DMultisampleArray; - auto& slot = arrayed ? m_fallbackTexture2DMultisampleArray : m_fallbackTexture2DMultisample; - if (slot != nullptr) { - return slot; + TextureInternalFormat internalFormat = TextureInternalFormat::RGBA8; + Uint32 domainSlot = 0; + switch (numericDomain) { + case SamplerNumericDomain::SignedInteger: + internalFormat = TextureInternalFormat::RGBA8I; + domainSlot = 1; + break; + case SamplerNumericDomain::UnsignedInteger: + internalFormat = TextureInternalFormat::RGBA8UI; + domainSlot = 2; + break; + case SamplerNumericDomain::Float: + case SamplerNumericDomain::Unknown: + default: + // Unknown reads as float, matching PlaceholderFormatForNumericDomain's own default: + // a shader whose sampler class could not be reflected is far likelier to be a plain + // sampler2DMS than an integer one, and a float view is the only one buildable without + // the mutable bit anyway. + break; + } + const Uint32 key = (arrayed ? kFallbackMultisampleExternalIndexCount / 2 : 0u) + domainSlot; + auto cached = m_fallbackMultisampleTextures.find(key); + if (cached != m_fallbackMultisampleTextures.end()) { + return cached->second; } const TextureUploadTarget uploadTarget = arrayed ? TextureUploadTarget::Texture2DMultisampleArray : TextureUploadTarget::Texture2DMultisample; + const Uint externalIndex = kFallbackMultisampleExternalIndexBase + key; SharedPtr texture; if (arrayed) { - texture = MakeShared( - kFallbackTexture2DMultisampleArrayExternalIndex); + texture = MakeShared(externalIndex); } else { - texture = MakeShared( - kFallbackTexture2DMultisampleExternalIndex); + texture = MakeShared(externalIndex); } - texture->SetInternalFormat(TextureInternalFormat::RGBA8); + texture->SetInternalFormat(internalFormat); // TWO samples, never one. VUID-RuntimeSpirv-samples-08726 forbids an OpTypeImage with // MS = 1 from reading a VK_SAMPLE_COUNT_1_BIT image, which is exactly the hazard // VkTextureManager::SyncTextureResource's one-sample floor exists to avoid; a placeholder @@ -1453,10 +1495,16 @@ namespace MobileGL::MG_Backend::DirectVulkan { texture->AllocateStorage(uploadTarget, 0, {.texelSize = {1, 1, 1}, .byteSize = 0}); texture->TruncateMipmapLevels(uploadTarget, 1); texture->MarkStorageDirty(uploadTarget, 0, false); - slot = texture; - MGLOG_D("UniformManager::GetFallbackMultisampleTexture: created placeholder target=%d", - static_cast(target)); - return slot; + // Worth knowing if it ever fires: an integer multisample format can legitimately support + // no count above one on a device (framebufferIntegerColorSampleCounts is allowed to be + // VK_SAMPLE_COUNT_1_BIT), and SyncTextureResource's round-down would then hand this + // placeholder a single-sampled image, which is the samples-08726 shape the SetSamples(2) + // above exists to avoid. It already warns from there; nothing better is available - a + // one-sample integer image is still a draw, and declining is the outcome this whole + // placeholder replaced. + MGLOG_D("UniformManager::GetFallbackMultisampleTexture: created placeholder target=%d domain=%d format=%d", + static_cast(target), static_cast(numericDomain), static_cast(internalFormat)); + return m_fallbackMultisampleTextures.emplace(key, Move(texture)).first->second; } VkBufferView UniformManager::AcquireUnboundTexelBufferView(VkFormat declaredFormat, @@ -1645,7 +1693,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { // its first use instead of leaving that work to happen inside an active pass. // Ask GetFallbackTexture rather than re-listing the targets it serves: that list grew // a multisample arm and the two must not drift apart. - texture = GetFallbackTexture(preferredTarget).get(); + texture = GetFallbackTexture(preferredTarget, programObj.samplerNumericDomainByBinding[binding]).get(); if (texture == nullptr) { return false; } diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h index b5d4dd1a..d1b363ed 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.h @@ -179,11 +179,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { static MG_State::GLState::ITextureObject* ResolveSamplerTextureRaw( const MG_State::GLState::ProgramObject& program, const ProgramFactory::VkProgramObject& programObj, Uint32 binding, Uint32 element); - SharedPtr GetFallbackTexture(TextureTarget target) const; - // The multisample arm of GetFallbackTexture. Separate object per target and no upload - // path: a multisample image cannot be written by a transfer, so its texels stay undefined - // - which is what GL promises for a texelFetch on an incomplete multisample texture. - SharedPtr GetFallbackMultisampleTexture(TextureTarget target) const; + // `numericDomain` is the sampler's class, and it matters only for the multisample arm - + // see GetFallbackMultisampleTexture for why the single-sampled fallback can ignore it. + SharedPtr GetFallbackTexture( + TextureTarget target, SamplerNumericDomain numericDomain) const; + // The multisample arm of GetFallbackTexture. One object per (target, numeric domain) and + // no upload path: a multisample image cannot be written by a transfer, so its texels stay + // undefined - which is what GL promises for a texelFetch on an incomplete multisample + // texture - and it cannot carry MUTABLE_FORMAT, so its format has to match the sampler's + // class outright rather than being reinterpreted at view time. + SharedPtr GetFallbackMultisampleTexture( + TextureTarget target, SamplerNumericDomain numericDomain) const; // ---- placeholders for UNBOUND image-backed descriptors ------------------------- // GL lets a program declare `samplerBuffer`, `imageBuffer` or `image2D` and bind nothing // to the unit it names: the fetch is then undefined (GL 4.6 core 8.9 for an incomplete @@ -297,8 +303,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { VkTextureManager* m_textureManager = nullptr; VkSamplerManager* m_samplerManager = nullptr; mutable SharedPtr m_fallbackTexture2D; - mutable SharedPtr m_fallbackTexture2DMultisample; - mutable SharedPtr m_fallbackTexture2DMultisampleArray; + // Keyed by (arrayed, numeric domain); see GetFallbackMultisampleTexture. Lazily populated, + // never evicted - at most six tiny 1x1 images - and torn down with the manager. + mutable UnorderedMap> m_fallbackMultisampleTextures; // See AcquireUnboundTexelBufferView / GetUnboundStorageImageTexture. Both are lazily // populated, never evicted (a program's declared formats are a fixed, tiny set) and torn // down with the manager. The texel views are keyed by format AND by storage-vs-sampled diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 89f3abed..22492d92 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -4776,7 +4776,34 @@ void main() { // when the device lacks independentBlend - the same read the payload does) // FBO-derived payload inputs (attachment presence/formats/draw-buffer gating) are // pinned by the render-pass hash key, exactly as the version-keyed memo relied on. - Uint64 VulkanRenderer::ComputePipelineStateHash(Uint32 colorAttachmentCount) const { + // The fixed-function sample mask this draw actually gets, and the ONE place that decides it. + // + // GL 4.6 core 17.3.3 puts SAMPLE_MASK/SAMPLE_MASK_VALUE among the multisample fragment + // operations and says they make no change "if MULTISAMPLE is disabled, or if the value of + // SAMPLE_BUFFERS is not one" - so on a single-sample draw framebuffer the mask is a no-op. + // Vulkan has no such rule: pSampleMask is ANDed with coverage at every rasterizationSamples, + // and at one sample that coverage is bit 0 alone. Handing the raw GL word straight through + // therefore turned `glEnable(GL_SAMPLE_MASK); glSampleMaski(0, 0x2);` followed by a draw to + // the default framebuffer - the ordinary MSAA-render-then-present shape, and what dEQP's + // multisample cases leave enabled - into a fully discarded, black draw. All-ones restores + // the null-pSampleMask meaning the pipeline had before the mask was plumbed at all. + // + // SAMPLE_BUFFERS is the load-bearing half: MultisampleEnabled defaults to TRUE, so the + // capability check alone would gate nothing. It is here for spec completeness - GL lets + // glDisable(GL_MULTISAMPLE) switch the whole step off on a multisample target too. + // + // Both callers - the payload and ComputePipelineStateHash's memo word - go through this, so + // the memo key cannot describe a different mask than the pipeline was built with. + Uint32 VulkanRenderer::ResolveEffectiveSampleMask(VkSampleCountFlagBits rasterizationSamples) const { + constexpr Uint32 kFullCoverage = 0xffffffffu; + if (rasterizationSamples == VK_SAMPLE_COUNT_1_BIT) return kFullCoverage; + if (!MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::Multisample)) return kFullCoverage; + if (!MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::SampleMask)) return kFullCoverage; + return MG_State::pGLContext->GetRenderStateParameters().SampleMaskValue; + } + + Uint64 VulkanRenderer::ComputePipelineStateHash(Uint32 colorAttachmentCount, + VkSampleCountFlagBits rasterizationSamples) const { // One bulk fetch instead of ~17 per-field accessor calls into MG_State: every // input below is a plain field of RenderStateParameters, and each accessor this // replaces (IsCapabilityEnabled / Get*) is a verified pure read of that same @@ -4795,7 +4822,13 @@ void main() { capabilityBits |= p.PrimitiveRestartFixedIndexEnabled ? 1ull << 7 : 0; capabilityBits |= p.DepthMask ? 1ull << 8 : 0; capabilityBits |= p.SampleShadingEnabled ? 1ull << 9 : 0; - capabilityBits |= p.SampleMaskEnabled ? 1ull << 10 : 0; + // The EFFECTIVE mask enable, not the raw GL bit: at one sample GL says the whole + // multisample fragment-operations step makes no change, so the pipeline is built with + // full coverage and the memo word has to say so too. Keying on the raw bit here while + // the payload gates on the sample count would let one FBO's cached pipeline answer for + // another whose sample count reads the mask differently. + const Bool sampleMaskEffective = ResolveEffectiveSampleMask(rasterizationSamples) != 0xffffffffu; + capabilityBits |= sampleMaskEffective ? 1ull << 10 : 0; Uint64 hash = CombinePipelineStateWord(0x243F6A8885A308D3ull, capabilityBits); // glMinSampleShading. Hashed by BITS, not by value: this memo compares hashes rather than // versions, so an unhashed float would let a pipeline built at one rate be handed back @@ -4811,7 +4844,7 @@ void main() { // while GL_SAMPLE_MASK is enabled - the enable bit is already in capabilityBits, and // folding one more word costs nothing on a path that only recomputes when the // pipeline-state version moved. - hash = CombinePipelineStateWord(hash, static_cast(p.SampleMaskValue)); + hash = CombinePipelineStateWord(hash, static_cast(ResolveEffectiveSampleMask(rasterizationSamples))); hash = CombinePipelineStateWord(hash, static_cast(p.PatchVertices)); // The default tessellation levels belong here for the same reason PatchVertices does: // when a program has an evaluation stage and no control stage, both are compiled into the @@ -4935,10 +4968,13 @@ void main() { // The version only guards recomputing the hash - unchanged version, unchanged bytes. const Uint renderStateVersion = MG_State::pGLContext->GetPipelineStateVersion(); if (!m_pipelineStateHashValid || m_pipelineStateHashVersion != renderStateVersion || - m_pipelineStateHashColorCount != renderPassEntry.colorAttachmentCount) { - m_pipelineStateHash = ComputePipelineStateHash(renderPassEntry.colorAttachmentCount); + m_pipelineStateHashColorCount != renderPassEntry.colorAttachmentCount || + m_pipelineStateHashSampleCount != renderPassEntry.sampleCount) { + m_pipelineStateHash = + ComputePipelineStateHash(renderPassEntry.colorAttachmentCount, renderPassEntry.sampleCount); m_pipelineStateHashVersion = renderStateVersion; m_pipelineStateHashColorCount = renderPassEntry.colorAttachmentCount; + m_pipelineStateHashSampleCount = renderPassEntry.sampleCount; m_pipelineStateHashValid = true; } const Uint64 pipelineStateHash = m_pipelineStateHash; @@ -5207,12 +5243,8 @@ void main() { .sampleShadingEnable = m_sampleRateShadingFeatureEnabled && MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::SampleShading), .minSampleShading = MG_State::pGLContext->GetMinSampleShadingValue(), - // GL_SAMPLE_MASK off means full coverage, which is what an all-ones mask says and what - // a null pSampleMask used to say by omission. One word: MaxSampleMaskWords is clamped - // to 1 on both backends, so glSampleMaski only ever writes index 0. - .sampleMask = MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::SampleMask) - ? MG_State::pGLContext->GetRenderStateParameters().SampleMaskValue - : 0xffffffffu, + // Word 1 keeps its all-ones initialiser: GL has no state for samples 32..63. + .sampleMask = {ResolveEffectiveSampleMask(renderPassEntry.sampleCount), 0xffffffffu}, .subpass = 0, .topology = vkTopology, .primitiveRestartEnable = primitiveRestartEnabled, @@ -6075,6 +6107,10 @@ void main() { snap.programFactoryEpoch = m_programFactory->GetCacheStructureEpoch(); } const auto& programObj = *programObjPtr; + // Pinned for BeginXfbCaptureForDraw, which otherwise decides from GL state alone and has + // no way to know the bound pipeline's last pre-rasterization module lost (or never got) + // its Xfb execution mode. See VkProgramObject::xfbCaptureDeclined. + m_currentDrawXfbCaptureDeclined = programObj.xfbCaptureDeclined; // The pipeline and the vertex-input pre-flight depend on the VAO only through // its resolved LAYOUT (layoutHash folds the attribute formats, bindings and the @@ -6237,10 +6273,13 @@ void main() { // instead of missing forever on a monotonic version. A miss falls through // to the full lookup. if (!m_pipelineStateHashValid || m_pipelineStateHashVersion != renderStateVersion || - m_pipelineStateHashColorCount != snap.renderPassColorCount) { - m_pipelineStateHash = ComputePipelineStateHash(snap.renderPassColorCount); + m_pipelineStateHashColorCount != snap.renderPassColorCount || + m_pipelineStateHashSampleCount != snap.renderPassSampleCount) { + m_pipelineStateHash = + ComputePipelineStateHash(snap.renderPassColorCount, snap.renderPassSampleCount); m_pipelineStateHashVersion = renderStateVersion; m_pipelineStateHashColorCount = snap.renderPassColorCount; + m_pipelineStateHashSampleCount = snap.renderPassSampleCount; m_pipelineStateHashValid = true; } const auto memoTransformFlags = @@ -6475,6 +6514,10 @@ void main() { } } const auto& programObj = *resolvedProgramObj; + // Pinned for BeginXfbCaptureForDraw, which otherwise decides from GL state alone and has + // no way to know the bound pipeline's last pre-rasterization module lost (or never got) + // its Xfb execution mode. See VkProgramObject::xfbCaptureDeclined. + m_currentDrawXfbCaptureDeclined = programObj.xfbCaptureDeclined; // For the snapshot's memoised entry pointer: if anything below inserts into the // program cache (blit/aux program compiles), the epoch moves and the snapshot // stores no pointer for this draw - the fast path then re-looks-up once. @@ -6513,11 +6556,31 @@ void main() { const Uint64 programLifetimeId = program.GetLifetimeId(); const Uint32 programVersion = program.GetBackendStateVersion(); const Uint64 bindGeneration = MG_State::pGLContext->GetTextureBindGeneration(); + // The bind generation alone stopped covering this set the moment ResolveSampledBinding + // started asking SamplesAsIncompleteTexture: membership now depends on the effective + // sampler PARAMETERS (MIN_FILTER decides whether the mip chain is read at all) and on + // the texture SHAPE, and neither moves the bind generation. A texture that flips + // incomplete -> complete under a fixed binding - one glTexParameteri, one + // glSamplerParameteri, a BASE_LEVEL/MAX_LEVEL change, or an upload that fills the + // chain - would keep replaying the FALLBACK out of this memo, so the real texture + // never got its pre-pass sync, its pending-clear materialisation or its sampled-layout + // transition, and the descriptor path would then transition it from INSIDE the open + // render pass, which the subpass declares no self-dependency for. + // + // The sampling-resolution generation is exactly the counter for that family and is + // deliberately coarse (any texture, any sampler), so this one term covers every input + // the predicate reads that the bind generation does not: TextureObjectBase:: + // BumpShapeVersion and SamplerObject::BumpVersion both bump it, while WHICH sampler + // object a unit carries goes through TextureUnit::SetSamplerObject and moves the bind + // generation instead. Same term the SetupDrawSnapshot fast path and the LOD memo + // already carry. + const Uint64 samplingGeneration = MG_State::pGLContext->GetSamplingResolutionGeneration(); const Bool sampledSetUnchanged = m_lastSampledSetValid && m_lastSampledSetProgramLifetimeId == programLifetimeId && m_lastSampledSetProgramVersion == programVersion && m_lastSampledSetTransformFlags == transformFlags && - m_lastSampledSetBindGeneration == bindGeneration; + m_lastSampledSetBindGeneration == bindGeneration && + m_lastSampledSetSamplingGeneration == samplingGeneration; if (!sampledSetUnchanged) { const Bool hasSampledTextures = m_uniformManager->CollectSampledTextures( program, programObj, sampledTextures, &m_sampledBindingRecordsScratch); @@ -6527,6 +6590,7 @@ void main() { m_lastSampledSetProgramVersion = programVersion; m_lastSampledSetTransformFlags = transformFlags; m_lastSampledSetBindGeneration = bindGeneration; + m_lastSampledSetSamplingGeneration = samplingGeneration; } // Complete a freshly-made LOD decision (see above): its params sum // can only be taken once the sampled set is known. A genuine @@ -6825,6 +6889,7 @@ void main() { snap.drawUsesDepthStencil = drawUsesDepthStencil; snap.renderPassExtent = renderPassEntry->extent; snap.renderPassColorCount = renderPassEntry->colorAttachmentCount; + snap.renderPassSampleCount = renderPassEntry->sampleCount; snap.pipeline = pipeline; // The layout identity the fast path's aux-memo compare answers against. // A memo hit here, not a rebuild: the pre-flight above resolved this @@ -11057,6 +11122,19 @@ void main() { if (!program || program->GetTransformFeedbackVaryingCount() == 0) { return false; } + // The bound pipeline's last pre-rasterization stage has to have been declared with Xfb + // (VUID-vkCmdBeginTransformFeedbackEXT-None-04128). Everything above this line reads GL + // state, which cannot answer that: a program can be built as a capture variant and still + // end up with a module carrying no Xfb mode - the clip/XFB validation backstop rewinding + // past the decoration, or XfbCaptureDecoratePass resolving none of the requested varyings + // and changing nothing. Declining the span leaves the capture buffers untouched, which is + // the same nothing the driver would have written, without the undefined behaviour. + if (m_currentDrawXfbCaptureDeclined) { + MGLOG_E_ONCE("BeginXfbCaptureForDraw: declining the capture span - the bound program's last " + "pre-rasterization stage carries no Xfb execution mode, so recording one would be " + "undefined behaviour rather than a capture"); + return false; + } const SizeT bufferCount = std::min(program->GetTransformFeedbackBufferCount(), 4); if (bufferCount == 0) { return false; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h index 9f3648ed..4929105d 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h @@ -768,9 +768,18 @@ namespace MobileGL::MG_Backend::DirectVulkan { // version: the version is monotonic and bumps on every pipeline-state // change, so an unchanged (version, colorAttachmentCount) proves the state // bytes are unchanged and the hash can be reused without re-reading them. - Uint64 ComputePipelineStateHash(Uint32 colorAttachmentCount) const; + Uint64 ComputePipelineStateHash(Uint32 colorAttachmentCount, + VkSampleCountFlagBits rasterizationSamples) const; + // The effective GL_SAMPLE_MASK word for a draw at this rasterization sample count; see + // the definition for the GL-vs-Vulkan rule it reconciles. Shared by the pipeline payload + // and the pipeline-state memo word so the two cannot disagree. + Uint32 ResolveEffectiveSampleMask(VkSampleCountFlagBits rasterizationSamples) const; Uint m_pipelineStateHashVersion = 0; Uint32 m_pipelineStateHashColorCount = 0; + // The sample count the cached hash was computed at. A pipeline-state input now depends on + // it (the effective sample mask), so a draw that changes only the target's sample count + // has to recompute rather than reuse. + VkSampleCountFlagBits m_pipelineStateHashSampleCount = VK_SAMPLE_COUNT_1_BIT; Uint64 m_pipelineStateHash = 0; Bool m_pipelineStateHashValid = false; // GetShaderTransformFlags memo. NOT pure in the pre-transform alone: the @@ -812,7 +821,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { // Skip the per-draw CollectSampledTextures walk (~5% of the render thread) when the sampled // texture SET is provably unchanged from the previous draw: same program (lifetime id + // backend-state version, which covers sampler-uniform reassignment / relink) and transform - // flags, and no texture bind/unbind/delete since (GetTextureBindGeneration). On a hit, + // flags, no texture bind/unbind/delete since (GetTextureBindGeneration), and nothing that + // moves a texture's shape or a sampler's parameters since (GetSamplingResolutionGeneration + // - membership depends on mipmap-completeness, which both of those decide). On a hit, // m_sampledTexturesScratch still holds the previous draw's list and steps 2-4 (feedback / // layout probe / transition) re-run on it, so layout correctness is unaffected - only the GL // walk is skipped. The program lifetime id (never reused, unlike the GL name) and the @@ -823,6 +834,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { Uint32 m_lastSampledSetProgramVersion = 0; ProgramFactory::CompileOptionFlags m_lastSampledSetTransformFlags = {}; Uint64 m_lastSampledSetBindGeneration = 0; + Uint64 m_lastSampledSetSamplingGeneration = 0; + // Set from the draw's resolved VkProgramObject on both the full and the fast setup paths; + // read by BeginXfbCaptureForDraw, which has only GL state otherwise. See + // VkProgramObject::xfbCaptureDeclined. + Bool m_currentDrawXfbCaptureDeclined = false; // Memo for the per-draw explicit-LOD-0 eligibility probe // (ProgramSamplesOnlySingleLevelTextures): same key family as the @@ -921,6 +937,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { // probe the pipeline memo after a state change without re-fetching the // render-pass entry (the pass itself is pinned by renderPassHash above). Uint32 renderPassColorCount = 0; + // Pinned with the colour count and for the same reason: the fast path recomputes the + // pipeline-state value hash from the snapshot, and that hash reads the sample count. + VkSampleCountFlagBits renderPassSampleCount = VK_SAMPLE_COUNT_1_BIT; VkPipeline pipeline = VK_NULL_HANDLE; // layoutHash of the snapshotting draw's vertex-input state. The pipeline and // the vertex-input pre-flight depend on the VAO only through this (plus the diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index f31b17a9..c363ad92 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -59,6 +59,7 @@ add_executable(MobileGLIntegrationTest Scenarios/AsyncCompileScenario.cpp Scenarios/XfbAfterClipDistanceScenario.cpp Scenarios/UnwrittenPositionOutputScenario.cpp + Scenarios/SampleMaskScopeScenario.cpp Scenarios/ThreeChannelAttachmentScenario.cpp Scenarios/SnormAttachmentScenario.cpp Scenarios/PipelineFailureScenario.cpp diff --git a/MobileGL/MG_IntegrationTest/Scenarios/SampleMaskScopeScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/SampleMaskScopeScenario.cpp new file mode 100644 index 00000000..160f6996 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/SampleMaskScopeScenario.cpp @@ -0,0 +1,178 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/SampleMaskScopeScenario.cpp +// Copyright (c) 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 +// +// Scenario - GL_SAMPLE_MASK IS A MULTISAMPLE FRAGMENT OPERATION, SO IT DOES NOTHING AT ONE SAMPLE. +// +// GL 4.6 core 17.3.3 groups alpha-to-coverage, sample coverage and the sample mask together and +// says they make no change "if MULTISAMPLE is disabled, or if the value of SAMPLE_BUFFERS is not +// one". SAMPLE_BUFFERS is 0 for a single-sample framebuffer, so on one the mask is inert whatever +// glSampleMaski last wrote. +// +// Vulkan has no such rule. VkPipelineMultisampleStateCreateInfo::pSampleMask is ANDed with +// rasterization coverage at every rasterizationSamples, and at one sample that coverage is bit 0 +// alone - so a mask with bit 0 clear discards every fragment of every primitive. Plumbing +// glSampleMaski straight into pSampleMask therefore turned an ordinary and legal GL sequence into +// a fully black draw: +// +// glEnable(GL_SAMPLE_MASK); glSampleMaski(0, 0x2); // while an MSAA target is bound +// ... render ... +// glBindFramebuffer(GL_FRAMEBUFFER, 0); draw a fullscreen quad to present +// +// Neither piece of state is per-framebuffer, so nothing resets it when the target changes, and +// dEQP/GL-CTS multisample cases leave exactly these masks behind. That is the MSAA-then-present +// shape every application uses. +// +// The cases below are single-sample by construction (the scenario harness's colour FBO), so each +// one asserts that the mask changed nothing. + +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr int kFboSize = 32; + + constexpr const char* kQuadVertexSource = R"(#version 430 core +void main() { + vec2 corner = vec2((gl_VertexID & 1) == 0 ? -1.0 : 1.0, + (gl_VertexID & 2) == 0 ? -1.0 : 1.0); + gl_Position = vec4(corner, 0.0, 1.0); +} +)"; + + constexpr const char* kGreenFragmentSource = R"(#version 430 core +out vec4 o_color; +void main() { + o_color = vec4(0.0, 1.0, 0.0, 1.0); +} +)"; + + class SampleMaskScopeScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + + m_target = MakeColorFbo(kFboSize, kFboSize); + ASSERT_NE(m_target.fbo, 0u) << "could not create the render target"; + glGenVertexArrays(1, &m_vao); + std::string error; + m_program = CompileProgram(kQuadVertexSource, kGreenFragmentSource, &error); + ASSERT_NE(m_program, 0u) << error; + } + + void TearDown() override { + if (!Ready()) return; + // Process-wide GL state: leaving it set would hand the next scenario in this + // process the very bug under test. + glDisable(GL_SAMPLE_MASK); + glSampleMaski(0, 0xFFFFFFFFu); + glBindVertexArray(0); + glUseProgram(0); + if (m_program != 0) glDeleteProgram(m_program); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + DestroyColorFbo(m_target); + ScenarioTest::TearDown(); + } + + void ExpectQuadStillPaints(const char* what) { + BindFbo(m_target); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + glBindVertexArray(m_vao); + glUseProgram(m_program); + glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); + glBindVertexArray(0); + EXPECT_EQ(FirstGLError(), 0u) << what << ": the draw raised a GL error"; + + const Image image = ReadPixels(kFboSize, kFboSize); + ASSERT_FALSE(image.Empty()) << what << ": the readback came back empty"; + EXPECT_TRUE(RegionIsMostly(image, 0, kFboSize - 1, 0, kFboSize - 1, "green", 0.0, what)) + << what << ": an all-black target means the sample mask discarded every fragment, " + << "which GL says it cannot do on a single-sample framebuffer"; + } + + ColorFbo m_target{}; + GLuint m_vao = 0; + unsigned int m_program = 0; + }; + + } // namespace + + // The exact reported shape: bit 0 clear, so the single sample of a single-sample target is + // masked off if the mask is applied at all. + TEST_F(SampleMaskScopeScenario, AMaskWithBitZeroClearDoesNotDiscardASingleSampleDraw) { + if (!Ready() || IsSkipped()) return; + glEnable(GL_SAMPLE_MASK); + glSampleMaski(0, 0x2); + ASSERT_EQ(FirstGLError(), 0u) << "setting the sample mask raised a GL error"; + ExpectQuadStillPaints("GL_SAMPLE_MASK enabled with mask 0x2"); + } + + // Zero is the strongest form of the same thing, and the mask value the CTS's mask_zero cases + // set. + TEST_F(SampleMaskScopeScenario, AZeroMaskDoesNotDiscardASingleSampleDraw) { + if (!Ready() || IsSkipped()) return; + glEnable(GL_SAMPLE_MASK); + glSampleMaski(0, 0x0); + ASSERT_EQ(FirstGLError(), 0u) << "setting the sample mask raised a GL error"; + ExpectQuadStillPaints("GL_SAMPLE_MASK enabled with mask 0"); + } + + // Control: the same mask word with the capability disabled has never had any effect, so this + // one passed before the fix too. It is here so a regression that ignores the enable bit + // instead of the sample count is still caught. + TEST_F(SampleMaskScopeScenario, ADisabledSampleMaskDoesNotDiscardASingleSampleDraw) { + if (!Ready() || IsSkipped()) return; + glDisable(GL_SAMPLE_MASK); + glSampleMaski(0, 0x0); + ASSERT_EQ(FirstGLError(), 0u) << "setting the sample mask raised a GL error"; + ExpectQuadStillPaints("GL_SAMPLE_MASK disabled with mask 0"); + } + + // The mask is state, not a draw parameter, so a second draw after the first must not inherit + // a pipeline built while the memo word and the payload disagreed. Two draws either side of a + // mask change, both to the same single-sample target, both required to paint. + TEST_F(SampleMaskScopeScenario, ChangingTheMaskBetweenSingleSampleDrawsKeepsBothPainting) { + if (!Ready() || IsSkipped()) return; + glEnable(GL_SAMPLE_MASK); + glSampleMaski(0, 0xFFFFFFFFu); + ExpectQuadStillPaints("first draw, full mask"); + glSampleMaski(0, 0x2); + ASSERT_EQ(FirstGLError(), 0u) << "changing the sample mask raised a GL error"; + ExpectQuadStillPaints("second draw, mask 0x2"); + } + + // GL_MAX_SAMPLE_MASK_WORDS must be 1 on both backends: MobileGL stores one word and + // SampleMaski_State raises GL_INVALID_VALUE for any maskNumber above 0, so advertising more + // makes dEQP's per-case gluStateReset - which issues glSampleMaski up to the advertised count + // - fail every case. DirectGLES clamped; DirectVulkan forwarded the raw device limit. + TEST_F(SampleMaskScopeScenario, TheAdvertisedSampleMaskWordCountMatchesWhatSampleMaskiAccepts) { + if (!Ready() || IsSkipped()) return; + GLint words = 0; + glGetIntegerv(GL_MAX_SAMPLE_MASK_WORDS, &words); + ASSERT_EQ(FirstGLError(), 0u) << "querying GL_MAX_SAMPLE_MASK_WORDS raised a GL error"; + EXPECT_EQ(words, 1) << "every word below the advertised count must be writable, and only word 0 is"; + for (GLint word = 0; word < words; ++word) { + glSampleMaski(static_cast(word), 0xFFFFFFFFu); + EXPECT_EQ(FirstGLError(), 0u) << "glSampleMaski(" << word << ", ...) was refused although " + << "GL_MAX_SAMPLE_MASK_WORDS advertises " << words << " words"; + } + } + +} // namespace MGITest diff --git a/MobileGL/MG_IntegrationTest/Scenarios/UnboundImageDescriptorScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/UnboundImageDescriptorScenario.cpp index 4d15cd79..b9e9de8a 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/UnboundImageDescriptorScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/UnboundImageDescriptorScenario.cpp @@ -155,6 +155,37 @@ void main() { } o_color = color; } +)"; + + // The integer spellings of the same thing. These are the ones a plain RGBA8 multisample + // placeholder cannot serve: a multisample image can never carry MUTABLE_FORMAT, so the + // reinterpreting view an integer sampler would need over UNORM texels is unbuildable and + // the descriptor resolve used to fail, losing the draw after the placeholder had already + // been created. + constexpr const char* kUsampler2DMSFragmentSource = R"(#version 430 core +uniform usampler2DMS u_unbound; +uniform int u_readUnbound; +out vec4 o_color; +void main() { + vec4 color = vec4(0.0, 1.0, 0.0, 1.0); + if (u_readUnbound != 0) { + color = vec4(texelFetch(u_unbound, ivec2(0), 0)); + } + o_color = color; +} +)"; + + constexpr const char* kIsampler2DMSFragmentSource = R"(#version 430 core +uniform isampler2DMS u_unbound; +uniform int u_readUnbound; +out vec4 o_color; +void main() { + vec4 color = vec4(0.0, 1.0, 0.0, 1.0); + if (u_readUnbound != 0) { + color = vec4(texelFetch(u_unbound, ivec2(0), 0)); + } + o_color = color; +} )"; constexpr const char* kImage2DFragmentSource = R"(#version 430 core @@ -454,6 +485,16 @@ void main() { ExpectDrawStillRuns(kSampler2DMSFragmentSource, "sampler2DMS"); } + TEST_F(UnboundImageDescriptorScenario, ADeclaredButUnboundUnsignedSampler2DMSDoesNotLoseTheDraw) { + if (!Ready() || IsSkipped()) return; + ExpectDrawStillRuns(kUsampler2DMSFragmentSource, "usampler2DMS"); + } + + TEST_F(UnboundImageDescriptorScenario, ADeclaredButUnboundSignedSampler2DMSDoesNotLoseTheDraw) { + if (!Ready() || IsSkipped()) return; + ExpectDrawStillRuns(kIsampler2DMSFragmentSource, "isampler2DMS"); + } + TEST_F(UnboundImageDescriptorScenario, AFormatlessWriteonlyImage2DLeftUnboundDoesNotLoseTheDispatch) { if (!Ready() || IsSkipped()) return; if (!LimitIsAtLeastOne(GL_MAX_COMPUTE_IMAGE_UNIFORMS)) { diff --git a/MobileGL/MG_State/GLState/TextureState/TextureState.h b/MobileGL/MG_State/GLState/TextureState/TextureState.h index c9d9d291..c85ae8a4 100644 --- a/MobileGL/MG_State/GLState/TextureState/TextureState.h +++ b/MobileGL/MG_State/GLState/TextureState/TextureState.h @@ -99,9 +99,14 @@ namespace MobileGL::MG_State::GLState { // is deliberately left unbound so it samples as (0,0,0,1)). Deliberately coarse - ANY // texture, ANY sampler - so that no mutation can slip past a per-unit binding memo; the // setters that feed it all early-out when the value is unchanged, so the redundant - // glTexParameteri calls applications issue every frame do not churn it. Kept separate - // from the bind generation because the sampled texture SET is unaffected by these, and - // the Vulkan backend's set memo keys on that one. + // glTexParameteri calls applications issue every frame do not churn it. + // + // Kept separate from the bind generation because the two answer different questions, NOT + // because the sampled texture SET is immune to this one - it is not, and the claim that + // it was is what this comment used to say. DirectVulkan leaves an incomplete texture out + // of the set entirely and substitutes a fallback, so completeness decides membership, and + // its sampled-set memo carries THIS generation alongside the bind one. Any memo of a + // resolved per-unit binding - or of which textures a draw samples at all - needs both. Uint64 GetSamplingResolutionGeneration() const { return m_samplingResolutionGeneration; } void BumpSamplingResolutionGeneration() { ++m_samplingResolutionGeneration; } diff --git a/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp b/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp index dfad5edf..447f8004 100644 --- a/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp +++ b/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp @@ -9,6 +9,7 @@ #include "Loader.h" #include +#include #include #include @@ -177,7 +178,15 @@ namespace MobileGL::MG_Util::BackendLoader { caps.MaxFramebufferSamples = ResolveConservativeFramebufferSampleLimit(p.limits); caps.MaxIntegerSamples = MaxSampleCountFromFlags(p.limits.sampledImageIntegerSampleCounts); caps.MaxSamples = caps.MaxFramebufferSamples; - caps.MaxSampleMaskWords = SaturateToInt(p.limits.maxSampleMaskWords); + // Clamped to one word, exactly as the GLES loader clamps the driver's value and for the + // same reason: MobileGL's sample-mask state IS a single 32-bit word + // (RenderState::SampleMaskValue) and SampleMaski_State() raises GL_INVALID_VALUE for any + // maskNumber other than 0. dEQP's per-case gluStateReset issues glSampleMaski up to + // GL_MAX_SAMPLE_MASK_WORDS, so advertising a device's real 2 would abort the whole glcts + // process after every single case - the failure da6f75dbd added the GLES clamp to stop, + // reproduced on this backend. One word is the spec minimum and therefore always legal. + // It is also what PipelineCreatePayload::sampleMask is sized for. + caps.MaxSampleMaskWords = std::min(SaturateToInt(p.limits.maxSampleMaskWords), 1); caps.MaxTextureImageUnits = SaturateToInt(p.limits.maxPerStageDescriptorSampledImages); caps.MaxVertexTextureImageUnits = SaturateToInt(p.limits.maxPerStageDescriptorSampledImages); caps.MaxComputeTextureImageUnits = SaturateToInt(p.limits.maxPerStageDescriptorSampledImages); @@ -300,7 +309,15 @@ namespace MobileGL::MG_Util::BackendLoader { caps.MaxFramebufferSamples = ResolveConservativeFramebufferSampleLimit(properties.limits); caps.MaxIntegerSamples = MaxSampleCountFromFlags(properties.limits.sampledImageIntegerSampleCounts); caps.MaxSamples = caps.MaxFramebufferSamples; - caps.MaxSampleMaskWords = SaturateToInt(properties.limits.maxSampleMaskWords); + // Clamped to one word, exactly as the GLES loader clamps the driver's value and for the + // same reason: MobileGL's sample-mask state IS a single 32-bit word + // (RenderState::SampleMaskValue) and SampleMaski_State() raises GL_INVALID_VALUE for any + // maskNumber other than 0. dEQP's per-case gluStateReset issues glSampleMaski up to + // GL_MAX_SAMPLE_MASK_WORDS, so advertising a device's real 2 would abort the whole glcts + // process after every single case - the failure da6f75dbd added the GLES clamp to stop, + // reproduced on this backend. One word is the spec minimum and therefore always legal. + // It is also what PipelineCreatePayload::sampleMask is sized for. + caps.MaxSampleMaskWords = std::min(SaturateToInt(properties.limits.maxSampleMaskWords), 1); caps.MaxTextureImageUnits = SaturateToInt(properties.limits.maxPerStageDescriptorSampledImages); caps.MaxVertexTextureImageUnits = SaturateToInt(properties.limits.maxPerStageDescriptorSampledImages); caps.MaxComputeTextureImageUnits = SaturateToInt(properties.limits.maxPerStageDescriptorSampledImages); diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index bdd6c50d..379adb91 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -758,6 +758,33 @@ namespace MobileGL { return false; } + Bool ShaderCompiler::ModuleDeclaresTransformFeedback(const Vector& spirv) { + if (spirv.empty()) { + return false; + } + std::unique_ptr context = spvtools::BuildModule( + SPV_ENV_VULKAN_1_1, MakeSpirvMessageConsumer("ModuleDeclaresTransformFeedback"), + spirv.data(), spirv.size()); + if (!context) { + // Unparseable is not a capture verdict; say no, which makes the caller decline + // the span rather than issue transform-feedback commands against it. + return false; + } + // The exact question VUID-vkCmdBeginTransformFeedbackEXT-None-04128 asks of the + // bound pipeline's last pre-rasterization stage: was it declared with the Xfb + // execution mode. Reading the execution modes rather than the TransformFeedback + // capability because the capability can legally be declared by a module that has + // no Xfb entry point, and the VUID is about the mode. + for (const spvtools::opt::Instruction& mode : context->module()->execution_modes()) { + if (mode.NumInOperands() >= 2 && + static_cast(mode.GetSingleWordInOperand(1)) == + spv::ExecutionMode::Xfb) { + return true; + } + } + return false; + } + Bool ShaderCompiler::ModuleDeclaresFloat64(const Vector& spirv) { if (spirv.empty()) { // Same reasoning as ModuleDeclaresBufferTextureSampler: a stage that produced diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index 07cdbe7a..2cfe97e3 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -532,6 +532,12 @@ namespace MobileGL { // check exists so that failure can be reported as the missing capability it is, // naming the shader, rather than as a driver info log nobody sees. static Bool ModuleDeclaresBufferTextureSampler(const Vector& spirv); + // Does this module carry the Xfb execution mode - i.e. would a + // vkCmdBeginTransformFeedbackEXT against a pipeline whose last pre-rasterization + // stage is this module satisfy VUID-vkCmdBeginTransformFeedbackEXT-None-04128? + // Asked of the FINAL bytes, so it answers for whatever the backend transform + // chain actually produced rather than for what it was asked to produce. + static Bool ModuleDeclaresTransformFeedback(const Vector& spirv); // True when the module still declares a 64-bit float type. After // SanitizeAndOptimizeBinary that can only mean DemoteFloat64Pass declined the