<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 31 May 2018 at 21:15, Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, May 31, 2018 at 5:44 PM, Alex Smith <<a href="mailto:asmith@feralinteractive.com">asmith@feralinteractive.com</a>> 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 <<a href="mailto:asmith@feralinteractive.com">asmith@feralinteractive.com</a>> > Cc: "18.1" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>> > --- > 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; </div></div>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. </blockquote><div> </div><div>Good point. Fixed, and pushed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> With that, this series is Reviewed-by: Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl">bas@basnieuwenhuizen.nl</a>> Thanks a lot! > } > > cmd_buffer->push_constant_stages &= ~stages; > -- > 2.14.3 > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>