[Mesa-stable] [Mesa-dev] [PATCH 1/2] anv: fix push descriptors with set > 0

Jason Ekstrand jason at jlekstrand.net
Fri Oct 6 04:03:23 UTC 2017


Regardless if what you choose to do with my comments, this patch is

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

On Thu, Oct 5, 2017 at 9:01 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:

> On Thu, Sep 28, 2017 at 5:05 PM, Lionel Landwerlin <
> lionel.g.landwerlin at 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-stable at lists.freedesktop.org>
>> Fixes: 9f60ed98e501 ("anv: add VK_KHR_push_descriptor support")
>> Reported-by: Daniel Ribeiro Maciel <daniel.maciel at gmail.com>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20171005/b16a2b23/attachment.html>


More information about the mesa-stable mailing list