LowLatency2: Check semaphore value before signalling

5 files changed, 87 insertions, 38 deletions

diff --git a/src/strategies/low_latency2/device_strategy.cc b/src/strategies/low_latency2/device_strategy.cc
index f82098d..81934ba 100644
--- a/src/strategies/low_latency2/device_strategy.cc
+++ b/src/strategies/low_latency2/device_strategy.cc
@@ -112,19 +112,18 @@ void LowLatency2DeviceStrategy::notify_latency_sleep_nv(
 
     const auto lock = std::scoped_lock{this->mutex};
 
+    const auto semaphore_signal =
+        SemaphoreSignal{info.signalSemaphore, info.value};
+
     const auto iter = this->swapchain_monitors.find(swapchain);
     if (iter == std::end(this->swapchain_monitors)) {
         // If we can't find the swapchain we have to signal the semaphore
         // anyway. We must *never* discard these semaphores without signalling
         // them first.
-        const auto ssi = VkSemaphoreSignalInfo{
-            .sType = VK_STRUCTURE_TYPE_SEMAPHORE_SIGNAL_INFO,
-            .semaphore = info.signalSemaphore,
-            .value = info.value};
-        THROW_NOT_VKSUCCESS(device.vtable.SignalSemaphore(device.device, &ssi));
+        semaphore_signal.signal(this->device);
         return;
     }
-    iter->second.notify_semaphore(info.signalSemaphore, info.value);
+    iter->second.notify_semaphore(semaphore_signal);
 }
 
 } // namespace low_latency
diff --git a/src/strategies/low_latency2/semaphore_signal.cc b/src/strategies/low_latency2/semaphore_signal.cc
new file mode 100644
index 0000000..9597b71
--- /dev/null
+++ b/src/strategies/low_latency2/semaphore_signal.cc
@@ -0,0 +1,31 @@
+#include "semaphore_signal.hh"
+
+#include "helper.hh"
+
+namespace low_latency {
+
+SemaphoreSignal::SemaphoreSignal(const VkSemaphore& semaphore,
+                                 const std::uint64_t& value)
+    : semaphore(semaphore), value(value) {}
+
+SemaphoreSignal::~SemaphoreSignal() {}
+
+void SemaphoreSignal::signal(const DeviceContext& device) const {
+
+    auto current = std::uint64_t{};
+    THROW_NOT_VKSUCCESS(device.vtable.GetSemaphoreCounterValue(
+        device.device, this->semaphore, &current));
+
+    // Don't signal if it has already been signalled.
+    if (current >= this->value) {
+        return;
+    }
+
+    const auto ssi =
+        VkSemaphoreSignalInfo{.sType = VK_STRUCTURE_TYPE_SEMAPHORE_SIGNAL_INFO,
+                              .semaphore = this->semaphore,
+                              .value = this->value};
+    THROW_NOT_VKSUCCESS(device.vtable.SignalSemaphore(device.device, &ssi));
+}
+
+} // namespace low_latency
\ No newline at end of file
diff --git a/src/strategies/low_latency2/semaphore_signal.hh b/src/strategies/low_latency2/semaphore_signal.hh
new file mode 100644
index 0000000..e1e1439
--- /dev/null
+++ b/src/strategies/low_latency2/semaphore_signal.hh
@@ -0,0 +1,41 @@
+#ifndef SEMAPHORE_SIGNAL_HH_
+#define SEMAPHORE_SIGNAL_HH_
+
+#include "device_context.hh"
+
+#include <cstdint>
+#include <vulkan/vulkan.h>
+
+// The VK_NV_low_latency2 extension supports a monotonically increasing
+// semaphore value. Monotonically increasing != strictly increasing. We have to
+// support a sequence with repeating values like 0, 1, 1, 1, 1, 2, 3. Vulkan
+// timeline semaphores do NOT support signalling <= its current value. While the
+// frame pacing isn't going to do anything in the case of repeating values (we
+// expect a global frame counter), it can happen in the case of swapchain
+// recreation or incomplete frames. This tiny class wraps a timeline semaphore
+// and associated value. It double checks that we haven't already signalled the
+// value.
+
+// TODO we _might_ want to make it so the destructor calls .signal(). This
+// would make it impossible to drop semaphores and cause hangs. However,
+// there are only a few places this can happen and I want to keep things
+// explicit for now.
+
+namespace low_latency {
+
+class SemaphoreSignal {
+  private:
+    const VkSemaphore semaphore{};
+    const std::uint64_t value{};
+
+  public:
+    SemaphoreSignal(const VkSemaphore& semaphore, const std::uint64_t& value);
+    ~SemaphoreSignal();
+
+  public:
+    void signal(const DeviceContext& device_context) const;
+};
+
+}; // namespace low_latency
+
+#endif
diff --git a/src/strategies/low_latency2/swapchain_monitor.cc b/src/strategies/low_latency2/swapchain_monitor.cc
index 6f55dd6..0dcd1ce 100644
--- a/src/strategies/low_latency2/swapchain_monitor.cc
+++ b/src/strategies/low_latency2/swapchain_monitor.cc
@@ -1,6 +1,5 @@
 #include "swapchain_monitor.hh"
 #include "device_context.hh"
-#include "helper.hh"
 
 #include <functional>
 
@@ -12,16 +11,6 @@ SwapchainMonitor::SwapchainMonitor(const DeviceContext& device)
 
 SwapchainMonitor::~SwapchainMonitor() {}
 
-void SwapchainMonitor::WakeupSemaphore::signal(
-    const DeviceContext& device) const {
-
-    const auto ssi =
-        VkSemaphoreSignalInfo{.sType = VK_STRUCTURE_TYPE_SEMAPHORE_SIGNAL_INFO,
-                              .semaphore = this->timeline_semaphore,
-                              .value = this->value};
-    THROW_NOT_VKSUCCESS(device.vtable.SignalSemaphore(device.device, &ssi));
-}
-
 void SwapchainMonitor::update_params(const bool was_low_latency_requested,
                                      const std::chrono::microseconds delay) {
 
@@ -49,7 +38,7 @@ void SwapchainMonitor::do_monitor(const std::stop_token stoken) {
         // If we're stopping, signal the semaphore and don't worry about work
         // actually completing. But we MUST drain them, or we get a hang.
         if (stoken.stop_requested()) {
-            pending_signal.wakeup_semaphore.signal(this->device);
+            pending_signal.semaphore_signal.signal(this->device);
             continue;
         }
 
@@ -73,21 +62,18 @@ void SwapchainMonitor::do_monitor(const std::stop_token stoken) {
         }
 
         this->last_signal_time.set(std::chrono::steady_clock::now());
-        pending_signal.wakeup_semaphore.signal(this->device);
+        pending_signal.semaphore_signal.signal(this->device);
     }
 }
 
-void SwapchainMonitor::notify_semaphore(const VkSemaphore& timeline_semaphore,
-                                        const std::uint64_t& value) {
+void SwapchainMonitor::notify_semaphore(
+    const SemaphoreSignal& semaphore_signal) {
 
     auto lock = std::unique_lock{this->mutex};
 
-    const auto wakeup_semaphore = WakeupSemaphore{
-        .timeline_semaphore = timeline_semaphore, .value = value};
-
     // Signal immediately if reflex is off or it's a no-op submit.
     if (!this->was_low_latency_requested) {
-        wakeup_semaphore.signal(this->device);
+        semaphore_signal.signal(this->device);
         return;
     }
 
@@ -103,13 +89,13 @@ void SwapchainMonitor::notify_semaphore(const VkSemaphore& timeline_semaphore,
                                 return frame_span->has_completed();
                             })) {
 
-        wakeup_semaphore.signal(this->device);
+        semaphore_signal.signal(this->device);
         this->pending_frame_spans.clear();
         return;
     }
 
     this->pending_signals.emplace_back(PendingSignal{
-        .wakeup_semaphore = wakeup_semaphore,
+        .semaphore_signal = semaphore_signal,
         .frame_spans = std::move(this->pending_frame_spans),
     });
     this->pending_frame_spans.clear();
diff --git a/src/strategies/low_latency2/swapchain_monitor.hh b/src/strategies/low_latency2/swapchain_monitor.hh
index ddc25ef..5906ad1 100644
--- a/src/strategies/low_latency2/swapchain_monitor.hh
+++ b/src/strategies/low_latency2/swapchain_monitor.hh
@@ -4,6 +4,7 @@
 
 #include "atomic_time_point.hh"
 #include "frame_span.hh"
+#include "semaphore_signal.hh"
 
 #include <vulkan/vulkan.h>
 
@@ -19,18 +20,10 @@ class DeviceContext;
 
 class SwapchainMonitor final {
   private:
-    struct WakeupSemaphore {
-        VkSemaphore timeline_semaphore{};
-        std::uint64_t value{};
-
-      public:
-        void signal(const DeviceContext& device) const;
-    };
-
     std::vector<std::unique_ptr<FrameSpan>> pending_frame_spans{};
 
     struct PendingSignal {
-        WakeupSemaphore wakeup_semaphore{};
+        SemaphoreSignal semaphore_signal;
         std::vector<std::unique_ptr<FrameSpan>> frame_spans{};
     };
     std::deque<PendingSignal> pending_signals{};
@@ -60,8 +53,7 @@ class SwapchainMonitor final {
     void update_params(const bool was_low_latency_requested,
                        const std::chrono::microseconds delay);
 
-    void notify_semaphore(const VkSemaphore& timeline_semaphore,
-                          const std::uint64_t& value);
+    void notify_semaphore(const SemaphoreSignal& semaphore_signal);
 
     void attach_work(std::vector<std::unique_ptr<FrameSpan>> submissions);
 };