[Mesa-dev] [PATCH] [swr] fix index buffers with non-zero indices
Kyriazis, George
george.kyriazis at intel.com
Tue Jan 31 20:11:34 UTC 2017
Thanks,
Good point. I'll send a new version our for review.
George
> -----Original Message-----
> From: ibmirkin at gmail.com [mailto:ibmirkin at gmail.com] On Behalf Of Ilia
> Mirkin
> Sent: Tuesday, January 31, 2017 11:50 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
>
> 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