[Mesa-dev] [PATCH] [swr] fix index buffers with non-zero indices

[Kyriazis, George]

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