From 96cf4531e11eac8175671be990a02c7b5d17fb60 Mon Sep 17 00:00:00 2001 From: Pierre-Eric Pelloux-Prayer Date: Wed, 7 Jun 2023 10:28:23 +0200 Subject: [PATCH] Revert "gallium/u_threaded: buffer sharedness tracking" MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This reverts commit 8f159a8576efbb7bb3755d215a54b87c4c99a0d2. This commit is correct but it exposes an existing bug: DISCARD_RANGE doesn't work well with shared buffers. So for now revert this commit as it's causing hangs on some APUs (see https://gitlab.freedesktop.org/drm/amd/-/issues/2447) and flickering in Metro Last Light Redux. Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/9108 Reviewed-by: Marek Olšák Part-of: --- src/gallium/auxiliary/util/u_threaded_context.c | 67 +++---------------------- src/gallium/auxiliary/util/u_threaded_context.h | 17 ------- 2 files changed, 6 insertions(+), 78 deletions(-) diff --git a/src/gallium/auxiliary/util/u_threaded_context.c b/src/gallium/auxiliary/util/u_threaded_context.c index e48c90a..7472928 100644 --- a/src/gallium/auxiliary/util/u_threaded_context.c +++ b/src/gallium/auxiliary/util/u_threaded_context.c @@ -738,40 +738,10 @@ 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) { - struct threaded_resource *tbuf = threaded_resource(buf); - tc_touch_buffer(tc, tbuf); - - uint32_t id = tbuf->buffer_id_unique; + uint32_t id = threaded_resource(buf)->buffer_id_unique; BITSET_SET(next->buffer_list, id & TC_BUFFER_ID_MASK); } @@ -779,10 +749,7 @@ 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) { - struct threaded_resource *tbuf = threaded_resource(buf); - tc_touch_buffer(tc, tbuf); - - uint32_t id = tbuf->buffer_id_unique; + uint32_t id = threaded_resource(buf)->buffer_id_unique; *binding = id; BITSET_SET(next->buffer_list, id & TC_BUFFER_ID_MASK); } @@ -1040,8 +1007,6 @@ 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); @@ -2436,9 +2401,7 @@ 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. - * Note that callers must've called tc_touch_buffer before calling - * this function. */ +/* Return true if the buffer has been invalidated or is idle. */ static bool tc_invalidate_buffer(struct threaded_context *tc, struct threaded_resource *tbuf) @@ -2459,7 +2422,7 @@ tc_invalidate_buffer(struct threaded_context *tc, struct pipe_resource *new_buf; /* Shared, pinned, and sparse buffers can't be reallocated. */ - if (tc_is_buffer_shared(tbuf) || + if (tbuf->is_shared || tbuf->is_user_ptr || tbuf->b.flags & (PIPE_RESOURCE_FLAG_SPARSE | PIPE_RESOURCE_FLAG_UNMAPPABLE)) return false; @@ -2504,8 +2467,6 @@ 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, @@ -2620,14 +2581,6 @@ 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. */ @@ -2930,10 +2883,7 @@ tc_buffer_unmap(struct pipe_context *_pipe, struct pipe_transfer *transfer) assert(tres->cpu_storage); if (tres->cpu_storage) { - /* Invalidations shouldn't fail as long as CPU storage is allowed. */ - ASSERTED bool invalidated = tc_invalidate_buffer(tc, tres); - assert(invalidated); - + tc_invalidate_buffer(tc, tres); tc_buffer_subdata(&tc->base, &tres->b, PIPE_MAP_UNSYNCHRONIZED | TC_TRANSFER_MAP_UPLOAD_CPU_STORAGE, @@ -3061,8 +3011,6 @@ 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. */ @@ -4419,10 +4367,7 @@ tc_invalidate_resource(struct pipe_context *_pipe, struct threaded_context *tc = threaded_context(_pipe); if (resource->target == PIPE_BUFFER) { - /* 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); + tc_invalidate_buffer(tc, threaded_resource(resource)); return; } diff --git a/src/gallium/auxiliary/util/u_threaded_context.h b/src/gallium/auxiliary/util/u_threaded_context.h index 0e5b3f9..dbc5d69 100644 --- a/src/gallium/auxiliary/util/u_threaded_context.h +++ b/src/gallium/auxiliary/util/u_threaded_context.h @@ -317,17 +317,6 @@ 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- @@ -355,12 +344,6 @@ 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; -- 2.7.4