From 8f159a8576efbb7bb3755d215a54b87c4c99a0d2 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Wed, 13 Jul 2022 21:11:33 +0200 Subject: [PATCH] gallium/u_threaded: buffer sharedness tracking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes TC's buffer invalidation code for buffers that are shared between contexts. TC is unable to notify other TCs in case it replaces a shared buffer's underlying storage when invalidating, causing those other TCs to use wrong buffer IDs for busyness tracking, which leads to corruption due to invalidation fast-paths being triggered when they shouldn't be. This patch addresses this issue by tracking if a buffer is shared, and if it is, disabling buffer storage replacement for the affected buffer. This is achieved by tracking which TC instance first accessed a certain buffer. If a second instance then accesses it as well, it will realize that it isn't the only one working on the buffer and mark the buffer accordingly. If TC needs to invalidate a buffer for the correctness of an operation at any point, it will fall back to doing the operation in a synchronous fashion with this patch if the buffer is shared and currently busy. It might be possible to later detect that a buffer has become un-shared; however, this is outside of the scope of this bugfix patch. v2: Do not disable buffer busyness tracking for shared buffers. Fixes: e9c41b32 ("gallium/u_threaded: add buffer lists - tracking of buffers referenced by tc") Reviewed-by: Marek Olšák Part-of: --- .../auxiliary/util/u_threaded_context.c | 67 +++++++++++++++++-- .../auxiliary/util/u_threaded_context.h | 17 +++++ 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/src/gallium/auxiliary/util/u_threaded_context.c b/src/gallium/auxiliary/util/u_threaded_context.c index 5db25b724dc..fcd65afdcf7 100644 --- a/src/gallium/auxiliary/util/u_threaded_context.c +++ b/src/gallium/auxiliary/util/u_threaded_context.c @@ -458,10 +458,40 @@ threaded_context_flush(struct pipe_context *_pipe, } } +/* Must be called before TC binds, maps, invalidates, or adds a buffer to a buffer list. */ +static void tc_touch_buffer(struct threaded_context *tc, struct threaded_resource *buf) +{ + const struct threaded_context *first_user = buf->first_user; + + /* Fast path exit to avoid additional branches */ + if (likely(first_user == tc)) + return; + + if (!first_user) + first_user = p_atomic_cmpxchg_ptr(&buf->first_user, NULL, tc); + + /* The NULL check might seem unnecessary here but it's actually critical: + * p_atomic_cmpxchg will return NULL if it succeeds, meaning that NULL is + * equivalent to "we're the first user" here. (It's equally important not + * to ignore the result of the cmpxchg above, since it might fail.) + * Without the NULL check, we'd set the flag unconditionally, which is bad. + */ + if (first_user && first_user != tc && !buf->used_by_multiple_contexts) + buf->used_by_multiple_contexts = true; +} + +static bool tc_is_buffer_shared(struct threaded_resource *buf) +{ + return buf->is_shared || buf->used_by_multiple_contexts; +} + static void tc_add_to_buffer_list(struct threaded_context *tc, struct tc_buffer_list *next, struct pipe_resource *buf) { - uint32_t id = threaded_resource(buf)->buffer_id_unique; + struct threaded_resource *tbuf = threaded_resource(buf); + tc_touch_buffer(tc, tbuf); + + uint32_t id = tbuf->buffer_id_unique; BITSET_SET(next->buffer_list, id & TC_BUFFER_ID_MASK); } @@ -469,7 +499,10 @@ tc_add_to_buffer_list(struct threaded_context *tc, struct tc_buffer_list *next, static void tc_bind_buffer(struct threaded_context *tc, uint32_t *binding, struct tc_buffer_list *next, struct pipe_resource *buf) { - uint32_t id = threaded_resource(buf)->buffer_id_unique; + struct threaded_resource *tbuf = threaded_resource(buf); + tc_touch_buffer(tc, tbuf); + + uint32_t id = tbuf->buffer_id_unique; *binding = id; BITSET_SET(next->buffer_list, id & TC_BUFFER_ID_MASK); } @@ -727,6 +760,8 @@ threaded_resource_init(struct pipe_resource *res, bool allow_cpu_storage) { struct threaded_resource *tres = threaded_resource(res); + tres->first_user = NULL; + tres->used_by_multiple_contexts = false; tres->latest = &tres->b; tres->cpu_storage = NULL; util_range_init(&tres->valid_buffer_range); @@ -2048,7 +2083,9 @@ tc_call_replace_buffer_storage(struct pipe_context *pipe, void *call, uint64_t * return call_size(tc_replace_buffer_storage); } -/* Return true if the buffer has been invalidated or is idle. */ +/* Return true if the buffer has been invalidated or is idle. + * Note that callers must've called tc_touch_buffer before calling + * this function. */ static bool tc_invalidate_buffer(struct threaded_context *tc, struct threaded_resource *tbuf) @@ -2069,7 +2106,7 @@ tc_invalidate_buffer(struct threaded_context *tc, struct pipe_resource *new_buf; /* Shared, pinned, and sparse buffers can't be reallocated. */ - if (tbuf->is_shared || + if (tc_is_buffer_shared(tbuf) || tbuf->is_user_ptr || tbuf->b.flags & (PIPE_RESOURCE_FLAG_SPARSE | PIPE_RESOURCE_FLAG_UNMAPPABLE)) return false; @@ -2114,6 +2151,8 @@ tc_invalidate_buffer(struct threaded_context *tc, return true; } +/* Note that callers must've called tc_touch_buffer first before + * calling tc_improve_map_buffer_flags. */ static unsigned tc_improve_map_buffer_flags(struct threaded_context *tc, struct threaded_resource *tres, unsigned usage, @@ -2228,6 +2267,14 @@ tc_buffer_map(struct pipe_context *_pipe, if (usage & PIPE_MAP_THREAD_SAFE) tc_buffer_disable_cpu_storage(resource); + tc_touch_buffer(tc, tres); + + /* CPU storage relies on buffer invalidation never failing. With shared buffers, + * invalidation might not always be possible, so CPU storage can't be used. + */ + if (tc_is_buffer_shared(tres)) + tc_buffer_disable_cpu_storage(resource); + usage = tc_improve_map_buffer_flags(tc, tres, usage, box->x, box->width); /* If the CPU storage is enabled, return it directly. */ @@ -2508,7 +2555,10 @@ tc_buffer_unmap(struct pipe_context *_pipe, struct pipe_transfer *transfer) assert(tres->cpu_storage); if (tres->cpu_storage) { - tc_invalidate_buffer(tc, tres); + /* Invalidations shouldn't fail as long as CPU storage is allowed. */ + ASSERTED bool invalidated = tc_invalidate_buffer(tc, tres); + assert(invalidated); + tc_buffer_subdata(&tc->base, &tres->b, PIPE_MAP_UNSYNCHRONIZED | TC_TRANSFER_MAP_UPLOAD_CPU_STORAGE, @@ -2636,6 +2686,8 @@ tc_buffer_subdata(struct pipe_context *_pipe, if (!size) return; + tc_touch_buffer(tc, tres); + usage |= PIPE_MAP_WRITE; /* PIPE_MAP_DIRECTLY supresses implicit DISCARD_RANGE. */ @@ -3912,7 +3964,10 @@ tc_invalidate_resource(struct pipe_context *_pipe, struct threaded_context *tc = threaded_context(_pipe); if (resource->target == PIPE_BUFFER) { - tc_invalidate_buffer(tc, threaded_resource(resource)); + /* This can fail, in which case we simply ignore the invalidation request. */ + struct threaded_resource *tbuf = threaded_resource(resource); + tc_touch_buffer(tc, tbuf); + tc_invalidate_buffer(tc, tbuf); return; } diff --git a/src/gallium/auxiliary/util/u_threaded_context.h b/src/gallium/auxiliary/util/u_threaded_context.h index 95aa55e724f..597a5b22de2 100644 --- a/src/gallium/auxiliary/util/u_threaded_context.h +++ b/src/gallium/auxiliary/util/u_threaded_context.h @@ -315,6 +315,17 @@ typedef bool (*tc_is_resource_busy)(struct pipe_screen *screen, struct threaded_resource { struct pipe_resource b; + /* Pointer to the TC that first used this threaded_resource (buffer). This is used to + * allow TCs to determine whether they have been given a buffer that was created by a + * different TC, in which case all TCs have to disable busyness tracking and buffer + * replacement for that particular buffer. + * DO NOT DEREFERENCE. The only operation allowed on this pointer is equality-checking + * since it might be dangling if a buffer has been shared and its first_user has + * already been destroyed. The pointer is const void to discourage such disallowed usage. + * This is NULL if no TC has used this buffer yet. + */ + const void *first_user; + /* Since buffer invalidations are queued, we can't use the base resource * for unsychronized mappings. This points to the latest version of * the buffer after the latest invalidation. It's only used for unsychro- @@ -342,6 +353,12 @@ struct threaded_resource { */ struct util_range valid_buffer_range; + /* True if multiple threaded contexts have accessed this buffer. + * Disables non-multicontext-safe optimizations in TC. + * We can't just re-use is_shared for that purpose as that would confuse drivers. + */ + bool used_by_multiple_contexts; + /* Drivers are required to update this for shared resources and user * pointers. */ bool is_shared;