Attachment 112872 Details for Bug 88662 – proposed patch

[patch] proposed patch

0001-mesa-fix-display-list-8-byte-alignment-issue.patch (text/plain), 7.87 KB, created by Brian Paul on 2015-01-27 03:43:16 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Brian Paul

Created: 2015-01-27 03:43:16 UTC

Size: 7.87 KB

patch

obsolete

>From 095845a20358cb3c71192bef92c2ec8a0146d5e7 Mon Sep 17 00:00:00 2001
>From: Brian Paul <brianp@vmware.com>
>Date: Mon, 26 Jan 2015 20:32:58 -0700
>Subject: [PATCH] mesa: fix display list 8-byte alignment issue
>
>The _mesa_dlist_alloc() function is only guaranteed to return a pointer
>with 4-byte alignment.  On 64-bit systems which don't support unaligned
>loads (e.g. SPARC or MIPS) this could lead to a bus error in the VBO code.
>
>The solution is to add a new  _mesa_dlist_alloc_aligned() function which
>will return a pointer to an 8-byte aligned address on 64-bit systems.
>This is accomplished by inserting a 4-byte NOP instruction in the display
>list when needed.
>
>The only place this actually matters is the VBO code where we need to
>allocate a 'struct vbo_save_vertex_list' which needs to be 8-byte
>aligned (just as if it were malloc'd).
>
>The gears demo and others hit this bug.
>
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88662
>Cc: "10.4" <mesa-stable@lists.freedesktop.org>
>---
> src/mesa/main/dlist.c       | 72 +++++++++++++++++++++++++++++++++++++++++----
> src/mesa/main/dlist.h       |  3 ++
> src/mesa/vbo/vbo_save_api.c |  5 +++-
> 3 files changed, 73 insertions(+), 7 deletions(-)
>
>diff --git a/src/mesa/main/dlist.c b/src/mesa/main/dlist.c
>index 138d360..dc6070b 100644
>--- a/src/mesa/main/dlist.c
>+++ b/src/mesa/main/dlist.c
>@@ -487,6 +487,7 @@ typedef enum
>    /* The following three are meta instructions */
>    OPCODE_ERROR,                /* raise compiled-in error */
>    OPCODE_CONTINUE,
>+   OPCODE_NOP,                  /* No-op (used for 8-byte alignment */
>    OPCODE_END_OF_LIST,
>    OPCODE_EXT_0
> } OpCode;
>@@ -1012,13 +1013,16 @@ memdup(const void *src, GLsizei bytes)
>  * Allocate space for a display list instruction (opcode + payload space).
>  * \param opcode  the instruction opcode (OPCODE_* value)
>  * \param bytes   instruction payload size (not counting opcode)
>- * \return pointer to allocated memory (the opcode space)
>+ * \param align8  does the payload need to be 8-byte aligned?
>+ *                This is only relevant in 64-bit environments.
>+ * \return pointer to allocated memory (the payload will be at pointer+1)
>  */
> static Node *
>-dlist_alloc(struct gl_context *ctx, OpCode opcode, GLuint bytes)
>+dlist_alloc(struct gl_context *ctx, OpCode opcode, GLuint bytes, bool align8)
> {
>    const GLuint numNodes = 1 + (bytes + sizeof(Node) - 1) / sizeof(Node);
>    const GLuint contNodes = 1 + POINTER_DWORDS;  /* size of continue info */
>+   GLuint nopNode;
>    Node *n;
> 
>    if (opcode < OPCODE_EXT_0) {
>@@ -1032,7 +1036,19 @@ dlist_alloc(struct gl_context *ctx, OpCode opcode, GLuint bytes)
>       }
>    }
> 
>-   if (ctx->ListState.CurrentPos + numNodes + contNodes > BLOCK_SIZE) {
>+   if (sizeof(void *) == 8 && align8 && ctx->ListState.CurrentPos % 2 == 0) {
>+      /* The opcode would get placed at node[0] and the payload would start
>+       * at node[1].  But the payload needs to be at an even offset (8-byte
>+       * multiple).
>+       */
>+      nopNode = 1;
>+   }
>+   else {
>+      nopNode = 0;
>+   }
>+
>+   if (ctx->ListState.CurrentPos + nopNode + numNodes + contNodes
>+       > BLOCK_SIZE) {
>       /* This block is full.  Allocate a new block and chain to it */
>       Node *newblock;
>       n = ctx->ListState.CurrentBlock + ctx->ListState.CurrentPos;
>@@ -1042,13 +1058,34 @@ dlist_alloc(struct gl_context *ctx, OpCode opcode, GLuint bytes)
>          _mesa_error(ctx, GL_OUT_OF_MEMORY, "Building display list");
>          return NULL;
>       }
>+
>+      /* a fresh block should be 8-byte aligned on 64-bit systems */
>+      assert(((GLintptr) newblock) % sizeof(void *) == 0);
>+
>       save_pointer(&n[1], newblock);
>       ctx->ListState.CurrentBlock = newblock;
>       ctx->ListState.CurrentPos = 0;
>+
>+      /* Display list nodes are always 4 bytes.  If we need 8-byte alignment
>+       * we have to insert a NOP so that the payload of the real opcode lands
>+       * on an even location:
>+       *   node[0] = OPCODE_NOP
>+       *   node[1] = OPCODE_x;
>+       *   node[2] = start of payload
>+       */
>+      nopNode = sizeof(void *) == 8 && align8;
>    }
> 
>    n = ctx->ListState.CurrentBlock + ctx->ListState.CurrentPos;
>-   ctx->ListState.CurrentPos += numNodes;
>+   if (nopNode) {
>+      assert(ctx->ListState.CurrentPos % 2 == 0); /* even value */
>+      n[0].opcode = OPCODE_NOP;
>+      n++;
>+      /* The "real" opcode will now be at an odd location and the payload
>+       * will be at an even location.
>+       */
>+   }
>+   ctx->ListState.CurrentPos += nopNode + numNodes;
> 
>    n[0].opcode = opcode;
> 
>@@ -1069,7 +1106,22 @@ dlist_alloc(struct gl_context *ctx, OpCode opcode, GLuint bytes)
> void *
> _mesa_dlist_alloc(struct gl_context *ctx, GLuint opcode, GLuint bytes)
> {
>-   Node *n = dlist_alloc(ctx, (OpCode) opcode, bytes);
>+   Node *n = dlist_alloc(ctx, (OpCode) opcode, bytes, false);
>+   if (n)
>+      return n + 1;  /* return pointer to payload area, after opcode */
>+   else
>+      return NULL;
>+}
>+
>+
>+/**
>+ * Same as _mesa_dlist_alloc(), but return a pointer which is 8-byte
>+ * aligned in 64-bit environments, 4-byte aligned otherwise.
>+ */
>+void *
>+_mesa_dlist_alloc_aligned(struct gl_context *ctx, GLuint opcode, GLuint bytes)
>+{
>+   Node *n = dlist_alloc(ctx, (OpCode) opcode, bytes, true);
>    if (n)
>       return n + 1;  /* return pointer to payload area, after opcode */
>    else
>@@ -1119,7 +1171,7 @@ _mesa_dlist_alloc_opcode(struct gl_context *ctx,
> static inline Node *
> alloc_instruction(struct gl_context *ctx, OpCode opcode, GLuint nparams)
> {
>-   return dlist_alloc(ctx, opcode, nparams * sizeof(Node));
>+   return dlist_alloc(ctx, opcode, nparams * sizeof(Node), false);
> }
> 
> 
>@@ -8897,6 +8949,9 @@ execute_list(struct gl_context *ctx, GLuint list)
>          case OPCODE_CONTINUE:
>             n = (Node *) get_pointer(&n[1]);
>             break;
>+         case OPCODE_NOP:
>+            /* no-op */
>+            break;
>          case OPCODE_END_OF_LIST:
>             done = GL_TRUE;
>             break;
>@@ -9947,6 +10002,9 @@ print_list(struct gl_context *ctx, GLuint list, const char *fname)
>             fprintf(f, "DISPLAY-LIST-CONTINUE\n");
>             n = (Node *) get_pointer(&n[1]);
>             break;
>+         case OPCODE_NOP:
>+            fprintf(f, "NOP\n");
>+            break;
>          case OPCODE_END_OF_LIST:
>             fprintf(f, "END-LIST %u\n", list);
>             done = GL_TRUE;
>@@ -10097,6 +10155,8 @@ _mesa_init_display_list(struct gl_context *ctx)
>    ctx->List.ListBase = 0;
> 
>    save_vtxfmt_init(&ctx->ListState.ListVtxfmt);
>+
>+   InstSize[OPCODE_NOP] = 1;
> }
> 
> 
>diff --git a/src/mesa/main/dlist.h b/src/mesa/main/dlist.h
>index c57eb74..6189632 100644
>--- a/src/mesa/main/dlist.h
>+++ b/src/mesa/main/dlist.h
>@@ -60,6 +60,9 @@ extern void _mesa_compile_error( struct gl_context *ctx, GLenum error, const cha
> 
> extern void *_mesa_dlist_alloc(struct gl_context *ctx, GLuint opcode, GLuint sz);
> 
>+extern void *
>+_mesa_dlist_alloc_aligned(struct gl_context *ctx, GLuint opcode, GLuint bytes);
>+
> extern GLint _mesa_dlist_alloc_opcode( struct gl_context *ctx, GLuint sz,
>                                        void (*execute)( struct gl_context *, void * ),
>                                        void (*destroy)( struct gl_context *, void * ),
>diff --git a/src/mesa/vbo/vbo_save_api.c b/src/mesa/vbo/vbo_save_api.c
>index 5055c22..beef342 100644
>--- a/src/mesa/vbo/vbo_save_api.c
>+++ b/src/mesa/vbo/vbo_save_api.c
>@@ -375,11 +375,14 @@ _save_compile_vertex_list(struct gl_context *ctx)
>     * being compiled.
>     */
>    node = (struct vbo_save_vertex_list *)
>-      _mesa_dlist_alloc(ctx, save->opcode_vertex_list, sizeof(*node));
>+      _mesa_dlist_alloc_aligned(ctx, save->opcode_vertex_list, sizeof(*node));
> 
>    if (!node)
>       return;
> 
>+   /* Make sure the pointer is aligned to the size of a pointer */
>+   assert((GLintptr) node % sizeof(void *) == 0);
>+
>    /* Duplicate our template, increment refcounts to the storage structs:
>     */
>    memcpy(node->attrsz, save->attrsz, sizeof(node->attrsz));
>-- 
>1.9.1
>

Actions: View | Splinter Review

Attachments on bug 88662: 112872

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.