diff --git a/core/libs/prx/libSceAgcDriver/CMakeLists.txt b/core/libs/prx/libSceAgcDriver/CMakeLists.txt index f112af6adeae..2d2c3be94735 100644 --- a/core/libs/prx/libSceAgcDriver/CMakeLists.txt +++ b/core/libs/prx/libSceAgcDriver/CMakeLists.txt @@ -1086,6 +1086,12 @@ if(BUILD_TESTING) set_tests_properties(agc_unchanged_cpu_stamps PROPERTIES ENVIRONMENT_MODIFICATION "PATH=path_list_prepend:$") endif() + add_test(NAME agc_repeated_neighbour_stores COMMAND agc_driver_recorder_tests --repeated-neighbour-stores-only) + set_tests_properties(agc_repeated_neighbour_stores PROPERTIES SKIP_RETURN_CODE 77 TIMEOUT 30) + if(WIN32) + set_tests_properties(agc_repeated_neighbour_stores PROPERTIES ENVIRONMENT_MODIFICATION + "PATH=path_list_prepend:$") + endif() add_test(NAME agc_storage_kept_unit_settle COMMAND agc_driver_recorder_tests --kept-unit-settle-only) set_tests_properties(agc_storage_kept_unit_settle PROPERTIES SKIP_RETURN_CODE 77 TIMEOUT 30) if(WIN32) diff --git a/core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp b/core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp index d0859ab06105..23c2d4b10c96 100644 --- a/core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp +++ b/core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp @@ -881,7 +881,6 @@ struct WriteTracker { struct DriverPieces { std::uint32_t whole = 0; std::uint32_t dropped = 0; - std::uint32_t next = 0; std::array pieces{}; bool covers(std::uint32_t begin, std::uint32_t end, std::uint64_t generation) const { @@ -929,9 +928,22 @@ struct WriteTracker { return; } auto& entry = driverPieces[block]; - auto& slot = entry.pieces[entry.next++ % entry.pieces.size()]; - entry.dropped = std::max(entry.dropped, slot.generation); - slot = {stampGeneration, from, to}; + DriverPiece* covered = nullptr; + for (auto& piece : entry.pieces) { + if (piece.generation == 0 || piece.begin < from || piece.end > to) continue; + if (covered == nullptr) covered = &piece; + else piece = {}; + } + if (covered != nullptr) { + *covered = {stampGeneration, from, to}; + return; + } + auto* oldest = &entry.pieces.front(); + for (auto& piece : entry.pieces) { + if (piece.generation < oldest->generation) oldest = &piece; + } + entry.dropped = std::max(entry.dropped, oldest->generation); + *oldest = {stampGeneration, from, to}; } std::uint32_t stampOf(std::uint64_t block) const { diff --git a/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp b/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp index e132d8db4e78..cd1f30f46418 100644 --- a/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp +++ b/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp @@ -4780,6 +4780,117 @@ void refreshProofTests(const Device& device, Recorder& recorder) { } +void repeatedNeighbourStoreTests(const Device& device, Recorder& recorder) { + const auto& base = device.GetContext(); + constexpr std::uint32_t width = 1024; + constexpr std::uint32_t height = 512; + constexpr std::size_t surfaceBytes = std::size_t{width} * height * 4; + constexpr std::uint64_t offset = 0x2000; + constexpr std::size_t neighbourBytes = 0x6000; + constexpr std::size_t bytes = 35 * 65536; + constexpr std::array red{255, 0, 0, 255}; + const auto pattern = [](std::size_t i) { return static_cast(i * 131u + 7u); }; + for (const bool imported : {false, true}) { +#ifdef _WIN32 + if (imported) { + std::cout << "repeated neighbour stores over a watched host import: Linux write watch only\n"; + continue; + } +#endif + if (imported && base.hostImportAlignment == 0) { + std::cout << "host imports unavailable: repeated neighbour stores over a watched host import not tested\n"; + continue; + } + const auto decided = PrepareImportWatch(base); + if (imported) SetImportWatch(base, ImportWatch::Watch); + struct Restore { + const Context& context; + ImportWatch decided; + ~Restore() { SetImportWatch(context, decided); } + } restore{base, decided}; + const std::string what = imported ? "over a watched host import" : "over watched memory"; + void* block = AllocateWatched(bytes, 65536); + if (block == nullptr) { + std::cout << "no write watching: repeated neighbour stores not tested\n"; + return; + } + auto* memory = static_cast(block); + for (std::size_t i = 0; i < bytes; ++i) memory[i] = pattern(i); + const auto blockAddress = reinterpret_cast(block); + const auto address = blockAddress + offset; + { + GuestAllocations::Mutation mutation; + mutation.Add(block, bytes, true, true, true); + } + struct Unregister { + const Context& context; + void* block; + std::uint64_t address; + std::size_t bytes; + ~Unregister() { + { + GuestAllocations::Mutation mutation; + mutation.Remove(block); + } + HostImportFor(context, address, bytes); + ReleaseWatched(block, bytes); + } + } unregister{base, block, blockAddress, bytes}; + AgcDriver::GuestMemory::CollectWritesUncached(blockAddress, bytes); + TextureDetiler detiler(base); + auto context = base; + if (!imported) context.hostImportAlignment = 0; + context.detiler = &detiler; + GuestTextureResource resource{}; + resource.baseAddress = address; + resource.width = width; + resource.height = height; + resource.mipCount = 1; + resource.tileMode = TextureTileMode::kLinear; + resource.dimension = TextureDimension::k2D; + resource.format = 56; + resource.dstSelX = 4; + resource.dstSelY = 5; + resource.dstSelZ = 6; + resource.dstSelW = 7; + Require(DescribeSurface(resource).guestBytes == surfaceBytes, "(n) unexpected surface size"); + { + auto image = StorageTexture::Create(context, detiler, resource, 0); + image->Refresh(); + Require(image->Refresh(), "(n) " + what + ": the image does not settle after its upload"); + const VkImageSubresourceRange range{VK_IMAGE_ASPECT_COLOR_BIT, 0, 1, 0, 1}; + const auto commands = recorder.Commands(); + recorder.Keep(image); + const VkClearColorValue value{{1.0f, 0.0f, 0.0f, 1.0f}}; + RecordMemoryBarrier(context, commands, VK_PIPELINE_STAGE_ALL_COMMANDS_BIT, VK_PIPELINE_STAGE_TRANSFER_BIT, VK_ACCESS_SHADER_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT | VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT, VK_ACCESS_TRANSFER_WRITE_BIT); + context.Function("vkCmdClearColorImage")(commands, image->Image(), VK_IMAGE_LAYOUT_GENERAL, &value, 1, &range); + RecordMemoryBarrier(context, commands, VK_PIPELINE_STAGE_TRANSFER_BIT, VK_PIPELINE_STAGE_ALL_COMMANDS_BIT, VK_ACCESS_TRANSFER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT | VK_ACCESS_SHADER_WRITE_BIT | VK_ACCESS_TRANSFER_READ_BIT | VK_ACCESS_TRANSFER_WRITE_BIT); + image->MarkDirty(); + for (int store = 0; store < 5; ++store) AgcDriver::GuestMemory::MarkWritten(address + surfaceBytes, neighbourBytes); + Require(AgcDriver::GuestMemory::Watched(address, surfaceBytes) && (HostImportFor(context, address, surfaceBytes) != nullptr) == imported, "(n) " + what + ": the image is not on the path under test"); + image->WriteBack(); + recorder.Submit(); + device.WaitQueue(); + recorder.Sync(); + std::vector read(bytes); + AgcDriver::GuestMemory::Read(blockAddress, read); + std::size_t dropped = 0; + for (std::size_t at = offset; at < offset + surfaceBytes; at += 4) { + for (std::size_t c = 0; c < 4; ++c) { + if (std::to_integer(read[at + c]) != red[c]) { + ++dropped; + break; + } + } + } + if (dropped != 0) throw std::runtime_error("repeated neighbour stores " + what + ": " + std::to_string(dropped) + " of " + std::to_string(surfaceBytes / 4) + " texels the GPU wrote were not stored"); + for (std::size_t at = offset + surfaceBytes; at < offset + surfaceBytes + neighbourBytes; ++at) Require(std::to_integer(read[at]) == pattern(at), "(n) " + what + ": the write-back stored over the neighbour's bytes"); + } + recorder.Sync(); + } + std::cout << "repeated neighbour stores: ok\n"; +} + void unchangedCpuStampTests(const Device& device, Recorder& recorder) { const auto& base = device.GetContext(); constexpr std::uint32_t side = 256; @@ -8127,6 +8238,10 @@ int main(int argc, char** argv) { unchangedCpuStampTests(device, recorder); return 0; } + if (argc == 2 && std::string_view(argv[1]) == "--repeated-neighbour-stores-only") { + repeatedNeighbourStoreTests(device, recorder); + return 0; + } if (argc == 2 && std::string_view(argv[1]) == "--kept-unit-settle-only") { keptUnitSettleTests(device, recorder); return 0; @@ -8330,6 +8445,7 @@ int main(int argc, char** argv) { singlePassTests(device, recorder, false); singlePassTests(device, recorder, true); unchangedCpuStampTests(device, recorder); + repeatedNeighbourStoreTests(device, recorder); keptUnitSettleTests(device, recorder); settledBaselineTests(device, recorder); importWatchTests(device); diff --git a/core/libs/prx/libSceAgcDriver/tests/WriteTracking.cpp b/core/libs/prx/libSceAgcDriver/tests/WriteTracking.cpp index d69035db8c8b..3e738ced8121 100644 --- a/core/libs/prx/libSceAgcDriver/tests/WriteTracking.cpp +++ b/core/libs/prx/libSceAgcDriver/tests/WriteTracking.cpp @@ -101,6 +101,44 @@ void CheckSharedBlock() { if (!Watched(outside, unwatched.size())) Require(StoredOver(outside, unwatched.size(), TrackerGeneration()), "an unwatched range reads as not stored over"); } +void CheckRepeatedNeighbourStores() { + void* memory = AllocateWatched(3 * Block); + const auto base = reinterpret_cast(memory); + const auto boundary = base + 2 * Block - 0x9000; + const auto sharedFirst = boundary - (base + Block); + const auto neighbourBytes = static_cast(base + 3 * Block - boundary); + std::memset(memory, 0x11, 3 * Block); + + const auto synced = CollectWrites(base, 3 * Block); + Require(synced != 0, "the test block is not collected"); + for (int store = 0; store < 6; ++store) MarkWritten(boundary, neighbourBytes); + Require(!StoredOver(base + Block, static_cast(sharedFirst), synced), "repeated stores of the neighbour's bytes count for the first surface's part of the shared block"); + Require(StoredAllSince_nid_no_patch(boundary, neighbourBytes, synced), "repeated stores of the neighbour's bytes forget that they covered them"); + + const auto pieces = TrackerGeneration(); + for (int store = 0; store < 3; ++store) MarkWritten(boundary + static_cast(store) * 0x100, 4); + MarkWritten(boundary, 0x400); + Require(StoredOver(boundary + 0x200, 4, pieces) && !StoredOver(boundary + 0x400, 4, pieces), "a store over earlier pieces of the block answers for other bytes than its own"); + Require(!StoredOver(base + Block, static_cast(sharedFirst), pieces), "a store over earlier pieces counts for the first surface's part of the shared block"); + + void* fresh = AllocateWatched(2 * Block); + const auto freshBase = reinterpret_cast(fresh); + std::memset(fresh, 0x11, 2 * Block); + const auto freshSynced = CollectWrites(freshBase, 2 * Block); + const auto neighbour = freshBase + Block - 0x9000; + for (std::uint64_t store = 0; store < 4; ++store) MarkWritten(neighbour + store * 0x1000, 0x100); + const auto settled = TrackerGeneration(); + MarkWritten(neighbour, 0x100); + MarkWritten(neighbour + 0x5000, 0x100); + Require(!StoredOver(freshBase, static_cast(neighbour - freshBase), settled), "a full block forgets a store newer than its oldest"); + Require(StoredOver(freshBase, static_cast(neighbour - freshBase), freshSynced), "a forgotten store is not taken as a store over the rest of the block"); + + const auto partial = TrackerGeneration(); + MarkWritten(neighbour + 0x5080, 0x100); + Require(StoredOver(neighbour + 0x5000, 0x10, settled) && StoredOver(neighbour + 0x5100, 0x10, partial), "a store that overlaps an earlier piece without containing it replaces it"); + Require(!StoredOver(freshBase, static_cast(neighbour - freshBase), settled), "a full block forgets a store newer than its oldest"); +} + void CheckStoredAllSince() { constexpr std::size_t bytes = 4 * Block; auto* memory = static_cast(AllocateWatched(bytes)); @@ -931,6 +969,7 @@ int main() { } #endif CheckSharedBlock(); + CheckRepeatedNeighbourStores(); CheckStoredAllSince(); CheckOwnStore(); CheckUnwatch(); diff --git a/docs/dev/debt/libSceAgcDriver.md b/docs/dev/debt/libSceAgcDriver.md index 1bd073621410..f68713d9a009 100644 --- a/docs/dev/debt/libSceAgcDriver.md +++ b/docs/dev/debt/libSceAgcDriver.md @@ -74,6 +74,7 @@ - [Draw input snapshots](../../../core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp) (libSceAgcDriver): `PrepareDrawBindings` caps read-only input buffer snapshots at 64 MiB (overridable via `APS5_DRAW_SNAPSHOT_MAX_MIB`). Larger inputs (streaming heaps, bindless pools) or draws exceeding the cumulative budget skip the per-draw snapshot and bind in place, avoiding multi-gigabyte CPU copy stalls (such as Ratchet & Clank: Rift Apart PPSA01474 stalling at frame ~380 copying 36 GB every 5 s). If the title rewrites those large ranges on the CPU before GPU completion, the in-place binding observes the modification, as verified by `drawSnapshotCapTests`. - [Pending storage-image results](../../../core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp) (libSceAgcDriver) are stored when something reads their memory, not when the GPU work that wrote them completes. A title that waits for that work, frees the image and reuses its memory can have the new data overwritten by the late store (#1970). A title's own CPU reads are not seen either, so a title that reads the results after waiting for a label reads stale memory. `APS5_STORE_AT_FLIP=1` stores every pending result at each flip and `APS5_STORE_AT_LABEL=1` before each label the GPU writes; both store every pending image, not only the ones the waited-for work wrote, and neither is on by default. - [Pending storage-image results over memory outside the write watch](../../../core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp) (libSceAgcDriver) leave to the CPU the 64 KiB blocks whose bytes differ from the ones saved at the upload, the fill, the image copy, the previous store or the moment their range left the watch (`Unwatch`, or a host import that does not stay watched), and the blocks the watch saw the CPU write before that moment. A store made on the GPU saves its bytes through a host-visible copy that becomes the saved bytes when its batch completes; a store or a refresh that needs them earlier waits for that batch. A unit without a write generation saves the guest bytes when results land in it outside the watch. One whose range leaves the watch while its results are pending, and ranges that leave the watch through a failed collect, have no saved bytes; storing such results throws. +- [Driver-store pieces of a 64 KiB block](../../../core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp) (libSceAgcDriver) keep the byte ranges of up to four partial driver stores; a store whose range contains earlier ones takes their place, and a store that finds no free slot forgets the oldest. A forgotten store counts as a store over every range of the block for any generation before it: `StoredOver` answers true and `StoredAllSince` false, and a storage image's write-back leaves the block to the CPU and re-uploads its texels there unless they still equal the image's saved copy. More than four partial stores into one block whose ranges do not contain the ones kept before them can therefore still drop results an image holds in it. Keeping the extent of the forgotten ranges instead of one generation for the block is not implemented. - Windows host-pointer imports of the guest mapping itself are compared instead of write-watched, except writable imports of private `MEM_WRITE_WATCH` memory of 1 MiB or more when the import probe keeps the write watch and a settle queue exists (see Pages under an import window): a driver can mark imported pages dirty after the synchronous import window. Shared memory imported through a separate writable alias retains protection-based tracking on the guest views; driver access to the import alias does not disarm those views. [GuestMemory](../../../core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp) excludes every overlapping 64 KiB tracker block while its backing is retained. A full block replaced by fresh private backing regains tracking and gets a new generation; protection changes, partial replacements and shared remappings do not restore it. Excluded blocks keep their write-watch marks as a hint only (`UntrackedHint`): a storage image or a sampled texture snapshot over them compares its bytes again after a mark in its range, a driver store, a mapping change or a recorded GPU write instead of at every collect epoch or lookup, and a mark the driver makes without a CPU store costs one comparison. This preserves CPU edits through byte comparisons but limits snapshot reuse on excluded blocks to draws of one collect epoch and disables unit shadows for them; restoring tracking on import retirement without replacing the backing remains unimplemented. - [Pages under an import window](../../../core/libs/prx/libSceAgcDriver/Execution/src/GuestMemory.cpp) (libSceAgcDriver) whose write protection was resolved while a host import that leaves its range watched was made are compared with the bytes they held before the import and count as stored over only when those differ. A CPU store of the bytes already there within the window cannot be told apart from an import that resolves the write protection without storing, as the NVIDIA driver (610.57.04) does for every imported page, so such a store does not win over pending results. Where host-pointer and dma-buf imports differ in whether they stay watched, a failed host-pointer import and the dma-buf import after it are two windows, and the pages the failed one resolved count as stored over. On Windows, the NVIDIA driver (617.42) marks every page of a host import of private `MEM_WRITE_WATCH` memory written, asynchronously, until the import's first GPU use completes. Such an import stays watched only after a 4-byte read of it on the settle queue has completed inside the window. Each one waits for that read, which keeps its window open for one round trip on the settle queue, so a same-value CPU store from another thread in that time counts as unchanged too. An import made where no settle queue exists, and an import of under 1 MiB, is compared instead: the settle blocks the importing thread, and a title that imports a 64 KiB buffer around every submission lost about 19 % of its frame rate to that wait (Dripl1x, #6060). Settling without blocking the importing thread (submit the read, keep the import compared until its fence signals, then switch it to the write watch) is not implemented. - [Sparse dma-buf imports](../../../core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp) (libSceAgcDriver): on Linux a shared-backed range larger than one 64 MiB udmabuf buffer is imported as one sparse buffer bound to the 64 MiB buffers, on a queue of its own when the device has a second family that binds sparse memory. The path is decided before anything is imported, and a failure after that throws. A range takes the single udmabuf import instead, which udmabuf's `size_limit_mb` refuses above 64 MiB by default so the range is copied, when its offset in the backing file is not a multiple of the buffer's sparse block size (64 KiB on RADV), when the sparse buffer does not accept every memory type a chunk import can have, when the backing file ends inside its last chunk, or when it is imported at map time on a device with no second family (the bind then needs the GPU mutex); the NVIDIA proprietary driver imports no dma-buf memory