From 899126f03697536b4d53c52c2d8a4eb4bbddf647 Mon Sep 17 00:00:00 2001 From: Zhaoming Jiang Date: Fri, 20 Jan 2023 08:46:55 +0000 Subject: [PATCH] Dawn: Fix FeatureState of some features This CL fix FeatureState for some experimental features to prepare for refactoring adapter creation with adapter toggles. This CL also fix the related unittests. Bug: dawn:1495 Change-Id: Ibf043ed74c0bfc79c64986f2f96135d92adf3930 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/116841 Reviewed-by: Austin Eng Kokoro: Kokoro Auto-Submit: Zhaoming Jiang Commit-Queue: Zhaoming Jiang --- src/dawn/native/Features.cpp | 6 +- src/dawn/tests/unittests/FeatureTests.cpp | 107 +++++++++++++----- .../validation/QueryValidationTests.cpp | 8 ++ .../validation/UnsafeAPIValidationTests.cpp | 59 ---------- 4 files changed, 93 insertions(+), 87 deletions(-) diff --git a/src/dawn/native/Features.cpp b/src/dawn/native/Features.cpp index d839bd24e9..a94c6b427e 100644 --- a/src/dawn/native/Features.cpp +++ b/src/dawn/native/Features.cpp @@ -47,10 +47,12 @@ static constexpr FeatureEnumAndInfoList kFeatureNameAndInfoList = {{ "https://bugs.chromium.org/p/dawn/issues/detail?id=955", FeatureInfo::FeatureState::Stable}}, {Feature::PipelineStatisticsQuery, {"pipeline-statistics-query", "Support Pipeline Statistics Query", - "https://bugs.chromium.org/p/dawn/issues/detail?id=434", FeatureInfo::FeatureState::Stable}}, + "https://bugs.chromium.org/p/dawn/issues/detail?id=434", + FeatureInfo::FeatureState::Experimental}}, {Feature::TimestampQuery, {"timestamp-query", "Support Timestamp Query", - "https://bugs.chromium.org/p/dawn/issues/detail?id=434", FeatureInfo::FeatureState::Stable}}, + "https://bugs.chromium.org/p/dawn/issues/detail?id=434", + FeatureInfo::FeatureState::Experimental}}, {Feature::TimestampQueryInsidePasses, {"timestamp-query-inside-passes", "Support Timestamp Query inside render/compute pass", "https://bugs.chromium.org/p/dawn/issues/detail?id=434", diff --git a/src/dawn/tests/unittests/FeatureTests.cpp b/src/dawn/tests/unittests/FeatureTests.cpp index b4c7fced6c..7123e8528c 100644 --- a/src/dawn/tests/unittests/FeatureTests.cpp +++ b/src/dawn/tests/unittests/FeatureTests.cpp @@ -55,43 +55,98 @@ TEST_F(FeatureTests, AdapterWithRequiredFeatureDisabled) { mAdapterBase.SetSupportedFeatures(featureNamesWithoutOne); dawn::native::Adapter adapterWithoutFeature(&mAdapterBase); - wgpu::DeviceDescriptor deviceDescriptor; - wgpu::FeatureName featureName = FeatureEnumToAPIFeature(notSupportedFeature); - deviceDescriptor.requiredFeatures = &featureName; - deviceDescriptor.requiredFeaturesCount = 1; + // Test that creating device with and without DisallowUnsafeApis toggle disabled will both + // failed. - WGPUDevice deviceWithFeature = adapterWithoutFeature.CreateDevice( - reinterpret_cast(&deviceDescriptor)); - ASSERT_EQ(nullptr, deviceWithFeature); + // Without disabling DisallowUnsafeApis toggle + { + wgpu::DeviceDescriptor deviceDescriptor; + wgpu::FeatureName featureName = FeatureEnumToAPIFeature(notSupportedFeature); + deviceDescriptor.requiredFeatures = &featureName; + deviceDescriptor.requiredFeaturesCount = 1; + + WGPUDevice deviceWithFeature = adapterWithoutFeature.CreateDevice( + reinterpret_cast(&deviceDescriptor)); + ASSERT_EQ(nullptr, deviceWithFeature); + } + + // Disabling DisallowUnsafeApis toggle + { + wgpu::DeviceDescriptor unsafeDeviceDescriptor; + wgpu::FeatureName featureName = FeatureEnumToAPIFeature(notSupportedFeature); + unsafeDeviceDescriptor.requiredFeatures = &featureName; + unsafeDeviceDescriptor.requiredFeaturesCount = 1; + + wgpu::DawnTogglesDeviceDescriptor togglesDesc; + unsafeDeviceDescriptor.nextInChain = &togglesDesc; + const char* toggle = "disallow_unsafe_apis"; + togglesDesc.forceDisabledToggles = &toggle; + togglesDesc.forceDisabledTogglesCount = 1; + + WGPUDevice deviceWithFeature = adapterWithoutFeature.CreateDevice( + reinterpret_cast(&unsafeDeviceDescriptor)); + ASSERT_EQ(nullptr, deviceWithFeature); + } } } -// Test Device.GetEnabledFeatures() can return the names of the enabled features correctly. -TEST_F(FeatureTests, GetEnabledFeatures) { +// Test creating device requiring a supported feature can succeed (with DisallowUnsafeApis guarded +// for experimental features), and Device.GetEnabledFeatures() can return the names of the enabled +// features correctly. +TEST_F(FeatureTests, RequireAndGetEnabledFeatures) { dawn::native::Adapter adapter(&mAdapterBase); + dawn::native::FeaturesInfo featuresInfo; + for (size_t i = 0; i < kTotalFeaturesCount; ++i) { dawn::native::Feature feature = static_cast(i); wgpu::FeatureName featureName = FeatureEnumToAPIFeature(feature); - wgpu::DeviceDescriptor deviceDescriptor; - deviceDescriptor.requiredFeatures = &featureName; - deviceDescriptor.requiredFeaturesCount = 1; + // Test with DisallowUnsafeApis not disabled. + { + wgpu::DeviceDescriptor deviceDescriptor; + deviceDescriptor.requiredFeatures = &featureName; + deviceDescriptor.requiredFeaturesCount = 1; - // Some features may require DisallowUnsafeApis toggle disabled, otherwise CreateDevice may - // failed. - const char* const disableToggles[] = {"disallow_unsafe_apis"}; - wgpu::DawnTogglesDeviceDescriptor toggleDesc; - toggleDesc.forceDisabledToggles = disableToggles; - toggleDesc.forceDisabledTogglesCount = 1; - deviceDescriptor.nextInChain = &toggleDesc; + dawn::native::DeviceBase* deviceBase = dawn::native::FromAPI(adapter.CreateDevice( + reinterpret_cast(&deviceDescriptor))); - dawn::native::DeviceBase* deviceBase = dawn::native::FromAPI( - adapter.CreateDevice(reinterpret_cast(&deviceDescriptor))); + // Device creation should fail if requiring experimental features without disabling + // DisallowUnsafeApis + if (featuresInfo.GetFeatureInfo(featureName)->featureState == + dawn::native::FeatureInfo::FeatureState::Experimental) { + ASSERT_EQ(nullptr, deviceBase); + } else { + // Requiring stable features should succeed. + ASSERT_NE(nullptr, deviceBase); + ASSERT_EQ(1u, deviceBase->APIEnumerateFeatures(nullptr)); + wgpu::FeatureName enabledFeature; + deviceBase->APIEnumerateFeatures(&enabledFeature); + EXPECT_EQ(enabledFeature, featureName); + deviceBase->APIRelease(); + } + } - ASSERT_EQ(1u, deviceBase->APIEnumerateFeatures(nullptr)); - wgpu::FeatureName enabledFeature; - deviceBase->APIEnumerateFeatures(&enabledFeature); - EXPECT_EQ(enabledFeature, featureName); - deviceBase->APIRelease(); + // Test with DisallowUnsafeApis disabled, creating device should always succeed. + { + wgpu::DeviceDescriptor unsafeDeviceDescriptor; + unsafeDeviceDescriptor.requiredFeatures = &featureName; + unsafeDeviceDescriptor.requiredFeaturesCount = 1; + + const char* const disableToggles[] = {"disallow_unsafe_apis"}; + wgpu::DawnTogglesDeviceDescriptor toggleDesc; + toggleDesc.forceDisabledToggles = disableToggles; + toggleDesc.forceDisabledTogglesCount = 1; + unsafeDeviceDescriptor.nextInChain = &toggleDesc; + + dawn::native::DeviceBase* deviceBase = dawn::native::FromAPI(adapter.CreateDevice( + reinterpret_cast(&unsafeDeviceDescriptor))); + + ASSERT_NE(nullptr, deviceBase); + ASSERT_EQ(1u, deviceBase->APIEnumerateFeatures(nullptr)); + wgpu::FeatureName enabledFeature; + deviceBase->APIEnumerateFeatures(&enabledFeature); + EXPECT_EQ(enabledFeature, featureName); + deviceBase->APIRelease(); + } } } diff --git a/src/dawn/tests/unittests/validation/QueryValidationTests.cpp b/src/dawn/tests/unittests/validation/QueryValidationTests.cpp index d0289e201e..2360ec30bf 100644 --- a/src/dawn/tests/unittests/validation/QueryValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/QueryValidationTests.cpp @@ -43,6 +43,14 @@ TEST_F(QuerySetValidationTest, CreationWithoutFeatures) { CreateQuerySet(device, wgpu::QueryType::Occlusion, 1); // Creating a query set for other types of queries fails without features enabled. + ASSERT_DEVICE_ERROR(CreateQuerySet(device, wgpu::QueryType::PipelineStatistics, 1, + {wgpu::PipelineStatisticName::VertexShaderInvocations})); + ASSERT_DEVICE_ERROR(CreateQuerySet(device, wgpu::QueryType::PipelineStatistics, 1, + {wgpu::PipelineStatisticName::ClipperPrimitivesOut})); + ASSERT_DEVICE_ERROR(CreateQuerySet(device, wgpu::QueryType::PipelineStatistics, 1, + {wgpu::PipelineStatisticName::ComputeShaderInvocations})); + ASSERT_DEVICE_ERROR(CreateQuerySet(device, wgpu::QueryType::PipelineStatistics, 1, + {wgpu::PipelineStatisticName::FragmentShaderInvocations})); ASSERT_DEVICE_ERROR(CreateQuerySet(device, wgpu::QueryType::PipelineStatistics, 1, {wgpu::PipelineStatisticName::VertexShaderInvocations})); ASSERT_DEVICE_ERROR(CreateQuerySet(device, wgpu::QueryType::Timestamp, 1)); diff --git a/src/dawn/tests/unittests/validation/UnsafeAPIValidationTests.cpp b/src/dawn/tests/unittests/validation/UnsafeAPIValidationTests.cpp index 0cfa397c92..2194cd1aad 100644 --- a/src/dawn/tests/unittests/validation/UnsafeAPIValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/UnsafeAPIValidationTests.cpp @@ -37,65 +37,6 @@ class UnsafeAPIValidationTest : public ValidationTest { } }; -class UnsafeQueryAPIValidationTest : public ValidationTest { - protected: - WGPUDevice CreateTestDevice(dawn::native::Adapter dawnAdapter) override { - wgpu::DeviceDescriptor descriptor; - wgpu::FeatureName requiredFeatures[2] = {wgpu::FeatureName::PipelineStatisticsQuery, - wgpu::FeatureName::TimestampQuery}; - descriptor.requiredFeatures = requiredFeatures; - descriptor.requiredFeaturesCount = 2; - - wgpu::DawnTogglesDeviceDescriptor togglesDesc; - descriptor.nextInChain = &togglesDesc; - const char* toggle = "disallow_unsafe_apis"; - togglesDesc.forceEnabledToggles = &toggle; - togglesDesc.forceEnabledTogglesCount = 1; - - return dawnAdapter.CreateDevice(&descriptor); - } -}; - -// Check that pipeline statistics query are disallowed. -TEST_F(UnsafeQueryAPIValidationTest, PipelineStatisticsDisallowed) { - wgpu::QuerySetDescriptor descriptor; - descriptor.count = 1; - - // Control case: occlusion query creation is allowed. - { - descriptor.type = wgpu::QueryType::Occlusion; - device.CreateQuerySet(&descriptor); - } - - // Error case: pipeline statistics query creation is disallowed. - { - descriptor.type = wgpu::QueryType::PipelineStatistics; - std::vector pipelineStatistics = { - wgpu::PipelineStatisticName::VertexShaderInvocations}; - descriptor.pipelineStatistics = pipelineStatistics.data(); - descriptor.pipelineStatisticsCount = pipelineStatistics.size(); - ASSERT_DEVICE_ERROR(device.CreateQuerySet(&descriptor)); - } -} - -// Check timestamp queries are disallowed. -TEST_F(UnsafeQueryAPIValidationTest, TimestampQueryDisallowed) { - wgpu::QuerySetDescriptor descriptor; - descriptor.count = 1; - - // Control case: occlusion query creation is allowed. - { - descriptor.type = wgpu::QueryType::Occlusion; - device.CreateQuerySet(&descriptor); - } - - // Error case: timestamp query creation is disallowed. - { - descriptor.type = wgpu::QueryType::Timestamp; - ASSERT_DEVICE_ERROR(device.CreateQuerySet(&descriptor)); - } -} - // Check chromium_disable_uniformity_analysis is an unsafe API. TEST_F(UnsafeAPIValidationTest, chromium_disable_uniformity_analysis) { ASSERT_DEVICE_ERROR(utils::CreateShaderModule(device, R"(