From d64594ec7476782a4254502dd0e36b8f1f49e381 Mon Sep 17 00:00:00 2001 From: riperiperi Date: Fri, 29 Apr 2022 22:34:11 +0100 Subject: [PATCH 1/3] Fix various issues with texture sync (#3302) * Fix various issues with texture sync A variable called _actionRegistered is used to keep track of whether a tracking action has been registered for a given texture group handle. This variable is set when the action is registered, and should be unset when it is consumed. This is used to skip registering the tracking action if it's already registered, saving some time for render targets that are modified very often. There were two issues with this. The worst issue was that the tracking action handler exits early if the handle's modified flag is false... which means that it never reset _actionRegistered, as that was done within the Sync() method called later. The second issue was that this variable was set true after the sync action was registered, so it was technically possible for the action to run immediately, set the flag to false, then set it to true. Both situations would lead to the action never being registered again, as the texture group handle would be sure the action is already registered. This breaks the texture for the remaining runtime, or until it is disposed. It was also possible for a texture to register sync once, then on future frames the last modified sync number did not update. This may have caused some more minor issues. Seems to fix the Xenoblade flashing bug. Obviously this needs a lot of testing, since it was random chance. I typically had the most luck getting it to happen by switching time of day on the event theatre screen for a while, then entering the equipment screen by pressing X on an event. May also fix weird things like random chance air swimming in BOTW, maybe a few texture streaming bugs. * Exchange rather than CompareExchange --- Ryujinx.Graphics.Gpu/Image/TextureGroup.cs | 6 ++++ .../Image/TextureGroupHandle.cs | 35 +++++++++++++------ Ryujinx.Memory/Tracking/RegionHandle.cs | 7 ++-- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/Ryujinx.Graphics.Gpu/Image/TextureGroup.cs b/Ryujinx.Graphics.Gpu/Image/TextureGroup.cs index be0abea21..4bdc50788 100644 --- a/Ryujinx.Graphics.Gpu/Image/TextureGroup.cs +++ b/Ryujinx.Graphics.Gpu/Image/TextureGroup.cs @@ -1393,6 +1393,12 @@ namespace Ryujinx.Graphics.Gpu.Image /// The size of the flushing memory access public void FlushAction(TextureGroupHandle handle, ulong address, ulong size) { + // There is a small gap here where the action is removed but _actionRegistered is still 1. + // In this case it will skip registering the action, but here we are already handling it, + // so there shouldn't be any issue as it's the same handler for all actions. + + handle.ClearActionRegistered(); + if (!handle.Modified) { return; diff --git a/Ryujinx.Graphics.Gpu/Image/TextureGroupHandle.cs b/Ryujinx.Graphics.Gpu/Image/TextureGroupHandle.cs index 7d4aec411..34b59cffe 100644 --- a/Ryujinx.Graphics.Gpu/Image/TextureGroupHandle.cs +++ b/Ryujinx.Graphics.Gpu/Image/TextureGroupHandle.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; namespace Ryujinx.Graphics.Gpu.Image { @@ -32,9 +33,9 @@ namespace Ryujinx.Graphics.Gpu.Image private ulong _modifiedSync; /// - /// Whether a tracking action is currently registered or not. + /// Whether a tracking action is currently registered or not. (0/1) /// - private bool _actionRegistered; + private int _actionRegistered; /// /// Whether a sync action is currently registered or not. @@ -171,11 +172,9 @@ namespace Ryujinx.Graphics.Gpu.Image _syncActionRegistered = true; } - if (!_actionRegistered) + if (Interlocked.Exchange(ref _actionRegistered, 1) == 0) { _group.RegisterAction(this); - - _actionRegistered = true; } } @@ -233,8 +232,6 @@ namespace Ryujinx.Graphics.Gpu.Image /// The GPU context used to wait for sync public void Sync(GpuContext context) { - _actionRegistered = false; - bool needsSync = !context.IsGpuThread(); if (needsSync) @@ -263,21 +260,39 @@ namespace Ryujinx.Graphics.Gpu.Image } } + /// + /// Clears the action registered variable, indicating that the tracking action should be + /// re-registered on the next modification. + /// + public void ClearActionRegistered() + { + Interlocked.Exchange(ref _actionRegistered, 0); + } + /// /// Action to perform when a sync number is registered after modification. /// This action will register a read tracking action on the memory tracking handle so that a flush from CPU can happen. /// private void SyncAction() { + // The storage will need to signal modified again to update the sync number in future. + _group.Storage.SignalModifiedDirty(); + + lock (Overlaps) + { + foreach (Texture texture in Overlaps) + { + texture.SignalModifiedDirty(); + } + } + // Register region tracking for CPU? (again) _registeredSync = _modifiedSync; _syncActionRegistered = false; - if (!_actionRegistered) + if (Interlocked.Exchange(ref _actionRegistered, 1) == 0) { _group.RegisterAction(this); - - _actionRegistered = true; } } diff --git a/Ryujinx.Memory/Tracking/RegionHandle.cs b/Ryujinx.Memory/Tracking/RegionHandle.cs index 5ecd53a2b..b30dcbc25 100644 --- a/Ryujinx.Memory/Tracking/RegionHandle.cs +++ b/Ryujinx.Memory/Tracking/RegionHandle.cs @@ -144,9 +144,9 @@ namespace Ryujinx.Memory.Tracking { lock (_preActionLock) { - _preAction?.Invoke(address, size); + RegionSignal action = Interlocked.Exchange(ref _preAction, null); - _preAction = null; + action?.Invoke(address, size); } } finally @@ -252,8 +252,7 @@ namespace Ryujinx.Memory.Tracking lock (_preActionLock) { - RegionSignal lastAction = _preAction; - _preAction = action; + RegionSignal lastAction = Interlocked.Exchange(ref _preAction, action); if (lastAction == null && action != lastAction) { From 9eb5b7a10de596f9817859b6f383968a54899924 Mon Sep 17 00:00:00 2001 From: gdkchan Date: Sun, 1 May 2022 11:12:34 -0300 Subject: [PATCH 2/3] Restrict cases where vertex buffer size from index buffer type is used (#3304) --- Ryujinx.Graphics.Gpu/Engine/Threed/StateUpdater.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Ryujinx.Graphics.Gpu/Engine/Threed/StateUpdater.cs b/Ryujinx.Graphics.Gpu/Engine/Threed/StateUpdater.cs index c9a18f144..3bc15a317 100644 --- a/Ryujinx.Graphics.Gpu/Engine/Threed/StateUpdater.cs +++ b/Ryujinx.Graphics.Gpu/Engine/Threed/StateUpdater.cs @@ -928,7 +928,7 @@ namespace Ryujinx.Graphics.Gpu.Engine.Threed size = endAddress.Pack() - address + 1; - if (stride > 0 && indexTypeSmall) + if (stride > 0 && indexTypeSmall && _drawState.DrawIndexed && !instanced) { // If the index type is a small integer type, then we might be still able // to reduce the vertex buffer size based on the maximum possible index value. From 4a892fbdc9059504358ddf41c27576032e1ce414 Mon Sep 17 00:00:00 2001 From: riperiperi Date: Mon, 2 May 2022 11:31:53 +0100 Subject: [PATCH 3/3] Fix flush action from multiple threads regression (#3311) If two or more threads encounter a region of memory where a read action has been registered, then they must _both_ wait on the data. Clearing the action before it completed was causing the null check above to fail, so the action would only be run on the first thread, and the second would end up continuing without waiting. Depending on what the game does, this could be disasterous. This fixes a regression introduced by #3302 with Pokemon Legends Arceus, and possibly Catherine. There are likely other affected games. What is fixed in that PR should still be fixed. --- Ryujinx.Memory/Tracking/RegionHandle.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Ryujinx.Memory/Tracking/RegionHandle.cs b/Ryujinx.Memory/Tracking/RegionHandle.cs index b30dcbc25..14c6de2c9 100644 --- a/Ryujinx.Memory/Tracking/RegionHandle.cs +++ b/Ryujinx.Memory/Tracking/RegionHandle.cs @@ -144,9 +144,11 @@ namespace Ryujinx.Memory.Tracking { lock (_preActionLock) { - RegionSignal action = Interlocked.Exchange(ref _preAction, null); + _preAction?.Invoke(address, size); - action?.Invoke(address, size); + // The action is removed after it returns, to ensure that the null check above succeeds when + // it's still in progress rather than continuing and possibly missing a required data flush. + Interlocked.Exchange(ref _preAction, null); } } finally