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
<[email protected]> 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: [email protected] [mailto:[email protected]] On Behalf Of Ilia
>> Mirkin
>> Sent: Tuesday, January 31, 2017 11:17 AM
>> To: Kyriazis, George <[email protected]>
>> Cc: [email protected]
>> 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
>> <[email protected]> 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
>> > [email protected]
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
