[Mesa-dev] [PATCH] i965: Be more careful with the interleaved user array upload optimization
eric at anholt.net
Tue Jun 25 12:49:37 PDT 2013
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);
>>> glVertexAttribPointer(1, 4, GL_FLOAT, GL_FALSE, 16, &data);
>>> 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
Size: 197 bytes
Desc: not available
More information about the mesa-dev