From 187654eae6d6df619e8b055e09bb5ed1612a3740 Mon Sep 17 00:00:00 2001 From: jpolo1224 Date: Thu, 20 Aug 2026 00:16:33 -0400 Subject: [PATCH] Revert the frame-generation work entirely Both changes that were brought back are out again. Neither recovered the frame rate after switching frame generation off, and both broke rendering. That is every attempt at this reverted. What remains is the code as it stood before any of it: frame generation is slow, and switching it off does not release what it allocated. The GLES DSA shim is kept, because it belongs to the upstream merge and not to this work -- it landed in the wrong commit and reverting that commit took it with it, which broke the build for a reason unrelated to frame generation. The findings from the three audits are still correct as descriptions of what the code does. Acting on them, one at a time and with the reasoning written down each time, still made the result worse on every attempt. So the fault is not in the individual fixes but in something about this subsystem that reading it has not revealed, and the next attempt should start from a measurement on the device rather than from another reading of the source. --- 3rdparty/lsfg/armsx3_lsfg.map | 1 - 3rdparty/lsfg/armsx3_lsfg_shim.cpp | 24 +----- 3rdparty/lsfg/armsx3_lsfg_shim.h | 25 +----- rpcs3/Emu/RSX/VK/VKFrameGen.cpp | 132 +++-------------------------- rpcs3/Emu/RSX/VK/VKPresent.cpp | 52 +----------- 5 files changed, 18 insertions(+), 216 deletions(-) diff --git a/3rdparty/lsfg/armsx3_lsfg.map b/3rdparty/lsfg/armsx3_lsfg.map index 96e4f861e..2812d26fa 100644 --- a/3rdparty/lsfg/armsx3_lsfg.map +++ b/3rdparty/lsfg/armsx3_lsfg.map @@ -19,7 +19,6 @@ armsx3_lsfg_initialize; armsx3_lsfg_create_context_ahb; armsx3_lsfg_present; - armsx3_lsfg_present_fenced; armsx3_lsfg_destroy_context; armsx3_lsfg_wait_idle; armsx3_lsfg_finalize; diff --git a/3rdparty/lsfg/armsx3_lsfg_shim.cpp b/3rdparty/lsfg/armsx3_lsfg_shim.cpp index ae0743730..986503430 100644 --- a/3rdparty/lsfg/armsx3_lsfg_shim.cpp +++ b/3rdparty/lsfg/armsx3_lsfg_shim.cpp @@ -184,22 +184,6 @@ extern "C" int32_t armsx3_lsfg_create_context_ahb(void* in0, void* in1, void* co extern "C" int armsx3_lsfg_present(int32_t ctx, int in_sem, const int* out_sems, uint32_t out_count) { - // Forwarded rather than duplicated. Asking for no fence descriptor is exactly what this - // always did, and keeping one body means the two entry points cannot drift. - return armsx3_lsfg_present_fenced(ctx, in_sem, out_sems, out_count, nullptr); -} - -extern "C" int armsx3_lsfg_present_fenced(int32_t ctx, int in_sem, const int* out_sems, - uint32_t out_count, int* out_fence_fd) -{ - if (out_fence_fd) - { - // Written before anything that can fail. Every path out of here leaves the caller with a - // value it can act on, so it can never read an uninitialised int and close a descriptor - // belonging to something else -- which on Android is somebody's socket or an open asset. - *out_fence_fd = -1; - } - if (!g_initialized) { set_error("not initialized"); @@ -214,17 +198,13 @@ extern "C" int armsx3_lsfg_present_fenced(int32_t ctx, int in_sem, const int* ou outs.push_back(out_sems ? out_sems[i] : -1); } - // framegen writes the descriptor itself and leaves it at -1 when it cannot produce one, so a - // null out_fence_fd degrades to the plain present without a second code path here. if (g_performance) { - ARMSX3_LSFG_GUARD(LSFG_3_1P::presentContextFenced(ctx, in_sem, outs, out_fence_fd), - ARMSX3_LSFG_ERR_VULKAN) + ARMSX3_LSFG_GUARD(LSFG_3_1P::presentContext(ctx, in_sem, outs), ARMSX3_LSFG_ERR_VULKAN) } else { - ARMSX3_LSFG_GUARD(LSFG_3_1::presentContextFenced(ctx, in_sem, outs, out_fence_fd), - ARMSX3_LSFG_ERR_VULKAN) + ARMSX3_LSFG_GUARD(LSFG_3_1::presentContext(ctx, in_sem, outs), ARMSX3_LSFG_ERR_VULKAN) } return ARMSX3_LSFG_OK; diff --git a/3rdparty/lsfg/armsx3_lsfg_shim.h b/3rdparty/lsfg/armsx3_lsfg_shim.h index 38214a43d..05934475c 100644 --- a/3rdparty/lsfg/armsx3_lsfg_shim.h +++ b/3rdparty/lsfg/armsx3_lsfg_shim.h @@ -29,7 +29,7 @@ extern "C" { // Bump when anything below changes shape. The loader refuses a library whose version it does not // recognise, so a stale libarmsx3_lsfg.so on a user's device fails loudly at load instead of // quietly passing mismatched structs. -#define ARMSX3_LSFG_ABI_VERSION 3u +#define ARMSX3_LSFG_ABI_VERSION 2u // Mark the exported surface explicitly. // @@ -103,29 +103,6 @@ ARMSX3_LSFG_API int32_t armsx3_lsfg_create_context_ahb(void* in0, void* in1, voi // each out_sems[i] is signalled when output image i is ready. Pass -1 for an unused slot. ARMSX3_LSFG_API int armsx3_lsfg_present(int32_t ctx, int in_sem, const int* out_sems, uint32_t out_count); -// Generate frames for one presented pair, and hand back a fence for the result. -// -// Identical to armsx3_lsfg_present in every respect except that *out_fence_fd receives a sync file -// descriptor that becomes readable once the generation this call submitted has finished. The -// caller owns that fd and must close(2) it. -// -// This is the answer to armsx3_lsfg_wait_idle() below being the only completion signal on offer. -// framegen renders on its OWN VkDevice, so the caller cannot wait on its queues; before this -// entry point existed the only way to know the generated images were ready -- and, more -// importantly, that framegen had finished READING the caller's input images -- was a -// vkDeviceWaitIdle on framegen's device, once per presented frame. A sync fd can be waited on -// with poll(2) instead, which parks a thread rather than draining a GPU. -// -// *out_fence_fd is set to -1 whenever a descriptor is not available: an older library, a driver -// without VK_KHR_external_fence_fd, or work that had already completed by the time it was asked -// for. -1 is not an error and the return code is still ARMSX3_LSFG_OK -- the caller must fall -// back to armsx3_lsfg_wait_idle(), which is always correct. -// -// Added in ABI 3. Resolve it with dlsym rather than assuming it: this is the one entry point a -// caller can do without. -ARMSX3_LSFG_API int armsx3_lsfg_present_fenced(int32_t ctx, int in_sem, const int* out_sems, - uint32_t out_count, int* out_fence_fd); - ARMSX3_LSFG_API int armsx3_lsfg_destroy_context(int32_t ctx); // Read the user's own Lossless.dll and keep the shaders it contains. diff --git a/rpcs3/Emu/RSX/VK/VKFrameGen.cpp b/rpcs3/Emu/RSX/VK/VKFrameGen.cpp index 66687bc18..d69df9bb7 100644 --- a/rpcs3/Emu/RSX/VK/VKFrameGen.cpp +++ b/rpcs3/Emu/RSX/VK/VKFrameGen.cpp @@ -14,12 +14,6 @@ #include #include #include -// The completion signal framegen hands back is a sync file descriptor, not a VkFence we could -// wait on -- it renders on its own device. poll(2) waits on it, close(2) releases it, and errno -// is read to tell a signal-interrupted poll apart from a real failure. -#include -#include -#include #endif LOG_CHANNEL(framegen_log, "FRAMEGEN"); @@ -37,9 +31,6 @@ namespace vk::frame_gen int (*initialize)(uint64_t, int, float, uint64_t, int, armsx3_lsfg_shader_loader, void*) = nullptr; int32_t (*create_context_ahb)(void*, void*, void* const*, uint32_t, uint32_t, uint32_t, int32_t) = nullptr; int (*present)(int32_t, int, const int*, uint32_t) = nullptr; - // ABI 3 and later only, and the one entry point that is allowed to stay null: without - // it the present path keeps the device-wide wait it has always used. - int (*present_fenced)(int32_t, int, const int*, uint32_t, int*) = nullptr; int (*destroy_context)(int32_t) = nullptr; void (*wait_idle)() = nullptr; void (*finalize)() = nullptr; @@ -264,23 +255,6 @@ namespace vk::frame_gen return; } - // Resolved on its own, with dlsym rather than resolve(), because a null result here is - // not a failure: this entry point arrived in ABI 3 and the library is perfectly usable - // without it -- the present path simply keeps its vkDeviceWaitIdle. resolve() would log - // an error and, worse, fold into the `all` chain above and refuse the whole library. - // - // The version gate above already turns away anything that is not exactly this build's - // ABI, so in practice this is only null for a library that reports 3 and then does not - // export the symbol. Cheap enough to check rather than assume. - g_api.present_fenced = reinterpret_cast( - dlsym(g_api.handle, "armsx3_lsfg_present_fenced")); - - if (!g_api.present_fenced) - { - framegen_log.notice("Frame generation library has no fenced present; every generated " - "frame will cost a device wait"); - } - g_api.ok = true; framegen_log.success("Frame generation library loaded (ABI %u)", ARMSX3_LSFG_ABI_VERSION); } @@ -1040,74 +1014,6 @@ namespace vk::frame_gen g_context_outputs = 0; } - - // How long to wait on framegen's completion fence before giving up on it. - // - // Generation is a handful of compute dispatches over framebuffer-sized images; anything - // past this is not slow, it is wrong -- a lost device, or a descriptor that will never - // signal. The bound is what keeps a failure here a stall rather than a hang, and a frozen - // emulator is a far worse outcome than a frame of judder. - constexpr int k_fence_timeout_ms = 250; - - // Wait until the generation started by the present above has finished. - // - // Our device wrote the input images framegen reads, and there is no semaphore shared - // between the two devices, so something has to stand in for one. A sync fd is that - // something: poll(2) parks this thread until the fence framegen submitted signals, and - // framegen's queues keep running -- where the device wait below drains them entirely. - // - // Every path that does not end in a signalled descriptor falls back to that device wait, - // because it is always correct. The cost of being wrong here is not a slow frame, it is - // reading an image that is still being written. - void wait_for_generation(int fence_fd) - { - if (fence_fd < 0) - { - // No descriptor to wait on: a library with no fenced present, a driver without - // VK_KHR_external_fence_fd, or work that had already finished by the time framegen - // asked for the fd -- which the specification allows to come back as -1. All three - // want exactly what this line used to do unconditionally. - g_api.wait_idle(); - return; - } - - pollfd waiter{}; - waiter.fd = fence_fd; - waiter.events = POLLIN; - - int rc = 0; - - do - { - rc = ::poll(&waiter, 1, k_fence_timeout_ms); - } - while (rc < 0 && errno == EINTR); - - // A sync fd becomes readable when the fence it carries signals. Anything else -- a - // timeout (0), an error (< 0), or POLLERR/POLLNVAL on a descriptor that went away -- - // leaves us not knowing whether framegen has finished reading the inputs the next - // capture is about to overwrite, so take the wait that is always right. - if (rc <= 0 || !(waiter.revents & POLLIN)) - { - // Once, and only once. This runs on the present path: a line per frame would not - // be a diagnostic, it would be a second performance problem stacked on the first. - static bool s_warned = false; - - if (!s_warned) - { - s_warned = true; - framegen_log.warning("Frame generation fence did not signal within %dms (poll %d, revents 0x%x); " - "falling back to a device wait. Reported once per session.", - k_fence_timeout_ms, rc, static_cast(waiter.revents)); - } - - g_api.wait_idle(); - } - - // Ours to close either way -- framegen handed over ownership with the descriptor, and - // leaking one per frame would run the process out of file descriptors in minutes. - ::close(fence_fd); - } } u32 generated_frame_count() @@ -1272,41 +1178,25 @@ namespace vk::frame_gen } // Our device wrote the inputs; framegen's device is about to read them, and there is no - // semaphore shared between the two. Something has to stand in for one. + // semaphore shared between the two. A device-level wait is the only barrier available. // - // Upstream's semaphore path takes sync FDs and imports them as OPAQUE_FD - // (framegen/src/core/semaphore.cpp), which framegen's Android device deliberately does not - // enable -- vkImportSemaphoreFdKHR resolves to null inside it -- which is why every - // semaphore handed over below is -1. That left armsx3_lsfg_wait_idle(), a vkDeviceWaitIdle - // on framegen's whole device, as the only completion signal the library exposed, and it was - // paid once per presented frame. - // - // The fenced present replaces it with the fence framegen already submits alongside the - // generation work, exported as a sync fd: the same barrier, waited on with poll(2), with - // framegen's queues left running instead of drained. The plain present stays as the - // fallback for a library that does not export the new entry point, and - // wait_for_generation() falls back to the same device wait whenever no descriptor arrives - // or the poll does not complete -- neither may ever become a hang. - // - // The placement is unchanged and still matters: this runs before this frame's command - // buffer is submitted, on inputs that are a frame old, rather than after a full - // frame-completion wait. + // It cannot be pipelined away, and that is a property of the library rather than of this + // code: presentContext() submits on framegen's own device and the only completion signal it + // exposes is armsx3_lsfg_wait_idle(), a vkDeviceWaitIdle. Upstream's semaphore path takes + // sync FDs and imports them as OPAQUE_FD (framegen/src/core/semaphore.cpp), which Turnip and + // Mesa do not support on Android -- which is why every semaphore handed over below is -1. + // So what this costs is bounded by moving the wait, not by removing it: it now runs before + // this frame's command buffer is submitted, on inputs that are a frame old, rather than + // after a full frame-completion wait. int out_sems[3] = {-1, -1, -1}; - int fence_fd = -1; - const int rc = g_api.present_fenced - ? g_api.present_fenced(g_context, -1, out_sems, g_context_outputs, &fence_fd) - : g_api.present(g_context, -1, out_sems, g_context_outputs); - - if (rc != ARMSX3_LSFG_OK) + if (g_api.present(g_context, -1, out_sems, g_context_outputs) != ARMSX3_LSFG_OK) { - // fence_fd is still -1 on every failing path out of the shim, so there is nothing - // stranded here to close. disable(g_api.last_error()); return 0; } - wait_for_generation(fence_fd); + g_api.wait_idle(); // Report the real vs generated rate once a second. // diff --git a/rpcs3/Emu/RSX/VK/VKPresent.cpp b/rpcs3/Emu/RSX/VK/VKPresent.cpp index 0f1ecbec1..71b9a701f 100644 --- a/rpcs3/Emu/RSX/VK/VKPresent.cpp +++ b/rpcs3/Emu/RSX/VK/VKPresent.cpp @@ -232,29 +232,12 @@ vk::command_buffer_chunk* VKGSRender::present_generated_frame(VkImage src) // Zero timeout: if no swapchain image is free the display is already keeping up, and waiting // for one would make frame generation cost latency instead of adding smoothness. - // - // VK_SUBOPTIMAL_KHR has to be accepted, not treated as a failure. It is a SUCCESS code and the - // image IS acquired -- bailing on it returned without presenting, so the image was never handed - // back, and this driver reports SUBOPTIMAL as a standing condition rather than a one-off (see - // the same handling in present() below). That leaked one swapchain image per generated frame - // until the acquirable pool was empty, at which point the real frame's acquire in flip() blocks - // its full 100ms timeout every frame -- a hard lock at ten fps that only a swapchain rebuild - // clears, which is why it survived turning frame generation back off. - const VkResult acquire_result = - m_swapchain->acquire_next_swapchain_image(VK_NULL_HANDLE, 0ull, &image); - - if ((acquire_result != VK_SUCCESS && acquire_result != VK_SUBOPTIMAL_KHR) || image == umax) + if (m_swapchain->acquire_next_swapchain_image(VK_NULL_HANDLE, 0ull, &image) != VK_SUCCESS || + image == umax) { return nullptr; } - if (acquire_result == VK_SUBOPTIMAL_KHR) - { - // Worth acting on, but not from here: the rebuild happens on the real frame's path where - // there is somewhere to recover to. - should_reinitialize_swapchain = true; - } - auto* cmd = m_primary_cb_list.next(); cmd->reset(); cmd->begin(); @@ -289,36 +272,9 @@ vk::command_buffer_chunk* VKGSRender::present_generated_frame(VkImage src) // of this swapchain image. vk::queue_submit_t submit_info{}; submit_info.queue = m_device->get_graphics_queue(); + cmd->submit(submit_info); - // flush, because the present below runs on this thread while a deferred submit would not have - // happened yet under multithreaded RSX -- presenting a swapchain image before the blit that - // fills it. VKGSRender::present avoids this the same way, with a flush of its own. - cmd->submit(submit_info, VK_TRUE); - - // The result is not noise. On OUT_OF_DATE or SURFACE_LOST the image is NOT presented and is - // leaked exactly as an unaccepted SUBOPTIMAL above leaks one, and none of the recovery flags - // that present() sets would ever be raised, because generated frames never go through it. - switch (const VkResult present_result = m_swapchain->present(VK_NULL_HANDLE, image)) - { - case VK_SUCCESS: - break; - case VK_SUBOPTIMAL_KHR: - should_reinitialize_swapchain = true; - break; - case VK_ERROR_OUT_OF_DATE_KHR: - swapchain_unavailable = true; - break; - case VK_ERROR_SURFACE_LOST_KHR: - m_surface_lost = true; - swapchain_unavailable = true; - break; - default: - rsx_log.error("Generated-frame present returned %lld; treating the swapchain as lost.", - static_cast(present_result)); - swapchain_unavailable = true; - break; - } - + m_swapchain->present(VK_NULL_HANDLE, image); return cmd; }