[virglrenderer-devel] [PATCH v2] vrend: Associate host UBOs with the correct gallium uniform buffers

[Joe Kniss]

Looks good to me.



On Fri, May 4, 2018 at 4:00 AM, Elie Tournier <tournier.elie at gmail.com> wrote:
> On Thu, May 03, 2018 at 06:33:48PM +0300, Alexandros Frantzis wrote:
>> Take into account the gallium uniform buffer indices when associating
>> host UBOs with gallium uniform buffers.
>>
>> Previously the code disregarded the gallium uniform buffer indices,
>> leading, under specific circumstances, to the provision of incorrect
>> data to the shaders. The problem manifested typically when a context
>> contained active UBOs which were not accessed by a particular shader,
>> but ended up being used instead of the correct UBOs for that shader.
>> This occurred, for example, when running the dEQP-GLES3.functional.ubo.*
>> in batch mode, in which case left over UBOs from previous tests would
>> cause subsequent tests to fail.
>>
>> Fixes:
>> dEQP-GLES3.functional.ubo.* when run in batch mode
>>
> Tested-by: Elie Tournier <elie.tournier at collabora.com>
>> Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
>> ---
>> Changes in v2:
>>  - Fill in only the used part of the ubo_idx array in vrend_shader_info
>>
>>  src/vrend_renderer.c | 22 +++++++++++++++++++---
>>  src/vrend_shader.c   |  1 +
>>  src/vrend_shader.h   |  1 +
>>  3 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
>> index 8e55f63..ac9723c 100644
>> --- a/src/vrend_renderer.c
>> +++ b/src/vrend_renderer.c
>> @@ -1046,7 +1046,8 @@ static struct vrend_linked_shader_program *add_shader_program(struct vrend_conte
>>
>>           sprog->ubo_locs[id] = calloc(sprog->ss[id]->sel->sinfo.num_ubos, sizeof(uint32_t));
>>           for (i = 0; i < sprog->ss[id]->sel->sinfo.num_ubos; i++) {
>> -            snprintf(name, 32, "%subo%d", prefix, i + 1);
>> +            int ubo_idx = sprog->ss[id]->sel->sinfo.ubo_idx[i];
>> +            snprintf(name, 32, "%subo%d", prefix, ubo_idx);
>>              sprog->ubo_locs[id][i] = glGetUniformBlockIndex(prog_id, name);
>>           }
>>        } else
>> @@ -2858,25 +2859,40 @@ static void vrend_draw_bind_ubo(struct vrend_context *ctx)
>>     ubo_id = 0;
>>     for (shader_type = PIPE_SHADER_VERTEX; shader_type <= ctx->sub->last_shader_idx; shader_type++) {
>>        uint32_t mask;
>> -      int shader_ubo_idx = 0;
>> +      int shader_ubo_idx;
>>        struct pipe_constant_buffer *cb;
>>        struct vrend_resource *res;
>> +      struct vrend_shader_info* sinfo;
>> +
>>        if (!ctx->sub->const_bufs_used_mask[shader_type])
>>           continue;
>>
>>        if (!ctx->sub->prog->ubo_locs[shader_type])
>>           continue;
>>
>> +      sinfo = &ctx->sub->prog->ss[shader_type]->sel->sinfo;
>> +
>>        mask = ctx->sub->const_bufs_used_mask[shader_type];
>>        while (mask) {
>> +         /* The const_bufs_used_mask stores the gallium uniform buffer indices */
>>           i = u_bit_scan(&mask);
>>
>> +         /* The cbs array is indexed using the gallium uniform buffer index */
>>           cb = &ctx->sub->cbs[shader_type][i];
>>           res = (struct vrend_resource *)cb->buffer;
>> +
>> +         /* Find the index of the uniform buffer in the array of shader ubo data */
>> +         for (shader_ubo_idx = 0; shader_ubo_idx < sinfo->num_ubos; shader_ubo_idx++) {
>> +            if (sinfo->ubo_idx[shader_ubo_idx] == i)
>> +               break;
>> +         }
>> +         if (shader_ubo_idx == sinfo->num_ubos)
>> +             continue;
>> +
>>           glBindBufferRange(GL_UNIFORM_BUFFER, ubo_id, res->id,
>>                             cb->buffer_offset, cb->buffer_size);
>> +         /* The ubo_locs array is indexed using the shader ubo index */
>>           glUniformBlockBinding(ctx->sub->prog->id, ctx->sub->prog->ubo_locs[shader_type][shader_ubo_idx], ubo_id);
>> -         shader_ubo_idx++;
>>           ubo_id++;
>>        }
>>     }
>> diff --git a/src/vrend_shader.c b/src/vrend_shader.c
>> index bab603e..ba2c9c0 100644
>> --- a/src/vrend_shader.c
>> +++ b/src/vrend_shader.c
>> @@ -2830,6 +2830,7 @@ char *vrend_convert_shader(struct vrend_shader_cfg *cfg,
>>     sinfo->samplers_used_mask = ctx.samplers_used;
>>     sinfo->num_consts = ctx.num_consts;
>>     sinfo->num_ubos = ctx.num_ubo;
>> +   memcpy(sinfo->ubo_idx, ctx.ubo_idx, ctx.num_ubo * sizeof(*ctx.ubo_idx));
>>     sinfo->num_inputs = ctx.num_inputs;
>>     sinfo->num_interps = ctx.num_interps;
>>     sinfo->num_outputs = ctx.num_outputs;
>> diff --git a/src/vrend_shader.h b/src/vrend_shader.h
>> index e54a85c..681bfe2 100644
>> --- a/src/vrend_shader.h
>> +++ b/src/vrend_shader.h
>> @@ -41,6 +41,7 @@ struct vrend_shader_info {
>>     int num_interps;
>>     int num_outputs;
>>     int num_ubos;
>> +   int ubo_idx[32];
>>     int num_ucp;
>>     int glsl_ver;
>>     bool has_pervertex_out;
>> --
>> 2.17.0
>>
>> _______________________________________________
>> virglrenderer-devel mailing list
>> virglrenderer-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel