On Thu, May 31, 2018 at 5:44 PM, Alex Smith <asm...@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 <asm...@feralinteractive.com>
> Cc: "18.1" <mesa-sta...@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 <b...@basnieuwenhuizen.nl>
Thanks a lot!
> }
>
> cmd_buffer->push_constant_stages &= ~stages;
> --
> 2.14.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
