st/mesa: guard sampler views changes with a mutex

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 <marek.olsak@amd.com>

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,

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

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 (!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;
+         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];
+
+      /* 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);
 
-   return free;
+   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 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;
-
-      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;
-
-      pipe_sampler_view_release(st->pipe, &sv->view);
-      view = sv->view = st->pipe->create_sampler_view(st->pipe, buf, &templ);
+         assert(view->u.buf.offset == base);
+         assert(view->u.buf.size == size);
+         return view;
+      }
    }
+
+   unsigned base = stObj->base.BufferOffset;
+
+   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);
+
    return view;
 }

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,

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_view *sampler_views;
+   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_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