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

[Alex Smith]

On 31 May 2018 at 21:15, Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl> wrote:

> 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.
>

Good point. Fixed, and pushed.


>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180601/9a74cce4/attachment.html>