<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, May 27, 2016 at 1:01 PM, Jordan Justen <<a href="mailto:jordan.l.justen@intel.com" target="_blank">jordan.l.justen@intel.com</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 2016-05-20 12:41:04, Jason Ekstrand wrote: > Instead of blasting it out as part of the pipeline, we put it in the > command buffer and only blast it out when it's really needed. Since the > PUSH_CONSTANT_ALLOC commands aren't pipelined, they immediately cause a > stall which we would like to avoid. > --- > src/intel/vulkan/anv_cmd_buffer.c | 1 + > src/intel/vulkan/anv_pipeline.c | 22 ---------- > src/intel/vulkan/anv_private.h | 2 +- > src/intel/vulkan/genX_cmd_buffer.c | 78 +++++++++++++++++++++++++++++++---- > src/intel/vulkan/genX_pipeline_util.h | 12 ------ > 5 files changed, 71 insertions(+), 44 deletions(-) > > diff --git a/src/intel/vulkan/anv_cmd_buffer.c b/src/intel/vulkan/anv_cmd_buffer.c > index bba24e8..db7793e 100644 > --- a/src/intel/vulkan/anv_cmd_buffer.c > +++ b/src/intel/vulkan/anv_cmd_buffer.c > @@ -130,6 +130,7 @@ anv_cmd_state_reset(struct anv_cmd_buffer *cmd_buffer) > state->descriptors_dirty = 0; > state->push_constants_dirty = 0; > state->pipeline = NULL; > + state->push_constant_stages = 0; > state->restart_index = UINT32_MAX; > state->dynamic = default_dynamic_state; > state->need_query_wa = true; > diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c > index faa0adb..c010f0f 100644 > --- a/src/intel/vulkan/anv_pipeline.c > +++ b/src/intel/vulkan/anv_pipeline.c > @@ -939,28 +939,6 @@ anv_compute_urb_partition(struct anv_pipeline *pipeline) > pipeline->urb.start[MESA_SHADER_TESS_EVAL] = push_constant_chunks; > pipeline->urb.size[MESA_SHADER_TESS_EVAL] = 1; > pipeline->urb.entries[MESA_SHADER_TESS_EVAL] = 0; > - > - const unsigned stages = > - _mesa_bitcount(pipeline->active_stages & VK_SHADER_STAGE_ALL_GRAPHICS); > - unsigned size_per_stage = stages ? (push_constant_kb / stages) : 0; > - unsigned used_kb = 0; > - > - /* Broadwell+ and Haswell gt3 require that the push constant sizes be in > - * units of 2KB. Incidentally, these are the same platforms that have > - * 32KB worth of push constant space. > - */ > - if (push_constant_kb == 32) > - size_per_stage &= ~1u; > - > - for (int i = MESA_SHADER_VERTEX; i < MESA_SHADER_FRAGMENT; i++) { > - pipeline->urb.push_size[i] = > - (pipeline->active_stages & (1 << i)) ? size_per_stage : 0; > - used_kb += pipeline->urb.push_size[i]; > - assert(used_kb <= push_constant_kb); > - } > - > - pipeline->urb.push_size[MESA_SHADER_FRAGMENT] = > - push_constant_kb - used_kb; > } > > static void > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h > index 33cbff9..80e768d 100644 > --- a/src/intel/vulkan/anv_private.h > +++ b/src/intel/vulkan/anv_private.h > @@ -1177,6 +1177,7 @@ struct anv_cmd_state { > uint32_t restart_index; > struct anv_vertex_binding vertex_bindings[MAX_VBS]; > struct anv_descriptor_set * descriptors[MAX_SETS]; > + VkShaderStageFlags push_constant_stages; > struct anv_push_constants * push_constants[MESA_SHADER_STAGES]; > struct anv_state binding_tables[MESA_SHADER_STAGES]; > struct anv_state samplers[MESA_SHADER_STAGES]; > @@ -1411,7 +1412,6 @@ struct anv_pipeline { > uint32_t scratch_start[MESA_SHADER_STAGES]; > uint32_t total_scratch; > struct { > - uint8_t push_size[MESA_SHADER_FRAGMENT + 1]; > uint32_t start[MESA_SHADER_GEOMETRY + 1]; > uint32_t size[MESA_SHADER_GEOMETRY + 1]; > uint32_t entries[MESA_SHADER_GEOMETRY + 1]; > diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c > index ee47c29..f5e3530 100644 > --- a/src/intel/vulkan/genX_cmd_buffer.c > +++ b/src/intel/vulkan/genX_cmd_buffer.c > @@ -274,6 +274,72 @@ void genX(CmdPipelineBarrier)( > } > } > > +static void > +cmd_buffer_alloc_push_constants(struct anv_cmd_buffer *cmd_buffer) > +{ > + VkShaderStageFlags stages = cmd_buffer->state.pipeline->active_stages; > + > + /* In order to avoid thrash, we assume that vertex and fragment stages > + * always exist. In the rare case where one is missing *and* the other > + * uses push concstants, this may be suboptimal. However, avoiding stalls > + * seems more important. > + */ > + stages |= VK_SHADER_STAGE_FRAGMENT_BIT | VK_SHADER_STAGE_VERTEX_BIT; > + > + if (stages == cmd_buffer->state.push_constant_stages) > + return; > + > +#if GEN_GEN >= 8 > + const unsigned push_constant_kb = 32; > +#elif GEN_IS_HASWELL > + const unsigned push_constant_kb = cmd_buffer->device-><a href="http://info.gt" rel="noreferrer" target="_blank">info.gt</a> == 3 ? 32 : 16; > +#else > + const unsigned push_constant_kb = 16; > +#endif </div></div>I noted that this changed from the old code that just used 32k. I see that this matches src/mesa/drivers/dri/i965/gen7_urb.c. Maybe worth noting in the commit message? </blockquote><div> </div><div>I think we just use 32KB at one point, but I fixed that a while ago. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Reviewed-by: Jordan Justen <<a href="mailto:jordan.l.justen@intel.com">jordan.l.justen@intel.com</a>> </blockquote><div> </div><div>Thanks </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + > + const unsigned num_stages = > + _mesa_bitcount(stages & VK_SHADER_STAGE_ALL_GRAPHICS); > + unsigned size_per_stage = push_constant_kb / num_stages; > + > + /* Broadwell+ and Haswell gt3 require that the push constant sizes be in > + * units of 2KB. Incidentally, these are the same platforms that have > + * 32KB worth of push constant space. > + */ > + if (push_constant_kb == 32) > + size_per_stage &= ~1u; > + > + uint32_t kb_used = 0; > + for (int i = MESA_SHADER_VERTEX; i < MESA_SHADER_FRAGMENT; i++) { > + unsigned push_size = (stages & (1 << i)) ? size_per_stage : 0; > + anv_batch_emit(&cmd_buffer->batch, > + GENX(3DSTATE_PUSH_CONSTANT_ALLOC_VS), alloc) { > + alloc._3DCommandSubOpcode = 18 + i; > + alloc.ConstantBufferOffset = (push_size > 0) ? kb_used : 0; > + alloc.ConstantBufferSize = push_size; > + } > + kb_used += push_size; > + } > + > + anv_batch_emit(&cmd_buffer->batch, > + GENX(3DSTATE_PUSH_CONSTANT_ALLOC_PS), alloc) { > + alloc.ConstantBufferOffset = kb_used; > + alloc.ConstantBufferSize = push_constant_kb - kb_used; > + } > + > + cmd_buffer->state.push_constant_stages = stages; > + > + /* From the BDW PRM for 3DSTATE_PUSH_CONSTANT_ALLOC_VS: > + * > + * "The 3DSTATE_CONSTANT_VS must be reprogrammed prior to > + * the next 3DPRIMITIVE command after programming the > + * 3DSTATE_PUSH_CONSTANT_ALLOC_VS" > + * > + * Since 3DSTATE_PUSH_CONSTANT_ALLOC_VS is programmed as part of > + * pipeline setup, we need to dirty push constants. > + */ > + cmd_buffer->state.push_constants_dirty |= VK_SHADER_STAGE_ALL_GRAPHICS; > +} > + > static uint32_t > cmd_buffer_flush_push_constants(struct anv_cmd_buffer *cmd_buffer) > { > @@ -384,16 +450,10 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer *cmd_buffer) > > anv_batch_emit_batch(&cmd_buffer->batch, &pipeline->batch); > > - /* From the BDW PRM for 3DSTATE_PUSH_CONSTANT_ALLOC_VS: > - * > - * "The 3DSTATE_CONSTANT_VS must be reprogrammed prior to > - * the next 3DPRIMITIVE command after programming the > - * 3DSTATE_PUSH_CONSTANT_ALLOC_VS" > - * > - * Since 3DSTATE_PUSH_CONSTANT_ALLOC_VS is programmed as part of > - * pipeline setup, we need to dirty push constants. > + /* If the pipeline changed, we may need to re-allocate push constant > + * space in the URB. > */ > - cmd_buffer->state.push_constants_dirty |= VK_SHADER_STAGE_ALL_GRAPHICS; > + cmd_buffer_alloc_push_constants(cmd_buffer); > } > > #if GEN_GEN <= 7 > diff --git a/src/intel/vulkan/genX_pipeline_util.h b/src/intel/vulkan/genX_pipeline_util.h > index ecbe436..2ed55e0 100644 > --- a/src/intel/vulkan/genX_pipeline_util.h > +++ b/src/intel/vulkan/genX_pipeline_util.h > @@ -205,18 +205,6 @@ emit_urb_setup(struct anv_pipeline *pipeline) > } > #endif > > - unsigned push_start = 0; > - for (int i = MESA_SHADER_VERTEX; i <= MESA_SHADER_FRAGMENT; i++) { > - unsigned push_size = pipeline->urb.push_size[i]; > - anv_batch_emit(&pipeline->batch, > - GENX(3DSTATE_PUSH_CONSTANT_ALLOC_VS), alloc) { > - alloc._3DCommandSubOpcode = 18 + i; > - alloc.ConstantBufferOffset = (push_size > 0) ? push_start : 0; > - alloc.ConstantBufferSize = push_size; > - } > - push_start += pipeline->urb.push_size[i]; > - } > - > for (int i = MESA_SHADER_VERTEX; i <= MESA_SHADER_GEOMETRY; i++) { > anv_batch_emit(&pipeline->batch, GENX(3DSTATE_URB_VS), urb) { > urb._3DCommandSubOpcode = 48 + i; > -- > 2.5.0.400.gff86faf > </div></div>> _______________________________________________ > 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>