Add validation for GPURender/ComputePassTimestampLocation

Disallow duplicate location in timestampWrites in a render/compute pass
to match WebGPU SPEC.

Bug: dawn:1250
Change-Id: Id9e3b54530d37ffc0c00aa15c23312bb1ea30d00
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/90460
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Hao Li <hao.x.li@intel.com>

src/dawn/native/CommandEncoder.cpp

@@ -14,6 +14,7 @@
 
 #include "dawn/native/CommandEncoder.h"
 
+#include <unordered_set>
 #include <utility>
 #include <vector>
 
@@ -414,6 +415,30 @@ MaybeError ValidateRenderPassDepthStencilAttachment(
     return {};
 }
 
+MaybeError ValidateTimestampLocationOnRenderPass(
+    wgpu::RenderPassTimestampLocation location,
+    const std::unordered_set<wgpu::RenderPassTimestampLocation>& writtenLocations) {
+    DAWN_TRY(ValidateRenderPassTimestampLocation(location));
+
+    DAWN_INVALID_IF(writtenLocations.find(location) != writtenLocations.end(),
+                    "There are two same RenderPassTimestampLocation %u in a render pass.",
+                    location);
+
+    return {};
+}
+
+MaybeError ValidateTimestampLocationOnComputePass(
+    wgpu::ComputePassTimestampLocation location,
+    const std::unordered_set<wgpu::ComputePassTimestampLocation>& writtenLocations) {
+    DAWN_TRY(ValidateComputePassTimestampLocation(location));
+
+    DAWN_INVALID_IF(writtenLocations.find(location) != writtenLocations.end(),
+                    "There are two same ComputePassTimestampLocation %u in a compute pass.",
+                    location);
+
+    return {};
+}
+
 MaybeError ValidateRenderPassDescriptor(DeviceBase* device,
                                         const RenderPassDescriptor* descriptor,
                                         uint32_t* width,
@@ -465,15 +490,22 @@ MaybeError ValidateRenderPassDescriptor(DeviceBase* device,
         // not validated and encoded one by one, but encoded together after passing the
         // validation.
         QueryAvailabilityMap usedQueries;
+        // TODO(https://crbug.com/dawn/1452):
+        // 1. Add an enum that's TimestampLocationMask and has bit values.
+        // 2. Add a function with a switch that converts from one to the other.
+        // 3. type alias the ityp::bitset for that to call it TimestampLocationSet.
+        // 4. Use it here.
+        std::unordered_set<wgpu::RenderPassTimestampLocation> writtenLocations;
         for (uint32_t i = 0; i < descriptor->timestampWriteCount; ++i) {
             QuerySetBase* querySet = descriptor->timestampWrites[i].querySet;
             DAWN_ASSERT(querySet != nullptr);
             uint32_t queryIndex = descriptor->timestampWrites[i].queryIndex;
             DAWN_TRY_CONTEXT(ValidateTimestampQuery(device, querySet, queryIndex),
                              "validating querySet and queryIndex of timestampWrites[%u].", i);
-            DAWN_TRY_CONTEXT(
-                ValidateRenderPassTimestampLocation(descriptor->timestampWrites[i].location),
-                "validating location of timestampWrites[%u].", i);
+            DAWN_TRY_CONTEXT(ValidateTimestampLocationOnRenderPass(
+                                 descriptor->timestampWrites[i].location, writtenLocations),
+                             "validating location of timestampWrites[%u].", i);
+            writtenLocations.insert(descriptor->timestampWrites[i].location);
 
             auto checkIt = usedQueries.find(querySet);
             DAWN_INVALID_IF(checkIt != usedQueries.end() && checkIt->second[queryIndex],
@@ -503,14 +535,21 @@ MaybeError ValidateComputePassDescriptor(const DeviceBase* device,
     if (descriptor->timestampWriteCount > 0) {
         DAWN_ASSERT(descriptor->timestampWrites != nullptr);
 
+        // TODO(https://crbug.com/dawn/1452):
+        // 1. Add an enum that's TimestampLocationMask and has bit values.
+        // 2. Add a function with a switch that converts from one to the other.
+        // 3. type alias the ityp::bitset for that to call it TimestampLocationSet.
+        // 4. Use it here.
+        std::unordered_set<wgpu::ComputePassTimestampLocation> writtenLocations;
         for (uint32_t i = 0; i < descriptor->timestampWriteCount; ++i) {
             DAWN_ASSERT(descriptor->timestampWrites[i].querySet != nullptr);
             DAWN_TRY_CONTEXT(ValidateTimestampQuery(device, descriptor->timestampWrites[i].querySet,
                                                     descriptor->timestampWrites[i].queryIndex),
                              "validating querySet and queryIndex of timestampWrites[%u].", i);
-            DAWN_TRY_CONTEXT(
-                ValidateComputePassTimestampLocation(descriptor->timestampWrites[i].location),
-                "validating location of timestampWrites[%u].", i);
+            DAWN_TRY_CONTEXT(ValidateTimestampLocationOnComputePass(
+                                 descriptor->timestampWrites[i].location, writtenLocations),
+                             "validating location of timestampWrites[%u].", i);
+            writtenLocations.insert(descriptor->timestampWrites[i].location);
         }
     }
 

src/dawn/tests/unittests/validation/QueryValidationTests.cpp

@@ -352,13 +352,23 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnComputePass) {
         encoder.Finish();
     }
 
-    // Success to write timestamps at same location of compute pass
+    // Success to write timestamps at same location of different compute pass
+    {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        EncodeComputePassWithTimestampWrites(
+            encoder, {{querySet, 0, wgpu::ComputePassTimestampLocation::Beginning}});
+        EncodeComputePassWithTimestampWrites(
+            encoder, {{querySet, 0, wgpu::ComputePassTimestampLocation::Beginning}});
+        encoder.Finish();
+    }
+
+    // Fail to write timestamps at same location of a compute pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
         EncodeComputePassWithTimestampWrites(
             encoder, {{querySet, 0, wgpu::ComputePassTimestampLocation::Beginning},
                       {querySet, 1, wgpu::ComputePassTimestampLocation::Beginning}});
-        encoder.Finish();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps at invalid location of compute pass
@@ -427,7 +437,8 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) {
         ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
-    // Success to write timestamps to the same query index twice on different render pass
+    // Success to write timestamps to the same query index and location twice on different render
+    // pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
         EncodeRenderPassWithTimestampWrites(
@@ -449,13 +460,13 @@ TEST_F(TimestampQueryValidationTest, TimestampWritesOnRenderPass) {
         ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
-    // Success to write timestamps at same location of render pass
+    // Fail to write timestamps at same location of a render pass
     {
         wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
         EncodeRenderPassWithTimestampWrites(
             encoder, {{querySet, 0, wgpu::RenderPassTimestampLocation::Beginning},
                       {querySet, 1, wgpu::RenderPassTimestampLocation::Beginning}});
-        encoder.Finish();
+        ASSERT_DEVICE_ERROR(encoder.Finish());
     }
 
     // Fail to write timestamps at invalid location of render pass