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

[Joe Kniss]

Reviewed-by: Joe Kniss <djmk at chromiumos.org, djmk at google.com>

On Tue, May 8, 2018 at 3:19 PM, Joe Kniss <djmk at google.com> wrote:
> 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