Re: [Mesa-dev] Gallium: Removal of set_index_buffer (discussion)

[Marek Olšák]

On Tue, Feb 28, 2017 at 6:37 PM, Roland Scheidegger  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  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)

[Roland Scheidegger]

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.

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)

[Marek Olšák]

On Tue, Feb 28, 2017 at 5:11 PM, Jose Fonseca  wrote:
> 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)

[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).


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)

[Roland Scheidegger]

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)

[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 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)

[Roland Scheidegger]

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)

[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."

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)

[Roland Scheidegger]

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)

[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.
> 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)

[Roland Scheidegger]

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...

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)

[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.

>>>
>>>  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)

[Axel Davy]

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 ?

 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)

[Ilia Mirkin]

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 */
> 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)

[Marek Olšák]

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