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