[Mesa-dev] [PATCH 6/6] anv: Implement VK_KHR_depth_stencil_resolve
Jason Ekstrand
jason at jlekstrand.net
Fri Jan 11 18:31:51 UTC 2019
On Fri, Jan 11, 2019 at 3:21 AM Iago Toral <itoral at igalia.com> wrote:
> On Mon, 2019-01-07 at 09:39 -0600, Jason Ekstrand wrote:
> > ---
> > src/intel/vulkan/anv_device.c | 28 ++++++
> > src/intel/vulkan/anv_extensions.py | 1 +
> > src/intel/vulkan/anv_pass.c | 37 +++++++-
> > src/intel/vulkan/anv_private.h | 3 +
> > src/intel/vulkan/genX_cmd_buffer.c | 136
> > +++++++++++++++++++++++++++++
> > 5 files changed, 204 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/intel/vulkan/anv_device.c
> > b/src/intel/vulkan/anv_device.c
> > index 2a3919d2949..3761846bb7f 100644
> > --- a/src/intel/vulkan/anv_device.c
> > +++ b/src/intel/vulkan/anv_device.c
> > @@ -1138,6 +1138,34 @@ void anv_GetPhysicalDeviceProperties2(
> > break;
> > }
> >
> > + case
> > VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_DEPTH_STENCIL_RESOLVE_PROPERTIES_KH
> > R: {
> > + VkPhysicalDeviceDepthStencilResolvePropertiesKHR *props =
> > + (VkPhysicalDeviceDepthStencilResolvePropertiesKHR *)ext;
> > +
> > + /* We support all of the depth resolve modes */
> > + props->supportedDepthResolveModes =
> > + VK_RESOLVE_MODE_SAMPLE_ZERO_BIT_KHR |
> > + VK_RESOLVE_MODE_AVERAGE_BIT_KHR |
> > + VK_RESOLVE_MODE_MIN_BIT_KHR |
> > + VK_RESOLVE_MODE_MAX_BIT_KHR;
> > +
> > + /* Average doesn't make sense for stencil so we don't
> > support that */
> > + props->supportedStencilResolveModes =
> > + VK_RESOLVE_MODE_SAMPLE_ZERO_BIT_KHR;
> > + if (pdevice->info.gen >= 8) {
> > + /* The advanced stencil resolve modes currently require
> > stencil
> > + * sampling be supported by the hardware.
> > + */
> > + props->supportedStencilResolveModes |=
> > + VK_RESOLVE_MODE_MIN_BIT_KHR |
> > + VK_RESOLVE_MODE_MAX_BIT_KHR;
> > + }
> > +
> > + props->independentResolveNone = VK_TRUE;
> > + props->independentResolve = VK_TRUE;
> > + break;
> > + }
> > +
> > case VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_DRIVER_PROPERTIES_KHR:
> > {
> > VkPhysicalDeviceDriverPropertiesKHR *driver_props =
> > (VkPhysicalDeviceDriverPropertiesKHR *) ext;
> > diff --git a/src/intel/vulkan/anv_extensions.py
> > b/src/intel/vulkan/anv_extensions.py
> > index 388845003aa..2ea4cab0e97 100644
> > --- a/src/intel/vulkan/anv_extensions.py
> > +++ b/src/intel/vulkan/anv_extensions.py
> > @@ -76,6 +76,7 @@ EXTENSIONS = [
> > Extension('VK_KHR_bind_memory2', 1, True),
> > Extension('VK_KHR_create_renderpass2', 1, True),
> > Extension('VK_KHR_dedicated_allocation', 1, True),
> > + Extension('VK_KHR_depth_stencil_resolve', 1, True),
> > Extension('VK_KHR_descriptor_update_template', 1, True),
> > Extension('VK_KHR_device_group', 1, True),
> > Extension('VK_KHR_device_group_creation', 1, True),
> > diff --git a/src/intel/vulkan/anv_pass.c
> > b/src/intel/vulkan/anv_pass.c
> > index 7b17cc06935..196cf3ff8fd 100644
> > --- a/src/intel/vulkan/anv_pass.c
> > +++ b/src/intel/vulkan/anv_pass.c
> > @@ -74,6 +74,10 @@ anv_render_pass_compile(struct anv_render_pass
> > *pass)
> > subpass->depth_stencil_attachment->attachment ==
> > VK_ATTACHMENT_UNUSED)
> > subpass->depth_stencil_attachment = NULL;
> >
> > + if (subpass->ds_resolve_attachment &&
> > + subpass->ds_resolve_attachment->attachment ==
> > VK_ATTACHMENT_UNUSED)
> > + subpass->ds_resolve_attachment = NULL;
> > +
>
> This is a nitpick, but since we setup subpass->ds_resolve_attachment in
> anv_CreateRenderPass2KHR(), should't we just do this sanitation there?
>
Maybe? It's an interesting philosophical question. The original idea
behind the compile step was to make stuff like this unified between the two
create paths. That said, this only happens in the CreateRenderPass2KHR
path so should it go there? I don't know. I'm inclined to leave it as-is
if that's ok.
> > for (uint32_t j = 0; j < subpass->attachment_count; j++) {
> > struct anv_subpass_attachment *subpass_att = &subpass-
> > >attachments[j];
> > if (subpass_att->attachment == VK_ATTACHMENT_UNUSED)
> > @@ -116,6 +120,16 @@ anv_render_pass_compile(struct anv_render_pass
> > *pass)
> > color_att->usage |= VK_IMAGE_USAGE_TRANSFER_SRC_BIT;
> > }
> > }
> > +
> > + if (subpass->ds_resolve_attachment) {
> > + struct anv_subpass_attachment *ds_att =
> > + subpass->depth_stencil_attachment;
> > + UNUSED struct anv_subpass_attachment *resolve_att =
> > + subpass->ds_resolve_attachment;
> > +
> > + assert(resolve_att->usage ==
> > VK_IMAGE_USAGE_TRANSFER_DST_BIT);
> > + ds_att->usage |= VK_IMAGE_USAGE_TRANSFER_SRC_BIT;
> > + }
> > }
> >
> > /* From the Vulkan 1.0.39 spec:
> > @@ -342,10 +356,15 @@ VkResult anv_CreateRenderPass(
> > static unsigned
> > num_subpass_attachments2(const VkSubpassDescription2KHR *desc)
> > {
> > + const VkSubpassDescriptionDepthStencilResolveKHR *ds_resolve =
> > + vk_find_struct_const(desc->pNext,
> > + SUBPASS_DESCRIPTION_DEPTH_STENCIL_RESOLVE
> > _KHR);
> > +
> > return desc->inputAttachmentCount +
> > desc->colorAttachmentCount +
> > (desc->pResolveAttachments ? desc->colorAttachmentCount :
> > 0) +
> > - (desc->pDepthStencilAttachment != NULL);
> > + (desc->pDepthStencilAttachment != NULL) +
> > + (ds_resolve && ds_resolve-
> > >pDepthStencilResolveAttachment);
>
> If my suggestion below makes sense I guess could simplify this a bit
> and just add one if ds_resolve exists.
>
> > }
> >
> > VkResult anv_CreateRenderPass2KHR(
> > @@ -460,6 +479,22 @@ VkResult anv_CreateRenderPass2KHR(
> > .layout = desc->pDepthStencilAttachment->layout,
> > };
> > }
> > +
> > + const VkSubpassDescriptionDepthStencilResolveKHR *ds_resolve =
> > + vk_find_struct_const(desc->pNext,
> > + SUBPASS_DESCRIPTION_DEPTH_STENCIL_RESO
> > LVE_KHR);
> > +
> > + if (ds_resolve && ds_resolve->pDepthStencilResolveAttachment)
> > {
>
> Not that it matters much in practice I guess, but I understand that
> having a VkSubpassDescriptionDepthStencilResolveKHR without a valid
> pDepthStencilResolveAttachment is an application bug, right? In that
> case maybe something like this would make more sense?:
>
I just double-checked the spec and pDepthStencilResolveAttachment is
allowed to be NULL. Thanks for making me double-check though!
> if (ds_resolve) {
> assert(ds_resolve->pDepthStencilResolveAttachment);
> ...
> }
>
> > + subpass->ds_resolve_attachment = subpass_attachments++;
> > +
> > + *subpass->ds_resolve_attachment = (struct
> > anv_subpass_attachment) {
> > + .usage = VK_IMAGE_USAGE_TRANSFER_DST_BIT,
> > + .attachment = ds_resolve-
> > >pDepthStencilResolveAttachment->attachment,
> > + .layout = ds_resolve-
> > >pDepthStencilResolveAttachment->layout,
> > + };
> > + subpass->depth_resolve_mode = ds_resolve->depthResolveMode;
> > + subpass->stencil_resolve_mode = ds_resolve-
> > >stencilResolveMode;
> > + }
> > }
> >
> > for (uint32_t i = 0; i < pCreateInfo->dependencyCount; i++)
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index 6992db277fc..e16de7a7a45 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -3215,6 +3215,9 @@ struct anv_subpass {
> > struct anv_subpass_attachment * resolve_attachments;
> >
> > struct anv_subpass_attachment
> > * depth_stencil_attachment;
> > + struct anv_subpass_attachment
> > * ds_resolve_attachment;
> > + VkResolveModeFlagBitsKHR depth_resolve_mode;
> > + VkResolveModeFlagBitsKHR stencil_resolve_mode
> > ;
> >
> > uint32_t view_mask;
> >
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index b0b56472e57..7aadafd687e 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -3876,6 +3876,23 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
> > *cmd_buffer,
> > cmd_buffer_emit_depth_stencil(cmd_buffer);
> > }
> >
> > +static enum blorp_filter
> > +vk_to_blorp_resolve_mode(VkResolveModeFlagBitsKHR vk_mode)
> > +{
> > + switch (vk_mode) {
> > + case VK_RESOLVE_MODE_SAMPLE_ZERO_BIT_KHR:
> > + return BLORP_FILTER_SAMPLE_0;
> > + case VK_RESOLVE_MODE_AVERAGE_BIT_KHR:
> > + return BLORP_FILTER_AVERAGE;
> > + case VK_RESOLVE_MODE_MIN_BIT_KHR:
> > + return BLORP_FILTER_MIN_SAMPLE;
> > + case VK_RESOLVE_MODE_MAX_BIT_KHR:
> > + return BLORP_FILTER_MAX_SAMPLE;
> > + default:
> > + return BLORP_FILTER_NONE;
> > + }
> > +}
> > +
> > static void
> > cmd_buffer_end_subpass(struct anv_cmd_buffer *cmd_buffer)
> > {
> > @@ -3943,6 +3960,125 @@ cmd_buffer_end_subpass(struct anv_cmd_buffer
> > *cmd_buffer)
> > }
> > }
> >
> > + if (subpass->ds_resolve_attachment) {
> > + /* We are about to do some MSAA resolves. We need to flush so
> > that the
> > + * result of writes to the MSAA color attachments show up in
>
> This is not a color resolve path, I guess this was a copy&paste.
>
Yup. Fixed.
> > the sampler
> > + * when we blit to the single-sampled resolve target.
> > + */
> > + cmd_buffer->state.pending_pipe_bits |=
> > + ANV_PIPE_TEXTURE_CACHE_INVALIDATE_BIT |
> > + ANV_PIPE_DEPTH_CACHE_FLUSH_BIT;
> > +
> > + uint32_t src_att = subpass->depth_stencil_attachment-
> > >attachment;
> > + uint32_t dst_att = subpass->ds_resolve_attachment->attachment;
> > +
> > + assert(src_att < cmd_buffer->state.pass->attachment_count);
> > + assert(dst_att < cmd_buffer->state.pass->attachment_count);
> > +
> > + if (cmd_buffer-
> > >state.attachments[dst_att].pending_clear_aspects) {
> > + /* From the Vulkan 1.0 spec:
> > + *
> > + * If the first use of an attachment in a render pass is
> > as a
> > + * resolve attachment, then the loadOp is effectively
> > ignored
> > + * as the resolve is guaranteed to overwrite all pixels
> > in the
> > + * render area.
> > + */
> > + cmd_buffer-
> > >state.attachments[dst_att].pending_clear_aspects = 0;
> > + }
> > +
> > + struct anv_image_view *src_iview = fb->attachments[src_att];
> > + struct anv_image_view *dst_iview = fb->attachments[dst_att];
> > +
> > + const VkRect2D render_area = cmd_buffer->state.render_area;
> > +
> > + if ((src_iview->image->aspects & VK_IMAGE_ASPECT_DEPTH_BIT) &&
> > + subpass->depth_resolve_mode != VK_RESOLVE_MODE_NONE_KHR) {
> > +
> > + struct anv_attachment_state *src_state =
> > + &cmd_state->attachments[src_att];
> > + struct anv_attachment_state *dst_state =
> > + &cmd_state->attachments[dst_att];
> > +
> > + /* MSAA resolves sample from the source
> > attachment. Transition the
> > + * depth attachment first to get rid of any HiZ that we may
> > not be
> > + * able to handle.
> > + */
> > + transition_depth_buffer(cmd_buffer, src_iview->image,
> > + src_state->current_layout,
> > + VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OP
> > TIMAL);
> > + src_state->aux_usage =
> > + anv_layout_to_aux_usage(&cmd_buffer->device->info,
> > src_iview->image,
> > + VK_IMAGE_ASPECT_DEPTH_BIT,
> > +
> > _OPTIMAL);
> > + src_state->current_layout =
> > VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL;
> > +
> > + /* MSAA resolves write to the resolve attachment as if it
> > were any
> > + * other transfer op. Transition the resolve attachment
> > accordingly.
> > + */
> > + VkImageLayout dst_initial_layout = dst_state-
> > >current_layout;
> > +
> > + /* If our render area is the entire size of the image,
> > we're going to
> > + * blow it all away so we can claim the initial layout is
> > UNDEFINED
> > + * and we'll get a HiZ ambiguate instead of a resolve.
> > + */
> > + if (dst_iview->image->type != VK_IMAGE_TYPE_3D &&
> > + render_area.offset.x == 0 && render_area.offset.y == 0
> > &&
> > + render_area.extent.width == dst_iview->extent.width &&
> > + render_area.extent.height == dst_iview->extent.height)
> > + dst_initial_layout = VK_IMAGE_LAYOUT_UNDEFINED;
> > +
> > + transition_depth_buffer(cmd_buffer, dst_iview->image,
> > + dst_initial_layout,
> > + VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMA
> > L);
> > + dst_state->aux_usage =
> > + anv_layout_to_aux_usage(&cmd_buffer->device->info,
> > dst_iview->image,
> > + VK_IMAGE_ASPECT_DEPTH_BIT,
> > + VK_IMAGE_LAYOUT_TRANSFER_DST_OPT
> > IMAL);
> > + dst_state->current_layout =
> > VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL;
> > +
> > + enum blorp_filter filter =
> > + vk_to_blorp_resolve_mode(subpass->depth_resolve_mode);
> > +
> > + anv_image_msaa_resolve(cmd_buffer,
> > + src_iview->image, src_state-
> > >aux_usage,
> > + src_iview->planes[0].isl.base_level,
> > + src_iview-
> > >planes[0].isl.base_array_layer,
> > + dst_iview->image, dst_state-
> > >aux_usage,
> > + dst_iview->planes[0].isl.base_level,
> > + dst_iview-
> > >planes[0].isl.base_array_layer,
> > + VK_IMAGE_ASPECT_DEPTH_BIT,
> > + render_area.offset.x,
> > render_area.offset.y,
> > + render_area.offset.x,
> > render_area.offset.y,
> > + render_area.extent.width,
> > + render_area.extent.height,
> > + fb->layers, filter);
> > + }
> > +
> > + if ((src_iview->image->aspects & VK_IMAGE_ASPECT_STENCIL_BIT)
> > &&
> > + subpass->stencil_resolve_mode != VK_RESOLVE_MODE_NONE_KHR)
> > {
> > +
> > + enum isl_aux_usage src_aux_usage = ISL_AUX_USAGE_NONE;
> > + enum isl_aux_usage dst_aux_usage = ISL_AUX_USAGE_NONE;
> > +
> > + enum blorp_filter filter =
> > + vk_to_blorp_resolve_mode(subpass->stencil_resolve_mode);
> > +
> > + anv_image_msaa_resolve(cmd_buffer,
> > + src_iview->image, src_aux_usage,
> > + src_iview->planes[0].isl.base_level,
> > + src_iview-
> > >planes[0].isl.base_array_layer,
> > + dst_iview->image, dst_aux_usage,
> > + dst_iview->planes[0].isl.base_level,
> > + dst_iview-
> > >planes[0].isl.base_array_layer,
> > + VK_IMAGE_ASPECT_STENCIL_BIT,
> > + render_area.offset.x,
> > render_area.offset.y,
> > + render_area.offset.x,
> > render_area.offset.y,
> > + render_area.extent.width,
> > + render_area.extent.height,
> > + fb->layers, filter);
> > + }
> > + }
> > +
> > for (uint32_t i = 0; i < subpass->attachment_count; ++i) {
> > const uint32_t a = subpass->attachments[i].attachment;
> > if (a == VK_ATTACHMENT_UNUSED)
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190111/5ae73f35/attachment-0001.html>
More information about the mesa-dev
mailing list