Gpu: Fix thread safety of ReregisterRanges (#3865)

A quick fix to prevent reading the wrong value of Count when reregistering ranges for a new target buffer. Buffer flushes from another thread can modify the range list when the lock isn't active, which can change the count.

This prevents some crashes in Pokemon Scarlet/Violet. It's probably likely that buffer migration during flush is causing some other issues in this game, but this at least prevents the crashing.

Ryujinx.Graphics.Gpu/GpuChannel.cs

@@ -67,6 +67,7 @@ namespace Ryujinx.Graphics.Gpu
 
             // Since the memory manager changed, make sure we will get pools from addresses of the new memory manager.
             TextureManager.ReloadPools();
+            MemoryManager.Physical.BufferCache.QueuePrune();
         }
 
         /// <summary>
@@ -77,6 +78,7 @@ namespace Ryujinx.Graphics.Gpu
         private void MemoryUnmappedHandler(object sender, UnmapEventArgs e)
         {
             TextureManager.ReloadPools();
+            MemoryManager.Physical.BufferCache.QueuePrune();
         }
 
         /// <summary>

Ryujinx.Graphics.Gpu/Memory/BufferCache.cs

@@ -28,6 +28,7 @@ namespace Ryujinx.Graphics.Gpu.Memory
 
         private readonly Dictionary<ulong, BufferCacheEntry> _dirtyCache;
         private readonly Dictionary<ulong, BufferCacheEntry> _modifiedCache;
+        private bool _pruneCaches;
 
         public event Action NotifyBuffersModified;
 
@@ -136,6 +137,11 @@ namespace Ryujinx.Graphics.Gpu.Memory
         /// <param name="size">Size in bytes of the buffer</param>
         public void ForceDirty(MemoryManager memoryManager, ulong gpuVa, ulong size)
         {
+            if (_pruneCaches)
+            {
+                Prune();
+            }
+
             if (!_dirtyCache.TryGetValue(gpuVa, out BufferCacheEntry result) ||
                 result.EndGpuAddress < gpuVa + size ||
                 result.UnmappedSequence != result.Buffer.UnmappedSequence)
@@ -158,17 +164,29 @@ namespace Ryujinx.Graphics.Gpu.Memory
         /// <returns>True if modified, false otherwise</returns>
         public bool CheckModified(MemoryManager memoryManager, ulong gpuVa, ulong size, out ulong outAddr)
         {
-            if (!_modifiedCache.TryGetValue(gpuVa, out BufferCacheEntry result) ||
-                result.EndGpuAddress < gpuVa + size ||
-                result.UnmappedSequence != result.Buffer.UnmappedSequence)
+            if (_pruneCaches)
             {
-                ulong address = TranslateAndCreateBuffer(memoryManager, gpuVa, size);
-                result = new BufferCacheEntry(address, gpuVa, GetBuffer(address, size));
-
-                _modifiedCache[gpuVa] = result;
+                Prune();
             }
 
-            outAddr = result.Address;
+            // Align the address to avoid creating too many entries on the quick lookup dictionary.
+            ulong mask = BufferAlignmentMask;
+            ulong alignedGpuVa = gpuVa & (~mask);
+            ulong alignedEndGpuVa = (gpuVa + size + mask) & (~mask);
+
+            size = alignedEndGpuVa - alignedGpuVa;
+
+            if (!_modifiedCache.TryGetValue(alignedGpuVa, out BufferCacheEntry result) ||
+                result.EndGpuAddress < alignedEndGpuVa ||
+                result.UnmappedSequence != result.Buffer.UnmappedSequence)
+            {
+                ulong address = TranslateAndCreateBuffer(memoryManager, alignedGpuVa, size);
+                result = new BufferCacheEntry(address, alignedGpuVa, GetBuffer(address, size));
+
+                _modifiedCache[alignedGpuVa] = result;
+            }
+
+            outAddr = result.Address | (gpuVa & mask);
 
             return result.Buffer.IsModified(result.Address, size);
         }
@@ -435,6 +453,54 @@ namespace Ryujinx.Graphics.Gpu.Memory
             }
         }
 
+        /// <summary>
+        /// Prune any invalid entries from a quick access dictionary.
+        /// </summary>
+        /// <param name="dictionary">Dictionary to prune</param>
+        /// <param name="toDelete">List used to track entries to delete</param>
+        private void Prune(Dictionary<ulong, BufferCacheEntry> dictionary, ref List<ulong> toDelete)
+        {
+            foreach (var entry in dictionary)
+            {
+                if (entry.Value.UnmappedSequence != entry.Value.Buffer.UnmappedSequence)
+                {
+                    (toDelete ??= new()).Add(entry.Key);
+                }
+            }
+
+            if (toDelete != null)
+            {
+                foreach (ulong entry in toDelete)
+                {
+                    dictionary.Remove(entry);
+                }
+            }
+        }
+
+        /// <summary>
+        /// Prune any invalid entries from the quick access dictionaries.
+        /// </summary>
+        private void Prune()
+        {
+            List<ulong> toDelete = null;
+
+            Prune(_dirtyCache, ref toDelete);
+
+            toDelete?.Clear();
+
+            Prune(_modifiedCache, ref toDelete);
+
+            _pruneCaches = false;
+        }
+
+        /// <summary>
+        /// Queues a prune of invalid entries the next time a dictionary cache is accessed.
+        /// </summary>
+        public void QueuePrune()
+        {
+            _pruneCaches = true;
+        }
+
         /// <summary>
         /// Disposes all buffers in the cache.
         /// It's an error to use the buffer manager after disposal.

Ryujinx.Graphics.Gpu/Memory/BufferModifiedRangeList.cs

@@ -325,13 +325,15 @@ namespace Ryujinx.Graphics.Gpu.Memory
         public void ReregisterRanges(Action<ulong, ulong> rangeAction)
         {
             ref var ranges = ref ThreadStaticArray<BufferModifiedRange>.Get();
+            int count;
 
             // Range list must be consistent for this operation.
             lock (_lock)
             {
-                if (ranges.Length < Count)
+                count = Count;
+                if (ranges.Length < count)
                 {
-                    Array.Resize(ref ranges, Count);
+                    Array.Resize(ref ranges, count);
                 }
 
                 int i = 0;
@@ -342,7 +344,7 @@ namespace Ryujinx.Graphics.Gpu.Memory
             }
 
             ulong currentSync = _context.SyncNumber;
-            for (int i = 0; i < Count; i++)
+            for (int i = 0; i < count; i++)
             {
                 BufferModifiedRange range = ranges[i];
                 if (range.SyncNumber != currentSync)

Ryujinx.Graphics.Vulkan/BufferHolder.cs

@@ -210,7 +210,10 @@ namespace Ryujinx.Graphics.Vulkan
                 }
             }
 
-            if (cbs != null && !(_buffer.HasCommandBufferDependency(cbs.Value) && _waitable.IsBufferRangeInUse(cbs.Value.CommandBufferIndex, offset, dataSize)))
+            if (cbs != null &&
+                _gd.PipelineInternal.RenderPassActive &&
+                !(_buffer.HasCommandBufferDependency(cbs.Value) &&
+                _waitable.IsBufferRangeInUse(cbs.Value.CommandBufferIndex, offset, dataSize)))
             {
                 // If the buffer hasn't been used on the command buffer yet, try to preload the data.
                 // This avoids ending and beginning render passes on each buffer data upload.

Ryujinx.Graphics.Vulkan/PipelineBase.cs

@@ -49,7 +49,6 @@ namespace Ryujinx.Graphics.Vulkan
         private Auto<DisposableFramebuffer> _framebuffer;
         private Auto<DisposableRenderPass> _renderPass;
         private int _writtenAttachmentCount;
-        private bool _renderPassActive;
 
         private readonly DescriptorSetUpdater _descriptorSetUpdater;
 
@@ -73,6 +72,7 @@ namespace Ryujinx.Graphics.Vulkan
         private PipelineColorBlendAttachmentState[] _storedBlend;
 
         public ulong DrawCount { get; private set; }
+        public bool RenderPassActive { get; private set; }
 
         public unsafe PipelineBase(VulkanRenderer gd, Device device)
         {
@@ -838,6 +838,11 @@ namespace Ryujinx.Graphics.Vulkan
             stages.CopyTo(_newState.Stages.AsSpan().Slice(0, stages.Length));
 
             SignalStateChange();
+
+            if (_program.IsCompute)
+            {
+                EndRenderPass();
+            }
         }
 
         public void Specialize<T>(in T data) where T : unmanaged
@@ -1451,7 +1456,7 @@ namespace Ryujinx.Graphics.Vulkan
 
         private unsafe void BeginRenderPass()
         {
-            if (!_renderPassActive)
+            if (!RenderPassActive)
             {
                 var renderArea = new Rect2D(null, new Extent2D(FramebufferParams.Width, FramebufferParams.Height));
                 var clearValue = new ClearValue();
@@ -1467,18 +1472,18 @@ namespace Ryujinx.Graphics.Vulkan
                 };
 
                 Gd.Api.CmdBeginRenderPass(CommandBuffer, renderPassBeginInfo, SubpassContents.Inline);
-                _renderPassActive = true;
+                RenderPassActive = true;
             }
         }
 
         public void EndRenderPass()
         {
-            if (_renderPassActive)
+            if (RenderPassActive)
             {
                 PauseTransformFeedbackInternal();
                 Gd.Api.CmdEndRenderPass(CommandBuffer);
                 SignalRenderPassEnd();
-                _renderPassActive = false;
+                RenderPassActive = false;
             }
         }
 

Ryujinx.Graphics.Vulkan/ShaderCollection.cs

@@ -19,6 +19,7 @@ namespace Ryujinx.Graphics.Vulkan
 
         public bool HasMinimalLayout { get; }
         public bool UsePushDescriptors { get; }
+        public bool IsCompute { get; }
 
         public uint Stages { get; }
 
@@ -47,7 +48,6 @@ namespace Ryujinx.Graphics.Vulkan
         private VulkanRenderer _gd;
         private Device _device;
         private bool _initialized;
-        private bool _isCompute;
 
         private ProgramPipelineState _state;
         private DisposableRenderPass _dummyRenderPass;
@@ -91,7 +91,7 @@ namespace Ryujinx.Graphics.Vulkan
 
                 if (shader.StageFlags == ShaderStageFlags.ShaderStageComputeBit)
                 {
-                    _isCompute = true;
+                    IsCompute = true;
                 }
 
                 internalShaders[i] = shader;
@@ -163,7 +163,7 @@ namespace Ryujinx.Graphics.Vulkan
 
             try
             {
-                if (_isCompute)
+                if (IsCompute)
                 {
                     CreateBackgroundComputePipeline();
                 }

Ryujinx.HLE/HOS/Services/Am/AppletOE/ApplicationProxyService/ApplicationProxy/IApplicationFunctions.cs

@@ -26,8 +26,8 @@ namespace Ryujinx.HLE.HOS.Services.Am.AppletOE.ApplicationProxyService.Applicati
 {
     class IApplicationFunctions : IpcService
     {
-        private ulong _defaultSaveDataSize        = 200000000;
-        private ulong _defaultJournalSaveDataSize = 200000000;
+        private long _defaultSaveDataSize        = 200000000;
+        private long _defaultJournalSaveDataSize = 200000000;
 
         private KEvent _gpuErrorDetectedSystemEvent;
         private KEvent _friendInvitationStorageChannelEvent;
@@ -203,13 +203,13 @@ namespace Ryujinx.HLE.HOS.Services.Am.AppletOE.ApplicationProxyService.Applicati
         }
 
         [CommandHipc(25)] // 3.0.0+
-        // ExtendSaveData(u8 save_data_type, nn::account::Uid, u64 save_size, u64 journal_size) -> u64 result_code
+        // ExtendSaveData(u8 save_data_type, nn::account::Uid, s64 save_size, s64 journal_size) -> u64 result_code
         public ResultCode ExtendSaveData(ServiceCtx context)
         {
             SaveDataType saveDataType = (SaveDataType)context.RequestData.ReadUInt64();
             Uid          userId       = context.RequestData.ReadStruct<Uid>();
-            ulong        saveDataSize = context.RequestData.ReadUInt64();
-            ulong        journalSize  = context.RequestData.ReadUInt64();
+            long        saveDataSize  = context.RequestData.ReadInt64();
+            long        journalSize   = context.RequestData.ReadInt64();
 
             // NOTE: Service calls nn::fs::ExtendApplicationSaveData.
             //       Since LibHac currently doesn't support this method, we can stub it for now.
@@ -225,7 +225,7 @@ namespace Ryujinx.HLE.HOS.Services.Am.AppletOE.ApplicationProxyService.Applicati
         }
 
         [CommandHipc(26)] // 3.0.0+
-        // GetSaveDataSize(u8 save_data_type, nn::account::Uid) -> (u64 save_size, u64 journal_size)
+        // GetSaveDataSize(u8 save_data_type, nn::account::Uid) -> (s64 save_size, s64 journal_size)
         public ResultCode GetSaveDataSize(ServiceCtx context)
         {
             SaveDataType saveDataType = (SaveDataType)context.RequestData.ReadUInt64();
@@ -268,6 +268,23 @@ namespace Ryujinx.HLE.HOS.Services.Am.AppletOE.ApplicationProxyService.Applicati
             return ResultCode.Success;
         }
 
+        [CommandHipc(28)] // 11.0.0+
+        // GetSaveDataSizeMax() -> (s64 save_size_max, s64 journal_size_max)
+        public ResultCode GetSaveDataSizeMax(ServiceCtx context)
+        {
+            // NOTE: We are currently using a stub for GetSaveDataSize() which returns the default values.
+            //       For this method we shouldn't return anything lower than that, but since we aren't interacting
+            //       with fs to get the actual sizes, we return the default values here as well.
+            //       This also helps in case ExtendSaveData() has been executed and the default values were modified.
+
+            context.ResponseData.Write(_defaultSaveDataSize);
+            context.ResponseData.Write(_defaultJournalSaveDataSize);
+
+            Logger.Stub?.PrintStub(LogClass.ServiceAm);
+
+            return ResultCode.Success;
+        }
+
         [CommandHipc(30)]
         // BeginBlockingHomeButtonShortAndLongPressed()
         public ResultCode BeginBlockingHomeButtonShortAndLongPressed(ServiceCtx context)