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

[Roland Scheidegger]

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

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






> 
>>>>
>>>> +   void *user_indices;
>>>> +
>>>>      /* Pointers must be at the end for an optimal structure layout on
>>>> 64-bit. */
>>>>
>>>>
>>>> Comments welcome,
>>>>
>>>> Marek
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=VuVSYjOwW2vK7aFDJK8g1jjWk4YikjJ6y3TJ5mFdkl0&s=2zbVOE2ZE3M8z7z3IKYDRHx9SPh81WhmgilHm2qrPZ8&e= 
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=VuVSYjOwW2vK7aFDJK8g1jjWk4YikjJ6y3TJ5mFdkl0&s=2zbVOE2ZE3M8z7z3IKYDRHx9SPh81WhmgilHm2qrPZ8&e= 
>>
>>
>> My understanding of the current interface is that setting the index buffer
>> may be costly. Thus you want to set once, and use for several draw calls
>> with different start or other changes.
> 
> Not true. It's there only because the interface is based on DX10.
> 
> Marek
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=VuVSYjOwW2vK7aFDJK8g1jjWk4YikjJ6y3TJ5mFdkl0&s=2zbVOE2ZE3M8z7z3IKYDRHx9SPh81WhmgilHm2qrPZ8&e= 
>