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

[Ian Romanick]

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."

>         */
>        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);
>