Am 20.02.2017 um 21:58 schrieb Marek Olšák: > On Mon, Feb 20, 2017 at 9:28 PM, Roland Scheidegger <srol...@vmware.com> > 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. >> >>> >>>>>> >>>>>> 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. >> https://urldefense.proofpoint.com/v2/url?u=http-3A__irrlicht.sourceforge.net_forum_viewtopic.php-3Ff-3D7-26t-3D51444&d=DwIFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=_nDDEb8aspFcYmYCdx9G-Pfs4rRVzx4rodfxnJNkNyc&s=XPduNXrH7SGk7lVUD2izbWAOfERG60bJWTsI600UWCg&e= >> ). >> 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. I did some quick tests and I believe d3d10 doesn't actually require offsets not aligned to index size, but don't quote me on that. Nevertheless, I've got the feeling this might be expected to work with GL - doesn't look like it would be an error if your indices "pointer" in glDrawElements() isn't aligned, and if it's not an error I don't see why it wouldn't be well defined? (FWIW x86 supports this fine, but indeed not all cpu archs might. Even AVX2 gather supports non-aligned lookups - of course just for uint indices since gather doesn't support smaller than 32bit gathers.)
> >> >> 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 mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev