Thanks, Good point. I'll send a new version our for review.
George > -----Original Message----- > From: [email protected] [mailto:[email protected]] On Behalf Of Ilia > Mirkin > Sent: Tuesday, January 31, 2017 11:50 AM > To: Kyriazis, George <[email protected]> > Cc: [email protected] > 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 > <[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
