[Mesa-dev] [PATCH 1/8] vbo: Correctly handle attribute offsets in dlist draw.
Brian Paul
brianp at vmware.com
Wed Jan 31 16:56:58 UTC 2018
On 01/31/2018 09:54 AM, Brian Paul wrote:
> 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.
Oh, and it might be good to write a new piglit test or two to exercise
the POS/GENERIC0 aliasing stuff. I _think_ we might have one test along
those lines now (and I _may_ have even written it), but I can't look
right now.
>
> -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