[Mesa-dev] [PATCH 11/11] vbo: overhaul display list vertex array setup/binding code
Brian Paul
brianp at vmware.com
Mon Jan 29 18:40:37 UTC 2018
On 01/27/2018 08:01 AM, Mathias Fröhlich wrote:
> Hi Brian,
>
> That one will not work as is.
>
> Display lists are shared objects across contexts.
> Means past that change the
>
> const struct gl_vertex_array *inputs[VBO_ATTRIB_MAX];
>
> may be concurrently written to from different threads filling in current value
> pointers from the context where the list is executed from. That leaves current
> values form a differnet context and concurrency issues at least.
Ah right. At one point I thought about dlist sharing, but I forgot to
address that in the end.
I think one way to address that would be to put a mutex in the
vbo_save_vertex_list so it can only be accessed by one context at a
time. Plus, check if the calling context matches to compile-time
context - if they don't match, update the 'current values'.
I'll think about it more though.
> Beside that I am also not sure if the array update logic in bind_vertex_list
> works correctly when VBO_ATTRIB* arrays ending in an aliasing VERT_ATTRIB*
> slot are both enabled. You probably need to currectly mask out the VBO_ATTRIB
> arrays before you distribute them across the inputs[] array. But the the logic
> just to store subsequent node->arrays entries to save memory fails when
> skipping the masked out arrays.
Can you provide a concrete example of what you mean?
Thanks for reviewing!
-Brian
>
> best
>
> Mathias
>
> On Thursday, 25 January 2018 00:20:35 CET Brian Paul wrote:
>> Do more of the vertex array setup at display list compilation time
>> via the compile_vertex_list() function.
>>
>> Then, at draw time, just do final vertex array setup dependent on
>> whether we're using fixed function or a vertex shader.
>>
>> This can help apps which use a lot of short display list drawing.
>> ---
>> src/mesa/vbo/vbo_save.h | 11 ++-
>> src/mesa/vbo/vbo_save_api.c | 93 ++++++++++++++++++++++---
>> src/mesa/vbo/vbo_save_draw.c | 157 +++++++++++++
> +-----------------------------
>> 3 files changed, 147 insertions(+), 114 deletions(-)
>>
>> diff --git a/src/mesa/vbo/vbo_save.h b/src/mesa/vbo/vbo_save.h
>> index 51ea9cc..62a9d54 100644
>> --- a/src/mesa/vbo/vbo_save.h
>> +++ b/src/mesa/vbo/vbo_save.h
>> @@ -85,6 +85,16 @@ struct vbo_save_vertex_list {
>>
>> struct vbo_save_vertex_store *vertex_store;
>> struct vbo_save_primitive_store *prim_store;
>> +
>> + /* Array [bitcount(enabled)] of gl_vertex_array objects describing
>> + * the vertex data contained in this vbo_save_vertex_list.
>> + */
>> + struct gl_vertex_array *arrays;
>> +
>> + /* The arrays to actually use at draw time. Some will point at the
>> + * 'arrays' field above. The rest will point at the vbo->currval arrays
>> + */
>> + const struct gl_vertex_array *inputs[VBO_ATTRIB_MAX];
>> };
>>
>>
>> @@ -140,7 +150,6 @@ struct vbo_save_context {
>> GLvertexformat vtxfmt;
>> GLvertexformat vtxfmt_noop; /**< Used if out_of_memory is true */
>> struct gl_vertex_array arrays[VBO_ATTRIB_MAX];
>> - const struct gl_vertex_array *inputs[VBO_ATTRIB_MAX];
>>
>> GLbitfield64 enabled; /**< mask of enabled vbo arrays. */
>> GLubyte attrsz[VBO_ATTRIB_MAX]; /**< 1, 2, 3 or 4 */
>> diff --git a/src/mesa/vbo/vbo_save_api.c b/src/mesa/vbo/vbo_save_api.c
>> index 11c40a2..e394d88 100644
>> --- a/src/mesa/vbo/vbo_save_api.c
>> +++ b/src/mesa/vbo/vbo_save_api.c
>> @@ -412,8 +412,80 @@ convert_line_loop_to_strip(struct vbo_save_context
> *save,
>>
>>
>> /**
>> - * Insert the active immediate struct onto the display list currently
>> - * being built.
>> + * Prepare the vertex arrays which will be used for drawing the primitives
>> + * in the given vertex list. By doing it here, we can avoid doing this
>> + * work at display list execution/draw time.
>> + */
>> +static void
>> +setup_vertex_arrays(struct gl_context *ctx,
>> + struct vbo_save_vertex_list *node)
>> +{
>> + struct vbo_context *vbo = vbo_context(ctx);
>> + unsigned buffer_offset = node->buffer_offset;
>> + unsigned attr;
>> + const unsigned num_arrays = _mesa_bitcount_64(node->enabled);
>> + unsigned array_count;
>> +
>> + 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;
>> + }
>> +
>> + assert(!node->arrays);
>> + node->arrays = calloc(num_arrays, sizeof(struct gl_vertex_array));
>> + if (!node->arrays) {
>> + _mesa_error(ctx, GL_OUT_OF_MEMORY,
>> + "compiling vertex data into display list");
>> + /* just turn off all arrays */
>> + node->enabled = 0;
>> + return;
>> + }
>> +
>> + array_count = 0;
>> + for (attr = 0; attr < VBO_ATTRIB_MAX; attr++) {
>> + if (node->attrsz[attr]) {
>> + /* this vertex array points at actual per-vertex data in a VB */
>> + struct gl_vertex_array *array = &node->arrays[array_count];
>> +
>> + array->Ptr = (const GLubyte *) NULL + buffer_offset;
>> + array->Size = node->attrsz[attr];
>> + array->StrideB = node->vertex_size * sizeof(GLfloat);
>> + array->Type = node->attrtype[attr];
>> + array->Integer = vbo_attrtype_to_integer_flag(node-
>> attrtype[attr]);
>> + array->Format = GL_RGBA;
>> + array->_ElementSize = array->Size * sizeof(GLfloat);
>> + _mesa_reference_buffer_object(ctx,
>> + &array->BufferObj,
>> + node->vertex_store->bufferobj);
>> +
>> + node->inputs[attr] = array;
>> +
>> + buffer_offset += node->attrsz[attr] * sizeof(GLfloat);
>> +
>> + array_count++;
>> + }
>> + else {
>> + /* this vertex array points at currval/default values */
>> + node->inputs[attr] = &vbo->currval[attr];
>> + }
>> + }
>> +
>> + assert(array_count == num_arrays);
>> +}
>> +
>> +
>> +/**
>> + * Create a new vbo_save_vertex_list object and fill it with the vertices
>> + * and primitives accumulated in the vbo_save_context.
>> + * The new vbo_save_vertex_list is allocated from display list memory
>> + * and will automatically be inserted into the current display list.
>> */
>> static void
>> compile_vertex_list(struct gl_context *ctx)
>> @@ -430,6 +502,8 @@ compile_vertex_list(struct gl_context *ctx)
>> if (!node)
>> return;
>>
>> + memset(node, 0, sizeof(*node));
>> +
>> /* Make sure the pointer is aligned to the size of a pointer */
>> assert((GLintptr) node % sizeof(void *) == 0);
>>
>> @@ -568,6 +642,8 @@ compile_vertex_list(struct gl_context *ctx)
>> node->start_vertex = 0;
>> }
>>
>> + setup_vertex_arrays(ctx, node);
>> +
>> /* Reset our structures for the next run of vertices:
>> */
>> reset_counters(ctx);
>> @@ -1679,7 +1755,6 @@ static void
>> vbo_destroy_vertex_list(struct gl_context *ctx, void *data)
>> {
>> struct vbo_save_vertex_list *node = (struct vbo_save_vertex_list *)
> data;
>> - (void) ctx;
>>
>> if (--node->vertex_store->refcount == 0)
>> free_vertex_store(ctx, node->vertex_store);
>> @@ -1688,6 +1763,13 @@ vbo_destroy_vertex_list(struct gl_context *ctx, void
> *data)
>> free(node->prim_store);
>>
>> free(node->current_data);
>> +
>> + const unsigned num_arrays = _mesa_bitcount_64(node->enabled);
>> + for (unsigned i = 0; i < num_arrays; i++) {
>> + _mesa_reference_buffer_object(ctx, &(node->arrays[i].BufferObj),
> NULL);
>> + }
>> + free(node->arrays);
>> +
>> node->current_data = NULL;
>> }
>>
>> @@ -1752,7 +1834,6 @@ void
>> vbo_save_api_init(struct vbo_save_context *save)
>> {
>> struct gl_context *ctx = save->ctx;
>> - GLuint i;
>>
>> save->opcode_vertex_list =
>> _mesa_dlist_alloc_opcode(ctx,
>> @@ -1764,8 +1845,4 @@ vbo_save_api_init(struct vbo_save_context *save)
>> vtxfmt_init(ctx);
>> current_init(ctx);
>> _mesa_noop_vtxfmt_init(&save->vtxfmt_noop);
>> -
>> - /* These will actually get set again when binding/drawing */
>> - for (i = 0; i < VBO_ATTRIB_MAX; i++)
>> - save->inputs[i] = &save->arrays[i];
>> }
>> diff --git a/src/mesa/vbo/vbo_save_draw.c b/src/mesa/vbo/vbo_save_draw.c
>> index 5f5dcbe..1ef74d7 100644
>> --- a/src/mesa/vbo/vbo_save_draw.c
>> +++ b/src/mesa/vbo/vbo_save_draw.c
>> @@ -125,69 +125,63 @@ playback_copy_to_current(struct gl_context *ctx,
>> }
>>
>>
>> -
>> /**
>> - * Treat the vertex storage as a VBO, define vertex arrays pointing
>> - * into it:
>> + * Setup the vertex arrays for drawing the primitives described by 'node'.
>> + * This populates the node->inputs[] vertex arrays and calls
>> + * _mesa_set_drawing_arrays(). Most of the work was previously done in
>> + * the setup_vertex_arrays() function when the display list was compiled.
>> */
>> static void
>> bind_vertex_list(struct gl_context *ctx,
>> - const struct vbo_save_vertex_list *node)
>> + struct vbo_save_vertex_list *node)
>> {
>> 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; /** map from VERT_ATTRIB_x to VBO_ATTRIB_x */
>> - GLuint attr;
>> - GLubyte node_attrsz[VBO_ATTRIB_MAX]; /* copy of node->attrsz[] */
>> - GLenum node_attrtype[VBO_ATTRIB_MAX]; /* copy of node->attrtype[] */
>> + const enum vp_mode vp_mode = get_vp_mode(ctx);
>> + unsigned array_count = 0;
>> GLbitfield64 enabled = node->enabled;
>>
>> - memcpy(node_attrsz, node->attrsz, sizeof(node->attrsz));
>> - 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;
>> + if (vp_mode == VP_SHADER) {
>> + /* per-vertex materials are to be ignored when using a VS */
>> + enabled &= ~VBO_ATTRIBS_MATERIALS;
>> }
>>
>> - /* Install the default (ie Current) attributes first */
>> - for (attr = 0; attr < VERT_ATTRIB_FF_MAX; attr++) {
>> - save->inputs[attr] = &vbo->currval[VBO_ATTRIB_POS + attr];
>> - }
>> + /* Loop over the attributes which are present in the vertex buffer
>> + * and set the node->input vertex arrays to point to those attributes.
>> + */
>> + GLbitfield64 enabled_scan = enabled;
>> + while (enabled_scan) {
>> + const int attr = u_bit_scan64(&enabled_scan);
>> + unsigned dst;
>>
>> - /* Overlay other active attributes */
>> - switch (get_vp_mode(ctx)) {
>> - case VP_FF:
>> - /* Point the generic attributes at the legacy material values */
>> - for (attr = 0; attr < MAT_ATTRIB_MAX; attr++) {
>> - save->inputs[VERT_ATTRIB_GENERIC(attr)] =
>> - &vbo->currval[VBO_ATTRIB_MAT_FRONT_AMBIENT+attr];
>> + /* Here is where we resolve whether array[16...] points to legacy
>> + * material coefficients or generic vertex attributes.
>> + */
>> + if (vp_mode == VP_FF) {
>> + if (attr >= VBO_ATTRIB_FIRST_MATERIAL) {
>> + dst = VBO_ATTRIB_GENERIC0 + attr - VBO_ATTRIB_FIRST_MATERIAL;
>> + } else {
>> + dst = attr;
>> + }
>> + } else {
>> + assert(vp_mode == VP_SHADER);
>> + dst = attr;
>> }
>> - map = vbo->map_vp_none;
>>
>> - /* shift material attrib flags into generic attribute positions */
>> + assert(dst < VERT_ATTRIB_MAX);
>> + assert(node->arrays);
>> + node->inputs[dst] = &node->arrays[array_count++];
>> + }
>> +
>> + /* Final vertex array setup depending on whether we're using fixed
>> + * function or a vertex shader.
>> + */
>> + if (vp_mode == VP_FF) {
>> + /* Update the enabled attribute bitfield so the material attrib bits
>> + * replace the generic attrib bits.
>> + */
>> enabled = (enabled & VBO_ATTRIBS_LEGACY)
>> | ((enabled & VBO_ATTRIBS_MATERIALS) >> VBO_MATERIAL_SHIFT);
>> - break;
>> - case VP_SHADER:
>> - for (attr = 0; attr < VERT_ATTRIB_GENERIC_MAX; attr++) {
>> - save->inputs[VERT_ATTRIB_GENERIC(attr)] =
>> - &vbo->currval[VBO_ATTRIB_GENERIC0+attr];
>> - }
>> - map = vbo->map_vp_arb;
>> -
>> - /* per-vertex materials are to be ignored when using a VS */
>> - enabled &= ~VBO_ATTRIBS_MATERIALS;
>> -
>> + } else {
>> /* check if VERT_ATTRIB_POS is not read but VERT_BIT_GENERIC0 is
> read.
>> * In that case we effectively need to route the data from
>> * glVertexAttrib(0, val) calls to feed into the GENERIC0 input.
>> @@ -196,64 +190,19 @@ 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;
>> - enabled = (enabled & ~VERT_BIT_POS) | VERT_BIT_GENERIC0;
>> - }
>> - break;
>> - default:
>> - unreachable("Bad vertex program mode");
>> - }
>> -
>> -#ifdef DEBUG
>> - /* verify the enabled bitmask is consistent with attribute size info */
>> - {
>> - GLbitfield64 used = 0x0;
>> - for (attr = 0; attr < VERT_ATTRIB_MAX; attr++) {
>> - const GLuint src = map[attr];
>> - if (node_attrsz[src]) {
>> - assert(enabled & VERT_BIT(attr));
>> - used |= VERT_BIT(attr);
>> - } else {
>> - assert((enabled & VERT_BIT(attr)) == 0);
>> - }
>> + node->inputs[VERT_ATTRIB_GENERIC0] = &node-
>> arrays[VBO_ATTRIB_POS];
>> + node->inputs[VERT_ATTRIB_POS] = &vbo->currval[VBO_ATTRIB_POS];
>> + enabled &= ~BITFIELD64_BIT(VBO_ATTRIB_POS);
>> + enabled |= BITFIELD64_BIT(VBO_ATTRIB_GENERIC0);
>> }
>> - assert(used == enabled);
>> }
>> -#endif
>>
>> - GLbitfield64 enabled_scan = enabled;
>> - while (enabled_scan) {
>> - const int attr = u_bit_scan64(&enabled_scan);
>> - const GLuint src = map[attr];
>> -
>> - if (node_attrsz[src]) {
>> - struct gl_vertex_array *array = &arrays[attr];
>> -
>> - /* override the default array set above */
>> - save->inputs[attr] = array;
>> -
>> - array->Ptr = (const GLubyte *) NULL + buffer_offset;
>> - array->Size = node_attrsz[src];
>> - array->StrideB = node->vertex_size * sizeof(GLfloat);
>> - array->Type = node_attrtype[src];
>> - array->Integer = vbo_attrtype_to_integer_flag(node_attrtype[src]);
>> - array->Format = GL_RGBA;
>> - array->_ElementSize = array->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);
>> - }
>> - }
>> + /* make sure none of the extra VBO_ATTRIB bits are set */
>> + assert((enabled & (GLbitfield64) VERT_BIT_ALL) == enabled);
>>
>> _mesa_set_varying_vp_inputs(ctx, enabled);
>> - ctx->NewDriverState |= ctx->DriverFlags.NewArray;
>> +
>> + _mesa_set_drawing_arrays(ctx, node->inputs);
>> }
>>
>>
>> @@ -292,8 +241,8 @@ loopback_vertex_list(struct gl_context *ctx,
>> void
>> vbo_save_playback_vertex_list(struct gl_context *ctx, void *data)
>> {
>> - const struct vbo_save_vertex_list *node =
>> - (const struct vbo_save_vertex_list *) data;
>> + struct vbo_save_vertex_list *node =
>> + (struct vbo_save_vertex_list *) data;
>> struct vbo_context *vbo = vbo_context(ctx);
>> struct vbo_save_context *save = &vbo->save;
>> GLboolean remap_vertex_store = GL_FALSE;
>> @@ -346,8 +295,6 @@ vbo_save_playback_vertex_list(struct gl_context *ctx,
> void *data)
>>
>> bind_vertex_list(ctx, node);
>>
>> - _mesa_set_drawing_arrays(ctx, vbo->save.inputs);
>> -
>> /* Again...
>> */
>> if (ctx->NewState)
>>
>
>
>
>
More information about the mesa-dev
mailing list