gallium/u_threaded: buffer sharedness tracking

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 <marek.olsak@amd.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/17338>

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;
    }
 

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;