diff --git a/runtime/CMakeLists.txt b/runtime/CMakeLists.txt index 4e645678..69f7b9f8 100644 --- a/runtime/CMakeLists.txt +++ b/runtime/CMakeLists.txt @@ -795,6 +795,8 @@ if(BUILD_TESTING) add_test(NAME pad_external_input_test COMMAND pad_external_input_test) add_executable(host_overlay_test tests/test_host_overlay.c src/host_overlay.c) target_include_directories(host_overlay_test PRIVATE include src) + find_package(Threads REQUIRED) + target_link_libraries(host_overlay_test PRIVATE Threads::Threads) add_test(NAME host_overlay_test COMMAND host_overlay_test) add_executable(gte_register_access_test diff --git a/runtime/include/host_overlay.h b/runtime/include/host_overlay.h index b144c5a1..89e44d62 100644 --- a/runtime/include/host_overlay.h +++ b/runtime/include/host_overlay.h @@ -30,7 +30,15 @@ void psx_host_pause_set_refuse_probe(PsxHostPauseRefuseFn fn); * swap, with the drawable size in pixels. It runs on the thread that owns the * GL context (the render thread when it is on, else the main thread), with the * window framebuffer bound. The callback must leave GL state as it found it. - * Vulkan and the SDL renderer never call it. NULL unregisters. */ + * Vulkan and the SDL renderer never call it. NULL unregisters. + * + * Lifetime contract: the registrant owns ctx. psx_host_overlay_set_draw_cb + * (replace or NULL) returns only after any draw call that may still be using + * the previous fn/ctx has finished, so the registrant may free ctx and the GL + * objects it draws with right after it returns, and must keep them alive + * until then. Unregister before destroying those objects or the GL context + * (e.g. on renderer shutdown). Called from inside the callback it does not + * wait; the current call is then the last one with the old ctx. */ typedef void (*PsxHostOverlayDrawFn)(int width, int height, void *ctx); void psx_host_overlay_set_draw_cb(PsxHostOverlayDrawFn fn, void *ctx); int psx_host_overlay_has_draw(void); diff --git a/runtime/src/host_overlay.c b/runtime/src/host_overlay.c index 9374c11e..3b6f1223 100644 --- a/runtime/src/host_overlay.c +++ b/runtime/src/host_overlay.c @@ -4,6 +4,11 @@ #include #include #include +#if defined(_WIN32) +#include +#else +#include +#endif /* ---- P1: host pause ------------------------------------------------------ */ #define PSX_HOST_PAUSE_MAX 8 @@ -33,26 +38,50 @@ void psx_host_pause_set_refuse_probe(PsxHostPauseRefuseFn fn) { s_pause_refuse = /* ---- P2: overlay draw callback -------------------------------------------- */ /* Registered on the main thread, read on the render / present thread. The * function and its context are published together as one immutable record - * behind an atomic pointer (release on store, acquire on load), so a reader - * can never pair a live function with a stale or NULL context. Records are - * never freed: registration happens a handful of times per process, and a - * reader may still hold the previous record when it is replaced. */ + * behind an atomic pointer, so a reader never pairs a live function with a + * stale context. + * + * Lifetime: every call runs inside an in-flight count. Replacing or removing + * the callback swaps the record and then waits until no call that could have + * read the old record is still running, so when psx_host_overlay_set_draw_cb + * returns, the previous fn will not run again and its ctx (and the GL objects + * it draws with) may be freed by the caller. The old record is freed there + * too. A change made from inside the callback itself does not wait (it would + * wait for itself); that caller's own frame is the only one still using the + * old ctx and it returns before the next frame reads the new record. */ typedef struct { PsxHostOverlayDrawFn fn; void *ctx; } PsxHostOverlayDraw; static _Atomic(PsxHostOverlayDraw *) s_draw = NULL; +static atomic_int s_draw_inflight = 0; +#if defined(_MSC_VER) +static __declspec(thread) int s_in_draw = 0; +#else +static _Thread_local int s_in_draw = 0; +#endif + +static void draw_yield(void) { +#if defined(_WIN32) + Sleep(0); +#else + sched_yield(); +#endif +} void psx_host_overlay_set_draw_cb(PsxHostOverlayDrawFn fn, void *ctx) { - PsxHostOverlayDraw *rec = NULL; + PsxHostOverlayDraw *rec = NULL, *old; if (fn) { rec = (PsxHostOverlayDraw *)malloc(sizeof(*rec)); if (!rec) return; rec->fn = fn; rec->ctx = ctx; } - atomic_store_explicit(&s_draw, rec, memory_order_release); + old = atomic_exchange(&s_draw, rec); /* seq_cst: ordered with the readers' count */ + if (s_in_draw) return; /* from inside the callback: see above; old stays (rare) */ + while (atomic_load(&s_draw_inflight) > 0) draw_yield(); + free(old); } int psx_host_overlay_has_draw(void) { @@ -60,6 +89,15 @@ int psx_host_overlay_has_draw(void) { } void psx_host_overlay_draw(int width, int height) { - const PsxHostOverlayDraw *rec = atomic_load_explicit(&s_draw, memory_order_acquire); - if (rec && rec->fn && width > 0 && height > 0) rec->fn(width, height, rec->ctx); + PsxHostOverlayDraw *rec; + if (width <= 0 || height <= 0) return; + atomic_fetch_add(&s_draw_inflight, 1); /* seq_cst, before reading the record */ + rec = atomic_load(&s_draw); + if (rec && rec->fn) { + s_in_draw = 1; + rec->fn(width, height, rec->ctx); + s_in_draw = 0; + } + atomic_fetch_sub(&s_draw_inflight, 1); } + diff --git a/runtime/tests/test_host_overlay.c b/runtime/tests/test_host_overlay.c index 17d6bd6d..cf2ad265 100644 --- a/runtime/tests/test_host_overlay.c +++ b/runtime/tests/test_host_overlay.c @@ -2,6 +2,7 @@ #include "host_overlay.h" #include +#include #include static int g_fail = 0; @@ -50,7 +51,69 @@ static void test_draw(void) { CHECK(draws == 1 && !psx_host_overlay_has_draw()); } +/* Lifetime: once set_draw_cb(NULL) returns, the old callback never runs + * again, so its context can be freed. A reader thread draws continuously; + * the main thread unregisters and immediately marks the context dead. */ +#if !defined(_WIN32) || defined(__MINGW32__) +#include +#include +typedef struct { atomic_int alive; atomic_int calls; atomic_int used_dead; } LifeCtx; +static atomic_int g_stop; +static void life_draw(int w, int h, void* p) { + LifeCtx* c = (LifeCtx*)p; + (void)w; (void)h; + for (volatile int i = 0; i < 2000; ++i) {} /* a frame's worth of work */ + if (!atomic_load(&c->alive)) atomic_fetch_add(&c->used_dead, 1); + atomic_fetch_add(&c->calls, 1); +} +static void* life_reader(void* arg) { + (void)arg; + while (!atomic_load(&g_stop)) psx_host_overlay_draw(64, 64); + return NULL; +} +static void test_draw_lifetime(void) { + pthread_t t; + atomic_store(&g_stop, 0); + pthread_create(&t, NULL, life_reader, NULL); + int dead_uses = 0, total = 0; + for (int round = 0; round < 300; ++round) { + LifeCtx* c = (LifeCtx*)calloc(1, sizeof(*c)); + atomic_store(&c->alive, 1); + psx_host_overlay_set_draw_cb(life_draw, c); + while (atomic_load(&c->calls) < 2) {} + psx_host_overlay_set_draw_cb(NULL, NULL); /* must wait for the reader */ + atomic_store(&c->alive, 0); /* the owner tears down */ + for (volatile int i = 0; i < 20000; ++i) {} + dead_uses += atomic_load(&c->used_dead); + total += atomic_load(&c->calls); + free(c); + } + atomic_store(&g_stop, 1); + pthread_join(t, NULL); + CHECK(total > 0); + CHECK(dead_uses == 0); +} +#else +static void test_draw_lifetime(void) {} +#endif + +/* Changing the callback from inside it must not deadlock. */ +static void self_unregister(int w, int h, void* p) { + (void)w; (void)h; + ++*(int*)p; + psx_host_overlay_set_draw_cb(NULL, NULL); +} +static void test_draw_reentrant(void) { + int n = 0; + psx_host_overlay_set_draw_cb(self_unregister, &n); + psx_host_overlay_draw(64, 64); + psx_host_overlay_draw(64, 64); + CHECK(n == 1 && !psx_host_overlay_has_draw()); +} + int main(void) { + test_draw_lifetime(); + test_draw_reentrant(); test_draw(); test_pause(); printf("host_overlay_test: %s\n", g_fail ? "FAIL" : "ok");