On 20/02/17 20:28, Roland Scheidegger wrote:
Am 20.02.2017 um 20:56 schrieb Marek Olšák:
On Mon, Feb 20, 2017 at 8:29 PM, Axel Davy <axel.d...@ens.fr> wrote:
On 20/02/2017 20:11, Ilia Mirkin wrote:

On Mon, Feb 20, 2017 at 2:01 PM, Marek Olšák <mar...@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.

Yes. I think that having index_buffer != NULL implying indexed buffer, won't create any problems.



     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

https://msdn.microsoft.com/en-us/library/windows/desktop/dn899216(v=vs.85).aspx says:

Index data reads must be a multiple of the index data type size (i.e. only from addresses that are naturally aligned for the data).

But still, I think index buffer offsets should be in bytes, for consistency, with APIs, and vertex buffer offsets. See:


https://msdn.microsoft.com/en-us/library/windows/desktop/ff476898(v=vs.85).aspx#Index_Buffer


Having index buffer offsets being in elements is confusing -- it doesn't match the D3D10 API or GL API (since both take bytes), and it's semantically identical to pipe_draw_info::start member.


That is, we if don't want index buffer offset in bytes, then we should eliminate it alltogether...


I guess the question is: does hardware have different registers for "IndexBufferOffset" and "StartIndexLocation", or do these always get added together?


I think the usefulness of having separate entries is that it allows one to use linear suballocation of index buffer, and specify the sub-buffer offset without having to worry about the index data type of each draw call. But perhaps this is a nicety of the API, not HW.


Jose
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to