[Mesa-dev] [PATCH 3/3] anv/blorp: Properly handle VK_ATTACHMENT_UNUSED

[Nanley Chery]

On Tue, Apr 11, 2017 at 07:54:23AM -0700, Jason Ekstrand wrote:
> The Vulkan driver was originally written under the assumption that
> VK_ATTACHMENT_UNUSED was basically just for depth-stencil attachments.
> However, the way things fell together, VK_ATTACHMENT_UNUSED can be used
> anywhere in the subpass description.  The blorp-based clear and resolve
> code has a bunch of places where we walk lists of attachments and we
> weren't handling VK_ATTACHMENT_UNUSED everywhere.  This commit should
> fix all of them.
> 
> Cc: "13.0 17.0" <mesa-stable at lists.freedesktop.org>

I think specifying the specific stable branch in quotes has been
deprecated according to
https://www.mesa3d.org/submittingpatches.html#nominations

> ---
>  src/intel/vulkan/anv_blorp.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> index 72a468a..d27132a 100644
> --- a/src/intel/vulkan/anv_blorp.c
> +++ b/src/intel/vulkan/anv_blorp.c
> @@ -1148,6 +1148,9 @@ anv_cmd_buffer_flush_attachments(struct anv_cmd_buffer *cmd_buffer,
>  
>     for (uint32_t i = 0; i < subpass->color_count; ++i) {
>        uint32_t att = subpass->color_attachments[i].attachment;
> +      if (att == VK_ATTACHMENT_UNUSED)
> +         continue;
> +
>        assert(att < pass->attachment_count);
>        if (attachment_needs_flush(cmd_buffer, &pass->attachments[att], stage)) {
>           cmd_buffer->state.pending_pipe_bits |=
> @@ -1175,14 +1178,19 @@ subpass_needs_clear(const struct anv_cmd_buffer *cmd_buffer)
>  
>     for (uint32_t i = 0; i < cmd_state->subpass->color_count; ++i) {
>        uint32_t a = cmd_state->subpass->color_attachments[i].attachment;
> +      if (a == VK_ATTACHMENT_UNUSED)
> +         continue;
> +
> +      assert(a < cmd_state->pass->attachment_count);
>        if (cmd_state->attachments[a].pending_clear_aspects) {
>           return true;
>        }
>     }
>  
> -   if (ds != VK_ATTACHMENT_UNUSED &&
> -       cmd_state->attachments[ds].pending_clear_aspects) {
> -      return true;
> +   if (ds != VK_ATTACHMENT_UNUSED) {
> +      assert(ds < cmd_state->pass->attachment_count);
> +      if (cmd_state->attachments[ds].pending_clear_aspects)
> +         return true;

I'll refer to this hunk below.

>     }
>  
>     return false;
> @@ -1214,6 +1222,10 @@ anv_cmd_buffer_clear_subpass(struct anv_cmd_buffer *cmd_buffer)
>     struct anv_framebuffer *fb = cmd_buffer->state.framebuffer;
>     for (uint32_t i = 0; i < cmd_state->subpass->color_count; ++i) {
>        const uint32_t a = cmd_state->subpass->color_attachments[i].attachment;
> +      if (a == VK_ATTACHMENT_UNUSED)
> +         continue;
> +
> +      assert(a < cmd_state->pass->attachment_count);
>        struct anv_attachment_state *att_state = &cmd_state->attachments[a];
>  
>        if (!att_state->pending_clear_aspects)
> @@ -1273,6 +1285,7 @@ anv_cmd_buffer_clear_subpass(struct anv_cmd_buffer *cmd_buffer)
>     }
>  
>     const uint32_t ds = cmd_state->subpass->depth_stencil_attachment.attachment;
> +   assert(ds == VK_ATTACHMENT_UNUSED || ds < cmd_state->pass->attachment_count);
>  

I wonder why this assertion differs from the one two hunks up.

Nevertheless, this series is
Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>

>     if (ds != VK_ATTACHMENT_UNUSED &&
>         cmd_state->attachments[ds].pending_clear_aspects) {
> @@ -1578,8 +1591,12 @@ anv_cmd_buffer_resolve_subpass(struct anv_cmd_buffer *cmd_buffer)
>     blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer, 0);
>  
>     for (uint32_t i = 0; i < subpass->color_count; ++i) {
> -      ccs_resolve_attachment(cmd_buffer, &batch,
> -                             subpass->color_attachments[i].attachment);
> +      const uint32_t att = subpass->color_attachments[i].attachment;
> +      if (att == VK_ATTACHMENT_UNUSED)
> +         continue;
> +
> +      assert(att < cmd_buffer->state.pass->attachment_count);
> +      ccs_resolve_attachment(cmd_buffer, &batch, att);
>     }
>  
>     anv_cmd_buffer_flush_attachments(cmd_buffer, SUBPASS_STAGE_DRAW);
> @@ -1592,6 +1609,9 @@ anv_cmd_buffer_resolve_subpass(struct anv_cmd_buffer *cmd_buffer)
>           if (dst_att == VK_ATTACHMENT_UNUSED)
>              continue;
>  
> +         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:
>               *
> -- 
> 2.5.0.400.gff86faf
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev