Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
On Tue, Feb 28, 2017 at 6:37 PM, Roland Scheideggerwrote: > 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 wrote: > On 20/02/2017 20:11, Ilia Mirkin wrote: >> >> On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=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 ___
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
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 Davywrote: On 20/02/2017 20:11, Ilia Mirkin wrote: > > On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=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. Roland > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ff476898(v=vs.85).aspx#Index_Buffer > > > > Having index buffer offsets being in elements is confusing -- it doesn't > match the D3D10 API or GL API (since both take bytes), and it's > semantically identical to pipe_draw_info::start member. > > > That is, we if don't want index buffer offset in bytes, then we should > eliminate it alltogether... > > > I guess the question is: does hardware have different registers for > "IndexBufferOffset" and "StartIndexLocation", or
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
On Tue, Feb 28, 2017 at 5:11 PM, Jose Fonsecawrote: > 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 wrote: On 20/02/2017 20:11, Ilia Mirkin wrote: > > > On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=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). > > But still, I think index buffer offsets should be in bytes, for consistency, > with APIs, and vertex buffer offsets. See: > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ff476898(v=vs.85).aspx#Index_Buffer > > > Having index buffer offsets being in elements is confusing -- it doesn't > match the D3D10 API or GL API (since both take bytes), and it's semantically > identical to pipe_draw_info::start member. > > > That is, we if don't want index buffer offset in bytes, then we should > eliminate it alltogether... > > > I guess the question is: does hardware have different registers for > "IndexBufferOffset" and "StartIndexLocation", or do these always get added > together? For direct rendering, we only have the index buffer address, which combines GPU address + relative offset + start*index_size. It must be aligned to the element size, meaning that the low bits
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
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 Davywrote: On 20/02/2017 20:11, Ilia Mirkin wrote: On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=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). But still, I think index buffer offsets should be in bytes, for consistency, with APIs, and vertex buffer offsets. See: https://msdn.microsoft.com/en-us/library/windows/desktop/ff476898(v=vs.85).aspx#Index_Buffer Having index buffer offsets being in elements is confusing -- it doesn't match the D3D10 API or GL API (since both take bytes), and it's semantically identical to pipe_draw_info::start member. That is, we if don't want index buffer offset in bytes, then we should eliminate it alltogether... I guess the question is: does hardware have different registers for "IndexBufferOffset" and "StartIndexLocation", or do these always get added together? I think the usefulness of having separate entries is that it allows one to use linear suballocation of index buffer, and specify the sub-buffer offset without having to worry about the index data type of each draw call. But perhaps this is a nicety of the API, not HW. Jose ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
Am 21.02.2017 um 16:49 schrieb Axel Davy: > On 21/02/2017 16:00, Roland Scheidegger wrote: >> Am 21.02.2017 um 11:46 schrieb Marek Olšák: >>> On Tue, Feb 21, 2017 at 4:36 AM, Roland Scheidegger >>>wrote: Am 20.02.2017 um 21:58 schrieb Marek Olšák: > On Mon, Feb 20, 2017 at 9:28 PM, 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 wrote: On 20/02/2017 20:11, Ilia Mirkin wrote: > On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák > 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=DwIFaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=_nDDEb8aspFcYmYCdx9G-Pfs4rRVzx4rodfxnJNkNyc=XPduNXrH7SGk7lVUD2izbWAOfERG60bJWTsI600UWCg= >> ). >> 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.) >>> From GL 4.4 Core, section 6.2: >>> >>> "Clients must align
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
On 21/02/2017 16:00, Roland Scheidegger wrote: Am 21.02.2017 um 11:46 schrieb Marek Olšák: On Tue, Feb 21, 2017 at 4:36 AM, Roland Scheideggerwrote: Am 20.02.2017 um 21:58 schrieb Marek Olšák: On Mon, Feb 20, 2017 at 9:28 PM, 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 wrote: On 20/02/2017 20:11, Ilia Mirkin wrote: On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=DwIFaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=_nDDEb8aspFcYmYCdx9G-Pfs4rRVzx4rodfxnJNkNyc=XPduNXrH7SGk7lVUD2izbWAOfERG60bJWTsI600UWCg= ). 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.) From GL 4.4 Core, section 6.2: "Clients must align data elements consistent with the requirements of the client platform, with an additional base-level requirement that an offset within a buffer to a datum comprising N basic machine units be a multiple of N." Ah, nice find (not in the section I was looking for...). I suppose it isn't expected to work then (albeit it's interesting that it's still not an actual gl error anywhere). Roland What is a basic machine unit ? I_ it supposed to be index_size, or just the required alignment for the platform ? Axel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
Am 21.02.2017 um 11:46 schrieb Marek Olšák: > On Tue, Feb 21, 2017 at 4:36 AM, Roland Scheidegger> wrote: >> Am 20.02.2017 um 21:58 schrieb Marek Olšák: >>> On Mon, Feb 20, 2017 at 9:28 PM, 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 wrote: >> On 20/02/2017 20:11, Ilia Mirkin wrote: >>> >>> On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=DwIFaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=_nDDEb8aspFcYmYCdx9G-Pfs4rRVzx4rodfxnJNkNyc=XPduNXrH7SGk7lVUD2izbWAOfERG60bJWTsI600UWCg= ). 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.) > > From GL 4.4 Core, section 6.2: > > "Clients must align data elements consistent with the requirements of > the client platform, with an additional base-level requirement that an > offset within a buffer to a datum comprising N basic machine units be > a multiple of N." > Ah, nice find (not in the section I was looking for...). I suppose it isn't expected to work then (albeit it's interesting that it's still not an actual gl error
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
On Tue, Feb 21, 2017 at 4:36 AM, Roland Scheideggerwrote: > Am 20.02.2017 um 21:58 schrieb Marek Olšák: >> On Mon, Feb 20, 2017 at 9:28 PM, 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 wrote: > On 20/02/2017 20:11, Ilia Mirkin wrote: >> >> On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=DwIFaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=_nDDEb8aspFcYmYCdx9G-Pfs4rRVzx4rodfxnJNkNyc=XPduNXrH7SGk7lVUD2izbWAOfERG60bJWTsI600UWCg= >>> ). >>> 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.) From GL 4.4 Core, section 6.2: "Clients must align data elements consistent with the requirements of the client platform, with an additional base-level requirement that an offset within a buffer to a datum comprising N basic machine units be a multiple of N." Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
Am 20.02.2017 um 21:58 schrieb Marek Olšák: > On Mon, Feb 20, 2017 at 9:28 PM, 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 wrote: On 20/02/2017 20:11, Ilia Mirkin wrote: > > On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=DwIFaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=_nDDEb8aspFcYmYCdx9G-Pfs4rRVzx4rodfxnJNkNyc=XPduNXrH7SGk7lVUD2izbWAOfERG60bJWTsI600UWCg= >> ). >> 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 >
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
On Mon, Feb 20, 2017 at 9:28 PM, Roland Scheideggerwrote: > Am 20.02.2017 um 20:56 schrieb Marek Olšák: >> On Mon, Feb 20, 2017 at 8:29 PM, Axel Davy wrote: >>> On 20/02/2017 20:11, Ilia Mirkin wrote: On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=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 mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
Am 20.02.2017 um 20:56 schrieb Marek Olšák: > On Mon, Feb 20, 2017 at 8:29 PM, Axel Davywrote: >> On 20/02/2017 20:11, Ilia Mirkin wrote: >>> >>> On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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=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 > + void *user_indices; + /* Pointers must be at the end for an optimal structure layout on 64-bit. */ Comments welcome, Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=VuVSYjOwW2vK7aFDJK8g1jjWk4YikjJ6y3TJ5mFdkl0=2zbVOE2ZE3M8z7z3IKYDRHx9SPh81WhmgilHm2qrPZ8= >>> >>> ___ >>> mesa-dev mailing list >>> mesa-dev@lists.freedesktop.org >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0=VuVSYjOwW2vK7aFDJK8g1jjWk4YikjJ6y3TJ5mFdkl0=2zbVOE2ZE3M8z7z3IKYDRHx9SPh81WhmgilHm2qrPZ8= >>> >> >> >> My understanding of the current interface is that setting the index buffer >> may be costly. Thus you want to set once, and use for several draw calls >> with different start or other changes. > > Not true. It's there only because the interface is based on DX10. > > Marek > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org >
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
On Mon, Feb 20, 2017 at 8:29 PM, Axel Davywrote: > On 20/02/2017 20:11, Ilia Mirkin wrote: >> >> On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák 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. >>> >>> 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. >>> >>> + void *user_indices; >>> + >>> /* Pointers must be at the end for an optimal structure layout on >>> 64-bit. */ >>> >>> >>> Comments welcome, >>> >>> Marek >>> ___ >>> mesa-dev mailing list >>> mesa-dev@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > My understanding of the current interface is that setting the index buffer > may be costly. Thus you want to set once, and use for several draw calls > with different start or other changes. Not true. It's there only because the interface is based on DX10. Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
On 20/02/2017 20:11, Ilia Mirkin wrote: On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšákwrote: 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 ? 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. But we really want to have a byte offset. + void *user_indices; + /* Pointers must be at the end for an optimal structure layout on 64-bit. */ Comments welcome, Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev My understanding of the current interface is that setting the index buffer may be costly. Thus you want to set once, and use for several draw calls with different start or other changes. If setting the index buffer is indeed costly, and independant of the draw calls, the proposed change makes it a bit harder for the driver (it's as if we are setting the index buffer ever draw call, even if it didn't really change). For the state tracker, it doesn't make much difference. Axel Axel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšákwrote: > 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 */ > 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? > + void *user_indices; > + > /* Pointers must be at the end for an optimal structure layout on 64-bit. > */ > > > Comments welcome, > > Marek > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Gallium: Removal of set_index_buffer (discussion)
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 */ 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 */ + void *user_indices; + /* Pointers must be at the end for an optimal structure layout on 64-bit. */ Comments welcome, Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev