Fix timestamp period for timestamp-query-inside-passes on Metal

Still a missing condition to enable timestamp period calculation at
device initialization on Metal.

Refactor GPUTimestampCalibrationTests.cpp to check timestamp query
correctness on both D3D12 and Metal backends.

Bug: dawn:1193
Change-Id: I69feeaea0df309e15c008647d76b11899dcdc727
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/119320
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Hao Li <hao.x.li@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>

src/dawn/native/metal/DeviceMTL.h

@@ -160,6 +160,7 @@ class Device final : public DeviceBase {
     MTLTimestamp mGpuTimestamp API_AVAILABLE(macos(10.15), ios(14.0)) = 0;
     // The parameters for kalman filter
     std::unique_ptr<KalmanInfo> mKalmanInfo;
+    bool mIsTimestampQueryEnabled = false;
 
     // Support counter sampling between blit commands, dispatches and draw calls
     bool mCounterSamplingAtCommandBoundary;

src/dawn/native/metal/DeviceMTL.mm

@@ -131,6 +131,9 @@ Device::Device(AdapterBase* adapter,
         mCounterSamplingAtCommandBoundary = true;
         mCounterSamplingAtStageBoundary = false;
     }
+
+    mIsTimestampQueryEnabled =
+        HasFeature(Feature::TimestampQuery) || HasFeature(Feature::TimestampQueryInsidePasses);
 }
 
 Device::~Device() {
@@ -149,8 +152,7 @@ MaybeError Device::Initialize(const DeviceDescriptor* descriptor) {
 
     DAWN_TRY(mCommandContext.PrepareNextCommandBuffer(*mCommandQueue));
 
-    if (HasFeature(Feature::TimestampQuery) &&
+    if (mIsTimestampQueryEnabled && !IsToggleEnabled(Toggle::DisableTimestampQueryConversion)) {
-        !IsToggleEnabled(Toggle::DisableTimestampQueryConversion)) {
         // Make a best guess of timestamp period based on device vendor info, and converge it to
         // an accurate value by the following calculations.
         mTimestampPeriod = gpu_info::IsIntel(GetAdapter()->GetVendorId()) ? 83.333f : 1.0f;
@@ -266,8 +268,7 @@ MaybeError Device::TickImpl() {
 
     // Just run timestamp period calculation when timestamp feature is enabled and timestamp
     // conversion is not disabled.
-    if ((HasFeature(Feature::TimestampQuery) || HasFeature(Feature::TimestampQueryInsidePasses)) &&
+    if (mIsTimestampQueryEnabled && !IsToggleEnabled(Toggle::DisableTimestampQueryConversion)) {
-        !IsToggleEnabled(Toggle::DisableTimestampQueryConversion)) {
         if (@available(macos 10.15, iOS 14.0, *)) {
             UpdateTimestampPeriod(GetMTLDevice(), mKalmanInfo.get(), &mCpuTimestamp, &mGpuTimestamp,
                                   &mTimestampPeriod);

src/dawn/tests/BUILD.gn

@@ -647,14 +647,21 @@ source_set("white_box_tests_sources") {
   if (dawn_enable_d3d12) {
     sources += [
       "white_box/D3D12DescriptorHeapTests.cpp",
-      "white_box/D3D12GPUTimestampCalibrationTests.cpp",
       "white_box/D3D12ResidencyTests.cpp",
       "white_box/D3D12ResourceHeapTests.cpp",
+      "white_box/GPUTimestampCalibrationTests.cpp",
+      "white_box/GPUTimestampCalibrationTests.h",
+      "white_box/GPUTimestampCalibrationTests_D3D12.cpp",
     ]
   }
 
   if (dawn_enable_metal) {
-    sources += [ "white_box/MetalAutoreleasePoolTests.mm" ]
+    sources += [
+      "white_box/GPUTimestampCalibrationTests.cpp",
+      "white_box/GPUTimestampCalibrationTests.h",
+      "white_box/GPUTimestampCalibrationTests_Metal.mm",
+      "white_box/MetalAutoreleasePoolTests.mm",
+    ]
   }
 
   if (dawn_enable_opengles) {

src/dawn/tests/white_box/GPUTimestampCalibrationTests.cpp

@@ -16,11 +16,11 @@
 
 #include "dawn/native/Buffer.h"
 #include "dawn/native/CommandEncoder.h"
-#include "dawn/native/d3d12/DeviceD3D12.h"
 #include "dawn/tests/DawnTest.h"
+#include "dawn/tests/white_box/GPUTimestampCalibrationTests.h"
+#include "dawn/utils/ComboRenderPipelineDescriptor.h"
 #include "dawn/utils/WGPUHelpers.h"
 
-namespace dawn::native::d3d12 {
 namespace {
 
 using FeatureName = wgpu::FeatureName;
@@ -84,8 +84,7 @@ class ExpectBetweenTimestamps : public ::detail::Expectation {
 
 }  // anonymous namespace
 
-class D3D12GPUTimestampCalibrationTests
+class GPUTimestampCalibrationTests : public DawnTestWithParams<GPUTimestampCalibrationTestParams> {
-    : public DawnTestWithParams<GPUTimestampCalibrationTestParams> {
   protected:
     void SetUp() override {
         DawnTestWithParams<GPUTimestampCalibrationTestParams>::SetUp();
@@ -98,6 +97,14 @@ class D3D12GPUTimestampCalibrationTests
         DAWN_TEST_UNSUPPORTED_IF(GetParam().mFeatureName ==
                                      wgpu::FeatureName::TimestampQueryInsidePasses &&
                                  GetParam().mEncoderType == EncoderType::NonPass);
+
+        mBackend = GPUTimestampCalibrationTestBackend::Create(device);
+        DAWN_TEST_UNSUPPORTED_IF(!mBackend->IsSupported());
+    }
+
+    void TearDown() override {
+        mBackend = nullptr;
+        DawnTestWithParams::TearDown();
     }
 
     std::vector<wgpu::FeatureName> GetRequiredFeatures() override {
@@ -109,6 +116,38 @@ class D3D12GPUTimestampCalibrationTests
         return requiredFeatures;
     }
 
+    wgpu::ComputePipeline CreateComputePipeline() {
+        wgpu::ShaderModule module = utils::CreateShaderModule(device, R"(
+            @compute @workgroup_size(1)
+            fn main() {
+            })");
+
+        wgpu::ComputePipelineDescriptor descriptor;
+        descriptor.compute.module = module;
+        descriptor.compute.entryPoint = "main";
+
+        return device.CreateComputePipeline(&descriptor);
+    }
+
+    wgpu::RenderPipeline CreateRenderPipeline() {
+        utils::ComboRenderPipelineDescriptor descriptor;
+        descriptor.vertex.module = utils::CreateShaderModule(device, R"(
+                @vertex
+                fn main(@builtin(vertex_index) VertexIndex : u32) -> @builtin(position) vec4f {
+                    var pos = array(
+                        vec2f( 1.0,  1.0),
+                        vec2f(-1.0, -1.0),
+                        vec2f( 1.0, -1.0));
+                    return vec4f(pos[VertexIndex], 0.0, 1.0);
+                })");
+        descriptor.cFragment.module = utils::CreateShaderModule(device, R"(
+                @fragment fn main() -> @location(0) vec4f {
+                    return vec4f(0.0, 1.0, 0.0, 1.0);
+                })");
+
+        return device.CreateRenderPipeline(&descriptor);
+    }
+
     void EncodeTimestampQueryOnComputePass(const wgpu::CommandEncoder& encoder,
                                            const wgpu::QuerySet& querySet) {
         switch (GetParam().mFeatureName) {
@@ -123,12 +162,16 @@ class D3D12GPUTimestampCalibrationTests
                 descriptor.timestampWrites = timestampWrites.data();
 
                 wgpu::ComputePassEncoder pass = encoder.BeginComputePass(&descriptor);
+                pass.SetPipeline(CreateComputePipeline());
+                pass.DispatchWorkgroups(1);
                 pass.End();
                 break;
             }
             case wgpu::FeatureName::TimestampQueryInsidePasses: {
                 wgpu::ComputePassEncoder pass = encoder.BeginComputePass();
                 pass.WriteTimestamp(querySet, 0);
+                pass.SetPipeline(CreateComputePipeline());
+                pass.DispatchWorkgroups(1);
                 pass.WriteTimestamp(querySet, 1);
                 pass.End();
                 break;
@@ -154,12 +197,16 @@ class D3D12GPUTimestampCalibrationTests
                 renderPass.renderPassInfo.timestampWrites = timestampWrites.data();
 
                 wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo);
+                pass.SetPipeline(CreateRenderPipeline());
+                pass.Draw(3);
                 pass.End();
                 break;
             }
             case wgpu::FeatureName::TimestampQueryInsidePasses: {
                 wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo);
                 pass.WriteTimestamp(querySet, 0);
+                pass.SetPipeline(CreateRenderPipeline());
+                pass.Draw(3);
                 pass.WriteTimestamp(querySet, 1);
                 pass.End();
                 break;
@@ -207,13 +254,12 @@ class D3D12GPUTimestampCalibrationTests
         wgpu::CommandBuffer commands = encoder.Finish();
 
         // Start calibration between GPU timestamp and CPU timestamp
-        Device* d3DDevice = reinterpret_cast<Device*>(device.Get());
         uint64_t gpuTimestamp0, gpuTimestamp1;
         uint64_t cpuTimestamp0, cpuTimestamp1;
-        d3DDevice->GetCommandQueue()->GetClockCalibration(&gpuTimestamp0, &cpuTimestamp0);
+        mBackend->GetTimestampCalibration(&gpuTimestamp0, &cpuTimestamp0);
         queue.Submit(1, &commands);
         WaitForAllOperations();
-        d3DDevice->GetCommandQueue()->GetClockCalibration(&gpuTimestamp1, &cpuTimestamp1);
+        mBackend->GetTimestampCalibration(&gpuTimestamp1, &cpuTimestamp1);
 
         // Separate resolve queryset to reduce the execution time of the queue with WriteTimestamp,
         // so that the timestamp in the querySet will be closer to both gpuTimestamps from
@@ -225,9 +271,7 @@ class D3D12GPUTimestampCalibrationTests
 
         float errorToleranceRatio = 0.0f;
         if (!HasToggleEnabled("disable_timestamp_query_conversion")) {
-            uint64_t gpuFrequency;
+            float period = mBackend->GetTimestampPeriod();
-            d3DDevice->GetCommandQueue()->GetTimestampFrequency(&gpuFrequency);
-            float period = static_cast<float>(1e9) / gpuFrequency;
             gpuTimestamp0 = static_cast<uint64_t>(static_cast<double>(gpuTimestamp0 * period));
             gpuTimestamp1 = static_cast<uint64_t>(static_cast<double>(gpuTimestamp1 * period));
 
@@ -242,21 +286,22 @@ class D3D12GPUTimestampCalibrationTests
     }
 
   private:
+    std::unique_ptr<GPUTimestampCalibrationTestBackend> mBackend;
     bool mIsFeatureSupported = false;
 };
 
 // Check that the timestamps got by timestamp query are between the two timestamps from
 // GetClockCalibration() with the 'disable_timestamp_query_conversion' toggle disabled or enabled.
-TEST_P(D3D12GPUTimestampCalibrationTests, TimestampsCalibration) {
+TEST_P(GPUTimestampCalibrationTests, TimestampsCalibration) {
     RunTest();
 }
 
 DAWN_INSTANTIATE_TEST_P(
-    D3D12GPUTimestampCalibrationTests,
+    GPUTimestampCalibrationTests,
     // Test with the disable_timestamp_query_conversion toggle forced on and off.
     {D3D12Backend({"disable_timestamp_query_conversion"}, {}),
-     D3D12Backend({}, {"disable_timestamp_query_conversion"})},
+     D3D12Backend({}, {"disable_timestamp_query_conversion"}),
+     MetalBackend({"disable_timestamp_query_conversion"}, {}),
+     MetalBackend({}, {"disable_timestamp_query_conversion"})},
     {wgpu::FeatureName::TimestampQuery, wgpu::FeatureName::TimestampQueryInsidePasses},
     {EncoderType::NonPass, EncoderType::ComputePass, EncoderType::RenderPass});
-
-}  // namespace dawn::native::d3d12

src/dawn/tests/white_box/GPUTimestampCalibrationTests.h

@@ -0,0 +1,30 @@
+// Copyright 2023 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef SRC_DAWN_TESTS_WHITE_BOX_GPUTIMESTAMPCALIBRATIONTESTS_H_
+#define SRC_DAWN_TESTS_WHITE_BOX_GPUTIMESTAMPCALIBRATIONTESTS_H_
+
+#include <memory>
+
+class GPUTimestampCalibrationTestBackend {
+  public:
+    static std::unique_ptr<GPUTimestampCalibrationTestBackend> Create(const wgpu::Device& device);
+    virtual ~GPUTimestampCalibrationTestBackend() = default;
+
+    virtual bool IsSupported() const = 0;
+    virtual void GetTimestampCalibration(uint64_t* gpuTimestamp, uint64_t* cpuTimestamp) = 0;
+    virtual float GetTimestampPeriod() const = 0;
+};
+
+#endif  // SRC_DAWN_TESTS_WHITE_BOX_GPUTIMESTAMPCALIBRATIONTESTS_H_

src/dawn/tests/white_box/GPUTimestampCalibrationTests_D3D12.cpp

@@ -0,0 +1,42 @@
+// Copyright 2023 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <memory>
+
+#include "dawn/native/d3d12/DeviceD3D12.h"
+#include "dawn/tests/white_box/GPUTimestampCalibrationTests.h"
+
+class GPUTimestampCalibrationTestsD3D12 : public GPUTimestampCalibrationTestBackend {
+  public:
+    explicit GPUTimestampCalibrationTestsD3D12(const wgpu::Device& device) {
+        mBackendDevice = dawn::native::d3d12::ToBackend(dawn::native::FromAPI(device.Get()));
+    }
+
+    bool IsSupported() const override { return true; }
+
+    void GetTimestampCalibration(uint64_t* gpuTimestamp, uint64_t* cpuTimestamp) override {
+        mBackendDevice->GetCommandQueue()->GetClockCalibration(gpuTimestamp, cpuTimestamp);
+    }
+
+    float GetTimestampPeriod() const override { return mBackendDevice->GetTimestampPeriodInNS(); }
+
+  private:
+    dawn::native::d3d12::Device* mBackendDevice;
+};
+
+// static
+std::unique_ptr<GPUTimestampCalibrationTestBackend> GPUTimestampCalibrationTestBackend::Create(
+    const wgpu::Device& device) {
+    return std::make_unique<GPUTimestampCalibrationTestsD3D12>(device);
+}

src/dawn/tests/white_box/GPUTimestampCalibrationTests_Metal.mm

@@ -0,0 +1,51 @@
+// Copyright 2023 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <memory>
+
+#include "dawn/native/metal/DeviceMTL.h"
+#include "dawn/tests/white_box/GPUTimestampCalibrationTests.h"
+
+class GPUTimestampCalibrationTestsMetal : public GPUTimestampCalibrationTestBackend {
+  public:
+    explicit GPUTimestampCalibrationTestsMetal(const wgpu::Device& device) {
+        mBackendDevice = dawn::native::metal::ToBackend(dawn::native::FromAPI(device.Get()));
+    }
+
+    // The API used in timestamp calibration is only available on macOS 10.15+ and iOS 14.0+
+    bool IsSupported() const override {
+        if (@available(macos 10.15, iOS 14.0, *)) {
+            return true;
+        }
+        return false;
+    }
+
+    void GetTimestampCalibration(uint64_t* gpuTimestamp, uint64_t* cpuTimestamp) override {
+        if (@available(macos 10.15, iOS 14.0, *)) {
+            [mBackendDevice->GetMTLDevice() sampleTimestamps:cpuTimestamp
+                                                gpuTimestamp:gpuTimestamp];
+        }
+    }
+
+    float GetTimestampPeriod() const override { return mBackendDevice->GetTimestampPeriodInNS(); }
+
+  private:
+    dawn::native::metal::Device* mBackendDevice;
+};
+
+// static
+std::unique_ptr<GPUTimestampCalibrationTestBackend> GPUTimestampCalibrationTestBackend::Create(
+    const wgpu::Device& device) {
+    return std::make_unique<GPUTimestampCalibrationTestsMetal>(device);
+}