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

[Marek Olšák]

On Tue, Feb 28, 2017 at 5:11 PM, Jose Fonseca <jfonseca at vmware.com> 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 <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?

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 are dropped.
The relative offset could be folded into "start" in this case.

For indirect rendering, we have the index buffer address, which is GPU
address + relative offset. "start" is read from the indirect buffer.
That means we have to keep the relative offset, but we can limit its
use to indirect rendering only.

Marek