Mesa (master): v3dv: cleanup/remove support for pre-generated variants
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Sat Nov 14 16:19:06 UTC 2020
Module: Mesa
Branch: master
Commit: 1fe52af61a4e7bf17ea6a37159b1fe1dab850b1b
URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=1fe52af61a4e7bf17ea6a37159b1fe1dab850b1b
Author: Alejandro Piñeiro <apinheiro at igalia.com>
Date: Tue Nov 10 22:31:05 2020 +0100
v3dv: cleanup/remove support for pre-generated variants
In preparation to the changes that would allow to not need them.
It is worth to note that it is likely (we have some ideas in mind)
that we would need to bring back pre-generate variants on the
future. The approach is slightly different on v3dv_pipeline vs
v3dv_cmd_buffer:
* v3dv_pipeline: even after the clean-up, we had code for all the
functions they have, even if they were doing less things
(specifically, a second shader variant), so they still make sense
on their own, and serve as template for adding support of multiple
pre-generated shader variants in the future.
* v3dv_cmd_buffer: as we really don't need to fill up the key with
some after-pipeline data, we would end with some functions empty
(specifically cmd_buffer_populate_v3d_key). Even as a placeholder,
that would be odd. Additionally the current code has a lot of
boilerplate code (functions to fill up vs, cs and fs keys are
basically the same), and we already have in mind refactor them. So
it would be better to remove all of them, instead of keeping
around some code we would not be happy with. If in the future we
pregenerate more that one variant, hopefully the new code to chose
between them would be better.
v2: clarify the commit message, and fix typos on the comments (Iago)
Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/7545>
---
src/broadcom/vulkan/v3dv_cmd_buffer.c | 213 +---------------------------------
src/broadcom/vulkan/v3dv_pipeline.c | 53 ++-------
src/broadcom/vulkan/v3dv_private.h | 4 -
3 files changed, 12 insertions(+), 258 deletions(-)
diff --git a/src/broadcom/vulkan/v3dv_cmd_buffer.c b/src/broadcom/vulkan/v3dv_cmd_buffer.c
index 1f12c73bab5..967f1acba56 100644
--- a/src/broadcom/vulkan/v3dv_cmd_buffer.c
+++ b/src/broadcom/vulkan/v3dv_cmd_buffer.c
@@ -2895,205 +2895,6 @@ job_update_ez_state(struct v3dv_job *job,
}
}
-/* Note that the following poopulate methods doesn't do a detailed fill-up of
- * the v3d_fs_key. Here we just fill-up cmd_buffer specific info. All info
- * coming from the pipeline create info was alredy filled up when the pipeline
- * was created
- */
-static void
-cmd_buffer_populate_v3d_key(struct v3d_key *key,
- struct v3dv_cmd_buffer *cmd_buffer,
- VkPipelineBindPoint pipeline_binding)
-{
- if (cmd_buffer->state.pipeline->combined_index_map != NULL) {
- struct v3dv_descriptor_map *texture_map = &cmd_buffer->state.pipeline->texture_map;
- struct v3dv_descriptor_map *sampler_map = &cmd_buffer->state.pipeline->sampler_map;
- struct v3dv_descriptor_state *descriptor_state =
- &cmd_buffer->state.descriptor_state[pipeline_binding];
-
- /* At pipeline creation time it was pre-generated an all-16 bit and an
- * all-32 bit variant, so let's do the same here to avoid as much as
- * possible a new compilation.
- */
- uint32_t v3d_key_return_size = 16;
- hash_table_foreach(cmd_buffer->state.pipeline->combined_index_map, entry) {
- uint32_t combined_idx = (uint32_t)(uintptr_t) (entry->data);
- uint32_t combined_idx_key =
- cmd_buffer->state.pipeline->combined_index_to_key_map[combined_idx];
- uint32_t texture_idx;
- uint32_t sampler_idx;
-
- v3dv_pipeline_combined_index_key_unpack(combined_idx_key,
- &texture_idx, &sampler_idx);
-
- VkFormat vk_format;
- const struct v3dv_format *format;
-
- format =
- v3dv_descriptor_map_get_texture_format(descriptor_state,
- texture_map,
- cmd_buffer->state.pipeline->layout,
- texture_idx,
- &vk_format);
-
- const struct v3dv_sampler *sampler = NULL;
- if (sampler_idx != V3DV_NO_SAMPLER_IDX) {
- sampler =
- v3dv_descriptor_map_get_sampler(descriptor_state,
- sampler_map,
- cmd_buffer->state.pipeline->layout,
- sampler_idx);
- assert(sampler);
- }
-
- key->tex[combined_idx].return_size =
- v3dv_get_tex_return_size(format,
- sampler ? sampler->compare_enable : false);
-
- if (key->tex[combined_idx].return_size == 32) {
- v3d_key_return_size = 32;
- }
- }
- v3d_key_update_return_size(cmd_buffer->state.pipeline, key,
- v3d_key_return_size);
- }
-}
-
-static void
-update_fs_variant(struct v3dv_cmd_buffer *cmd_buffer)
-{
- struct v3dv_shader_variant *variant;
- struct v3dv_pipeline_stage *p_stage = cmd_buffer->state.pipeline->fs;
- struct v3d_fs_key local_key;
-
- /* We start with a copy of the original pipeline key */
- memcpy(&local_key, &p_stage->key.fs, sizeof(struct v3d_fs_key));
-
- cmd_buffer_populate_v3d_key(&local_key.base, cmd_buffer,
- VK_PIPELINE_BIND_POINT_GRAPHICS);
-
- VkResult vk_result;
- variant = v3dv_get_shader_variant(p_stage, NULL, &local_key.base,
- sizeof(struct v3d_fs_key),
- &cmd_buffer->device->alloc,
- &vk_result);
- /* At this point we are not creating a vulkan object to return to the
- * API user, so we can't really return back a OOM error
- */
- assert(variant);
- assert(vk_result == VK_SUCCESS);
-
- if (p_stage->current_variant != variant) {
- v3dv_shader_variant_unref(cmd_buffer->device, p_stage->current_variant);
- }
- p_stage->current_variant = variant;
-}
-
-static void
-update_vs_variant(struct v3dv_cmd_buffer *cmd_buffer)
-{
- struct v3dv_shader_variant *variant;
- struct v3dv_pipeline_stage *p_stage;
- struct v3d_vs_key local_key;
- VkResult vk_result;
-
- /* We start with a copy of the original pipeline key */
- p_stage = cmd_buffer->state.pipeline->vs;
- memcpy(&local_key, &p_stage->key.vs, sizeof(struct v3d_vs_key));
-
- cmd_buffer_populate_v3d_key(&local_key.base, cmd_buffer,
- VK_PIPELINE_BIND_POINT_GRAPHICS);
-
- variant = v3dv_get_shader_variant(p_stage, NULL, &local_key.base,
- sizeof(struct v3d_vs_key),
- &cmd_buffer->device->alloc,
- &vk_result);
- /* At this point we are not creating a vulkan object to return to the
- * API user, so we can't really return back a OOM error
- */
- assert(variant);
- assert(vk_result == VK_SUCCESS);
-
- if (p_stage->current_variant != variant) {
- v3dv_shader_variant_unref(cmd_buffer->device, p_stage->current_variant);
- }
- p_stage->current_variant = variant;
-
- /* Now the vs_bin */
- p_stage = cmd_buffer->state.pipeline->vs_bin;
- memcpy(&local_key, &p_stage->key.vs, sizeof(struct v3d_vs_key));
-
- cmd_buffer_populate_v3d_key(&local_key.base, cmd_buffer,
- VK_PIPELINE_BIND_POINT_GRAPHICS);
- variant = v3dv_get_shader_variant(p_stage, NULL, &local_key.base,
- sizeof(struct v3d_vs_key),
- &cmd_buffer->device->alloc,
- &vk_result);
-
- /* At this point we are not creating a vulkan object to return to the
- * API user, so we can't really return back a OOM error
- */
- assert(variant);
- assert(vk_result == VK_SUCCESS);
-
- if (p_stage->current_variant != variant) {
- v3dv_shader_variant_unref(cmd_buffer->device, p_stage->current_variant);
- }
- p_stage->current_variant = variant;
-}
-
-static void
-update_cs_variant(struct v3dv_cmd_buffer *cmd_buffer)
-{
- struct v3dv_shader_variant *variant;
- struct v3dv_pipeline_stage *p_stage = cmd_buffer->state.pipeline->cs;
- struct v3d_key local_key;
-
- /* We start with a copy of the original pipeline key */
- memcpy(&local_key, &p_stage->key.base, sizeof(struct v3d_key));
-
- cmd_buffer_populate_v3d_key(&local_key, cmd_buffer,
- VK_PIPELINE_BIND_POINT_COMPUTE);
-
- VkResult result;
- variant = v3dv_get_shader_variant(p_stage, NULL, &local_key,
- sizeof(struct v3d_key),
- &cmd_buffer->device->alloc,
- &result);
- /* At this point we are not creating a vulkan object to return to the
- * API user, so we can't really return back a OOM error
- */
- assert(variant);
- assert(result == VK_SUCCESS);
-
- if (p_stage->current_variant != variant) {
- v3dv_shader_variant_unref(cmd_buffer->device, p_stage->current_variant);
- }
- p_stage->current_variant = variant;
-}
-
-/*
- * Some updates on the cmd buffer requires also updates on the shader being
- * compiled at the pipeline. The poster boy here are textures, as the compiler
- * needs to do certain things depending on the texture format. So here we
- * re-create the v3d_keys and update the variant. Note that internally the
- * pipeline has a variant cache (hash table) to avoid unneeded compilations
- *
- */
-static void
-update_pipeline_variants(struct v3dv_cmd_buffer *cmd_buffer)
-{
- assert(cmd_buffer->state.pipeline);
-
- if (v3dv_pipeline_get_binding_point(cmd_buffer->state.pipeline) ==
- VK_PIPELINE_BIND_POINT_GRAPHICS) {
- update_fs_variant(cmd_buffer);
- update_vs_variant(cmd_buffer);
- } else {
- update_cs_variant(cmd_buffer);
- }
-}
-
static void
bind_graphics_pipeline(struct v3dv_cmd_buffer *cmd_buffer,
struct v3dv_pipeline *pipeline)
@@ -4249,13 +4050,6 @@ cmd_buffer_emit_pre_draw(struct v3dv_cmd_buffer *cmd_buffer)
job = cmd_buffer_pre_draw_split_job(cmd_buffer);
job->draw_count++;
- /* We may need to compile shader variants based on bound textures */
- uint32_t *dirty = &cmd_buffer->state.dirty;
- if (*dirty & (V3DV_CMD_DIRTY_PIPELINE |
- V3DV_CMD_DIRTY_DESCRIPTOR_SETS)) {
- update_pipeline_variants(cmd_buffer);
- }
-
/* GL shader state binds shaders, uniform and vertex attribute state. The
* compiler injects uniforms to handle some descriptor types (such as
* textures), so we need to regen that when descriptor state changes.
@@ -4263,6 +4057,7 @@ cmd_buffer_emit_pre_draw(struct v3dv_cmd_buffer *cmd_buffer)
* We also need to emit new shader state if we have a dirty viewport since
* that will require that we new uniform state for QUNIFORM_VIEWPORT_*.
*/
+ uint32_t *dirty = &cmd_buffer->state.dirty;
if (*dirty & (V3DV_CMD_DIRTY_PIPELINE |
V3DV_CMD_DIRTY_VERTEX_BUFFER |
V3DV_CMD_DIRTY_DESCRIPTOR_SETS |
@@ -5067,13 +4862,7 @@ cmd_buffer_emit_pre_dispatch(struct v3dv_cmd_buffer *cmd_buffer)
assert(cmd_buffer->state.pipeline);
assert(cmd_buffer->state.pipeline->active_stages == VK_SHADER_STAGE_COMPUTE_BIT);
- /* We may need to compile shader variants based on bound textures */
uint32_t *dirty = &cmd_buffer->state.dirty;
- if (*dirty & (V3DV_CMD_DIRTY_PIPELINE |
- V3DV_CMD_DIRTY_COMPUTE_DESCRIPTOR_SETS)) {
- update_pipeline_variants(cmd_buffer);
- }
-
*dirty &= ~(V3DV_CMD_DIRTY_PIPELINE |
V3DV_CMD_DIRTY_COMPUTE_DESCRIPTOR_SETS);
}
diff --git a/src/broadcom/vulkan/v3dv_pipeline.c b/src/broadcom/vulkan/v3dv_pipeline.c
index 11c0fac7a1b..c5ab1cc5f78 100644
--- a/src/broadcom/vulkan/v3dv_pipeline.c
+++ b/src/broadcom/vulkan/v3dv_pipeline.c
@@ -1587,36 +1587,15 @@ v3dv_get_shader_variant(struct v3dv_pipeline_stage *p_stage,
return variant;
}
-/* This methods updates the return size for a given key. It assumes that it
- * was already properly populated. So for example values for key->num_tex_used
- * should be correct at this point
- *
- * Note that even the @return_size to set is 32bit, it could be overriden to
- * 16bit, like for shadow textures, that we know in advance that they are
- * always 16bit.
- */
-void
-v3d_key_update_return_size(struct v3dv_pipeline *pipeline,
- struct v3d_key *key,
- uint32_t return_size)
-{
- assert(return_size == 32 || return_size == 16);
- struct v3dv_descriptor_map *texture_map = &pipeline->texture_map;
-
- for (uint32_t tex_idx = 0; tex_idx < key->num_tex_used; tex_idx++) {
- key->tex[tex_idx].return_size =
- texture_map->is_shadow[tex_idx] ? 16 : return_size;
-
- key->tex[tex_idx].return_channels =
- key->tex[tex_idx].return_size == 16 ? 2 : 4;
- }
-}
-
/*
* To avoid needed too many shader re-compilation after pipeline creation
* time, we pre-generate several options, so they are available on the default
- * cache. The poster boy here is return size for texture acceses, as the real
- * value needed would depend on the texture format used.
+ * cache.
+ *
+ * NOTE: although some time ago we pre-generated two variants, right now we
+ * only create one variant. We maintain this method because it is really
+ * likely that we would need to rely on multiple variants as we keep working
+ * on possible optimizations that can't be decided at pipeline creation time.
*/
static struct v3dv_shader_variant*
pregenerate_shader_variants(struct v3dv_pipeline_stage *p_stage,
@@ -1626,34 +1605,24 @@ pregenerate_shader_variants(struct v3dv_pipeline_stage *p_stage,
const VkAllocationCallbacks *pAllocator,
VkResult *out_vk_result)
{
- /* We assume that we receive the default 16 return size*/
- struct v3dv_shader_variant *variant_16 =
+ struct v3dv_shader_variant *default_variant =
v3dv_get_shader_variant(p_stage, cache, key, key_size,
pAllocator, out_vk_result);
if (*out_vk_result != VK_SUCCESS)
- return variant_16;
+ return default_variant;
if (!p_stage->pipeline->device->instance->default_pipeline_cache_enabled) {
/* If pipeline cache is disabled it doesn't make sense to pre-generate,
* as we are relying on the default pipeline cache to save the different
* pre-compiled variants
*/
- return variant_16;
+ return default_variant;
}
- v3d_key_update_return_size(p_stage->pipeline, key, 32);
-
- struct v3dv_shader_variant *variant_32 =
- v3dv_get_shader_variant(p_stage, cache, key, key_size,
- pAllocator, out_vk_result);
-
- /* get_shader_variant returns a new ref, so as we are going to use
- * variant_16, we need to unref this.
- */
- v3dv_shader_variant_unref(p_stage->pipeline->device, variant_32);
+ /* FIXME: placeholder. Here we would pre-generate other variants if needed */
- return variant_16;
+ return default_variant;
}
/* FIXME: C&P from st, common place? */
diff --git a/src/broadcom/vulkan/v3dv_private.h b/src/broadcom/vulkan/v3dv_private.h
index 90a5b286781..36193fe5611 100644
--- a/src/broadcom/vulkan/v3dv_private.h
+++ b/src/broadcom/vulkan/v3dv_private.h
@@ -1810,10 +1810,6 @@ struct v3dv_cl_reloc v3dv_write_uniforms_wg_offsets(struct v3dv_cmd_buffer *cmd_
struct v3dv_pipeline_stage *p_stage,
uint32_t **wg_count_offsets);
-void v3d_key_update_return_size(struct v3dv_pipeline *pipeline,
- struct v3d_key *key,
- uint32_t return_size);
-
struct v3dv_shader_variant *
v3dv_get_shader_variant(struct v3dv_pipeline_stage *p_stage,
struct v3dv_pipeline_cache *cache,
More information about the mesa-commit
mailing list