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

[Marek Olšák]

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

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