[Mesa-dev] [PATCH 3/3] radv: Handle GFX9 merged shaders in radv_flush_constants()

[Bas Nieuwenhuizen]

On Thu, May 31, 2018 at 5:44 PM, Alex Smith <asmith at feralinteractive.com> wrote:
> This was not previously handled correctly. For example,
> push_constant_stages might only contain MESA_SHADER_VERTEX because
> only that stage was changed by CmdPushConstants or
> CmdBindDescriptorSets.
>
> In that case, if vertex has been merged with tess control, then the
> push constant address wouldn't be updated since
> pipeline->shaders[MESA_SHADER_VERTEX] would be NULL.
>
> Use radv_get_shader() instead of getting the shader directly so that
> we get the right shader if merged. Also, skip emitting the address
> redundantly - if two merged stages are set in push_constant_stages
> this change would have made the address get emitted twice.
>
> Signed-off-by: Alex Smith <asmith at feralinteractive.com>
> Cc: "18.1" <mesa-stable at lists.freedesktop.org>
> ---
>  src/amd/vulkan/radv_cmd_buffer.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c
> index da9591b9a5..c6a2d6c5b9 100644
> --- a/src/amd/vulkan/radv_cmd_buffer.c
> +++ b/src/amd/vulkan/radv_cmd_buffer.c
> @@ -1585,6 +1585,7 @@ radv_flush_constants(struct radv_cmd_buffer *cmd_buffer,
>                                          ? cmd_buffer->state.compute_pipeline
>                                          : cmd_buffer->state.pipeline;
>         struct radv_pipeline_layout *layout = pipeline->layout;
> +       struct radv_shader_variant *shader, *prev_shader;
>         unsigned offset;
>         void *ptr;
>         uint64_t va;
> @@ -1609,11 +1610,17 @@ radv_flush_constants(struct radv_cmd_buffer *cmd_buffer,
>         MAYBE_UNUSED unsigned cdw_max = radeon_check_space(cmd_buffer->device->ws,
>                                                            cmd_buffer->cs, MESA_SHADER_STAGES * 4);
>
> +       prev_shader = NULL;
>         radv_foreach_stage(stage, stages) {
> -               if (pipeline->shaders[stage]) {
> +               shader = radv_get_shader(pipeline, stage);
> +
> +               /* Avoid redundantly emitting the address for merged stages. */
> +               if (shader && shader != prev_shader) {
>                         radv_emit_userdata_address(cmd_buffer, pipeline, stage,
>                                                    AC_UD_PUSH_CONSTANTS, va);
>                 }
> +
> +               prev_shader = shader;

This emits the same shader twice if we have a geometry shader and a
vertex shader but no tessellation shaders in cases were the stage mask
is larger than needed and includes the tessellation stages. On the
iteration for the tess shaders, prev_shader will be reset to NULL, and
hence when we visit the geometry shader we will emit the constants
again.

I think this should be solved by moving the prev_shader update within
the if statement.

With that, this series is

Reviewed-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>

Thanks a lot!


>         }
>
>         cmd_buffer->push_constant_stages &= ~stages;
> --
> 2.14.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev