On Mon, Feb 20, 2017 at 9:28 PM, Roland Scheidegger <[email protected]> wrote: > Am 20.02.2017 um 20:56 schrieb Marek Olšák: >> On Mon, Feb 20, 2017 at 8:29 PM, Axel Davy <[email protected]> wrote: >>> On 20/02/2017 20:11, Ilia Mirkin wrote: >>>> >>>> On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák <[email protected]> wrote: >>>>> >>>>> Hi, >>>>> >>>>> I'd like to remove pipe_context::set_index_buffer. It's not useful to >>>>> most drivers and the interface is inconvenient for Mesa/OpenGL, >>>>> because it's a draw state that is set with a separate driver callback, >>>>> which is an unnecessary driver roundtrip taking some CPU cycles. I'd >>>>> prefer to pass the index buffer via pipe_draw_info. >>>>> >>>>> I'm aware that the interface was inherited from DX10, but I don't >>>>> think that makes any difference here. DX10 state trackers can pass the >>>>> index buffer via pipe_draw_info too. >>>>> >>>>> This is my proposal: >>>>> >>>>> iff --git a/src/gallium/include/pipe/p_state.h >>>>> b/src/gallium/include/pipe/p_state.h >>>>> index ce19b92..cffbb33 100644 >>>>> --- a/src/gallium/include/pipe/p_state.h >>>>> +++ b/src/gallium/include/pipe/p_state.h >>>>> @@ -635,7 +635,7 @@ struct pipe_index_buffer >>>>> */ >>>>> struct pipe_draw_info >>>>> { >>>>> - boolean indexed; /**< use index buffer */ >>>>> + ubyte index_size; /**< 0 = non-indexed */ >>> >>> Isn't that enough to say non-index when index_buffer and user_indices are >>> NULL ? >> >> We still need index_size and it's only 8 bits as opposed to 64 bits. > FWIW at least in d3d10 you can actually have indexed rendering without > an index buffer bound. This is perfectly valid, you're just expected to > return always zero for all indices... Albeit I believe we actually deal > with this with a dummy buffer. > >> >>>>> >>>>> enum pipe_prim_type mode; /**< the mode of the primitive */ >>>>> boolean primitive_restart; >>>>> ubyte vertices_per_patch; /**< the number of vertices per patch */ >>>>> @@ -666,12 +666,18 @@ struct pipe_draw_info >>>>> >>>>> unsigned indirect_params_offset; /**< must be 4 byte aligned */ >>>>> >>>>> + /** >>>>> + * Index buffer. Only one can be non-NULL. >>>>> + */ >>>>> + struct pipe_resource *index_buffer; /* "start" is the offset */ >>>> >>>> Works for me. Is start the offset in bytes or is start * index_size >>>> the offset in bytes? >>> >>> Same question here. My understanding is that start is in terms of start * >>> index_size bytes. >> >> offset = start * index_size; >> >>> But we really want to have a byte offset. >> >> The offset should be aligned to index_size, otherwise hardware won't work. > Are you sure of that? d3d10 doesn't seem to have such a requirement, or > if it has I can't find it now (so, the startIndex really is in terms of > index units, but the offset of the buffer is in terms of bytes, and the > docs don't seem to mention it's limited to index alignment). > I don't actually see such a limitation in GL neither, albeit some quick > googling seems to suggest YMMV (e.g. > http://irrlicht.sourceforge.net/forum/viewtopic.php?f=7&t=51444). > So, I can't quite tell right now if we really need byte offsets...
It's a natural requirement of hardware. It doesn't have to be documented IMO. CPUs might not support it either. > > Otherwise we should be able to deal with the interface change (that > said, arguably the old one is quite consistent with the analogous > set_vertex_buffers call - vulkan also has two analogous entry points for > setting vertex and index buffers, so it can't be all that wrong). > Do you have some evidence this really saves some measurable cpu cycles? Yes it can be measurable, but it's not massive. setup_index_buffer is close to 1% in torcs. We can also lose time elsewhere due to cache evictions. It's never just about CPU time in one function. My plan is: - get rid of pipe_index_buffer - get rid of _mesa_index_buffer - make _mesa_prim the same as pipe_draw_info - pretty much set pipe_draw_info in the vbo module In light of that, the performance question of set_index_buffer has little relevance. Marek _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
