[Mesa-dev] [PATCH] i965: Be more careful with the interleaved user array upload optimization

[Ian Romanick]

On 06/25/2013 11:57 AM, Eric Anholt wrote:
> Ian Romanick <idr at freedesktop.org> writes:
>
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> The checks to determine when the data can be uploaded in an interleaved
>> fashion can be tricked by certain data layouts.  For example,
>>
>>      float data[...];
>>
>>      glVertexAttribPointer(0, 4, GL_FLOAT, GL_FALSE, 16, &data[0]);
>>      glVertexAttribPointer(1, 4, GL_FLOAT, GL_FALSE, 16, &data[4]);
>>      glDrawArrays(GL_POINTS, 0, 1);
>>
>> will hit the interleaved path with an incorrect size (16 bytes instead
>> of 32 bytes).  As a result, the data for attribute 1 never gets
>> uploaded.  The single element draw case is the only sensible case I can
>> think of for non-interleaved-that-looks-like-interleaved data, but there
>> may be others as well.
>>
>> To fix this, make sure that the end of the element in the array being
>> checked is within the stride "window."  Previously the code would check
>> that the begining of the element was within the window.
>>
>> NOTE: This is a candidate for stable branches.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> Cc: Kenneth Graunke <kenneth at whitecape.org>
>> Cc: Eric Anholt <eric at anholt.net>
>> ---
>>   src/mesa/drivers/dri/i965/brw_draw_upload.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c b/src/mesa/drivers/dri/i965/brw_draw_upload.c
>> index 2ded14b..d19250b 100644
>> --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
>> +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
>> @@ -506,8 +506,22 @@ static void brw_prepare_vertices(struct brw_context *brw)
>>   	    ptr = glarray->Ptr;
>>   	 }
>>   	 else if (interleaved != glarray->StrideB ||
>> -		  (uintptr_t)(glarray->Ptr - ptr) > interleaved)
>> +                  (uintptr_t)(glarray->Ptr - ptr) + glarray->_ElementSize > interleaved)
>
> I think we should retain the previous check, too, to make sure that we
> don't do it if people have overlapping elements where attribute 1 starts
> just before and extending into the start of attribute 0.  It sounds
> crazy, but it'll be hell to debug if anyone ever does.  (I'm imagining a
> single-element attribute like the original bug report, where someone
> stacked up a vec2 before a vec4, but accidentally specified "16" as the
> stride on the vec2 because it shouldn't matter)
>

git n00b failure. :(  I have an uncommitted change in my tree that makes 
the check actually work.

+                  glarray->Ptr < ptr ||

Without this, piles of piglit tests fail because the data is interleaved 
in the opposite order from what the fast path can handle... basically 
making more instances of the original bug.

I also think this addition covers the case you're concerned about.  Yeah?

>>   	 {
>> +            /* If the stride is different or if the stride doesn't cover the
>> +             * entire range of the data element, disable the interleaved
>> +             * upload optimization.  The second case can most commonly occur
>> +             * in cases where there is a single vertex and, for example, the
>> +             * data is stored on the application's stack.
>
> "the stride" is making me think of the stride of this attribute, not the
> original one that might get uploaded in interleaved mode.
>
> Perhaps:
>
> "If our stride is different from the first attribute's stride, or if the
> first attribute's stride didn't cover our element, disable..."

Yeah, I like that.

> The most common case for the second, of course, is when the data isn't
> anywhere close to interleaved. :)
>
>> +             * NOTE: This will also disable the optimization in cases where
>> +             * the data is in a different order than the array indices.
>> +             * Something like:
>> +             *
>> +             *     float data[...];
>> +             *     glVertexAttribPointer(0, 4, GL_FLOAT, 16, &data[4]);
>> +             *     glVertexAttribPointer(1, 4, GL_FLOAT, 16, &data[0]);
>> +             */
>>   	    interleaved = 0;
>
> I think the "16" instead of "32" in the comment code will surprise
> people reading it (though 16 was important in the commit message
> explaining the original bug).  With the check and this comment changed
> (whether or not you like other comment rewording),

Okay.

> Reviewed-by: Eric Anholt <eric at anholt.net>