From 637240d824051b8b99f35c165cabe31106612f2a Mon Sep 17 00:00:00 2001 From: =?utf8?q?Nicolai=20H=C3=A4hnle?= Date: Sun, 22 Oct 2017 17:38:36 +0200 Subject: [PATCH] st/mesa: guard sampler views changes with a mutex MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Some locking is unfortunately required, because well-formed GL programs can have multiple threads racing to access the same texture, e.g.: two threads/contexts rendering from the same texture, or one thread destroying a context while the other is rendering from or modifying a texture. Since even the simple mutex caused noticable slowdowns in the piglit drawoverhead micro-benchmark, this patch uses a slightly more involved approach to keep locks out of the fast path: - the initial lookup of sampler views happens without taking a lock - a per-texture lock is only taken when we have to modify the sampler view(s) - since each thread mostly operates only on the entry corresponding to its context, the main issue is re-allocation of the sampler view array when it needs to be grown, but the old copy is not freed Old copies of the sampler views array are kept around in a linked list until the entire texture object is deleted. The total memory wasted in this way is roughly equal to the size of the current sampler views array. Fixes non-deterministic memory corruption in some dEQP-EGL.functional.sharing.gles2.multithread.* tests, e.g. dEQP-EGL.functional.sharing.gles2.multithread.simple.images.texture_source.create_texture_render Reviewed-by: Marek Olšák --- src/mesa/state_tracker/st_atom_sampler.c | 21 +- src/mesa/state_tracker/st_cb_texture.c | 12 + src/mesa/state_tracker/st_sampler_view.c | 270 ++++++++++++++++------- src/mesa/state_tracker/st_sampler_view.h | 3 + src/mesa/state_tracker/st_texture.h | 40 +++- 5 files changed, 250 insertions(+), 96 deletions(-) diff --git a/src/mesa/state_tracker/st_atom_sampler.c b/src/mesa/state_tracker/st_atom_sampler.c index ff3f49fa4e4..6d83f963f0a 100644 --- a/src/mesa/state_tracker/st_atom_sampler.c +++ b/src/mesa/state_tracker/st_atom_sampler.c @@ -43,6 +43,7 @@ #include "st_cb_texture.h" #include "st_format.h" #include "st_atom.h" +#include "st_sampler_view.h" #include "st_texture.h" #include "pipe/p_context.h" #include "pipe/p_defines.h" @@ -164,26 +165,18 @@ st_convert_sampler(const struct st_context *st, if (st->apply_texture_swizzle_to_border_color) { const struct st_texture_object *stobj = st_texture_object_const(texobj); - const struct pipe_sampler_view *sv = NULL; - - /* Just search for the first used view. We can do this because the - swizzle is per-texture, not per context. */ /* XXX: clean that up to not use the sampler view at all */ - for (unsigned i = 0; i < stobj->num_sampler_views; ++i) { - if (stobj->sampler_views[i].view) { - sv = stobj->sampler_views[i].view; - break; - } - } + const struct st_sampler_view *sv = st_texture_get_current_sampler_view(st, stobj); if (sv) { + struct pipe_sampler_view *view = sv->view; union pipe_color_union tmp; const unsigned char swz[4] = { - sv->swizzle_r, - sv->swizzle_g, - sv->swizzle_b, - sv->swizzle_a, + view->swizzle_r, + view->swizzle_g, + view->swizzle_b, + view->swizzle_a, }; st_translate_color(&msamp->BorderColor, &tmp, diff --git a/src/mesa/state_tracker/st_cb_texture.c b/src/mesa/state_tracker/st_cb_texture.c index 3b7a3b5e985..7766273381b 100644 --- a/src/mesa/state_tracker/st_cb_texture.c +++ b/src/mesa/state_tracker/st_cb_texture.c @@ -151,10 +151,21 @@ static struct gl_texture_object * st_NewTextureObject(struct gl_context * ctx, GLuint name, GLenum target) { struct st_texture_object *obj = ST_CALLOC_STRUCT(st_texture_object); + if (!obj) + return NULL; + + /* Pre-allocate a sampler views container to save a branch in the fast path. */ + obj->sampler_views = calloc(1, sizeof(struct st_sampler_views) + sizeof(struct st_sampler_view)); + if (!obj->sampler_views) { + free(obj); + return NULL; + } + obj->sampler_views->max = 1; DBG("%s\n", __func__); _mesa_initialize_texture_object(ctx, &obj->base, name, target); + simple_mtx_init(&obj->validate_mutex, mtx_plain); obj->needs_validation = true; return &obj->base; @@ -171,6 +182,7 @@ st_DeleteTextureObject(struct gl_context *ctx, pipe_resource_reference(&stObj->pt, NULL); st_texture_release_all_sampler_views(st, stObj); st_texture_free_sampler_views(stObj); + simple_mtx_destroy(&stObj->validate_mutex); _mesa_delete_texture_object(ctx, texObj); } diff --git a/src/mesa/state_tracker/st_sampler_view.c b/src/mesa/state_tracker/st_sampler_view.c index 892725671d2..b737011d501 100644 --- a/src/mesa/state_tracker/st_sampler_view.c +++ b/src/mesa/state_tracker/st_sampler_view.c @@ -43,24 +43,39 @@ /** - * Try to find a matching sampler view for the given context. - * If none is found an empty slot is initialized with a - * template and returned instead. + * Set the given view as the current context's view for the texture. + * + * Overwrites any pre-existing view of the context. + * + * Takes ownership of the view (i.e., stores the view without incrementing the + * reference count). + * + * \return the view, or NULL on error. In case of error, the reference to the + * view is released. */ -static struct st_sampler_view * -st_texture_get_sampler_view(struct st_context *st, - struct st_texture_object *stObj) +static struct pipe_sampler_view * +st_texture_set_sampler_view(struct st_context *st, + struct st_texture_object *stObj, + struct pipe_sampler_view *view, + bool glsl130_or_later, bool srgb_skip_decode) { + struct st_sampler_views *views; struct st_sampler_view *free = NULL; + struct st_sampler_view *sv; GLuint i; - for (i = 0; i < stObj->num_sampler_views; ++i) { - struct st_sampler_view *sv = &stObj->sampler_views[i]; + simple_mtx_lock(&stObj->validate_mutex); + views = stObj->sampler_views; + + for (i = 0; i < views->count; ++i) { + sv = &views->views[i]; + /* Is the array entry used ? */ if (sv->view) { /* check if the context matches */ if (sv->view->context == st->pipe) { - return sv; + pipe_sampler_view_release(st->pipe, &sv->view); + goto found; } } else { /* Found a free slot, remember that */ @@ -69,19 +84,98 @@ st_texture_get_sampler_view(struct st_context *st, } /* Couldn't find a slot for our context, create a new one */ + if (free) { + sv = free; + } else { + if (views->count >= views->max) { + /* Allocate a larger container. */ + unsigned new_max = 2 * views->max; + unsigned new_size = sizeof(*views) + new_max * sizeof(views->views[0]); + + if (new_max < views->max || + new_max > (UINT_MAX - sizeof(*views)) / sizeof(views->views[0])) { + pipe_sampler_view_release(st->pipe, &view); + goto out; + } + + struct st_sampler_views *new_views = malloc(new_size); + if (!new_views) { + pipe_sampler_view_release(st->pipe, &view); + goto out; + } + + new_views->count = views->count; + new_views->max = new_max; + memcpy(&new_views->views[0], &views->views[0], + views->count * sizeof(views->views[0])); + + /* Initialize the pipe_sampler_view pointers to zero so that we don't + * have to worry about racing against readers when incrementing + * views->count. + */ + memset(&new_views->views[views->count], 0, + (new_max - views->count) * sizeof(views->views[0])); + + /* Use memory release semantics to ensure that concurrent readers will + * get the correct contents of the new container. + * + * Also, the write should be atomic, but that's guaranteed anyway on + * all supported platforms. + */ + p_atomic_set(&stObj->sampler_views, new_views); + + /* We keep the old container around until the texture object is + * deleted, because another thread may still be reading from it. We + * double the size of the container each time, so we end up with + * at most twice the total memory allocation. + */ + views->next = stObj->sampler_views_old; + stObj->sampler_views_old = views; + + views = new_views; + } + + sv = &views->views[views->count]; - if (!free) { - /* Haven't even found a free one, resize the array */ - unsigned new_size = (stObj->num_sampler_views + 1) * - sizeof(struct st_sampler_view); - stObj->sampler_views = realloc(stObj->sampler_views, new_size); - free = &stObj->sampler_views[stObj->num_sampler_views++]; - free->view = NULL; + /* Since modification is guarded by the lock, only the write part of the + * increment has to be atomic, and that's already guaranteed on all + * supported platforms without using an atomic intrinsic. + */ + views->count++; } - assert(free->view == NULL); +found: + assert(sv->view == NULL); + + sv->glsl130_or_later = glsl130_or_later; + sv->srgb_skip_decode = srgb_skip_decode; + sv->view = view; + +out: + simple_mtx_unlock(&stObj->validate_mutex); + return view; +} + + +/** + * Return the most-recently validated sampler view for the texture \p stObj + * in the given context, if any. + * + * Performs no additional validation. + */ +const struct st_sampler_view * +st_texture_get_current_sampler_view(const struct st_context *st, + const struct st_texture_object *stObj) +{ + const struct st_sampler_views *views = p_atomic_read(&stObj->sampler_views); + + for (unsigned i = 0; i < views->count; ++i) { + const struct st_sampler_view *sv = &views->views[i]; + if (sv->view && sv->view->context == st->pipe) + return sv; + } - return free; + return NULL; } @@ -95,14 +189,17 @@ st_texture_release_sampler_view(struct st_context *st, { GLuint i; - for (i = 0; i < stObj->num_sampler_views; ++i) { - struct pipe_sampler_view **sv = &stObj->sampler_views[i].view; + simple_mtx_lock(&stObj->validate_mutex); + struct st_sampler_views *views = stObj->sampler_views; + for (i = 0; i < views->count; ++i) { + struct pipe_sampler_view **sv = &views->views[i].view; if (*sv && (*sv)->context == st->pipe) { pipe_sampler_view_reference(sv, NULL); break; } } + simple_mtx_unlock(&stObj->validate_mutex); } @@ -116,8 +213,18 @@ st_texture_release_all_sampler_views(struct st_context *st, { GLuint i; - for (i = 0; i < stObj->num_sampler_views; ++i) - pipe_sampler_view_release(st->pipe, &stObj->sampler_views[i].view); + /* TODO: This happens while a texture is deleted, because the Driver API + * is asymmetric: the driver allocates the texture object memory, but + * mesa/main frees it. + */ + if (!stObj->sampler_views) + return; + + simple_mtx_lock(&stObj->validate_mutex); + struct st_sampler_views *views = stObj->sampler_views; + for (i = 0; i < views->count; ++i) + pipe_sampler_view_release(st->pipe, &views->views[i].view); + simple_mtx_unlock(&stObj->validate_mutex); } @@ -126,7 +233,12 @@ st_texture_free_sampler_views(struct st_texture_object *stObj) { free(stObj->sampler_views); stObj->sampler_views = NULL; - stObj->num_sampler_views = 0; + + while (stObj->sampler_views_old) { + struct st_sampler_views *views = stObj->sampler_views_old; + stObj->sampler_views_old = views->next; + free(views); + } } @@ -410,23 +522,21 @@ st_get_texture_sampler_view_from_stobj(struct st_context *st, bool glsl130_or_later, bool ignore_srgb_decode) { - struct st_sampler_view *sv; - struct pipe_sampler_view *view; + const struct st_sampler_view *sv; bool srgb_skip_decode = false; - sv = st_texture_get_sampler_view(st, stObj); - view = sv->view; - if (!ignore_srgb_decode && samp->sRGBDecode == GL_SKIP_DECODE_EXT) srgb_skip_decode = true; - if (view && + sv = st_texture_get_current_sampler_view(st, stObj); + + if (sv && sv->glsl130_or_later == glsl130_or_later && sv->srgb_skip_decode == srgb_skip_decode) { /* Debug check: make sure that the sampler view's parameters are * what they're supposed to be. */ - MAYBE_UNUSED struct pipe_sampler_view *view = sv->view; + struct pipe_sampler_view *view = sv->view; assert(stObj->pt == view->texture); assert(!check_sampler_swizzle(st, stObj, view, glsl130_or_later)); assert(get_sampler_view_format(st, stObj, srgb_skip_decode) == view->format); @@ -439,18 +549,15 @@ st_get_texture_sampler_view_from_stobj(struct st_context *st, assert(!stObj->layer_override || (stObj->layer_override == view->u.tex.first_layer && stObj->layer_override == view->u.tex.last_layer)); + return view; } - else { - /* create new sampler view */ - enum pipe_format format = get_sampler_view_format(st, stObj, srgb_skip_decode); - - sv->glsl130_or_later = glsl130_or_later; - sv->srgb_skip_decode = srgb_skip_decode; - pipe_sampler_view_release(st->pipe, &sv->view); - view = sv->view = + /* create new sampler view */ + enum pipe_format format = get_sampler_view_format(st, stObj, srgb_skip_decode); + struct pipe_sampler_view *view = st_create_texture_sampler_view_from_stobj(st, stObj, format, glsl130_or_later); - } + + view = st_texture_set_sampler_view(st, stObj, view, glsl130_or_later, srgb_skip_decode); return view; } @@ -460,58 +567,65 @@ struct pipe_sampler_view * st_get_buffer_sampler_view_from_stobj(struct st_context *st, struct st_texture_object *stObj) { - struct st_sampler_view *sv; + const struct st_sampler_view *sv; struct st_buffer_object *stBuf = st_buffer_object(stObj->base.BufferObject); if (!stBuf || !stBuf->buffer) return NULL; - sv = st_texture_get_sampler_view(st, stObj); + sv = st_texture_get_current_sampler_view(st, stObj); struct pipe_resource *buf = stBuf->buffer; - struct pipe_sampler_view *view = sv->view; - if (view && view->texture == buf) { - /* Debug check: make sure that the sampler view's parameters are - * what they're supposed to be. - */ - assert(st_mesa_format_to_pipe_format(st, stObj->base._BufferObjectFormat) + if (sv) { + struct pipe_sampler_view *view = sv->view; + + if (view->texture == buf) { + /* Debug check: make sure that the sampler view's parameters are + * what they're supposed to be. + */ + assert(st_mesa_format_to_pipe_format(st, stObj->base._BufferObjectFormat) == view->format); - assert(view->target == PIPE_BUFFER); - unsigned base = stObj->base.BufferOffset; - MAYBE_UNUSED unsigned size = MIN2(buf->width0 - base, + assert(view->target == PIPE_BUFFER); + unsigned base = stObj->base.BufferOffset; + MAYBE_UNUSED unsigned size = MIN2(buf->width0 - base, (unsigned) stObj->base.BufferSize); - assert(view->u.buf.offset == base); - assert(view->u.buf.size == size); - } else { - unsigned base = stObj->base.BufferOffset; + assert(view->u.buf.offset == base); + assert(view->u.buf.size == size); + return view; + } + } - if (base >= buf->width0) - return NULL; + unsigned base = stObj->base.BufferOffset; - unsigned size = buf->width0 - base; - size = MIN2(size, (unsigned)stObj->base.BufferSize); - if (!size) - return NULL; + if (base >= buf->width0) + return NULL; + + unsigned size = buf->width0 - base; + size = MIN2(size, (unsigned)stObj->base.BufferSize); + if (!size) + return NULL; + + /* Create a new sampler view. There is no need to clear the entire + * structure (consider CPU overhead). + */ + struct pipe_sampler_view templ; + + templ.format = + st_mesa_format_to_pipe_format(st, stObj->base._BufferObjectFormat); + templ.target = PIPE_BUFFER; + templ.swizzle_r = PIPE_SWIZZLE_X; + templ.swizzle_g = PIPE_SWIZZLE_Y; + templ.swizzle_b = PIPE_SWIZZLE_Z; + templ.swizzle_a = PIPE_SWIZZLE_W; + templ.u.buf.offset = base; + templ.u.buf.size = size; + + struct pipe_sampler_view *view = + st->pipe->create_sampler_view(st->pipe, buf, &templ); + + view = st_texture_set_sampler_view(st, stObj, view, false, false); - /* Create a new sampler view. There is no need to clear the entire - * structure (consider CPU overhead). - */ - struct pipe_sampler_view templ; - - templ.format = - st_mesa_format_to_pipe_format(st, stObj->base._BufferObjectFormat); - templ.target = PIPE_BUFFER; - templ.swizzle_r = PIPE_SWIZZLE_X; - templ.swizzle_g = PIPE_SWIZZLE_Y; - templ.swizzle_b = PIPE_SWIZZLE_Z; - templ.swizzle_a = PIPE_SWIZZLE_W; - templ.u.buf.offset = base; - templ.u.buf.size = size; - - pipe_sampler_view_release(st->pipe, &sv->view); - view = sv->view = st->pipe->create_sampler_view(st->pipe, buf, &templ); - } return view; } diff --git a/src/mesa/state_tracker/st_sampler_view.h b/src/mesa/state_tracker/st_sampler_view.h index cf46513f1b4..47c100df647 100644 --- a/src/mesa/state_tracker/st_sampler_view.h +++ b/src/mesa/state_tracker/st_sampler_view.h @@ -68,6 +68,9 @@ st_texture_release_all_sampler_views(struct st_context *st, void st_texture_free_sampler_views(struct st_texture_object *stObj); +const struct st_sampler_view * +st_texture_get_current_sampler_view(const struct st_context *st, + const struct st_texture_object *stObj); struct pipe_sampler_view * st_get_texture_sampler_view_from_stobj(struct st_context *st, diff --git a/src/mesa/state_tracker/st_texture.h b/src/mesa/state_tracker/st_texture.h index 8b549b86085..c10a2753104 100644 --- a/src/mesa/state_tracker/st_texture.h +++ b/src/mesa/state_tracker/st_texture.h @@ -31,6 +31,7 @@ #include "pipe/p_context.h" #include "util/u_sampler.h" +#include "util/simple_mtx.h" #include "main/mtypes.h" @@ -61,6 +62,16 @@ struct st_sampler_view { }; +/** + * Container for per-context sampler views of a texture. + */ +struct st_sampler_views { + struct st_sampler_views *next; + uint32_t max; + uint32_t count; + struct st_sampler_view views[0]; +}; + /** * Subclass of gl_texure_image. */ @@ -105,13 +116,34 @@ struct st_texture_object */ struct pipe_resource *pt; - /* Number of views in sampler_views array */ - GLuint num_sampler_views; + /* Protect modifications of the sampler_views array */ + simple_mtx_t validate_mutex; - /* Array of sampler views (one per context) attached to this texture + /* Container of sampler views (one per context) attached to this texture * object. Created lazily on first binding in context. + * + * Purely read-only accesses to the current context's own sampler view + * require no locking. Another thread may simultaneously replace the + * container object in order to grow the array, but the old container will + * be kept alive. + * + * Writing to the container (even for modifying the current context's own + * sampler view) always requires taking the validate_mutex to protect against + * concurrent container switches. + * + * NULL'ing another context's sampler view is allowed only while + * implementing an API call that modifies the texture: an application which + * calls those while simultaneously reading the texture in another context + * invokes undefined behavior. (TODO: a dubious violation of this rule is + * st_finalize_texture, which is a lazy operation that corresponds to a + * texture modification.) + */ + struct st_sampler_views *sampler_views; + + /* Old sampler views container objects that have not been freed yet because + * other threads/contexts may still be reading from them. */ - struct st_sampler_view *sampler_views; + struct st_sampler_views *sampler_views_old; /* True if this texture comes from the window system. Such a texture * cannot be reallocated and the format can only be changed with a sampler -- 2.30.2