[Mesa-stable] [Mesa-dev] [PATCH] mesa: fix display list 8-byte alignment issue

Brian Paul brianp at vmware.com
Fri Jan 30 07:47:52 PST 2015


On 01/30/2015 03:25 AM, Jose Fonseca wrote:
> Looks good to me.
>
>
> Just one minor suggestion: if we replaced
>
>    sizeof(void *) == 8
>
> with
>
>    sizeof(void *) > sizeof(GLuint)

Yes, or more accurately, sizeof(Node).

>
> we would avoid the magic number 8 and make the code correct for any
> pointer size.

Thanks for the review.

-Brian


>
> 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-stable mailing list