[Mesa-dev] [PATCH 2/4] anv/pipeline: don't take the layout from the pipeline to compile shaders
Jason Ekstrand
jason at jlekstrand.net
Thu Jan 25 23:14:54 UTC 2018
I had a few nits below. With those fixed,
Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
On Thu, Jan 25, 2018 at 4:24 AM, Iago Toral Quiroga <itoral at igalia.com>
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 <jason at jlekstrand.net>
> ---
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180125/68f0f2fa/attachment-0001.html>
More information about the mesa-dev
mailing list