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

[Marek Olšák]

On Mon, Feb 20, 2017 at 9:28 PM, Roland Scheidegger <sroland at vmware.com> 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.
>
>>
>>>>>
>>>>>      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...

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