<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">I had a few nits below. With those fixed,</div><div class="gmail_quote"> </div><div class="gmail_quote">Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div class="gmail_quote"> </div><div class="gmail_quote"> </div><div class="gmail_quote">On Thu, Jan 25, 2018 at 4:24 AM, Iago Toral Quiroga <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The Vulkan spec states that VkPipelineLayout objects must not be destroyed while any command buffer that uses them is in the recording state, but it permits them to be destroyed otherwise. This means that applications are allowed to free pipeline layouts after command recording is finished even if there are pipeline objects that still exist and were created with these layouts. There are two solutions to this, one is to use reference counting on pipeline layout objects. The other is to avoid holding references to pipeline layouts where they are not really needed. This patch takes a step towards the second option by making the pipeline shader compile code take pipeline layout from the VkGraphicsPipelineCreateInfo provided rather than the pipeline object. A follow-up patch will remove any remaining uses of the layout field so we can remove it from the pipeline object and avoid the need for reference counting. Suggested-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> --- src/intel/vulkan/anv_nir.h | 3 +- src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 2 +- src/intel/vulkan/anv_nir_lower_ycbcr_textures.c | 9 ++-- src/intel/vulkan/anv_pipeline.c | 54 ++++++++++++++++-------- 4 files changed, 44 insertions(+), 24 deletions(-) diff --git a/src/intel/vulkan/anv_nir.h b/src/intel/vulkan/anv_nir.h index 8ac0a119dac..ce95b40b014 100644 --- a/src/intel/vulkan/anv_nir.h +++ b/src/intel/vulkan/anv_nir.h @@ -38,9 +38,10 @@ void anv_nir_lower_push_constants(nir_shader *shader); bool anv_nir_lower_multiview(nir_shader *shader, uint32_t view_mask); bool anv_nir_lower_ycbcr_textures(nir_shader *shader, - struct anv_pipeline *pipeline); + struct anv_pipeline_layout *layout); void anv_nir_apply_pipeline_layout(struct anv_pipeline *pipeline, + struct anv_pipeline_layout *layout, nir_shader *shader, struct brw_stage_prog_data *prog_data, struct anv_pipeline_bind_map *map); diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c index 6775f9b464e..acabc5426be 100644 --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c @@ -326,11 +326,11 @@ setup_vec4_uniform_value(uint32_t *params, uint32_t offset, unsigned n) void anv_nir_apply_pipeline_layout(struct anv_pipeline *pipeline, + struct anv_pipeline_layout *layout, nir_shader *shader, struct brw_stage_prog_data *prog_data, struct anv_pipeline_bind_map *map) { - struct anv_pipeline_layout *layout = pipeline->layout; gl_shader_stage stage = shader->info.stage; struct apply_pipeline_layout_state state = { diff --git a/src/intel/vulkan/anv_nir_lower_ycbcr_textures.c b/src/intel/vulkan/anv_nir_lower_ycbcr_textures.c index 028f24e2f60..ad793ee0a0c 100644 --- a/src/intel/vulkan/anv_nir_lower_ycbcr_textures.c +++ b/src/intel/vulkan/anv_nir_lower_ycbcr_textures.c @@ -316,13 +316,13 @@ swizzle_channel(struct isl_swizzle swizzle, unsigned channel) } static bool -try_lower_tex_ycbcr(struct anv_pipeline *pipeline, +try_lower_tex_ycbcr(struct anv_pipeline_layout *layout, nir_builder *builder, nir_tex_instr *tex) { nir_variable *var = tex->texture->var; const struct anv_descriptor_set_layout *set_layout = - pipeline->layout->set[var->data.descriptor_set].layout; + layout->set[var->data.descriptor_set].layout; const struct anv_descriptor_set_binding_layout *binding = &set_layout->binding[var->data.binding]; @@ -440,7 +440,8 @@ try_lower_tex_ycbcr(struct anv_pipeline *pipeline, } bool -anv_nir_lower_ycbcr_textures(nir_shader *shader, struct anv_pipeline *pipeline) +anv_nir_lower_ycbcr_textures(nir_shader *shader, + struct anv_pipeline_layout *layout) { bool progress = false; @@ -458,7 +459,7 @@ anv_nir_lower_ycbcr_textures(nir_shader *shader, struct anv_pipeline *pipeline) continue; nir_tex_instr *tex = nir_instr_as_tex(instr); - function_progress |= try_lower_tex_ycbcr(pipeline, &builder, tex); + function_progress |= try_lower_tex_ycbcr(layout, &builder, tex); } } diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c index 4e66f8665fa..4dc18096af5 100644 --- a/src/intel/vulkan/anv_pipeline.c +++ b/src/intel/vulkan/anv_pipeline.c @@ -349,6 +349,7 @@ populate_cs_prog_key(const struct gen_device_info *devinfo, static void anv_pipeline_hash_shader(struct anv_pipeline *pipeline, + struct anv_pipeline_layout *layout, struct anv_shader_module *module, const char *entrypoint, gl_shader_stage stage, @@ -363,9 +364,8 @@ anv_pipeline_hash_shader(struct anv_pipeline *pipeline, _mesa_sha1_update(&ctx, &pipeline->subpass->view_mask, sizeof(pipeline->subpass->view_mask)); } - if (pipeline->layout) { - _mesa_sha1_update(&ctx, pipeline->layout->sha1, - sizeof(pipeline->layout->sha1)); + if (layout) { + _mesa_sha1_update(&ctx, layout->sha1, sizeof(layout->sha1)); } </blockquote><div> </div><div>Now that it's one line, we can drop the braces. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> _mesa_sha1_update(&ctx, module->sha1, sizeof(module->sha1)); _mesa_sha1_update(&ctx, entrypoint, strlen(entrypoint)); @@ -382,6 +382,7 @@ anv_pipeline_hash_shader(struct anv_pipeline *pipeline, static nir_shader * anv_pipeline_compile(struct anv_pipeline *pipeline, void *mem_ctx, + struct anv_pipeline_layout *layout, struct anv_shader_module *module, const char *entrypoint, gl_shader_stage stage, @@ -398,7 +399,7 @@ anv_pipeline_compile(struct anv_pipeline *pipeline, if (nir == NULL) return NULL; - NIR_PASS_V(nir, anv_nir_lower_ycbcr_textures, pipeline); + NIR_PASS_V(nir, anv_nir_lower_ycbcr_textures, layout); NIR_PASS_V(nir, anv_nir_lower_push_constants); @@ -438,8 +439,8 @@ anv_pipeline_compile(struct anv_pipeline *pipeline, pipeline->needs_data_cache = true; /* Apply the actual pipeline layout to UBOs, SSBOs, and textures */ - if (pipeline->layout) - anv_nir_apply_pipeline_layout(pipeline, nir, prog_data, map); + if (layout) + anv_nir_apply_pipeline_layout(pipeline, layout, nir, prog_data, map); if (stage != MESA_SHADER_COMPUTE) brw_nir_analyze_ubo_ranges(compiler, nir, prog_data->ubo_ranges); @@ -508,8 +509,11 @@ anv_pipeline_compile_vs(struct anv_pipeline *pipeline, populate_vs_prog_key(&pipeline->device->info, &key); + struct anv_pipeline_layout *layout = + anv_pipeline_layout_from_handle(info->layout); </blockquote><div> </div><div>ANV_FROM_HANDLE </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + if (cache) { - anv_pipeline_hash_shader(pipeline, module, entrypoint, + anv_pipeline_hash_shader(pipeline, layout, module, entrypoint, MESA_SHADER_VERTEX, spec_info, &key, sizeof(key), sha1); bin = anv_pipeline_cache_search(cache, sha1, 20); @@ -527,7 +531,7 @@ anv_pipeline_compile_vs(struct anv_pipeline *pipeline, void *mem_ctx = ralloc_context(NULL); - nir_shader *nir = anv_pipeline_compile(pipeline, mem_ctx, + nir_shader *nir = anv_pipeline_compile(pipeline, mem_ctx, layout, module, entrypoint, MESA_SHADER_VERTEX, spec_info, &prog_data.base.base, &map); @@ -633,11 +637,14 @@ anv_pipeline_compile_tcs_tes(struct anv_pipeline *pipeline, populate_sampler_prog_key(&pipeline->device->info, &tes_key.tex); tcs_key.input_vertices = info->pTessellationState->patchControlPoints; + struct anv_pipeline_layout *layout = + anv_pipeline_layout_from_handle(info->layout); </blockquote><div> </div><div>ANV_FROM_HANDLE </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + if (cache) { - anv_pipeline_hash_shader(pipeline, tcs_module, tcs_entrypoint, + anv_pipeline_hash_shader(pipeline, layout, tcs_module, tcs_entrypoint, MESA_SHADER_TESS_CTRL, tcs_spec_info, &tcs_key, sizeof(tcs_key), tcs_sha1); - anv_pipeline_hash_shader(pipeline, tes_module, tes_entrypoint, + anv_pipeline_hash_shader(pipeline, layout, tes_module, tes_entrypoint, MESA_SHADER_TESS_EVAL, tes_spec_info, &tes_key, sizeof(tes_key), tes_sha1); memcpy(&tcs_sha1[20], tes_sha1, 20); @@ -666,11 +673,13 @@ anv_pipeline_compile_tcs_tes(struct anv_pipeline *pipeline, void *mem_ctx = ralloc_context(NULL); nir_shader *tcs_nir = - anv_pipeline_compile(pipeline, mem_ctx, tcs_module, tcs_entrypoint, + anv_pipeline_compile(pipeline, mem_ctx, layout, + tcs_module, tcs_entrypoint, MESA_SHADER_TESS_CTRL, tcs_spec_info, &tcs_prog_data.base.base, &tcs_map); nir_shader *tes_nir = - anv_pipeline_compile(pipeline, mem_ctx, tes_module, tes_entrypoint, + anv_pipeline_compile(pipeline, mem_ctx, layout, + tes_module, tes_entrypoint, MESA_SHADER_TESS_EVAL, tes_spec_info, &tes_prog_data.base.base, &tes_map); if (tcs_nir == NULL || tes_nir == NULL) { @@ -771,8 +780,11 @@ anv_pipeline_compile_gs(struct anv_pipeline *pipeline, populate_gs_prog_key(&pipeline->device->info, &key); + struct anv_pipeline_layout *layout = + anv_pipeline_layout_from_handle(info->layout); </blockquote><div> </div><div>ANV_FROM_HANDLE </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + if (cache) { - anv_pipeline_hash_shader(pipeline, module, entrypoint, + anv_pipeline_hash_shader(pipeline, layout, module, entrypoint, MESA_SHADER_GEOMETRY, spec_info, &key, sizeof(key), sha1); bin = anv_pipeline_cache_search(cache, sha1, 20); @@ -790,7 +802,7 @@ anv_pipeline_compile_gs(struct anv_pipeline *pipeline, void *mem_ctx = ralloc_context(NULL); - nir_shader *nir = anv_pipeline_compile(pipeline, mem_ctx, + nir_shader *nir = anv_pipeline_compile(pipeline, mem_ctx, layout, module, entrypoint, MESA_SHADER_GEOMETRY, spec_info, &prog_data.base.base, &map); @@ -849,8 +861,11 @@ anv_pipeline_compile_fs(struct anv_pipeline *pipeline, populate_wm_prog_key(pipeline, info, &key); + struct anv_pipeline_layout *layout = + anv_pipeline_layout_from_handle(info->layout); </blockquote><div> </div><div>ANV_FROM_HANDLE </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + if (cache) { - anv_pipeline_hash_shader(pipeline, module, entrypoint, + anv_pipeline_hash_shader(pipeline, layout, module, entrypoint, MESA_SHADER_FRAGMENT, spec_info, &key, sizeof(key), sha1); bin = anv_pipeline_cache_search(cache, sha1, 20); @@ -868,7 +883,7 @@ anv_pipeline_compile_fs(struct anv_pipeline *pipeline, void *mem_ctx = ralloc_context(NULL); - nir_shader *nir = anv_pipeline_compile(pipeline, mem_ctx, + nir_shader *nir = anv_pipeline_compile(pipeline, mem_ctx, layout, module, entrypoint, MESA_SHADER_FRAGMENT, spec_info, &prog_data.base, &map); @@ -997,8 +1012,11 @@ anv_pipeline_compile_cs(struct anv_pipeline *pipeline, populate_cs_prog_key(&pipeline->device->info, &key); + struct anv_pipeline_layout *layout = + anv_pipeline_layout_from_handle(info->layout); </blockquote><div> </div><div>ANV_FROM_HANDLE </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + if (cache) { - anv_pipeline_hash_shader(pipeline, module, entrypoint, + anv_pipeline_hash_shader(pipeline, layout, module, entrypoint, MESA_SHADER_COMPUTE, spec_info, &key, sizeof(key), sha1); bin = anv_pipeline_cache_search(cache, sha1, 20); @@ -1016,7 +1034,7 @@ anv_pipeline_compile_cs(struct anv_pipeline *pipeline, void *mem_ctx = ralloc_context(NULL); - nir_shader *nir = anv_pipeline_compile(pipeline, mem_ctx, + nir_shader *nir = anv_pipeline_compile(pipeline, mem_ctx, layout, module, entrypoint, MESA_SHADER_COMPUTE, spec_info, &prog_data.base, &map); -- 2.14.1 </blockquote></div> </div></div>