On Tue, Feb 28, 2017 at 6:37 PM, Roland Scheidegger <srol...@vmware.com> wrote: > Am 28.02.2017 um 17:11 schrieb Jose Fonseca: >> On 20/02/17 20:28, Roland Scheidegger wrote: >>> Am 20.02.2017 um 20:56 schrieb Marek Olšák: >>>> On Mon, Feb 20, 2017 at 8:29 PM, Axel Davy <axel.d...@ens.fr> wrote: >>>>> On 20/02/2017 20:11, Ilia Mirkin wrote: >>>>>> >>>>>> On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák <mar...@gmail.com> 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. >> >> Yes. I think that having index_buffer != NULL implying indexed buffer, >> won't create any problems. >> >>>> >>>>>>> >>>>>>> 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... >>> >>> 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? >>> >>> Roland >> >> https://msdn.microsoft.com/en-us/library/windows/desktop/dn899216(v=vs.85).aspx >> says: >> >> Index data reads must be a multiple of the index data type size (i.e. >> only from addresses that are naturally aligned for the data). > Ahh that was what I couldn't find... > >> >> But still, I think index buffer offsets should be in bytes, for >> consistency, with APIs, and vertex buffer offsets. See: > Well I suppose it's sort of inconsistent either way, because buffer > offsets are usually in bytes, but start indices in terms of elements. > But we should be able to live with it either way - one or the other has > to be converted depending on the type.
It's not really about consistency. Vertex buffers really need a byte offset, because e.g. the offset of R8G8B8A8_UNORM doesn't have to a multiple of element size (4), it must only be a multiple of channel size (1). However, index buffers have only one "channel" per element, thus start/count is sufficient. Marek _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev