<div dir="ltr"><div>Regardless if what you choose to do with my comments, this patch is </div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div class="gmail_extra"> <div class="gmail_quote">On Thu, Oct 5, 2017 at 9:01 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Thu, Sep 28, 2017 at 5:05 PM, Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com" target="_blank">lionel.g.landwerlin@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">When writing to set > 0, we were just wrongly writing to set 0. This commit fixes this by lazily allocating each set as we write to them. We didn't go for having them directly into the command buffer as this would require an additional ~45Kb per command buffer. v2: Allocate push descriptors from system memory rather than in BO streams. (Lionel) Cc: "17.2 17.1" <<a href="mailto:mesa-stable@lists.freedesktop.org" target="_blank">mesa-stable@lists.freedesktop.org</a>> Fixes: 9f60ed98e501 ("anv: add VK_KHR_push_descriptor support") Reported-by: Daniel Ribeiro Maciel <<a href="mailto:daniel.maciel@gmail.com" target="_blank">daniel.maciel@gmail.com</a>> Signed-off-by: Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com" target="_blank">lionel.g.landwerlin@intel.com</a>> --- src/intel/vulkan/anv_cmd_buffer.c | 58 ++++++++++++++++++++++++++++++++++----- src/intel/vulkan/anv_private.h | 3 +- 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/src/intel/vulkan/anv_cmd_buffer.c b/src/intel/vulkan/anv_cmd_buffer.c index 3b59af8f6f4..2ef7f9bf260 100644 --- a/src/intel/vulkan/anv_cmd_buffer.c +++ b/src/intel/vulkan/anv_cmd_buffer.c @@ -120,6 +120,12 @@ anv_cmd_state_reset(struct anv_cmd_buffer *cmd_buffer) cmd_buffer->batch.status = VK_SUCCESS; memset(&state->descriptors, 0, sizeof(state->descriptors)); + for (uint32_t i = 0; i < ARRAY_SIZE(state->push_descriptors); i++) { + if (state->push_descriptors[i] != NULL) { + vk_free(&cmd_buffer->pool->alloc, state->push_descriptors[i]); </blockquote><div> </div></div></div><div>It's safe to vk_free a null pointer. There's no need for the check. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + state->push_descriptors[i] = 0; </blockquote><div> </div><div>This is a pointer, NULL would be more appropriate. </div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + } + } for (uint32_t i = 0; i < MESA_SHADER_STAGES; i++) { if (state->push_constants[i] != NULL) { vk_free(&cmd_buffer->pool->alloc, state->push_constants[i]); @@ -216,8 +222,8 @@ static VkResult anv_create_cmd_buffer( anv_state_stream_init(&cmd_buffer->dynamic_state_stream, &device->dynamic_state_pool, 16384); - memset(&cmd_buffer->state.push_descriptor, 0, - sizeof(cmd_buffer->state.push_descriptor)); + memset(cmd_buffer->state.push_descriptors, 0, + sizeof(cmd_buffer->state.push_descriptors)); if (pool) { list_addtail(&cmd_buffer->pool_link, &pool->cmd_buffers); @@ -269,6 +275,8 @@ VkResult anv_AllocateCommandBuffers( static void anv_cmd_buffer_destroy(struct anv_cmd_buffer *cmd_buffer) { + struct anv_cmd_state *state = &cmd_buffer->state; + list_del(&cmd_buffer->pool_link); anv_cmd_buffer_fini_batch_bo_chain(cmd_buffer); @@ -276,7 +284,13 @@ anv_cmd_buffer_destroy(struct anv_cmd_buffer *cmd_buffer) anv_state_stream_finish(&cmd_buffer->surface_state_stream); anv_state_stream_finish(&cmd_buffer->dynamic_state_stream); - vk_free(&cmd_buffer->pool->alloc, cmd_buffer->state.attachments); + for (uint32_t i = 0; i < ARRAY_SIZE(state->push_descriptors); i++) { + if (state->push_descriptors[i] != NULL) { + vk_free(&cmd_buffer->pool->alloc, state->push_descriptors[i]); </blockquote><div> </div></div></div><div>Same comment as above. You don't need the NULL check. Also, I realized while reviewing this that we aren't freeing the push constant structs. I just sent a patch which calls anv_cmd_state_reset in anv_cmd_buffer_destroy which should let you drop this hunk. </div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + } + } + + vk_free(&cmd_buffer->pool->alloc, state->attachments); vk_free(&cmd_buffer->pool->alloc, cmd_buffer); } @@ -834,6 +848,26 @@ anv_cmd_buffer_get_depth_stencil_view(const struct anv_cmd_buffer *cmd_buffer) return iview; } +static VkResult +anv_cmd_buffer_ensure_push_descriptor_set(struct anv_cmd_buffer *cmd_buffer, + uint32_t set) +{ + struct anv_push_descriptor_set **push_set = + &cmd_buffer->state.push_descriptors[set]; + + if (*push_set == NULL) { + *push_set = vk_alloc(&cmd_buffer->pool->alloc, + sizeof(struct anv_push_descriptor_set), 8, + VK_SYSTEM_ALLOCATION_SCOPE_OBJECT); + if (*push_set == NULL) { + anv_batch_set_error(&cmd_buffer->batch, VK_ERROR_OUT_OF_HOST_MEMORY); + return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY); + } + } + + return VK_SUCCESS; +} + void anv_CmdPushDescriptorSetKHR( VkCommandBuffer commandBuffer, VkPipelineBindPoint pipelineBindPoint, @@ -851,12 +885,17 @@ void anv_CmdPushDescriptorSetKHR( const struct anv_descriptor_set_layout *set_layout = layout->set[_set].layout; - struct anv_descriptor_set *set = &cmd_buffer->state.push_descriptor.set; + + if (anv_cmd_buffer_ensure_push_descriptor_set(cmd_buffer, _set) != VK_SUCCESS) + return; + struct anv_push_descriptor_set *push_set = + cmd_buffer->state.push_descriptors[_set]; + struct anv_descriptor_set *set = &push_set->set; set->layout = set_layout; set->size = anv_descriptor_set_layout_size(set_layout); set->buffer_count = set_layout->buffer_count; - set->buffer_views = cmd_buffer->state.push_descriptor.buffer_views; + set->buffer_views = push_set->buffer_views; /* Go through the user supplied descriptors. */ for (uint32_t i = 0; i < descriptorWriteCount; i++) { @@ -937,12 +976,17 @@ void anv_CmdPushDescriptorSetWithTemplateKHR( const struct anv_descriptor_set_layout *set_layout = layout->set[_set].layout; - struct anv_descriptor_set *set = &cmd_buffer->state.push_descriptor.set; + + if (anv_cmd_buffer_ensure_push_descriptor_set(cmd_buffer, _set) != VK_SUCCESS) + return; + struct anv_push_descriptor_set *push_set = + cmd_buffer->state.push_descriptors[_set]; + struct anv_descriptor_set *set = &push_set->set; set->layout = set_layout; set->size = anv_descriptor_set_layout_size(set_layout); set->buffer_count = set_layout->buffer_count; - set->buffer_views = cmd_buffer->state.push_descriptor.buffer_views; + set->buffer_views = push_set->buffer_views; anv_descriptor_set_write_template(set, cmd_buffer->device, diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h index 3ba623a37fd..e81385263f2 100644 --- a/src/intel/vulkan/anv_private.h +++ b/src/intel/vulkan/anv_private.h @@ -1268,7 +1268,6 @@ struct anv_push_descriptor_set { /* Put this field right behind anv_descriptor_set so it fills up the * descriptors[0] field. */ struct anv_descriptor descriptors[MAX_PUSH_DESCRIPTORS]; - struct anv_buffer_view buffer_views[MAX_PUSH_DESCRIPTORS]; }; @@ -1686,7 +1685,7 @@ struct anv_cmd_state { struct anv_dynamic_state dynamic; bool need_query_wa; - struct anv_push_descriptor_set push_descriptor; + struct anv_push_descriptor_set * push_descriptors[MAX_SETS]; /** * Whether or not the gen8 PMA fix is enabled. We ensure that, at the top -- 2.14.2 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">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> </div></div> </blockquote></div> </div>