diff --git a/MobileGL/MG_Backend/BackendObject.h b/MobileGL/MG_Backend/BackendObject.h index 189e075a..96a0ef97 100644 --- a/MobileGL/MG_Backend/BackendObject.h +++ b/MobileGL/MG_Backend/BackendObject.h @@ -301,6 +301,12 @@ namespace MobileGL { struct DynamicBackendParameters { SizeT UniformBufferOffsetAlignment = 256; + // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, which is a SEPARATE limit from the + // uniform one and is routinely larger: Adreno 830 reports 32 for uniform buffers and + // 64 for storage buffers. Answering the storage query with the uniform value let an + // application bind a storage range at an offset the driver cannot address, which it + // accepted without error and then wrote somewhere else entirely. + SizeT ShaderStorageBufferOffsetAlignment = 256; // GL_MAX_TEXTURE_MAX_ANISOTROPY_EXT. 1.0 means the backend cannot filter anisotropically, // which is also why the extension is not advertised in that case. Float MaxTextureMaxAnisotropy = 1.0f; diff --git a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp index 5b567dd9..90e9c6a3 100644 --- a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp @@ -1323,6 +1323,8 @@ namespace MobileGL::MG_Backend::DirectGLES { void BackendObject_DirectGLES::UpdateDynamicBackendParameters() { m_dynamicParameters.UniformBufferOffsetAlignment = m_GLESCapabilities.UniformBufferOffsetAlignment; + m_dynamicParameters.ShaderStorageBufferOffsetAlignment = + m_GLESCapabilities.ShaderStorageBufferOffsetAlignment; m_dynamicParameters.MaxTextureMaxAnisotropy = m_GLESCapabilities.MaxTextureMaxAnisotropy; m_dynamicParameters.AliasedLineWidthRangeMin = m_GLESCapabilities.AliasedLineWidthRangeMin; m_dynamicParameters.AliasedLineWidthRangeMax = m_GLESCapabilities.AliasedLineWidthRangeMax; diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index 5f1a2433..3ba144f7 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -6341,8 +6341,9 @@ namespace MobileGL::MG_Backend::DirectGLES { // neither std430 nor std140") and the stage never reaches the driver. Collapse the // block into one uint array at offset 0 and re-index each counter to the element // that used to be at its byte offset; the buffer then stays bound whole, which it - // has to (GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT is 32 on this device, so an - // 8-byte bind offset is not expressible). BEFORE SetAtomicCounterBlockBindings + // has to (GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT is 64 on Adreno 830 and 32 or + // more everywhere else, so an 8-byte bind offset is not expressible on any of them). + // BEFORE SetAtomicCounterBlockBindings // below, which only moves the block's BINDING and needs the block intact. // // NO KEY MATERIAL either, for the same reason - and note where the application's diff --git a/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp b/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp index 554e153e..14a0de4e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp @@ -855,6 +855,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { static constexpr SizeT kMaxAdvertisedShaderStorageBlockSize = 512ull * 1024ull * 1024ull; m_dynamicParameters.UniformBufferOffsetAlignment = m_vulkanCaps.UniformBufferOffsetAlignment; + m_dynamicParameters.ShaderStorageBufferOffsetAlignment = m_vulkanCaps.ShaderStorageBufferOffsetAlignment; m_dynamicParameters.AliasedLineWidthRangeMin = m_vulkanCaps.AliasedLineWidthRangeMin; m_dynamicParameters.AliasedLineWidthRangeMax = m_vulkanCaps.AliasedLineWidthRangeMax; // Without the samplerAnisotropy feature the limit is unusable, so report 1.0 (no anisotropy) diff --git a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp index 281c371c..a7f0c1b3 100644 --- a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp +++ b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp @@ -2406,7 +2406,12 @@ namespace MobileGL::MG_Impl::GLImpl { *params = static_cast(dynamicParameters.PointSizeGranularity); break; case GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT: - *params = static_cast(dynamicParameters.UniformBufferOffsetAlignment); + // The STORAGE alignment, which is its own limit - this used to answer with the + // uniform one. They differ on real hardware (Adreno 830: 32 uniform, 64 storage), and + // under-reporting it is silent: ValidateBindBufferRange accepts the offset, the ES + // driver accepts it too without raising an error, and the shader's writes then land + // at an address the application never bound. + *params = static_cast(dynamicParameters.ShaderStorageBufferOffsetAlignment); break; case GL_SMOOTH_LINE_WIDTH_RANGE: params[0] = static_cast(dynamicParameters.SmoothLineWidthRangeMin); diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index 65edd04f..74ed4326 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -988,6 +988,49 @@ TEST(GetterSanity, ReportsFragmentInterpolationLimitsForFloatAndIntegerQueries) MG_State::pGLContext = Move(previousContext); } +// GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT used to be answered with the UNIFORM buffer +// alignment. The two are separate limits and the storage one is the larger on real hardware +// (Adreno 830: 32 uniform, 64 storage), so the substitution under-reported it - and an +// under-reported alignment is silent all the way down: the frontend validator accepts the +// offset, the ES driver accepts the glBindBufferRange too without raising an error, and the +// shader's stores land at an address the application never bound. The two values are +// deliberately different here so a query that reads the wrong field cannot coincide with the +// right answer. +TEST(GetterSanity, StorageAndUniformBufferOffsetAlignmentsAreSeparateLimits) { + using namespace MobileGL; + + auto previousContext = Move(MG_State::pGLContext); + auto previousBackend = Move(MG_Backend::pActiveBackendObject); + MG_State::pGLContext = MakeUnique(); + + MG_Backend::DynamicBackendParameters params; + params.UniformBufferOffsetAlignment = 32; + params.ShaderStorageBufferOffsetAlignment = 64; + MG_Backend::pActiveBackendObject = MakeUnique(params); + + GLint uniformAlignment = 0; + MG_Impl::GLImpl::GetIntegerv(GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT, &uniformAlignment); + EXPECT_EQ(uniformAlignment, 32); + + GLint storageAlignment = 0; + MG_Impl::GLImpl::GetIntegerv(GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, &storageAlignment); + EXPECT_EQ(storageAlignment, 64); + + // And the other way round, so the test fails on a getter that simply swapped the two fields. + params.UniformBufferOffsetAlignment = 128; + params.ShaderStorageBufferOffsetAlignment = 16; + MG_Backend::pActiveBackendObject = MakeUnique(params); + + MG_Impl::GLImpl::GetIntegerv(GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT, &uniformAlignment); + EXPECT_EQ(uniformAlignment, 128); + MG_Impl::GLImpl::GetIntegerv(GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, &storageAlignment); + EXPECT_EQ(storageAlignment, 16); + EXPECT_EQ(MG_Impl::GLImpl::GetError(), GL_NO_ERROR); + + MG_Backend::pActiveBackendObject = Move(previousBackend); + MG_State::pGLContext = Move(previousContext); +} + TEST(GetterSanity, PerStageImageUniformQueriesMatchShaderCompilerLimits) { using namespace MobileGL; diff --git a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp index 5ae9ba31..5ac65a0f 100644 --- a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp +++ b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.cpp @@ -1064,6 +1064,16 @@ namespace MobileGL::MG_Util::BackendLoader { MGLOG_I("OpenGL ES capabilities:"); glesFuncs.glGetIntegerv(GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT, &caps.UniformBufferOffsetAlignment); MGLOG_I(" GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT: %d", caps.UniformBufferOffsetAlignment); + // ES 3.1 core, so no extension gate - but a driver that somehow leaves it at zero would + // make every storage-range offset legal, so an unusable answer keeps the 256 default. + GLint shaderStorageOffsetAlignment = 0; + glesFuncs.glGetIntegerv(GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, &shaderStorageOffsetAlignment); + while (glesFuncs.glGetError() != GL_NO_ERROR) { + } + if (shaderStorageOffsetAlignment > 0) { + caps.ShaderStorageBufferOffsetAlignment = shaderStorageOffsetAlignment; + } + MGLOG_I(" GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT: %d", caps.ShaderStorageBufferOffsetAlignment); GLfloat aliasedLineWidthRange[2] = {1.0f, 1.0f}; GLfloat smoothLineWidthRange[2] = {1.0f, 1.0f}; GLfloat smoothLineWidthGranularity = 1.0f; diff --git a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h index b3eec874..d5e9d9e3 100644 --- a/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h +++ b/MobileGL/MG_Util/BackendLoaders/OpenGL/Loader.h @@ -1231,6 +1231,10 @@ namespace MobileGL { // InstanceIndex, which includes firstInstance. Bool IndirectDrawInstanceIdIncludesBaseInstance = false; Int UniformBufferOffsetAlignment = 256; + // Its storage-buffer counterpart, queried separately because it is a separate limit: + // Adreno 830 answers 32 for GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT and 64 for + // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT. + Int ShaderStorageBufferOffsetAlignment = 256; Float AliasedLineWidthRangeMin = 1.0f; Float AliasedLineWidthRangeMax = 1.0f; Float SmoothLineWidthRangeMin = 1.0f; diff --git a/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp b/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp index 44511b19..01b6e1e6 100644 --- a/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp +++ b/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.cpp @@ -154,6 +154,7 @@ namespace MobileGL::MG_Util::BackendLoader { caps.DriverVersionString = DecodeDriverVersion(p.driverVersion); caps.VendorId = p.vendorID; caps.UniformBufferOffsetAlignment = static_cast(p.limits.minUniformBufferOffsetAlignment); + caps.ShaderStorageBufferOffsetAlignment = static_cast(p.limits.minStorageBufferOffsetAlignment); caps.AliasedLineWidthRangeMin = p.limits.lineWidthRange[0]; caps.AliasedLineWidthRangeMax = p.limits.lineWidthRange[1]; caps.MaxSamplerAnisotropy = p.limits.maxSamplerAnisotropy; @@ -272,6 +273,8 @@ namespace MobileGL::MG_Util::BackendLoader { caps.DriverVersionString = DecodeDriverVersion(properties.driverVersion); caps.VendorId = properties.vendorID; caps.UniformBufferOffsetAlignment = static_cast(properties.limits.minUniformBufferOffsetAlignment); + caps.ShaderStorageBufferOffsetAlignment = + static_cast(properties.limits.minStorageBufferOffsetAlignment); caps.AliasedLineWidthRangeMin = properties.limits.lineWidthRange[0]; caps.AliasedLineWidthRangeMax = properties.limits.lineWidthRange[1]; caps.MaxSamplerAnisotropy = properties.limits.maxSamplerAnisotropy; diff --git a/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.h b/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.h index a4d60fdd..135070ce 100644 --- a/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.h +++ b/MobileGL/MG_Util/BackendLoaders/Vulkan/Loader.h @@ -18,6 +18,10 @@ namespace MobileGL { // VkPhysicalDeviceProperties::vendorID, for device-quirk vendor gating. Uint32 VendorId = 0; Int UniformBufferOffsetAlignment = 256; + // VkPhysicalDeviceLimits::minStorageBufferOffsetAlignment. A separate limit from + // the uniform one on Vulkan too, and the one GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT + // has to answer with. + Int ShaderStorageBufferOffsetAlignment = 256; Float AliasedLineWidthRangeMin = 1.0f; Float AliasedLineWidthRangeMax = 1.0f; // VkPhysicalDeviceLimits::maxSamplerAnisotropy. Whether it can be used at all depends on diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.h index effb3782..26133e0a 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/FlattenAtomicCounterBlockPass.h @@ -44,9 +44,10 @@ namespace MobileGL { // // Why not simply rebase the offsets to zero and bind the buffer 8 bytes in: because // glBindBufferRange's offset must be a multiple of - // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, which the target device reports as 32. - // A byte offset of 8 cannot be expressed as a binding at all, so the correction has - // to live in the shader's indexing, where it costs nothing. + // GL_SHADER_STORAGE_BUFFER_OFFSET_ALIGNMENT, which is 64 on Adreno 830 and no smaller + // than 32 on the other targets. A byte offset of 8 cannot be expressed as a binding + // on any of them, so the correction has to live in the shader's indexing, where it + // costs nothing. // // A block that is ALREADY laid out naturally - which is every shader that omits the // offset qualifier, and so very nearly all of them - is left byte-identical: the