From 8d36681bf1eb732307086203f3bbd2509f55c234 Mon Sep 17 00:00:00 2001 From: riperiperi Date: Sat, 6 Mar 2021 14:43:55 +0000 Subject: [PATCH] Improve handling for unmapped GPU resources (#2083) * Improve handling for unmapped GPU resources - Fixed a memory tracking bug that would set protection on empty PTEs - When a texture's memory is (partially) unmapped, all pool references are forcibly removed and the texture must be rediscovered to draw with it. This will also force the texture discovery to always compare the texture's range for a match. - RegionHandles now know if they are unmapped, and automatically unset their dirty flag when unmapped. - Partial texture sync now loads only the region of texture that has been modified. Unmapped memory tracking handles cause dirty flags for a texture group handle to be ignored. This greatly improves the emulator's stability for newer UE4 games. * Address feedback, fix MultiRange slice Fixed an issue where the size of the multi-range slice would be miscalculated. * Update Ryujinx.Memory/Range/MultiRange.cs (feedback) Co-authored-by: Mary Co-authored-by: Mary --- Ryujinx.Cpu/MemoryManager.cs | 2 +- Ryujinx.Cpu/Tracking/CpuRegionHandle.cs | 1 + Ryujinx.Graphics.Gpu/Image/Pool.cs | 2 +- Ryujinx.Graphics.Gpu/Image/Texture.cs | 76 +++++++++++++++++++- Ryujinx.Graphics.Gpu/Image/TextureGroup.cs | 13 ++-- Ryujinx.Graphics.Gpu/Image/TextureManager.cs | 4 +- Ryujinx.Graphics.Gpu/Image/TexturePool.cs | 52 +++++++++++++- Ryujinx.Memory/Range/MultiRange.cs | 48 +++++++++++++ Ryujinx.Memory/Tracking/MemoryTracking.cs | 9 +++ Ryujinx.Memory/Tracking/RegionHandle.cs | 25 ++++++- Ryujinx.Memory/Tracking/VirtualRegion.cs | 12 ++++ 11 files changed, 229 insertions(+), 15 deletions(-) diff --git a/Ryujinx.Cpu/MemoryManager.cs b/Ryujinx.Cpu/MemoryManager.cs index 83c288ae9..59654e8bb 100644 --- a/Ryujinx.Cpu/MemoryManager.cs +++ b/Ryujinx.Cpu/MemoryManager.cs @@ -627,7 +627,7 @@ namespace Ryujinx.Cpu { pte = Volatile.Read(ref pageRef); } - while (Interlocked.CompareExchange(ref pageRef, (pte & invTagMask) | tag, pte) != pte); + while (pte != 0 && Interlocked.CompareExchange(ref pageRef, (pte & invTagMask) | tag, pte) != pte); pageStart++; } diff --git a/Ryujinx.Cpu/Tracking/CpuRegionHandle.cs b/Ryujinx.Cpu/Tracking/CpuRegionHandle.cs index f4391aad4..6a530b0e7 100644 --- a/Ryujinx.Cpu/Tracking/CpuRegionHandle.cs +++ b/Ryujinx.Cpu/Tracking/CpuRegionHandle.cs @@ -8,6 +8,7 @@ namespace Ryujinx.Cpu.Tracking private readonly RegionHandle _impl; public bool Dirty => _impl.Dirty; + public bool Unmapped => _impl.Unmapped; public ulong Address => _impl.Address; public ulong Size => _impl.Size; public ulong EndAddress => _impl.EndAddress; diff --git a/Ryujinx.Graphics.Gpu/Image/Pool.cs b/Ryujinx.Graphics.Gpu/Image/Pool.cs index c5aef77f6..855f63443 100644 --- a/Ryujinx.Graphics.Gpu/Image/Pool.cs +++ b/Ryujinx.Graphics.Gpu/Image/Pool.cs @@ -117,7 +117,7 @@ namespace Ryujinx.Graphics.Gpu.Image /// Performs the disposal of all resources stored on the pool. /// It's an error to try using the pool after disposal. /// - public void Dispose() + public virtual void Dispose() { if (Items != null) { diff --git a/Ryujinx.Graphics.Gpu/Image/Texture.cs b/Ryujinx.Graphics.Gpu/Image/Texture.cs index 4d4091cb1..26e102a93 100644 --- a/Ryujinx.Graphics.Gpu/Image/Texture.cs +++ b/Ryujinx.Graphics.Gpu/Image/Texture.cs @@ -21,6 +21,12 @@ namespace Ryujinx.Graphics.Gpu.Image // This method uses much more memory so we want to avoid it if possible. private const int ByteComparisonSwitchThreshold = 4; + private struct TexturePoolOwner + { + public TexturePool Pool; + public int ID; + } + private GpuContext _context; private SizeInfo _sizeInfo; @@ -64,7 +70,13 @@ namespace Ryujinx.Graphics.Gpu.Image /// Set when a texture has been changed size. This indicates that it may need to be /// changed again when obtained as a sampler. /// - public bool ChangedSize { get; internal set; } + public bool ChangedSize { get; private set; } + + /// + /// Set when a texture's GPU VA has ever been partially or fully unmapped. + /// This indicates that the range must be fully checked when matching the texture. + /// + public bool ChangedMapping { get; private set; } private int _depth; private int _layers; @@ -121,6 +133,7 @@ namespace Ryujinx.Graphics.Gpu.Image public bool IsView => _viewStorage != this; private int _referenceCount; + private List _poolOwners; /// /// Constructs a new instance of the cached GPU texture. @@ -190,6 +203,7 @@ namespace Ryujinx.Graphics.Gpu.Image _viewStorage = this; _views = new List(); + _poolOwners = new List(); } /// @@ -1218,6 +1232,20 @@ namespace Ryujinx.Graphics.Gpu.Image _referenceCount++; } + /// + /// Increments the reference count and records the given texture pool and ID as a pool owner. + /// + /// The texture pool this texture has been added to + /// The ID of the reference to this texture in the pool + public void IncrementReferenceCount(TexturePool pool, int id) + { + lock (_poolOwners) + { + _poolOwners.Add(new TexturePoolOwner { Pool = pool, ID = id }); + } + _referenceCount++; + } + /// /// Decrements the texture reference count. /// When the reference count hits zero, the texture may be deleted and can't be used anymore. @@ -1244,6 +1272,48 @@ namespace Ryujinx.Graphics.Gpu.Image return newRefCount <= 0; } + /// + /// Decrements the texture reference count, also removing an associated pool owner reference. + /// When the reference count hits zero, the texture may be deleted and can't be used anymore. + /// + /// The texture pool this texture is being removed from + /// The ID of the reference to this texture in the pool + /// True if the texture is now referenceless, false otherwise + public bool DecrementReferenceCount(TexturePool pool, int id = -1) + { + lock (_poolOwners) + { + int references = _poolOwners.RemoveAll(entry => entry.Pool == pool && entry.ID == id || id == -1); + + if (references == 0) + { + // This reference has already been removed. + return _referenceCount <= 0; + } + + Debug.Assert(references == 1); + } + + return DecrementReferenceCount(); + } + + /// + /// Forcibly remove this texture from all pools that reference it. + /// + /// Indicates if the removal is being done from another thread. + public void RemoveFromPools(bool deferred) + { + lock (_poolOwners) + { + foreach (var owner in _poolOwners) + { + owner.Pool.ForceRemove(this, owner.ID, deferred); + } + + _poolOwners.Clear(); + } + } + /// /// Delete the texture if it is not used anymore. /// The texture is considered unused when the reference count is zero, @@ -1282,7 +1352,11 @@ namespace Ryujinx.Graphics.Gpu.Image /// public void Unmapped() { + ChangedMapping = true; + IsModified = false; // We shouldn't flush this texture, as its memory is no longer mapped. + + RemoveFromPools(true); } /// diff --git a/Ryujinx.Graphics.Gpu/Image/TextureGroup.cs b/Ryujinx.Graphics.Gpu/Image/TextureGroup.cs index 5d150559f..39567a823 100644 --- a/Ryujinx.Graphics.Gpu/Image/TextureGroup.cs +++ b/Ryujinx.Graphics.Gpu/Image/TextureGroup.cs @@ -122,6 +122,7 @@ namespace Ryujinx.Graphics.Gpu.Image { bool dirty = false; bool anyModified = false; + bool anyUnmapped = false; for (int i = 0; i < regionCount; i++) { @@ -130,6 +131,7 @@ namespace Ryujinx.Graphics.Gpu.Image bool modified = group.Modified; bool handleDirty = false; bool handleModified = false; + bool handleUnmapped = false; foreach (CpuRegionHandle handle in group.Handles) { @@ -140,6 +142,7 @@ namespace Ryujinx.Graphics.Gpu.Image } else { + handleUnmapped |= handle.Unmapped; handleModified |= modified; } } @@ -159,18 +162,20 @@ namespace Ryujinx.Graphics.Gpu.Image dirty |= handleDirty; } + anyUnmapped |= handleUnmapped; + if (group.NeedsCopy) { // The texture we copied from is still being written to. Copy from it again the next time this texture is used. texture.SignalGroupDirty(); } - _loadNeeded[baseHandle + i] = handleDirty; + _loadNeeded[baseHandle + i] = handleDirty && !handleUnmapped; } if (dirty) { - if (_handles.Length > 1 && (anyModified || split)) + if (anyUnmapped || (_handles.Length > 1 && (anyModified || split))) { // Partial texture invalidation. Only update the layers/levels with dirty flags of the storage. @@ -194,8 +199,6 @@ namespace Ryujinx.Graphics.Gpu.Image /// The number of handles to synchronize private void SynchronizePartial(int baseHandle, int regionCount) { - ReadOnlySpan fullData = _context.PhysicalMemory.GetSpan(Storage.Range); - for (int i = 0; i < regionCount; i++) { if (_loadNeeded[baseHandle + i]) @@ -212,7 +215,7 @@ namespace Ryujinx.Graphics.Gpu.Image int endOffset = (offsetIndex + 1 == _allOffsets.Length) ? (int)Storage.Size : _allOffsets[offsetIndex + 1]; int size = endOffset - offset; - ReadOnlySpan data = fullData.Slice(offset, size); + ReadOnlySpan data = _context.PhysicalMemory.GetSpan(Storage.Range.GetSlice((ulong)offset, (ulong)size)); data = Storage.ConvertToHostCompatibleFormat(data, info.BaseLevel, true); diff --git a/Ryujinx.Graphics.Gpu/Image/TextureManager.cs b/Ryujinx.Graphics.Gpu/Image/TextureManager.cs index f13c3443d..07bc7c7e4 100644 --- a/Ryujinx.Graphics.Gpu/Image/TextureManager.cs +++ b/Ryujinx.Graphics.Gpu/Image/TextureManager.cs @@ -715,7 +715,9 @@ namespace Ryujinx.Graphics.Gpu.Image // we know the textures are located at the same memory region. // If they don't, it may still be mapped to the same physical region, so we // do a more expensive check to tell if they are mapped into the same physical regions. - if (overlap.Info.GpuAddress != info.GpuAddress && !_context.MemoryManager.CompareRange(overlap.Range, info.GpuAddress)) + // If the GPU VA for the texture has ever been unmapped, then the range must be checked regardless. + if ((overlap.Info.GpuAddress != info.GpuAddress || overlap.ChangedMapping) && + !_context.MemoryManager.CompareRange(overlap.Range, info.GpuAddress)) { continue; } diff --git a/Ryujinx.Graphics.Gpu/Image/TexturePool.cs b/Ryujinx.Graphics.Gpu/Image/TexturePool.cs index 58e881ca7..eece2a79e 100644 --- a/Ryujinx.Graphics.Gpu/Image/TexturePool.cs +++ b/Ryujinx.Graphics.Gpu/Image/TexturePool.cs @@ -2,6 +2,7 @@ using Ryujinx.Common.Logging; using Ryujinx.Graphics.GAL; using Ryujinx.Graphics.Texture; using System; +using System.Collections.Concurrent; using System.Collections.Generic; namespace Ryujinx.Graphics.Gpu.Image @@ -12,6 +13,7 @@ namespace Ryujinx.Graphics.Gpu.Image class TexturePool : Pool { private int _sequenceNumber; + private readonly ConcurrentQueue _dereferenceQueue = new ConcurrentQueue(); /// /// Intrusive linked list node used on the texture pool cache. @@ -53,6 +55,8 @@ namespace Ryujinx.Graphics.Gpu.Image TextureInfo info = GetInfo(descriptor, out int layerSize); + ProcessDereferenceQueue(); + texture = Context.Methods.TextureManager.FindOrCreateTexture(TextureSearchFlags.ForSampler, info, layerSize); // If this happens, then the texture address is invalid, we can't add it to the cache. @@ -61,7 +65,7 @@ namespace Ryujinx.Graphics.Gpu.Image return null; } - texture.IncrementReferenceCount(); + texture.IncrementReferenceCount(this, id); Items[id] = texture; @@ -92,6 +96,39 @@ namespace Ryujinx.Graphics.Gpu.Image return texture; } + /// + /// Forcibly remove a texture from this pool's items. + /// If deferred, the dereference will be queued to occur on the render thread. + /// + /// The texture being removed + /// The ID of the texture in this pool + /// If true, queue the dereference to happen on the render thread, otherwise dereference immediately + public void ForceRemove(Texture texture, int id, bool deferred) + { + Items[id] = null; + + if (deferred) + { + _dereferenceQueue.Enqueue(texture); + } + else + { + texture.DecrementReferenceCount(); + } + } + + /// + /// Process the dereference queue, decrementing the reference count for each texture in it. + /// This is used to ensure that texture disposal happens on the render thread. + /// + private void ProcessDereferenceQueue() + { + while (_dereferenceQueue.TryDequeue(out Texture toRemove)) + { + toRemove.DecrementReferenceCount(); + } + } + /// /// Implementation of the texture pool range invalidation. /// @@ -99,6 +136,8 @@ namespace Ryujinx.Graphics.Gpu.Image /// Size of the range being invalidated protected override void InvalidateRangeImpl(ulong address, ulong size) { + ProcessDereferenceQueue(); + ulong endAddress = address + size; for (; address < endAddress; address += DescriptorSize) @@ -118,7 +157,7 @@ namespace Ryujinx.Graphics.Gpu.Image continue; } - texture.DecrementReferenceCount(); + texture.DecrementReferenceCount(this, id); Items[id] = null; } @@ -342,7 +381,14 @@ namespace Ryujinx.Graphics.Gpu.Image /// The texture to be deleted protected override void Delete(Texture item) { - item?.DecrementReferenceCount(); + item?.DecrementReferenceCount(this); + } + + public override void Dispose() + { + ProcessDereferenceQueue(); + + base.Dispose(); } } } \ No newline at end of file diff --git a/Ryujinx.Memory/Range/MultiRange.cs b/Ryujinx.Memory/Range/MultiRange.cs index b40daa8aa..3d505c7ca 100644 --- a/Ryujinx.Memory/Range/MultiRange.cs +++ b/Ryujinx.Memory/Range/MultiRange.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; namespace Ryujinx.Memory.Range { @@ -75,6 +76,53 @@ namespace Ryujinx.Memory.Range } } + /// + /// Gets a slice of the multi-range. + /// + /// Offset of the slice into the multi-range in bytes + /// Size of the slice in bytes + /// A new multi-range representing the given slice of this one + public MultiRange GetSlice(ulong offset, ulong size) + { + if (HasSingleRange) + { + if (_singleRange.Size - offset < size) + { + throw new ArgumentOutOfRangeException(nameof(size)); + } + + return new MultiRange(_singleRange.Address + offset, size); + } + else + { + var ranges = new List(); + + foreach (MemoryRange range in _ranges) + { + if ((long)offset <= 0) + { + ranges.Add(new MemoryRange(range.Address, Math.Min(size, range.Size))); + size -= range.Size; + } + else if (offset < range.Size) + { + ulong sliceSize = Math.Min(size, range.Size - offset); + ranges.Add(new MemoryRange(range.Address + offset, sliceSize)); + size -= sliceSize; + } + + if ((long)size <= 0) + { + break; + } + + offset -= range.Size; + } + + return new MultiRange(ranges.ToArray()); + } + } + /// /// Gets the physical region at the specified index. /// diff --git a/Ryujinx.Memory/Tracking/MemoryTracking.cs b/Ryujinx.Memory/Tracking/MemoryTracking.cs index aff223e80..6485e566c 100644 --- a/Ryujinx.Memory/Tracking/MemoryTracking.cs +++ b/Ryujinx.Memory/Tracking/MemoryTracking.cs @@ -74,6 +74,14 @@ namespace Ryujinx.Memory.Tracking for (int i = 0; i < count; i++) { VirtualRegion region = results[i]; + + // If the region has been fully remapped, signal that it has been mapped again. + bool remapped = _memoryManager.IsRangeMapped(region.Address, region.Size); + if (remapped) + { + region.SignalMappingChanged(true); + } + region.RecalculatePhysicalChildren(); region.UpdateProtection(); } @@ -99,6 +107,7 @@ namespace Ryujinx.Memory.Tracking for (int i = 0; i < count; i++) { VirtualRegion region = results[i]; + region.SignalMappingChanged(false); region.RecalculatePhysicalChildren(); } } diff --git a/Ryujinx.Memory/Tracking/RegionHandle.cs b/Ryujinx.Memory/Tracking/RegionHandle.cs index 4da184dd7..da3ee99aa 100644 --- a/Ryujinx.Memory/Tracking/RegionHandle.cs +++ b/Ryujinx.Memory/Tracking/RegionHandle.cs @@ -12,6 +12,7 @@ namespace Ryujinx.Memory.Tracking public class RegionHandle : IRegionHandle, IRange { public bool Dirty { get; private set; } + public bool Unmapped { get; private set; } public ulong Address { get; } public ulong Size { get; } @@ -37,10 +38,11 @@ namespace Ryujinx.Memory.Tracking /// Tracking object for the target memory block /// Virtual address of the region to track /// Size of the region to track - /// Initial value of the dirty flag - internal RegionHandle(MemoryTracking tracking, ulong address, ulong size, bool dirty = true) + /// True if the region handle starts mapped + internal RegionHandle(MemoryTracking tracking, ulong address, ulong size, bool mapped = true) { - Dirty = dirty; + Dirty = mapped; + Unmapped = !mapped; Address = address; Size = size; EndAddress = address + size; @@ -128,6 +130,23 @@ namespace Ryujinx.Memory.Tracking _regions.Add(region); } + /// + /// Signal that this handle has been mapped or unmapped. + /// + /// True if the handle has been mapped, false if unmapped + internal void SignalMappingChanged(bool mapped) + { + if (Unmapped == mapped) + { + Unmapped = !mapped; + + if (Unmapped) + { + Dirty = false; + } + } + } + /// /// Check if this region overlaps with another. /// diff --git a/Ryujinx.Memory/Tracking/VirtualRegion.cs b/Ryujinx.Memory/Tracking/VirtualRegion.cs index 15a11568e..fcf2fbe06 100644 --- a/Ryujinx.Memory/Tracking/VirtualRegion.cs +++ b/Ryujinx.Memory/Tracking/VirtualRegion.cs @@ -66,6 +66,18 @@ namespace Ryujinx.Memory.Tracking UpdatePhysicalChildren(); } + /// + /// Signal that this region has been mapped or unmapped. + /// + /// True if the region has been mapped, false if unmapped + public void SignalMappingChanged(bool mapped) + { + foreach (RegionHandle handle in Handles) + { + handle.SignalMappingChanged(mapped); + } + } + /// /// Gets the strictest permission that the child handles demand. Assumes that the tracking lock has been obtained. ///