<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Apr 12, 2017 at 2:54 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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" <<a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a>> I think specifying the specific stable branch in quotes has been deprecated according to <a href="https://www.mesa3d.org/submittingpatches.html#nominations" rel="noreferrer" target="_blank">https://www.mesa3d.org/submittingpatches.html#nominations</a> <div><div class="h5"> > --- > 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; </div></div>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. </blockquote><div> </div><div>Not a good reason, but I prefer the simpler assert above but trying to do that here would have meant an extra unneeded level of control-flow nesting. The one above is just an "if (...) return true;" so adding nesting wasn't a big deal. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Nevertheless, this series is Reviewed-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> <div><div class="h5"> > 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 > </div></div>> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>