[3/3] radv: Handle GFX9 merged shaders in radv_flush_constants()

Submitted by Alex Smith on May 31, 2018, 3:44 p.m.

Details

Message ID 20180531154420.1801-3-asmith@feralinteractive.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Alex Smith May 31, 2018, 3:44 p.m.
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@feralinteractive.com>
Cc: "18.1" <mesa-stable@lists.freedesktop.org>
---
 src/amd/vulkan/radv_cmd_buffer.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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;
 	}
 
 	cmd_buffer->push_constant_stages &= ~stages;

Comments

On Thu, May 31, 2018 at 5:44 PM, Alex Smith <asmith@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@feralinteractive.com>
> Cc: "18.1" <mesa-stable@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@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
On 31 May 2018 at 21:15, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote:

> On Thu, May 31, 2018 at 5:44 PM, Alex Smith <asmith@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@feralinteractive.com>
> > Cc: "18.1" <mesa-stable@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.
>

Good point. Fixed, and pushed.


>
> With that, this series is
>
> Reviewed-by: Bas Nieuwenhuizen <bas@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
>