I had a few nits below. With those fixed, Reviewed-by: Jason Ekstrand <[email protected]>
On Thu, Jan 25, 2018 at 4:24 AM, Iago Toral Quiroga <[email protected]> wrote: > 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 <[email protected]> > --- > 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)); > } > Now that it's one line, we can drop the braces. > _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); > ANV_FROM_HANDLE > + > 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); > ANV_FROM_HANDLE > + > 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); > ANV_FROM_HANDLE > + > 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); > ANV_FROM_HANDLE > + > 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); > ANV_FROM_HANDLE > + > 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 > >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
