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

[Roland Scheidegger]

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