[Mesa-dev] [PATCH 3/8] i965: Expand comments in brw_upload_indices().

[Kenneth Graunke]

On 03/13/2014 08:59 AM, Ian Romanick wrote:
> On 03/13/2014 01:57 AM, Kenneth Graunke wrote:
>> This function has three cases:
>> 1. Data is in user arrays
>> 2. Data is in a buffer object, but misaligned.
>> 3. Data is acceptable as is.
>>
>> Previously, there was a "Turn into a proper VBO" comment above all three
>> cases, even though it only applies to case 1, and maybe case 2 (with a
>> specific interpretation of "proper".)  This patch replaces that with
>> more specific comments for each case.
>>
>> The expanded comments explain where the alignment restrictions come from
>> and give an example of how to produce misaligned data in OpenGL.
>>
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>>  src/mesa/drivers/dri/i965/brw_draw_upload.c | 20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c b/src/mesa/drivers/dri/i965/brw_draw_upload.c
>> index f2945c1..98a8277 100644
>> --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
>> +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
>> @@ -838,18 +838,25 @@ static void brw_upload_indices(struct brw_context *brw)
>>     ib_size = ib_type_size * index_buffer->count;
>>     bufferobj = index_buffer->obj;
>>  
>> -   /* Turn into a proper VBO:
>> -    */
>>     if (!_mesa_is_bufferobj(bufferobj)) {
>> -      /* Get new bufferobj, offset:
>> -       */
>> +      /* Copy the index data from user arrays into a buffer object. */
>>        intel_upload_data(brw, index_buffer->ptr, ib_size, ib_type_size,
>>  			&bo, &offset);
>>     } else {
>>        offset = (GLuint) (unsigned long) index_buffer->ptr;
>>  
>> -      /* If the index buffer isn't aligned to its element size, we have to
>> -       * rebase it into a temporary.
>> +      /* Even though the index data is in a buffer object, it may not meet
>> +       * the hardware's alignment restrictions.  From the 3DSTATE_INDEX_BUFFER
>> +       * documentation, DWord 1:
>> +       *
>> +       * "This field contains the size-aligned (as specified by Index Format)
>> +       *  Graphics Address of the first element of interest within the index
>> +       *  buffer."
>> +       *
>> +       * OpenGL allows arbitrary byte offsets into the buffer.  For example,
>> +       * glDrawElements with index == 1 and a type of GL_UNSIGNED_SHORT.
>> +       *
>> +       * If the data isn't aligned to the element size, copy it to a temporary.
> 
> Have we actually encountered applications that do that?  The GL spec
> does say:
> 
>     "Clients must align data elements consistent with the requirements
>     of the client platform, with an additional base-level requirement
>     that an offset within a buffer to a datum comprising N basic
>     machine units be a multiple of N."

I sure don't know of any.  Haihao added this code in 2007 - commit
ea07a0df9a2f689b8f5acaf92c40bbbd602cab3c, referencing bug #11910, where
someone (probably on the QA team at the time?) had written a test case
that did this, and used an offset of 1 when they probably meant 1 *
sizeof(GLuint).

Based on your spec citation, this sure sounds like it should never
happen.  I don't see any code in api_validate.c or vbo_exec_array.c that
checks for misalignment, raises an error, and prevents us from reaching
the drawing function...but maybe I'm just blind.  It seems like such
code ought to exist.

>>         */
>>        if ((ib_type_size - 1) & offset) {
>>           perf_debug("copying index buffer to a temporary to work around "
>> @@ -866,6 +873,7 @@ static void brw_upload_indices(struct brw_context *brw)
>>  
>>           ctx->Driver.UnmapBuffer(ctx, bufferobj, MAP_INTERNAL);
>>        } else {
>> +         /* The index data is already in an acceptable BO.  Use as is. */
>>           bo = intel_bufferobj_buffer(brw, intel_buffer_object(bufferobj),
>>                                       offset, ib_size);
>>           drm_intel_bo_reference(bo);
>>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140314/3cbb45d4/attachment.sig>