Commit 63d54d0d authored by Alyssa Rosenzweig's avatar Alyssa Rosenzweig 💜
Browse files

panfrost: Protect the variants array with a lock



Without a lock, two threads may bind the same shader CSO simultaneously,
allocate the same variant simultaneously, and then race each other in
the compiler. This manifests in various ways, most commonly failing the
assertion that UBO pushing has only run once. The simple_mtx_t solution
is used in Iris. Fixes the crash in:

dEQP-EGL.functional.sharing.gles2.multithread.simple.buffers.bufferdata_render
Signed-off-by: Alyssa Rosenzweig's avatarAlyssa Rosenzweig <alyssa@collabora.com>
Cc: mesa-stable
parent 49d69528
Pipeline #380308 waiting for manual action with stages
...@@ -298,6 +298,8 @@ panfrost_create_shader_state( ...@@ -298,6 +298,8 @@ panfrost_create_shader_state(
struct panfrost_device *dev = pan_device(pctx->screen); struct panfrost_device *dev = pan_device(pctx->screen);
so->base = *cso; so->base = *cso;
simple_mtx_init(&so->lock, mtx_plain);
/* Token deep copy to prevent memory corruption */ /* Token deep copy to prevent memory corruption */
if (cso->type == PIPE_SHADER_IR_TGSI) if (cso->type == PIPE_SHADER_IR_TGSI)
...@@ -338,6 +340,8 @@ panfrost_delete_shader_state( ...@@ -338,6 +340,8 @@ panfrost_delete_shader_state(
panfrost_bo_unreference(shader_state->linkage.bo); panfrost_bo_unreference(shader_state->linkage.bo);
} }
simple_mtx_destroy(&cso->lock);
free(cso->variants); free(cso->variants);
free(so); free(so);
} }
...@@ -456,6 +460,8 @@ panfrost_bind_shader_state( ...@@ -456,6 +460,8 @@ panfrost_bind_shader_state(
signed variant = -1; signed variant = -1;
struct panfrost_shader_variants *variants = (struct panfrost_shader_variants *) hwcso; struct panfrost_shader_variants *variants = (struct panfrost_shader_variants *) hwcso;
simple_mtx_lock(&variants->lock);
for (unsigned i = 0; i < variants->variant_count; ++i) { for (unsigned i = 0; i < variants->variant_count; ++i) {
if (panfrost_variant_matches(ctx, &variants->variants[i], type)) { if (panfrost_variant_matches(ctx, &variants->variants[i], type)) {
variant = i; variant = i;
...@@ -536,6 +542,11 @@ panfrost_bind_shader_state( ...@@ -536,6 +542,11 @@ panfrost_bind_shader_state(
update_so_info(&shader_state->stream_output, update_so_info(&shader_state->stream_output,
shader_state->info.outputs_written); shader_state->info.outputs_written);
} }
/* TODO: it would be more efficient to release the lock before
* compiling instead of after, but that can race if thread A compiles a
* variant while thread B searches for that same variant */
simple_mtx_unlock(&variants->lock);
} }
static void * static void *
......
...@@ -44,6 +44,7 @@ ...@@ -44,6 +44,7 @@
#include "pipe/p_state.h" #include "pipe/p_state.h"
#include "util/u_blitter.h" #include "util/u_blitter.h"
#include "util/hash_table.h" #include "util/hash_table.h"
#include "util/simple_mtx.h"
#include "midgard/midgard_compile.h" #include "midgard/midgard_compile.h"
#include "compiler/shader_enums.h" #include "compiler/shader_enums.h"
...@@ -290,6 +291,9 @@ struct panfrost_shader_variants { ...@@ -290,6 +291,9 @@ struct panfrost_shader_variants {
struct pipe_compute_state cbase; struct pipe_compute_state cbase;
}; };
/** Lock for the variants array */
simple_mtx_t lock;
struct panfrost_shader_state *variants; struct panfrost_shader_state *variants;
unsigned variant_space; unsigned variant_space;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment