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

[Roland Scheidegger]

Am 21.02.2017 um 16:49 schrieb Axel Davy:
> On 21/02/2017 16:00, Roland Scheidegger wrote:
>> Am 21.02.2017 um 11:46 schrieb 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."
>>>
>> Ah, nice find (not in the section I was looking for...). I suppose it
>> isn't expected to work then (albeit it's interesting that it's still not
>> an actual gl error anywhere).
>>
>> Roland
>>
> What is a basic machine unit ? I_ it supposed to be index_size, or just
> the required alignment for the platform ?

Basic machine unit is always one byte for any known implementation.

Roland