[Mesa-dev] [PATCH] mesa: Make gl_vertex_array contain pointers to first order VAO members.

[Emil Velikov]

Hi Mathias,

Just putting forward some suggestions. Please take them with a pinch of salt.

On 6 March 2018 at 07:48,  <Mathias.Froehlich at gmx.net> wrote:
> From: Mathias Fröhlich <mathias.froehlich at web.de>
>
> Hi,
>
> The change basically strips down gl_vertex_array to two pointers
> into the VAO. Consequently the current vertex attributes in the
> vbo module need to be represented by structs present in the VAO
> instead of gl_vertex_array instances.
> The change prepares the backends somewhat to use the _DrawVAO for
> draw operations in the longer term.
>
> The change introduced no piglit quick regressions on classic swrast
> to test drivers using tnl, i965 and radeonsi.
>
> Please review!
> Thanks!
>
> best
> Mathias
>
>
> Instead of keeping a copy of the vertex array content in
> struct gl_vertex_array only keep pointers to the first order
> information originaly in the VAO.
> For that represent the current values by struct gl_array_attributes
> and struct gl_vertex_buffer_binding.
>
> Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> ---
>  src/mesa/drivers/dri/i965/brw_context.h       |   2 +-
>  src/mesa/drivers/dri/i965/brw_draw.c          |  30 +++---
>  src/mesa/drivers/dri/i965/brw_draw_upload.c   | 130 ++++++++++++++------------
>  src/mesa/drivers/dri/i965/genX_state_upload.c |  23 +++--
>  src/mesa/drivers/dri/nouveau/nouveau_vbo_t.c  |  86 ++++++++++-------
>  src/mesa/main/arrayobj.c                      |  16 ----
>  src/mesa/main/attrib.c                        |   1 -
>  src/mesa/main/mtypes.h                        |  47 +++-------
>  src/mesa/main/varray.c                        |  21 -----
>  src/mesa/main/varray.h                        |  49 +++-------
>  src/mesa/state_tracker/st_atom.c              |   7 +-
>  src/mesa/state_tracker/st_atom_array.c        | 115 ++++++++++++++---------
>  src/mesa/state_tracker/st_cb_rasterpos.c      |  26 +++---
>  src/mesa/state_tracker/st_draw_feedback.c     |  46 ++++++---
>  src/mesa/tnl/t_draw.c                         |  81 ++++++++--------
>  src/mesa/tnl/t_rebase.c                       |  20 ++--
>  src/mesa/tnl/t_rebase.h                       |   2 +-
>  src/mesa/vbo/vbo.h                            |   4 +-
>  src/mesa/vbo/vbo_context.c                    |  52 +++++------
>  src/mesa/vbo/vbo_exec.c                       |  16 ++--
>  src/mesa/vbo/vbo_exec_api.c                   |  22 ++---
>  src/mesa/vbo/vbo_private.h                    |   3 +-
>  src/mesa/vbo/vbo_save_draw.c                  |   2 +-
>  src/mesa/vbo/vbo_split.c                      |   2 +-
>  src/mesa/vbo/vbo_split.h                      |   4 +-
>  src/mesa/vbo/vbo_split_copy.c                 |  97 +++++++++++--------
>  src/mesa/vbo/vbo_split_inplace.c              |   6 +-
>  27 files changed, 480 insertions(+), 430 deletions(-)
>
Wondering if one cannot split this up somehow. Perhaps introduce
gl_vertex_array::VertexAttrib first and the gl_vertex_buffer_binding
struct as 2/2?
It's a big thing to suggest, yet it would make it easier to catch all
the subtleties.


> @@ -462,21 +459,8 @@ void
>  _mesa_update_vao_derived_arrays(struct gl_context *ctx,
>                                  struct gl_vertex_array_object *vao)
>  {
> -   GLbitfield arrays = vao->NewArrays;
> -
>     /* Make sure we do not run into problems with shared objects */
>     assert(!vao->SharedAndImmutable || vao->NewArrays == 0);
> -
> -   while (arrays) {
> -      const int attrib = u_bit_scan(&arrays);
> -      struct gl_vertex_array *array = &vao->_VertexArray[attrib];
> -      const struct gl_array_attributes *attribs =
> -         &vao->VertexAttrib[attrib];
> -      const struct gl_vertex_buffer_binding *buffer_binding =
> -         &vao->BufferBinding[attribs->BufferBindingIndex];
> -
> -      _mesa_update_vertex_array(ctx, array, attribs, buffer_binding);
> -   }
... and we can drop _mesa_update_vao_derived_arrays() with a follow-up patch?


> +/**
> + * Vertex array information which is derived from gl_array_attributes
> + * and gl_vertex_buffer_binding information.  Used by the VBO module and
> + * device drivers.
> + */
> +struct gl_vertex_array
> +{
> +   /** Vertex attribute array */
> +   const struct gl_array_attributes *VertexAttrib;
> +   /** Vertex buffer binding */
> +   const struct gl_vertex_buffer_binding *BufferBinding;

Worth expanding the documentation to cover the const-ness or lifetime?
AKA why were we coping all of the data before, and we don't need to now.

Mildly related:
AEarray and AEattrib look surprisingly similar to gl_vertex_array now.
I'm guessing that you're heading there next?

> +/**
> + * Copy one vertex array to another.
> + */
> +static inline void
> +_mesa_copy_vertex_array(struct gl_vertex_array *dst,
> +                        const struct gl_vertex_array *src)
> +{
> +   dst->VertexAttrib = src->VertexAttrib;
> +   dst->BufferBinding = src->BufferBinding;
> +}
> +
Say "Shallow copy" in the inline comment?

HTH
Emil