On 31 May 2018 at 21:15, Bas Nieuwenhuizen <b...@basnieuwenhuizen.nl> wrote:
> 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.
>
Good point. Fixed, and pushed.
>
> 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
