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

[Roland Scheidegger]

Am 20.02.2017 um 21:58 schrieb 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.
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__irrlicht.sourceforge.net_forum_viewtopic.php-3Ff-3D7-26t-3D51444&d=DwIFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=_nDDEb8aspFcYmYCdx9G-Pfs4rRVzx4rodfxnJNkNyc&s=XPduNXrH7SGk7lVUD2izbWAOfERG60bJWTsI600UWCg&e= ).
>> 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.
I did some quick tests and I believe d3d10 doesn't actually require
offsets not aligned to index size, but don't quote me on that.
Nevertheless, I've got the feeling this might be expected to work with
GL - doesn't look like it would be an error if your indices "pointer" in
glDrawElements() isn't aligned, and if it's not an error I don't see why
it wouldn't be well defined?
(FWIW x86 supports this fine, but indeed not all cpu archs might. Even
AVX2 gather supports non-aligned lookups - of course just for uint
indices since gather doesn't support smaller than 32bit gathers.)


> 
>>
>> 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
>