<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>