fix(agc): wait for an image another thread is storing before a host read

core/libs/prx/libSceAgcDriver/Graphics/include/Texture.hpp

@@ -209,6 +209,7 @@ class StorageTexture : public std::enable_shared_from_this<StorageTexture> {
     // overlapping pending image, as before).
     static bool FlushPending(std::uint64_t address, std::size_t bytes, const StorageTexture* except = nullptr, const char* reason = "memory access", PublishScope scope = PublishScope::Whole, bool* published = nullptr);
     static void FlushAllPending(const char* reason);
+    static bool WaitForFlushing(std::uint64_t address, std::size_t bytes, const StorageTexture* except = nullptr);
     static void SaveBeforeUnwatch(std::uint64_t address, std::size_t bytes);
     static constexpr std::uint64_t SmallStoreBytes = 64 * 1024;
     static std::size_t StoreSmallPending(const char* reason);

core/libs/prx/libSceAgcDriver/Graphics/src/Recorder.cpp

@@ -1049,6 +1049,7 @@ void FlushForAccess(std::uint64_t address, std::size_t bytes) {
     std::size_t images = 0, evicted = 0;
     const bool kept = !HookFlushCpuBlocks() && StorageTexture::AccessKeptByCpu(address, bytes, &images, &evicted);
     if (kept || (images == 0 && !HookFlushCpuBlocks())) {
+        stored = StorageTexture::WaitForFlushing(address, bytes);
         published = StorageTexture::PublishShadowsOnly(address, bytes, scope);
         if (kept && HookSyncProfiled()) CountRecordedStoreSkip(RecordedStoreKey(), evicted);
     } else {

core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp

@@ -33,6 +33,7 @@
 #include <limits>
 #include <exception>
 #include <optional>
+#include <thread>
 #include <unordered_map>
 #include <utility>
 
@@ -1488,6 +1489,7 @@ struct PendingWrites {
     PendingList textures;
     // Images taken out of `textures` by a FlushPending still storing them (see adjacentPendingUnchanged).
     std::vector<StorageTexture*> flushing;
+    std::atomic<std::size_t> flushingCount{0};
 };
 
 PendingWrites& Pending() {
@@ -3666,21 +3668,24 @@ bool StorageTexture::FlushPending(std::uint64_t address, std::size_t bytes, cons
         }
         // Still pending for the adjacency rule of writeBack until each one is stored.
         for (const auto& texture : flush) pending.flushing.push_back(texture.get());
+        pending.flushingCount.store(pending.flushing.size(), std::memory_order_release);
         if (!flush.empty()) BumpPendingSerial();
     }
     if (flush.empty()) {
+        const bool waited = WaitForFlushing(address, bytes, except);
         // Units shadowed over the range (another image's results retiled into the import's unit
         // shadow, or an earlier flush's) still reach the import as the scope asks.
         const bool units = PublishShadowsOnly(address, bytes, scope, reason);
         if (published != nullptr) *published = units;
-        return false;
+        return waited;
     }
     struct Unregister {
         const std::vector<std::shared_ptr<StorageTexture>>& flush;
         ~Unregister() {
             auto& pending = Pending();
             std::lock_guard lock(pending.mutex);
             for (const auto& texture : flush) std::erase(pending.flushing, texture.get());
+            pending.flushingCount.store(pending.flushing.size(), std::memory_order_release);
             BumpPendingSerial();
         }
     } unregister{flush};
@@ -3726,6 +3731,20 @@ bool StorageTexture::FlushPending(std::uint64_t address, std::size_t bytes, cons
     return true;
 }
 
+bool StorageTexture::WaitForFlushing(std::uint64_t address, std::size_t bytes, const StorageTexture* except) {
+    auto& pending = Pending();
+    if (pending.flushingCount.load(std::memory_order_acquire) == 0 || GuestMemory::GpuMutex().HeldByThisThread()) return false;
+    const auto* exempt = refreshing;
+    for (bool waited = false;; waited = true) {
+        {
+            std::lock_guard lock(pending.mutex);
+            if (std::none_of(pending.flushing.begin(), pending.flushing.end(), [&](const StorageTexture* texture) { return texture != except && texture != exempt && texture->overlaps(address, bytes); })) return waited;
+        }
+        { std::lock_guard gpu(GuestMemory::GpuMutex()); }
+        std::this_thread::yield();
+    }
+}
+
 void StorageTexture::SaveBeforeUnwatch(std::uint64_t address, std::size_t bytes) {
     std::vector<std::shared_ptr<StorageTexture>> overlapping;
     {

core/libs/prx/libSceAgcDriver/tests/execution/StorageFileWrite.cpp

@@ -18,6 +18,10 @@
 #include <limits>
 #include <span>
 #include <string>
+#include <thread>
+#include <atomic>
+#include <condition_variable>
+#include <mutex>
 #include <stdexcept>
 #include <vector>
 #ifdef _WIN32
@@ -240,6 +244,68 @@ bool CheckWrite(Api api, const char* name, GuestSurface& surface) {
     return differences == 0;
 }
 
+void CheckConcurrentFlushWait(GuestSurface& surface) {
+    Produce(surface.Data());
+    const auto address = reinterpret_cast<std::uintptr_t>(surface.Data());
+    Require(AgcDriver::Graphics::StorageTexture::FindPending(address, SurfaceBytes) != nullptr, "file write: concurrent flush setup has no pending image");
+    std::mutex mutex;
+    std::condition_variable changed;
+    bool gpuLocked = false;
+    bool releaseGpu = false;
+    std::thread holder([&] {
+        std::lock_guard gpu(AgcDriver::GuestMemory::GpuMutex());
+        {
+            std::lock_guard lock(mutex);
+            gpuLocked = true;
+        }
+        changed.notify_all();
+        std::unique_lock lock(mutex);
+        changed.wait(lock, [&] { return releaseGpu; });
+    });
+    {
+        std::unique_lock lock(mutex);
+        changed.wait(lock, [&] { return gpuLocked; });
+    }
+    std::exception_ptr flushFailure;
+    std::thread flusher([&] {
+        try { AgcDriver::Graphics::StorageTexture::FlushPending(address, SurfaceBytes, nullptr, "concurrent flush test"); }
+        catch (...) { flushFailure = std::current_exception(); }
+    });
+    const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5);
+    while (AgcDriver::Graphics::StorageTexture::FindPending(address, SurfaceBytes) != nullptr && std::chrono::steady_clock::now() < deadline) std::this_thread::yield();
+    const bool taken = AgcDriver::Graphics::StorageTexture::FindPending(address, SurfaceBytes) == nullptr;
+    std::vector<std::byte> seen(SurfaceBytes);
+    std::atomic<bool> readDone = false;
+    std::exception_ptr readFailure;
+    std::thread reader;
+    if (taken) {
+        reader = std::thread([&] {
+            try {
+                AgcDriver::Graphics::StorageTexture::FlushPending(address, SurfaceBytes, nullptr, "concurrent read test");
+                AgcDriver::GuestMemory::Read(address, seen);
+            } catch (...) {
+                readFailure = std::current_exception();
+            }
+            readDone.store(true, std::memory_order_release);
+            changed.notify_all();
+        });
+        std::unique_lock lock(mutex);
+        changed.wait_for(lock, std::chrono::milliseconds(50), [&] { return readDone.load(std::memory_order_acquire); });
+        releaseGpu = true;
+    } else {
+        std::lock_guard lock(mutex);
+        releaseGpu = true;
+    }
+    changed.notify_all();
+    holder.join();
+    flusher.join();
+    if (reader.joinable()) reader.join();
+    Require(taken, "file write: concurrent flush did not take the pending image");
+    if (flushFailure != nullptr) std::rethrow_exception(flushFailure);
+    if (readFailure != nullptr) std::rethrow_exception(readFailure);
+    Require(std::any_of(seen.begin(), seen.end(), [](std::byte value) { return value != std::byte{}; }), "file write: concurrent guest read returned before an overlapping image was stored");
+}
+
 void CheckShutdownAndGuard() {
     OutputFile file;
     std::vector<std::byte> source(TransferBytes, std::byte{0x5a});
@@ -266,6 +332,7 @@ int main() {
         }
         GuestSurface surface;
         GuestSurface unrelated;
+        CheckConcurrentFlushWait(surface);
         Produce(unrelated.Data());
         bool correct = true;
         const std::array<std::pair<Api, const char*>, 12> cases{{

core/libs/prx/libSceAgcDriver/tests/execution/StorageReadDuringIdleStore.cpp

@@ -0,0 +1,159 @@
+#include "prx/libSceAgcDriver/Execution/include/Driver.hpp"
+#include "prx/libSceAgcDriver/Execution/include/GuestMemory.hpp"
+#include "prx/libSceAgcDriver/Graphics/include/Draw.hpp"
+#include "prx/libSceAgcDriver/Graphics/include/Texture.hpp"
+#include "prx/libc/include/GuestAllocations.hpp"
+#include "prx/libc/include/GuestArena.hpp"
+#include "prx/libc/include/GuestWriteWatch.hpp"
+#include "VulkanTestDevice.hpp"
+#include <algorithm>
+#include <array>
+#include <atomic>
+#include <cstdio>
+#include <cstring>
+#include <iostream>
+#include <mutex>
+#include <span>
+#include <stdexcept>
+#include <thread>
+#include <vector>
+#ifdef _WIN32
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+#include <windows.h>
+#else
+#include <sys/mman.h>
+#endif
+
+namespace {
+
+using AgcDriver::Graphics::Require;
+using AgcDriver::Graphics::StorageTexture;
+constexpr std::uint32_t Width = 128;
+constexpr std::uint32_t Height = 120;
+constexpr std::size_t SurfaceBytes = 0x20000;
+constexpr int Rounds = 20;
+alignas(256) constexpr std::array<std::uint32_t, 11> WriteCode{
+    0xd7460004u, 0x0401060eu, 0xd7460005u, 0x0405060fu, 0x7e100d04u, 0x7e120d05u,
+    0x7e140c0du, 0x7e1602f2u, 0xf0200f08u, 0x00000804u, 0xbf810000u,
+};
+
+class GuestSurface {
+public:
+    GuestSurface() {
+#ifdef _WIN32
+        block = static_cast<std::uint8_t*>(GuestArena::GuestArenaAllocate_nid_postfix(SurfaceBytes, SurfaceBytes));
+        GuestArena::GuestArenaCommit_nid_postfix(block, SurfaceBytes, PAGE_READWRITE, SurfaceBytes);
+#else
+        void* mapped = mmap(nullptr, SurfaceBytes * 2, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+        Require(mapped != MAP_FAILED, "idle store read: cannot allocate the guest surface");
+        const auto begin = reinterpret_cast<std::uintptr_t>(mapped);
+        const auto aligned = (begin + SurfaceBytes - 1) & ~(std::uintptr_t{SurfaceBytes - 1});
+        if (aligned != begin) munmap(mapped, aligned - begin);
+        const auto tail = begin + SurfaceBytes * 2 - aligned - SurfaceBytes;
+        if (tail != 0) munmap(reinterpret_cast<void*>(aligned + SurfaceBytes), tail);
+        block = reinterpret_cast<std::uint8_t*>(aligned);
+        if (AgcDriver::GuestMemory::WriteWatched()) GuestWriteWatch::GuestWriteWatchRegister_nid_postfix(block, SurfaceBytes);
+#endif
+        GuestAllocations::Mutation mutation;
+        mutation.Add(block, SurfaceBytes, true, true, true);
+    }
+
+    ~GuestSurface() {
+        {
+            GuestAllocations::Mutation mutation;
+            mutation.Remove(block);
+        }
+#ifdef _WIN32
+        GuestArena::GuestArenaReset_nid_postfix(block, SurfaceBytes);
+        GuestArena::GuestArenaRelease_nid_postfix(block, SurfaceBytes);
+#else
+        munmap(block, SurfaceBytes);
+        if (AgcDriver::GuestMemory::WriteWatched()) GuestWriteWatch::GuestWriteWatchUnregister_nid_postfix(block, SurfaceBytes);
+#endif
+    }
+
+    GuestSurface(const GuestSurface&) = delete;
+    GuestSurface& operator=(const GuestSurface&) = delete;
+    std::uint8_t* Data() { return block; }
+    std::uint64_t Address() const { return reinterpret_cast<std::uintptr_t>(block); }
+
+private:
+    std::uint8_t* block = nullptr;
+};
+
+void Produce(GuestSurface& surface) {
+    std::memset(surface.Data(), 0, SurfaceBytes);
+    const auto address = surface.Address();
+    const std::array<std::uint32_t, 14> data{
+        static_cast<std::uint32_t>(address >> 8u),
+        static_cast<std::uint32_t>((address >> 40u) & 0xffu) | (71u << 20u) | (((Width - 1u) & 3u) << 30u),
+        ((Width - 1u) >> 2u) | ((Height - 1u) << 14u),
+        0xfacu | (0x09u << 20u) | (9u << 28u),
+        0u, 0u, 0u, 0u, 0u, 0u, 0u, 0u, 0u, 7u,
+    };
+    std::vector<std::uint32_t> words;
+    const auto registers = [&](std::uint32_t first, std::span<const std::uint32_t> values) {
+        words.push_back(0xc0007600u | (static_cast<std::uint32_t>(values.size()) << 16u));
+        words.push_back(first);
+        words.insert(words.end(), values.begin(), values.end());
+    };
+    const std::array<std::uint32_t, 3> shape{8, 8, 1};
+    registers(0x207, shape);
+    const auto code = reinterpret_cast<std::uintptr_t>(WriteCode.data());
+    const std::array<std::uint32_t, 2> program{static_cast<std::uint32_t>(code >> 8u), static_cast<std::uint32_t>(code >> 40u)};
+    registers(0x20c, program);
+    const std::array<std::uint32_t, 1> resource{(static_cast<std::uint32_t>(data.size()) << 1u) | (7u << 7u) | (1u << 11u)};
+    registers(0x213, resource);
+    registers(0x240, data);
+    words.insert(words.end(), {0xc0031500u, Width / 8u, Height / 8u, 1u, 0x41u});
+    Packet packet{words.data(), static_cast<std::uint32_t>(words.size()), 0, {}};
+    AgcDriver::Submit(&packet, 0);
+    AgcDriverSuspendPoint_nid_postfix();
+    AgcDriverWaitIdle_nid_postfix();
+    Require(StorageTexture::FindPending(surface.Address(), SurfaceBytes) != nullptr, "idle store read: the GPU producer left no pending image");
+}
+
+bool AllZero(std::span<const std::byte> bytes) {
+    return std::none_of(bytes.begin(), bytes.end(), [](std::byte value) { return value != std::byte{}; });
+}
+
+}
+
+int main() {
+    try {
+        {
+            const auto device = OpenVulkanTestDevice();
+            if (!device) return VulkanTestSkipped;
+        }
+        GuestSurface surface;
+        int stale = 0;
+        for (int round = 0; round < Rounds; ++round) {
+            Produce(surface);
+            std::atomic<bool> started{false};
+            std::thread idle([&] {
+                started = true;
+                std::lock_guard gpu(AgcDriver::GuestMemory::GpuMutex());
+                StorageTexture::FlushAllPending("idle");
+            });
+            while (!started) {}
+            while (StorageTexture::FindPending(surface.Address(), SurfaceBytes) != nullptr) {}
+            std::vector<std::byte> seen(SurfaceBytes);
+            AgcDriver::GuestMemory::Read(surface.Address(), seen);
+            idle.join();
+            stale += AllZero(seen);
+            std::vector<std::byte> stored(SurfaceBytes);
+            AgcDriver::GuestMemory::Read(surface.Address(), stored);
+            Require(!AllZero(stored), "idle store read: the GPU producer wrote only zeros");
+        }
+        std::printf("idle store read: %d of %d host reads saw the memory before the store\n", stale, Rounds);
+        Require(stale == 0, "idle store read: a host read did not wait for the image another thread was storing");
+        AgcDriverShutdown_nid_postfix();
+        return 0;
+    } catch (const std::exception& error) {
+        std::cerr << error.what() << "\n";
+        AgcDriverShutdown_nid_postfix();
+        return 1;
+    }
+}