[Mesa-dev] [PATCH] mesa: fix display list 8-byte alignment issue
Jose Fonseca
jfonseca at vmware.com
Fri Jan 30 02:25:15 PST 2015
Looks good to me.
Just one minor suggestion: if we replaced
sizeof(void *) == 8
with
sizeof(void *) > sizeof(GLuint)
we would avoid the magic number 8 and make the code correct for any
pointer size.
Jose
On 28/01/15 03:06, Brian Paul wrote:
> 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://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D88662&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=DaIFyU2hnmYCL1EaelhvLHTsOdyZV8y6pEVRwRkcp8Q&s=g3pos-5bc_Uu5plvnuQvIjeEJXLDgTr5eOznhu6o-Fo&e=
> Cc: "10.4" <mesa-stable at 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));
>
More information about the mesa-dev
mailing list