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

[José Fonseca]

On Mon, 2011-02-14 at 11:04 -0800, Marek Olšák wrote:
> On Mon, Feb 14, 2011 at 6:58 PM, José Fonseca <jfonseca at vmware.com<mailto: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?

As I said, I'm perfectly fine with the removal of max_index.

Mesa state tracker may not have been doing the right thing before. It
should have been

   max_index = (pipe_vertex_buffer::size - pipe_vertex_buffer::buffer_offset - util_format_size(vertex_element->format)) / pipe_vertex_buffer::stride + 1

this is the logic that needs to be reinstated in the draw module to prevent buffer overflows spite bogus indices. But no sweat -- I have a few related draw changes on the pipe and I can easily do this.

Jose