From 0d174cbd4555e69660a900577a52f3d4bf7391b9 Mon Sep 17 00:00:00 2001 From: gdkchan Date: Tue, 19 Oct 2021 17:25:32 -0300 Subject: [PATCH] EventWait should not signal the event when it returns Success (#2739) * Fix race when EventWait is called and a wait is done on the CPU * This is useless now * Fix EventSignal * Ensure the signal belongs to the current fence, to avoid stale signals --- .../Synchronization/SynchronizationManager.cs | 4 +-- .../Synchronization/Syncpoint.cs | 8 +++--- .../Synchronization/SyncpointWaiterHandle.cs | 2 +- .../NvHostCtrl/NvHostCtrlDeviceFile.cs | 21 +++----------- .../NvHostCtrl/Types/NvHostEvent.cs | 28 +++++++++++++------ .../Services/SurfaceFlinger/SurfaceFlinger.cs | 2 +- .../SurfaceFlinger/Types/AndroidFence.cs | 5 ++-- 7 files changed, 35 insertions(+), 35 deletions(-) diff --git a/Ryujinx.Graphics.Gpu/Synchronization/SynchronizationManager.cs b/Ryujinx.Graphics.Gpu/Synchronization/SynchronizationManager.cs index d246f4747..968de930d 100644 --- a/Ryujinx.Graphics.Gpu/Synchronization/SynchronizationManager.cs +++ b/Ryujinx.Graphics.Gpu/Synchronization/SynchronizationManager.cs @@ -70,7 +70,7 @@ namespace Ryujinx.Graphics.Gpu.Synchronization /// The callback to call when the threshold is reached /// Thrown when id >= MaxHardwareSyncpoints /// The created SyncpointWaiterHandle object or null if already past threshold - public SyncpointWaiterHandle RegisterCallbackOnSyncpoint(uint id, uint threshold, Action callback) + public SyncpointWaiterHandle RegisterCallbackOnSyncpoint(uint id, uint threshold, Action callback) { if (id >= MaxHardwareSyncpoints) { @@ -120,7 +120,7 @@ namespace Ryujinx.Graphics.Gpu.Synchronization using (ManualResetEvent waitEvent = new ManualResetEvent(false)) { - var info = _syncpoints[id].RegisterCallback(threshold, () => waitEvent.Set()); + var info = _syncpoints[id].RegisterCallback(threshold, (x) => waitEvent.Set()); if (info == null) { diff --git a/Ryujinx.Graphics.Gpu/Synchronization/Syncpoint.cs b/Ryujinx.Graphics.Gpu/Synchronization/Syncpoint.cs index 30f8cc86d..39fb83c05 100644 --- a/Ryujinx.Graphics.Gpu/Synchronization/Syncpoint.cs +++ b/Ryujinx.Graphics.Gpu/Synchronization/Syncpoint.cs @@ -34,13 +34,13 @@ namespace Ryujinx.Graphics.Gpu.Synchronization /// The target threshold /// The callback to call when the threshold is reached /// The created SyncpointWaiterHandle object or null if already past threshold - public SyncpointWaiterHandle RegisterCallback(uint threshold, Action callback) + public SyncpointWaiterHandle RegisterCallback(uint threshold, Action callback) { lock (_waiters) { if (Value >= threshold) { - callback(); + callback(null); return null; } @@ -111,13 +111,13 @@ namespace Ryujinx.Graphics.Gpu.Synchronization // we can't call it inside the lock. if (expired != null) { - expired.Callback(); + expired.Callback(expired); if (expiredList != null) { for (int i = 0; i < expiredList.Count; i++) { - expiredList[i].Callback(); + expiredList[i].Callback(expiredList[i]); } } } diff --git a/Ryujinx.Graphics.Gpu/Synchronization/SyncpointWaiterHandle.cs b/Ryujinx.Graphics.Gpu/Synchronization/SyncpointWaiterHandle.cs index 28ce343e4..027b5141f 100644 --- a/Ryujinx.Graphics.Gpu/Synchronization/SyncpointWaiterHandle.cs +++ b/Ryujinx.Graphics.Gpu/Synchronization/SyncpointWaiterHandle.cs @@ -5,6 +5,6 @@ namespace Ryujinx.Graphics.Gpu.Synchronization public class SyncpointWaiterHandle { internal uint Threshold; - internal Action Callback; + internal Action Callback; } } diff --git a/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/NvHostCtrlDeviceFile.cs b/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/NvHostCtrlDeviceFile.cs index f0e5634e6..9af1ce7d9 100644 --- a/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/NvHostCtrlDeviceFile.cs +++ b/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/NvHostCtrlDeviceFile.cs @@ -314,24 +314,11 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl return NvInternalResult.InvalidInput; } - lock (hostEvent.Lock) - { + hostEvent.Cancel(_device.Gpu); - NvHostEventState oldState = hostEvent.State; + _device.System.HostSyncpoint.UpdateMin(hostEvent.Fence.Id); - if (oldState == NvHostEventState.Waiting) - { - hostEvent.State = NvHostEventState.Cancelling; - - hostEvent.Cancel(_device.Gpu); - } - - hostEvent.State = NvHostEventState.Cancelled; - - _device.System.HostSyncpoint.UpdateMin(hostEvent.Fence.Id); - - return NvInternalResult.Success; - } + return NvInternalResult.Success; } } @@ -486,7 +473,7 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl if (Event != null) { if (Event.State == NvHostEventState.Available || - Event.State == NvHostEventState.Signaled || + Event.State == NvHostEventState.Signaled || Event.State == NvHostEventState.Cancelled) { eventIndex = index; diff --git a/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/Types/NvHostEvent.cs b/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/Types/NvHostEvent.cs index 76e1564bd..f57a4eff9 100644 --- a/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/Types/NvHostEvent.cs +++ b/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/Types/NvHostEvent.cs @@ -68,10 +68,17 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl } } - private void GpuSignaled() + private void GpuSignaled(SyncpointWaiterHandle waiterInformation) { lock (Lock) { + // If the signal does not match our current waiter, + // then it is from a past fence and we should just ignore it. + if (waiterInformation != null && waiterInformation != _waiterInformation) + { + return; + } + ResetFailingState(); Signal(); @@ -82,9 +89,14 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl { lock (Lock) { - if (_waiterInformation != null) + NvHostEventState oldState = State; + + State = NvHostEventState.Cancelling; + + if (oldState == NvHostEventState.Waiting && _waiterInformation != null) { gpuContext.Synchronization.UnregisterCallback(Fence.Id, _waiterInformation); + _waiterInformation = null; if (_previousFailingFence.Id == Fence.Id && _previousFailingFence.Value == Fence.Value) { @@ -96,10 +108,10 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl _previousFailingFence = Fence; } - - Signal(); } + State = NvHostEventState.Cancelled; + Event.WritableEvent.Clear(); } } @@ -108,9 +120,6 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl { lock (Lock) { - Fence = fence; - State = NvHostEventState.Waiting; - // NOTE: nvservices code should always wait on the GPU side. // If we do this, we may get an abort or undefined behaviour when the GPU processing thread is blocked for a long period (for example, during shader compilation). // The reason for this is that the NVN code will try to wait until giving up. @@ -123,12 +132,15 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl bool timedOut = Fence.Wait(gpuContext, Timeout.InfiniteTimeSpan); - GpuSignaled(); + ResetFailingState(); return timedOut; } else { + Fence = fence; + State = NvHostEventState.Waiting; + _waiterInformation = gpuContext.Synchronization.RegisterCallbackOnSyncpoint(Fence.Id, Fence.Value, GpuSignaled); return true; diff --git a/Ryujinx.HLE/HOS/Services/SurfaceFlinger/SurfaceFlinger.cs b/Ryujinx.HLE/HOS/Services/SurfaceFlinger/SurfaceFlinger.cs index 39aebde53..c45c4b9da 100644 --- a/Ryujinx.HLE/HOS/Services/SurfaceFlinger/SurfaceFlinger.cs +++ b/Ryujinx.HLE/HOS/Services/SurfaceFlinger/SurfaceFlinger.cs @@ -392,7 +392,7 @@ namespace Ryujinx.HLE.HOS.Services.SurfaceFlinger } else { - item.Fence.RegisterCallback(_device.Gpu, () => + item.Fence.RegisterCallback(_device.Gpu, (x) => { _device.Gpu.Window.SignalFrameReady(); _device.Gpu.GPFifo.Interrupt(); diff --git a/Ryujinx.HLE/HOS/Services/SurfaceFlinger/Types/AndroidFence.cs b/Ryujinx.HLE/HOS/Services/SurfaceFlinger/Types/AndroidFence.cs index 515efd053..5b72e257f 100644 --- a/Ryujinx.HLE/HOS/Services/SurfaceFlinger/Types/AndroidFence.cs +++ b/Ryujinx.HLE/HOS/Services/SurfaceFlinger/Types/AndroidFence.cs @@ -1,5 +1,6 @@ using Ryujinx.Common.Logging; using Ryujinx.Graphics.Gpu; +using Ryujinx.Graphics.Gpu.Synchronization; using Ryujinx.HLE.HOS.Services.Nv.Types; using System; using System.Runtime.CompilerServices; @@ -66,7 +67,7 @@ namespace Ryujinx.HLE.HOS.Services.SurfaceFlinger return false; } - public void RegisterCallback(GpuContext gpuContext, Action callback) + public void RegisterCallback(GpuContext gpuContext, Action callback) { ref NvFence fence = ref NvFences[FenceCount - 1]; @@ -76,7 +77,7 @@ namespace Ryujinx.HLE.HOS.Services.SurfaceFlinger } else { - callback(); + callback(null); } }