On Wed, Jan 24, 2018 at 2:11 AM, Iago Toral <ito...@igalia.com> wrote:
> On Wed, 2018-01-24 at 01:51 -0800, Jason Ekstrand wrote: > > Thanks for the detailed explanation. I have a couple of comments: > > 1) This is going to conflict with some of the stuff I've got in-flight > that reworks fast-clears significantly. I think it will make everything > easier if we land the reworks first. > > > Ok, no problem. Let me know when that stuff lands and I'll re-work this on > top. Should I also hold back the first patch in this series? > That would make things easier. > 2) It may be better to add a pending_clear_views to anv_attachment_state > and use that for "pending" state and switch pending_clear_aspects to > clear_aspects. > > > Yes, after I wrote this patch I was also wondering whether that would make > things cleaner. I'll do this in a v3. > > Thanks > Iago > > On Wed, Jan 24, 2018 at 12:44 AM, Iago Toral <ito...@igalia.com> wrote: > > That's a good question, I was asking this myself when I did this. The spec > says: > > "loadOp is a VkAttachmentLoadOp > <https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VkAttachmentLoadOp> > value specifying how the contents of color and depth components of the > attachment are treated at the beginning of the subpass where it is first > used." > > So I figured the correct thing to do that follows the spirit of this was > to wait until the first subpass that uses a particular view to clear it. > Now, I have checked the spec (with KHX_multiview included) and it seems to > have some more text in place to address this question:bit more: > > "The load and store operations apply to attachments on a per-view basis. > For example, an attachment using VK_ATTACHMENT_LOAD_OP_CLEAR will have > each view cleared on first use, but the first use of one view may be > temporally distant from the first use of another view. > (...) > This interpretation motivates the answers to questions like “when does the > load op apply” - it is on the first use of each view of an attachment, as > if each view were a separate attachment." > > I understand this is consistent with the implementation in this patch > (although I wonder if there are any scenarios where this has actual > implications...) > > Iago > > On Tue, 2018-01-23 at 23:57 -0800, Jason Ekstrand wrote: > > Do we really want to wait until a subpass that uses the given view or do > we just want to clear all of the relevant slices in the first subpass that > touches it? I haven't really thought about this much. > > On Fri, Jan 5, 2018 at 8:38 AM, Iago Toral Quiroga <ito...@igalia.com> > wrote: > > When multiview is active a subpass clear may only clear a subset of the > attachment layers. Other subpasses in the same render pass may also > clear too and we want to honor those clears as well, however, we need to > ensure that we only clear a layer once, on the first subpass that uses > a particular layer (view) of a given attachment. > > This means that when we check if a subpass attachment needs to be cleared > we need to check if all the layers used by that subpass (as indicated by > its view_mask) have already been cleared in previous subpasses or not, in > which case, we must clear any pending layers used by the subpass, and only > those pending. > > Fixes work-in-progress CTS tests: > dEQP-VK.multiview.readback_implicit_clear.* > --- > src/intel/vulkan/anv_blorp.c | 56 ++++++++++++++++++++++++++++++ > ++++++++++---- > 1 file changed, 52 insertions(+), 4 deletions(-) > > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c > index 18fa4a4ae5..0dac20ea80 100644 > --- a/src/intel/vulkan/anv_blorp.c > +++ b/src/intel/vulkan/anv_blorp.c > @@ -1110,6 +1110,39 @@ enum subpass_stage { > SUBPASS_STAGE_RESOLVE, > }; > > +/** > + * Goes through all the previous subpasses to the current subpass and > collects > + * information about the layers (views) of the given attachment that have > + * already been cleared by previous subpasses. The function return the > layers > + * (views), if any, that still need to be cleared when current subpass > starts. > + */ > +static uint32_t > +subpass_views_with_pending_clear(const struct anv_cmd_buffer *cmd_buffer, > + const uint32_t att_idx) > +{ > + const struct anv_cmd_state *cmd_state = &cmd_buffer->state; > + const struct anv_render_pass *pass = cmd_state->pass; > + const struct anv_subpass *subpass = cmd_state->subpass; > + > + if (!subpass->view_mask) > + return 0; > + > + uint32_t cleared_views = 0; > + uint32_t subpass_idx = 0; > + const struct anv_subpass *s = &pass->subpasses[subpass_idx]; > + while (s != subpass) { > + for (uint32_t i = 0; i < s->color_count; i++) { > + uint32_t idx = s->color_attachments[i].attachment; > + if (idx != att_idx) > + continue; > + cleared_views |= s->view_mask; > + } > + s = &pass->subpasses[++subpass_idx]; > + } > + > + return subpass->view_mask & (~cleared_views); > +} > + > static bool > subpass_needs_clear(const struct anv_cmd_buffer *cmd_buffer) > { > @@ -1142,7 +1175,6 @@ anv_cmd_buffer_clear_subpass(struct anv_cmd_buffer > *cmd_buffer) > const struct anv_cmd_state *cmd_state = &cmd_buffer->state; > const VkRect2D render_area = cmd_buffer->state.render_area; > > - > if (!subpass_needs_clear(cmd_buffer)) > return; > > @@ -1205,7 +1237,9 @@ anv_cmd_buffer_clear_subpass(struct anv_cmd_buffer > *cmd_buffer) > assert(image->n_planes == 1); > if (cmd_state->subpass->view_mask) { > uint32_t view_idx; > - for_each_bit(view_idx, cmd_state->subpass->view_mask) { > + uint32_t pending_clear_mask = > + subpass_views_with_pending_clear(cmd_buffer, a); > + for_each_bit(view_idx, pending_clear_mask) { > blorp_fast_clear(&batch, &surf, > iview->planes[0].isl.format, > iview->planes[0].isl.base_level, > view_idx, 1, > @@ -1227,7 +1261,9 @@ anv_cmd_buffer_clear_subpass(struct anv_cmd_buffer > *cmd_buffer) > assert(image->n_planes == 1); > if (cmd_state->subpass->view_mask) { > uint32_t view_idx; > - for_each_bit(view_idx, cmd_state->subpass->view_mask) { > + uint32_t pending_clear_mask = > + subpass_views_with_pending_clear(cmd_buffer, a); > + for_each_bit(view_idx, pending_clear_mask) { > blorp_clear(&batch, &surf, iview->planes[0].isl.format, > anv_swizzle_for_render(iview-> > planes[0].isl.swizzle), > iview->planes[0].isl.base_level, > @@ -1249,7 +1285,19 @@ anv_cmd_buffer_clear_subpass(struct anv_cmd_buffer > *cmd_buffer) > } > } > > - att_state->pending_clear_aspects = 0; > + if (!cmd_state->subpass->view_mask) { > + att_state->pending_clear_aspects = 0; > + } else { > + /* If multiview is enabled, then we are only done clearing when > the > + * last subpass that uses this attachment has been processed, > + */ > + uint32_t last_subpass_idx = > + cmd_state->pass->attachments[a].last_subpass_idx; > + const struct anv_subpass *last_subpass = > + &cmd_state->pass->subpasses[last_subpass_idx]; > + if (last_subpass == cmd_state->subpass) > + att_state->pending_clear_aspects = 0; > + } > } > > const uint32_t ds = cmd_state->subpass->depth_sten > cil_attachment.attachment; > -- > 2.11.0 > > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
