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

[Jason Ekstrand]

On Fri, Apr 21, 2017 at 8:05 AM, Juan A. Suarez Romero <jasuarez at igalia.com>
wrote:

> On Tue, 2017-04-11 at 07:54 -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>
> > ---
> >  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;
>
> Seems this relies on commit 608d17b80e617b0052 (anv: Store the user's
> VkAttachmentReference).
>
> At least, without that patch, att would be
>
> uint32_t att = subpass->color_attachments[i];
>
>
> Should we propose the above commit to stable? Or should we keep as it
> is, and use later att.attachment?
>
>
> BTW, this very same happens in couple of places more in this patch.
>

No surprises there, I'm afraid.  If it's not too difficult, I think just
modifying the patch as you described above rather than pulling extra stuff
into stable.

--Jason


> > +      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;
> >     }
> >
> >     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);
> >
> >     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:
> >               *
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20170421/03dc1f44/attachment.html>