From 36f10df775cf0c678548b97346432095823dfd8a Mon Sep 17 00:00:00 2001 From: riperiperi Date: Mon, 1 May 2023 19:32:32 +0100 Subject: [PATCH] GPU: Fix errors handling texture remapping (#4745) * GPU: Fix errors handling texture remapping - Fixes an error where a pool entry and memory mapping changing at the same time could cause a texture to rebind its data from the wrong GPU VA (data swaps) - Fixes an error where the texture pool could act on a mapping change before the mapping has actually been changed ("Unmapped" event happens before change, we need to signal it changed _after_ it completes) TODO: remove textures from partially mapped list... if they aren't. * Add Remap actions for handling post-mapping behaviours * Remove unused code. * Address feedback * Nit --- src/Ryujinx.Graphics.Gpu/Image/Texture.cs | 4 +- .../Image/TextureCache.cs | 79 ++++++++++++++++--- src/Ryujinx.Graphics.Gpu/Image/TexturePool.cs | 10 ++- .../Memory/MemoryManager.cs | 26 +++++- .../Memory/UnmapEventArgs.cs | 12 ++- src/Ryujinx.Memory/Range/MemoryRange.cs | 14 ++++ src/Ryujinx.Memory/Range/MultiRange.cs | 9 +++ 7 files changed, 135 insertions(+), 19 deletions(-) diff --git a/src/Ryujinx.Graphics.Gpu/Image/Texture.cs b/src/Ryujinx.Graphics.Gpu/Image/Texture.cs index 84808a84d..f0df55e68 100644 --- a/src/Ryujinx.Graphics.Gpu/Image/Texture.cs +++ b/src/Ryujinx.Graphics.Gpu/Image/Texture.cs @@ -1610,6 +1610,8 @@ namespace Ryujinx.Graphics.Gpu.Image /// public void UpdatePoolMappings() { + ChangedMapping = true; + lock (_poolOwners) { ulong address = 0; @@ -1685,8 +1687,6 @@ namespace Ryujinx.Graphics.Gpu.Image { Group.ClearModified(unmapRange); } - - UpdatePoolMappings(); } /// diff --git a/src/Ryujinx.Graphics.Gpu/Image/TextureCache.cs b/src/Ryujinx.Graphics.Gpu/Image/TextureCache.cs index c3243cf23..97d78a349 100644 --- a/src/Ryujinx.Graphics.Gpu/Image/TextureCache.cs +++ b/src/Ryujinx.Graphics.Gpu/Image/TextureCache.cs @@ -64,7 +64,7 @@ namespace Ryujinx.Graphics.Gpu.Image } /// - /// Handles removal of textures written to a memory region being unmapped. + /// Handles marking of textures written to a memory region being (partially) remapped. /// /// Sender object /// Event arguments @@ -80,26 +80,41 @@ namespace Ryujinx.Graphics.Gpu.Image overlapCount = _textures.FindOverlaps(unmapped, ref overlaps); } - for (int i = 0; i < overlapCount; i++) + if (overlapCount > 0) { - overlaps[i].Unmapped(unmapped); + for (int i = 0; i < overlapCount; i++) + { + overlaps[i].Unmapped(unmapped); + } } - // If any range was previously unmapped, we also need to purge - // all partially mapped texture, as they might be fully mapped now. - for (int i = 0; i < unmapped.Count; i++) + lock (_partiallyMappedTextures) { - if (unmapped.GetSubRange(i).Address == MemoryManager.PteUnmapped) + if (overlapCount > 0 || _partiallyMappedTextures.Count > 0) { - lock (_partiallyMappedTextures) + e.AddRemapAction(() => { - foreach (var texture in _partiallyMappedTextures) + lock (_partiallyMappedTextures) { - texture.Unmapped(unmapped); - } - } + if (overlapCount > 0) + { + for (int i = 0; i < overlapCount; i++) + { + _partiallyMappedTextures.Add(overlaps[i]); + } + } - break; + // Any texture that has been unmapped at any point or is partially unmapped + // should update their pool references after the remap completes. + + MultiRange unmapped = ((MemoryManager)sender).GetPhysicalRegions(e.Address, e.Size); + + foreach (var texture in _partiallyMappedTextures) + { + texture.UpdatePoolMappings(); + } + } + }); } } } @@ -1135,6 +1150,44 @@ namespace Ryujinx.Graphics.Gpu.Image } } + /// + /// Queries a texture's memory range and marks it as partially mapped or not. + /// Partially mapped textures re-evaluate their memory range after each time GPU memory is mapped. + /// + /// GPU memory manager where the texture is mapped + /// The virtual address of the texture + /// The texture to be marked + /// The physical regions for the texture, found when evaluating whether the texture was partially mapped + public MultiRange UpdatePartiallyMapped(MemoryManager memoryManager, ulong address, Texture texture) + { + MultiRange range; + lock (_partiallyMappedTextures) + { + range = memoryManager.GetPhysicalRegions(address, texture.Size); + bool partiallyMapped = false; + + for (int i = 0; i < range.Count; i++) + { + if (range.GetSubRange(i).Address == MemoryManager.PteUnmapped) + { + partiallyMapped = true; + break; + } + } + + if (partiallyMapped) + { + _partiallyMappedTextures.Add(texture); + } + else + { + _partiallyMappedTextures.Remove(texture); + } + } + + return range; + } + /// /// Adds a texture to the short duration cache. This typically keeps it alive for two ticks. /// diff --git a/src/Ryujinx.Graphics.Gpu/Image/TexturePool.cs b/src/Ryujinx.Graphics.Gpu/Image/TexturePool.cs index 5277e7899..dbcb2e75e 100644 --- a/src/Ryujinx.Graphics.Gpu/Image/TexturePool.cs +++ b/src/Ryujinx.Graphics.Gpu/Image/TexturePool.cs @@ -272,7 +272,15 @@ namespace Ryujinx.Graphics.Gpu.Image ulong address = descriptor.UnpackAddress(); - MultiRange range = _channel.MemoryManager.GetPhysicalRegions(address, texture.Size); + if (!descriptor.Equals(ref DescriptorCache[request.ID])) + { + // If the pool entry has already been replaced, just remove the texture. + + texture.DecrementReferenceCount(); + continue; + } + + MultiRange range = _channel.MemoryManager.Physical.TextureCache.UpdatePartiallyMapped(_channel.MemoryManager, address, texture); // If the texture is not mapped at all, delete its reference. diff --git a/src/Ryujinx.Graphics.Gpu/Memory/MemoryManager.cs b/src/Ryujinx.Graphics.Gpu/Memory/MemoryManager.cs index b0f7e7992..0d4a41f02 100644 --- a/src/Ryujinx.Graphics.Gpu/Memory/MemoryManager.cs +++ b/src/Ryujinx.Graphics.Gpu/Memory/MemoryManager.cs @@ -365,6 +365,22 @@ namespace Ryujinx.Graphics.Gpu.Memory } } + /// + /// Runs remap actions that are added to an unmap event. + /// These must run after the mapping completes. + /// + /// Event with remap actions + private void RunRemapActions(UnmapEventArgs e) + { + if (e.RemapActions != null) + { + foreach (Action action in e.RemapActions) + { + action(); + } + } + } + /// /// Maps a given range of pages to the specified CPU virtual address. /// @@ -379,12 +395,15 @@ namespace Ryujinx.Graphics.Gpu.Memory { lock (_pageTable) { - MemoryUnmapped?.Invoke(this, new UnmapEventArgs(va, size)); + UnmapEventArgs e = new(va, size); + MemoryUnmapped?.Invoke(this, e); for (ulong offset = 0; offset < size; offset += PageSize) { SetPte(va + offset, PackPte(pa + offset, kind)); } + + RunRemapActions(e); } } @@ -398,12 +417,15 @@ namespace Ryujinx.Graphics.Gpu.Memory lock (_pageTable) { // Event handlers are not expected to be thread safe. - MemoryUnmapped?.Invoke(this, new UnmapEventArgs(va, size)); + UnmapEventArgs e = new(va, size); + MemoryUnmapped?.Invoke(this, e); for (ulong offset = 0; offset < size; offset += PageSize) { SetPte(va + offset, PteUnmapped); } + + RunRemapActions(e); } } diff --git a/src/Ryujinx.Graphics.Gpu/Memory/UnmapEventArgs.cs b/src/Ryujinx.Graphics.Gpu/Memory/UnmapEventArgs.cs index 305747f85..837b5aab4 100644 --- a/src/Ryujinx.Graphics.Gpu/Memory/UnmapEventArgs.cs +++ b/src/Ryujinx.Graphics.Gpu/Memory/UnmapEventArgs.cs @@ -1,14 +1,24 @@ -namespace Ryujinx.Graphics.Gpu.Memory +using System; +using System.Collections.Generic; + +namespace Ryujinx.Graphics.Gpu.Memory { public class UnmapEventArgs { public ulong Address { get; } public ulong Size { get; } + public List RemapActions { get; private set; } public UnmapEventArgs(ulong address, ulong size) { Address = address; Size = size; } + + public void AddRemapAction(Action action) + { + RemapActions ??= new List(); + RemapActions.Add(action); + } } } diff --git a/src/Ryujinx.Memory/Range/MemoryRange.cs b/src/Ryujinx.Memory/Range/MemoryRange.cs index 7465fbcb3..c7ee6db22 100644 --- a/src/Ryujinx.Memory/Range/MemoryRange.cs +++ b/src/Ryujinx.Memory/Range/MemoryRange.cs @@ -57,5 +57,19 @@ return thisAddress < otherEndAddress && otherAddress < thisEndAddress; } + + /// + /// Returns a string summary of the memory range. + /// + /// A string summary of the memory range + public override string ToString() + { + if (Address == ulong.MaxValue) + { + return $"[Unmapped 0x{Size:X}]"; + } + + return $"[0x{Address:X}, 0x{EndAddress:X})"; + } } } diff --git a/src/Ryujinx.Memory/Range/MultiRange.cs b/src/Ryujinx.Memory/Range/MultiRange.cs index 9dbd76eca..42ef24be6 100644 --- a/src/Ryujinx.Memory/Range/MultiRange.cs +++ b/src/Ryujinx.Memory/Range/MultiRange.cs @@ -319,5 +319,14 @@ namespace Ryujinx.Memory.Range return hash.ToHashCode(); } + + /// + /// Returns a string summary of the ranges contained in the MultiRange. + /// + /// A string summary of the ranges contained within + public override string ToString() + { + return HasSingleRange ? _singleRange.ToString() : string.Join(", ", _ranges); + } } }