[Mesa-dev] [PATCH 4/6] gallium: remove pipe_vertex_buffer::max_index

[Marek Olšák]

On Mon, Feb 14, 2011 at 6:58 PM, José Fonseca <jfonseca at vmware.com> wrote:

> Marek,
>
> I'm OK with removing pipe_vertex_buffer::max_index but there is a bit
> more work involved, as they are not really equivalent in the guarantees.
>
> pipe_vertex_buffer::max_index is an attribute of the vertex buffer -- it
> describe the max index that can be fetch from the buffer without running
> into a buffer overflow.  It is an hard limit -- it must be set
> accurately by the state tracker or crashes will occur.  It can be
> removed because it can be derived from the vertex element size, vertex
> element stride, vertex buffer offset, and vertex buffer size.
>
> pipe_draw_info::max_index is an attribute of the index buffer: it
> describes the maximum index in the index buffer. It is an hint -- there
> may be higher indices in the index buffer, and if so it is OK for the
> driver to ignore those vertices, but it should not crash with a buffer
> overflow.
>
> Therefore, in order to safely remove pipe_vertex_buffer::max_index, we
> should compute the max_index inside the draw module / pipe drivers, and
> ensure vertices with higher indices will never be removed.
>
> There are a few places in this patch where you replace
> pipe_vertex_buffer::max_index with ~0 or no checks, which means that
> places which where previous robust to pipe_draw_info::max_index == ~0
> and bogus indices will now start crashing.
>

You're right in theory. In practice, pipe_vertex_buffer::max_index was
really derived from the value which is put in pipe_draw_info::max_index and
was correctly initialized for user buffers only. It was set to ~0 for
hardware buffers. Moreover, it could also be computed with:

pipe_vertex_buffer::max_index = (pipe_resource::width0 -
pipe_vertex_buffer::buffer_offset) / pipe_vertex_buffer::stride - 1

So it was already redundant. Basically, pipe_resource::width0 is also
redundant for user buffers, because it is actually computed from
pipe_draw_info::max_index too. It's all logical because the index bounds are
the only info we have about user buffers and we compute all the other
properties from it. This is how width0 is computed:

pipe_resource::width0 = (pipe_draw_info::max_index + 1) *
pipe_vertex_buffer::stride;

Now we substitute width0 in the first formula:

pipe_vertex_buffer::max_index = ((pipe_draw_info::max_index + 1) *
pipe_vertex_buffer::stride - pipe_vertex_buffer::buffer_offset) /
pipe_vertex_buffer::stride - 1

If we examine the state tracker code, we'll notice that buffer_offset is
always 0 for user buffers. After simplification, we get this:

pipe_vertex_buffer::max_index = pipe_draw_info::max_index

And that's the whole story. That said, I'd like not to call
set_vertex_buffers only to update max_index if I can get the same info from
pipe_draw_info. Because pipe_vertex_buffer::max_index was really the maximum
index value in the index buffer, we don't need to clamp anything when we
fetch vertices, the clamping would basically do nothing. That's why I
removed the clamping in Draw and put ~0 in the translate::run parameter and
in several other places.

Does this explanation justify all of my changes in the patch to you?

Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110214/d4fcc28f/attachment-0001.htm>