[Mesa-dev] [PATCH] vbo: Try to reuse the same VAO more often for successive dlists.

[Brian Paul]

On 02/28/2018 10:50 PM, Mathias.Froehlich at gmx.net wrote:
> From: Mathias Fröhlich <mathias.froehlich at web.de>
> 
> Hi Brian,
> 
> that's what I mentioned yesterday.
> You may want to test this change against your use cases of your recent
> draw primitive optimization.
> With one openscenegraph dlist based workload this change improves the
> framerates on radeonsi by about 10% compared to without that change.
> 
> If it helps, please review...

I'll try to re-check the dlist application later.  For now though, this 
patch looks good.  One thing below...


> 
> best
> Mathias
> 
> 
> The change tries to catch more opportunities to reuse the same set
> of VAO's when building up display lists. Instead of checking the
> offset with respect to the beginning of the vertex buffer object
> the change tries to apply this same optimization with respect to the
> previous display list node.
> 
> Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> ---
>   src/mesa/vbo/vbo_save_api.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/vbo/vbo_save_api.c b/src/mesa/vbo/vbo_save_api.c
> index 47ee355e72..c2a9921bb7 100644
> --- a/src/mesa/vbo/vbo_save_api.c
> +++ b/src/mesa/vbo/vbo_save_api.c
> @@ -542,11 +542,18 @@ compile_vertex_list(struct gl_context *ctx)
>   
>      /* Duplicate our template, increment refcounts to the storage structs:
>       */
> +   GLintptr old_offset = 0;
> +   if (save->VAO[0]) {
> +      old_offset = save->VAO[0]->BufferBinding[0].Offset
> +         + save->VAO[0]->VertexAttrib[VERT_ATTRIB_POS].RelativeOffset;
> +   }
>      const GLsizei stride = save->vertex_size*sizeof(GLfloat);
>      GLintptr buffer_offset =
>          (save->buffer_map - save->vertex_store->buffer_map) * sizeof(GLfloat);
> +   assert(old_offset <= buffer_offset);
> +   const GLintptr offset_diff = buffer_offset - old_offset;
>      GLuint start_offset = 0;
> -   if (0 < buffer_offset && 0 < stride && buffer_offset % stride == 0) {
> +   if (0 < offset_diff && 0 < stride && offset_diff % stride == 0) {

Minor nit: I find offset_diff > 0 && stride > 0 to be a little easier to 
read.

In any case, Reviewed-by: Brian Paul <brianp at vmware.com>


>         /* 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
> @@ -558,8 +565,9 @@ compile_vertex_list(struct gl_context *ctx)
>         /* We cannot immediately update the primitives as some methods below
>          * still need the uncorrected start vertices
>          */
> -      start_offset = buffer_offset/stride;
> -      buffer_offset = 0;
> +      start_offset = offset_diff/stride;
> +      assert(old_offset == buffer_offset - offset_diff);
> +      buffer_offset = old_offset;
>      }
>      GLuint offsets[VBO_ATTRIB_MAX];
>      for (unsigned i = 0, offset = 0; i < VBO_ATTRIB_MAX; ++i) {
> @@ -666,6 +674,9 @@ compile_vertex_list(struct gl_context *ctx)
>          */
>         free_vertex_store(ctx, save->vertex_store);
>         save->vertex_store = NULL;
> +      /* When we have a new vbo, we will for sure need a new vao */
> +      for (gl_vertex_processing_mode vpm = 0; vpm < VP_MODE_MAX; ++vpm)
> +         _mesa_reference_vao(ctx, &save->VAO[vpm], NULL);
>   
>         /* Allocate and map new store:
>          */
>