[Mesa-dev] [PATCH] [swr] fix index buffers with non-zero indices
Ilia Mirkin
imirkin at alum.mit.edu
Tue Jan 31 17:50:28 UTC 2017
Here's the current code:
for (UINT i = 0; i < ctx->num_vertex_buffers; i++) {
uint32_t size, pitch, elems, partial_inbounds;
const uint8_t *p_data;
struct pipe_vertex_buffer *vb = &ctx->vertex_buffer[i];
pitch = vb->stride;
if (!vb->user_buffer) {
/* VBO
* size is based on buffer->width0 rather than info.max_index
* to prevent having to validate VBO on each draw */
size = vb->buffer->width0;
elems = size / pitch;
partial_inbounds = size % pitch;
p_data = swr_resource_data(vb->buffer) + vb->buffer_offset;
} else {
/* Client buffer
* client memory is one-time use, re-trigger SWR_NEW_VERTEX to
* revalidate on each draw */
post_update_dirty_flags |= SWR_NEW_VERTEX;
uint32_t base;
swr_user_vbuf_range(&info, ctx->velems, vb, i, &elems,
&base, &size);
partial_inbounds = 0;
/* Copy only needed vertices to scratch space */
size = AlignUp(size, 4);
const void *ptr = (const uint8_t *) vb->user_buffer + base;
memcpy(scratch, ptr, size);
ptr = scratch;
scratch += size;
p_data = (const uint8_t *)ptr - base;
}
swrVertexBuffers[i] = {0};
swrVertexBuffers[i].index = i;
swrVertexBuffers[i].pitch = pitch;
swrVertexBuffers[i].pData = p_data;
swrVertexBuffers[i].size = size;
swrVertexBuffers[i].maxVertex = elems;
swrVertexBuffers[i].partialInboundsSize = partial_inbounds;
}
You're modifying only the client buffer case. The VBO case
(!vb->user_buffer) is not adjusted in any way. Your patch changes the
base vertex passed in no matter what (and remember - I'm 99% sure it's
possible to mix VBO and non-VBO in the same draw). So it stands to
reason that now VBO-based draws with base vertex set will now be done
so incorrectly when min_index != 0.
I think the solution to your problem is to include a minVertex in the
vertex buffer definition, and clamp the range on that in the vertex
jit.
Cheers,
-ilia
On Tue, Jan 31, 2017 at 12:32 PM, Kyriazis, George
<george.kyriazis at intel.com> wrote:
> Ilia,
>
> The problem is the following: This occurs only with index buffers that do not contain vertex index 0. The vertex fetcher operates in blocks of 8 vertices. Consequently, the last batch of 8 could access elements outside the range of the index buffer. There is a chance that those elements will be outside range / non valid. The 1st phase of the vertex fetcher replaces invalid indices with 0 (in later phases, the actual vertex fetch operation is masked). Vertex index 0 is outside the range of the copied index buffer, and we SEGV accessing indices with 0 index. The BaseVertex offset gets added to every index in the index buffer, so the 0 index will now point to the real beginning of the (copied) index buffer, instead of unallocated memory.
>
> VBOs should be similar to DrawElement calls where we don't access client memory directly (ie. we copy the appropriate vertex and index buffers, which is the case that this fixes). What do you think?
>
> Thanks,
>
> George
>
>> -----Original Message-----
>> From: ibmirkin at gmail.com [mailto:ibmirkin at gmail.com] On Behalf Of Ilia
>> Mirkin
>> Sent: Tuesday, January 31, 2017 11:17 AM
>> To: Kyriazis, George <george.kyriazis at intel.com>
>> Cc: mesa-dev at lists.freedesktop.org
>> Subject: Re: [Mesa-dev] [PATCH] [swr] fix index buffers with non-zero
>> indices
>>
>> What's the problem being fixed exactly? We're passing what is effectively
>> the zero index to the core, and the core shouldn't ever access data outside of
>> [min, max] + bias.
>>
>> That said, what you have is probably a cleaner implementation. However I'm
>> a bit concerned about how it will function with VBO's rather than client-side
>> arrays. I don't think min_index is guaranteed to be zero in that case, e.g. it
>> could be set via glDrawRangeElements[BaseVertex].
>>
>> So you're effectively mixing a zero-based pointer for VBO and a min-based
>> pointer for client-side arrays. I don't think this can work... perhaps you need
>> to add a minVertex in addition to the maxVertex?
>>
>> Cheers,
>>
>> -ilia
>>
>> On Tue, Jan 31, 2017 at 11:38 AM, George Kyriazis
>> <george.kyriazis at intel.com> wrote:
>> > Fix issue with index buffers that do not contain 0 index. Use core
>> > BaseVertex functionality to offset index buffer indices, instead of
>> > offsetting vertex buffer to point before the buffer origin.
>> > ---
>> > src/gallium/drivers/swr/swr_draw.cpp | 2 +-
>> > src/gallium/drivers/swr/swr_state.cpp | 2 +-
>> > 2 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/src/gallium/drivers/swr/swr_draw.cpp
>> > b/src/gallium/drivers/swr/swr_draw.cpp
>> > index c4d5e5c..88000e5 100644
>> > --- a/src/gallium/drivers/swr/swr_draw.cpp
>> > +++ b/src/gallium/drivers/swr/swr_draw.cpp
>> > @@ -200,7 +200,7 @@ swr_draw_vbo(struct pipe_context *pipe, const
>> struct pipe_draw_info *info)
>> > info->count,
>> > info->instance_count,
>> > info->start,
>> > - info->index_bias,
>> > + info->index_bias - info->min_index,
>> > info->start_instance);
>> > else
>> > SwrDrawInstanced(ctx->swrContext, diff --git
>> > a/src/gallium/drivers/swr/swr_state.cpp
>> > b/src/gallium/drivers/swr/swr_state.cpp
>> > index f1f4963..f03f814 100644
>> > --- a/src/gallium/drivers/swr/swr_state.cpp
>> > +++ b/src/gallium/drivers/swr/swr_state.cpp
>> > @@ -1133,7 +1133,7 @@ swr_update_derived(struct pipe_context *pipe,
>> > memcpy(scratch, ptr, size);
>> > ptr = scratch;
>> > scratch += size;
>> > - p_data = (const uint8_t *)ptr - base;
>> > + p_data = (const uint8_t *)ptr;
>> > }
>> >
>> > swrVertexBuffers[i] = {0};
>> > --
>> > 2.7.4
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list