[Mesa-dev] [PATCH 1/8] vbo: Correctly handle attribute offsets in dlist draw.
Brian Paul
brianp at vmware.com
Wed Jan 31 16:54:55 UTC 2018
Hi Matthias,
Nice work! It's nice to get rid of some of those attribute loops.
The series looks good to me, just assorted nit-picks and comments on a
few patches.
-Brian
On 01/31/2018 12:55 AM, Mathias.Froehlich at gmx.net wrote:
> From: Mathias Fröhlich <mathias.froehlich at web.de>
>
> When executing a display list draw, for the offset
> list to be correct, the offset computation needs to
> accumulate all attribute size values in order.
> Specifically, if we are shuffling around the position
> and generic0 attributes, we may violate the order or
> if we do not walk the generic vbo attributes we may
> skip some of the attributes.
> Even if this is an unlikely usecase we can fix this
"use case"
> by precomputing the offsets on the full attribute list
> and store the full offset list in the display list node.
>
> v2: Formatting fix
> v3: Rebase
>
> Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> ---
> src/mesa/vbo/vbo_save.h | 1 +
> src/mesa/vbo/vbo_save_api.c | 19 ++++++++++++++++++
> src/mesa/vbo/vbo_save_draw.c | 47 +++++++++++++++-----------------------------
> 3 files changed, 36 insertions(+), 31 deletions(-)
>
> diff --git a/src/mesa/vbo/vbo_save.h b/src/mesa/vbo/vbo_save.h
> index 1dc66a598e..cb0bff2341 100644
> --- a/src/mesa/vbo/vbo_save.h
> +++ b/src/mesa/vbo/vbo_save.h
> @@ -64,6 +64,7 @@ struct vbo_save_vertex_list {
> GLbitfield64 enabled; /**< mask of enabled vbo arrays. */
> GLubyte attrsz[VBO_ATTRIB_MAX];
> GLenum16 attrtype[VBO_ATTRIB_MAX];
> + GLuint offsets[VBO_ATTRIB_MAX];
> GLuint vertex_size; /**< size in GLfloats */
>
> /* Copy of the final vertex from node->vertex_store->bufferobj.
> diff --git a/src/mesa/vbo/vbo_save_api.c b/src/mesa/vbo/vbo_save_api.c
> index a0fb62d814..fb51bdb84e 100644
> --- a/src/mesa/vbo/vbo_save_api.c
> +++ b/src/mesa/vbo/vbo_save_api.c
> @@ -420,6 +420,8 @@ compile_vertex_list(struct gl_context *ctx)
> {
> struct vbo_save_context *save = &vbo_context(ctx)->save;
> struct vbo_save_vertex_list *node;
> + GLuint offset;
> + unsigned i;
>
> /* Allocate space for this structure in the display list currently
> * being compiled.
> @@ -443,6 +445,23 @@ compile_vertex_list(struct gl_context *ctx)
> node->vertex_size = save->vertex_size;
> node->buffer_offset =
> (save->buffer_map - save->vertex_store->buffer_map) * sizeof(GLfloat);
> + 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.
> + */
> + offset = 0;
> + } else {
> + offset = node->buffer_offset;
> + }
> + for (i = 0; i < VBO_ATTRIB_MAX; ++i) {
> + node->offsets[i] = offset;
> + offset += node->attrsz[i] * sizeof(GLfloat);
> + }
> node->vertex_count = save->vert_count;
> node->wrap_count = save->copied.nr;
> node->dangling_attr_ref = save->dangling_attr_ref;
> diff --git a/src/mesa/vbo/vbo_save_draw.c b/src/mesa/vbo/vbo_save_draw.c
> index fd0ccc1230..291d99ed9a 100644
> --- a/src/mesa/vbo/vbo_save_draw.c
> +++ b/src/mesa/vbo/vbo_save_draw.c
> @@ -26,6 +26,7 @@
> * Keith Whitwell <keithw at vmware.com>
> */
>
> +#include <stdbool.h>
> #include "main/glheader.h"
> #include "main/bufferobj.h"
> #include "main/context.h"
> @@ -137,29 +138,10 @@ bind_vertex_list(struct gl_context *ctx,
> struct vbo_context *vbo = vbo_context(ctx);
> struct vbo_save_context *save = &vbo->save;
> struct gl_vertex_array *arrays = save->arrays;
> - GLuint buffer_offset = node->buffer_offset;
> const GLubyte *map;
> GLuint attr;
> - GLubyte node_attrsz[VBO_ATTRIB_MAX]; /* copy of node->attrsz[] */
> - GLenum16 node_attrtype[VBO_ATTRIB_MAX]; /* copy of node->attrtype[] */
> GLbitfield varying_inputs = 0x0;
> -
> - STATIC_ASSERT(sizeof(node_attrsz) == sizeof(node->attrsz));
> - memcpy(node_attrsz, node->attrsz, sizeof(node->attrsz));
> - STATIC_ASSERT(sizeof(node_attrtype) == sizeof(node->attrtype));
> - 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;
> - }
> + bool generic_from_pos = false;
>
> /* Install the default (ie Current) attributes first */
> for (attr = 0; attr < VERT_ATTRIB_FF_MAX; attr++) {
> @@ -191,10 +173,7 @@ bind_vertex_list(struct gl_context *ctx,
> ctx->VertexProgram._Current->info.inputs_read;
> if ((inputs_read & VERT_BIT_POS) == 0 &&
> (inputs_read & VERT_BIT_GENERIC0)) {
> - save->inputs[VERT_ATTRIB_GENERIC0] = save->inputs[0];
> - node_attrsz[VERT_ATTRIB_GENERIC0] = node_attrsz[0];
> - node_attrtype[VERT_ATTRIB_GENERIC0] = node_attrtype[0];
> - node_attrsz[0] = 0;
> + generic_from_pos = true;
> }
> break;
> default:
> @@ -203,31 +182,37 @@ bind_vertex_list(struct gl_context *ctx,
>
> for (attr = 0; attr < VERT_ATTRIB_MAX; attr++) {
> const GLuint src = map[attr];
> + const GLubyte size = node->attrsz[src];
>
> - if (node_attrsz[src]) {
> + if (size) {
> struct gl_vertex_array *array = &arrays[attr];
> + const GLenum16 type = node->attrtype[src];
>
> /* override the default array set above */
That comment is no longer quite correct.
> save->inputs[attr] = array;
>
> - array->Ptr = (const GLubyte *) NULL + buffer_offset;
> - array->Size = node_attrsz[src];
> + array->Ptr = (const GLubyte *) NULL + node->offsets[src];
> + array->Size = size;
> array->StrideB = node->vertex_size * sizeof(GLfloat);
> - array->Type = node_attrtype[src];
> - array->Integer = vbo_attrtype_to_integer_flag(node_attrtype[src]);
> + array->Type = type;
> + array->Integer = vbo_attrtype_to_integer_flag(type);
> array->Format = GL_RGBA;
> - array->_ElementSize = array->Size * sizeof(GLfloat);
> + array->_ElementSize = size * sizeof(GLfloat);
> _mesa_reference_buffer_object(ctx,
> &array->BufferObj,
> node->vertex_store->bufferobj);
>
> assert(array->BufferObj->Name);
>
> - buffer_offset += node_attrsz[src] * sizeof(GLfloat);
> varying_inputs |= VERT_BIT(attr);
> }
> }
>
> + if (generic_from_pos) {
> + varying_inputs |= (varying_inputs & VERT_BIT_POS) << VERT_ATTRIB_GENERIC0;
> + save->inputs[VERT_ATTRIB_GENERIC0] = save->inputs[VERT_ATTRIB_POS];
> + }
> +
> _mesa_set_varying_vp_inputs(ctx, varying_inputs);
> ctx->NewDriverState |= ctx->DriverFlags.NewArray;
> }
>
More information about the mesa-dev
mailing list