mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-08 17:28:49 -10:00
GFX/THUMBNAIL: Fix data race on thumbnail state with threaded video
gfx_thumbnail_handle_upload() runs on the main thread (via the task
queue's gather/callback path) and writes texture, width, height,
then status into a gfx_thumbnail_t. gfx_thumbnail_draw() runs on
the video thread under threaded video and reads the same fields.
On architectures with weak memory ordering (ARM64: Switch, Android),
the status write can become visible to the video thread before the
texture/width/height writes, causing gfx_thumbnail_draw() to
attempt rendering with stale or zero values. This results in
crashes or rendering corruption, particularly with Vulkan and D3D12
backends where invalid texture handles trigger driver-level faults.
Fix: release/acquire ordering on the status field, plus field
snapshots in the draw function.
Write side (gfx_thumbnail_handle_upload):
- Both status writes (GFX_THUMBNAIL_STATUS_MISSING at the default
fallback path, GFX_THUMBNAIL_STATUS_AVAILABLE at the success
path) now use GFX_THUMB_STATUS_STORE, a release-store macro.
This ensures all prior stores (texture handle from
video_driver_texture_load, cached width/height) are committed
to memory before the status value becomes visible to other
threads.
Read side (gfx_thumbnail_draw):
- The status check now uses GFX_THUMB_STATUS_LOAD, an acquire-load
macro. If the video thread sees AVAILABLE, all data fields
written before the release-store are guaranteed visible.
- Immediately after the acquire-load confirms AVAILABLE, all
referenced fields (texture, width, height, alpha, flags) are
copied into stack locals. Only these snapshots are used for
the remainder of the draw call — the live struct is not touched
again.
- gfx_thumbnail_get_draw_dimensions() receives a stack-local
gfx_thumbnail_t built from the snapshots, so it also operates
on consistent data without signature changes.
- draw.texture uses the snapshotted handle, not the live field.
Platform support:
GCC/Clang: __atomic_store_n / __atomic_load_n with
__ATOMIC_RELEASE / __ATOMIC_ACQUIRE.
Compiles to STLR/LDAR on ARM64 (single-cycle),
plain MOV on x86 (free — TSO provides the
guarantee), LWSYNC/LD on PowerPC.
MSVC: _InterlockedExchange / _InterlockedOr.
Only targets x86/x64 in RetroArch's matrix.
Fallback: Plain volatile read/write. Sufficient on
single-core platforms and x86. Platforms with
weak ordering that lack these builtins do not
run threaded video.
No external call site changes. No signature changes.
No behavioral or visual changes. Stack cost is ~32 bytes
(5 snapshot locals + 1 temp gfx_thumbnail_t in a nested scope).
This commit is contained in:
1 parent
0abe6dc47d
commit
50931c0a84
1 file changed
+81
-12
+81
-12
@@ -43,6 +43,40 @@
|
||||
#define DEFAULT_GFX_THUMBNAIL_STREAM_DELAY 16.66667f * 3
|
||||
#define DEFAULT_GFX_THUMBNAIL_FADE_DURATION 166.66667f
|
||||
|
||||
/* Helpers for cross-thread thumbnail status synchronisation.
|
||||
*
|
||||
* The main thread writes texture/width/height then publishes
|
||||
* status = AVAILABLE. The video thread reads status; if it
|
||||
* sees AVAILABLE, the data fields must be visible.
|
||||
*
|
||||
* On GCC/Clang (covers ARM64, x86, PowerPC, MIPS):
|
||||
* release-store / acquire-load give the required ordering.
|
||||
* On MSVC (x86/x64 only in RetroArch's target matrix):
|
||||
* x86 total-store-order makes plain volatile sufficient,
|
||||
* but _InterlockedOr gives an explicit acquire load.
|
||||
* Fallback:
|
||||
* Plain volatile read/write — sufficient on single-core
|
||||
* and x86. Platforms with weak ordering that lack GCC
|
||||
* builtins do not run threaded video in practice.
|
||||
*/
|
||||
#if defined(__GNUC__)
|
||||
#define GFX_THUMB_STATUS_STORE(ptr, val) \
|
||||
__atomic_store_n((int*)(ptr), (int)(val), __ATOMIC_RELEASE)
|
||||
#define GFX_THUMB_STATUS_LOAD(ptr) \
|
||||
((enum gfx_thumbnail_status)__atomic_load_n((int*)(ptr), __ATOMIC_ACQUIRE))
|
||||
#elif defined(_MSC_VER)
|
||||
#include <intrin.h>
|
||||
#define GFX_THUMB_STATUS_STORE(ptr, val) \
|
||||
_InterlockedExchange((volatile long*)(ptr), (long)(val))
|
||||
#define GFX_THUMB_STATUS_LOAD(ptr) \
|
||||
((enum gfx_thumbnail_status)_InterlockedOr((volatile long*)(ptr), 0))
|
||||
#else
|
||||
#define GFX_THUMB_STATUS_STORE(ptr, val) \
|
||||
do { (*(volatile int*)(ptr)) = (int)(val); } while(0)
|
||||
#define GFX_THUMB_STATUS_LOAD(ptr) \
|
||||
((enum gfx_thumbnail_status)(*(volatile int*)(ptr)))
|
||||
#endif
|
||||
|
||||
/* Utility structure, sent as userdata when pushing
|
||||
* an image load */
|
||||
typedef struct
|
||||
@@ -174,8 +208,11 @@ static void gfx_thumbnail_handle_upload(
|
||||
gfx_thumbnail_reset(thumbnail_tag->thumbnail);
|
||||
|
||||
/* Set thumbnail 'missing' status by default
|
||||
* (saves a number of checks later) */
|
||||
thumbnail_tag->thumbnail->status = GFX_THUMBNAIL_STATUS_MISSING;
|
||||
* (saves a number of checks later)
|
||||
* > Release-store ensures prior texture reset is
|
||||
* visible before status change */
|
||||
GFX_THUMB_STATUS_STORE(&thumbnail_tag->thumbnail->status,
|
||||
GFX_THUMBNAIL_STATUS_MISSING);
|
||||
|
||||
/* If we reach this stage, thumbnail 'fade in'
|
||||
* animations should be applied (based on current
|
||||
@@ -196,8 +233,12 @@ static void gfx_thumbnail_handle_upload(
|
||||
thumbnail_tag->thumbnail->width = img->width;
|
||||
thumbnail_tag->thumbnail->height = img->height;
|
||||
|
||||
/* Update thumbnail status */
|
||||
thumbnail_tag->thumbnail->status = GFX_THUMBNAIL_STATUS_AVAILABLE;
|
||||
/* Update thumbnail status
|
||||
* > Release-store ensures texture/width/height writes
|
||||
* are visible to the video thread before it sees
|
||||
* AVAILABLE via acquire-load in gfx_thumbnail_draw() */
|
||||
GFX_THUMB_STATUS_STORE(&thumbnail_tag->thumbnail->status,
|
||||
GFX_THUMBNAIL_STATUS_AVAILABLE);
|
||||
|
||||
end:
|
||||
/* Clean up */
|
||||
@@ -945,8 +986,12 @@ void gfx_thumbnail_draw(
|
||||
)
|
||||
return;
|
||||
|
||||
/* Only draw thumbnail if it is available... */
|
||||
if (thumbnail->status == GFX_THUMBNAIL_STATUS_AVAILABLE)
|
||||
/* Only draw thumbnail if it is available...
|
||||
* > Acquire-load pairs with release-store in
|
||||
* gfx_thumbnail_handle_upload() to ensure
|
||||
* texture/width/height are visible when we
|
||||
* read AVAILABLE */
|
||||
if (GFX_THUMB_STATUS_LOAD(&thumbnail->status) == GFX_THUMBNAIL_STATUS_AVAILABLE)
|
||||
{
|
||||
gfx_display_ctx_draw_t draw;
|
||||
struct video_coords coords;
|
||||
@@ -955,7 +1000,18 @@ void gfx_thumbnail_draw(
|
||||
float draw_height;
|
||||
float draw_x;
|
||||
float draw_y;
|
||||
float thumbnail_alpha = thumbnail->alpha * alpha;
|
||||
|
||||
/* Snapshot fields — read once, use local copies for the
|
||||
* remainder of this draw call. The main thread may update
|
||||
* the live struct concurrently (upload callback, reset, or
|
||||
* animation tick on alpha), but these locals are stable. */
|
||||
uintptr_t thumb_texture = thumbnail->texture;
|
||||
unsigned thumb_width = thumbnail->width;
|
||||
unsigned thumb_height = thumbnail->height;
|
||||
float thumb_alpha = thumbnail->alpha;
|
||||
uint8_t thumb_flags = thumbnail->flags;
|
||||
|
||||
float thumbnail_alpha = thumb_alpha * alpha;
|
||||
float thumbnail_color[16] = {
|
||||
1.0f, 1.0f, 1.0f, 1.0f,
|
||||
1.0f, 1.0f, 1.0f, 1.0f,
|
||||
@@ -969,10 +1025,23 @@ void gfx_thumbnail_draw(
|
||||
if (thumbnail_alpha < 1.0f)
|
||||
gfx_display_set_alpha(thumbnail_color, thumbnail_alpha);
|
||||
|
||||
/* Get thumbnail dimensions */
|
||||
gfx_thumbnail_get_draw_dimensions(
|
||||
thumbnail, width, height, scale_factor,
|
||||
&draw_width, &draw_height);
|
||||
/* Get thumbnail dimensions using snapshot values
|
||||
* > Build a temporary on the stack so
|
||||
* gfx_thumbnail_get_draw_dimensions reads
|
||||
* consistent data without touching the live struct */
|
||||
{
|
||||
gfx_thumbnail_t thumb_snapshot;
|
||||
thumb_snapshot.texture = thumb_texture;
|
||||
thumb_snapshot.width = thumb_width;
|
||||
thumb_snapshot.height = thumb_height;
|
||||
thumb_snapshot.alpha = thumb_alpha;
|
||||
thumb_snapshot.flags = thumb_flags;
|
||||
thumb_snapshot.status = GFX_THUMBNAIL_STATUS_AVAILABLE;
|
||||
thumb_snapshot.delay_timer = 0.0f;
|
||||
gfx_thumbnail_get_draw_dimensions(
|
||||
&thumb_snapshot, width, height, scale_factor,
|
||||
&draw_width, &draw_height);
|
||||
}
|
||||
|
||||
if (dispctx->blend_begin)
|
||||
dispctx->blend_begin(userdata);
|
||||
@@ -1006,7 +1075,7 @@ void gfx_thumbnail_draw(
|
||||
draw.rotation = 0.0f;
|
||||
draw.coords = &coords;
|
||||
draw.matrix_data = &mymat;
|
||||
draw.texture = thumbnail->texture;
|
||||
draw.texture = thumb_texture;
|
||||
draw.prim_type = GFX_DISPLAY_PRIM_TRIANGLESTRIP;
|
||||
draw.pipeline_id = 0;
|
||||
|
||||
|
||||
Reference in new issue
Block a user