<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jan 15, 2018 at 7:11 AM, Pohjolainen, Topi <span dir="ltr"><<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Dec 15, 2017 at 05:09:09PM -0800, Jason Ekstrand wrote:<br>
> The Vulkan spec says:<br>
><br>
>     "pipelineBindPoint is a VkPipelineBindPoint indicating whether the<br>
>     descriptors will be used by graphics pipelines or compute pipelines.<br>
>     There is a separate set of bind points for each of graphics and<br>
>     compute, so binding one does not disturb the other."<br>
><br>
> Up until now, we've been ignoring the pipeline bind point and had just<br>
> one bind point for everything.  This commit separates things out into<br>
> separate bind points.<br>
><br>
> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=102897" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/<wbr>show_bug.cgi?id=102897</a><br>
> ---<br>
>  src/intel/vulkan/anv_cmd_<wbr>buffer.c     | 65 ++++++++++++++++++++++++++----<wbr>-----<br>
>  src/intel/vulkan/anv_<wbr>descriptor_set.c |  2 ++<br>
>  src/intel/vulkan/anv_private.h        | 11 +++---<br>
>  src/intel/vulkan/genX_cmd_<wbr>buffer.c    | 24 +++++++------<br>
>  4 files changed, 70 insertions(+), 32 deletions(-)<br>
><br>
> diff --git a/src/intel/vulkan/anv_cmd_<wbr>buffer.c b/src/intel/vulkan/anv_cmd_<wbr>buffer.c<br>
> index 636f515..9720e7e 100644<br>
> --- a/src/intel/vulkan/anv_cmd_<wbr>buffer.c<br>
> +++ b/src/intel/vulkan/anv_cmd_<wbr>buffer.c<br>
> @@ -124,12 +124,20 @@ anv_cmd_state_init(struct anv_cmd_buffer *cmd_buffer)<br>
>  }<br>
><br>
>  static void<br>
> +anv_cmd_pipeline_state_<wbr>finish(struct anv_cmd_buffer *cmd_buffer,<br>
> +                              struct anv_cmd_pipeline_state *pipe_state)<br>
> +{<br>
> +   for (uint32_t i = 0; i < ARRAY_SIZE(pipe_state->push_<wbr>descriptors); i++)<br>
> +      vk_free(&cmd_buffer->pool-><wbr>alloc, pipe_state->push_descriptors[<wbr>i]);<br>
> +}<br>
> +<br>
> +static void<br>
>  anv_cmd_state_finish(struct anv_cmd_buffer *cmd_buffer)<br>
>  {<br>
>     struct anv_cmd_state *state = &cmd_buffer->state;<br>
><br>
> -   for (uint32_t i = 0; i < ARRAY_SIZE(state->push_<wbr>descriptors); i++)<br>
> -      vk_free(&cmd_buffer->pool-><wbr>alloc, state->push_descriptors[i]);<br>
> +   anv_cmd_pipeline_state_finish(<wbr>cmd_buffer, &state->gfx.base);<br>
> +   anv_cmd_pipeline_state_finish(<wbr>cmd_buffer, &state->compute.base);<br>
><br>
>     for (uint32_t i = 0; i < MESA_SHADER_STAGES; i++)<br>
>        vk_free(&cmd_buffer->pool-><wbr>alloc, state->push_constants[i]);<br>
> @@ -495,6 +503,7 @@ void anv_CmdSetStencilReference(<br>
><br>
>  static void<br>
>  anv_cmd_buffer_bind_<wbr>descriptor_set(struct anv_cmd_buffer *cmd_buffer,<br>
> +                                   VkPipelineBindPoint bind_point,<br>
>                                     struct anv_pipeline_layout *layout,<br>
>                                     uint32_t set_index,<br>
>                                     struct anv_descriptor_set *set,<br>
> @@ -504,7 +513,14 @@ anv_cmd_buffer_bind_<wbr>descriptor_set(struct anv_cmd_buffer *cmd_buffer,<br>
>     struct anv_descriptor_set_layout *set_layout =<br>
>        layout->set[set_index].layout;<br>
><br>
> -   cmd_buffer->state.descriptors[<wbr>set_index] = set;<br>
> +   struct anv_cmd_pipeline_state *pipe_state;<br>
> +   if (bind_point == VK_PIPELINE_BIND_POINT_<wbr>COMPUTE) {<br>
> +      pipe_state = &cmd_buffer->state.compute.<wbr>base;<br>
> +   } else {<br>
> +      assert(bind_point == VK_PIPELINE_BIND_POINT_<wbr>GRAPHICS);<br>
> +      pipe_state = &cmd_buffer->state.gfx.base;<br>
> +   }<br>
> +   pipe_state->descriptors[set_<wbr>index] = set;<br>
><br>
>     if (dynamic_offsets) {<br>
>        if (set_layout->dynamic_offset_<wbr>count > 0) {<br>
> @@ -514,9 +530,9 @@ anv_cmd_buffer_bind_<wbr>descriptor_set(struct anv_cmd_buffer *cmd_buffer,<br>
>           /* Assert that everything is in range */<br>
>           assert(set_layout->dynamic_<wbr>offset_count <= *dynamic_offset_count);<br>
>           assert(dynamic_offset_start + set_layout->dynamic_offset_<wbr>count <=<br>
> -                ARRAY_SIZE(cmd_buffer->state.<wbr>dynamic_offsets));<br>
> +                ARRAY_SIZE(pipe_state-><wbr>dynamic_offsets));<br>
><br>
> -         typed_memcpy(&cmd_buffer-><wbr>state.dynamic_offsets[dynamic_<wbr>offset_start],<br>
> +         typed_memcpy(&pipe_state-><wbr>dynamic_offsets[dynamic_<wbr>offset_start],<br>
>                        *dynamic_offsets, set_layout->dynamic_offset_<wbr>count);<br>
><br>
>           *dynamic_offsets += set_layout->dynamic_offset_<wbr>count;<br>
> @@ -524,7 +540,13 @@ anv_cmd_buffer_bind_<wbr>descriptor_set(struct anv_cmd_buffer *cmd_buffer,<br>
>        }<br>
>     }<br>
><br>
> -   cmd_buffer->state.descriptors_<wbr>dirty |= set_layout->shader_stages;<br>
> +   if (bind_point == VK_PIPELINE_BIND_POINT_<wbr>COMPUTE) {<br>
> +      cmd_buffer->state.descriptors_<wbr>dirty |= VK_SHADER_STAGE_COMPUTE_BIT;<br>
> +   } else {<br>
> +      assert(bind_point == VK_PIPELINE_BIND_POINT_<wbr>GRAPHICS);<br>
> +      cmd_buffer->state.descriptors_<wbr>dirty |=<br>
> +         set_layout->shader_stages & VK_SHADER_STAGE_ALL_GRAPHICS;<br>
<br>
</div></div>Should we put () around the right hand side? We seem to be using that<br>
elsewhere.<br><div><div class="h5"></div></div></blockquote><div><br></div><div>Meh.  I think it's fairly obvious what's going on.  I do sometimes insist on wraping == statements in () because a = b == c is something I find hard to read but a = b & c seems obvious to me.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> +   }<br>
>  }<br>
><br>
>  void anv_CmdBindDescriptorSets(<br>
> @@ -544,8 +566,8 @@ void anv_CmdBindDescriptorSets(<br>
><br>
>     for (uint32_t i = 0; i < descriptorSetCount; i++) {<br>
>        ANV_FROM_HANDLE(anv_<wbr>descriptor_set, set, pDescriptorSets[i]);<br>
> -      anv_cmd_buffer_bind_<wbr>descriptor_set(cmd_buffer, layout,<br>
> -                                         firstSet + i, set,<br>
> +      anv_cmd_buffer_bind_<wbr>descriptor_set(cmd_buffer, pipelineBindPoint,<br>
> +                                         layout, firstSet + i, set,<br>
>                                           &dynamicOffsetCount,<br>
>                                           &pDynamicOffsets);<br>
>     }<br>
> @@ -851,10 +873,19 @@ anv_cmd_buffer_get_depth_<wbr>stencil_view(const struct anv_cmd_buffer *cmd_buffer)<br>
><br>
>  static struct anv_push_descriptor_set *<br>
>  anv_cmd_buffer_get_push_<wbr>descriptor_set(struct anv_cmd_buffer *cmd_buffer,<br>
> +                                       VkPipelineBindPoint bind_point,<br>
>                                         uint32_t set)<br>
>  {<br>
> +   struct anv_cmd_pipeline_state *pipe_state;<br>
> +   if (bind_point == VK_PIPELINE_BIND_POINT_<wbr>COMPUTE) {<br>
> +      pipe_state = &cmd_buffer->state.compute.<wbr>base;<br>
> +   } else {<br>
> +      assert(bind_point == VK_PIPELINE_BIND_POINT_<wbr>GRAPHICS);<br>
> +      pipe_state = &cmd_buffer->state.gfx.base;<br>
> +   }<br>
> +<br>
>     struct anv_push_descriptor_set **push_set =<br>
> -      &cmd_buffer->state.push_<wbr>descriptors[set];<br>
> +      &pipe_state->push_descriptors[<wbr>set];<br>
><br>
>     if (*push_set == NULL) {<br>
>        *push_set = vk_alloc(&cmd_buffer->pool-><wbr>alloc,<br>
> @@ -880,15 +911,14 @@ void anv_CmdPushDescriptorSetKHR(<br>
>     ANV_FROM_HANDLE(anv_cmd_<wbr>buffer, cmd_buffer, commandBuffer);<br>
>     ANV_FROM_HANDLE(anv_pipeline_<wbr>layout, layout, _layout);<br>
><br>
> -   assert(pipelineBindPoint == VK_PIPELINE_BIND_POINT_<wbr>GRAPHICS ||<br>
> -          pipelineBindPoint == VK_PIPELINE_BIND_POINT_<wbr>COMPUTE);<br>
>     assert(_set < MAX_SETS);<br>
><br>
>     const struct anv_descriptor_set_layout *set_layout =<br>
>        layout->set[_set].layout;<br>
><br>
>     struct anv_push_descriptor_set *push_set =<br>
> -      anv_cmd_buffer_get_push_<wbr>descriptor_set(cmd_buffer, _set);<br>
> +      anv_cmd_buffer_get_push_<wbr>descriptor_set(cmd_buffer,<br>
> +                                             pipelineBindPoint, _set);<br>
>     if (!push_set)<br>
>        return;<br>
><br>
> @@ -958,8 +988,8 @@ void anv_CmdPushDescriptorSetKHR(<br>
>        }<br>
>     }<br>
><br>
> -   anv_cmd_buffer_bind_<wbr>descriptor_set(cmd_buffer, layout, _set,<br>
> -                                      set, NULL, NULL);<br>
> +   anv_cmd_buffer_bind_<wbr>descriptor_set(cmd_buffer, pipelineBindPoint,<br>
> +                                      layout, _set, set, NULL, NULL);<br>
>  }<br>
><br>
>  void anv_<wbr>CmdPushDescriptorSetWithTempla<wbr>teKHR(<br>
> @@ -980,7 +1010,8 @@ void anv_<wbr>CmdPushDescriptorSetWithTempla<wbr>teKHR(<br>
>        layout->set[_set].layout;<br>
><br>
>     struct anv_push_descriptor_set *push_set =<br>
> -      anv_cmd_buffer_get_push_<wbr>descriptor_set(cmd_buffer, _set);<br>
> +      anv_cmd_buffer_get_push_<wbr>descriptor_set(cmd_buffer,<br>
> +                                             template->bind_point, _set);<br>
>     if (!push_set)<br>
>        return;<br>
><br>
> @@ -997,6 +1028,6 @@ void anv_<wbr>CmdPushDescriptorSetWithTempla<wbr>teKHR(<br>
>                                       template,<br>
>                                       pData);<br>
><br>
> -   anv_cmd_buffer_bind_<wbr>descriptor_set(cmd_buffer, layout, _set,<br>
> -                                      set, NULL, NULL);<br>
> +   anv_cmd_buffer_bind_<wbr>descriptor_set(cmd_buffer, template->bind_point,<br>
> +                                      layout, _set, set, NULL, NULL);<br>
>  }<br>
> diff --git a/src/intel/vulkan/anv_<wbr>descriptor_set.c b/src/intel/vulkan/anv_<wbr>descriptor_set.c<br>
> index 629c041..63557ab 100644<br>
> --- a/src/intel/vulkan/anv_<wbr>descriptor_set.c<br>
> +++ b/src/intel/vulkan/anv_<wbr>descriptor_set.c<br>
> @@ -891,6 +891,8 @@ VkResult anv_<wbr>CreateDescriptorUpdateTemplate<wbr>KHR(<br>
>     if (template == NULL)<br>
>        return vk_error(VK_ERROR_OUT_OF_HOST_<wbr>MEMORY);<br>
><br>
> +   template->bind_point = pCreateInfo-><wbr>pipelineBindPoint;<br>
> +<br>
>     if (pCreateInfo->templateType == VK_DESCRIPTOR_UPDATE_TEMPLATE_<wbr>TYPE_DESCRIPTOR_SET_KHR)<br>
>        template->set = pCreateInfo->set;<br>
><br>
> diff --git a/src/intel/vulkan/anv_<wbr>private.h b/src/intel/vulkan/anv_<wbr>private.h<br>
> index e8d0d27..f1868a3 100644<br>
> --- a/src/intel/vulkan/anv_<wbr>private.h<br>
> +++ b/src/intel/vulkan/anv_<wbr>private.h<br>
> @@ -1319,6 +1319,8 @@ struct anv_descriptor_template_entry {<br>
>  };<br>
><br>
>  struct anv_descriptor_update_template {<br>
> +    VkPipelineBindPoint bind_point;<br>
> +<br>
>     /* The descriptor set this template corresponds to. This value is only<br>
>      * valid if the template was created with the templateType<br>
>      * VK_DESCRIPTOR_UPDATE_TEMPLATE_<wbr>TYPE_DESCRIPTOR_SET_KHR.<br>
> @@ -1686,6 +1688,11 @@ struct anv_attachment_state {<br>
>   */<br>
>  struct anv_cmd_pipeline_state {<br>
>     struct anv_pipeline *pipeline;<br>
> +<br>
> +   struct anv_descriptor_set *descriptors[MAX_SETS];<br>
> +   uint32_t dynamic_offsets[MAX_DYNAMIC_<wbr>BUFFERS];<br>
> +<br>
> +   struct anv_push_descriptor_set *push_descriptors[MAX_SETS];<br>
>  };<br>
><br>
>  /** State tracking for graphics pipeline<br>
> @@ -1734,16 +1741,12 @@ struct anv_cmd_state {<br>
>     VkRect2D                                     render_area;<br>
>     uint32_t                                     restart_index;<br>
>     struct anv_vertex_binding                    vertex_bindings[MAX_VBS];<br>
> -   struct anv_descriptor_set *                  descriptors[MAX_SETS];<br>
> -   uint32_t                                     dynamic_offsets[MAX_DYNAMIC_<wbr>BUFFERS];<br>
>     VkShaderStageFlags                           push_constant_stages;<br>
>     struct anv_push_constants *                  push_constants[MESA_SHADER_<wbr>STAGES];<br>
>     struct anv_state                             binding_tables[MESA_SHADER_<wbr>STAGES];<br>
>     struct anv_state                             samplers[MESA_SHADER_STAGES];<br>
>     struct anv_dynamic_state                     dynamic;<br>
><br>
> -   struct anv_push_descriptor_set *             push_descriptors[MAX_SETS];<br>
> -<br>
>     /**<br>
>      * Whether or not the gen8 PMA fix is enabled.  We ensure that, at the top<br>
>      * of any command buffer it is disabled by disabling it in EndCommandBuffer<br>
> diff --git a/src/intel/vulkan/genX_cmd_<wbr>buffer.c b/src/intel/vulkan/genX_cmd_<wbr>buffer.c<br>
> index 994c996..75aa40a 100644<br>
> --- a/src/intel/vulkan/genX_cmd_<wbr>buffer.c<br>
> +++ b/src/intel/vulkan/genX_cmd_<wbr>buffer.c<br>
> @@ -1434,32 +1434,32 @@ cmd_buffer_alloc_push_<wbr>constants(struct anv_cmd_buffer *cmd_buffer)<br>
>  }<br>
><br>
>  static const struct anv_descriptor *<br>
> -anv_descriptor_for_binding(<wbr>const struct anv_cmd_buffer *cmd_buffer,<br>
> +anv_descriptor_for_binding(<wbr>const struct anv_cmd_pipeline_state *pipe_state,<br>
>                             const struct anv_pipeline_binding *binding)<br>
>  {<br>
>     assert(binding->set < MAX_SETS);<br>
>     const struct anv_descriptor_set *set =<br>
> -      cmd_buffer->state.descriptors[<wbr>binding->set];<br>
> +      pipe_state->descriptors[<wbr>binding->set];<br>
>     const uint32_t offset =<br>
>        set->layout->binding[binding-><wbr>binding].descriptor_index;<br>
>     return &set->descriptors[offset + binding->index];<br>
>  }<br>
><br>
>  static uint32_t<br>
> -dynamic_offset_for_binding(<wbr>const struct anv_cmd_buffer *cmd_buffer,<br>
> +dynamic_offset_for_binding(<wbr>const struct anv_cmd_pipeline_state *pipe_state,<br>
>                             const struct anv_pipeline *pipeline,<br>
>                             const struct anv_pipeline_binding *binding)<br>
>  {<br>
>     assert(binding->set < MAX_SETS);<br>
>     const struct anv_descriptor_set *set =<br>
> -      cmd_buffer->state.descriptors[<wbr>binding->set];<br>
> +      pipe_state->descriptors[<wbr>binding->set];<br>
><br>
>     uint32_t dynamic_offset_idx =<br>
>        pipeline->layout->set[binding-<wbr>>set].dynamic_offset_start +<br>
>        set->layout->binding[binding-><wbr>binding].dynamic_offset_index +<br>
>        binding->index;<br>
><br>
> -   return cmd_buffer->state.dynamic_<wbr>offsets[dynamic_offset_idx];<br>
> +   return pipe_state->dynamic_offsets[<wbr>dynamic_offset_idx];<br>
>  }<br>
><br>
>  static VkResult<br>
> @@ -1567,7 +1567,7 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,<br>
>        }<br>
><br>
>        const struct anv_descriptor *desc =<br>
> -         anv_descriptor_for_binding(<wbr>cmd_buffer, binding);<br>
> +         anv_descriptor_for_binding(<wbr>pipe_state, binding);<br>
><br>
>        switch (desc->type) {<br>
>        case VK_DESCRIPTOR_TYPE_SAMPLER:<br>
> @@ -1643,7 +1643,7 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,<br>
>        case VK_DESCRIPTOR_TYPE_STORAGE_<wbr>BUFFER_DYNAMIC: {<br>
>           /* Compute the offset within the buffer */<br>
>           uint32_t dynamic_offset =<br>
> -            dynamic_offset_for_binding(<wbr>cmd_buffer, pipeline, binding);<br>
> +            dynamic_offset_for_binding(<wbr>pipe_state, pipeline, binding);<br>
>           uint64_t offset = desc->offset + dynamic_offset;<br>
>           /* Clamp to the buffer size */<br>
>           offset = MIN2(offset, desc->buffer->size);<br>
> @@ -1724,7 +1724,7 @@ emit_samplers(struct anv_cmd_buffer *cmd_buffer,<br>
>     for (uint32_t s = 0; s < map->sampler_count; s++) {<br>
>        struct anv_pipeline_binding *binding = &map->sampler_to_descriptor[s]<wbr>;<br>
>        const struct anv_descriptor *desc =<br>
> -         anv_descriptor_for_binding(<wbr>cmd_buffer, binding);<br>
> +         anv_descriptor_for_binding(<wbr>pipe_state, binding);<br>
><br>
>        if (desc->type != VK_DESCRIPTOR_TYPE_SAMPLER &&<br>
>            desc->type != VK_DESCRIPTOR_TYPE_COMBINED_<wbr>IMAGE_SAMPLER)<br>
> @@ -1848,7 +1848,8 @@ static void<br>
>  cmd_buffer_flush_push_<wbr>constants(struct anv_cmd_buffer *cmd_buffer,<br>
>                                  VkShaderStageFlags dirty_stages)<br>
>  {<br>
> -   const struct anv_pipeline *pipeline = cmd_buffer->state.gfx.base.<wbr>pipeline;<br>
> +   const struct anv_cmd_graphics_state *gfx_state = &cmd_buffer->state.gfx;<br>
> +   const struct anv_pipeline *pipeline = gfx_state->base.pipeline;<br>
><br>
>     static const uint32_t push_constant_opcodes[] = {<br>
>        [MESA_SHADER_VERTEX]                      = 21,<br>
> @@ -1901,7 +1902,7 @@ cmd_buffer_flush_push_<wbr>constants(struct anv_cmd_buffer *cmd_buffer,<br>
>                    &bind_map->surface_to_<wbr>descriptor[surface];<br>
><br>
>                 const struct anv_descriptor *desc =<br>
> -                  anv_descriptor_for_binding(<wbr>cmd_buffer, binding);<br>
> +                  anv_descriptor_for_binding(&<wbr>gfx_state->base, binding);<br>
><br>
>                 struct anv_address read_addr;<br>
>                 uint32_t read_len;<br>
> @@ -1917,7 +1918,8 @@ cmd_buffer_flush_push_<wbr>constants(struct anv_cmd_buffer *cmd_buffer,<br>
>                    assert(desc->type == VK_DESCRIPTOR_TYPE_UNIFORM_<wbr>BUFFER_DYNAMIC);<br>
><br>
>                    uint32_t dynamic_offset =<br>
> -                     dynamic_offset_for_binding(<wbr>cmd_buffer, pipeline, binding);<br>
> +                     dynamic_offset_for_binding(&<wbr>gfx_state->base,<br>
> +                                                pipeline, binding);<br>
>                    uint32_t buf_offset =<br>
>                       MIN2(desc->offset + dynamic_offset, desc->buffer->size);<br>
>                    uint32_t buf_range =<br>
> --<br>
> 2.5.0.400.gff86faf<br>
><br>
</div></div>> ______________________________<wbr>_________________<br>
> mesa-dev mailing list<br>
> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</blockquote></div><br></div></div>