Regardless if what you choose to do with my comments, this patch is
Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
On Thu, Oct 5, 2017 at 9:01 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> 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_descri
>> ptor.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_descri
>> ptor.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
