<div dir="auto"><div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jun 6, 2019, 10:07 AM Samuel Pitoiset <<a href="mailto:samuel.pitoiset@gmail.com">samuel.pitoiset@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On 6/6/19 3:41 AM, Bas Nieuwenhuizen wrote: > On Wed, Jun 5, 2019 at 12:04 PM Samuel Pitoiset > <<a href="mailto:samuel.pitoiset@gmail.com" target="_blank" rel="noreferrer">samuel.pitoiset@gmail.com</a>> wrote: >> >> On 6/5/19 2:51 AM, Bas Nieuwenhuizen wrote: >>> On Thu, May 30, 2019 at 4:02 PM Samuel Pitoiset >>> <<a href="mailto:samuel.pitoiset@gmail.com" target="_blank" rel="noreferrer">samuel.pitoiset@gmail.com</a>> wrote: >>>> From the Vulkan spec 1.1.109: >>>> >>>> "Some implementations may need to evaluate depth image values >>>> while performing image layout transitions. To accommodate this, >>>> instances of the VkSampleLocationsInfoEXT structure can be >>>> specified for each situation where an explicit or automatic >>>> layout transition has to take place. [...] and >>>> VkRenderPassSampleLocationsBeginInfoEXT can be chained from >>>> VkRenderPassBeginInfo to provide sample locations for layout >>>> transitions performed implicitly by a render pass instance." >>>> >>>> Signed-off-by: Samuel Pitoiset <<a href="mailto:samuel.pitoiset@gmail.com" target="_blank" rel="noreferrer">samuel.pitoiset@gmail.com</a>> >>>> --- >>>> src/amd/vulkan/radv_cmd_buffer.c | 155 ++++++++++++++++++++++++++++--- >>>> src/amd/vulkan/radv_private.h | 9 ++ >>>> 2 files changed, 150 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c >>>> index 570acaa0905..81b3f5f9886 100644 >>>> --- a/src/amd/vulkan/radv_cmd_buffer.c >>>> +++ b/src/amd/vulkan/radv_cmd_buffer.c >>>> @@ -2645,11 +2645,55 @@ void radv_subpass_barrier(struct radv_cmd_buffer *cmd_buffer, >>>> NULL); >>>> } >>>> >>>> +static uint32_t >>>> +radv_get_subpass_id(struct radv_cmd_buffer *cmd_buffer) >>>> +{ >>>> + struct radv_cmd_state *state = &cmd_buffer->state; >>>> + uint32_t subpass_id = state->subpass - state->pass->subpasses; >>>> + >>>> + /* The id of this subpass shouldn't exceed the number of subpasses in >>>> + * this render pass minus 1. >>>> + */ >>>> + assert(subpass_id < state->pass->subpass_count); >>>> + return subpass_id; >>>> +} >>>> + >>>> +static struct radv_sample_locations_state * >>>> +radv_get_attachment_sample_locations(struct radv_cmd_buffer *cmd_buffer, >>>> + uint32_t att_idx) >>>> +{ >>>> + struct radv_cmd_state *state = &cmd_buffer->state; >>>> + uint32_t subpass_id = radv_get_subpass_id(cmd_buffer); >>> On the start of the first subpass this may not work as the subpass is >>> not set yet? >> Yes, this patch needs <a href="https://patchwork.freedesktop.org/series/61387/" rel="noreferrer noreferrer" target="_blank">https://patchwork.freedesktop.org/series/61387/</a> >>>> + struct radv_image_view *view = state->framebuffer->attachments[att_idx].attachment; >>>> + >>>> + if (view->image->info.samples == 1) >>>> + return NULL; >>>> + >>>> + if (state->pass->attachments[att_idx].first_subpass_idx == subpass_id) { >>>> + /* Return the initial sample locations if this is the initial >>>> + * layout transition of the given subpass attachemnt. >>>> + */ >>>> + if (state->attachments[att_idx].sample_location.count > 0) >>>> + return &state->attachments[att_idx].sample_location; >>>> + } else { >>>> + /* Otherwise return the subpass sample locations if defined. */ >>>> + if (state->subpass_sample_locs) { >>>> + for (uint32_t i = 0; i < state->num_subpass_sample_locs; i++) { >>>> + if (state->subpass_sample_locs[i].subpass_idx == subpass_id) >>>> + return &state->subpass_sample_locs[i].sample_location; > I think there is an off-by-1 here with the subpass patch applied. > > For the transition from subpass 0 to subpass 1, we should be using the > locations from subpass 0, but by setting the subpass before the > transitions, the subpass_id is 1. No? For the transition from subpass 0 to subpass 1, state->subpass should be equal to state->pass->subpasses, so radv_get_subpass_id() returns 0? </blockquote></div></div><div dir="auto"> </div><div dir="auto">The 0->1 transitions happen in the begin of subpass 1. If you set the subpass before image transitions, the subpass_id is going to be 1.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > >>>> + } >>>> + } >>>> + } >>>> + >>>> + return NULL; >>>> +} >>>> + >>>> static void radv_handle_subpass_image_transition(struct radv_cmd_buffer *cmd_buffer, >>>> struct radv_subpass_attachment att) >>>> { >>>> unsigned idx = att.attachment; >>>> struct radv_image_view *view = cmd_buffer->state.framebuffer->attachments[idx].attachment; >>>> + struct radv_sample_locations_state *sample_locs; >>>> VkImageSubresourceRange range; >>>> range.aspectMask = 0; >>>> range.baseMipLevel = view->base_mip; >>>> @@ -2668,10 +2712,15 @@ static void radv_handle_subpass_image_transition(struct radv_cmd_buffer *cmd_buf >>>> range.layerCount = util_last_bit(cmd_buffer->state.subpass->view_mask); >>>> } >>>> >>>> + /* Get the subpass sample locations for the given attachment, if NULL >>>> + * is returned the driver will use the default HW locations. >>>> + */ >>>> + sample_locs = radv_get_attachment_sample_locations(cmd_buffer, idx); >>>> + >>>> radv_handle_image_transition(cmd_buffer, >>>> view->image, >>>> cmd_buffer->state.attachments[idx].current_layout, >>>> - att.layout, 0, 0, &range, NULL); >>>> + att.layout, 0, 0, &range, sample_locs); >>>> >>>> cmd_buffer->state.attachments[idx].current_layout = att.layout; >>>> >>>> @@ -2687,6 +2736,89 @@ radv_cmd_buffer_set_subpass(struct radv_cmd_buffer *cmd_buffer, >>>> cmd_buffer->state.dirty |= RADV_CMD_DIRTY_FRAMEBUFFER; >>>> } >>>> >>>> +static VkResult >>>> +radv_cmd_state_setup_sample_locations(struct radv_cmd_buffer *cmd_buffer, >>>> + struct radv_render_pass *pass, >>>> + const VkRenderPassBeginInfo *info) >>>> +{ >>>> + const struct VkRenderPassSampleLocationsBeginInfoEXT *sample_locs = >>>> + vk_find_struct_const(info->pNext, >>>> + RENDER_PASS_SAMPLE_LOCATIONS_BEGIN_INFO_EXT); >>>> + struct radv_cmd_state *state = &cmd_buffer->state; >>>> + struct radv_framebuffer *framebuffer = state->framebuffer; >>>> + >>>> + if (!sample_locs) { >>>> + state->subpass_sample_locs = NULL; >>>> + return VK_SUCCESS; >>>> + } >>>> + >>>> + for (uint32_t i = 0; i < sample_locs->attachmentInitialSampleLocationsCount; i++) { >>>> + const VkAttachmentSampleLocationsEXT *att_sample_locs = >>>> + &sample_locs->pAttachmentInitialSampleLocations[i]; >>>> + uint32_t att_idx = att_sample_locs->attachmentIndex; >>>> + struct radv_attachment_info *att = &framebuffer->attachments[att_idx]; >>>> + struct radv_image *image = att->attachment->image; >>>> + >>>> + assert(vk_format_is_depth_or_stencil(image->vk_format)); >>>> + >>>> + /* From the Vulkan spec 1.1.108: >>>> + * >>>> + * "If the image referenced by the framebuffer attachment at >>>> + * index attachmentIndex was not created with >>>> + * VK_IMAGE_CREATE_SAMPLE_LOCATIONS_COMPATIBLE_DEPTH_BIT_EXT >>>> + * then the values specified in sampleLocationsInfo are >>>> + * ignored." >>>> + */ >>>> + if (!(image->flags & VK_IMAGE_CREATE_SAMPLE_LOCATIONS_COMPATIBLE_DEPTH_BIT_EXT)) >>>> + continue; >>>> + >>>> + const VkSampleLocationsInfoEXT *sample_locs_info = >>>> + &att_sample_locs->sampleLocationsInfo; >>>> + >>>> + state->attachments[att_idx].sample_location.per_pixel = >>>> + sample_locs_info->sampleLocationsPerPixel; >>>> + state->attachments[att_idx].sample_location.grid_size = >>>> + sample_locs_info->sampleLocationGridSize; >>>> + state->attachments[att_idx].sample_location.count = >>>> + sample_locs_info->sampleLocationsCount; >>>> + typed_memcpy(&state->attachments[att_idx].sample_location.locations[0], >>>> + sample_locs_info->pSampleLocations, >>>> + sample_locs_info->sampleLocationsCount); >>>> + } >>>> + >>>> + state->subpass_sample_locs = vk_alloc(&cmd_buffer->pool->alloc, >>>> + sample_locs->postSubpassSampleLocationsCount * >>>> + sizeof(state->subpass_sample_locs[0]), >>>> + 8, VK_SYSTEM_ALLOCATION_SCOPE_OBJECT); >>>> + if (state->subpass_sample_locs == NULL) { >>>> + cmd_buffer->record_result = VK_ERROR_OUT_OF_HOST_MEMORY; >>>> + return cmd_buffer->record_result; >>>> + } >>>> + >>>> + state->num_subpass_sample_locs = sample_locs->postSubpassSampleLocationsCount; >>>> + >>>> + for (uint32_t i = 0; i < sample_locs->postSubpassSampleLocationsCount; i++) { >>>> + const VkSubpassSampleLocationsEXT *subpass_sample_locs_info = >>>> + &sample_locs->pPostSubpassSampleLocations[i]; >>>> + const VkSampleLocationsInfoEXT *sample_locs_info = >>>> + &subpass_sample_locs_info->sampleLocationsInfo; >>>> + >>>> + state->subpass_sample_locs[i].subpass_idx = >>>> + subpass_sample_locs_info->subpassIndex; >>>> + state->subpass_sample_locs[i].sample_location.per_pixel = >>>> + sample_locs_info->sampleLocationsPerPixel; >>>> + state->subpass_sample_locs[i].sample_location.grid_size = >>>> + sample_locs_info->sampleLocationGridSize; >>>> + state->subpass_sample_locs[i].sample_location.count = >>>> + sample_locs_info->sampleLocationsCount; >>>> + typed_memcpy(&state->subpass_sample_locs[i].sample_location.locations[0], >>>> + sample_locs_info->pSampleLocations, >>>> + sample_locs_info->sampleLocationsCount); >>>> + } >>>> + >>>> + return VK_SUCCESS; >>>> +} >>>> + >>>> static VkResult >>>> radv_cmd_state_setup_attachments(struct radv_cmd_buffer *cmd_buffer, >>>> struct radv_render_pass *pass, >>>> @@ -2741,6 +2873,7 @@ radv_cmd_state_setup_attachments(struct radv_cmd_buffer *cmd_buffer, >>>> } >>>> >>>> state->attachments[i].current_layout = att->initial_layout; >>>> + state->attachments[i].sample_location.count = 0; >>>> } >>>> >>>> return VK_SUCCESS; >>>> @@ -3171,6 +3304,7 @@ VkResult radv_EndCommandBuffer( >>>> si_cp_dma_wait_for_idle(cmd_buffer); >>>> >>>> vk_free(&cmd_buffer->pool->alloc, cmd_buffer->state.attachments); >>>> + vk_free(&cmd_buffer->pool->alloc, cmd_buffer->state.subpass_sample_locs); >>>> >>>> if (!cmd_buffer->device->ws->cs_finalize(cmd_buffer->cs)) >>>> return vk_error(cmd_buffer->device->instance, VK_ERROR_OUT_OF_DEVICE_MEMORY); >>>> @@ -3669,19 +3803,6 @@ void radv_TrimCommandPool( >>>> } >>>> } >>>> >>>> -static uint32_t >>>> -radv_get_subpass_id(struct radv_cmd_buffer *cmd_buffer) >>>> -{ >>>> - struct radv_cmd_state *state = &cmd_buffer->state; >>>> - uint32_t subpass_id = state->subpass - state->pass->subpasses; >>>> - >>>> - /* The id of this subpass shouldn't exceed the number of subpasses in >>>> - * this render pass minus 1. >>>> - */ >>>> - assert(subpass_id < state->pass->subpass_count); >>>> - return subpass_id; >>>> -} >>>> - >>>> static void >>>> radv_cmd_buffer_begin_subpass(struct radv_cmd_buffer *cmd_buffer, >>>> uint32_t subpass_id) >>>> @@ -3751,6 +3872,10 @@ void radv_CmdBeginRenderPass( >>>> if (result != VK_SUCCESS) >>>> return; >>>> >>>> + result = radv_cmd_state_setup_sample_locations(cmd_buffer, pass, pRenderPassBegin); >>>> + if (result != VK_SUCCESS) >>>> + return; >>>> + >>>> radv_cmd_buffer_begin_subpass(cmd_buffer, 0); >>>> } >>>> >>>> @@ -4593,11 +4718,13 @@ void radv_CmdEndRenderPass( >>>> radv_cmd_buffer_end_subpass(cmd_buffer); >>>> >>>> vk_free(&cmd_buffer->pool->alloc, cmd_buffer->state.attachments); >>>> + vk_free(&cmd_buffer->pool->alloc, cmd_buffer->state.subpass_sample_locs); >>>> >>>> cmd_buffer->state.pass = NULL; >>>> cmd_buffer->state.subpass = NULL; >>>> cmd_buffer->state.attachments = NULL; >>>> cmd_buffer->state.framebuffer = NULL; >>>> + cmd_buffer->state.subpass_sample_locs = NULL; >>>> } >>>> >>>> void radv_CmdEndRenderPass2KHR( >>>> diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h >>>> index 180f8b976ab..76cc79b387d 100644 >>>> --- a/src/amd/vulkan/radv_private.h >>>> +++ b/src/amd/vulkan/radv_private.h >>>> @@ -1024,6 +1024,7 @@ struct radv_attachment_state { >>>> uint32_t cleared_views; >>>> VkClearValue clear_value; >>>> VkImageLayout current_layout; >>>> + struct radv_sample_locations_state sample_location; >>>> }; >>>> >>>> struct radv_descriptor_state { >>>> @@ -1035,6 +1036,11 @@ struct radv_descriptor_state { >>>> uint32_t dynamic_buffers[4 * MAX_DYNAMIC_BUFFERS]; >>>> }; >>>> >>>> +struct radv_subpass_sample_locs_state { >>>> + uint32_t subpass_idx; >>>> + struct radv_sample_locations_state sample_location; >>>> +}; >>>> + >>>> struct radv_cmd_state { >>>> /* Vertex descriptors */ >>>> uint64_t vb_va; >>>> @@ -1057,6 +1063,9 @@ struct radv_cmd_state { >>>> struct radv_streamout_state streamout; >>>> VkRect2D render_area; >>>> >>>> + uint32_t num_subpass_sample_locs; >>>> + struct radv_subpass_sample_locs_state * subpass_sample_locs; >>>> + >>>> /* Index buffer */ >>>> struct radv_buffer *index_buffer; >>>> uint64_t index_offset; >>>> -- >>>> 2.21.0 >>>> >>>> _______________________________________________ >>>> mesa-dev mailing list >>>> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank" rel="noreferrer">mesa-dev@lists.freedesktop.org</a> >>>> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div></div>