On Thu, Sep 28, 2017 at 5:05 PM, Lionel Landwerlin <
lionel.g.landwer...@intel.com> wrote:
> 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" <mesa-sta...@lists.freedesktop.org>
> Fixes: 9f60ed98e501 ("anv: add VK_KHR_push_descriptor support")
> Reported-by: Daniel Ribeiro Maciel <daniel.mac...@gmail.com>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwer...@intel.com>
> ---
> 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]);
>
It's safe to vk_free a null pointer. There's no need for the check.
> + state->push_descriptors[i] = 0;
>
This is a pointer, NULL would be more appropriate.
> + }
> + }
> 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]);
>
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.
> + }
> + }
> +
> + 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
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
