[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 <axel.davy at ens.fr> wrote:
>>> On 20/02/2017 20:11, Ilia Mirkin wrote:
>>>>
>>>> On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák <maraeo at gmail.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I'd like to remove pipe_context::set_index_buffer. It's not useful to
>>>>> most drivers and the interface is inconvenient for Mesa/OpenGL,
>>>>> because it's a draw state that is set with a separate driver callback,
>>>>> which is an unnecessary driver roundtrip taking some CPU cycles. I'd
>>>>> prefer to pass the index buffer via pipe_draw_info.
>>>>>
>>>>> I'm aware that the interface was inherited from DX10, but I don't
>>>>> think that makes any difference here. DX10 state trackers can pass the
>>>>> index buffer via pipe_draw_info too.
>>>>>
>>>>> This is my proposal:
>>>>>
>>>>> iff --git a/src/gallium/include/pipe/p_state.h
>>>>> b/src/gallium/include/pipe/p_state.h
>>>>> index ce19b92..cffbb33 100644
>>>>> --- a/src/gallium/include/pipe/p_state.h
>>>>> +++ b/src/gallium/include/pipe/p_state.h
>>>>> @@ -635,7 +635,7 @@ struct pipe_index_buffer
>>>>>    */
>>>>>   struct pipe_draw_info
>>>>>   {
>>>>> -   boolean indexed;  /**< use index buffer */
>>>>> +   ubyte index_size;  /**< 0 = non-indexed */
>>>
>>> Isn't that enough to say non-index when index_buffer and user_indices are
>>> NULL ?
>>
>> We still need index_size and it's only 8 bits as opposed to 64 bits.
> FWIW at least in d3d10 you can actually have indexed rendering without
> an index buffer bound. This is perfectly valid, you're just expected to
> return always zero for all indices... Albeit I believe we actually deal
> with this with a dummy buffer.

Yes.  I think that having index_buffer != NULL implying indexed buffer, 
won't create any problems.

>>
>>>>>
>>>>>      enum pipe_prim_type mode;  /**< the mode of the primitive */
>>>>>      boolean primitive_restart;
>>>>>      ubyte vertices_per_patch; /**< the number of vertices per patch */
>>>>> @@ -666,12 +666,18 @@ struct pipe_draw_info
>>>>>
>>>>>      unsigned indirect_params_offset; /**< must be 4 byte aligned */
>>>>>
>>>>> +   /**
>>>>> +    * Index buffer. Only one can be non-NULL.
>>>>> +    */
>>>>> +   struct pipe_resource *index_buffer; /* "start" is the offset */
>>>>
>>>> Works for me. Is start the offset in bytes or is start * index_size
>>>> the offset in bytes?
>>>
>>> Same question here. My understanding is that start is in terms of start *
>>> index_size bytes.
>>
>> offset = start * index_size;
>>
>>> But we really want to have a byte offset.
>>
>> The offset should be aligned to index_size, otherwise hardware won't work.
> Are you sure of that? d3d10 doesn't seem to have such a requirement, or
> if it has I can't find it now (so, the startIndex really is in terms of
> index units, but the offset of the buffer is in terms of bytes, and the
> docs don't seem to mention it's limited to index alignment).
> I don't actually see such a limitation in GL neither, albeit some quick
> googling seems to suggest YMMV (e.g.
> http://irrlicht.sourceforge.net/forum/viewtopic.php?f=7&t=51444).
> So, I can't quite tell right now if we really need byte offsets...
>
> Otherwise we should be able to deal with the interface change (that
> said, arguably the old one is quite consistent with the analogous
> set_vertex_buffers call - vulkan also has two analogous entry points for
> setting vertex and index buffers, so it can't be all that wrong).
> Do you have some evidence this really saves some measurable cpu cycles?
>
> Roland

https://msdn.microsoft.com/en-us/library/windows/desktop/dn899216(v=vs.85).aspx 
says:

   Index data reads must be a multiple of the index data type size (i.e. 
only from addresses that are naturally aligned for the data).

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