Move CreateFence from Device to Queue

Bug: dawn:113
Change-Id: I5ec829d8945cdc25644f481acc07a9f6d8b13aef
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/5200
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

dawn.json

@@ -436,13 +436,6 @@
                 "name": "create command encoder",
                 "returns": "command encoder"
             },
-            {
-                "name": "create fence",
-                "returns": "fence",
-                "args": [
-                    {"name": "descriptor", "type": "fence descriptor", "annotation": "const*"}
-                ]
-            },
             {
                 "name": "create input state builder",
                 "returns": "input state builder"
@@ -714,6 +707,13 @@
                     {"name": "fence", "type": "fence"},
                     {"name": "signal value", "type": "uint64_t"}
                 ]
+            },
+            {
+                "name": "create fence",
+                "returns": "fence",
+                "args": [
+                    {"name": "descriptor", "type": "fence descriptor", "annotation": "const*"}
+                ]
             }
         ]
     },

dawn_wire.json

@@ -58,7 +58,7 @@
         ],
         "client_handwritten_commands": [
             "BufferUnmap",
-            "DeviceCreateFence",
+            "QueueCreateFence",
             "FenceGetCompletedValue",
             "QueueSignal"
         ],

src/dawn_native/Device.cpp

@@ -159,15 +159,6 @@ namespace dawn_native {
 
         return result;
     }
-    FenceBase* DeviceBase::CreateFence(const FenceDescriptor* descriptor) {
-        FenceBase* result = nullptr;
-
-        if (ConsumedError(CreateFenceInternal(&result, descriptor))) {
-            return FenceBase::MakeError(this);
-        }
-
-        return result;
-    }
     InputStateBuilder* DeviceBase::CreateInputStateBuilder() {
         return new InputStateBuilder(this);
     }
@@ -302,13 +293,6 @@ namespace dawn_native {
         return {};
     }
 
-    MaybeError DeviceBase::CreateFenceInternal(FenceBase** result,
-                                               const FenceDescriptor* descriptor) {
-        DAWN_TRY(ValidateFenceDescriptor(this, descriptor));
-        *result = new FenceBase(this, descriptor);
-        return {};
-    }
-
     MaybeError DeviceBase::CreatePipelineLayoutInternal(
         PipelineLayoutBase** result,
         const PipelineLayoutDescriptor* descriptor) {

src/dawn_native/Device.h

@@ -91,7 +91,6 @@ namespace dawn_native {
         BufferBase* CreateBuffer(const BufferDescriptor* descriptor);
         CommandEncoderBase* CreateCommandEncoder();
         ComputePipelineBase* CreateComputePipeline(const ComputePipelineDescriptor* descriptor);
-        FenceBase* CreateFence(const FenceDescriptor* descriptor);
         InputStateBuilder* CreateInputStateBuilder();
         PipelineLayoutBase* CreatePipelineLayout(const PipelineLayoutDescriptor* descriptor);
         QueueBase* CreateQueue();
@@ -158,7 +157,6 @@ namespace dawn_native {
         MaybeError CreateBufferInternal(BufferBase** result, const BufferDescriptor* descriptor);
         MaybeError CreateComputePipelineInternal(ComputePipelineBase** result,
                                                  const ComputePipelineDescriptor* descriptor);
-        MaybeError CreateFenceInternal(FenceBase** result, const FenceDescriptor* descriptor);
         MaybeError CreatePipelineLayoutInternal(PipelineLayoutBase** result,
                                                 const PipelineLayoutDescriptor* descriptor);
         MaybeError CreateQueueInternal(QueueBase** result);

src/dawn_native/Fence.cpp

@@ -16,13 +16,14 @@
 
 #include "common/Assert.h"
 #include "dawn_native/Device.h"
+#include "dawn_native/Queue.h"
 #include "dawn_native/ValidationUtils_autogen.h"
 
 #include <utility>
 
 namespace dawn_native {
 
-    MaybeError ValidateFenceDescriptor(DeviceBase*, const FenceDescriptor* descriptor) {
+    MaybeError ValidateFenceDescriptor(const FenceDescriptor* descriptor) {
         if (descriptor->nextInChain != nullptr) {
             return DAWN_VALIDATION_ERROR("nextInChain must be nullptr");
         }
@@ -32,10 +33,11 @@ namespace dawn_native {
 
     // Fence
 
-    FenceBase::FenceBase(DeviceBase* device, const FenceDescriptor* descriptor)
+    FenceBase::FenceBase(QueueBase* queue, const FenceDescriptor* descriptor)
-        : ObjectBase(device),
+        : ObjectBase(queue->GetDevice()),
           mSignalValue(descriptor->initialValue),
-          mCompletedValue(descriptor->initialValue) {
+          mCompletedValue(descriptor->initialValue),
+          mQueue(queue) {
     }
 
     FenceBase::FenceBase(DeviceBase* device, ObjectBase::ErrorTag tag) : ObjectBase(device, tag) {
@@ -86,6 +88,11 @@ namespace dawn_native {
         return mSignalValue;
     }
 
+    const QueueBase* FenceBase::GetQueue() const {
+        ASSERT(!IsError());
+        return mQueue.Get();
+    }
+
     void FenceBase::SetSignaledValue(uint64_t signalValue) {
         ASSERT(!IsError());
         ASSERT(signalValue > mSignalValue);

src/dawn_native/Fence.h

@@ -26,16 +26,17 @@
 
 namespace dawn_native {
 
-    MaybeError ValidateFenceDescriptor(DeviceBase*, const FenceDescriptor* descriptor);
+    MaybeError ValidateFenceDescriptor(const FenceDescriptor* descriptor);
 
     class FenceBase : public ObjectBase {
       public:
-        FenceBase(DeviceBase* device, const FenceDescriptor* descriptor);
+        FenceBase(QueueBase* queue, const FenceDescriptor* descriptor);
         ~FenceBase();
 
         static FenceBase* MakeError(DeviceBase* device);
 
         uint64_t GetSignaledValue() const;
+        const QueueBase* GetQueue() const;
 
         // Dawn API
         uint64_t GetCompletedValue() const;
@@ -61,6 +62,7 @@ namespace dawn_native {
 
         uint64_t mSignalValue;
         uint64_t mCompletedValue;
+        Ref<QueueBase> mQueue;
         SerialMap<OnCompletionData> mRequests;
     };
 

src/dawn_native/Queue.cpp

@@ -47,6 +47,14 @@ namespace dawn_native {
         GetDevice()->GetFenceSignalTracker()->UpdateFenceOnComplete(fence, signalValue);
     }
 
+    FenceBase* QueueBase::CreateFence(const FenceDescriptor* descriptor) {
+        if (GetDevice()->ConsumedError(ValidateCreateFence(descriptor))) {
+            return FenceBase::MakeError(GetDevice());
+        }
+
+        return new FenceBase(this, descriptor);
+    }
+
     MaybeError QueueBase::ValidateSubmit(uint32_t commandCount,
                                          CommandBufferBase* const* commands) {
         DAWN_TRY(GetDevice()->ValidateObject(this));
@@ -86,10 +94,21 @@ namespace dawn_native {
         DAWN_TRY(GetDevice()->ValidateObject(this));
         DAWN_TRY(GetDevice()->ValidateObject(fence));
 
+        if (fence->GetQueue() != this) {
+            return DAWN_VALIDATION_ERROR(
+                "Fence must be signaled on the queue on which it was created.");
+        }
         if (signalValue <= fence->GetSignaledValue()) {
             return DAWN_VALIDATION_ERROR("Signal value less than or equal to fence signaled value");
         }
         return {};
     }
 
+    MaybeError QueueBase::ValidateCreateFence(const FenceDescriptor* descriptor) {
+        DAWN_TRY(GetDevice()->ValidateObject(this));
+        DAWN_TRY(ValidateFenceDescriptor(descriptor));
+
+        return {};
+    }
+
 }  // namespace dawn_native

src/dawn_native/Queue.h

@@ -31,12 +31,14 @@ namespace dawn_native {
         // Dawn API
         void Submit(uint32_t commandCount, CommandBufferBase* const* commands);
         void Signal(FenceBase* fence, uint64_t signalValue);
+        FenceBase* CreateFence(const FenceDescriptor* descriptor);
 
       private:
         virtual void SubmitImpl(uint32_t commandCount, CommandBufferBase* const* commands) = 0;
 
         MaybeError ValidateSubmit(uint32_t commandCount, CommandBufferBase* const* commands);
         MaybeError ValidateSignal(const FenceBase* fence, uint64_t signalValue);
+        MaybeError ValidateCreateFence(const FenceDescriptor* descriptor);
     };
 
 }  // namespace dawn_native

src/dawn_wire/client/ApiProcs.cpp

@@ -133,10 +133,11 @@ namespace dawn_wire { namespace client {
         cmd.Serialize(allocatedBuffer, *buffer->device->GetClient());
     }
 
-    dawnFence ClientDeviceCreateFence(dawnDevice cSelf, dawnFenceDescriptor const* descriptor) {
+    dawnFence ClientQueueCreateFence(dawnQueue cSelf, dawnFenceDescriptor const* descriptor) {
-        Device* device = reinterpret_cast<Device*>(cSelf);
+        Queue* queue = reinterpret_cast<Queue*>(cSelf);
+        Device* device = queue->device;
 
-        DeviceCreateFenceCmd cmd;
+        QueueCreateFenceCmd cmd;
         cmd.self = cSelf;
         auto* allocation = device->GetClient()->FenceAllocator().New(device);
         cmd.result = ObjectHandle{allocation->object->id, allocation->serial};
@@ -149,6 +150,7 @@ namespace dawn_wire { namespace client {
         dawnFence cFence = reinterpret_cast<dawnFence>(allocation->object.get());
 
         Fence* fence = reinterpret_cast<Fence*>(cFence);
+        fence->queue = queue;
         fence->signaledValue = descriptor->initialValue;
         fence->completedValue = descriptor->initialValue;
         return cFence;
@@ -156,6 +158,12 @@ namespace dawn_wire { namespace client {
 
     void ClientQueueSignal(dawnQueue cQueue, dawnFence cFence, uint64_t signalValue) {
         Fence* fence = reinterpret_cast<Fence*>(cFence);
+        Queue* queue = reinterpret_cast<Queue*>(cQueue);
+        if (fence->queue != queue) {
+            fence->device->HandleError(
+                "Fence must be signaled on the queue on which it was created.");
+            return;
+        }
         if (signalValue <= fence->signaledValue) {
             fence->device->HandleError("Fence value less than or equal to signaled value");
             return;

src/dawn_wire/client/Fence.h

@@ -22,6 +22,7 @@
 
 namespace dawn_wire { namespace client {
 
+    struct Queue;
     struct Fence : ObjectBase {
         using ObjectBase::ObjectBase;
 
@@ -32,6 +33,7 @@ namespace dawn_wire { namespace client {
             dawnFenceOnCompletionCallback completionCallback = nullptr;
             dawnCallbackUserdata userdata = 0;
         };
+        Queue* queue = nullptr;
         uint64_t signaledValue = 0;
         uint64_t completedValue = 0;
         SerialMap<OnCompletionData> requests;

src/tests/end2end/FenceTests.cpp

@@ -69,7 +69,7 @@ class FenceTests : public DawnTest {
 TEST_P(FenceTests, SimpleSignal) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 1u;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     // Completed value starts at initial value
     EXPECT_EQ(fence.GetCompletedValue(), 1u);
@@ -85,7 +85,7 @@ TEST_P(FenceTests, SimpleSignal) {
 TEST_P(FenceTests, OnCompletionOrdering) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 0u;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     queue.Signal(fence, 4);
 
@@ -126,7 +126,7 @@ TEST_P(FenceTests, OnCompletionOrdering) {
 TEST_P(FenceTests, MultipleSignalOnCompletion) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 0u;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     queue.Signal(fence, 2);
     queue.Signal(fence, 4);
@@ -144,7 +144,7 @@ TEST_P(FenceTests, MultipleSignalOnCompletion) {
 TEST_P(FenceTests, OnCompletionMultipleCallbacks) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 0u;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     queue.Signal(fence, 4);
 
@@ -182,13 +182,13 @@ TEST_P(FenceTests, OnCompletionMultipleCallbacks) {
 TEST_P(FenceTests, DISABLED_DestroyBeforeOnCompletionEnd) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 0u;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     // The fence in this block will be deleted when it goes out of scope
     {
         dawn::FenceDescriptor descriptor;
         descriptor.initialValue = 0u;
-        dawn::Fence testFence = device.CreateFence(&descriptor);
+        dawn::Fence testFence = queue.CreateFence(&descriptor);
 
         queue.Signal(testFence, 4);
 

src/tests/end2end/IOSurfaceWrappingTests.cpp

@@ -351,7 +351,7 @@ class IOSurfaceUsageTests : public IOSurfaceTestBase {
         // Maybe it is because the Metal command buffer has been submitted but not "scheduled" yet?
         dawn::FenceDescriptor fenceDescriptor;
         fenceDescriptor.initialValue = 0u;
-        dawn::Fence fence = device.CreateFence(&fenceDescriptor);
+        dawn::Fence fence = queue.CreateFence(&fenceDescriptor);
         queue.Signal(fence, 1);
 
         while (fence.GetCompletedValue() < 1) {

src/tests/unittests/validation/FenceValidationTests.cpp

@@ -77,7 +77,7 @@ TEST_F(FenceValidationTest, CreationSuccess) {
     {
         dawn::FenceDescriptor descriptor;
         descriptor.initialValue = 0;
-        device.CreateFence(&descriptor);
+        queue.CreateFence(&descriptor);
     }
 }
 
@@ -86,7 +86,7 @@ TEST_F(FenceValidationTest, GetCompletedValue) {
     {
         dawn::FenceDescriptor descriptor;
         descriptor.initialValue = 1;
-        dawn::Fence fence = device.CreateFence(&descriptor);
+        dawn::Fence fence = queue.CreateFence(&descriptor);
         EXPECT_EQ(fence.GetCompletedValue(), 1u);
     }
 }
@@ -96,7 +96,7 @@ TEST_F(FenceValidationTest, GetCompletedValue) {
 TEST_F(FenceValidationTest, OnCompletionImmediate) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 1;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     EXPECT_CALL(*mockFenceOnCompletionCallback, Call(DAWN_FENCE_COMPLETION_STATUS_SUCCESS, 0))
         .Times(1);
@@ -111,7 +111,7 @@ TEST_F(FenceValidationTest, OnCompletionImmediate) {
 TEST_F(FenceValidationTest, OnCompletionLargerThanSignaled) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 1;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     // Cannot signal for values > signaled value
     EXPECT_CALL(*mockFenceOnCompletionCallback, Call(DAWN_FENCE_COMPLETION_STATUS_ERROR, 0))
@@ -130,7 +130,7 @@ TEST_F(FenceValidationTest, OnCompletionLargerThanSignaled) {
 TEST_F(FenceValidationTest, GetCompletedValueInsideCallback) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 1;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     queue.Signal(fence, 3);
     fence.OnCompletion(2u, ToMockFenceOnCompletionCallback, 0);
@@ -145,7 +145,7 @@ TEST_F(FenceValidationTest, GetCompletedValueInsideCallback) {
 TEST_F(FenceValidationTest, GetCompletedValueAfterCallback) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 1;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     queue.Signal(fence, 2);
     fence.OnCompletion(2u, ToMockFenceOnCompletionCallback, 0);
@@ -159,7 +159,7 @@ TEST_F(FenceValidationTest, GetCompletedValueAfterCallback) {
 TEST_F(FenceValidationTest, SignalError) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 1;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     // value < fence signaled value
     ASSERT_DEVICE_ERROR(queue.Signal(fence, 0));
@@ -171,7 +171,7 @@ TEST_F(FenceValidationTest, SignalError) {
 TEST_F(FenceValidationTest, SignalSuccess) {
     dawn::FenceDescriptor descriptor;
     descriptor.initialValue = 1;
-    dawn::Fence fence = device.CreateFence(&descriptor);
+    dawn::Fence fence = queue.CreateFence(&descriptor);
 
     // Success
     queue.Signal(fence, 2);
@@ -183,3 +183,34 @@ TEST_F(FenceValidationTest, SignalSuccess) {
     Flush();
     EXPECT_EQ(fence.GetCompletedValue(), 6u);
 }
+
+// Test it is invalid to signal a fence on a different queue than it was created on
+TEST_F(FenceValidationTest, SignalWrongQueue) {
+    dawn::Queue queue2 = device.CreateQueue();
+
+    dawn::FenceDescriptor descriptor;
+    descriptor.initialValue = 1;
+    dawn::Fence fence = queue.CreateFence(&descriptor);
+
+    ASSERT_DEVICE_ERROR(queue2.Signal(fence, 2));
+}
+
+// Test that signaling a fence on a wrong queue does not update fence signaled value
+TEST_F(FenceValidationTest, SignalWrongQueueDoesNotUpdateValue) {
+    dawn::Queue queue2 = device.CreateQueue();
+
+    dawn::FenceDescriptor descriptor;
+    descriptor.initialValue = 1;
+    dawn::Fence fence = queue.CreateFence(&descriptor);
+
+    ASSERT_DEVICE_ERROR(queue2.Signal(fence, 2));
+
+    // Fence value should be unchanged.
+    Flush();
+    EXPECT_EQ(fence.GetCompletedValue(), 1u);
+
+    // Signaling with 2 on the correct queue should succeed
+    queue.Signal(fence, 2);
+    Flush();
+    EXPECT_EQ(fence.GetCompletedValue(), 2u);
+}

src/tests/unittests/wire/WireFenceTests.cpp

@@ -55,25 +55,25 @@ class WireFenceTests : public WireTest {
         mockDeviceErrorCallback = std::make_unique<MockDeviceErrorCallback>();
         mockFenceOnCompletionCallback = std::make_unique<MockFenceOnCompletionCallback>();
 
+        {
+            queue = dawnDeviceCreateQueue(device);
+            apiQueue = api.GetNewQueue();
+            EXPECT_CALL(api, DeviceCreateQueue(apiDevice)).WillOnce(Return(apiQueue));
+            EXPECT_CALL(api, QueueRelease(apiQueue));
+            FlushClient();
+        }
         {
             dawnFenceDescriptor descriptor;
             descriptor.initialValue = 1;
             descriptor.nextInChain = nullptr;
 
             apiFence = api.GetNewFence();
-            fence = dawnDeviceCreateFence(device, &descriptor);
+            fence = dawnQueueCreateFence(queue, &descriptor);
 
-            EXPECT_CALL(api, DeviceCreateFence(apiDevice, _)).WillOnce(Return(apiFence));
+            EXPECT_CALL(api, QueueCreateFence(apiQueue, _)).WillOnce(Return(apiFence));
             EXPECT_CALL(api, FenceRelease(apiFence));
             FlushClient();
         }
-        {
-            queue = dawnDeviceCreateQueue(device);
-            apiQueue = api.GetNewQueue();
-            EXPECT_CALL(api, DeviceCreateQueue(apiDevice)).WillOnce(Return(apiQueue));
-            EXPECT_CALL(api, QueueRelease(apiQueue));
-            FlushClient();
-        }
     }
 
     void TearDown() override {
@@ -264,3 +264,44 @@ TEST_F(WireFenceTests, DestroyBeforeOnCompletionEnd) {
 
     dawnFenceRelease(fence);
 }
+
+// Test that signaling a fence on a wrong queue is invalid
+TEST_F(WireFenceTests, SignalWrongQueue) {
+    dawnQueue queue2 = dawnDeviceCreateQueue(device);
+    dawnQueue apiQueue2 = api.GetNewQueue();
+    EXPECT_CALL(api, DeviceCreateQueue(apiDevice)).WillOnce(Return(apiQueue2));
+    EXPECT_CALL(api, QueueRelease(apiQueue2));
+    FlushClient();
+
+    dawnCallbackUserdata userdata = 1520;
+    dawnDeviceSetErrorCallback(device, ToMockDeviceErrorCallback, userdata);
+
+    EXPECT_CALL(*mockDeviceErrorCallback, Call(_, userdata)).Times(1);
+    dawnQueueSignal(queue2, fence, 2u);  // error
+}
+
+// Test that signaling a fence on a wrong queue does not update fence signaled value
+TEST_F(WireFenceTests, SignalWrongQueueDoesNotUpdateValue) {
+    dawnQueue queue2 = dawnDeviceCreateQueue(device);
+    dawnQueue apiQueue2 = api.GetNewQueue();
+    EXPECT_CALL(api, DeviceCreateQueue(apiDevice)).WillOnce(Return(apiQueue2));
+    EXPECT_CALL(api, QueueRelease(apiQueue2));
+    FlushClient();
+
+    dawnCallbackUserdata userdata = 1024;
+    dawnDeviceSetErrorCallback(device, ToMockDeviceErrorCallback, userdata);
+
+    EXPECT_CALL(*mockDeviceErrorCallback, Call(_, userdata)).Times(1);
+    dawnQueueSignal(queue2, fence, 2u);  // error
+
+    // Fence value should be unchanged.
+    FlushClient();
+    FlushServer();
+    EXPECT_EQ(dawnFenceGetCompletedValue(fence), 1u);
+
+    // Signaling with 2 on the correct queue should succeed
+    DoQueueSignal(2u);  // success
+    FlushClient();
+    FlushServer();
+    EXPECT_EQ(dawnFenceGetCompletedValue(fence), 2u);
+}