On Mon, 2011-02-14 at 11:04 -0800, Marek Olšák wrote: > On Mon, Feb 14, 2011 at 6:58 PM, José Fonseca > <jfons...@vmware.com<mailto:jfons...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
