On 01/12/2018 03:52 PM, Ian Romanick wrote:
On 01/12/2018 02:23 PM, Brian Paul wrote:
The vbo_save_vertex_list structure records one or more glBegin/End
primitives which all have the same vertex format.
To draw these primitives, we setup the vertex array state, then
issue the drawing command. Before, the 'start' vertex was typically
zero and we used the vertex array pointer to indicate where the
vertex data starts.
This patch checks if the vertex buffer offset is an exact multiple of
the vertex size. If so, that means we can use zero-based vertex array
pointers and use the draw's start value to indicate where the vertex
data starts.
This means a series of display list drawing commands may have
identical vertex array state. This will get filtered out by the
Gallium CSO module so we can issue a tight series of drawing commands
without state changes to the device.
Note that this also works for a series of glCallList commands (not
just one list that contains multiple glBegin/End pairs).
No Piglit or conform changes.
Do you know if any of these tests actually hit these paths?
I don't. I ran Piglit/conform just to be sure there were no regressions.
I always
worry about changes to display list handing because there is so little
testing. :(
I test a number of Windows CAD apps/etc which use display lists. In
fact, this change is a result of trying to optimize one of those. It's
pretty hairy code, but I think I understand it fairly well now.
A few nits below.
---
src/mesa/vbo/vbo_save.h | 17 +++++++++++++++++
src/mesa/vbo/vbo_save_api.c | 15 +++++++++++++++
src/mesa/vbo/vbo_save_draw.c | 12 ++++++++++++
3 files changed, 44 insertions(+)
diff --git a/src/mesa/vbo/vbo_save.h b/src/mesa/vbo/vbo_save.h
index 9d13e0a..468a04a 100644
--- a/src/mesa/vbo/vbo_save.h
+++ b/src/mesa/vbo/vbo_save.h
@@ -86,6 +86,23 @@ struct vbo_save_vertex_list {
struct vbo_save_primitive_store *prim_store;
};
+
+/**
+ * Is the vertex lists's buffer offset an exact multiple of the
+ * vertex size (in bytes)? This is used to check for a vertex array /
+ * drawing optimization.
+ */
+static inline bool
+aligned_vertex_buffer_offset(const struct vbo_save_vertex_list *node)
+{
+ unsigned vertex_size = node->vertex_size * sizeof(GLfloat); /* in bytes */
+ if (vertex_size)
+ return node->buffer_offset % vertex_size == 0;
+ else
+ return false;
I think this would be more clear as
return vertex_size != 0 && node->buffer_offset % vertex_size == 0;
Ok.
+}
+
+
/* These buffers should be a reasonable size to support upload to
* hardware. Current vbo implementation will re-upload on any
* changes, so don't make too big or apps which dynamically create
diff --git a/src/mesa/vbo/vbo_save_api.c b/src/mesa/vbo/vbo_save_api.c
index 42d883f..b9d382a 100644
--- a/src/mesa/vbo/vbo_save_api.c
+++ b/src/mesa/vbo/vbo_save_api.c
@@ -546,6 +546,21 @@ compile_vertex_list(struct gl_context *ctx)
save->prim_store = alloc_prim_store();
}
+ /**
The Doxygen /** start marker doesn't do anything useful here.
right.
+ * If the vertex buffer offset is a multiple of the vertex size,
+ * we can use the _mesa_prim::start value to indicate where the
+ * vertices starts, instead of the buffer offset. Also see the
+ * bind_vertex_list() function.
+ */
+ if (aligned_vertex_buffer_offset(node)) {
+ const unsigned start_offset =
+ node->buffer_offset / (node->vertex_size * sizeof(GLfloat));
+ unsigned i;
+ for (i = 0; i < save->prim_count; i++) {
+ save->prims[i].start += start_offset;
+ }
I believe the currently common style is:
for (unsigned i = 0; i < save->prim_count; i++)
save->prims[i].start += start_offset;
Yeah, I'm stuck on old habits.
v2 coming.
-Brian
+ }
+
/* Reset our structures for the next run of vertices:
*/
reset_counters(ctx);
diff --git a/src/mesa/vbo/vbo_save_draw.c b/src/mesa/vbo/vbo_save_draw.c
index 1694a04..b63a9a8 100644
--- a/src/mesa/vbo/vbo_save_draw.c
+++ b/src/mesa/vbo/vbo_save_draw.c
@@ -146,6 +146,18 @@ bind_vertex_list(struct gl_context *ctx,
memcpy(node_attrsz, node->attrsz, sizeof(node->attrsz));
memcpy(node_attrtype, node->attrtype, sizeof(node->attrtype));
+ if (aligned_vertex_buffer_offset(node)) {
+ /* The vertex size is an exact multiple of the buffer offset.
+ * This means that we can use zero-based vertex attribute pointers
+ * and specify the start of the primitive with the _mesa_prim::start
+ * field. This results in issuing several draw calls with identical
+ * vertex attribute information. This can result in fewer state
+ * changes in drivers. In particular, the Gallium CSO module will
+ * filter out redundant vertex buffer changes.
+ */
+ buffer_offset = 0;
+ }
+
/* Install the default (ie Current) attributes first, then overlay
* all active ones.
*/
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
