From d474c141045048f7c2bfcab81539dd6681392df0 Mon Sep 17 00:00:00 2001 From: Ricardo Garcia Date: Tue, 4 May 2021 10:09:55 +0200 Subject: [PATCH] Fixes for robust buffer with variable pointer tests This commit fixes several issues with robust buffer access with variable pointers tests: * The variable pointer tests were checking support for the variablePointersStorageBuffer feature but they weren't actually activating the feature when creating the robust buffer access device. * Support checks were being done at instance creation time instead of using the proper checkSupport method. * In-bound checks for 64-bit format accesses were only checking 4 out of every 8 bytes in the buffer. * The verification routines were not taking into account 64-bit nonatomic accesses could be legally split into two 32-bit accesses that could be checked independently. * The possibility of satisfying a (0,0,0,x) pattern in output data was not being checked correctly for 64-bit formats, always using 32-bit vectors and values. Affected tests: dEQP-VK.robustness.buffer_access.* dEQP-VK.robustness.vertex_access.* Components: Vulkan VK-GL-CTS issue: 2908 Change-Id: I85aeb085cb31d63b70acec08551c53fcc7d2be3c --- ...tBufferAccessWithVariablePointersTests.cpp | 93 +++++++++---------- .../vulkan/robustness/vktRobustnessUtil.cpp | 31 ++++--- 2 files changed, 64 insertions(+), 60 deletions(-) diff --git a/external/vulkancts/modules/vulkan/robustness/vktRobustBufferAccessWithVariablePointersTests.cpp b/external/vulkancts/modules/vulkan/robustness/vktRobustBufferAccessWithVariablePointersTests.cpp index 52ff86d2c..291ba37be 100644 --- a/external/vulkancts/modules/vulkan/robustness/vktRobustBufferAccessWithVariablePointersTests.cpp +++ b/external/vulkancts/modules/vulkan/robustness/vktRobustBufferAccessWithVariablePointersTests.cpp @@ -63,29 +63,17 @@ using namespace vk; namespace { -// A function for getting information on variable pointer features supported through physical device -vk::VkPhysicalDeviceVariablePointersFeatures querySupportedVariablePointersFeatures (const Context& context) +// Creates a custom device with robust buffer access and variable pointer features. +Move createRobustBufferAccessVariablePointersDevice (Context& context) { - VkPhysicalDeviceVariablePointersFeatures extensionFeatures = - { - VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_VARIABLE_POINTER_FEATURES_KHR, // sType - DE_NULL, // pNext - false, // variablePointersStorageBuffer - false, // variablePointers - }; + auto pointerFeatures = context.getVariablePointersFeatures(); - VkPhysicalDeviceFeatures2 features; - deMemset(&features, 0, sizeof(features)); - features.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2; - features.pNext = &extensionFeatures; - - // Call the getter only if supported. Otherwise above "zero" defaults are used - if (context.isInstanceFunctionalitySupported("VK_KHR_get_physical_device_properties2")) - { - context.getInstanceInterface().getPhysicalDeviceFeatures2(context.getPhysicalDevice(), &features); - } + VkPhysicalDeviceFeatures2 features2 = initVulkanStructure(); + features2.features = context.getDeviceFeatures(); + features2.features.robustBufferAccess = VK_TRUE; + features2.pNext = &pointerFeatures; - return extensionFeatures; + return createRobustBufferAccessDevice(context, &features2); } // A supplementary structures that can hold information about buffer size @@ -201,6 +189,8 @@ public: { } + void checkSupport (Context &context) const override; + protected: const VkShaderStageFlags m_shaderStage; const ShaderType m_shaderType; @@ -224,6 +214,13 @@ RobustAccessWithPointersTest::RobustAccessWithPointersTest(tcu::TestContext& te DE_ASSERT(m_shaderStage == VK_SHADER_STAGE_VERTEX_BIT || m_shaderStage == VK_SHADER_STAGE_FRAGMENT_BIT || m_shaderStage == VK_SHADER_STAGE_COMPUTE_BIT); } +void RobustAccessWithPointersTest::checkSupport (Context &context) const +{ + const auto& pointerFeatures = context.getVariablePointersFeatures(); + if (!pointerFeatures.variablePointersStorageBuffer) + TCU_THROW(NotSupportedError, "VariablePointersStorageBuffer SPIR-V capability not supported"); +} + // A subclass for testing reading with variable pointers class RobustReadTest : public RobustAccessWithPointersTest { @@ -309,7 +306,7 @@ public: virtual tcu::TestStatus iterate (void); - virtual bool verifyResult (void); + virtual bool verifyResult (bool splitAccess = false); private: bool isExpectedValueFromInBuffer (VkDeviceSize offsetInBytes, @@ -1286,13 +1283,7 @@ RobustReadTest::RobustReadTest (tcu::TestContext& testContext, TestInstance* RobustReadTest::createInstance (Context& context) const { - VkPhysicalDeviceVariablePointersFeatures pointerFeatures = querySupportedVariablePointersFeatures(context); - - if (pointerFeatures.variablePointersStorageBuffer != DE_TRUE) - return new NotSupportedInstance(context, std::string("VariablePointersStorageBuffer support is required for this test.")); - - // We need a device with enabled robust buffer access feature (it is disabled in default device) - Move device = createRobustBufferAccessDevice(context); + auto device = createRobustBufferAccessVariablePointersDevice(context); return new ReadInstance(context, device, m_shaderType, m_shaderStage, m_bufferFormat, m_readAccessRange, m_accessOutOfBackingMemory); } @@ -1326,12 +1317,7 @@ RobustWriteTest::RobustWriteTest (tcu::TestContext& testContext, TestInstance* RobustWriteTest::createInstance (Context& context) const { - VkPhysicalDeviceVariablePointersFeatures pointerFeatures = querySupportedVariablePointersFeatures(context); - if (pointerFeatures.variablePointersStorageBuffer != DE_TRUE) - return new NotSupportedInstance(context, std::string("VariablePointersStorageBuffer support is required for this test.")); - - // We need a device with enabled robust buffer access feature (it is disabled in default device) - Move device = createRobustBufferAccessDevice(context); + auto device = createRobustBufferAccessVariablePointersDevice(context); return new WriteInstance(context, device, m_shaderType, m_shaderStage, m_bufferFormat, m_writeAccessRange, m_accessOutOfBackingMemory); } @@ -1519,25 +1505,29 @@ AccessInstance::AccessInstance (Context& context, // Verifies if the buffer has the value initialized by BufferAccessInstance::populateReadBuffer at a given offset. bool AccessInstance::isExpectedValueFromInBuffer (VkDeviceSize offsetInBytes, - const void* valuePtr, + const void* valuePtr, VkDeviceSize valueSize) { DE_ASSERT(offsetInBytes % 4 == 0); DE_ASSERT(offsetInBytes < m_inBufferAccess.allocSize); + DE_ASSERT(valueSize == 4ull || valueSize == 8ull); const deUint32 valueIndex = deUint32(offsetInBytes / 4) + 2; if (isUintFormat(m_bufferFormat)) { - return !deMemCmp(valuePtr, &valueIndex, (size_t)valueSize); + const deUint32 expectedValues[2] = { valueIndex, valueIndex + 1u }; + return !deMemCmp(valuePtr, &expectedValues, (size_t)valueSize); } else if (isIntFormat(m_bufferFormat)) { - const deInt32 value = -deInt32(valueIndex); - return !deMemCmp(valuePtr, &value, (size_t)valueSize); + const deInt32 value = -deInt32(valueIndex); + const deInt32 expectedValues[2] = { value, value - 1 }; + return !deMemCmp(valuePtr, &expectedValues, (size_t)valueSize); } else if (isFloatFormat(m_bufferFormat)) { + DE_ASSERT(valueSize == 4ull); const float value = float(valueIndex); return !deMemCmp(valuePtr, &value, (size_t)valueSize); } @@ -1602,7 +1592,7 @@ tcu::TestStatus AccessInstance::iterate (void) return tcu::TestStatus::fail("Invalid value(s) found"); } -bool AccessInstance::verifyResult (void) +bool AccessInstance::verifyResult (bool splitAccess) { std::ostringstream logMsg; tcu::TestLog& log = m_context.getTestContext().getLog(); @@ -1613,7 +1603,8 @@ bool AccessInstance::verifyResult (void) deUint32 valueNdx = 0; const VkDeviceSize maxAccessRange = isReadAccess ? m_inBufferAccess.maxAccessRange : m_outBufferAccess.maxAccessRange; const bool isR64 = (m_bufferFormat == VK_FORMAT_R64_UINT || m_bufferFormat == VK_FORMAT_R64_SINT); - const deUint32 elementSize = isR64 ? 8 : 4; + const deUint32 unsplitElementSize = (isR64 ? 8u : 4u); + const deUint32 elementSize = ((isR64 && !splitAccess) ? 8u : 4u); for (VkDeviceSize offsetInBytes = 0; offsetInBytes < m_outBufferAccess.allocSize; offsetInBytes += elementSize) { @@ -1652,15 +1643,15 @@ bool AccessInstance::verifyResult (void) switch (m_shaderType) { case SHADER_TYPE_SCALAR_COPY: - operandSize = elementSize; // Size of scalar + operandSize = unsplitElementSize; // Size of scalar break; case SHADER_TYPE_VECTOR_COPY: - operandSize = elementSize * 4; // Size of vec4 + operandSize = unsplitElementSize * 4; // Size of vec4 break; case SHADER_TYPE_MATRIX_COPY: - operandSize = elementSize * 16; // Size of mat4 + operandSize = unsplitElementSize * 16; // Size of mat4 break; default: @@ -1724,7 +1715,7 @@ bool AccessInstance::verifyResult (void) } } - if (!isValidValue) + if (!isValidValue && !splitAccess) { // Check if we are satisfying the [0, 0, 0, x] pattern, where x may be either 0 or 1, // or the maximum representable positive integer value (if the format is integer-based). @@ -1732,12 +1723,12 @@ bool AccessInstance::verifyResult (void) const bool canMatchVec4Pattern = (isReadAccess && !isValuePartiallyOutOfBounds && (m_shaderType == SHADER_TYPE_VECTOR_COPY) - && (offsetInBytes / 4 + 1) % 4 == 0); + && (offsetInBytes / elementSize + 1) % 4 == 0); bool matchesVec4Pattern = false; if (canMatchVec4Pattern) { - matchesVec4Pattern = verifyOutOfBoundsVec4(static_cast(static_cast(outValuePtr)) - 3, m_bufferFormat); + matchesVec4Pattern = verifyOutOfBoundsVec4(outValuePtr - 3u * elementSize, m_bufferFormat); } if (!canMatchVec4Pattern || !matchesVec4Pattern) @@ -1764,7 +1755,7 @@ bool AccessInstance::verifyResult (void) { if (isReadAccess) { - if (!isExpectedValueFromInBuffer(offsetInBytes, outValuePtr, 4)) + if (!isExpectedValueFromInBuffer(offsetInBytes, outValuePtr, elementSize)) { logMsg << ", Failed: unexpected value"; allOk = false; @@ -1773,7 +1764,7 @@ bool AccessInstance::verifyResult (void) else { // Out of bounds writes may change values within the bounds. - if (!isValueWithinBufferOrZero(inDataPtr, m_inBufferAccess.accessRange, outValuePtr, 4)) + if (!isValueWithinBufferOrZero(inDataPtr, m_inBufferAccess.accessRange, outValuePtr, elementSize)) { logMsg << ", Failed: unexpected value"; allOk = false; @@ -1785,6 +1776,12 @@ bool AccessInstance::verifyResult (void) log << tcu::TestLog::Message << logMsg.str() << tcu::TestLog::EndMessage; + if (!allOk && unsplitElementSize > 4u && !splitAccess) + { + // "Non-atomic accesses to storage buffers that are a multiple of 32 bits may be decomposed into 32-bit accesses that are individually bounds-checked." + return verifyResult(true/*splitAccess*/); + } + return allOk; } diff --git a/external/vulkancts/modules/vulkan/robustness/vktRobustnessUtil.cpp b/external/vulkancts/modules/vulkan/robustness/vktRobustnessUtil.cpp index 092324723..40840a4d7 100644 --- a/external/vulkancts/modules/vulkan/robustness/vktRobustnessUtil.cpp +++ b/external/vulkancts/modules/vulkan/robustness/vktRobustnessUtil.cpp @@ -151,25 +151,32 @@ bool isValueWithinBufferOrZero (const void* buffer, VkDeviceSize bufferSize, con return isValueWithinBuffer(buffer, bufferSize, valuePtr, valueSizeInBytes) || isValueZero(valuePtr, valueSizeInBytes); } +template +bool verifyVec4IntegerValues (const void* vecPtr) +{ + const T Tzero = T{0}; + const T Tone = T{1}; + const T Tmax = std::numeric_limits::max(); + + T values[4]; + deMemcpy(values, vecPtr, 4*sizeof(T)); + return (values[0] == Tzero && values[1] == Tzero && values[2] == Tzero && + (values[3] == Tzero || values[3] == Tone || values[3] == Tmax)); +} + bool verifyOutOfBoundsVec4 (const void* vecPtr, VkFormat bufferFormat) { if (isUintFormat(bufferFormat)) { - const deUint32* data = (deUint32*)vecPtr; - - return data[0] == 0u - && data[1] == 0u - && data[2] == 0u - && (data[3] == 0u || data[3] == 1u || data[3] == std::numeric_limits::max()); + if (bufferFormat == VK_FORMAT_R64_UINT) + return verifyVec4IntegerValues(vecPtr); + return verifyVec4IntegerValues(vecPtr); } else if (isIntFormat(bufferFormat)) { - const deInt32* data = (deInt32*)vecPtr; - - return data[0] == 0 - && data[1] == 0 - && data[2] == 0 - && (data[3] == 0 || data[3] == 1 || data[3] == std::numeric_limits::max()); + if (bufferFormat == VK_FORMAT_R64_SINT) + return verifyVec4IntegerValues(vecPtr); + return verifyVec4IntegerValues(vecPtr); } else if (isFloatFormat(bufferFormat)) { -- 2.34.1