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

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


Axel