Hi Mathias,
Just putting forward some suggestions. Please take them with a pinch of salt.
On 6 March 2018 at 07:48, <mathias.froehl...@gmx.net> wrote:
> From: Mathias Fröhlich <mathias.froehl...@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.froehl...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
