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

[Eric Anholt]

Ian Romanick <idr at freedesktop.org> writes:

> 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?

Yeah, that works.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130625/e1bce1ac/attachment.pgp>