From 544646ee39b1db0ba707e8199db24767d557f2e9 Mon Sep 17 00:00:00 2001 From: Dean Galvin Date: Tue, 29 Sep 2026 13:16:36 +1300 Subject: [PATCH] fix(agc): keep render target results where the write tracker cannot prove memory fresh Two paths lost a render target's results whenever CollectWrites answers 0 (no write watching: the Linux guest arena, or memory outside the Windows arena), where every unit reads as changed at every check. StorageTexture::Refresh dropped every pending result whenever the surface's DCC keys read as a clear code, not only when the keys changed to one. Draws never update the title's keys, so a resident target cleared under a clear code keeps that code while holding newer draw results, and the lookup of the next draw sampling the target re-cleared the image. Astro Bot's intro composite sampled an empty UI layer on Linux. Only new clear keys drop results now; results kept under the image's own clear code are stored whole (the store marks every key uncompressed, as writeBack does) and re-uploaded like any untracked unit. A unit shadow slab is fresh only at a tracker generation and MarkShadowed ignores generation 0, so a write-back retiled into a slab over unwatched memory was never marked, published or read back: its texels were lost and the next upload re-read the stale import. UnitShadowEnabled now also requires a write-watched arena, and ShadowDestinationFor offers no slab for a range outside the arena; those retiles go into the import, as with APS5_NO_UNIT_SHADOW=1. agc_driver_recorder_tests gains a GPU test over unwatched memory: a target cleared under 0000 keys, drawn into, then refreshed keeps the draw's texels and stores them to guest memory; no shadow slab is offered for it; new 0001 keys still drop pending results. --- .../libSceAgcDriver/Graphics/src/Texture.cpp | 3 +- .../Graphics/src/UnitShadow.cpp | 13 +- .../prx/libSceAgcDriver/tests/Recorder.cpp | 119 +++++++++++++++++- 3 files changed, 131 insertions(+), 4 deletions(-) diff --git a/core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp b/core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp index f5cd4eccb..4115fce75 100644 --- a/core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp +++ b/core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp @@ -1098,7 +1098,7 @@ bool StorageTexture::Refresh() { // clear code make every result dead on hardware too. Without a generation (no tracking) a // unit's stamps say nothing, so it is stored. const auto droppable = [&](std::uint32_t unit) { - if (IsDccClear(keys)) return true; + if (keysChanged && IsDccClear(keys)) return true; return layerGeneration[unit] != 0 && unit < stampedBlocks.size() && stampedBlocks[unit] != 0; }; // A clear code -> uncompressed flip on an image with results pending: an unstamped pending @@ -1140,6 +1140,7 @@ bool StorageTexture::Refresh() { stored[layer] = true; anyStored = true; } + if (anyStored && !keysChanged && descriptor.dccAddress != 0 && IsDccClear(uploadedKeys)) stored.assign(trackedLayers, true); if (pendingResults) { static std::atomic reports{0}; if (reports.fetch_add(1) < 8) std::fprintf(stderr, "[gpu] storage image 0x%llx: guest memory changed while GPU results were pending; keeping the CPU's blocks\n", static_cast(descriptor.baseAddress)); diff --git a/core/libs/prx/libSceAgcDriver/Graphics/src/UnitShadow.cpp b/core/libs/prx/libSceAgcDriver/Graphics/src/UnitShadow.cpp index 7e3661a1d..c93e1a47d 100644 --- a/core/libs/prx/libSceAgcDriver/Graphics/src/UnitShadow.cpp +++ b/core/libs/prx/libSceAgcDriver/Graphics/src/UnitShadow.cpp @@ -4,6 +4,7 @@ #include "prx/libSceAgcDriver/Graphics/include/Resources.hpp" #include "prx/libSceAgcDriver/Graphics/include/Texture.hpp" #include "prx/libSceAgcDriver/Execution/include/GuestMemory.hpp" +#include "prx/libc/include/GuestArena.hpp" #include #include #include @@ -14,6 +15,7 @@ #include #include #include +#include namespace AgcDriver::Graphics { @@ -416,8 +418,8 @@ bool evictOne(Shadows& registry) { } bool UnitShadowEnabled() { - static const bool disabled = std::getenv("APS5_NO_UNIT_SHADOW") != nullptr; - return !disabled; + static const bool enabled = std::getenv("APS5_NO_UNIT_SHADOW") == nullptr && GuestArena::GuestArenaWriteWatched_nid_postfix(); + return enabled; } bool ShadowVerify() { @@ -517,6 +519,13 @@ VkDeviceSize SlabOffset(const HostImport& import, const ShadowSlab& slab, std::u std::optional ShadowDestinationFor(const Context& context, const HostImport& import, std::uint64_t begin, std::uint64_t end) { if (!UnitShadowEnabled() || end <= begin || begin < import.base || end > import.base + import.bytes) return std::nullopt; + static const auto arena = [] { + std::uintptr_t base = 0; + std::size_t size = 0; + GuestArena::GuestArenaRange_nid_postfix(&base, &size); + return std::pair{base, size}; + }(); + if (arena.second == 0 || begin < arena.first || end > arena.first + arena.second) return std::nullopt; auto& registry = Registry(); std::shared_ptr shadow; std::size_t slabIndex = 0; diff --git a/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp b/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp index 8679cf508..2e14fc548 100644 --- a/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp +++ b/core/libs/prx/libSceAgcDriver/tests/Recorder.cpp @@ -4,6 +4,8 @@ #include "prx/libSceAgcDriver/Graphics/include/Resources.hpp" #include "prx/libSceAgcDriver/Graphics/include/ShaderResources.hpp" #include "prx/libSceAgcDriver/Graphics/include/Shaders.hpp" +#include "prx/libSceAgcDriver/Graphics/include/Texture.hpp" +#include "prx/libSceAgcDriver/Graphics/include/TextureDetiler.hpp" #include "prx/libSceAgcDriver/Graphics/include/UnitShadow.hpp" #include "prx/libSceAgcDriver/Execution/include/BdaFeatures.hpp" #include "prx/libSceAgcDriver/Execution/include/GuestMemory.hpp" @@ -22,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -106,6 +109,7 @@ public: function("vkGetPhysicalDeviceProperties")(context.physical, &properties); context.limits = properties.limits; context.bufferDeviceAddress = true; + context.formatProperties = function("vkGetPhysicalDeviceFormatProperties"); context.Function("vkGetDeviceQueue")(context.device, family, 0, &context.queue); VkCommandPoolCreateInfo pool{VK_STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO}; pool.queueFamilyIndex = family; @@ -709,7 +713,7 @@ void unitShadowTests(const Device& device, Recorder& recorder) { const auto& context = device.GetContext(); if (!UnitShadowEnabled()) { Require(!AnyShadowedOverlaps(0x10000, 16) && PublishShadow(0x10000, 16, PublishScope::Whole, PublishReason::Hook) == 0, "unit shadows are off but not inert"); - std::cout << "unit shadows off (APS5_NO_UNIT_SHADOW): primitives inert\n"; + std::cout << "unit shadows off (APS5_NO_UNIT_SHADOW, or an arena without write watching): primitives inert\n"; return; } if (context.hostImportAlignment == 0 || !GuestArena::GuestArenaAvailable_nid_postfix() || !GuestArena::GuestArenaWriteWatched_nid_postfix()) { @@ -893,6 +897,118 @@ void unitShadowTests(const Device& device, Recorder& recorder) { else Require(words[128 * unit] == 0x77, "the eviction before the retire did not publish unit 128"); } +void storageRefreshTests(const Device& device, Recorder& recorder) { + const auto& base = device.GetContext(); + if (base.hostImportAlignment == 0) { + std::cout << "host imports unavailable: storage refresh not tested\n"; + return; + } + constexpr std::uint32_t side = 256; + constexpr std::size_t surfaceBytes = side * side * 4; + constexpr std::size_t keyCount = surfaceBytes / 256; + constexpr std::size_t bytes = surfaceBytes + 65536; +#ifdef _WIN32 + void* block = VirtualAlloc(nullptr, bytes, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); +#else + void* block = std::aligned_alloc(65536, bytes); +#endif + Require(block != nullptr, "cannot allocate the storage refresh block"); + auto* texels = static_cast(block); + auto* keys = texels + surfaceBytes; + std::memset(texels, 0x55, surfaceBytes); + std::memset(keys, 0x00, keyCount); + const auto address = reinterpret_cast(block); + { + GuestAllocations::Mutation mutation; + mutation.Add(block, bytes, true, true); + } + struct Unregister { + const Context& context; + void* block; + std::uint64_t address; + ~Unregister() { + { + GuestAllocations::Mutation mutation; + mutation.Remove(block); + } + HostImportFor(context, address, bytes); + } + } unregister{base, block, address}; + const auto* import = HostImportFor(base, address, bytes); + if (import == nullptr) { + std::cout << "host import of the storage refresh block refused: storage refresh not tested\n"; + return; + } + Require(!ShadowDestinationFor(base, *import, address, address + 65536).has_value(), "a unit shadow was offered for memory the write tracker does not watch"); + if (!GuestArena::GuestArenaWriteWatched_nid_postfix()) Require(!UnitShadowEnabled(), "unit shadows are on without write watching"); + TextureDetiler detiler(base); + auto context = base; + context.detiler = &detiler; + GuestTextureResource resource{}; + resource.baseAddress = address; + resource.width = side; + resource.height = side; + resource.mipCount = 1; + resource.tileMode = TextureTileMode::kR64KBX; + resource.dimension = TextureDimension::k2D; + resource.format = 56; + resource.dstSelX = 4; + resource.dstSelY = 5; + resource.dstSelZ = 6; + resource.dstSelW = 7; + resource.dccAddress = address + surfaceBytes; + Require(DescribeSurface(resource).guestBytes == surfaceBytes, "the test surface has an unexpected size"); + DccKeyProof proof; + Require(ProvedClearKeys(resource, surfaceBytes, proof) == DccKeys::Clear0000, "0x00 keys are not a 0000 clear"); + { + auto image = std::make_shared(context, detiler, resource, 0); + const VkImageSubresourceRange range{VK_IMAGE_ASPECT_COLOR_BIT, 0, 1, 0, 1}; + const auto draw = [&](VkClearColorValue value) { + const auto commands = recorder.Commands(); + recorder.Keep(image); + 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(); + }; + const auto holds = [&](std::array texel) { + Buffer readback(context, surfaceBytes, VK_BUFFER_USAGE_TRANSFER_DST_BIT); + const auto commands = recorder.Commands(); + 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_READ_BIT); + VkBufferImageCopy copy{}; + copy.imageSubresource = {VK_IMAGE_ASPECT_COLOR_BIT, 0, 0, 1}; + copy.imageExtent = {side, side, 1}; + context.Function("vkCmdCopyImageToBuffer")(commands, image->Image(), VK_IMAGE_LAYOUT_GENERAL, readback.Handle(), 1, ©); + RecordMemoryBarrier(context, commands, VK_PIPELINE_STAGE_TRANSFER_BIT, VK_PIPELINE_STAGE_HOST_BIT, VK_ACCESS_TRANSFER_WRITE_BIT, VK_ACCESS_HOST_READ_BIT); + recorder.Submit(); + device.WaitQueue(); + recorder.Sync(); + const auto pixels = readback.Bytes(); + for (std::size_t i = 0; i < pixels.size(); ++i) { + if (std::to_integer(pixels[i]) != texel[i % 4]) return false; + } + return true; + }; + const auto memoryHolds = [&](std::array texel) { + for (std::size_t i = 0; i < surfaceBytes; ++i) { + if (texels[i] != texel[i % 4]) return false; + } + return true; + }; + Require(holds({0, 0, 0, 0}), "a surface under 0000 clear keys was not cleared"); + draw({{1.0f, 0.0f, 0.0f, 1.0f}}); + image->Refresh(); + Require(holds({255, 0, 0, 255}), "a refresh under the image's own clear keys dropped the draw's results"); + Require(memoryHolds({255, 0, 0, 255}), "the refresh's store of the results did not reach guest memory"); + draw({{0.0f, 1.0f, 0.0f, 1.0f}}); + std::memset(keys, 0x40, keyCount); + image->Refresh(); + Require(holds({0, 0, 0, 255}), "new 0001 clear keys did not clear the image"); + Require(memoryHolds({255, 0, 0, 255}), "results dead under new clear keys were stored"); + } + recorder.Sync(); +} + // The data word positions of a dispatch-cache variant (Driver.cpp's data-only hits): leaves // located among the runs' words, aliased, unaligned, out-of-run and mismatched ones skipped. void dataWordPositionsTests() { @@ -972,6 +1088,7 @@ int main() { resourceReadTests(device, recorder); storeRunTests(device, recorder); unitShadowTests(device, recorder); + storageRefreshTests(device, recorder); dataWordPositionsTests(); dataRefreshTests(device, recorder); std::cout << "Recorder read tracking and label tests passed\n";