Mesa (master): llvmpipe: Don't use u_ringbuffer for lp_scene_queue

[GitLab Mirror]

Module: Mesa
Branch: master
Commit: 397d1a18ef78ddf46efda44d6783105f9fd87f7e
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=397d1a18ef78ddf46efda44d6783105f9fd87f7e

Author: Caio Marcelo de Oliveira Filho <caio.oliveira at intel.com>
Date:   Wed Jun 12 15:32:30 2019 -0700

llvmpipe: Don't use u_ringbuffer for lp_scene_queue

Inline the ring buffer and signal logic into lp_scene_queue instead of
using a u_ringbuffer.  The code ends up simpler since there's no need
to handle serializing data from / to packets.

This fixes a crash when compiling Mesa with LTO, that happened because
of util_ringbuffer_dequeue() was writing data after the "header
packet", as shown below

    struct scene_packet {
       struct util_packet header;
       struct lp_scene *scene;
    };

    /* Snippet of old lp_scene_deque(). */
    packet.scene = NULL;
    ret = util_ringbuffer_dequeue(queue->ring,
                                  &packet.header,
                                  sizeof packet / 4,
    return packet.scene;

but due to the way aliasing analysis work the compiler didn't
considered the "&packet->header" to alias with "packet->scene".  With
the aggressive inlining done by LTO, this would end up always
returning NULL instead of the content read by
util_ringbuffer_dequeue().

Issue found by Marco Simental and iThiago Macieira.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110884
Reviewed-by: Roland Scheidegger <sroland at vmware.com>

---

 src/gallium/drivers/llvmpipe/lp_scene_queue.c | 84 +++++++++++++++------------
 1 file changed, 48 insertions(+), 36 deletions(-)

diff --git a/src/gallium/drivers/llvmpipe/lp_scene_queue.c b/src/gallium/drivers/llvmpipe/lp_scene_queue.c
index debc7a6fe18..5f267d04ca4 100644
--- a/src/gallium/drivers/llvmpipe/lp_scene_queue.c
+++ b/src/gallium/drivers/llvmpipe/lp_scene_queue.c
@@ -32,25 +32,33 @@
  * which are produced by the "rast" code when it finishes rendering a scene.
  */
 
-#include "util/u_ringbuffer.h"
+#include "os/os_thread.h"
 #include "util/u_memory.h"
 #include "lp_scene_queue.h"
+#include "util/u_math.h"
 
 
 
-#define MAX_SCENE_QUEUE 4
+#define SCENE_QUEUE_SIZE 4
+
 
-struct scene_packet {
-   struct util_packet header;
-   struct lp_scene *scene;
-};
 
 /**
  * A queue of scenes
  */
 struct lp_scene_queue
 {
-   struct util_ringbuffer *ring;
+   struct lp_scene *scenes[SCENE_QUEUE_SIZE];
+
+   mtx_t mutex;
+   cnd_t change;
+
+   /* These values wrap around, so that head == tail means empty.  When used
+    * to index the array, we use them modulo the queue size.  This scheme
+    * works because the queue size is a power of two.
+    */
+   unsigned head;
+   unsigned tail;
 };
 
 
@@ -59,20 +67,19 @@ struct lp_scene_queue
 struct lp_scene_queue *
 lp_scene_queue_create(void)
 {
+   /* Circular queue behavior depends on size being a power of two. */
+   STATIC_ASSERT(SCENE_QUEUE_SIZE > 0);
+   STATIC_ASSERT((SCENE_QUEUE_SIZE & (SCENE_QUEUE_SIZE - 1)) == 0);
+
    struct lp_scene_queue *queue = CALLOC_STRUCT(lp_scene_queue);
+
    if (!queue)
       return NULL;
 
-   queue->ring = util_ringbuffer_create( MAX_SCENE_QUEUE * 
-                                         sizeof( struct scene_packet ) / 4);
-   if (queue->ring == NULL)
-      goto fail;
+   (void) mtx_init(&queue->mutex, mtx_plain);
+   cnd_init(&queue->change);
 
    return queue;
-
-fail:
-   FREE(queue);
-   return NULL;
 }
 
 
@@ -80,7 +87,8 @@ fail:
 void
 lp_scene_queue_destroy(struct lp_scene_queue *queue)
 {
-   util_ringbuffer_destroy(queue->ring);
+   cnd_destroy(&queue->change);
+   mtx_destroy(&queue->mutex);
    FREE(queue);
 }
 
@@ -89,19 +97,25 @@ lp_scene_queue_destroy(struct lp_scene_queue *queue)
 struct lp_scene *
 lp_scene_dequeue(struct lp_scene_queue *queue, boolean wait)
 {
-   struct scene_packet packet;
-   enum pipe_error ret;
+   mtx_lock(&queue->mutex);
 
-   packet.scene = NULL;
+   if (wait) {
+      /* Wait for queue to be not empty. */
+      while (queue->head == queue->tail)
+         cnd_wait(&queue->change, &queue->mutex);
+   } else {
+      if (queue->head == queue->tail) {
+         mtx_unlock(&queue->mutex);
+         return NULL;
+      }
+   }
 
-   ret = util_ringbuffer_dequeue(queue->ring,
-                                 &packet.header,
-                                 sizeof packet / 4,
-                                 wait );
-   if (ret != PIPE_OK)
-      return NULL;
+   struct lp_scene *scene = queue->scenes[queue->head++ % SCENE_QUEUE_SIZE];
+
+   cnd_signal(&queue->change);
+   mtx_unlock(&queue->mutex);
 
-   return packet.scene;
+   return scene;
 }
 
 
@@ -109,16 +123,14 @@ lp_scene_dequeue(struct lp_scene_queue *queue, boolean wait)
 void
 lp_scene_enqueue(struct lp_scene_queue *queue, struct lp_scene *scene)
 {
-   struct scene_packet packet;
-
-   packet.header.dwords = sizeof packet / 4;
-   packet.header.data24 = 0;
-   packet.scene = scene;
-
-   util_ringbuffer_enqueue(queue->ring, &packet.header);
-}
-
-
+   mtx_lock(&queue->mutex);
 
+   /* Wait for free space. */
+   while (queue->tail - queue->head >= SCENE_QUEUE_SIZE)
+      cnd_wait(&queue->change, &queue->mutex);
 
+   queue->scenes[queue->tail++ % SCENE_QUEUE_SIZE] = scene;
 
+   cnd_signal(&queue->change);
+   mtx_unlock(&queue->mutex);
+}