From 764891e670718026c89c452d9a603a36fcf31539 Mon Sep 17 00:00:00 2001 From: Thog Date: Sat, 2 May 2020 22:47:06 +0200 Subject: [PATCH] nvservice: add a lock around NvHostEvent and remove release fence on SFv2 (#1197) * nvservice: add a lock to NvHostEvent * Disable surface flinger release fence and readd infinite timeout * FenceAction: Add a timeout of 1 seconds as this shouldn't wait forever anyuway * surfaceflinger: remove leftovers from the release fence * Don't allow infinite timeout on syncpoint while printing all timeout for better debugging --- .../Synchronization/SynchronizationManager.cs | 9 +- .../NvHostCtrl/NvHostCtrlDeviceFile.cs | 314 ++++++++++-------- .../NvHostCtrl/Types/NvHostEvent.cs | 105 +++--- .../Services/SurfaceFlinger/SurfaceFlinger.cs | 20 +- 4 files changed, 237 insertions(+), 211 deletions(-) diff --git a/Ryujinx.Graphics.Gpu/Synchronization/SynchronizationManager.cs b/Ryujinx.Graphics.Gpu/Synchronization/SynchronizationManager.cs index 2c9f383a4..494b0c030 100644 --- a/Ryujinx.Graphics.Gpu/Synchronization/SynchronizationManager.cs +++ b/Ryujinx.Graphics.Gpu/Synchronization/SynchronizationManager.cs @@ -112,14 +112,10 @@ namespace Ryujinx.Graphics.Gpu.Synchronization throw new ArgumentOutOfRangeException(nameof(id)); } - bool warnAboutTimeout = false; - // TODO: Remove this when GPU channel scheduling will be implemented. if (timeout == Timeout.InfiniteTimeSpan) { timeout = TimeSpan.FromSeconds(1); - - warnAboutTimeout = true; } using (ManualResetEvent waitEvent = new ManualResetEvent(false)) @@ -135,10 +131,7 @@ namespace Ryujinx.Graphics.Gpu.Synchronization if (!signaled && info != null) { - if (warnAboutTimeout) - { - Logger.PrintError(LogClass.Gpu, $"Wait on syncpoint {id} for threshold {threshold} took more than {timeout.TotalMilliseconds}ms, resuming execution..."); - } + Logger.PrintError(LogClass.Gpu, $"Wait on syncpoint {id} for threshold {threshold} took more than {timeout.TotalMilliseconds}ms, resuming execution..."); _syncpoints[id].UnregisterCallback(info); } diff --git a/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/NvHostCtrlDeviceFile.cs b/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/NvHostCtrlDeviceFile.cs index 585127834..0035401a1 100644 --- a/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/NvHostCtrlDeviceFile.cs +++ b/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/NvHostCtrlDeviceFile.cs @@ -95,26 +95,29 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl private KEvent QueryEvent(uint eventId) { - uint eventSlot; - uint syncpointId; - - if ((eventId >> 28) == 1) + lock (_events) { - eventSlot = eventId & 0xFFFF; - syncpointId = (eventId >> 16) & 0xFFF; - } - else - { - eventSlot = eventId & 0xFF; - syncpointId = eventId >> 4; - } + uint eventSlot; + uint syncpointId; - if (eventSlot >= EventsCount || _events[eventSlot] == null || _events[eventSlot].Fence.Id != syncpointId) - { - return null; - } + if ((eventId >> 28) == 1) + { + eventSlot = eventId & 0xFFFF; + syncpointId = (eventId >> 16) & 0xFFF; + } + else + { + eventSlot = eventId & 0xFF; + syncpointId = eventId >> 4; + } - return _events[eventSlot].Event; + if (eventSlot >= EventsCount || _events[eventSlot] == null || _events[eventSlot].Fence.Id != syncpointId) + { + return null; + } + + return _events[eventSlot].Event; + } } public override NvInternalResult QueryEvent(out int eventHandle, uint eventId) @@ -226,61 +229,71 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl private NvInternalResult EventRegister(ref uint userEventId) { - NvInternalResult result = EventUnregister(ref userEventId); - - if (result == NvInternalResult.Success) + lock (_events) { - _events[userEventId] = new NvHostEvent(_device.System.HostSyncpoint, userEventId, _device.System); + NvInternalResult result = EventUnregister(ref userEventId); + + if (result == NvInternalResult.Success) + { + _events[userEventId] = new NvHostEvent(_device.System.HostSyncpoint, userEventId, _device.System); + } + + return result; } - return result; } private NvInternalResult EventUnregister(ref uint userEventId) { - if (userEventId >= EventsCount) + lock (_events) { - return NvInternalResult.InvalidInput; + if (userEventId >= EventsCount) + { + return NvInternalResult.InvalidInput; + } + + NvHostEvent hostEvent = _events[userEventId]; + + if (hostEvent == null) + { + return NvInternalResult.Success; + } + + if (hostEvent.State == NvHostEventState.Available || + hostEvent.State == NvHostEventState.Cancelled || + hostEvent.State == NvHostEventState.Signaled) + { + _events[userEventId].Dispose(); + _events[userEventId] = null; + + return NvInternalResult.Success; + } + + return NvInternalResult.Busy; } - - NvHostEvent hostEvent = _events[userEventId]; - - if (hostEvent == null) - { - return NvInternalResult.Success; - } - - if (hostEvent.State == NvHostEventState.Available || - hostEvent.State == NvHostEventState.Cancelled || - hostEvent.State == NvHostEventState.Signaled) - { - _events[userEventId].Dispose(); - _events[userEventId] = null; - - return NvInternalResult.Success; - } - - return NvInternalResult.Busy; } private NvInternalResult EventKill(ref ulong eventMask) { - NvInternalResult result = NvInternalResult.Success; - - for (uint eventId = 0; eventId < EventsCount; eventId++) + lock (_events) { - if ((eventMask & (1UL << (int)eventId)) != 0) - { - NvInternalResult tmp = EventUnregister(ref eventId); + NvInternalResult result = NvInternalResult.Success; - if (tmp != NvInternalResult.Success) + for (uint eventId = 0; eventId < EventsCount; eventId++) + { + if ((eventMask & (1UL << (int)eventId)) != 0) { - result = tmp; + NvInternalResult tmp = EventUnregister(ref eventId); + + if (tmp != NvInternalResult.Success) + { + result = tmp; + } } } - } - return result; + return result; + } } private NvInternalResult EventSignal(ref uint userEventId) @@ -292,27 +305,34 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl return NvInternalResult.InvalidInput; } - NvHostEvent hostEvent = _events[eventId]; - - if (hostEvent == null) + lock (_events) { - return NvInternalResult.InvalidInput; + NvHostEvent hostEvent = _events[eventId]; + + if (hostEvent == null) + { + return NvInternalResult.InvalidInput; + } + + lock (hostEvent.Lock) + { + + NvHostEventState oldState = hostEvent.State; + + 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; + } } - - NvHostEventState oldState = hostEvent.State; - - 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; } private NvInternalResult SyncptReadMinOrMax(ref NvFence arguments, bool max) @@ -379,67 +399,81 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl uint eventIndex; - if (isWaitEventAsyncCmd) + lock (_events) { - eventIndex = value; - - if (eventIndex >= EventsCount) + if (isWaitEventAsyncCmd) { - return NvInternalResult.InvalidInput; - } + eventIndex = value; - hostEvent = _events[eventIndex]; - } - else - { - hostEvent = GetFreeEvent(fence.Id, out eventIndex); - } - - if (hostEvent != null && - (hostEvent.State == NvHostEventState.Available || - hostEvent.State == NvHostEventState.Signaled || - hostEvent.State == NvHostEventState.Cancelled)) - { - bool timedOut = hostEvent.Wait(_device.Gpu, fence); - - if (timedOut) - { - if (isWaitEventCmd) + if (eventIndex >= EventsCount) { - value = ((fence.Id & 0xfff) << 16) | 0x10000000; - } - else - { - value = fence.Id << 4; + return NvInternalResult.InvalidInput; } - value |= eventIndex; - - result = NvInternalResult.TryAgain; + hostEvent = _events[eventIndex]; } else { - value = fence.Value; - - return NvInternalResult.Success; + hostEvent = GetFreeEventLocked(fence.Id, out eventIndex); } - } - else - { - Logger.PrintError(LogClass.ServiceNv, $"Invalid Event at index {eventIndex} (isWaitEventAsyncCmd: {isWaitEventAsyncCmd}, isWaitEventCmd: {isWaitEventCmd})"); if (hostEvent != null) { - Logger.PrintError(LogClass.ServiceNv, hostEvent.DumpState(_device.Gpu)); - } + lock (hostEvent.Lock) + { + if (hostEvent.State == NvHostEventState.Available || + hostEvent.State == NvHostEventState.Signaled || + hostEvent.State == NvHostEventState.Cancelled) + { + bool timedOut = hostEvent.Wait(_device.Gpu, fence); - result = NvInternalResult.InvalidInput; + if (timedOut) + { + if (isWaitEventCmd) + { + value = ((fence.Id & 0xfff) << 16) | 0x10000000; + } + else + { + value = fence.Id << 4; + } + + value |= eventIndex; + + result = NvInternalResult.TryAgain; + } + else + { + value = fence.Value; + + return NvInternalResult.Success; + } + } + else + { + Logger.PrintError(LogClass.ServiceNv, $"Invalid Event at index {eventIndex} (isWaitEventAsyncCmd: {isWaitEventAsyncCmd}, isWaitEventCmd: {isWaitEventCmd})"); + + if (hostEvent != null) + { + Logger.PrintError(LogClass.ServiceNv, hostEvent.DumpState(_device.Gpu)); + } + + result = NvInternalResult.InvalidInput; + } + } + } + else + { + Logger.PrintError(LogClass.ServiceNv, $"Invalid Event at index {eventIndex} (isWaitEventAsyncCmd: {isWaitEventAsyncCmd}, isWaitEventCmd: {isWaitEventCmd})"); + + result = NvInternalResult.InvalidInput; + } } return result; } - public NvHostEvent GetFreeEvent(uint id, out uint eventIndex) + private NvHostEvent GetFreeEventLocked(uint id, out uint eventIndex) { eventIndex = EventsCount; @@ -490,38 +524,44 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl { Logger.PrintWarning(LogClass.ServiceNv, "Closing channel"); - // If the device file need to be closed, cancel all user events and dispose events. - for (int i = 0; i < _events.Length; i++) + lock (_events) { - NvHostEvent evnt = _events[i]; - - if (evnt != null) + // If the device file need to be closed, cancel all user events and dispose events. + for (int i = 0; i < _events.Length; i++) { - if (evnt.State == NvHostEventState.Waiting) - { - evnt.State = NvHostEventState.Cancelling; + NvHostEvent evnt = _events[i]; - evnt.Cancel(_device.Gpu); - } - else if (evnt.State == NvHostEventState.Signaling) + if (evnt != null) { - // Wait at max 9ms if the guest app is trying to signal the event while closing it.. - int retryCount = 0; - do + lock (evnt.Lock) { - if (retryCount++ > 9) + if (evnt.State == NvHostEventState.Waiting) { - break; + evnt.State = NvHostEventState.Cancelling; + + evnt.Cancel(_device.Gpu); + } + else if (evnt.State == NvHostEventState.Signaling) + { + // Wait at max 9ms if the guest app is trying to signal the event while closing it.. + int retryCount = 0; + do + { + if (retryCount++ > 9) + { + break; + } + + // TODO: This should be handled by the kernel (reschedule the current thread ect), waiting for Kernel decoupling work. + Thread.Sleep(1); + } while (evnt.State != NvHostEventState.Signaled); } - // TODO: This should be handled by the kernel (reschedule the current thread ect), waiting for Kernel decoupling work. - Thread.Sleep(1); - } while (evnt.State != NvHostEventState.Signaled); + evnt.Dispose(); + + _events[i] = null; + } } - - evnt.Dispose(); - - _events[i] = null; } } } 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 f62df8b3f..03df77b16 100644 --- a/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/Types/NvHostEvent.cs +++ b/Ryujinx.HLE/HOS/Services/Nv/NvDrvServices/NvHostCtrl/Types/NvHostEvent.cs @@ -21,6 +21,8 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl private NvFence _previousFailingFence; private uint _failingCount; + public object Lock = new object(); + /// /// Max failing count until waiting on CPU. /// FIXME: This seems enough for most of the cases, reduce if needed. @@ -49,83 +51,88 @@ namespace Ryujinx.HLE.HOS.Services.Nv.NvDrvServices.NvHostCtrl _failingCount = 0; } - public void Reset() - { - Fence.Id = 0; - Fence.Value = 0; - State = NvHostEventState.Available; - } - private void Signal() { - NvHostEventState oldState = State; - - State = NvHostEventState.Signaling; - - if (oldState == NvHostEventState.Waiting) + lock (Lock) { - Event.WritableEvent.Signal(); - } + NvHostEventState oldState = State; - State = NvHostEventState.Signaled; + State = NvHostEventState.Signaling; + + if (oldState == NvHostEventState.Waiting) + { + Event.WritableEvent.Signal(); + } + + State = NvHostEventState.Signaled; + } } private void GpuSignaled() { - ResetFailingState(); + lock (Lock) + { + ResetFailingState(); - Signal(); + Signal(); + } } public void Cancel(GpuContext gpuContext) { - if (_waiterInformation != null) + lock (Lock) { - gpuContext.Synchronization.UnregisterCallback(Fence.Id, _waiterInformation); - - if (_previousFailingFence.Id == Fence.Id && _previousFailingFence.Value == Fence.Value) + if (_waiterInformation != null) { - _failingCount++; - } - else - { - _failingCount = 1; + gpuContext.Synchronization.UnregisterCallback(Fence.Id, _waiterInformation); - _previousFailingFence = Fence; + if (_previousFailingFence.Id == Fence.Id && _previousFailingFence.Value == Fence.Value) + { + _failingCount++; + } + else + { + _failingCount = 1; + + _previousFailingFence = Fence; + } + + Signal(); } - Signal(); + Event.WritableEvent.Clear(); } - - Event.WritableEvent.Clear(); } public bool Wait(GpuContext gpuContext, NvFence fence) { - 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. - // This is done by trying to wait and signal multiple times until aborting after you are past the timeout. - // As such, if it fails too many time, we enforce a wait on the CPU side indefinitely. - // This allows to keep GPU and CPU in sync when we are slow. - if (_failingCount == FailingCountMax) + lock (Lock) { - Logger.PrintWarning(LogClass.ServiceNv, "GPU processing thread is too slow, waiting on CPU..."); + Fence = fence; + State = NvHostEventState.Waiting; - bool timedOut = Fence.Wait(gpuContext, Timeout.InfiniteTimeSpan); + // 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. + // This is done by trying to wait and signal multiple times until aborting after you are past the timeout. + // As such, if it fails too many time, we enforce a wait on the CPU side indefinitely. + // This allows to keep GPU and CPU in sync when we are slow. + if (_failingCount == FailingCountMax) + { + Logger.PrintWarning(LogClass.ServiceNv, "GPU processing thread is too slow, waiting on CPU..."); - GpuSignaled(); + bool timedOut = Fence.Wait(gpuContext, Timeout.InfiniteTimeSpan); - return timedOut; - } - else - { - _waiterInformation = gpuContext.Synchronization.RegisterCallbackOnSyncpoint(Fence.Id, Fence.Value, GpuSignaled); + GpuSignaled(); - return true; + return timedOut; + } + else + { + _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 12ebc7d7f..784f8c466 100644 --- a/Ryujinx.HLE/HOS/Services/SurfaceFlinger/SurfaceFlinger.cs +++ b/Ryujinx.HLE/HOS/Services/SurfaceFlinger/SurfaceFlinger.cs @@ -26,8 +26,6 @@ namespace Ryujinx.HLE.HOS.Services.SurfaceFlinger private Stopwatch _chrono; - private AndroidFence _vblankFence; - private long _ticks; private long _ticksPerFrame; @@ -49,7 +47,6 @@ namespace Ryujinx.HLE.HOS.Services.SurfaceFlinger { public Layer Layer; public BufferItem Item; - public AndroidFence Fence; } public SurfaceFlinger(Switch device) @@ -69,13 +66,6 @@ namespace Ryujinx.HLE.HOS.Services.SurfaceFlinger UpdateSwapInterval(1); - _vblankFence = AndroidFence.NoFence; - _vblankFence.AddFence(new NvFence - { - Id = NvHostSyncpt.VBlank0SyncpointId, - Value = 0 - }); - _composerThread.Start(); } @@ -222,8 +212,6 @@ namespace Ryujinx.HLE.HOS.Services.SurfaceFlinger { lock (Lock) { - _vblankFence.NvFences[0].Increment(_device.Gpu); - // TODO: support multilayers (& multidisplay ?) if (_layers.Count == 0) { @@ -298,14 +286,10 @@ namespace Ryujinx.HLE.HOS.Services.SurfaceFlinger flipX, flipY); - // Enforce that dequeueBuffer wait for the next vblank - _vblankFence.NvFences[0].Value++; - TextureCallbackInformation textureCallbackInformation = new TextureCallbackInformation { Layer = layer, Item = item, - Fence = _vblankFence }; _device.Gpu.Window.EnqueueFrameThreadSafe( @@ -330,7 +314,9 @@ namespace Ryujinx.HLE.HOS.Services.SurfaceFlinger private void ReleaseBuffer(TextureCallbackInformation information) { - information.Layer.Consumer.ReleaseBuffer(information.Item, ref information.Fence); + AndroidFence fence = AndroidFence.NoFence; + + information.Layer.Consumer.ReleaseBuffer(information.Item, ref fence); } private void AcquireBuffer(GpuContext ignored, object obj)