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

[Marek Olšák]

On Tue, Feb 21, 2017 at 4:36 AM, Roland Scheidegger <sroland at vmware.com> wrote:
> 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.)

>From GL 4.4 Core, section 6.2:

"Clients must align data elements consistent with the requirements of
the client platform, with an additional base-level requirement that an
offset within a buffer to a datum comprising N basic machine units be
a multiple of N."

Marek